[EH] Fix inconsistency of refcounting in Emscripten EH vs. Wasm EH

[aheejin]

The place we increase refcount is different in Emscripten EH and Wasm EH, which is explained in detail here:

emscripten/tests/test_core.py

Lines 1689 to 1697 in 2a3bb65

	#ifdef __USING_EMSCRIPTEN_EXCEPTION__
	// FIXME Currently Wasm EH and Emscripten EH increases
	// refcounts in different places. Wasm EH sets the refcount to
	// 1 when throwing, and decrease it in __cxa_end_catch.
	// Emscripten EH sets the refcount to 0 when throwing, and
	// increase it in __cxa_begin_catch, and decrease it in
	// __cxa_end_catch. Fix this inconsistency later.
	incrementExceptionRefcount(p);
	#endif

So if you want to make the code work for both of them, you should put something like #ifdef in the code to prevent memory from leaking. It'd be good to fix this inconsistency.

[sbc100]

I would hope that we can find a way to avoid exposing users this this kind of low level API. Maybe its not possible avoid?

[aheejin]

I'm not sure how to avoid it. I thought about decreasing refcount within getExceptionMessage, but that means the user can't call getExceptionMessage twice with a caught exception.

[stevenwdv]

Is there any progress on eliminating the inconsistency? It doesn't seem possible to detect at runtime (or even at compile time, via ifdef with a macro) which exception handling method is used, so you have to keep the code consistent with compiler flags. Additionally, even if I do it right now, I don't know which direction will be future-proof when this inconsistency is fixed: will we need to increment then, or not?

Maybe you could add something like Module.incrementExceptionRefcountIfNeeded(), or maybe better: Module.consumeException(ex, () => {...}), which handles increment/decrement as needed. (Ideally you'd want to make sure this cannot be called twice, as the exception would've been freed. Maybe set a custom property on it (some Symbol?)?)

[stevenwdv]

EDIT: I just realized the issue below also applies to getExceptionMessage etc., so I might be missing something, because that would've been hard to overlook...?

Wait, while trying to find a way to detect Emscripten EH vs WASM EH, I just realized that the exception type is of course not the same for Emscripten EH as for WASM EH. For WASM EH it's WebAssembly.Error, while for Emscripten EH it's EmscriptenEH (or CppException), which is an Error. This of course also means that the increment/decrement code is different.

But anyway, that's not the point, which is that I think Emscripten EH's increment/decrement implementation is incompatible and broken for the example:
Calls to incrementExceptionRefcount/decrementExceptionRefcount are directly forwarded to __cxa_increment_exception_refcount/__cxa_decrement_exception_refcount, which expect raw pointers, not JS objects.

emscripten/src/lib/libexceptions.js

Lines 336 to 340 in f216821

	$incrementExceptionRefcount__deps: ['__cxa_increment_exception_refcount'],
	$incrementExceptionRefcount: (ptr) => ___cxa_increment_exception_refcount(ptr),
	$decrementExceptionRefcount__deps: ['__cxa_decrement_exception_refcount'],
	$decrementExceptionRefcount: (ptr) => ___cxa_decrement_exception_refcount(ptr),

The call then simply becomes becomes a silent NO-OP as the JS object gets mapped to a nullptr: (This should probably cause an error instead, to prevent unexpected behavior)

emscripten/system/lib/libcxxabi/src/cxa_exception_emscripten.cpp

Lines 124 to 132 in f216821

	void
	__cxa_increment_exception_refcount(void *thrown_object) throw() {
	if (thrown_object != NULL )
	{
	__cxa_exception* exception_header = cxa_exception_from_thrown_object(thrown_object);
	DEBUG("INC: %p refcnt=%zu", thrown_object, exception_header->referenceCount);
	std::__libcpp_atomic_add(&exception_header->referenceCount, size_t(1));
	}
	}

Am I right, or...?

PS: Is there a way to turn on the DEBUG log statement?

[aheejin]

I think #25988 will fix it. Thanks for reminding me of the issue.

Wait, while trying to find a way to detect Emscripten EH vs WASM EH, I just realized that the exception type is of course not the same for Emscripten EH as for WASM EH. For WASM EH it's WebAssembly.Error, while for Emscripten EH it's EmscriptenEH (or CppException), which is an Error. This of course also means that the increment/decrement code is different.
But anyway, that's not the point, which is that I think Emscripten EH's increment/decrement implementation is incompatible and broken for the example: Calls to incrementExceptionRefcount/decrementExceptionRefcount are directly forwarded to __cxa_increment_exception_refcount/__cxa_decrement_exception_refcount, which expect raw pointers, not JS objects.

No, what these library functions (__cxa_throw, __cxa_increment_exception_refcount, ...) deal with is the exception pointer which is in the linear memory, not the Exception or Error objects. For example, if we have this program:

int main() {
  throw 3;
}

Its LLVM bitcode would look like this:

define i32 @main() {
entry:
  %exception = call ptr @__cxa_allocate_exception(i32 4)
  store i32 3, ptr %exception, align 16
  call void @__cxa_throw(ptr %exception, ptr @_ZTIi, ptr null)
  unreachable
}

Note that the value we throw is not '3', but a pointer to a __cxa_exception object. (Actually, it is the result of calling thrown_object_from_cxa_exception on that __cxa_exception object)

CppException wrapping happens inside __cxa_throw. So __cxa_throw still takes a pointer. Also WebAssembly.Exception (not WebAssembly.Error) wrapping happens when Wasm throw instruction is executed, andtry_table-catch takes care of unwrapping, so the user program and the library doesn't need to deal with it.

PS: Is there a way to turn on the DEBUG log statement?

No I don't think a user can recompile libraries with -DEXCEPTIONS_DEBUG (without modifying system_libs.py directly, which you can easily do actually. But I hope you wouldn't need it after the fix)