This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] EmscriptenEHSjLj: Don't abort if __THREW__ is defined
ClosedPublic

Authored by sbc100 on Apr 3 2019, 2:04 PM.

Details

Summary

This allows THREW to be defined in the current module, although
it is still required to be a GlobalVariable.

In emscripten we want to be able to compile the source
code that defined this symbol.

Previously we were avoid this by not running this pass when building
that compiler-rt library, but I have change out to build it using the
normal compiler path: https://github.com/emscripten-core/emscripten/pull/8391

Diff Detail

Repository
rL LLVM

Event Timeline

sbc100 created this revision.Apr 3 2019, 2:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2019, 2:04 PM
sbc100 edited the summary of this revision. (Show Details)Apr 3 2019, 2:06 PM
sbc100 added reviewers: dschuff, kripken.
efriedma added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
625 ↗(On Diff #193592)

It's not a good idea to use cast<> if you can't prove it will succeed.

sbc100 updated this revision to Diff 193596.Apr 3 2019, 2:17 PM
  • feedback
sbc100 edited the summary of this revision. (Show Details)Apr 3 2019, 2:17 PM
sbc100 marked an inline comment as done.Apr 3 2019, 2:20 PM
kripken added inline comments.Apr 3 2019, 4:30 PM
llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
338 ↗(On Diff #193596)

I'm a little confused that previously we mentioned the linkage, but no longer do, and getorInsertGlobal doesn't document itself as setting Extrenal as the linkage?

sbc100 marked an inline comment as done.Apr 3 2019, 5:10 PM
sbc100 added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
338 ↗(On Diff #193596)
kripken accepted this revision.Apr 3 2019, 5:14 PM

Fair enough. Slightly worries me since it's not obvious from the name or the docs, and so perhaps it might change one day, but that's overly paranoid perhaps.

This revision is now accepted and ready to land.Apr 3 2019, 5:14 PM
sbc100 added a comment.Apr 3 2019, 6:28 PM

Fair enough. Slightly worries me since it's not obvious from the name or the docs, and so perhaps it might change one day, but that's overly paranoid perhaps.

The only thing we care about here is that it exists so I think we should be ok.

This revision was automatically updated to reflect the committed changes.