GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-99108: Build the hashlib HACL* code as a static library. (fix wasm builds) by gpshead · Pull Request #101917 · python/cpython

Conversation

@gpshead

@gpshead gpshead commented Feb 14, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

A followup to #101707 which broke some builds. This builds HACL* as a library in one place.

This doesn't solve all the problems, the wasm-wasi build is fixed by this. The wasm-enscripten build is not. Because it has a linker that can't cope with the same .a file being listed twice on its command line and decides to ignorantly process that as two inputs with duplicate symbols.

  • Issue: Replace built-in hashlib with verified implementations from HACL* #99108

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
gpshead added 2 commits February 14, 2023 21:37
@gpshead
Build HACL* SHA2 as a static library.
22b2267
@gpshead
direct .a reference (no real difference)
3c7e18e
@bedevere-bot bedevere-bot added the awaiting core review label Feb 14, 2023
@gpshead

gpshead commented Feb 14, 2023

Copy link
Copy Markdown
Member Author

!buildbot wasm
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Feb 14, 2023

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @gpshead for commit 3c7e18e 🤖

The command will test the builders whose names match following regular expression: wasm

The builders matched are:

  • wasm32-wasi PR
  • wasm32-emscripten node (dynamic linking) PR
  • wasm32-emscripten node (pthreads) PR
  • wasm32-emscripten browser (dynamic linking, no tests) PR
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead

gpshead commented Feb 14, 2023

Copy link
Copy Markdown
Member Author

The wasm-wasi bot using llvm-ar to build libpython3.12.a appears happy with this.
The wasm-enscripten ones that use emscripten/emar to build libpython3.12.a still does not.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead gpshead added the OS-wasi label Feb 14, 2023
@gpshead
use -l instead of the .a file again.
a5bb63e 
is this the problem? the .a file is showing up on the emcc linking command line
twice, is that really the source of the "duplicate" symbols?  is emcc that
dumb?
@gpshead

gpshead commented Feb 14, 2023

Copy link
Copy Markdown
Member Author

!buildbot wasm
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Feb 14, 2023

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @gpshead for commit a5bb63e 🤖

The command will test the builders whose names match following regular expression: wasm

The builders matched are:

  • wasm32-wasi PR
  • wasm32-emscripten node (dynamic linking) PR
  • wasm32-emscripten node (pthreads) PR
  • wasm32-emscripten browser (dynamic linking, no tests) PR
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead

gpshead commented Feb 14, 2023

Copy link
Copy Markdown
Member Author

Emscripten emcc which calls wasm-ld winds up with a duplicate of -LModules/_hacl -lHacl_Streaming_SHA2 on its command line or a direct Modules/_hacl/libHacl_Streaming_SHA2.a on its command line (the emcc script turns the first into the latter regardless). And whatever version of enscripten wasm-ld this is... is brain dead and does not deduplicate input files. Thus reporting "duplicate symbol" because the same .a is listed twice. 💩

Real linkers (like clang and thus llvm's ld.lld) do not have a problem with this.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead
gpshead requested a review from brettcannon February 14, 2023 22:42
@gpshead gpshead changed the title Build the hashlib HACL* code as a static library. gh:99108: Build the hashlib HACL* code as a static library. Feb 14, 2023
@gpshead gpshead changed the title gh:99108: Build the hashlib HACL* code as a static library. gh-99108: Build the hashlib HACL* code as a static library. Feb 14, 2023
@bedevere-bot bedevere-bot mentioned this pull request Feb 14, 2023
Replace built-in hashlib with verified implementations from HACL* #99108
Closed
@gpshead gpshead added the skip news label Feb 14, 2023
@gpshead
gpshead marked this pull request as ready for review February 14, 2023 22:44
@gpshead
Go back to the .a file, emcc undoes -L -l anyways.
6cd13a4
@gpshead

gpshead commented Feb 14, 2023

Copy link
Copy Markdown
Member Author

Realistically: We should probably just combine _sha256 and _sha512 into a single _sha2 module as another way to work around this. The .so files for each of those are carrying the code for both regardless given how we link the shared libraries.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead gpshead changed the title gh-99108: Build the hashlib HACL* code as a static library. gh-99108: Build the hashlib HACL* code as a static library. (fix wasm builds) Feb 14, 2023
@gpshead

gpshead commented Feb 14, 2023

Copy link
Copy Markdown
Member Author

Realistically: We should probably just combine _sha256 and _sha512 into a single _sha2 module as another way to work around this. The .so files for each of those are carrying the code for both regardless given how we link the shared libraries.

I'll do that as its own follow on PR. This one at least addresses 2 of the 4 bot failures and defines the static library as we want regardless.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead
gpshead merged commit d777790 into python:main Feb 14, 2023
@bedevere-bot bedevere-bot removed the awaiting core review label Feb 14, 2023
@gpshead
gpshead deleted the build/libhacl branch February 14, 2023 23:57
@gpshead gpshead mentioned this pull request Feb 15, 2023
gh-99108: Refactor _sha256 & _sha512 into _sha2. #101924
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brettcannon brettcannon Awaiting requested review from brettcannon

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

OS-wasi skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@gpshead @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.