gh-130213: POC let blake2module.c see far less internals - #130483
gh-130213: POC let blake2module.c see far less internals#130483chris-eibl wants to merge 4 commits into
Conversation
Sorry, something went wrong.
and move all privates into internal/Hacl_Hash_Blake2b_Simd256.h
because for the POC I haven't yet done the similar changes for SIMD128
from pythoncore.vcxproj. This is not needed and clang-cl warns about non-standard Microsoft includes when compiling
|
|
||
| #include <stdbool.h> | ||
|
|
||
| #undef HACL_CAN_COMPILE_SIMD128 |
There was a problem hiding this comment.
Just temporarily, because I only moved the SIMD256 internals for the POC.
Sorry, something went wrong.
| #endif | ||
| #ifdef HACL_CAN_COMPILE_SIMD256 | ||
| Hacl_Hash_Blake2b_Simd256_state_t *blake2b_256_state; | ||
| void *blake2b_256_state; |
There was a problem hiding this comment.
IMHO, this is sufficient and a good abstraction. blake2module.c does not need anything about Hacl_Hash_Blake2b_Simd256_state_t. The implementations just cast back to Hacl_Hash_Blake2b_Simd256_state_t.
Sorry, something went wrong.
| <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.
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.
Sorry, something went wrong.
|
But since this is generated code we must not touch, can we do that in a wrapper? E.g. py_hash_wrap_simd256.c containing uint8_t Py_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;
return Hacl_Hash_Blake2b_Simd256_digest(s, dst);
} |
Sorry, something went wrong.
|
@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? |
Sorry, something went wrong.
|
Also it would help me to understand how pressing this is, so that I can prioritize the upstream work accordingly. Thank you! |
Sorry, something went wrong.
|
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 :) |
Sorry, something went wrong.
Looks promising! # Note: we would like to maintain the invariant (as of Feb 2025) that we NEVER include libintvector.h from a # public header. See https://github.com/python/cpython/issues/130213 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 #include "libintvector.h" in it? |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
Superseded by #130332 or #130960, where the latter would integrate the abstraction done in hacl-star/hacl-star#1025. |
Sorry, something went wrong.
Just a POC, because this is touching generated code.
Limit the view of the blake2module.c to the internals of the SIMD* implementations by using void *.
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_hashlib is 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 the void * "public" functions are still invoked, and that they work like before after casting back to Hacl_Hash_Blake2b_Simd256_state_t *.