wasmz.test_cubescript fails on the upstream backend #9527
Comments
kripken
changed the title
|
@sbc100 Sorry, I forgot to assign this to myself, but I've already started to look into it, and have a partial theory. Unless you've already made progress towards a fix, I can probably finish it? |
|
Awesome! Please take it |
|
It looks like this is a regression from the sbrk optimizations I did a while back. sbrk is now implemented entirely in wasm, which allows eval-ctors to do a lot more work - it thinks it can eval mallocs at compile time. However, we set the location of the brk in JS at runtime, which it doesn't see, so it sees a 0 there, and can mess things up... To fix this, we need to apply not just the sbrk ptr location (already done), but also its initial value, at compile time. I also think this is necessary regardless of eval-ctors, for standalone mode, where there is no JS. edit: this happens in cubescript because it has a bunch of global ctors that malloc and write a little data, and nothing else that prevents ctor evalling. edit: luckily this was easy to debug using wasm-reduce, it turned a 154,012 byte binary into 314 bytes, which I could read and fully understand. |
|
This is actually more serious than I thought. We allow JS to dynamicalliy allocate during startup, so ctor-eval should not make any assumptions about that value. Will disable that optimization (only affects |
Emscripten side of WebAssembly/binaryen#2366 (that must roll first), this tells binaryen to set the value in the wasm binary, which fixes standalone wasm module's sbrk() usage (as normally we set this is JS). This also disables eval-ctors for the wasm backend, see #9527 , which made an incorrect assumption about the sbrk initial location - JS may modify it during startup, so we can't assume it's constant. This affects only -Oz so it's probably not many users. After this PR we should be correct in both standalone and non-standalone modes, and later we can look into re-enabling eval-ctors.
We did not turn it on by default because of #9527, but a risk remained that a user could manually turn it on, which could lead to that bug or to WebAssembly/binaryen#2418 This skips running it, and shows a debug note. This seems better than erroring as that would mean fastcomp projects must change their build flags for upstream. Avoids WebAssembly/binaryen#2418 but does not fix it (we'll need to fix that too, to turn it back on eventually.)
|
This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant. |
|
Oh this is exciting.. now that we no longer dynamically allocate I think we can re-enable ctor evaluation! |
|
Hmm yes, it might be easier now, good point... |
Emscripten side of WebAssembly/binaryen#2366 (that must roll first), this tells binaryen to set the value in the wasm binary, which fixes standalone wasm module's sbrk() usage (as normally we set this is JS). This also disables eval-ctors for the wasm backend, see emscripten-core#9527 , which made an incorrect assumption about the sbrk initial location - JS may modify it during startup, so we can't assume it's constant. This affects only -Oz so it's probably not many users. After this PR we should be correct in both standalone and non-standalone modes, and later we can look into re-enabling eval-ctors.
We did not turn it on by default because of emscripten-core#9527, but a risk remained that a user could manually turn it on, which could lead to that bug or to WebAssembly/binaryen#2418 This skips running it, and shows a debug note. This seems better than erroring as that would mean fastcomp projects must change their build flags for upstream. Avoids WebAssembly/binaryen#2418 but does not fix it (we'll need to fix that too, to turn it back on eventually.)
|
This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant. |
None of our CI tests that combination, so this was missed. It might be a very old breakage, I didn't check how far back it is, I just noticed it as I ran some random tests.
Fails on
update: this is due to
eval_ctors.The text was updated successfully, but these errors were encountered: