-
Couldn't load subscription status.
- Fork 3.4k
[EH] Make abort() work with Wasm EH #16910
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
Changes from 4 commits
f650912
a6145eb
a50f2dc
dd74bc5
f4e389a
b6fc45f
9ee27ca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -605,12 +605,20 @@ function abort(what) { | |
| // Use a wasm runtime error, because a JS error might be seen as a foreign | ||
| // exception, which means we'd run destructors on it. We need the error to | ||
| // simply make the program stop. | ||
| // FIXME This is not working now, because Wasm EH currently does not assume | ||
| // all RuntimeErrors are from traps; it decides whether a RuntimeError is from | ||
| // a trap or not based on a hidden field within the object. So at the moment | ||
| // we don't have a way of throwing a trap from JS. TODO Make a JS API that | ||
aheejin marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| // allows this. | ||
aheejin marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| // Suppress closure compiler warning here. Closure compiler's builtin extern | ||
| // defintion for WebAssembly.RuntimeError claims it takes no arguments even | ||
| // though it can. | ||
| // TODO(https://github.com/google/closure-compiler/pull/3913): Remove if/when upstream closure gets fixed. | ||
|
|
||
| #if EXCEPTION_HANDLING == 1 | ||
| // In the meantime, we resort to wasm code for trapping. | ||
aheejin marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| ___trap(); | ||
| #else | ||
| /** @suppress {checkTypes} */ | ||
| var e = new WebAssembly.RuntimeError(what); | ||
|
|
||
|
|
@@ -621,6 +629,7 @@ function abort(what) { | |
| // in code paths apart from instantiation where an exception is expected | ||
| // to be thrown when abort is called. | ||
| throw e; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it might make sense to delete this path and just use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I didn't do that in this PR because
I don't think either of these is an important enough though. WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm OK with landing this now and then looking at possible simplification later. I think I'm currently a fan of (in release builds) avoiding the JS import of abort completely.. and keeping this fulling native where possible. |
||
| #endif | ||
| } | ||
|
|
||
| // {{MEM_INITIALIZER}} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| void __trap() { | ||
| __builtin_trap(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets just call this file There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1639,6 +1639,20 @@ class Polymorphic {virtual void member(){}}; | |||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| ''', 'exception caught: std::bad_typeid') | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| @with_both_eh_sjlj | ||||||||||||||||||||||||||||||||||||||||
| def test_terminate_abort(self): | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
| # std::terminate eventually calls abort(). We used ti implement abort() with | ||||||||||||||||||||||||||||||||||||||||
aheejin marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||
| # throwing a JS exception, but this can be again caught by std::terminate's | ||||||||||||||||||||||||||||||||||||||||
| # cleanup and cause an infinite loop. When Wasm EH is enabled, abort() is | ||||||||||||||||||||||||||||||||||||||||
| # implemented by a trap now. | ||||||||||||||||||||||||||||||||||||||||
| err = self.do_run(r''' | ||||||||||||||||||||||||||||||||||||||||
| #include <exception> | ||||||||||||||||||||||||||||||||||||||||
| int main() { | ||||||||||||||||||||||||||||||||||||||||
| std::terminate(); | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
| // Use a wasm runtime error, because a JS error might be seen as a foreign | |
| // exception, which means we'd run destructors on it. We need the error to | |
| // simply make the program stop. | |
| // Suppress closure compiler warning here. Closure compiler's builtin extern | |
| // defintion for WebAssembly.RuntimeError claims it takes no arguments even | |
| // though it can. | |
| // TODO(https://github.com/google/closure-compiler/pull/3913): Remove if/when upstream closure gets fixed. | |
| /** @suppress {checkTypes} */ | |
| var e = new WebAssembly.RuntimeError(what); | |
| #if MODULARIZE | |
| readyPromiseReject(e); | |
| #endif | |
| // Throw the error whether or not MODULARIZE is set because abort is used | |
| // in code paths apart from instantiation where an exception is expected | |
| // to be thrown when abort is called. | |
| throw e; |
This ends up throwing a JS exception. This is basically just a foreign exception from the wasm perspective, and is caught by catch_all, and calls std::terminate again. And the whole process continues until the call stack is exhausted.
What #9730 tried to do was throwing a trap, because Wasm catch/catch_all don't catch traps. Traps become RuntimeErrors after they hit a JS frame. To be consistent, we decided catch/catch_all shouldn't catch them after they become RuntimeErrors. That's the reason #9730 changed the code to throw not just any random thing but RuntimeError. But somehow we decided that we make that trap distinction not based on RuntimeError class but some hidden field (WebAssembly/exception-handling#89 (comment)).
So to answer your original question, if we use abort(), it will just throw RuntimeError and end there without a problem, because there's no noexcept function involved and there will be no catch_all and call std::terminate. We need std::terminate to show the call stack exhausting behavior.
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 think I understand.. Perhaps a more direct test would be that its not possible to catch exceptions thrown by abort(). e.g.:
try {
abort();
} catch (...) {
print('should never see this\n');
}
Here we are testing that whatever the low level abort does is not catchable.. which I think is important part of this change.
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.
This is what I wrote and deleted. Actually I was confused and this is not caught by the catch, because our catch (...) only catches C++ exceptions for now. I think whether catch (...) should catch foreign exceptions or not is something we should decide in the tool convention, and current impl is not catching it. So JS exceptions are only caught by the cleanups, which run destructors.
So I think removing noexcept from std::terminate and std::__terminate work; it will unwind the stack and run the destructors, but that's maybe OK with users..? (This is one of the things we discussed in the meeting this morning; whether abort should unwind or not)
Uh oh!
There was an error while loading. Please reload this page.