-
Notifications
You must be signed in to change notification settings - Fork 13.7k
[WebAssembly] Generate __clang_call_terminate for Emscripten EH #129020
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
When an exception thrown ends up calling `std::terminate`, for example, because an exception is thrown within a `noexcept` function or an exception is thrown from `__cxa_end_catch` during handling the previous exception, the libc++abi spec says we are supposed to call `__cxa_begin_catch` before `std::terminate`: https://libcxxabi.llvm.org/spec.html > 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. The default Itanium ABI generates a call to `__clang_call_terminate()`, which is a function that calls `__cxa_begin_catch` and then `std::terminate`: ```ll define void @__clang_call_terminate(ptr noundef %0) { %2 = call ptr @__cxa_begin_catch(ptr %0) call void @_ZSt9terminatev() unreachable } ``` But we replaced this with just a call to `std::terminate` in llvm@561abd8 because this caused some tricky transformation problems for Wasm EH. The detailed explanation why is in the commit description, but the summary is for Wasm EH it needs a `try` with both `catch` and `catch_all` and it was tricky to deal with. But that commit replaced `__clang_call_terminate` with `std::terminate` for all Wasm programs and not only the ones that use Wasm EH. So Emscripten EH was also affected by that commit. Emscripten EH is not able to catch foreign exceptions anyway, so this is unnecessary compromise. This makes we use `__clang_call_terminate` as in the default Itanium EH for Emscripten EH. We may later fix Wasm EH too but that requires more efforts in the backend. Related issue: emscripten-core/emscripten#23720
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-codegen Author: Heejin Ahn (aheejin) ChangesWhen an exception thrown ends up calling The default Itanium ABI generates a call to define void @<!-- -->__clang_call_terminate(ptr noundef %0) {
%2 = call ptr @<!-- -->__cxa_begin_catch(ptr %0)
call void @<!-- -->_ZSt9terminatev()
unreachable
} But we replaced this with just a call to But that commit replaced This makes we use Related issue: Full diff: https://github.com/llvm/llvm-project/pull/129020.diff 3 Files Affected:
diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp
index bcd171724c41d..f3ffa0b2e84c1 100644
--- a/clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -5150,9 +5150,14 @@ WebAssemblyCXXABI::emitTerminateForUnexpectedException(CodeGenFunction &CGF,
// Itanium ABI calls __clang_call_terminate(), which __cxa_begin_catch() on
// the violating exception to mark it handled, but it is currently hard to do
// with wasm EH instruction structure with catch/catch_all, we just call
- // std::terminate and ignore the violating exception as in CGCXXABI.
+ // std::terminate and ignore the violating exception as in CGCXXABI in Wasm EH
+ // and calls __clang_call_terminate only in Emscripten EH.
// TODO Consider code transformation that makes calling __clang_call_terminate
- // possible.
+ // in Wasm EH possible.
+ if (Exn && !EHPersonality::get(CGF).isWasmPersonality()) {
+ assert(CGF.CGM.getLangOpts().CPlusPlus);
+ return CGF.EmitNounwindRuntimeCall(getClangCallTerminateFn(CGF.CGM), Exn);
+ }
return CGCXXABI::emitTerminateForUnexpectedException(CGF, Exn);
}
diff --git a/clang/test/CodeGenCXX/wasm-eh.cpp b/clang/test/CodeGenCXX/wasm-eh.cpp
index 9dc15633bfed9..e8797794e7c1e 100644
--- a/clang/test/CodeGenCXX/wasm-eh.cpp
+++ b/clang/test/CodeGenCXX/wasm-eh.cpp
@@ -6,6 +6,9 @@
// RUN: %clang_cc1 %s -triple wasm32-unknown-unknown -fms-extensions -fexceptions -fcxx-exceptions -mllvm -wasm-enable-eh -exception-model=wasm -target-feature +exception-handling -emit-llvm -o - -std=c++11 | FileCheck %s
// RUN: %clang_cc1 %s -triple wasm64-unknown-unknown -fms-extensions -fexceptions -fcxx-exceptions -mllvm -wasm-enable-eh -exception-model=wasm -target-feature +exception-handling -emit-llvm -o - -std=c++11 | FileCheck %s
+// Test code generation for Wasm EH using WebAssembly EH proposal.
+// (https://github.com/WebAssembly/exception-handling/blob/main/proposals/exception-handling/Exceptions.md)
+
void may_throw();
void dont_throw() noexcept;
@@ -381,6 +384,15 @@ void test8() {
// CHECK: unreachable
+void noexcept_throw() noexcept {
+ throw 3;
+}
+
+// CATCH-LABEL: define void @_Z14noexcept_throwv()
+// CHECK: %{{.*}} = cleanuppad within none []
+// CHECK-NEXT: call void @_ZSt9terminatev()
+
+
// RUN: %clang_cc1 %s -triple wasm32-unknown-unknown -fms-extensions -fexceptions -fcxx-exceptions -exception-model=wasm -target-feature +exception-handling -emit-llvm -o - -std=c++11 2>&1 | FileCheck %s --check-prefix=WARNING-DEFAULT
// RUN: %clang_cc1 %s -triple wasm32-unknown-unknown -fms-extensions -fexceptions -fcxx-exceptions -exception-model=wasm -target-feature +exception-handling -Wwasm-exception-spec -emit-llvm -o - -std=c++11 2>&1 | FileCheck %s --check-prefix=WARNING-ON
// RUN: %clang_cc1 %s -triple wasm32-unknown-unknown -fms-extensions -fexceptions -fcxx-exceptions -exception-model=wasm -target-feature +exception-handling -Wno-wasm-exception-spec -emit-llvm -o - -std=c++11 2>&1 | FileCheck %s --check-prefix=WARNING-OFF
diff --git a/clang/test/CodeGenCXX/wasm-em-eh.cpp b/clang/test/CodeGenCXX/wasm-em-eh.cpp
new file mode 100644
index 0000000000000..9b2ccb2103bbc
--- /dev/null
+++ b/clang/test/CodeGenCXX/wasm-em-eh.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 %s -triple wasm32-unknown-unknown -fexceptions -fcxx-exceptions -emit-llvm -o - -std=c++11 2>&1 | FileCheck %s
+
+// Test code generation for Wasm's Emscripten (JavaScript-style) EH.
+
+void noexcept_throw() noexcept {
+ throw 3;
+}
+
+// CATCH-LABEL: define void @_Z14noexcept_throwv()
+// CHECK: %[[LPAD:.*]] = landingpad { ptr, i32 }
+// CHECK-NEXT: catch ptr null
+// CHECK-NEXT: %[[EXN:.*]] = extractvalue { ptr, i32 } %[[LPAD]], 0
+// CHECK-NEXT: call void @__clang_call_terminate(ptr %[[EXN]])
|
// TODO Consider code transformation that makes calling __clang_call_terminate | ||
// possible. | ||
// in Wasm EH possible. | ||
if (Exn && !EHPersonality::get(CGF).isWasmPersonality()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that isWasmPersonality()
is false when using emscripten EH; out of curiosity, what is the value of EHPersonality
in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't change anything in this case, and I checked now, and it is GNU_CPlusPlus
defined here:
llvm-project/clang/lib/CodeGen/CGCleanup.h
Line 680 in f3b1849
static const EHPersonality GNU_CPlusPlus; |
EHPersonality::isWasmPersonality()
returns true when it is GNU_Wasm_CPlusPlus
:
llvm-project/clang/lib/CodeGen/CGCleanup.h
Line 701 in f3b1849
bool isWasmPersonality() const { return this == &GNU_Wasm_CPlusPlus; } |
clang/test/CodeGenCXX/wasm-em-eh.cpp
Outdated
@@ -0,0 +1,13 @@ | |||
// RUN: %clang_cc1 %s -triple wasm32-unknown-unknown -fexceptions -fcxx-exceptions -emit-llvm -o - -std=c++11 2>&1 | FileCheck %s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this triple be wasm32-unknown-emscripten? Or do we use em-exceptions by default on all wasm triples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't do anything differently wrt EH depending on whether it is unknown
or emscripten
triple. But given that this test only applies to Emscripten, I changed it to wasm-unknown-emscripten
.
Co-authored-by: Derek Schuff <[email protected]>
…#129020) When an exception thrown ends up calling `std::terminate`, for example, because an exception is thrown within a `noexcept` function or an exception is thrown from `__cxa_end_catch` during handling the previous exception, the libc++abi spec says we are supposed to call `__cxa_begin_catch` before `std::terminate`: https://libcxxabi.llvm.org/spec.html > 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. The default Itanium ABI generates a call to `__clang_call_terminate()`, which is a function that calls `__cxa_begin_catch` and then `std::terminate`: ```ll define void @__clang_call_terminate(ptr noundef %0) { %2 = call ptr @__cxa_begin_catch(ptr %0) call void @_ZSt9terminatev() unreachable } ``` But we replaced this with just a call to `std::terminate` in llvm@561abd8 because this caused some tricky transformation problems for Wasm EH. The detailed explanation why is in the commit description, but the summary is for Wasm EH it needed a `try` with both `catch` and `catch_all` and it was tricky to deal with. But that commit replaced `__clang_call_terminate` with `std::terminate` for all Wasm programs and not only the ones that use Wasm EH. So Emscripten EH was also affected by that commit. Emscripten EH is not able to catch foreign exceptions anyway, so this is unnecessary compromise. This makes we use `__clang_call_terminate` as in the default Itanium EH for Emscripten EH. We may later fix Wasm EH too but that requires more efforts in the backend. Related issue: emscripten-core/emscripten#23720
I think it is generally better for tests have some descriptive function names so that we can insert new tests in the middle and don't have to renumber all tests. Also recently I added a (named) test to this file in llvm#129020 so I think it's consistent for other tests to be named.
I think it is generally better for tests have some descriptive function names so that we can insert new tests in the middle and don't have to renumber all tests. Also recently I added a (named) test to this file in #129020 so I think it's consistent for other tests to be named.
When an exception thrown ends up calling
std::terminate
, for example, because an exception is thrown within anoexcept
function or an exception is thrown from__cxa_end_catch
during handling the previous exception, the libc++abi spec says we are supposed to call__cxa_begin_catch
beforestd::terminate
:https://libcxxabi.llvm.org/spec.html
The default Itanium ABI generates a call to
__clang_call_terminate()
, which is a function that calls__cxa_begin_catch
and thenstd::terminate
:But we replaced this with just a call to
std::terminate
in 561abd8 because this caused some tricky transformation problems for Wasm EH. The detailed explanation why is in the commit description, but the summary is for Wasm EH it needed atry
with bothcatch
andcatch_all
and it was tricky to deal with.But that commit replaced
__clang_call_terminate
withstd::terminate
for all Wasm programs and not only the ones that use Wasm EH. So Emscripten EH was also affected by that commit. Emscripten EH is not able to catch foreign exceptions anyway, so this is unnecessary compromise.This makes we use
__clang_call_terminate
as in the default Itanium EH for Emscripten EH. We may later fix Wasm EH too but that requires more efforts in the backend.Related issue:
emscripten-core/emscripten#23720