FIX Fill in GOT entries when libs are loaded with allowUndefined #22053
Conversation
…fined: true` Resolves emscripten-core#22052
hoodmane
force-pushed
the
got-null-allow-undefined
branch
from
14b7425 to
d4eff65
Compare
| rtn.value = addFunction(value, value.sig); | ||
| } else if (typeof value == {{{ POINTER_JS_TYPE }}}) { | ||
| rtn.value = value; | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Sounds good I'll try factoring out the shared logic and moving the change to updateGot.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
How about if we rename reportUndefinedSymbols to handleUndefinedSymbols and have it take an reportErrors flags?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Oh sorry, yes, strong symbols should always win, although I'm not sure how important that edge case is.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 was a problem hiding this comment.
There is also a non-zero cost to adding JS functions to the wasm table.. see the addFunction helper.
There was a problem hiding this comment.
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 |
I agree.. we should probably split the |
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 |
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
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. |
Resolves #22052.