-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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-99108: Import SHA2-384/512 from HACL* #101707
Conversation
(I pulled the wrong revision of HACL*, hence the build breakage -- I'll fix this shortly.) |
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.
Build system changes look good to me.
|
|
|
|
Thanks for merging, @gpshead. It looks like this broke the wasm build because there are now duplicate symbols. If I had to guess, I would say that the use-case where two C files in Modules/ depend on the same C file did not show up before in the build. Would it be enough to remove one of the two references to Hacl_Streaming_SHA2.c in Modules/Setup.stdlib.in? |
I doubt just removing one reference is enough? Looking at things I suspect what we want is for the _hacl/ stuff to be built into a single .a library and the sha256module.c and sha512module.c to both link against that. so that they pull only the symbols they use in, instead of what appears to be happening today where both extension module libraries wind up with all of the symbols from the This should require some I'm untangling this. |
This builds HACL* as a library in one place. A followup to #101707 which broke some WASM builds. This fixes 2/4 of them, but the enscripten toolchain in the others don't deduplicate linker arguments and error out. A follow-on PR will address those.
That fixes 2/4 of the build bot failures. the remaining wasm32-enscripten dynamic failures are because that uses --whole-archive linking so the duplicates (duplicate as in the .a file is listed twice on the wasm-ld command line at this point) cause trouble. I'm going to follow up with a refactoring PR that merges _sha256 and _sha512 into a single _sha2 module. Those two don't need to be separate modules, that is a historical oddity. |
Followup on #99108, and almost identical to #99109