Reland "Fix renaming in FixInvokeFunctionNamesWalker (#2513)" #2542
Conversation
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.
|
Please check if binaryen_js build runs OK before landing |
Is that not part of the CI? I see travis-emcc-tests.sh is run by travis, is that not good enough? |
|
Not sure why, but I think the CI tests with fastcomp? The previous version of this PR broke binaryen.js build with the wasm backend. |
|
I think maybe its because the travis-emcc-tests.sh uses used the emsdk docker image to do the build, and that will end up using binaryen docker, which won't include any of the change in the PR. I'll give it a go building locally. |
|
Verified both the failure and the fix! |
|
Any progress on this.? |
|
I'm going to revert once again until I can debug this. |
|
Is anyone looking into this. Is it possible to know the estimated time for it's fix.? |
|
@rohitsaini1995 I think this was already reverted in #2576. |
|
@aheejin Actually I am asking about fix of this emscripten-core/emscripten#9950 which is dependent on this binaryen fix , I think.. #2576 is the revert commit , not the fix commit. |
In the previous iteration of this change we were not calling
renameFunctionsfor each of the functions we removed.The problem manifested itself when we rename the imported function to
emscripten_longjmp_jmpbuftoemscripten_longjmp. In this case theimport of
emscripten_longjmpalready exists so we remove the import ofemscripten_longjmp_jmpbufbut we were not correclty callingrenameFunctions to handle the rename of all the uses.
Add an additional test case to cover the failures that we saw on the
emscripten tree.