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-128627: Use __builtin_wasm_test_function_pointer_signature for Emscripten trampoline by hoodmane · Pull Request #137470 · python/cpython

Conversation

@hoodmane

@hoodmane hoodmane commented Aug 6, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Since llvm/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.

  • Issue: Emscripten: Use wasm-gc for call trampolines instead of JS type reflection #128627

Sorry, something went wrong.

Uh oh!

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

All reactions
@hoodmane
hoodmane requested review from corona10 and erlend-aasland as code owners August 6, 2025 15:35
@bedevere-app bedevere-app Bot added the awaiting review label Aug 6, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Aug 6, 2025
Emscripten: Use wasm-gc for call trampolines instead of JS type reflection #128627
Closed
@hoodmane hoodmane added the skip news label Aug 6, 2025
@hoodmane
pythongh-128627: Use __builtin_wasm_test_function_pointer_signature f…
0ffb76b
…or Emscripten trampolines

Since llvm/llvm-project#150201 was merged,
there is now a better way to do this. Requires Emscripten 4.0.12.
@hoodmane
hoodmane force-pushed the __builtin_wasm_test_function_pointer_signature branch from 6bbee1e to 0ffb76b Compare August 6, 2025 15:41
@hoodmane
hoodmane requested a review from freakboy3742 August 6, 2025 15:42
@hoodmane

hoodmane commented Aug 6, 2025

Copy link
Copy Markdown
Contributor Author

!buildbot emscripten

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot

bedevere-bot commented Aug 6, 2025

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @hoodmane for commit 0ffb76b 🤖

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
All reactions

Sorry, something went wrong.

Uh oh!

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

@hoodmane
Add forgotten file
04e0541
@hoodmane

hoodmane commented Aug 6, 2025

Copy link
Copy Markdown
Contributor Author

!buildbot emscripten

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot

bedevere-bot commented Aug 6, 2025

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @hoodmane for commit 04e0541 🤖

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
All reactions

Sorry, something went wrong.

Uh oh!

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

freakboy3742
freakboy3742 reviewed Aug 6, 2025
View reviewed changes
Comment thread Makefile.pre.in Resolved

Uh oh!

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

@freakboy3742

freakboy3742 commented Aug 6, 2025

Copy link
Copy Markdown
Contributor

There's three blockers to this at present:

  1. The check-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 from gh-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 (and on 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 it right now. And (1) and (2) will need a fix regardless.

hoodmane reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

freakboy3742
freakboy3742 requested changes Aug 6, 2025
View reviewed changes

@freakboy3742 freakboy3742 left a comment

Copy link
Copy Markdown
Contributor

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.

Marking as "needs changes" - see previous comment.

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-app bedevere-app Bot removed the awaiting review label Aug 6, 2025
@bedevere-app

bedevere-app Bot commented Aug 6, 2025

Copy link
Copy Markdown

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 phrase I 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.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-app bedevere-app Bot added the awaiting changes label Aug 6, 2025
@hoodmane

hoodmane commented Aug 7, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

hoodmane added 2 commits August 7, 2025 10:38
@hoodmane
Mark global as const
b633734
@hoodmane
Add prefix to globals
538e563
@hoodmane
hoodmane requested a review from AA-Turner as a code owner August 7, 2025 08:41
hoodmane added 2 commits August 7, 2025 10:46
@hoodmane
Add a comment
7ed9118
@hoodmane
Update comment
fff815c
@picnixz

picnixz commented Aug 7, 2025

Copy link
Copy Markdown
Member

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

hoodmane reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

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

@hoodmane

hoodmane commented Aug 7, 2025

Copy link
Copy Markdown
Contributor Author

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
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@hoodmane

hoodmane commented Aug 7, 2025

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@hoodmane

hoodmane commented Aug 7, 2025

Copy link
Copy Markdown
Contributor Author

I can't reproduce the NotImplementedError 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)])
All reactions

Sorry, something went wrong.

Uh oh!

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

@freakboy3742

freakboy3742 commented Aug 8, 2025

Copy link
Copy Markdown
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... :-)

All reactions

Sorry, something went wrong.

Uh oh!

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

@hoodmane
hoodmane requested a review from ZeroIntensity as a code owner September 17, 2025 08:40
hoodmane added 5 commits September 17, 2025 10:52
@hoodmane
More comments
3379e1c
@hoodmane
Cleanup
7d5a013
@hoodmane
Reduce diff
b80911b
@hoodmane
Reduce diff
b4f07d9
@hoodmane
Cleanup
51af9e1
freakboy3742
freakboy3742 requested changes Sep 17, 2025
View reviewed changes

@freakboy3742 freakboy3742 left a comment

Copy link
Copy Markdown
Contributor

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Makefile.pre.in Resolved

Uh oh!

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

@hoodmane
typedef => #define
a1753d6
freakboy3742
freakboy3742 reviewed Sep 17, 2025
View reviewed changes
Comment thread Python/emscripten_trampoline_inner.c Outdated
// 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.

#define PyObject void;

@freakboy3742 freakboy3742 Sep 17, 2025

Copy link
Copy Markdown
Contributor

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions
@freakboy3742

freakboy3742 commented Sep 17, 2025

Copy link
Copy Markdown
Contributor

!buildbot emscripten

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot

bedevere-bot commented Sep 17, 2025

Copy link
Copy Markdown

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

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
All reactions

Sorry, something went wrong.

Uh oh!

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

@hoodmane
Fix again
0ca9e4c
@hoodmane

hoodmane commented Sep 17, 2025

Copy link
Copy Markdown
Contributor Author

!buildbot emscripten

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot

bedevere-bot commented Sep 17, 2025

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @hoodmane for commit 0ca9e4c 🤖

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
All reactions

Sorry, something went wrong.

Uh oh!

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

@hoodmane
Add comment
4002452
freakboy3742
freakboy3742 approved these changes Sep 17, 2025
View reviewed changes

@freakboy3742 freakboy3742 left a comment

Copy link
Copy Markdown
Contributor

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.

LGTM!

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting changes labels Sep 17, 2025
@freakboy3742
freakboy3742 merged commit 2629ee4 into python:main Sep 17, 2025
@bedevere-app bedevere-app Bot removed the awaiting merge label Sep 17, 2025
@freakboy3742 freakboy3742 added awaiting merge needs backport to 3.14 bugs and security fixes labels Sep 17, 2025
@miss-islington-app

miss-islington-app Bot commented Sep 17, 2025

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 17, 2025
@hoodmane @miss-islington
pythongh-128627: Use __builtin_wasm_test_function_pointer_signature f…
1118501
…or Emscripten trampoline (pythonGH-137470)

With llvm/llvm-project#150201 being merged, there is
now a better way to generate the Emscripten trampoline, instead of including
hand-generated binary WASM content. Requires Emscripten 4.0.12.
(cherry picked from commit 2629ee4)

Co-authored-by: Hood Chatham <[email protected]>
@bedevere-app

bedevere-app Bot commented Sep 17, 2025

Copy link
Copy Markdown

GH-139039 is a backport of this pull request to the 3.14 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-app bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label Sep 17, 2025
hugovk pushed a commit that referenced this pull request Sep 17, 2025
@miss-islington @hoodmane
[3.14] gh-128627: Use __builtin_wasm_test_function_pointer_signature …
0e4e608
…for Emscripten trampoline (GH-137470) (#139039)

gh-128627: Use __builtin_wasm_test_function_pointer_signature for Emscripten trampoline (GH-137470)

With llvm/llvm-project#150201 being merged, there is
now a better way to generate the Emscripten trampoline, instead of including
hand-generated binary WASM content. Requires Emscripten 4.0.12.
(cherry picked from commit 2629ee4)

Co-authored-by: Hood Chatham <[email protected]>
This was referenced Feb 20, 2026
Emscripten: Trampoline doesn't work with emcc 4.0.19 due to table/memory visibility #145037
Closed
gh-145037: Fix Emscripten trampoline with emcc >= 4.0.19 #145038
Merged
Fix wasm-gc call trampoline when RELOCATABLE isn't passed pyodide/pyodide#6115
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@freakboy3742 freakboy3742 freakboy3742 approved these changes
@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland is a code owner
@corona10 corona10 Awaiting requested review from corona10 corona10 is a code owner
@AA-Turner AA-Turner Awaiting requested review from AA-Turner AA-Turner is a code owner
@emmatyping emmatyping Awaiting requested review from emmatyping emmatyping is a code owner
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner
@ZeroIntensity ZeroIntensity Awaiting requested review from ZeroIntensity ZeroIntensity is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

awaiting merge 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.

5 participants

@hoodmane @bedevere-bot @freakboy3742 @picnixz @hugovk
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 01402520 — 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.