Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible regression in pthreads w/loadDynamicLibrary (3.1.40 through HEAD, last working 3.1.39) #20507

Closed
bobziuchkovski opened this issue Oct 21, 2023 · 2 comments · Fixed by #19496

Comments

@bobziuchkovski
Copy link

bobziuchkovski commented Oct 21, 2023

Summary

It looks like there might be pthread+loadDynamicLibrary regression in emscripten 3.1.40 through current HEAD caused by the changes in #19390. It manifests as onmessage() captured an uncaught exception: RangeError: WebAssembly.Table.get(): invalid index [index] into funcref table of size [size] when using loadDynamicLibrary to load a side module and then attempting to invoke a function in the side module from a worker pthread where the worker is provided the shared handle to the side module functions.

Versions info

I tested official emscripten releases up to 3.1.39 (last working) and from 3.1.40-3.1.47 (all exhibiting the above error).

I then bisected the emscripten-releases and discovered:

Reproducing the error

Unfortunately I've been unable to put together a minimal reproducer for the issue, so the reproduction code is quite complex: it's the full Godot engine running a minimal project export. I've been looking into this as part of debugging godotengine/godot#82865.

That said, I've compiled the same Godot 4.2beta2 release against the three different emscripten releases mentioned above and posted the resulting web export to itch.io for each, along with a console log capture:

Compiler/Linker flags

For all of these tests, I compiled godot 4.2beta2 with: scons platform=web target=template_debug production=no 'LINKFLAGS=-sDYLINK_DEBUG=1 -sPTHREADS_DEBUG=1 -sASSERTIONS=1' verbose=yes dlink_enabled=yes

This translates to roughly the following compiler/linker flags:

  • main module (test.js) in the itch.io project exports:
    • compile: em++ -std=gnu++17 -sUSE_PTHREADS=1 -Os -fno-exceptions -sMAIN_MODULE=1
    • link: em++ -o test.js -sMAIN_MODULE=1 -sEXPORT_ALL=1 -sWARN_ON_UNDEFINED_SYMBOLS=0 -sDYLINK_DEBUG=1 -sPTHREADS_DEBUG=1 -sASSERTIONS=1 --profiling-funcs -sINITIAL_MEMORY=32MB -sUSE_WEBGL2=1 -sOFFSCREEN_FRAMEBUFFER=1 -sUSE_PTHREADS=1 -sPTHREAD_POOL_SIZE=8 -sWASM_MEM_MAX=2048MB -sENVIRONMENT=web,worker -sMODULARIZE=1 -sEXPORT_NAME='Godot' -sALLOW_MEMORY_GROWTH=1 -sINVOKE_RUN=0 -sEXPORTED_RUNTIME_METHODS=['callMain','cwrap'] -sEXIT_RUNTIME=1 -sGL_WORKAROUND_SAFARI_GETCONTEXT_BUG=0 -s [--js-library <godot js libs>...] -lidbfs.js
  • side module (test.side.wasm) in the itch.io project exports:
    • compile C: emcc -std=gnu11 -sUSE_PTHREADS=1 -sSIDE_MODULE=2 -fvisibility=hidden -Os -fno-exceptions
    • compile C++: em++ -std=gnu++17 -sUSE_PTHREADS=1 -sSIDE_MODULE=2 -fvisibility=hidden -Os -fno-exceptions
    • link: em++ -o test.side.wasm -sDYLINK_DEBUG=1 -sPTHREADS_DEBUG=1 -sASSERTIONS=1 --profiling-funcs -sINITIAL_MEMORY=32MB -sUSE_WEBGL2=1 -sOFFSCREEN_FRAMEBUFFER=1 -sUSE_PTHREADS=1 -sPTHREAD_POOL_SIZE=8 -sWASM_MEM_MAX=2048MB -sSIDE_MODULE=2 -fvisibility=hidden -sENVIRONMENT=web,worker -sMODULARIZE=1 -sEXPORT_NAME='Godot' -sALLOW_MEMORY_GROWTH=1 -sINVOKE_RUN=0 -sEXPORTED_RUNTIME_METHODS=['callMain','cwrap'] -sEXIT_RUNTIME=1 -sGL_WORKAROUND_SAFARI_GETCONTEXT_BUG=0 -s <godot libs>

Other

I'm happy to rebuild/re-export with any combination of flags that would be helpful for diagnosis. I chose -sDYLINK_DEBUG=1 -sPTHREADS_DEBUG=1 -sASSERTIONS=1 thinking this would gather the right level of detail in the console logs, but there might be other flags I missed.

Godot has a few internal overrides it defines here (4.2beta4 tree). I'm not sure if there's some interplay with the fact that test.side.wasm is defined in the dynamicLibraries list statically in there? The rest of the js-specific code is in the parent dir of that file.

@kleisauke
Copy link
Collaborator

I think PR #19496 would fix this.

@bobziuchkovski
Copy link
Author

I think PR #19496 would fix this.

Thanks! I can confirm rebuilding with your changes from #19496 applied to emscripten-releases 04351ae156616a0aaab2da324b8f791fda86e6ee (HEAD from earlier today) fixes the issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants