search

emscripten-core / emscripten

Emscripten: An LLVM-to-WebAssembly Compiler
Other
25.67k stars 3.29k forks source link

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

Open hoodmane opened 4 months ago

hoodmane commented 4 months ago

Resolves #22052.

hoodmane commented 3 months ago

@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 commented 3 months ago

@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

hoodmane commented 3 months ago

@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 commented 3 months ago

@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?

sbc100 commented 3 months ago

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?

hoodmane commented 3 months ago

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

hoodmane commented 3 months ago

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

hoodmane commented 3 months ago

Regression was between 3.1.56 and 3.1.57.

hoodmane commented 3 months ago

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