FIX Fill in GOT entries when libs are loaded with allowUndefined by hoodmane · Pull Request #22053 · emscripten-core/emscripten · GitHub

Resolves #22052.

…fined: true` Resolves emscripten-core#22052

It looks like this code is very similar to existing code in both reportUndefinedSymbols and updateGOT. I wonder if we can factor this better?

Also, I think there is probably some reason we delay updated the GOT at GOTHandler time and instead do it later on. Perhaps we could instead modify the existing updateGOT function?

Sounds good I'll try factoring out the shared logic and moving the change to updateGot.

Is __cxa_throw effected because its a JS symbol?

The problem specifically seems to be that updateGOT fills in the GOT table for all wasm exports from the loaded library. So it misses JavaScript symbols exported from the main module. Perhaps it would fix it to add GOT entries for all of the JS-implemented library funcs in the main module?

How about if we rename reportUndefinedSymbols to handleUndefinedSymbols and have it take an reportErrors flags?

Well it seems to me that the only reason we need this GOT update behavior in reportUndefinedSymbols is because of JS symbols from the main module which are not handled by updateGOT. So they only get added to the GOT if first we have a library that needs them. Why not just stick all JS symbols in the GOT at startup and not worry about having something like handleUndefinedSymbols?

Oh sorry, yes, strong symbols should always win, although I'm not sure how important that edge case is.

But you think the case where a JS symbol is overridden by a native symbol is more important? Are the JS symbols automatically counted as weak?

No, not necessarily. I was just trying to imagine/remember the reason for factoring the code this way.

I do think we have good test coverage here, so I think its reasonably safe to try refactoring this code and if the tests pass it should be fine.

There is also a non-zero cost to adding JS functions to the wasm table.. see the addFunction helper.

Right, in the future where WebAssembly.Function is available it will be better. Is it possible we could just import and reexport all these JS functions from the main module so that they would all get added by updateGOT in the normal way?

@sbc100 I think ideally reportUndefinedSymbols() shouldn't adjust the GOT because the name makes it sound like it's for error detection/debugging. The fact that it can be necessary to keep things from breaking is unfortunate given the name.

@sbc100 I think ideally reportUndefinedSymbols() shouldn't adjust the GOT because the name makes it sound like it's for error detection/debugging. The fact that it can be necessary to keep things from breaking is unfortunate given the name.

I agree.. we should probably split the reportUndefinedSymbols logic

@sbc100 Could this be a regression from #21785? It seems like a regression from Pyodide 0.25 to 0.26 and we upgraded from Emscripten 3.1.52 to 3.1.58. In the changelog in that range, #21785 seems like the most likely culprit.

@sbc100 Could this be a regression from #21785? It seems like a regression from Pyodide 0.25 to 0.26 and we upgraded from Emscripten 3.1.52 to 3.1.58. In the changelog in that range, #21785 seems like the most likely culprit.

I assume that by "this" you are referring to #22052? Its hard to say right now since I don't fully understand the exact issue. However I will say that #21785 pertains the native symbols and which of them get exported on the Module, whereas #22052 appears to pertain to JS symbols, is that right?

It seems like a regression from Pyodide 0.25 to 0.26 and we upgraded from Emscripten 3.1.52 to 3.1.58

Is it possible to confirm that for sure by upgrading just emscripten, and keeping all other things constant? Even better could you bisect to find out exactly which emscripten change caused the regression?

Okay it reproduces on 3.1.57. So maybe not a regression, will check 3.1.52 next.

Does not reproduce in 3.1.52, so indeed a regression since then. Will bisect.

Regression was between 3.1.56 and 3.1.57.

The regression was #21638. Reverting this on top of the main branch has no conflicts and makes the problem go away.

Suggested in discussion on emscripten-core#22053

Did this PR get superseded or do we still want to work on landing it?

I will try to check in a couple days but I think it's still a problem and it would be nice to work on landing a fix.

hoodmane mentioned this pull request Jun 4, 2024

FIX Prevent segfaults in CLI runner pyodide/pyodide#4836

Merged

2 tasks

FIX Don't leave GOT entries null when libs are loaded with `allowUnde…

d4eff65

…fined: true` Resolves emscripten-core#22052

hoodmane force-pushed the got-null-allow-undefined branch from 14b7425 to d4eff65 Compare June 4, 2024 14:51

sbc100 reviewed Jun 4, 2024

View reviewed changes

hoodmane mentioned this pull request Jun 5, 2024

Emscripten calls null GOT entry when side module loaded with allowUndefined: true #22052

Open

hoodmane added a commit to hoodmane/emscripten that referenced this pull request Jun 21, 2024

NFC Factor out common code between updateGOT and reportUndefinedSymbols

2447011

Suggested in discussion on emscripten-core#22053

hoodmane mentioned this pull request Jun 21, 2024

NFC Factor out common code between updateGOT and reportUndefinedSymbols #22124

Open

hoodmane added a commit to hoodmane/emscripten that referenced this pull request Jun 21, 2024

NFC Factor out common code between updateGOT and reportUndefinedSymbols

e816e8c

Suggested in discussion on emscripten-core#22053

+                      rtn.value = addFunction(value, value.sig);
+                    } else if (typeof value == {{{ POINTER_JS_TYPE }}}) {
+                      rtn.value = value;
+                    }

FIX Fill in GOT entries when libs are loaded with allowUndefined by hoodmane · Pull Request #22053 · emscripten-core/emscripten · GitHub

Uh oh!

FIX Fill in GOT entries when libs are loaded with allowUndefined - #22053

FIX Fill in GOT entries when libs are loaded with allowUndefined#22053hoodmane wants to merge 1 commit intoemscripten-core:mainemscripten-core/emscripten:mainfrom hoodmane:got-null-allow-undefinedhoodmane/emscripten:got-null-allow-undefinedCopy head branch name to clipboard

Conversation

hoodmane commented Jun 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hoodmane commented Jun 5, 2024

Uh oh!

sbc100 commented Jun 5, 2024

Uh oh!

hoodmane commented Jun 8, 2024

Uh oh!

sbc100 commented Jun 8, 2024

Uh oh!

sbc100 commented Jun 8, 2024

Uh oh!

hoodmane commented Jun 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hoodmane commented Jun 9, 2024

Uh oh!

hoodmane commented Jun 9, 2024

Uh oh!

hoodmane commented Jun 21, 2024

Uh oh!

sbc100 commented Feb 8, 2025

Uh oh!

hoodmane commented Feb 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

FIX Fill in GOT entries when libs are loaded with allowUndefined#22053
hoodmane wants to merge 1 commit into
emscripten-core:mainfrom
hoodmane:got-null-allow-undefined

hoodmane commented Jun 9, 2024 •
edited
Loading