Page MenuHomePhabricator

[WebAssembly] Update WebAssemblyLowerEmscriptenEHSjLj to support separate compilation.
ClosedPublic

Authored by sbc100 on Jul 11 2018, 4:59 PM.

Details

Summary

Previously this pass would assume that it was running over the
entire program. This meant it could get away with creating the
two helper functions:

  • setThrew
  • setTempRet0

Instead we now assume these will be provided at link time. In
emscripten this will most likely be done via libcxxabi.

Additionally we previously created three global variable:

  • THREW
  • _threwValue
  • __tempRet0

These are now allow assumed to be available at link time, but
we also have to handle the case where they exist in the module
being compiled (e.g. when we are doing LTO with libcxxabi).

Diff Detail

Repository
rL LLVM

Event Timeline

sbc100 created this revision.Jul 11 2018, 4:59 PM
sbc100 updated this revision to Diff 155087.Jul 11 2018, 5:03 PM
  • comment
  1. For emscripten-based EH, we don't use a separate libcxxabi; emscripten provides handful of library functions in JavaScript. Are you planning to provide them from emscripten and import them from wasm?
  2. Because now this does not create those functions and global variables anymore, certain tests, such as lower-em-exceptions.ll, will fail. I guess you should update that as well.

Emscripten does have at least part of libcxxabi. All I need a library file where I can add these things. Here is my provisional change: https://github.com/kripken/emscripten/compare/sjlj?expand=1

Ah yes, thanks for pointing out that test. I will run them all an fix up.

aheejin added inline comments.Jul 11 2018, 5:32 PM
lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
72 ↗(On Diff #155088)

Because you are gonna provide setThrew and setTempRet0 in a separate cpp file, "their values should be set in JS code" does not strictly sound correct, because that cpp file is gonna be a part of wasm as well. Could you elaborate that these functions will be provided from library that's a part of emscripten?

And for the equivalent asm.js version of JS code below ↓ , I know that's not a part of your change, but I think we can delete this by now, because we are gonna switch to the wasm as a default backend anyway. Maybe show the code in cpp form would be better.

339 ↗(On Diff #155088)

Nit: LLVM style is GlobalVariable *V

632 ↗(On Diff #155088)

threwValue -> __threwValue

lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
487 ↗(On Diff #155088)

Nit: unneeded braces

sbc100 abandoned this revision.Jul 12 2018, 12:00 PM

Abandoning in favor of weak linking approach: https://reviews.llvm.org/D49263

dschuff added inline comments.Jul 12 2018, 12:04 PM
lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
487 ↗(On Diff #155088)

Nit: no, those braces are needed IMHO :D

sbc100 reclaimed this revision.Sep 28 2018, 2:39 PM

Re-opening since we need these symbols to be available pre-LTO.. i.e. before codegen.

sbc100 updated this revision to Diff 167556.Sep 28 2018, 3:12 PM

Rebase and fix tests

sbc100 updated this revision to Diff 167557.Sep 28 2018, 3:13 PM
  • feedback
sbc100 marked 3 inline comments as done.Sep 28 2018, 3:15 PM
sbc100 added inline comments.
lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
487 ↗(On Diff #155088)

At least in lld the policy is not to include them this these cases.

sbc100 updated this revision to Diff 167558.Sep 28 2018, 3:15 PM
sbc100 marked 3 inline comments as done.
  • feedback
jgravelle-google accepted this revision.Sep 28 2018, 3:42 PM
This revision is now accepted and ready to land.Sep 28 2018, 3:42 PM
This revision was automatically updated to reflect the committed changes.