Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-130213: POC let blake2module.c see far less internals #130483

Closed
wants to merge 4 commits into from

Conversation

chris-eibl
Copy link
Contributor

@chris-eibl chris-eibl commented Feb 23, 2025

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 *.

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
@@ -40,7 +40,7 @@
#endif

#include <stdbool.h>

#undef HACL_CAN_COMPILE_SIMD128
Copy link
Contributor Author

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.

@@ -358,7 +356,7 @@ typedef struct {
Hacl_Hash_Blake2s_Simd128_state_t *blake2s_128_state;
#endif
#ifdef HACL_CAN_COMPILE_SIMD256
Hacl_Hash_Blake2b_Simd256_state_t *blake2b_256_state;
void *blake2b_256_state;
Copy link
Contributor Author

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 about Hacl_Hash_Blake2b_Simd256_state_t. The implementations just cast back to Hacl_Hash_Blake2b_Simd256_state_t.

@@ -100,7 +100,7 @@
<ItemDefinitionGroup>
<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>
Copy link
Contributor Author

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
Copy link
Contributor Author

chris-eibl commented Feb 23, 2025

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);
}

@msprotz
Copy link
Contributor

msprotz commented Feb 24, 2025

@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?

@msprotz
Copy link
Contributor

msprotz commented Feb 24, 2025

Also it would help me to understand how pressing this is, so that I can prioritize the upstream work accordingly. Thank you!

@chris-eibl
Copy link
Contributor Author

chris-eibl commented Feb 24, 2025

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 :)

@chris-eibl
Copy link
Contributor Author

@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?

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?

@msprotz
Copy link
Contributor

msprotz commented Feb 24, 2025

and it still has the #include "libintvector.h" in it?

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
Copy link
Contributor Author

Superseded by #130332.

@chris-eibl chris-eibl deleted the hacl_proof_of_concept branch March 7, 2025 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants