[embind] Allow awaiting Promises in non-Promise coroutines & catching Promise C++ exceptions

[stevenwdv]

Still crashes on test_embind_val_coro_catch_cpp_exception, not sure why due to #26213. EDIT: resolved.

TODO:

• What to do on rejection in the !__cpp_exceptions case in a non-Promise coroutine?
  • I'll just rethrow the JS exception in await_resume anyway.
• And should a non-C++ exception immediately reject a Promise since we cannot catch it (Allow catching JavaScript exceptions from C++ #11496)? Can we even distinguish C++ exceptions from non-C++ ones?
  • Seems hard to distinguish since there's 3 possible exception types depending on the compiler settings, once of which is just a number.
  • We need to, as otherwise test_embind_val_coro_propagate_js_error fails

Fixes: #26064
Fixes: #25396

[stevenwdv]

Btw, if anyone knows why the rethrow of the val wrapping a std::runtime_error in await_resume may be crashing, let me know, because I haven't figured it out.

[stevenwdv]

The cause of the crash seems to be #26213

[stevenwdv]

Currently -fno-exceptions tests seem to fail on #26290, although it could be that there's another cause that __cxa_throw gets linked in despite exceptions being disabled..?

[stevenwdv]

Currently -fno-exceptions tests seem to fail on #26290, although it could be that there's another cause that __cxa_throw gets linked in despite exceptions being disabled..?

Actually, the error message for test_embind_val_coro_propagate_js_error_noexcept, besides complaining about exceptionLast, also says:

error: DISABLE_EXCEPTION_THROWING was set (likely due to -fno-exceptions), which means no C++ exception throwing support code is linked in, but such support is required by symbol '__cxa_throw'. Either do not set DISABLE_EXCEPTION_THROWING (if you do want exception throwing) or compile all source files with -fno-exceptions (so that no exceptions support code is required); also make sure DISABLE_EXCEPTION_CATCHING is set to the right value - if you want exceptions, it should be off, and vice versa.

But I don't see where I'm referencing __cxa_throw? @aheejin sorry for mentioning you, would you know more about this? (In this test I'm intentionally disabling exceptions to test if Promises still reject when awaiting promises rejected by JS errors.)

---

Besides that test_embind_val_coro_catch_cpp_exception apparently now fails, but I haven't looked into that yet.

[aheejin]

error: DISABLE_EXCEPTION_THROWING was set (likely due to -fno-exceptions), which means no C++ exception throwing support code is linked in, but such support is required by symbol '__cxa_throw'. Either do not set DISABLE_EXCEPTION_THROWING (if you do want exception throwing) or compile all source files with -fno-exceptions (so that no exceptions support code is required); also make sure DISABLE_EXCEPTION_CATCHING is set to the right value - if you want exceptions, it should be off, and vice versa.

But I don't see where I'm referencing __cxa_throw? @aheejin sorry for mentioning you, would you know more about this? (In this test I'm intentionally disabling exceptions to test if Promises still reject when awaiting promises rejected by JS errors.)

Sorry for the delay. I submitted #26519 that should fix #26290. About test_embind_val_coro_propagate_js_error_noexcept, I asked Gemini about it, and its answer was:

When std::get fails because the variant holds a different type, it throws std::bad_variant_access by calling std::__throw_bad_variant_access(). Since emscripten/val.h is heavily used within Embind, including inside system/lib/embind/bind.cpp (which is compiled globally into Emscripten's libembind-rtti.a with exceptions enabled), the compiler instantiated the throwing version of __throw_bad_variant_access() in bind.o, inserting a dependency on __cxa_throw. When your test links against Embind but supplies -fno-exceptions, it removes the __cxa_throw implementation from the linked C++ standard library, leading to the missing symbol error.

The Fix:
I replaced all uses of std::get with std::get_if inside val::awaiter. By doing this, we bypass the potential for std::bad_variant_access and __cxa_throw entirely. Instead, if a state mismatch occurs, we just assert() that the pointer returned by std::get_if is valid.

And the below is its suggested fix. I'm not familiar with embind so I'm not sure how much this makes sense though. But it seems to fix the error.

diff --git a/system/include/emscripten/val.h b/system/include/emscripten/val.h
index bfe6c47bf..ecb7d017c 100644
--- a/system/include/emscripten/val.h
+++ b/system/include/emscripten/val.h
@@ -690,7 +690,9 @@ class val::awaiter {
   > state;
 
   void awaitSuspendImpl(state_coro coro) {
-    internal::_emval_coro_suspend(std::get<state_promise>(state).promise.as_handle(), this);
+    auto* promise_ptr = std::get_if<state_promise>(&state);
+    assert(promise_ptr);
+    internal::_emval_coro_suspend(promise_ptr->promise.as_handle(), this);
     state.emplace<state_coro>(coro);
   }
 
@@ -720,7 +722,9 @@ public:
   // When JS invokes `resume_with` with some value, store that value and resume
   // the coroutine.
   void resume_with(val&& result) {
-    auto coro = std::get<state_coro>(state);
+    auto* coro_ptr = std::get_if<state_coro>(&state);
+    assert(coro_ptr);
+    auto coro = *coro_ptr;
     state.emplace<state_result>(std::move(result));
     coro.handle.resume();
   }
@@ -737,7 +741,10 @@ public:
       return std::move(result->result);
     }
     // If a JS exception ended up here, it will be uncaught as C++ code cannot catch it
-    std::get<state_error>(state).error.throw_();
+    auto* error_ptr = std::get_if<state_error>(&state);
+    assert(error_ptr);
+    error_ptr->error.throw_();
+    __builtin_unreachable();
   }
 };
 
@@ -799,7 +806,9 @@ public:
 };
 
 inline void val::awaiter::reject_with(val&& error) {
-  auto coro = std::get<state_coro>(state);
+  auto* coro_ptr = std::get_if<state_coro>(&state);
+  assert(coro_ptr);
+  auto coro = *coro_ptr;
 
 
   if (coro.isValPromise) {

[stevenwdv]

@aheejin Thanks! Your suggestions seem to fix the problems I was having! I'm surprised Gemini was actually able to find this.

Let's see if the CI builds fine now.

[stevenwdv]

Almost. Still some failures:

• Codesize Checks fail. I don't think I have permission to run the rebaseline workflow?
• test-wasm2js1 fails on test_embind_val_coro_catch_cpp_exception with "wasm2js cannot convert (try [...])", but I don't know enough to know why. Can someone help? I'll add a decorator to the test.
• test-windows fails on test_other.other.test_pkg_config_ports with "pkg-config is required to run this test". This seems unrelated to this MR?

[aheejin]

• Codesize Checks fail. I don't think I have permission to run the rebaseline workflow?

Running ./tools/maint/rebaseline_tests.py will create a codesize-rebaseline commit, which you can push. Your toolchain needs to be emsdk tot to get accurate results.

[stevenwdv]

@aheejin Ah, thanks, missed that. It's still not working though, but it literally says:

The following (2) test expectation files were updated by
running the tests with --rebaseline:

codesize/test_codesize_minimal_pthreads.json: 26409 => 26409 [+0 bytes / +0.00%]
codesize/test_codesize_minimal_pthreads_memgrowth.json: 26812 => 26812 [+0 bytes / +0.00%]

Average change: +0.00% (+0.00% - +0.00%)

Test expectations are out-of-date on the PR branch.

This seems a little odd...

[brendandahl]

Minor nit on a comment, but looks good overall.

[stevenwdv]

Huh, again I did the code size rebaseline after installing the latest tot but it's still complaining

[sbc100]

I just landed #26651 .. if you rebase on top of that the codesize tests should match again.

[sbc100]

@brendandahl we can land this without the rebase if you are good with it?

[sbc100]

Actually we probably should rebase because this change can effect codesize I think

[sbc100]

lgtm, but I really don't understand the C++ coro stuff, so I'll leave it to @RReverser or @brendandahl to give final approval.