[PATCH] D67208: [WebAssembly] Add -fwasm-exceptions for wasm EH

[Heejin Ahn via Phabricator via cfe-commits]

aheejin added a comment.

In D67208#1660041 <https://reviews.llvm.org/D67208#1660041>, @sbc100 wrote:

>   `-pthreads`  enabling `-matomics` and `-mbulk-memory`make some sense because each of those low level flags might make sense on its own.   But if `-fwasm-exceptions` only going to enable `-mexception-handling` then I'm not sure I see the point in adding it.  Or is there something else it will enable?


I think I didn't provide enough motives; let me explain this over again.
**tl;dr: I think `-fwasm-exceptions`is necessary. Something we should decide is whether we should make `-mexception-handling` turn it on automatically.**

1. Why we need `LangOptions::WasmExceptions`

What we need to additionally set besides `-mattr=+exception-handling` in the backend is `-exception-model=wasm`, which sets the exception model <https://github.com/llvm/llvm-project/blob/b1cf175271820b17c27edfd483c2ab52ce0afcfb/llvm/include/llvm/MC/MCTargetOptions.h#L17-L24>. (We need to set this programmatically. I used `llc` options just for illustration) This part corresponds to this code in `clang/lib/CodeGen/BackendUtil.cpp` above.

  if (LangOpts.WasmExceptions)
    Options.ExceptionModel = llvm::ExceptionHandling::Wasm;

If we don't have `LangOption::WasmExceptions`, we would need to invoke target features to set this option, which is not very clean and inconsistent with other exception models.

Also, because we didn't have `LangOptions::WasmExceptions`, the current code for selecting personality function <https://github.com/llvm/llvm-project/blob/b1cf175271820b17c27edfd483c2ab52ce0afcfb/clang/lib/CodeGen/CGException.cpp#L162-L173> is not very clear. This CL fixes this too.

2. Why we need `-fwasm-exceptions`

`-fwasm-exceptions` is the means to set `LangOptions::WasmExceptions`. Please see the code for `clang/lib/Frontend/CompilerInvocation.cpp` in this CL.

  Arg *A = Args.getLastArg(
      options::OPT_fsjlj_exceptions, options::OPT_fseh_exceptions,
      options::OPT_fdwarf_exceptions, options::OPT_fwasm_exceptions);
  if (A) {
    ...
    Opts.SjLjExceptions = Opt.matches(options::OPT_fsjlj_exceptions);
    Opts.SEHExceptions = Opt.matches(options::OPT_fseh_exceptions);
    Opts.DWARFExceptions = Opt.matches(options::OPT_fdwarf_exceptions);
    Opts.WasmExceptions = Opt.matches(options::OPT_fwasm_exceptions);
  }

If we don't have `options::OPT_fwasm_exceptions` there, the code to determine whether we have to set `Opts.WasmExceptions` will be not very clean and inconsistent with other model options.
(Here this `Opts.WasmExceptions` will be the same as `LangOpts.WasmExceptions` in `clang/lib/CodeGen/BackendUtil.cpp` above.)

3. Why I think choosing `-fwasm-exceptions`, not `-mexception-handling`, as the controlling option, is better

Given that we need `-fwasm-exceptions` anyway (according to 1 and 2 above), I think `-fwasm-exceptions` sets `-mexception-handling` makes more sense than the other way around. Also, as I said above, I'm not sure if using architectural flags to control features is a good idea. They are pretty low-level, and they only exist because wasm features are developed one by one; if we were able to put everything into MVP they wouldn't exist. Wouldn't we someday turn on all these architectural flags on by default? And it's possible that we want to use the current emscripten-based exception even if we have all architectural features turned on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67208