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

Fri Aug 13 15:09:15 PDT 2021

aheejin added a comment.

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

>> 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 think the current LLVM usages are more for "any exceptions" and not only meant for C++ exceptions, meaning, anything that can generate `invoke`s. Clang can compile Objective C, and I guess it will apply to other frontends of LLVM if they generate `invoke`.

> 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 think this is a valid point of view. I was mostly thinking it would be better to give a choice even if we are running on a brand-new engine that support all proposed features, in the same way we let users use Emscripten EH even if the engine and toolchain support Wasm EH. But this is also necessary an interim measure while Wasm EH support is not very stable.

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

If we disable SjLj lowering entirely, they will error out at link time or something because we don't have those functions in Emscripten library. The current default is to use Emscripten SjLj.

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

I think it would error out, which I think is clearer. Also note that these are not user-facing flags so if they are inconsistent it is more likely to be an error from the driver side.

> And either `-wasm-enable-eh` or `-wasm-enable-sjlj` without `-exception-model-wasm` would be an error?

I think so too.

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

I still support `-wasm-enable-eh` as a parallel of `-wasm-enable-sjlj`; I think it is less confusing. Later, if we think everyone should switch to Wasm EH/SjLj and refrain from using Emscripten EH/SjLj, we can make the default values of `-wasm-enable-sjlj` and `-wasm-enable-eh` true. These `-wasm-enable-***` flags will only be used to determine how to lower `invoke`s and `setjmp`/`longjmp`s in LowerEmscriptenEHSjLJ and WasmEHPrepare passes, and our later backend passes checks the exception model only without caring whether EH instructions were generated from exceptions or SjLj.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107685