Skip to content

Reland "Fix renaming in FixInvokeFunctionNamesWalker (#2513)" #2542

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

Merged
merged 3 commits into from
Dec 21, 2019
Merged

Conversation

sbc100
Copy link
Member

@sbc100 sbc100 commented Dec 19, 2019

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.
@aheejin
Copy link
Member

aheejin commented Dec 20, 2019

Please check if binaryen_js build runs OK before landing

@sbc100
Copy link
Member Author

sbc100 commented Dec 20, 2019

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?

@aheejin
Copy link
Member

aheejin commented Dec 20, 2019

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.

@sbc100
Copy link
Member Author

sbc100 commented Dec 20, 2019

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.

@sbc100
Copy link
Member Author

sbc100 commented Dec 21, 2019

Verified both the failure and the fix!

@sbc100 sbc100 merged commit f62e171 into master Dec 21, 2019
@sbc100 sbc100 deleted the reland branch December 21, 2019 00:02
@kripken
Copy link
Member

kripken commented Dec 21, 2019

@rohitsaini1995
Copy link

Any progress on this.?

@sbc100
Copy link
Member Author

sbc100 commented Jan 7, 2020

I'm going to revert once again until I can debug this.

@rohitsaini1995
Copy link

Is anyone looking into this. Is it possible to know the estimated time for it's fix.?

@aheejin
Copy link
Member

aheejin commented Jan 13, 2020

@rohitsaini1995 I think this was already reverted in #2576.

@rohitsaini1995
Copy link

rohitsaini1995 commented Jan 14, 2020

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants