[EH] Move EH status management to exceptions.js - #27528
Conversation
emscripten-core#27498 moved EH status management (`uncaughtExceptionCount` and `exceptionLast` from `libexceptions.js` to `libunwind.js`, but because in the native libcxxabi and libunwind, the management is done within libcxxabi, I think it'd be more consistent if we do the same for the JS libraries.
| ptr = exceptionLast ?? new CppException(ptr); | ||
| if (!exceptionLast) { | ||
| exceptionLast = new CppException(ptr); | ||
| } |
There was a problem hiding this comment.
I think you can write this as ptr = exceptionLast ??= new CppException(ptr) ?
| info.init(type, destructor); | ||
| #if !DISABLE_EXCEPTION_CATCHING | ||
| ___cxa_increment_exception_refcount(ptr); | ||
| ptr = new CppException(ptr); |
There was a problem hiding this comment.
I wonder if the new CppException stuff should go in libunwind ? Doesn't have to part of this PR, but really CppException here should probably be called something like NativeException right?
Maybe CppException is fine since we already have __cpp_exception tag (which is used for all LLVM languages not just c++)
There was a problem hiding this comment.
CppException is in the area of libcxxabi, which manages C++ exceptions, no? libunwind is basically language-agnostic; it just provides the low-level functions to throw something and unwind stack.
The reason Rust team proposed to move __cpp_exception to libunwind was for some other reasons (llvm/llvm-project#185770). Also they seem to be using __cpp_exceptions tag for Rust exceptions too (rust-lang/rust#160067 (comment)), presumably because it was easier for them to use it than adding another tag.
There was a problem hiding this comment.
Thats what I'm saying. I think that CppException is just and object we use the wrap native exceptions, as in any exception that comes out of wasm/llvm. So both rust and C++ share libunwind they probably both want the JS wrapper object around the native exception (to get backtraces).
I think CppException and __cpp_exception are both just slightly unfortunately named. They probably should have been called __libunwind_exception and LibunwindException maybe? Or something else that is not C++ specific I guess?
There was a problem hiding this comment.
The purpose of CppException if really most like JSWrapperForNativeException I think??
There was a problem hiding this comment.
I think they were intended to represent C++ exceptions. __cpp_exception is a tag, and EH instructions use it to check whether the current exception is a C++ exception or a foreign one. And CppException is a container to manage EH stack traces (which libunwind is not really supposed to know about) and also it is to distinguish the exception from EmscriptenSjLj: https://github.com/emscripten-core/emscripten/blob/main/src/runtime_exceptions.js
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/env python3 | |||
| #!/usr/bin/env python3SDfdlskjfdskl | |||
|
|
||
| var LibraryExceptions = { | ||
| #if !WASM_EXCEPTIONS | ||
| $uncaughtExceptionCount: '0', |
There was a problem hiding this comment.
I think this can just be the number zero rather than a string?
This is an automatic change generated by tools/maint/rebaseline_tests.py. The following (10) test expectation files were updated by running the tests with `--rebaseline`: ``` codesize/test_codesize_cxx_ctors1.json: 153937 => 153947 [+10 bytes / +0.01%] codesize/test_codesize_cxx_ctors2.json: 153343 => 153353 [+10 bytes / +0.01%] codesize/test_codesize_cxx_except.json: 200109 => 200106 [-3 bytes / -0.00%] codesize/test_codesize_cxx_mangle.json: 266439 => 266436 [-3 bytes / -0.00%] codesize/test_codesize_cxx_noexcept.json: 155847 => 155857 [+10 bytes / +0.01%] codesize/test_codesize_cxx_wasmfs.json: 180965 => 180975 [+10 bytes / +0.01%] codesize/test_codesize_hello_O0.json: 38609 => 38586 [-23 bytes / -0.06%] test/codesize/test_codesize_minimal_O0.expected.js updated codesize/test_codesize_minimal_O0.json: 19718 => 19695 [-23 bytes / -0.12%] codesize/test_unoptimized_code_size.json: 172395 => 172339 [-56 bytes / -0.03%] Average change: -0.02% (-0.12% - +0.01%) ```
2 participants
#27498 moved EH status management (
uncaughtExceptionCountandexceptionLastfromlibexceptions.jstolibunwind.js, but because in the native libcxxabi and libunwind, the management is done within libcxxabi, I think it'd be more consistent if we do the same for the JS libraries.