Skip to content

[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.

Sign up for GitHub

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

Merged
merged 7 commits into from
May 11, 2022
Merged

[EH] Make abort() work with Wasm EH #16910

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f650912
Make `do_run` tests functions consistent
aheejin May 6, 2022
a6145eb
[EH] Make abort() work with Wasm EH
aheejin May 6, 2022
a50f2dc
Merge branch 'main' into terminate
aheejin May 7, 2022
dd74bc5
Implement `__trap` in C
aheejin May 7, 2022
f4e389a
Apply suggestions from code review
aheejin May 10, 2022
b6fc45f
exception_builtins.c -> __trap.c
aheejin May 10, 2022
9ee27ca
Address comments
aheejin May 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions emcc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2566,6 +2566,9 @@ def get_full_import_name(name):
if settings.SUPPORT_LONGJMP == 'wasm':
settings.DEFAULT_LIBRARY_FUNCS_TO_INCLUDE.append('__c_longjmp')

if settings.EXCEPTION_HANDLING:
settings.REQUIRED_EXPORTS += ['__trap']

return target, wasm_target


Expand Down
11 changes: 10 additions & 1 deletion src/preamble.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 approach does not work in Wasm EH because it 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 wasm trap from JS. TODO Make a JS API that
// allows this in the wasm spec.

// 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
// See above, in the meantime, we resort to wasm code for trapping.
___trap();
#else
/** @suppress {checkTypes} */
var e = new WebAssembly.RuntimeError(what);

Expand All @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 __trap in all cases.. but I guess that can be a followup maybe?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I didn't do that in this PR because

  1. This changes the error message slightly for existing users, which I don't think is a big deal, but just to be conservative
  2. I eventually want to revive this, by making JS API that can create traps; so I was somewhat hesitant to delete this part.

I don't think either of these is an important enough though. WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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}}
Expand Down
3 changes: 3 additions & 0 deletions system/lib/compiler-rt/__trap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
void __trap() {
__builtin_trap();
}
14 changes: 14 additions & 0 deletions tests/test_core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1639,6 +1639,20 @@ class Polymorphic {virtual void member(){}};
}
''', 'exception caught: std::bad_typeid')

@with_both_eh_sjlj
def test_terminate_abort(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about renaming this "test_abort_no_dtors" and rewriting it to call C abort, with a C++ object on the stack?

The reason I ask is that this change seem more about the behaviour of abort than the behaviour of the higher level std::terminate

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

# std::terminate eventually calls abort(). We used to implement abort() with
# 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();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you just call C abort() here to be more direct?

Copy link
Member Author

@aheejin aheejin May 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs a little more context.
noexcept function shouldn't throw, so noexcept function code generation is to invoke every function call in those functions and in case they throw, call std::terminate. This codegen comes from clang and native platforms do this too. So in wasm, they become something like

try
  function body
catch_all
  call std::terminate
end

std::terminate calls std::__terminate. Both of std::terminate and std::__terminate are noexcept. So that means their code is structured like that, which sounds like self-calling, but normally no function calls in those functions should ever throw, so that's fine. But in our case, abort ends up throwing, which is a problem.

The function body of __terminate eventually calls JS abort, and ends up here:

emscripten/src/preamble.js

Lines 605 to 623 in 970998b

// 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.

Copy link
Collaborator

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.

Copy link
Member Author

@aheejin aheejin May 10, 2022 •
edited
Loading

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)

}
''', assert_returncode=NON_ZERO)
self.assertNotContained('Maximum call stack size exceeded', err)

def test_iostream_ctors(self):
# iostream stuff must be globally constructed before user global
# constructors, so iostream works in global constructors
Expand Down
3 changes: 2 additions & 1 deletion tools/system_libs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,7 +708,8 @@ class libcompiler_rt(MTLibrary, SjLjLibrary):
'stack_ops.S',
'stack_limits.S',
'emscripten_setjmp.c',
'emscripten_exception_builtins.c'
'emscripten_exception_builtins.c',
'__trap.c'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a trailing comma.. to make future additions cleaner.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

])


Expand Down