This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Forbid use of EM_ASM with setjmp/longjmp
ClosedPublic

Authored by quantum on Aug 16 2019, 10:54 AM.

Details

Summary

We tried to support EM_ASM with setjmp/longjmp in binaryen. But with dynamic
linking thrown into the mix, the code is no longer understandable and cannot
be maintained. We also discovered more bugs in the EM_ASM handling code.

To ensure maintainability and correctness of the binaryen code, EM_ASM will
no longer be supported with setjmp/longjmp. This is probably fine since the
support was added recently and haven't be published.

Diff Detail

Repository
rL LLVM

Event Timeline

quantum created this revision.Aug 16 2019, 10:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2019, 10:54 AM
kripken accepted this revision.Aug 16 2019, 10:59 AM

lgtm module the error message, let's find the best phrasing there.

llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
1004 ↗(On Diff #215635)

How about "alongside" instead of "with", to hint that the issue is using both in the same function?

And/or maybe replace the final "." with ", or refactor the EM_ASM to another function"?

This revision is now accepted and ready to land.Aug 16 2019, 10:59 AM
tlively added inline comments.Aug 16 2019, 10:59 AM
llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
550 ↗(On Diff #215635)

Where does this list of functions come from? How do we know it's exhaustive?

554 ↗(On Diff #215635)

Maybe use std::any_of over an array of the function names as plain strings to reduce repetition?

quantum updated this revision to Diff 215640.Aug 16 2019, 11:08 AM
quantum marked 6 inline comments as done.

Address review comments.

llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
550 ↗(On Diff #215635)

Stated that the list is exhaustive and link the header where these functions came from.

554 ↗(On Diff #215635)

This is the style used in this file, so we are doing it for consistency.

1004 ↗(On Diff #215635)

Done.

tlively accepted this revision.Aug 16 2019, 11:18 AM
This revision was automatically updated to reflect the committed changes.
sbc100 added inline comments.Aug 16 2019, 11:32 AM
llvm/trunk/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
555

Why not just compare names here?

quantum marked 2 inline comments as done.Aug 16 2019, 12:30 PM
quantum added inline comments.
llvm/trunk/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
555

For consistency with WebAssemblyLowerEmscriptenEHSjLj::canLongjmp.