-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
_emscripten_memcpy_js
runtime crash with MAIN/SIDE_MODULE + WASM_BIGINT
#22161
Comments
So as long as the main module and the side module agree on whether bulk memory is enabled it works? That makes sense to me. Are you saying you want to be able to have them disagree and things still work? |
I can't seem to reproduce this locally when I add
Furthermore I'm rather confused the undefined reference to |
The problem is that using the same flags for side and main module (WASM_BIGINT) seems to produce incompatible binaries unless I also specify the bulk memory option... or at least that was my diagnosis of Godot dlink builds being broken with that error after updating emscripten. Looking at your snippet, the only difference I can spot is that the test also sets the I've tried removing the env variable from the test, and indeed it started working once I remove that from the test... So the issue is maybe the interaction between I'm pretty much at a loss here, the only thing I know is that our single threaded dlink-enabled builds are broken since we update to newer emscripten (from 3.1.39) and added I've opened #22167 with the failing test, to see if we can reproduce in CI. |
So the test is indeed failing in CI with:
If I read the stack trace correctly, the side is calling the main module |
I will see if I can reproduce with BTW, are you using |
I was able to reproduce using |
In particular `EMCC_FORCE_STDLIBS=1` would previously place libc before libbulkmemory which don't work. libbulkmemory always needs to come first on the link line. Fixes: emscripten-core#22161
In particular `EMCC_FORCE_STDLIBS=1` would previously place libc before libbulkmemory which don't work. libbulkmemory always needs to come first on the link line. Fixes: emscripten-core#22161
In particular `EMCC_FORCE_STDLIBS=1` would previously place libc before libbulkmemory which don't work. libbulkmemory always needs to come first on the link line. Fixes: emscripten-core#22161
Yes, we are, we build Godot in the following way when supporting extensions (via dynamic linking):
Godot extensions (which are developed by third parties, and built as dynamic libraries), can then be successfully linked against the minimal runtime, even if the Godot code doesn't use a specific stdlib feature. In the past (the first builds dates back to 2020), without using Similarly, I couldn't build the whole godot code with By building the Godot code as I'm not sure there's a better way to allow extension developers to use the C++ standard library, without
Thank you so much 💙 , I tested the patch, and confirmed it fixes both my test, and Godot nothreads+dlink builds. |
The whole of the C/C++ standard library should be included when you build with |
I've tested, and indeed it's not necessary, thank you! I've opened godotengine/godot#93853 to remove the env variable from our builds. |
Version of emscripten/emsdk:
Failing command line in full:
I've created a small test showing the issue (patch: test.diff.txt ):
The gist is that:
The following works:
If we add the
-sWASM_BIGINT
flag instead, it will crash at runtime with:As far as I understood,
WASM_BIGINT
bump the minimum supported Safari version (and in fact, the-ssMIN_SAFARI_VERSION=150000
also breaks the build), resulting in an incorrectBULK_MEMORY
configuration (main module with bulk memory side module without).Indeed, adding
-mbulk-memory
to the (side module) command fixes theWASM_BIGINT
builds.This does not seem to happen when PTRHEADS is also enabled (since AFAU that will always force bulk memory on even on the side module).
The text was updated successfully, but these errors were encountered: