Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
gh-130213: POC let blake2module.c see far less internals#130483
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
and move all privates into internal/Hacl_Hash_Blake2b_Simd256.h
because for the POC I haven't yet done the similar changesfor SIMD128
from pythoncore.vcxproj. This is not needed and clang-clwarns about non-standard Microsoft includes when compiling
| #include<stdbool.h> | ||
| #undef HACL_CAN_COMPILE_SIMD128 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just temporarily, because I only moved the SIMD256 internals for the POC.
| #endif | ||
| #ifdefHACL_CAN_COMPILE_SIMD256 | ||
| Hacl_Hash_Blake2b_Simd256_state_t*blake2b_256_state; | ||
| void*blake2b_256_state; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMHO, this is sufficient and a good abstraction.blake2module.c does not need anything aboutHacl_Hash_Blake2b_Simd256_state_t. The implementations just cast back toHacl_Hash_Blake2b_Simd256_state_t.
| <ClCompile> | ||
| <AdditionalOptions>/Zm200 %(AdditionalOptions)</AdditionalOptions> | ||
| <AdditionalIncludeDirectories>$(PySourcePath)Modules\_hacl\include;$(PySourcePath)Modules\_hacl\internal;$(PySourcePath)Python;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> | ||
| <AdditionalIncludeDirectories>$(PySourcePath)Modules\_hacl\include;$(PySourcePath)Python;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is not needed for any*.c module we compile, since the internal files use e.g.
#include"internal/Hacl_Hash_Blake2b_Simd256.h"
Furthermore, clang-cl warns about a non-standard Microsoft include.
chris-eibl commentedFeb 23, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
But since this is generated code we must not touch, can we do that in a wrapper? E.g. uint8_tPy_Wrap_Hacl_Hash_Blake2b_Simd256_digest(void*state,uint8_t*dst) {Hacl_Hash_Blake2b_Simd256_state_t*s= (Hacl_Hash_Blake2b_Simd256_state_t*)state;returnHacl_Hash_Blake2b_Simd256_digest(s,dst);} |
@chris-eibl : it looks like your edits more or less correspond to what I've been doing upstream here:hacl-star/hacl-star#1025 correct? would you mind giving me some feedback on this PR? I think I need to hide a couple more types, but if I did that, then perhaps this would be what you need? |
Also it would help me to understand how pressing this is, so that I can prioritize the upstream work accordingly. Thank you! |
chris-eibl commentedFeb 24, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I think the clang-cl part is not pressing at all. It's quite niche. We could easily do#130447 as a temporary workaround. Or just do nothing and use versions >= 19.0.0 in the meantime. Regarding affected clang compilers on Linux / MacOS is above my paygrade, so that's for others to judge :) |
Looks promising! This will definitely solve the issues here. But if I am not mistaken,dist/gcc-compatible/Hacl_Hash_Blake2b_Simd256.h is a public header (it's the one that blake2module.c includes) and it still has the |
msprotz commentedFeb 24, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Yes there's still too much stuff in Hacl_Streaming_Types.h -- once I manage to move the types in there too to the forward declaration style, then that include can go. |
chris-eibl commentedMar 7, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Superseded by#130332 or#130960, where the latter would integrate the abstraction done inhacl-star/hacl-star#1025. |
Uh oh!
There was an error while loading.Please reload this page.
Just a POC, because this is touching generated code.
Limit the view of the
blake2module.cto the internals of the SIMD* implementations by usingvoid *.For me this now works for clang-cl 18.1.8, which does not "allow to see" the needed intrinsics without compiling for the proper architecture. I just temporarily use
#undef HACL_CAN_COMPILE_SIMD128, because I did the POC only for the SIMD256 case.And
test_hashlibis still green :)And the fast SIMD256 path is used, if the host where the binary is running supports it, without the need of SIMD flags when compiling
blake2module.c.I've checked in a debugging session during running
test_hashlib, that thevoid *"public" functions are still invoked, and that they work like before after casting back toHacl_Hash_Blake2b_Simd256_state_t *.