Move parts of emscripten exception handling to native code. NFC

[sbc100]

Specifically, this change moves the allocation and reference counting
functions.

This saves on code size but more importantly reduces the number and of
complexity of imports/exports, which in turn helps with the wasm64 work
I've been doing.

[kripken]

Diffing the cpp file to the file it is adapted from, I see stuff like this:

 //  Allocate a __cxa_exception object, and zero-fill it.
 //  Reserve "thrown_size" bytes on the end for the user's exception
 //  object. Zero-fill the object. If memory can't be allocated, call
 //  std::terminate. Return a pointer to the memory to be used for the
 //  user's exception object.
 void *__cxa_allocate_exception(size_t thrown_size) _NOEXCEPT {
     size_t actual_size = cxa_exception_size_from_exception_thrown_size(thrown_size);
 
-    // Allocate extra space before the __cxa_exception header to ensure the
-    // start of the thrown object is sufficiently aligned.
-    size_t header_offset = get_cxa_exception_offset();
     char *raw_buffer =
-        (char *)__aligned_malloc_with_fallback(header_offset + actual_size);
+        (char *)__aligned_malloc_with_fallback(actual_size);
     if (NULL == raw_buffer)
         std::terminate();
     __cxa_exception *exception_header =
-        static_cast<__cxa_exception *>((void *)(raw_buffer + header_offset));
+        static_cast<__cxa_exception *>((void *)(raw_buffer));
     ::memset(exception_header, 0, actual_size);
     return thrown_object_from_cxa_exception(exception_header);
 }

Why do we not have the header_offset? Documenting such changes would be good.

The other part of the diff that looks unclear to me is this:

 void __cxa_decrement_exception_refcount(void *thrown_object) _NOEXCEPT {
     if (thrown_object != NULL )
     {
         __cxa_exception* exception_header = cxa_exception_from_thrown_object(thrown_object);
-        if (std::__libcpp_atomic_add(&exception_header->referenceCount, size_t(-1)) == 0)
+        DEBUG("DEC: %p refcnt=%zu rethrown=%d\n", thrown_object, exception_header->referenceCount, exception_header->rethrown);
+        assert(exception_header->referenceCount > 0);
+        if (std::__libcpp_atomic_add(&exception_header->referenceCount, size_t(-1)) == 0 && !exception_header->rethrown)
         {
+            DEBUG("DEL: %p\n", thrown_object);
             if (NULL != exception_header->exceptionDestructor)
                 exception_header->exceptionDestructor(thrown_object);
             __cxa_free_exception(thrown_object);
         }
     }
 }

We add && !exception_header->rethrown but I'm not sure why?

[sbc100]

Yes, those deltas are deliberate, and made to match the JS implementation. Honestly I'm not totally sure why they exist, but they match the existing JS code.

[sbc100]

Added more comments

[kripken]

Ok, sgtm to match the old JS behavior even if we don't fully understand it.

[RReverser]

Oh I started working on something similar but then decided to search PRs just in case, and glad I did. There seems to be a lot of conflicts - mainly in tests - but otherwise seems like this was in mergeable state?

[sbc100]

I landed a smaller version of this patch in #18292. I'm not sure its worth revisiting this larger change. Would be good to have but not sure if its high enough priority given that it works, and we have native exception handling coming down the pipe soon.

[RReverser]

I'm not sure its worth revisiting this larger change. Would be good to have but not sure if its high enough priority given that it works, and we have native exception handling coming down the pipe soon.

Unfortunately, one doesn't contradict the other. I still need to work on something like this for project I'm working on at the moment, because neither Wasmtime nor Wasmer support Wasm exceptions yet, so the only chance for STANDALONE_WASM is manually shimming out Emscripten exception support in the custom runtime we have.

Moving parts of the Emscripten EH that just modify C++ exception structures from JS to C++ would make this much easier, as in the end I'd only need to shim out invoke_* and __cxa_throw (and maybe few other small things) rather than the whole API.

[RReverser]

I guess, looking at the number of conflicts, I should just try & start anew after all... (unless you think this PR is still salvageable)

[RReverser]

I'll try to rebase it first after all.

[sbc100]

Sounds good. I would still love to see this land if you can make it happen.

[sbc100]

These utilities seem to be duplicated here and in the new utils file.

[RReverser]

Yeah, just like in cxa_exception they were originally borrowed from, they're static local helpers everywhere.

[RReverser]

@sbc100 FWIW this is WIP, I just wanted to push as I got far enough in my conflict resolution & new changes that I didn't want to lose.

[RReverser]

I'll continue working on this tomorrow. For now I managed to rebase most of this stuff, although had to deal with new helpers exposed to JS that were added since this PR.

Since they're shared between backends, I've moved them to a new .cpp file together with (yet another) copy of used static function helpers.

Ideal end state I think would be to reuse cxa_exception.cpp between both backends with some ifdef sprinkled to exclude bits implemented in JS, but that would be out of scope of this particular PR - for now just trying to get it to do what it did originally.

[RReverser]

Also had to add __cxa_decrement_exception_refcount here to make some tests pass; I'm not sure why it wasn't necessary for the original PR but became now - maybe I'm missing a better place where it should be added, or maybe something changed in tests.

[RReverser]

One more thing I noticed is that this part

emscripten/emcc.py

Lines 2785 to 2806 in b5c68f6

	if not settings.DISABLE_EXCEPTION_CATCHING:
	settings.REQUIRED_EXPORTS += [
	# For normal builds the entries in deps_info.py are enough to include
	# these symbols whenever __cxa_find_matching_catch_* functions are
	# found. However, under LTO these symbols don't exist prior to linking
	# so we include then unconditionally when exceptions are enabled.
	'__cxa_is_pointer_type',
	'__cxa_can_catch',
	# __cxa_begin_catch depends on this but we can't use deps info in this
	# case because that only works for user-level code, and __cxa_begin_catch
	# can be used by the standard library.
	'__cxa_increment_exception_refcount',
	# Same for __cxa_end_catch
	'__cxa_decrement_exception_refcount',
	# Emscripten exception handling can generate invoke calls, and they call
	# setThrew(). We cannot handle this using deps_info as the invokes are not
	# emitted because of library function usage, but by codegen itself.
	'setThrew',
	'__cxa_free_exception',
	]

seems to overlap in terms of intent with this one

emscripten/emcc.py

Lines 2897 to 2911 in b5c68f6

	# Make `getExceptionMessage` and other necessary functions available for use.
	if settings.EXPORT_EXCEPTION_HANDLING_HELPERS:
	# If the user explicitly gave EXPORT_EXCEPTION_HANDLING_HELPERS=1 without
	# enabling EH, errors out.
	if settings.DISABLE_EXCEPTION_CATCHING and not settings.WASM_EXCEPTIONS:
	exit_with_error('EXPORT_EXCEPTION_HANDLING_HELPERS requires either of -fexceptions or -fwasm-exceptions')
	# We also export refcount increasing and decreasing functions because if you
	# catch an exception, be it an Emscripten exception or a Wasm exception, in
	# JS, you may need to manipulate the refcount manually not to leak memory.
	# What you need to do is different depending on the kind of EH you use
	# (https://github.com/emscripten-core/emscripten/issues/17115).
	settings.DEFAULT_LIBRARY_FUNCS_TO_INCLUDE += ['$getExceptionMessage', '$incrementExceptionRefcount', '$decrementExceptionRefcount']
	settings.EXPORTED_FUNCTIONS += ['getExceptionMessage', '___get_exception_message', '_free']
	if settings.WASM_EXCEPTIONS:
	settings.EXPORTED_FUNCTIONS += ['___cpp_exception', '___cxa_increment_exception_refcount', '___cxa_decrement_exception_refcount', '___thrown_object_from_unwind_exception']

Should they be merged somehow or are they responsible for different things? (one is under if settings.EXPORT_EXCEPTION_HANDLING_HELPERS and another says REQUIRED_EXPORTS, but not sure if the difference is intentional)

[sbc100]

This seems to have broken some LTO tests: https://logs.chromium.org/logs/emscripten-releases/buildbucket/cr-buildbucket/8784015253103176065/+/u/Emscripten_testsuite__LTO_/stdout

e.g:

RuntimeError: function signature mismatch
    at __cxa_decrement_exception_refcount (<anonymous>:wasm-function[1464]:0x2173e)
    at /usr/local/google/home/sbc/dev/wasm/emscripten/out/test/test_exceptions_allowed_uncaught.js:719:22
    at ___cxa_end_catch (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/test_exceptions_allowed_uncaught.js:1238:7)
    at invoke_v (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/test_exceptions_allowed_uncaught.js:4570:29)
    at <anonymous>:wasm-function[86]:0x6b2f
    at <anonymous>:wasm-function[141]:0x780a

[RReverser]

Ugh I was hoping Emscripten's own CI checks would be enough, and they passed. Can you revert please?

[sbc100]

I'm investigating now. Will revert if I can't find a quick fix.

[sbc100]

(sadly we don't have time to run all tests in all configurations during CI, so we run some only the emscripten-releases waterfall: https://ci.chromium.org/p/emscripten-releases/g/main/console)

[RReverser]

From a quick look, the signature seems the same as before despite moving func from JS to C++. I'm curious why it complains...

[sbc100]

Its particularly interesting that it only happens for LTO..

[sbc100]

I think I have a fix.

[RReverser]

Could it be related to _LIBCXXABI_NO_CFI?

[dschuff]

This is still causing a failure in the LLVM testsuite: https://logs.chromium.org/logs/emscripten-releases/buildbucket/cr-buildbucket/8783849429524138417/+/u/LLVM_testsuite/stdout

wasm-ld: error: lib/libgtest.a(gtest-all.cc.o): undefined symbol: __cxa_free_exception
em++: error: '/b/s/w/ir/x/w/install/bin/wasm-ld -o MicroBenchmarks/libs/benchmark/test/perf_counters_gtest.wasm -L/b/s/w/ir/cache/builder/emscripten-releases/build/llvmtest-out MicroBenchmarks/libs/benchmark/test/CMakeFiles/perf_counters_gtest.dir/perf_counters_gtest.cc.o MicroBenchmarks/libs/benchmark/src/libbenchmark.a lib/libgmock_main.a lib/libgmock.a lib/libgtest.a -L/b/s/w/ir/x/w/install/emscripten/cache/sysroot/lib/wasm32-emscripten -lGL -lal -lhtml5 -lstubs -lc -ldlmalloc -lcompiler_rt -lc++-noexcept -lc++abi-noexcept -lsockets -mllvm -combiner-global-alias-analysis=false -mllvm -enable-emscripten-sjlj -mllvm -disable-lsr --allow-undefined-file=/b/s/w/ir/x/t/tmp2wer4hv6.undefined --strip-debug --export-if-defined=main --export-if-defined=__start_em_asm --export-if-defined=__stop_em_asm --export-if-defined=__start_em_lib_deps --export-if-defined=__stop_em_lib_deps --export-if-defined=__start_em_js --export-if-defined=__stop_em_js --export-if-defined=__main_argc_argv --export-if-defined=fflush --export=stackSave --export=stackRestore --export=stackAlloc --export=__errno_location --export=__get_temp_ret --export=__set_temp_ret --export=__funcs_on_exit --export=__wasm_call_ctors --export=__cxa_is_pointer_type --export=malloc --export=__cxa_increment_exception_refcount --export=setThrew --export=__cxa_decrement_exception_refcount --export=__cxa_can_catch --export=setTempRet0 --export-table -z stack-size=131072 --initial-memory=1073741824 --no-entry --max-memory=1073741824 --global-base=1024' failed (returned 1)

[dschuff]

I have it reproing locally too, let me know if you want more detail or the files.

[sbc100]

I'm guessing that test is building with the default settings? (i.e. DISABLE_EXCEPTION_CATCHING=1 + DISABLE_EXCEPTION_THROWING=0).. hense the linking with -lc++-noexcept -lc++abi-noexcept. Are you sure it was this change that caused the failure to start?

[sbc100]

I'm guessing the change to system/lib/libcxxabi/src/cxa_noexception.cpp that was part of this change needs to have __cxa_free_exception as well as the __cxa_allocate_exception

[sbc100]

Yup that looks like it the problem.. the sad thing is I have no idea why non of our core tests required that. Any idea what gtest-all.cc.o is doing to depend on that symbol? (so we can add a test?)

[dschuff]

According to https://itanium-cxx-abi.github.io/cxx-abi/abi-eh.html#cxx-destroy it seems like __cxa_end_catch should usually call __cxa_free_exception (and indeed this seems to be the case in cxa_exception.cpp where it's called via cxa_decrement_exception_refcount and this is mirrored in cxa_exception_emscripten.cpp. So it seems that basically anything with a catch block should depend on it?

[dschuff]

Oh wait, I see what you're saying. If catching is disabled, then why are we still getting this dependency? Yeah I don't quite get it yet either.

[dschuff]

I disassembled gtest-all.cc.o and found that __cxa_free_exception is actually being called directly by testing::UnitTest::AddTestPartResult(testing::TestPartResult::Type, char const*, int, std::__2::basic_string<char, std::__2::char_traits<char>, std::__2::allocator<char> > const&, std::__2::basic_string<char, std::__2::char_traits<char>, std::__2::allocator<char> > const&) and not by decrement_exception_refcount. The source is https://chromium.googlesource.com/native_client/pnacl-llvm-testsuite/+/edc9be64895f43c79aa34fad32801ada216ce229/MicroBenchmarks/libs/benchmark/googletest/googletest/src/gtest.cc#5351 but there's nothing obvious to me just by looking.

[sbc100]

In emscripten we default to throwing being enabled but catching being disabled.. which is a little unusual.

[sbc100]

I don't see __cxa_free_exception in the source code, so I assume you don't mean they the are actually calling it directly from their source, but that clang is generating a call to it?

[sbc100]

I looks like it must be generated here: https://github.com/llvm/llvm-project/blob/64ae7669a7cefa12ea1117680cf9bfb5c3f6084c/clang/lib/CodeGen/CGException.cpp#L380-L388

Maybe we can add a test that simply calls it directly? Or figure out how to trigger the above code indirectly in C++?

[dschuff]

Yeah, if a generated call is possible, then a test that calls it directly probably makes sense.