[PATCH] D44931: [WebAssembly] Use Windows EH instructions for Wasm EH

[Heejin Ahn via Phabricator via cfe-commits]

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