Remove assert for __get_temp_ret() and __set_temp_ret() after Binaryen JS FFI legalizer pass#26791
Remove assert for __get_temp_ret() and __set_temp_ret() after Binaryen JS FFI legalizer pass#26791juj wants to merge 1 commit intoemscripten-core:mainfrom
Conversation
juj
force-pushed
the
fix_wasm2js0_test_autodebug_wasm
branch
from
da647ad to
9fd8bab
Compare
|
Can you add wasm2js0.test_autodebug_wasm to the core3+extras list in We run all the tests in wasm2js1 mode in CI.. i wonder why opt level zero would mean these exports don't exist but they do exist at opt level 1 and above? I would imagine it would be other way around. |
| flags += ['--pass-arg=legalize-js-interface-exported-helpers'] | ||
| # Ensure that the legalizer pass does not erase debug info | ||
| if settings.DEBUG_LEVEL: | ||
| flags += ['-g'] |
There was a problem hiding this comment.
I think the run_binaryen_command wrapper is supposed to take are of adding this (or not) at the appropriate times.
There was a problem hiding this comment.
It is/was not happening at least for this FFI JS run pass. Adding -g manually here does help the output retain debug info.
…and __set_temp_ret() if they are unused, so do not emit assert() for their existence. Fixes emscripten-core#25549.
Check.
I don't think they exist in any wasm2js* level after the JS FFI legalization wasm-opt pass runs (they exist in the output before the legalization wasm-opt run, but not after). But wasm2js0 is the only build that has |
juj
force-pushed
the
fix_wasm2js0_test_autodebug_wasm
branch
from
9fd8bab to
d9eb90c
Compare
| # are created by binaryen will be missing. | ||
| continue | ||
| if settings.LEGALIZE_JS_FFI and sym in {'__get_temp_ret', '__set_temp_ret'}: | ||
| # The Binaryen legalizer pass will drop exports of __get_temp_ret() and __set_temp_ret() |
There was a problem hiding this comment.
We have this code elsewhere:
if settings.LEGALIZE_JS_FFI:
settings.REQUIRED_EXPORTS += ['__get_temp_ret', '__set_temp_ret']
Which means wasm-ld will always export them when LEGALIZE_JS_FFI.
If they are then removed at some point (for example by DCE) then that should be not different any other removed exports. i.e. the list of exports here should already take into account any DCE.
There was a problem hiding this comment.
then that should be not different any other removed exports
They still go to the list of exports to generate assert()s for, which is why the test case fails.
2 participants
Binaryen JS FFI legalizer pass seems to be dropping __get_temp_ret() and __set_temp_ret() if they are unused, so do not emit assert() for their existence. Fixes #25549.
I am not 100% sure about this, but this did fix the test
wasm2js0.test_autodebug_wasmto pass.