This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Add -fwasm-exceptions for wasm EH
ClosedPublic

Authored by aheejin on Sep 4 2019, 9:53 PM.

Details

Summary

This adds -fwasm-exceptions (in similar fashion with
-fdwarf-exceptions or -fsjlj-exceptions) that turns on everything
with wasm exception handling from the frontend to the backend.

We currently have -mexception-handling in clang frontend, but this is
only about the architecture capability and does not turn on other
necessary options such as the exception model in the backend. (This can
be turned on with llc -exception-model=wasm, but llc is not invoked
separately as a command line tool, so this option has to be transferred
from clang.)

Turning on -fwasm-exceptions in clang also turns on
-mexception-handling if not specified, and will error out if
-mno-exception-handling is specified.

Diff Detail

Repository
rL LLVM

Event Timeline

aheejin created this revision.Sep 4 2019, 9:53 PM
aheejin updated this revision to Diff 218834.Sep 4 2019, 9:55 PM
  • Remove a blank line

Is there a plan to turn this on by default at some point, or will users always have to opt-in to using exceptions? Also, would it make sense to automatically turn this feature on if the user instead supplies -mexception-handling to turn on the target feature?

sbc100 added a comment.Sep 5 2019, 2:47 PM

I don't really see why we can't just have the existing -mexception-handling take on this new meaning?

sbc100 added a comment.Sep 5 2019, 2:48 PM

Is there some reason why clang can't pass the -exception-model=wasm flag programatically to llc when -mmexception-handling is set?

aheejin added a comment.EditedSep 5 2019, 3:26 PM

@tlively @sbc100 What I wanted was to make -fwasm-exceptions as something like -pthreads, which serves as the only flag that needs to be turned on and turns on all other necessary flags, such as +matomics, +mbulk-memory, and --shared-memory. I think all those architectural flags starting with -m can be someday all turned on, when all our existing proposals become the new MVP, and I still want to give users an option to opt out of threads of exception handling at that point. And it also makes sense to have a LangOption of WasmExceptions given that there are LangOptions for all other exception models. I think this covers most of the questions above.

@tlively

Is there a plan to turn this on by default at some point, or will users always have to opt-in to using exceptions?

I'm planning to create an emscripten option, something like -s EXCEPTION_HANDLING=1, to turn this option on. Maybe at some point, when we don't use emscripten exception anymore and all wasm platforms (wasi and emscripten and what not) use the new exception proposal, we can consider turning this on by default. Until then I incline not to.

sbc100 added a comment.Sep 5 2019, 3:33 PM

But why not make -mexception-handling be the option that enabled everything? I mean -mexception-handling is a flag we have today.. if you add -fwasm-exceptions what does -mexception-handling meaning? Is it still useful?

dschuff added a comment.EditedSep 5 2019, 3:45 PM

If this is to be like -fdwarf-exceptions I assume the idea is that eventually it will default on when targeting wasm, and become one of those flags that most users never set. In that case it really just selects the default exception model, and thus it should be compatible with -fno-exceptions just as -fdwarf-exceptions is.
Also -fno-exceptions doesn't really mean "no exceptions whatsoever" because IIRC if you call something that the compiler isn't sure never throws, it generates an implicit catch-all that calls std::terminate. So how it does that would still be affected by the exception model. (and whatever downstream invoke-removing pass or postprocessing tool might care).

But why not make -mexception-handling be the option that enabled everything? I mean -mexception-handling is a flag we have today.. if you add -fwasm-exceptions what does -mexception-handling meaning? Is it still useful?

As I said above, I'm not sure making architecture flags control everything is a good idea. I view those flags as very low-level flags whose only job is exactly turn on those architecture features (Others may view them differently though). Likewise, currently setting --pthreads controls other features, but setting -matomics or -mbulk-memory does not control other features.

aheejin added a comment.EditedSep 5 2019, 4:14 PM

If this is to be like -fdwarf-exceptions I assume the idea is that eventually it will default on when targeting wasm, and become one of those flags that most users never set. In that case it really just selects the default exception model, and thus it should be compatible with -fno-exceptions just as -fdwarf-exceptions is.\

-fwasm-exceptions works fine with -fno-exceptions. That has the same effect as when you specify both -fdwarf-exceptions and -fno-exceptions together; it just lowers invokes to calls. Is that not what you mean? What -fwasm-exceptions is not compatible with is -mno-exception-handling.

Also -fno-exceptions doesn't really mean "no exceptions whatsoever" because IIRC if you call something that the compiler isn't sure never throws, it generates an implicit catch-all that calls std::terminate. So how it does that would still be affected by the exception model. (and whatever downstream invoke-removing pass or postprocessing tool might care).

-fno-exceptions does not generate try-catch with calls to std::terminate. The native x86 compiler does not do that either. It just assumes exceptions don't exist. (And you can't use keywords like try, catch, or throw with -fno-exceptions.) What you described is when we use noexcept on the function declaration but the function calls another function that can throw, I think.

sbc100 added a comment.Sep 5 2019, 4:41 PM

Derek, are you saying that -mexception-handling is somehow related to -fno-exceptions. What is the relationship? I wasn't aware this change was related to -fno-exceptions.

-pthreads enabling -matomics and -mbulk-memorymake 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? Also, we didn't add -pthread, it was pre-existing we we didn't need to justify adding it but rather make it work.

Sorry, I'm not necessarily against this change but I want to avoid adding flags unless there is a clear need.

Oh, you're right, I'm conflating -mexception-handling with -fexceptions. And also -fno-exceptions with noexcept. So... just forget I was here

This comment was removed by aheejin.
aheejin added a comment.EditedSep 5 2019, 7:35 PM

-pthreads enabling -matomics and -mbulk-memorymake 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-exceptionsis 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. (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.SjLjExceptions)
  Options.ExceptionModel = llvm::ExceptionHandling::SjLj;
if (LangOpts.SEHExceptions)
  Options.ExceptionModel = llvm::ExceptionHandling::WinEH;
if (LangOpts.DWARFExceptions)
  Options.ExceptionModel = llvm::ExceptionHandling::DwarfCFI;
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 is not very clear. This CL fixes this too.

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

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

Thanks for the clarification. It makes sense to me that -mexception-handling only enables the architectural feature and a separate flag enables the behavior change. This is indeed consistent with how -pthread works.

What happens when users have exceptions in their code but don't pass -fwasm-exceptions? Do they get the old SJLJ emulated exception handling?

aheejin added a comment.EditedSep 6 2019, 4:12 PM

What happens when users have exceptions in their code but don't pass Do they get the old SJLJ emulated exception handling?

The current emscripten EH is enabled by clang -mllvm -enable-emscripten-cxx-exceptions, which is not a clang flag (it's passed by -mllvm, so it is for the LLVM). The reason it is not a clang flag is emscripten EH does not need any action in clang. This flag is set when you pass 0 or 2 to -s DISABLE_EXCEPTION_CATCHING= emscripten flag.

If users use exceptions but don't pass -fwasm-exceptions, if they pass -mllvm -enable-emscripten-cxx-exceptions instead, emscripten EH will be used. If they pass neither, neither EH will be used and all invokes will be lowered to calls.

Having said that, -fwasm-exceptions and -mllvm -enable-emscripten-cxx-exceptions are not compatible. If we can check this that'd be better. I'll check.

aheejin updated this revision to Diff 219203.EditedSep 6 2019, 4:50 PM
  • Error out when -fwasm-exceptions is specified with -mllvm -enable-emscripten-cxx-exceptions
tlively accepted this revision.Sep 11 2019, 1:12 PM
This revision is now accepted and ready to land.Sep 11 2019, 1:12 PM
sbc100 accepted this revision.Sep 11 2019, 2:48 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 11 2019, 9:01 PM