diff --git a/ChangeLog.md b/ChangeLog.md index 75bd790c9c33a..8bddb07e46308 100644 --- a/ChangeLog.md +++ b/ChangeLog.md @@ -20,6 +20,9 @@ See docs/process.md for more on how version tagging works. 4.0.23 (in development) ----------------------- +- The inconsistency of incrementing / decrementing refcounts between Wasm EH and + Emscripten EH has been fixed. See `test_EXPORT_EXCEPTION_HANDLING_HELPERS` in + `test_core.py` to see the usage. (#25988) - The `select()` and `poll()` system calls can now block under certain circumstances. Specifically, if they are called from a background thread and file descriptors include pipes. (#25523, #25990) diff --git a/site/source/docs/porting/exceptions.rst b/site/source/docs/porting/exceptions.rst index 4dffc6fe76264..27e97ee6dad8d 100644 --- a/site/source/docs/porting/exceptions.rst +++ b/site/source/docs/porting/exceptions.rst @@ -155,16 +155,10 @@ leaked. .. note:: If you catch a Wasm exception and do not rethrow it, you need to free the storage associated with the exception in JS using - ``decrementExceptionRefcount`` method because the exception - catching code in Wasm does not have a chance to free it. But currently due to - an implementation issue that Wasm EH and Emscripten (JS-based) EH, you need - to call incrementExceptionRefcount additionally in case of Emscripten EH. See - https://github.com/emscripten-core/emscripten/issues/17115 for details and a - code example. - -.. todo:: Fix the above-mentinoed `inconsistency - `_ between Wasm - EH and Emscripten EH, on the reference counting. + ``decrementExceptionRefcount`` method because the exception catching code in + Wasm does not have a chance to free it. See + ``test_EXPORT_EXCEPTION_HANDLING_HELPERS`` in test/test_core.py for an + example usage. Using Exceptions and setjmp-longjmp Together diff --git a/site/source/docs/tools_reference/settings_reference.rst b/site/source/docs/tools_reference/settings_reference.rst index 02f13f5a6aa5b..f499f319a6596 100644 --- a/site/source/docs/tools_reference/settings_reference.rst +++ b/site/source/docs/tools_reference/settings_reference.rst @@ -1134,9 +1134,7 @@ the JS library: Setting this option also adds refcount increasing and decreasing functions ('incrementExceptionRefcount' and 'decrementExceptionRefcount') in the JS library because if you catch an exception from 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). +the refcount manually not to leak memory. See test_EXPORT_EXCEPTION_HANDLING_HELPERS in test/test_core.py for an example usage. diff --git a/src/lib/libexceptions.js b/src/lib/libexceptions.js index 6d55a107eee4d..216fc4cb4aadb 100644 --- a/src/lib/libexceptions.js +++ b/src/lib/libexceptions.js @@ -82,7 +82,11 @@ var LibraryExceptions = { // Here, we throw an exception after recording a couple of values that we need to remember // We also remember that it was the last exception thrown as we need to know that later. - __cxa_throw__deps: ['$ExceptionInfo', '$exceptionLast', '$uncaughtExceptionCount'], + __cxa_throw__deps: ['$ExceptionInfo', '$exceptionLast', '$uncaughtExceptionCount' +#if !DISABLE_EXCEPTION_CATCHING + , '__cxa_increment_exception_refcount' +#endif + ], __cxa_throw: (ptr, type, destructor) => { #if EXCEPTION_DEBUG dbg('__cxa_throw: ' + [ptrToString(ptr), type, ptrToString(destructor)]); @@ -90,6 +94,9 @@ var LibraryExceptions = { var info = new ExceptionInfo(ptr); // Initialize ExceptionInfo content after it was allocated in __cxa_allocate_exception. info.init(type, destructor); +#if !DISABLE_EXCEPTION_CATCHING + ___cxa_increment_exception_refcount(ptr); +#endif {{{ storeException('exceptionLast', 'ptr') }}} uncaughtExceptionCount++; {{{ makeThrow('exceptionLast') }}} @@ -98,7 +105,11 @@ var LibraryExceptions = { // This exception will be caught twice, but while begin_catch runs twice, // we early-exit from end_catch when the exception has been rethrown, so // pop that here from the caught exceptions. - __cxa_rethrow__deps: ['$exceptionCaught', '$exceptionLast', '$uncaughtExceptionCount'], + __cxa_rethrow__deps: ['$exceptionCaught', '$exceptionLast', '$uncaughtExceptionCount' +#if !DISABLE_EXCEPTION_CATCHING + , '__cxa_increment_exception_refcount' +#endif + ], __cxa_rethrow: () => { var info = exceptionCaught.pop(); if (!info) { @@ -112,6 +123,9 @@ var LibraryExceptions = { info.set_caught(false); uncaughtExceptionCount++; } +#if !DISABLE_EXCEPTION_CATCHING + ___cxa_increment_exception_refcount(ptr); +#endif #if EXCEPTION_DEBUG dbg('__cxa_rethrow, popped ' + [ptrToString(ptr), exceptionLast, 'stack', exceptionCaught]); @@ -122,8 +136,7 @@ var LibraryExceptions = { llvm_eh_typeid_for: (type) => type, - __cxa_begin_catch__deps: ['$exceptionCaught', '__cxa_increment_exception_refcount', - '__cxa_get_exception_ptr', + __cxa_begin_catch__deps: ['$exceptionCaught', '__cxa_get_exception_ptr', '$uncaughtExceptionCount'], __cxa_begin_catch: (ptr) => { var info = new ExceptionInfo(ptr); @@ -136,7 +149,6 @@ var LibraryExceptions = { #if EXCEPTION_DEBUG dbg('__cxa_begin_catch ' + [ptrToString(ptr), 'stack', exceptionCaught]); #endif - ___cxa_increment_exception_refcount(ptr); return ___cxa_get_exception_ptr(ptr); }, diff --git a/src/settings.js b/src/settings.js index a5268b0e9275d..99a624b1db406 100644 --- a/src/settings.js +++ b/src/settings.js @@ -768,9 +768,7 @@ var DISABLE_EXCEPTION_THROWING = false; // Setting this option also adds refcount increasing and decreasing functions // ('incrementExceptionRefcount' and 'decrementExceptionRefcount') in the JS // library because if you catch an exception from 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). +// the refcount manually not to leak memory. // // See test_EXPORT_EXCEPTION_HANDLING_HELPERS in test/test_core.py for an // example usage. diff --git a/test/codesize/test_codesize_cxx_except.json b/test/codesize/test_codesize_cxx_except.json index 1dd754aabbe4d..15cb284562627 100644 --- a/test/codesize/test_codesize_cxx_except.json +++ b/test/codesize/test_codesize_cxx_except.json @@ -1,10 +1,10 @@ { - "a.out.js": 23309, - "a.out.js.gz": 9122, + "a.out.js": 23315, + "a.out.js.gz": 9125, "a.out.nodebug.wasm": 171263, "a.out.nodebug.wasm.gz": 57315, - "total": 194572, - "total_gz": 66437, + "total": 194578, + "total_gz": 66440, "sent": [ "__cxa_begin_catch", "__cxa_end_catch", diff --git a/test/codesize/test_codesize_cxx_mangle.json b/test/codesize/test_codesize_cxx_mangle.json index 2bab7bc7315f1..a7dc6f34822f3 100644 --- a/test/codesize/test_codesize_cxx_mangle.json +++ b/test/codesize/test_codesize_cxx_mangle.json @@ -1,10 +1,10 @@ { - "a.out.js": 23359, - "a.out.js.gz": 9142, + "a.out.js": 23365, + "a.out.js.gz": 9145, "a.out.nodebug.wasm": 235290, "a.out.nodebug.wasm.gz": 78917, - "total": 258649, - "total_gz": 88059, + "total": 258655, + "total_gz": 88062, "sent": [ "__cxa_begin_catch", "__cxa_end_catch", diff --git a/test/test_core.py b/test/test_core.py index 6c50db4ef2d9a..29e5fc1d54dc0 100644 --- a/test/test_core.py +++ b/test/test_core.py @@ -1551,18 +1551,8 @@ class myexception : public exception { } catch(p) { // Because we are catching and handling the exception in JS, the normal // exception catching C++ code doesn't kick in, so we need to make sure we free - // the exception, if necessary. By incrementing and decrementing the refcount - // we trigger the free'ing of the exception if its refcount was zero. -#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. - // https://github.com/emscripten-core/emscripten/issues/17115 - incrementExceptionRefcount(p); -#endif + // the exception, if necessary. By decrementing the refcount we trigger the + // free'ing of the exception. out(getExceptionMessage(p).toString()); decrementExceptionRefcount(p); }