Page MenuHomePhabricator

[WebAssembly] Disable uses of __clang_call_terminate
ClosedPublic

Authored by aheejin on Mar 2 2021, 11:46 PM.

Details

Summary

Background:

Wasm EH, while using Windows EH (catchpad/cleanuppad based) IR, uses
Itanium-based libraries and ABIs with some modifications.

__clang_call_terminate is a wrapper generated in Clang's Itanium C++
ABI implementation. It contains this code, in C-style pseudocode:

void __clang_call_terminate(void *exn) {
  __cxa_begin_catch(exn);
  std::terminate();
}

So this function is a wrapper to call __cxa_begin_catch on the
exception pointer before termination.

In Itanium ABI, this function is called when another exception is thrown
while processing an exception. The pointer for this second, violating
exception is passed as the argument of this __clang_call_terminate,
which calls __cxa_begin_catch with that pointer and calls
std::terminate to terminate the program.

The spec for __cxa_begin_catch says,

When the personality routine encounters a termination condition, it
will call __cxa_begin_catch() to mark the exception as handled and then
call terminate(), which shall not return to its caller.

In wasm EH's Clang implementation, this function is called from
cleanuppads that terminates the program, which we also call terminate
pads. Cleanuppads normally don't access the thrown exception and the
wasm backend converts them to catch_all blocks. But because we need
the exception pointer in this cleanuppad, we generate
wasm.get.exception intrinsic (which will eventually be lowered to
catch instruction) as we do in the catchpads. But because terminate
pads are cleanup pads and should run even when a foreign exception is
thrown, so what we have been doing is:

  1. In WebAssemblyLateEHPrepare::ensureSingleBBTermPads(), we make sure terminate pads are in this simple shape:
%exn = catch
call @__clang_call_terminate(%exn)
unreachable
  1. In WebAssemblyHandleEHTerminatePads pass at the end of the pipeline, we attach a catch_all to terminate pads, so they will be in this form:
%exn = catch
call @__clang_call_terminate(%exn)
unreachable
catch_all
call @std::terminate()
unreachable

In catch_all part, we don't have the exception pointer, so we call
std::terminate() directly. The reason we ran HandleEHTerminatePads at
the end of the pipeline, separate from LateEHPrepare, was it was
convenient to assume there was only a single catch part per try
during CFGSort and CFGStackify.


Problem:

While it thinks terminate pads could have been possibly split or calls
to __clang_call_terminate could have been duplicated,
WebAssemblyLateEHPrepare::ensureSingleBBTermPads() assumes terminate
pads contain no more than calls to __clang_call_terminate and
unreachable instruction. I assumed that because in LLVM very limited
forms of transformations are done to catchpads and cleanuppads to
maintain the scoping structure. But it turned out to be incorrect;
passes can merge multiple cleanuppads into one, including terminate
pads, as long as the new code has a correct scoping structure. One pass
that does this I observed was SimplifyCFG, but there can be more. After
this transformation, a single cleanuppad can contain any number of other
instructions with the call to __clang_call_terminate and can span many
BBs. It wouldn't be practical to duplicate all these BBs within the
cleanuppad to generate the equivalent catch_all blocks, only with
calls to __clang_call_terminate replaced by calls to std::terminate.

Unless we do more complicated transformation to split those calls to
__clang_call_terminate into a separate cleanuppad, it is tricky to
solve.


Solution (?):

This CL just disables the generation and use of __clang_call_terminate
and calls std::terminate() directly in its place.

The possible downside of this approach can be, because the Itanium ABI
intended to "mark" the violating exception handled, we don't do that
anymore. What __cxa_begin_catch actually does is increment the
exception's handler count and decrement the uncaught exception count,
which in my opinion do not matter much given that we are about to
terminate the program anyway. Also it does not affect info like stack
traces that can be possibly shown to developers.

And while we use a variant of Itanium EH ABI, we can make some
deviations if we choose to; we are already different in that in the
current version of the EH spec we don't support two-phase unwinding. We
can possibly consider a more complicated transformation later to
reenable this, but I don't think that has high priority.

Changes in this CL contains:

  • In Clang, we don't generate a call to wasm.get.exception() intrinsic and __clang_call_terminate function in terminate pads anymore; we simply generate calls to std::terminate(), which is the default implementation of CGCXXABI::emitTerminateForUnexpectedException.
  • Remove WebAssembly::ensureSingleBBTermPads() function and WebAssemblyHandleEHTerminatePads` pass, because terminate pads are already catch_all now (because they don't need the exception pointer) and we don't need these transformations anymore.
  • Change tests to use std::terminate directly. Also removes tests that tested LateEHPrepare::ensureSingleBBTermPads and HandleEHTerminatePads pass.
  • Drive-by fix: Add some function attributes to EH intrinsic declarations

Fixes https://github.com/emscripten-core/emscripten/issues/13582.

Diff Detail

Event Timeline

aheejin created this revision.Mar 2 2021, 11:46 PM
aheejin requested review of this revision.Mar 2 2021, 11:46 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 2 2021, 11:46 PM
aheejin edited the summary of this revision. (Show Details)Mar 2 2021, 11:52 PM
aheejin edited the summary of this revision. (Show Details)Mar 2 2021, 11:54 PM
aheejin edited the summary of this revision. (Show Details)
aheejin edited the summary of this revision. (Show Details)
aheejin edited the summary of this revision. (Show Details)Mar 2 2021, 11:59 PM
tlively accepted this revision.Mar 3 2021, 10:48 AM

Nice, this actually looks like a good simplification in addition to fixing the specific problem.

clang/lib/CodeGen/ItaniumCXXABI.cpp
4650
This revision is now accepted and ready to land.Mar 3 2021, 10:48 AM
dschuff accepted this revision.Mar 3 2021, 11:40 AM

I agree this is a good approach, and I also like that it's smaller and simpler. In the future we can revisit whether following this particular Itanium convention buys us anything useful or not.

aheejin marked an inline comment as done.Mar 4 2021, 3:53 AM
aheejin updated this revision to Diff 328116.Mar 4 2021, 3:53 AM

Address comments

This revision was landed with ongoing or failed builds.Mar 4 2021, 2:58 PM
This revision was automatically updated to reflect the committed changes.