aheejin added inline comments.
================
Comment at: lib/CodeGen/CGCXXABI.h:610
+struct CatchRetScope final : EHScopeStack::Cleanup {
+ llvm::CatchPadInst *CPI;
----------------
dschuff wrote:
> Should be `public`?
This code was moved from [[
https://github.com/llvm-mirror/clang/blob/c4bfd75d786a2a77c779cee6976534f37202ac21/lib/CodeGen/MicrosoftCXXABI.cpp#L865
| here ]] in `lib/CodeGen/MicrosoftCXXABI.cpp`. There are dozens of classes
inheriting from `EHScopeStack::Cleanup` and they all use private inheritance.
Examples [[
https://github.com/llvm-mirror/clang/blob/c4bfd75d786a2a77c779cee6976534f37202ac21/lib/CodeGen/CGDecl.cpp#L449
| 1 ]] [[
https://github.com/llvm-mirror/clang/blob/c4bfd75d786a2a77c779cee6976534f37202ac21/lib/CodeGen/CGDecl.cpp#L504
| 2 ]] [[
https://github.com/llvm-mirror/clang/blob/master/lib/CodeGen/CGException.cpp#L347
| 3 ]] [[
https://github.com/llvm-mirror/clang/blob/c4bfd75d786a2a77c779cee6976534f37202ac21/lib/CodeGen/ItaniumCXXABI.cpp#L3728
| 4 ]]
The [[
https://github.com/llvm-mirror/clang/blob/c4bfd75d786a2a77c779cee6976534f37202ac21/lib/CodeGen/EHScopeStack.h#L140-L147
| comment ]] from `EHScopeStack::Cleanup` says all subclasses must be
POD-like, which I guess is the reason, but I'm not very sure.
================
Comment at: lib/CodeGen/CGCleanup.h:630
static const EHPersonality MSVC_CxxFrameHandler3;
+ static const EHPersonality GNU_Wasm_CPlusCPlus;
----------------
dschuff wrote:
> We might consider having 2 personalities: one for use with builtin
> exceptions, and other for emulated exceptions? I'm imagining that with this
> style of IR we might be able to do emulated exceptions better than we have
> for emscripten today. We don't have to think about that now, but maybe the
> name of the personality might reflect it: e.g. `GNU_Wasm_CPlusPlus_Native`
> (or builtin) vs `GNU_Wasm_CPlusPlus_Emulated` (or external).
(We talked in person :) ) I'll think about it. But I guess we can change the
name once we start implementing that feature?
================
Comment at: lib/CodeGen/CGException.cpp:1236
+ // them, we should unwind to the next EH enclosing scope. We generate a call
+ // to rethrow function here to do that.
+ if (EHPersonality::get(*this).isWasmPersonality() && !HasCatchAll) {
----------------
dschuff wrote:
> Why do we need to call `__cxa_rethrow` instead of just emitting a rethrow
> instruction intrinsic to unwind?
Because we have to run the library code as well. As well as in other functions
in libcxxabi, [[
https://github.com/llvm-mirror/libcxxabi/blob/565ba0415b6b17bbca46820a0fcfe4b6ab5abce2/src/cxa_exception.cpp#L571-L607
| `__cxa_rethrow` ]] has some bookeeping code like incrementing the handler
count or something. After it is re-caught by an enclosing scope, it is
considered as a newly thrown exception and the enclosing scope is run functions
like `__cxa_begin_catch` and `__cxa_end_catch`. So we also have to run
`__cxa_rethrow` when rethrowing something to make sure everything is matched.
The actual `rethrow` instruction will be added next to `__cxa_rethrow` in the
backend, or can be embedded within `__cxa_rethrow` function later if we decide
how to pass an exception value to a function. (which might be one reason why we
want to keep the first-class exception thing)
================
Comment at: lib/CodeGen/CGException.cpp:1534
+ // In case of wasm personality, we need to pass the exception value to
+ // __clang_call_terminate function.
+ if (getLangOpts().CPlusPlus &&
----------------
dschuff wrote:
> Why?
Not strictly necessarily, because we can modify libcxxabi to our liking. I was
trying to keep the same behavior as Itanium-style libcxxabi. The
`__clang_call_terminate` function that's called when an EH cleanup throws is as
follows:
```
; Function Attrs: noinline noreturn nounwind
define linkonce_odr hidden void @__clang_call_terminate(i8*) #6 comdat {
%2 = call i8* @__cxa_begin_catch(i8* %0) #2
call void @_ZSt9terminatev() #8
unreachable
}
```
So it calls `__cxa_begin_catch` on the exception value before calling
`std::terminate`. We can change this behavior for wasm if we want, and I guess
we need some proxy object in case of a foreign exception, but anyway I was
trying to keep the behavior the same unless there's any reason not to.
================
Comment at: test/CodeGenCXX/wasm-eh.cpp:33
+// CHECK-NEXT: %[[CATCHPAD:.*]] = catchpad within %[[CATCHSWITCH]] [i8*
bitcast (i8** @_ZTIi to i8*), i8* bitcast (i8** @_ZTId to i8*)]
+// CHECK-NEXT: %[[EXN:.*]] = call i8* @llvm.wasm.get.exception()
+// CHECK-NEXT: store i8* %[[EXN]], i8** %exn.slot
----------------
majnemer wrote:
> I'd expect a funclet bundle operand here..
Even if it cannot throw? Looks like even
`CodeGenFunction::getBundlesForFunclet` function does not add funclet bundle
operand in case the callee cannot throw: [[
https://github.com/llvm-mirror/clang/blob/1ec33d54dfc07388c7bc7d637723ceeaa74869ee/lib/CodeGen/CGCall.cpp#L3647-L3650
| code ]]
Repository:
rC Clang
https://reviews.llvm.org/D44931
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
