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

Fri Aug 13 12:05:17 PDT 2021

dschuff added a comment.

> In D107685#2942894 <https://reviews.llvm.org/D107685#2942894>, @aheejin wrote:
>
> 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.

This case of "disable exceptions but use wasm EH for SjLj" is the case I was thinking about too, and I do think we should support that. There is one nuance here though about lowering invokes. The traditional emscripten way to disable exceptions is to generate the code with `invoke`s and then lower them away. But the upstream way is to use `-fignore-exceptions` which IIUC does not do that; instead it just does not generate any invokes at all (see e.g. clang/test/CodeGen/ignore-exceptions.cpp). And of course C does not generate any invokes. 
Although, looking at your examples below, I guess that distinction doesn't actually matter in this case.

> 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.

My intuition is that `-exception-model` should really only affect C++ exceptions and not be a generic flag. But because it seems to be an LLVM-internal thing (as opposed to a user-facing clang flag) I guess I'm not that surprised that the usage isn't purely for that.
I would actually be OK with using `-mattr` to mean "use wasm EH for setjmp/longjmp". With other attributes (and I think even on other targets), AFAIK it doesn't just mean that the target is new enough, but it also causes codegen to actually generate the instructions as well?  
I suppose you could make the argument that it should be possible to enable EH instructions but disable SJLJ lowering (in that case, what would you do? I guess you'd just have to lower setjmp/longjmp to call an import?)

> 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?

So `-exception-model=wasm` without either `-wasm-enable-eh` or `-wasm-enable-sjlj` would be an error, or a no-op? And either `-wasm-enable-eh` or `-wasm-enable-sjlj` without `-exception-model-wasm` would be an error?
I guess that would be ok as long as we have reasonable errors (clang users wouldn't need to care of course, but Rust or other users of just LLVM might still want to be able to use the capability of setjmp/longjmp without having to generate full WinEH-style IR)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107685