Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-128627: Use __builtin_wasm_test_function_pointer_signature for Emscripten trampoline#137470

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

Conversation

@hoodmane
Copy link
Contributor

@hoodmanehoodmane commentedAug 6, 2025
edited
Loading

Sincellvm/llvm-project#150201 was merged, there is now a better way to do this. Requires Emscripten 4.0.12. This lets us get rid of all the hand encoded webassembly.

@freakboy3742 this is blocked on updating the buildbot to use Emscripten 4.0.12, not sure what that entails.

…or Emscripten trampolinesSincellvm/llvm-project#150201 was merged,there is now a better way to do this. Requires Emscripten 4.0.12.
@hoodmanehoodmaneforce-pushed the__builtin_wasm_test_function_pointer_signature branch from6bbee1e to0ffb76bCompareAugust 6, 2025 15:41
@hoodmane
Copy link
ContributorAuthor

!buildbot emscripten

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@hoodmane for commit0ffb76b 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F137470%2Fmerge

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

The builders matched are:

  • WASM Emscripten PR

@hoodmane
Copy link
ContributorAuthor

!buildbot emscripten

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@hoodmane for commit04e0541 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F137470%2Fmerge

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

The builders matched are:

  • WASM Emscripten PR

@freakboy3742
Copy link
Contributor

There's three blockers to this at present:

  1. Thecheck-c-globals analyser check that is failing CI on this PR
  2. The test suite CI failure that is also visible on main, because of the change fromgh-131876: extract_hashlib helpers into a separate directory #136995.gh-131876: extract_hashlib helpers into a separate directory #137319 looks like a possible fix; however, it looks like it's on hold pending some other changes.
  3. The refactoring of the build script that we discussed at EuroPython (andon the buildmaster-config PR) to encode the Emscripten version as part of the build script, and use an argument to point at a local cache directory.

The alternative to (3) would be to violate the "RC1 is where we lock in the Emscripten version" rule, and bump to 4.0.12. That isn't an approach we could take in the general case, but given this is the first "back to Tier 3" release, and there's no impact on downstream packagers (since nobody can publish to PyPI, so nobody will be relying on the 4.0.11 ABI guarantee), I could probably be convinced to do this - but I'd like@hugovk to sign off on the plan as release manager.

Of course (3) is going to be needed in the fullness of time - it's just a question of whether we need to do itright now. And (1) and (2) will need a fix regardless.

hoodmane reacted with thumbs up emoji

Copy link
Contributor

@freakboy3742freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Marking as "needs changes" - see previous comment.

@bedevere-app
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@hoodmane
Copy link
ContributorAuthor

hoodmane commentedAug 7, 2025
edited
Loading

This PR isn't urgent so I think it would be fine to block it on making an Emscripten version manager. But it would also be fine to bump the Emscripten version for 3.14 imo.

@picnixz
Copy link
Member

I reverted the hashlib refactorization by the way, so failures on main shouldn't be because of this.

hoodmane reacted with heart emoji

@hoodmane
Copy link
ContributorAuthor

Thanks@picnixz. Failure on main is a runtime failure in a test about calendars and locales, not the linker error.

picnixz reacted with thumbs up emoji

@hoodmane
Copy link
ContributorAuthor

Hmm the C parser doesn't work with out of tree builds because it passes a bunch of invalid paths like-I/home/rchatham/cpython/*./Include cc@ericsnowcurrently
https://github.com/python/cpython/blob/main/Tools/c-analyzer/cpython/_parser.py?plain=1#L120

@hoodmane
Copy link
ContributorAuthor

I can't reproduce theNotImplementedError locally:

NotImplementedError: (Struct(file=FileInfo(filename='/home/runner/work/cpython/cpython/Include/pystate.h',lno=62),name='_stack_chunk',data=[Member(name='previous',vartype=VarType(typequal=None,typespec='struct _stack_chunk',abstract='*'),size=None),Member(name='size',vartype=VarType(typequal=None,typespec='size_t',abstract=''),size=None),Member(name='top',vartype=VarType(typequal=None,typespec='size_t',abstract=''),size=None),Member(name='data',vartype=VarType(typequal=None,typespec='PyObject',abstract='* [1]'),size=None)],parent=None), [TypeDef(file=FileInfo(filename='/home/runner/work/cpython/cpython/Include/pytypedefs.h',lno=18),name='PyObject',data=VarType(typequal=None,typespec='struct _object',abstract=''),parent=None),TypeDef(file=FileInfo(filename='/home/runner/work/cpython/cpython/Python/emscripten_trampoline_inner.c',lno=1),name='PyObject',data=VarType(typequal=None,typespec='void',abstract=''),parent=None)])

@freakboy3742
Copy link
Contributor

I reverted the hashlib refactorization by the way, so failures on main shouldn't be because of this.

My apologies - I missed that the cause of the CI failures on main had changed. Thanks for the revert - now to work out what's going on with Calendars... :-)

Copy link
Contributor

@freakboy3742freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

With the exception of the generated files failure, this looks good to me.

// support wasm-gc yet. If the JS runtime does not support wasm-gc (or has buggy
// support like iOS), we will use the JS trampoline fallback.

#definePyObject void;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Would be worth adding a short comment about why this is needed.

@freakboy3742
Copy link
Contributor

!buildbot emscripten

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@freakboy3742 for commita1753d6 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F137470%2Fmerge

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

The builders matched are:

  • WASM Emscripten PR

@hoodmane
Copy link
ContributorAuthor

!buildbot emscripten

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@hoodmane for commit0ca9e4c 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F137470%2Fmerge

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

The builders matched are:

  • WASM Emscripten PR

Copy link
Contributor

@freakboy3742freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM!

@freakboy3742freakboy3742 merged commit2629ee4 intopython:mainSep 17, 2025
46 of 47 checks passed
@miss-islington-app
Copy link

Thanks@hoodmane for the PR, and@freakboy3742 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestSep 17, 2025
…or Emscripten trampoline (pythonGH-137470)Withllvm/llvm-project#150201 being merged, there isnow a better way to generate the Emscripten trampoline, instead of includinghand-generated binary WASM content. Requires Emscripten 4.0.12.(cherry picked from commit2629ee4)Co-authored-by: Hood Chatham <roberthoodchatham@gmail.com>
@bedevere-app
Copy link

GH-139039 is a backport of this pull request to the3.14 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.14bugs and security fixes labelSep 17, 2025
hugovk pushed a commit that referenced this pull requestSep 17, 2025
…for Emscripten trampoline (GH-137470) (#139039)gh-128627: Use __builtin_wasm_test_function_pointer_signature for Emscripten trampoline (GH-137470)Withllvm/llvm-project#150201 being merged, there isnow a better way to generate the Emscripten trampoline, instead of includinghand-generated binary WASM content. Requires Emscripten 4.0.12.(cherry picked from commit2629ee4)Co-authored-by: Hood Chatham <roberthoodchatham@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@freakboy3742freakboy3742freakboy3742 approved these changes

@erlend-aaslanderlend-aaslandAwaiting requested review from erlend-aaslanderlend-aasland is a code owner

@corona10corona10Awaiting requested review from corona10corona10 is a code owner

@AA-TurnerAA-TurnerAwaiting requested review from AA-TurnerAA-Turner is a code owner

@emmatypingemmatypingAwaiting requested review from emmatypingemmatyping is a code owner

@ericsnowcurrentlyericsnowcurrentlyAwaiting requested review from ericsnowcurrentlyericsnowcurrently is a code owner

@ZeroIntensityZeroIntensityAwaiting requested review from ZeroIntensityZeroIntensity is a code owner

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@hoodmane@bedevere-bot@freakboy3742@picnixz@hugovk

[8]ページ先頭

©2009-2025 Movatter.jp