Fix renaming in FixInvokeFunctionNamesWalker #2513
Conversation
scripts/test/generate_lld_tests.py
Outdated
| '-z', '-stack-size=1048576', | ||
| obj_path, '-o', wasm_path, | ||
| '--allow-undefined', | ||
| '--strip-debug', |
There was a problem hiding this comment.
why strip debug info? I think that's why function names are lost in the new outputs, making it harder to read them (and to compare to before, to understand the patch)
There was a problem hiding this comment.
Because this bug occurred because we were inadvertently relying on debug names in this pass.
None of the stuff we do in wasm-emscripten-finalize should depend on the debug name section (I think).
There was a problem hiding this comment.
Hopefully they should become more readable again if we implement #2504... but that might lead to the same kind of sematic bugs I guess... e.g. using a functions name when you meant to refer to its import name
There was a problem hiding this comment.
I don't know wasm-emscripten-finalize well enough to know if it depends on function names from the name section or not. cc @jgravelle-google
There was a problem hiding this comment.
In emscripten -O2 and above we pass --strip-debug to lld which means the binary that wasm-emscripten-finalize sees in those cases doesn't contain any debug name section. This is why this particular bug only shows up at -O2.
What is more I think its important that continue to not depend on debug names so I think both part of this change are warranted.
sbc100
force-pushed
the
fix_longjump_renameing
branch
from
9b77469 to
217dc35
Compare
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Makes sense, I agree we should focus on testing the non-debug case.
However it would be nice to not lose coverage for the case where we do have debug names, so that we keep those names around. Maybe adding a little single test in unit/ for that?
I'm not sure what you mean by loosing coverage? Its seems to me we are increasing coverage by covering modules without debug info. Do you mean that the output is less readable and we are loosing readability of the test output? |
|
Yes, I mean that in the case that we do build with debug info in emscripten, it would be good if we verified the wasm-emscripten-finalize output has proper names. I guess that ends up tested in emscripten somehow though, e.g. in the stack trace tests. |
sbc100
force-pushed
the
fix_longjump_renameing
branch
from
217dc35 to
06ad51e
Compare
|
OK, I'll limited to use of |
This fixes emscripten-core/emscripten#9950. The issue only shows up when debug names are not present so most of the changes in CL come from disabling debug names in the lld tests. We want to make sure that wasm-emscripten-finalize runs fine without debug names so I think it makes most sense to test in this mode. The actual bugfix is in wasm-emscripten.cpp as part of the FixInvokeFunctionNamesWalker. The problem was the name of the function rather than is import name was being added to importRenames. This means that when debug names were present (and the two names were the same) we didn't see the bug.
sbc100
force-pushed
the
fix_longjump_renameing
branch
from
06ad51e to
9a645a9
Compare
|
@sbc100 This breaks emscripten tests on the roller. Disabled autoroll on binaryen for now. |
|
I'm going to revert this and try to re-land once fixed. |
In the previous iteration of this change we were not calling `renameFunctions` for each of the functions we removed. The problem manifested itself when we rename the imported function to `emscripten_longjmp_jmpbuf` to `emscripten_longjmp`. In this case the import of `emscripten_longjmp` already exists so we remove the import of `emscripten_longjmp_jmpbuf` but we were not correclty calling renameFunctions to handle the rename of all the uses. Add an additional test case to cover the failures that we saw on the emscripten tree.
In the previous iteration of this change we were not calling `renameFunctions` for each of the functions we removed. The problem manifested itself when we rename the imported function to `emscripten_longjmp_jmpbuf` to `emscripten_longjmp`. In this case the import of `emscripten_longjmp` already exists so we remove the import of `emscripten_longjmp_jmpbuf` but we were not correclty calling renameFunctions to handle the rename of all the uses. Add an additional test case to cover the failures that we saw on the emscripten tree.
* Reland "Fix renaming in FixInvokeFunctionNamesWalker (#2513)" In the previous iteration of this change we were not calling `renameFunctions` for each of the functions we removed. The problem manifested itself when we rename the imported function to `emscripten_longjmp_jmpbuf` to `emscripten_longjmp`. In this case the import of `emscripten_longjmp` already exists so we remove the import of `emscripten_longjmp_jmpbuf` but we were not correclty calling renameFunctions to handle the rename of all the uses. Add an additional test case to cover the failures that we saw on the emscripten tree.
This is a partial revert of #9750. The binaryen change that was designed to allow this to work was reverted: WebAssembly/binaryen#2513
This is a partial revert of #9750. The binaryen change that was designed to allow this to work was reverted: WebAssembly/binaryen#2513
This is a partial revert of #9750. The binaryen change that was designed to allow this to work was reverted: WebAssembly/binaryen#2513
2 participants
This fixes emscripten-core/emscripten#9950.
The issue only shows up when debug names are not present so most of
the changes in CL come from disabling debug names in the lld tests.
We want to make sure that wasm-emscripten-finalize runs fine without
debug names so I think it makes most sense to test in this mode.
The actual bugfix is in wasm-emscripten.cpp as part of the
FixInvokeFunctionNamesWalker. The problem was the name of the function
rather than is import name was being added to importRenames. This means
that when debug names were present (and the two names were the same)
we didn't see the bug.