[EH] Use _UA_SEARCH_PHASE in personality function (NFC) #17991
Conversation
In two-phase unwinding, the first phase is the search phase (`_UA_SEARCH_PHASE`) and the second one is the cleanup phase (`_UA_CLEANUP_PHASE`). The search phase searches up the stack to see if there is a matching catch handler, and if it finds one, it caches the result. And in the second cleanup phase, it retrieves the cached result (to avoid doing same work twice) and unwinds the stack. Wasm does not do the two-phase unwinding; it only has a single phase. We used `_UA_CLEANUP_PHASE` for this single phase, so in Wasm the cleanup phase is supposed to the search. So we several many custom `#ifdef`s to use the code guarded by `_UA_SEARCH_PHASE`, for example: https://github.com/aheejin/emscripten/blob/d57db5bea1719319a680699c50b91fa3d88fa0ec/system/lib/libcxxabi/src/cxa_personality.cpp#L771-L776 https://github.com/aheejin/emscripten/blob/d57db5bea1719319a680699c50b91fa3d88fa0ec/system/lib/libcxxabi/src/cxa_personality.cpp#L850-L855 These are apparently gone in emscripten-core#14288, which replaced many `if`s with `assert`s. This in effect removed our special handling for `_UA_CLEANUP_PHASE`; there are several `assert`s that asserts the current phase is `_UA_SEARCH_PHASE`, while Wasm is in `_UA_CLEANUP_PHASE`. But this has not caused problems so far because we have built libc++abi with `-NDEBUG`, so all assertions were no-op. https://github.com/emscripten-core/emscripten/blob/40fb7d2071e439f1de614898b88518df582faa94/tools/system_libs.py#L1366 But this is now a problem because emscripten-core#17979 adds a debug build of libc++abi, which enables assertions. Come to think of it, I'm not sure why I decided to use `_UA_CLEANUP_PHASE` for our single phase in the first place. If we use `_UA_SEARCH_PHASE`, we can remove more our custom code and reduce the difference between our port and the upstream library.
| #ifdef __USING_WASM_EXCEPTIONS__ | ||
| // Wasm only uses a single phase (_UA_SEARCH_PHASE), so save the | ||
| // results here. | ||
| set_registers(unwind_exception, context, results); | ||
| #endif |
There was a problem hiding this comment.
This is to save our selector computation result.
In the upstream version, the search phase searches for a matching catch handler and caches the result, and the second cleanup phase retrieves the result and sets registers with the final selector value. (In Wasm's libunwind, they are not registers but just a field in a struct.). They do it in the second cleanup phase:
But now we only use the search phase, we do it here. Instead, we are able to remove our duplicated caching code in line 984-993 below.
| #ifdef __USING_WASM_EXCEPTIONS__ | ||
| // Wasm uses only one phase in _UA_CLEANUP_PHASE, so we should set | ||
| // these here. | ||
| __cxa_exception* exception_header = (__cxa_exception*)(unwind_exception+1) - 1; | ||
| exception_header->handlerSwitchValue = static_cast<int>(results.ttypeIndex); | ||
| exception_header->actionRecord = results.actionRecord; | ||
| exception_header->languageSpecificData = results.languageSpecificData; | ||
| exception_header->catchTemp = reinterpret_cast<void*>(results.landingPad); | ||
| exception_header->adjustedPtr = results.adjustedPtr; | ||
| #endif |
There was a problem hiding this comment.
This is what the upstream code does in the first search phase, but we used only the cleanup phase, we duplicated the code here. Now we can remove it.
| assert(actions & _UA_SEARCH_PHASE); | ||
| results.ttypeIndex = ttypeIndex; |
There was a problem hiding this comment.
This is not related to this PR, but I compared our cxa_personality.cpp with llvm 14.0.0 version (on which our current libc++abi is based on), this part is missing. I guess it was dropped when we upgraded the library. I don't think Wasm is using this part of the code though, reducing difference from the upstream code seems better.
In two-phase unwinding, the first phase is the search phase (
_UA_SEARCH_PHASE) and the second one is the cleanup phase (_UA_CLEANUP_PHASE). The search phase searches up the stack to see if there is a matching catch handler, and if it finds one, it caches the result. And in the second cleanup phase, it retrieves the cached result (to avoid doing same work twice) and unwinds the stack.Wasm does not do the two-phase unwinding; it only has a single phase. We used
_UA_CLEANUP_PHASEfor this single phase, so in Wasm the cleanup phase is supposed to the search. So we several many custom#ifdefs to use the code guarded by_UA_SEARCH_PHASE, for example: https://github.com/aheejin/emscripten/blob/d57db5bea1719319a680699c50b91fa3d88fa0ec/system/lib/libcxxabi/src/cxa_personality.cpp#L771-L776 https://github.com/aheejin/emscripten/blob/d57db5bea1719319a680699c50b91fa3d88fa0ec/system/lib/libcxxabi/src/cxa_personality.cpp#L850-L855These are apparently gone in #14288, which replaced many
ifs withasserts. This in effect removed our special handling for_UA_CLEANUP_PHASE; there are severalasserts that asserts the current phase is_UA_SEARCH_PHASE, while Wasm is in_UA_CLEANUP_PHASE. But this has not caused problems so far because we have built libc++abi with-NDEBUG, so all assertions were no-op.emscripten/tools/system_libs.py
Line 1366 in 40fb7d2
Come to think of it, I'm not sure why I decided to use
_UA_CLEANUP_PHASEfor our single phase in the first place. If we use_UA_SEARCH_PHASE, we can remove more our custom code and reduce the difference between our port and the upstream library.