Skip to content

Consistent detection of invoke_ functions in PostEmscripten.cpp #2619

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 1 commit into from
Jan 24, 2020

Conversation

sbc100
Copy link
Member

@sbc100 sbc100 commented Jan 23, 2020

We should be looking at the import name when determining if a function
is an invoke function.

This is a precursor to re-landing the fix for
emscripten-core/emscripten#9950.

We should be looking at the import name when determining if a function
is an invoke function.

This is a precursor to re-landing the fix for
emscripten-core/emscripten#9950.
@sbc100 sbc100 requested a review from kripken January 23, 2020 22:55
@kripken
Copy link
Member

kripken commented Jan 23, 2020

Would be good to have a test but if the emcc side will have one that's good enough probably.

@sbc100 sbc100 merged commit b880950 into master Jan 24, 2020
@sbc100 sbc100 deleted the invoke_detection branch January 24, 2020 01:44
@kripken
Copy link
Member

kripken commented Jan 24, 2020

Looks like this broke the binaryen roll. Just wasm3.test_exceptions_white_list_2.

kripken added a commit that referenced this pull request Jan 24, 2020
We ignored them, which is a bad default, as typically they imply
we can call anything in the table (and the table might change).
Instead, notice indirect calls during traversal, and force the user
to decide whether to ignore them or not.

This was only an issue in PostEmscripten because the other
user, Asyncify, already had indirect call analysis because it
needed it for other things.

Fixes a bug uncovered by #2619 and fixes the current binaryen
roll.
sbc100 added a commit that referenced this pull request Jan 24, 2020
This reverts commit 132daae.

This change is the same as before but the fix in #2619 should now make it safe.
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.

2 participants