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

aheejin · 2022-05-07T00:17:22Z

We used to implement abort() with throwing a JS exception. #9730
changed it to RuntimeError, because it is the class a trap becomes
once it hits a JS frame.

But this turned out to be not working, 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. Relevant past discussion:
WebAssembly/exception-handling#89 (comment)

So at the moment we don't have a way of throwing a trap from JS, which
is inconvenient. I think we eventually want to make JS API for this, but
for the moment, this PR resorts to a wasm function that traps. This at
least makes calling std::terminate work. So far calling it exhausted
the call stack and aborted.

Fixes #16407.

- `do_run` also sets `expected_output=None`, like `do_runf`. - `do_run`, `do_run_from_file`, and `do_run_in_out_file_test` now return the stdout+stderr output, like `do_runf`.

We used to implement `abort()` with throwing a JS exception. emscripten-core#9730 changed it to `RuntimeError`, because it is the class a trap becomes once it hits a JS frame. But this turned out to be not working, because Wasm EH currently does not assume all `RuntimeError`s are from traps; it decides whether a `RuntimeError` is from a trap or not based on a hidden field within the object. Relevant past discussion: WebAssembly/exception-handling#89 (comment) So at the moment we don't have a way of throwing a trap from JS, which is inconvenient. I think we eventually want to make JS API for this, but for the moment, this PR resorts to a wasm function that traps. This at least makes calling `std::terminate` work. So far calling it exhausted the call stack and aborted. Fixes emscripten-core#16407.

aheejin · 2022-05-07T00:17:58Z

This depends on #16909 and also contains the diff of #16909. Will rebase it after it lands.

sbc100

I think there are two different notions of abort() in emscripten:

Native code calls abort()
JS code calls abort()

When native code calls abort() we currently map that to JS library function (also called abort() which when calls the JS handler for abort()).

What if we just implemented the native abort() in C and only use the JS abort for JS aborts.. would that work? that sounds like a good idea for code size anyway since it would avoid the extra JS import.

In that JS code that calls abort() would get a different behaviour.. but I think that maybe OK? What exactly is the observable differences between JS throwing and wasm trapping?

sbc100 · 2022-05-07T00:21:10Z

system/lib/compiler-rt/exception_builtins.S

+.globl __trap
+
+__trap:
+  .functype __trap() -> ()


I think you can just write this in C as void __trap() { __builtin_trap() }

sbc100 · 2022-05-07T00:21:44Z

tests/test_core.py

+    err = self.do_run(r'''
+#include <exception>
+int main() {
+  std::terminate();


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

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.

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.

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)

aheejin · 2022-05-07T02:08:02Z

I think there are two different notions of abort() in emscripten:

Native code calls abort()

JS code calls abort()

When native code calls abort() we currently map that to JS library function (also called abort() which when calls the JS handler for abort()).

What if we just implemented the native abort() in C and only use the JS abort for JS aborts.. would that work? that sounds like a good idea for code size anyway since it would avoid the extra JS import.

What are the cases that JS code calls abort? Would those cases be OK to throw JS exceptions that can be caught by Wasm catches?

In that JS code that calls abort() would get a different behaviour.. but I think that maybe OK? What exactly is the observable differences between JS throwing and wasm trapping?

Yeah because one prints something like "exception thrown" and the other prints "unreachable" reached, so the message will be little different. To be precise, the original message:

Aborted(native code called abort())
exiting due to exception: RuntimeError: Aborted(native code called abort()),RuntimeError: Aborted(native code called abort())

The message with wasm trap:

Aborted(native code called abort())
exiting due to exception: RuntimeError: unreachable,RuntimeError: unreachable

It's not ideal, but I think better than the current

Aborted(native code called abort())
Aborted(native code called abort())
...
Aborted(native code called abort())
Aborted(native code called abort())
exiting due to exception: RangeError: Maximum call stack size exceeded,RangeError:
 Maximum call stack size exceeded

sbc100 · 2022-05-09T16:59:07Z

I think there are two different notions of abort() in emscripten:

Native code calls abort()

JS code calls abort()

When native code calls abort() we currently map that to JS library function (also called abort() which when calls the JS handler for abort()).
What if we just implemented the native abort() in C and only use the JS abort for JS aborts.. would that work? that sounds like a good idea for code size anyway since it would avoid the extra JS import.

What are the cases that JS code calls abort? Would those cases be OK to throw JS exceptions that can be caught by Wasm catches?

Think like calling assert from JS. Other examples you can find like this:

$ git grep "abort('" *.js
...
src/library_async.js:        abort('invalid state: ' + Asyncify.state);
src/library_browser.js:      abort('Module.requestFullScreen has been replaced by Module.requestFullscreen (without a capital S)');
src/library_exceptions.js:      abort('no exception to throw');
src/library_fs.js:      abort('FS.absolutePath has been removed; use PATH_FS.resolve instead');
src/library_fs.js:      abort('FS.createFolder has been removed; use FS.mkdir instead');
src/library_fs.js:      abort('FS.createLink has been removed; use FS.symlink instead');
src/library_fs.js:      abort('FS.joinPath has been removed; use PATH.join instead');
src/library_fs.js:      abort('FS.mmapAlloc has been replaced by the top level function mmapA
...

I wouldn't expect those to be caught by anybody.. but that same is true for all abort calls. They signal the program is not longer running/runnable.

In that JS code that calls abort() would get a different behaviour.. but I think that maybe OK? What exactly is the observable differences between JS throwing and wasm trapping?

Yeah because one prints something like "exception thrown" and the other prints "unreachable" reached, so the message will be little different. To be precise, the original message:
Aborted(native code called abort())
exiting due to exception: RuntimeError: Aborted(native code called abort()),RuntimeError: Aborted(native code called abort())
The message with wasm trap:
Aborted(native code called abort())
exiting due to exception: RuntimeError: unreachable,RuntimeError: unreachable
It's not ideal, but I think better than the current

Yeah, I think for debug builds its important to keep the nice messages, but maybe not for release builds.

Aborted(native code called abort())
Aborted(native code called abort())
...
Aborted(native code called abort())
Aborted(native code called abort())
exiting due to exception: RangeError: Maximum call stack size exceeded,RangeError:
 Maximum call stack size exceeded

kripken

lgtm, looks like the right approach!

Some minor comment suggestions, feel free to accept what you think makes sense.

Separately (I mean, not urgent in this PR), I like @sbc100 's idea to implement abort() without any JS in release builds, as a simplification&optimization there.

Also separately, since this PR doesn't affect any metadce test expections, I guess we don't have any metadce tests with wasm EH? It might be a good idea to add one, maybe around here:

emscripten/tests/test_other.py

Lines 7340 to 7341 in 5584dd1

    
               'except':   (['-O2', '-fexceptions'],    [], ['waka']), # noqa 
        
               # exceptions does not pull in demangling by default, which increases code size

tests/test_core.py

src/preamble.js

aheejin · 2022-05-09T19:15:18Z

Hmm, I think I have another simpler approach. Let me upload that first.

aheejin · 2022-05-09T23:56:38Z

Sorry, nevermind what I wrote and deleted a few minutes ago.

sbc100 · 2022-05-09T23:56:03Z

system/lib/compiler-rt/exception_builtins.c

@@ -0,0 +1,3 @@
+void __trap() {
+  __builtin_trap();


Lets just call this file __trap.c for now?

sbc100 · 2022-05-09T23:59:52Z

tests/test_core.py

+    err = self.do_run(r'''
+#include <exception>
+int main() {
+  std::terminate();


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.

Co-authored-by: Alon Zakai <alonzakai@gmail.com>

aheejin · 2022-05-10T00:37:26Z

I think #16921 can be a simpler approach. That version of abort unwinds the stack, but maybe it's OK?

Like 30 mins ago I posted something along the line of #16910 (comment), but deleted it after that. I was confused; currently catch (...) doesn't catch foreign exceptions. I've been debating about whether we should make catch (...) catch foreign exceptions. I think that's something we can decide, but the current impl doesn't do that, so abort will only ever be caught by cleanups.

sbc100 · 2022-05-10T00:44:04Z

I think #16921 can be a simpler approach. That version of abort unwinds the stack, but maybe it's OK?

Like 30 mins ago I posted something along the line of #16910 (comment), but deleted it after that. I was confused; currently catch (...) doesn't catch foreign exceptions. I've been debating about whether we should make catch (...) catch foreign exceptions. I think that's something we can decide, but the current impl doesn't do that, so abort will only ever be caught by cleanups.

I see, so then maybe something like then:

test_abort_no_cleanup.cpp:

class Foo {
   ~Foo() { printf("should never print\n"); }
};

int main() {
  Foo a;
  try {
     abort();
  catch(...) {
  }
}

aheejin · 2022-05-10T01:16:32Z

I see, so then maybe something like then:

test_abort_no_cleanup.cpp:
class Foo {
   ~Foo() { printf("should never print\n"); }
};

int main() {
  Foo a;
  try {
     abort();
  catch(...) {
  }
}

#16921 will print "should never print". That's because abort will now unwind the stack. This PR (#16910) will NOT print "shoulnd never print", because it's a trap and traps are not catchable by any instructions including catch_all(cleanup)s.

If we are not OK with the unwinding behavior (#16921), we should stick to this PR I guess. I assumed abort shouldn't unwind the stack, but the meeting this morning, if I remember correctly, it was suggested that maybe it's OK for abort to unwind the stack, so I came up with #16921. But I'm not really sure which one we should choose; maybe the same behavior with native platforms (this PR) will be less confusing.

sbc100 · 2022-05-10T01:23:41Z

Sorry for the confusion. abort does indeed have to unwind back the JS event loop somehow.. but it should not run any destructors. It needs to unwind without any cleanup/destructors being called.

aheejin · 2022-05-10T01:46:02Z

Sorry for the confusion. abort does indeed have to unwind back the JS event loop somehow.. but it should not run any destructors. It needs to unwind without any cleanup/destructors being called.

I think that's a separate matter? #16871 was to make a note that it's not working at the moment. Yeah so in that case we need an exception that does not run destructors, which we currently don't have. But isn't that different from abort? Do we use abort in emscripten_set_main_loop?

In another way, we can solve this by creating a JS tag and making the toolchain to add catch $js_tag / rethrow to every catch_all so that they can bypass destructors. This will increase code size though.

sbc100 · 2022-05-10T02:28:48Z

Sorry for the confusion. abort does indeed have to unwind back the JS event loop somehow.. but it should not run any destructors. It needs to unwind without any cleanup/destructors being called.

I think that's a separate matter? #16871 was to make a note that it's not working at the moment. Yeah so in that case we need an exception that does not run destructors, which we currently don't have. But isn't that different from abort? Do we use abort in emscripten_set_main_loop?

In another way, we can solve this by creating a JS tag and making the toolchain to add catch $js_tag / rethrow to every catch_all so that they can bypass destructors. This will increase code size though.

For emscripten_set_main_loop we do a throw 'unwind' from JS. That should not run destructors either IIUC.

aheejin · 2022-05-10T02:54:43Z

Sorry for the confusion. abort does indeed have to unwind back the JS event loop somehow.. but it should not run any destructors. It needs to unwind without any cleanup/destructors being called.

I think that's a separate matter? #16871 was to make a note that it's not working at the moment. Yeah so in that case we need an exception that does not run destructors, which we currently don't have. But isn't that different from abort? Do we use abort in emscripten_set_main_loop?
In another way, we can solve this by creating a JS tag and making the toolchain to add catch $js_tag / rethrow to every catch_all so that they can bypass destructors. This will increase code size though.

For emscripten_set_main_loop we do a throw 'unwind' from JS. That should not run destructors either IIUC.

Yeah that's not working now, which is the reason I commented about it in #16871. But that's not abort, right? So we should decide whether we should allow abort to run destructors or not.

aheejin · 2022-05-10T03:35:34Z

@kripken

Also separately, since this PR doesn't affect any metadce test expections, I guess we don't have any metadce tests with wasm EH? It might be a good idea to add one, maybe around here:

emscripten/tests/test_other.py

Lines 7340 to 7341 in 5584dd1

'except': (['-O2', '-fexceptions'], [], ['waka']), # noqa

# exceptions does not pull in demangling by default, which increases code size

Done in #16924.

sbc100 · 2022-05-10T03:59:18Z

Sorry for the confusion. abort does indeed have to unwind back the JS event loop somehow.. but it should not run any destructors. It needs to unwind without any cleanup/destructors being called.

I think that's a separate matter? #16871 was to make a note that it's not working at the moment. Yeah so in that case we need an exception that does not run destructors, which we currently don't have. But isn't that different from abort? Do we use abort in emscripten_set_main_loop?
In another way, we can solve this by creating a JS tag and making the toolchain to add catch $js_tag / rethrow to every catch_all so that they can bypass destructors. This will increase code size though.

For emscripten_set_main_loop we do a throw 'unwind' from JS. That should not run destructors either IIUC.

Yeah that's not working now, which is the reason I commented about it in #16871. But that's not abort, right? So we should decide whether we should allow abort to run destructors or not.

abort is defined not run destructors, or any kind of cleanup to flushing, and I think that pretty fundamental/important.

Are you saying that abort is not working this way even with emscripten-sjlj exceptions? Or are you just talking about with wasm exceptions?

aheejin · 2022-05-10T04:41:57Z

abort is defined not run destructors, or any kind of cleanup to flushing, and I think that pretty fundamental/important.

I thought so too, but I think it was suggested by Dan that abort can maybe unwind the stack, so I explored that possibility. But maybe I was mistaken about what he said?

Are you saying that abort is not working this way even with emscripten-sjlj exceptions? Or are you just talking about with wasm exceptions?

With Emscripten EH / SjLj, everything works fine. abort is not caught by wasm.

With Wasm EH,
With this PR (#16910), abort traps, so it is not caught by wasm, and the stack is not unwound as you said it should. But I'm not sure if we can use this __trap function in emscripten_set_main_loop too. I'm not very familiar with that function.
With #16921, abort unwinds the stack, which I guess you wouldn't want.
We can land one of these two.

sbc100 · 2022-05-10T05:23:06Z

abort is defined not run destructors, or any kind of cleanup to flushing, and I think that pretty fundamental/important.

I thought so too, but I think it was suggested by Dan that abort can maybe unwind the stack, so I explored that possibility. But maybe I was mistaken about what he said?

I think that was in the context of pthead_exit rather than abort.. but I didn't think that the discussion of unwinding implied the running destructors. Maybe I'm not up on the terminology but I thought we could talk about "unwinding the stack" independently of whether the running C++ destructors are run as we do it. Its still called "unwinding" either way isn't it? Or am I wrong about there?

At least in emscripten today throw the string "unwind" when we don't want destructors run.

aheejin · 2022-05-10T07:46:08Z

Then replace "unwinding the stack" with "running destructors" in #16910 (comment) and that's what I meant. I meant running destructors all along. Nevermind unwinding for now.

Anyway, as I said, this PR does not run destructors.
But I'm not sure we can use this __trap for emscripten_set_main_loop.

dschuff · 2022-05-10T18:46:34Z

After thinking about this a bit more, IMO it would be better not to have abort or std::terminate call destructors. These functions really are kind of like wasm's trap; i.e. something has gone very wrong and the program cannot continue (and perhaps may even be in an inconsistent state). Calling abort instead of something like exit(1) means that the programmer has decided that they don't want any atexit handlers to run either. So I think we should try not to run any more user code if we can.

dschuff · 2022-05-10T18:50:38Z

Actually, I went and looked up abort and std::terminate in the standards. std::terminate is more interesting than I thought:
Check out § 18.5.1 of C++17 and §7.22.4.1 of C11. C++17 lists a 13 cases where std::terminate is called, and then has the following:

In such cases, std::terminate() is called (21.8.4). In the situation where no matching handler is found,
it is implementation-defined whether or not the stack is unwound before std::terminate() is called. In
the situation where the search for a handler (18.3) encounters the outermost block of a function with a
non-throwing exception specification (18.4), it is implementation-defined whether the stack is unwound,
unwound partially, or not unwound at all before std::terminate() is called. In all other situations, the
stack shall not be unwound before std::terminate() is called. An implementation is not permitted to finish
stack unwinding prematurely based on a determination that the unwind process will eventually cause a call
to std::terminate().

My read on that is that it's always OK not to unwind the stack and sometimes required.

sbc100 · 2022-05-10T18:59:25Z

Actually, I went and looked up abort and std::terminate in the standards. std::terminate is more interesting than I thought: Check out § 18.5.1 of C++17 and §7.22.4.1 of C11. C++17 lists a 13 cases where std::terminate is called, and then has the following:
In such cases, std::terminate() is called (21.8.4). In the situation where no matching handler is found,
it is implementation-defined whether or not the stack is unwound before std::terminate() is called. In
the situation where the search for a handler (18.3) encounters the outermost block of a function with a
non-throwing exception specification (18.4), it is implementation-defined whether the stack is unwound,
unwound partially, or not unwound at all before std::terminate() is called. In all other situations, the
stack shall not be unwound before std::terminate() is called. An implementation is not permitted to finish
stack unwinding prematurely based on a determination that the unwind process will eventually cause a call
to std::terminate().
My read on that is that it's always OK not to unwind the stack and sometimes required.

All the seems to be about what to do before one calls std::terminate isn't it? i.e. it not about what std::terminate itself does but about if/when std::terminate is called by the runtime. If one actually calls std::terminate or abort or _exit .. then one should never run any destructors (or any more user code).

aheejin · 2022-05-10T20:22:35Z

Yeah, reading it also convinces me that we shouldn't call destructors after all. Thanks for the reference. I guess we can land this and abandon #16921?

sbc100

lgtm % comments

sbc100 · 2022-05-10T21:24:31Z

tools/system_libs.py

@@ -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'


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

sbc100 · 2022-05-10T21:25:57Z

tests/test_core.py

@@ -1639,6 +1639,20 @@ class Polymorphic {virtual void member(){}};
 }
 ''', 'exception caught: std::bad_typeid')

+  @with_both_eh_sjlj
+  def test_terminate_abort(self):


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

sbc100 · 2022-05-10T21:28:23Z

src/preamble.js

@@ -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;


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?

Yeah I didn't do that in this PR because

This changes the error message slightly for existing users, which I don't think is a big deal, but just to be conservative

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?

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.

When landing emscripten-core#16910, I didn't rebase it onto emscripten-core#16924 so they raced in the commit CI. This reflects the changes in emscripten-core#16910.

We decided use a trap for `abort` function in case of Wasm EH in order to prevent infinite-looping (emscripten-core#16910). https://github.com/emscripten-core/emscripten/blob/44b2c2a1ecad39c534e14179acb49419dfee528b/src/preamble.js#L472-L474 The short reason is, in Wasm EH `RuntimeError` is treated as a foreign exception and caught by `catch_all`, so you try to abort the program and throw a `RuntimeError`, but it is caught by some `catch_all` used in the cleanup (=destructors, etc) code, causing the program to continue to run. `__trap` is defined in compiler-rt and exported when Wasm EH is enabled. This has worked so far, but in case we fail to instantiate a Wasm module and call `abort` because of it, we don't have access to the imported `Module['asm']['__trap']`, because `Module['asm']` has not been set: https://github.com/emscripten-core/emscripten/blob/44b2c2a1ecad39c534e14179acb49419dfee528b/src/preamble.js#L848 So the `abort` call will ends up like this: ```console ... TypeError: Cannot read properties of undefined (reading '__trap') at ___trap (/usr/local/google/home/aheejin/test/gl84/exported_api.js:5152:34) at abort (/usr/local/google/home/aheejin/test/gl84/exported_api.js:892:5) at /usr/local/google/home/aheejin/test/gl84/exported_api.js:1172:5 ``` which may not be the worst thing in the world given that we are crashing anyway, but not the situation we intended. This PR lets us throw `RuntimeError` in case we don't have the wasm module instantiated and have access to `Module['asm']['__trap']`. This is OK even with Wasm EH because we haven't been running the module, there's no risk of infinite-looping we tried to prevent in emscripten-core#16910. I'm not sure how to add a test for this, because the test would require a module that fails to instantiate. This was found while I was debugging something else.

We decided use a trap for `abort` function in case of Wasm EH in order to prevent infinite-looping (emscripten-core#16910). https://github.com/emscripten-core/emscripten/blob/44b2c2a1ecad39c534e14179acb49419dfee528b/src/preamble.js#L472-L474 The short reason is, in Wasm EH `RuntimeError` is treated as a foreign exception and caught by `catch_all`, so you try to abort the program and throw a `RuntimeError`, but it is caught by some `catch_all` used in the cleanup (=destructors, etc) code, causing the program to continue to run. `__trap` is defined in compiler-rt and exported when Wasm EH is enabled. This has worked so far, but in case we fail to instantiate a Wasm module and call `abort` because of it, we don't have access to the imported `Module['asm']['__trap']`, because `Module['asm']` has not been set: https://github.com/emscripten-core/emscripten/blob/44b2c2a1ecad39c534e14179acb49419dfee528b/src/preamble.js#L848 So the `abort` call will ends like this: ```console TypeError: Cannot read properties of undefined (reading '__trap') at ___trap (/usr/local/google/home/aheejin/test/gl84/exported_api.js:5152:34) at abort (/usr/local/google/home/aheejin/test/gl84/exported_api.js:892:5) at /usr/local/google/home/aheejin/test/gl84/exported_api.js:1172:5 ``` which may not be the worst thing in the world given that we are crashing anyway, but not the situation we intended. This PR lets us throw `RuntimeError` in case we don't have the wasm module instantiated and have access to `Module['asm']['__trap']`. This is OK even with Wasm EH because we haven't been running the module, there's no risk of infinite-looping we tried to prevent in emscripten-core#16910. I'm not sure how to add a test for this, because the test would require a module that fails to instantiate. This was found while I was debugging something else.

We decided use a trap for `abort` function in case of Wasm EH in order to prevent infinite-looping (emscripten-core#16910). https://github.com/emscripten-core/emscripten/blob/44b2c2a1ecad39c534e14179acb49419dfee528b/src/preamble.js#L472-L474 The short reason is, in Wasm EH `RuntimeError` is treated as a foreign exception and caught by `catch_all`, so you try to abort the program and throw a `RuntimeError`, but it is caught by some `catch_all` used in the cleanup (=destructors, etc) code, causing the program to continue to run. `__trap` is defined in compiler-rt and exported when Wasm EH is enabled. This has worked so far, but in case we fail to instantiate a Wasm module and call `abort` because of it, we don't have access to the imported `Module['asm']['__trap']`, because `Module['asm']` has not been set: https://github.com/emscripten-core/emscripten/blob/44b2c2a1ecad39c534e14179acb49419dfee528b/src/preamble.js#L848 So the `abort` call will end like this: ```console TypeError: Cannot read properties of undefined (reading '__trap') at ___trap (/usr/local/google/home/aheejin/test/gl84/exported_api.js:5152:34) at abort (/usr/local/google/home/aheejin/test/gl84/exported_api.js:892:5) at /usr/local/google/home/aheejin/test/gl84/exported_api.js:1172:5 ``` which may not be the worst thing in the world given that we are crashing anyway, but not the situation we intended. This PR lets us throw `RuntimeError` in case we don't have the wasm module instantiated and have access to `Module['asm']['__trap']`. This is OK even with Wasm EH because we haven't been running the module, there's no risk of infinite-looping we tried to prevent in emscripten-core#16910. I'm not sure how to add a test for this, because the test would require a module that fails to instantiate. This was found while I was debugging something else.

We decided use a trap for `abort` function in case of Wasm EH in order to prevent infinite-looping (#16910). https://github.com/emscripten-core/emscripten/blob/44b2c2a1ecad39c534e14179acb49419dfee528b/src/preamble.js#L472-L474 The short reason is, in Wasm EH `RuntimeError` is treated as a foreign exception and caught by `catch_all`, so you try to abort the program and throw a `RuntimeError`, but it is caught by some `catch_all` used in the cleanup (=destructors, etc) code, causing the program to continue to run. `__trap` is defined in compiler-rt and exported when Wasm EH is enabled. This has worked so far, but in case we fail to instantiate a Wasm module and call `abort` because of it, we don't have access to the imported `Module['asm']['__trap']`, because `Module['asm']` has not been set: https://github.com/emscripten-core/emscripten/blob/44b2c2a1ecad39c534e14179acb49419dfee528b/src/preamble.js#L848 So the `abort` call will end like this: ```console TypeError: Cannot read properties of undefined (reading '__trap') at ___trap (/usr/local/google/home/aheejin/test/gl84/exported_api.js:5152:34) at abort (/usr/local/google/home/aheejin/test/gl84/exported_api.js:892:5) at /usr/local/google/home/aheejin/test/gl84/exported_api.js:1172:5 ``` which may not be the worst thing in the world given that we are crashing anyway, but not the situation we intended. This PR lets us throw `RuntimeError` in case we don't have the wasm module instantiated and have access to `Module['asm']['__trap']`. This is OK even with Wasm EH because we haven't been running the module, there's no risk of infinite-looping we tried to prevent in #16910.

aheejin added 2 commits May 6, 2022 16:13

Make do_run tests functions consistent

f650912

- `do_run` also sets `expected_output=None`, like `do_runf`. - `do_run`, `do_run_from_file`, and `do_run_in_out_file_test` now return the stdout+stderr output, like `do_runf`.

aheejin requested review from kripken, sbc100 and dschuff May 7, 2022 00:17

sbc100 reviewed May 7, 2022

View reviewed changes

Merge branch 'main' into terminate

a50f2dc

Implement __trap in C

dd74bc5

kripken approved these changes May 9, 2022

View reviewed changes

tests/test_core.py Outdated Show resolved Hide resolved

src/preamble.js Outdated Show resolved Hide resolved

src/preamble.js Outdated Show resolved Hide resolved

src/preamble.js Outdated Show resolved Hide resolved

src/preamble.js Outdated Show resolved Hide resolved

sbc100 reviewed May 9, 2022

View reviewed changes

Apply suggestions from code review

f4e389a

Co-authored-by: Alon Zakai <alonzakai@gmail.com>

aheejin mentioned this pull request May 10, 2022

[EH] Make std::terminate() work with EH #16921

Closed

exception_builtins.c -> __trap.c

b6fc45f

sbc100 reviewed May 10, 2022

View reviewed changes

Address comments

9ee27ca

sbc100 approved these changes May 11, 2022

View reviewed changes

aheejin merged commit 4cf36b6 into emscripten-core:main May 11, 2022

aheejin deleted the terminate branch May 11, 2022 01:54

aheejin mentioned this pull request May 11, 2022

[EH] Update Wasm EH metadce test after #16910 #16935

Merged

aheejin added a commit that referenced this pull request May 11, 2022

[EH] Update Wasm EH metadce test after #16910 (#16935)

d864146

aheejin mentioned this pull request Feb 4, 2023

Use RuntimeError in abort() before module instantiation #18664

Merged

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

	'except': (['-O2', '-fexceptions'], [], ['waka']), # noqa
	# exceptions does not pull in demangling by default, which increases code size

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

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

Conversation

aheejin commented May 7, 2022

aheejin commented May 7, 2022

sbc100 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aheejin May 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aheejin May 10, 2022 • edited Loading

Choose a reason for hiding this comment

aheejin commented May 7, 2022

sbc100 commented May 9, 2022

kripken left a comment

Choose a reason for hiding this comment

aheejin commented May 9, 2022

aheejin commented May 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aheejin commented May 10, 2022

sbc100 commented May 10, 2022

aheejin commented May 10, 2022 • edited Loading

sbc100 commented May 10, 2022

aheejin commented May 10, 2022

sbc100 commented May 10, 2022

aheejin commented May 10, 2022

aheejin commented May 10, 2022 • edited Loading

sbc100 commented May 10, 2022

aheejin commented May 10, 2022

sbc100 commented May 10, 2022

aheejin commented May 10, 2022 • edited Loading

dschuff commented May 10, 2022

dschuff commented May 10, 2022

sbc100 commented May 10, 2022

aheejin commented May 10, 2022

sbc100 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aheejin May 7, 2022 •

edited

Loading

aheejin May 10, 2022 •

edited

Loading

aheejin commented May 10, 2022 •

edited

Loading

aheejin commented May 10, 2022 •

edited

Loading

aheejin commented May 10, 2022 •

edited

Loading