[PATCH] D107685: [WebAssembly] Tidy up EH/SjLj options

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

In D107685#2942259 <https://reviews.llvm.org/D107685#2942259>, @dschuff wrote:

> I get that enabling the new SjLj will require enabling the wasm EH feature, but it's not obvious why it should require `--exception-model=wasm` Is it possible to enable the new SJLJ with `exception-model=none` and have it force on `-mattr=+exception-handling` (or failing that, require only `-mattr=+exception-handling` and not both)? I would think e.g. this should eventually be the default for plain C code.

Hmm, I've been thinking about this and I don't quite like the current candidates I proposed either. For example, what if someone wants to disable exceptions, i.e., lower all `invoke`s to `calls`, but want to handle SjLj using Wasm EH instructions? The current options don't allow that, because `-wasm-enable-sjlj` requires `-exception-model=wasm`, and `-exception-model=wasm` means we enable Wasm EH.

I feel `-exception-model=wasm` should be treated more like `-mattr=+exception-handling`, which means, the capability of handling wasm EH instructions. We use it with that meaning already in various parts of codebase. For example in WebAssembly target:
https://github.com/llvm/llvm-project/blob/76beb4184cfcf41cb60fbcfb836b41eb58321b32/llvm/lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp#L143-L144
https://github.com/llvm/llvm-project/blob/76beb4184cfcf41cb60fbcfb836b41eb58321b32/llvm/lib/Target/WebAssembly/WebAssemblyLateEHPrepare.cpp#L119-L121
https://github.com/llvm/llvm-project/blob/76beb4184cfcf41cb60fbcfb836b41eb58321b32/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp#L1566-L1568

But also in common code:
https://github.com/llvm/llvm-project/blob/76beb4184cfcf41cb60fbcfb836b41eb58321b32/llvm/lib/CodeGen/TargetPassConfig.cpp#L920-L926
https://github.com/llvm/llvm-project/blob/76beb4184cfcf41cb60fbcfb836b41eb58321b32/llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp#L412
https://github.com/llvm/llvm-project/blob/76beb4184cfcf41cb60fbcfb836b41eb58321b32/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp#L413-L415

In all these place, what they check is not whether we have lowered our `invoke`s into `call`s or anything, but rather "Do we have Wasm EH instructions that need to be supported?" And the answer should be yes when we use Wasm EH instructions for SjLj too. It is hard to replace this with `-mattr=+exception-handling`, because, `-exception-model=` is not used this way only by us but by common code too, and I don't think the `-mattr=` flag is for that purpose in the first place. It simply means the current architecture is new enough to support the feature, and it doesn't mean we should use Wasm EH for anything.

So, how about adding a new option, let's say, `-wasm-enable-eh`, which is a parallel of `-wasm-enable-sjlj`? This still will only affect `opt` and `llc` and it will be set appropriately by `clang` flags which will be controlled by `emcc`, so it doesn't mean we require users to pass an additional flag by doing this. `-exception-model=wasm` and `-mattr=+exception-handling` mean just the capability of our backend that we can handle Wasm EH instructions in case they are used anywhere in the module. If you use either of `-wasm-enable-eh` or `-wasm-enable-sjlj`, both of `-exception-model=wasm` and `-mattr=+exception-handling` should be present. (`-mattr=+exception-handling can be omitted if it is in the bitcode file attributes, which will be the case in most cases)

Even with both of  `-exception-model=wasm` and `-mattr=+exception-handling` present, we can choose to handle neither of exceptions or SjLj or only one of them, or both. The list of options required for each mode will be:

- Emscripten EH: `-enable-emscripten-cxx-exceptions`
- Emscripten SjLj: `-enable-emscripten-sjlj`
- Wasm EH: `-wasm-enable-eh -exception-model=wasm -mattr=+exception-handling`
- Wasm SjLj: `-wasm-enable-sjlj -exception-model=wasm -mattr=+exception-handling`

What do you think?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107685/new/

https://reviews.llvm.org/D107685