[libc++abi] Simplify scan_eh_tab

[MaskRay]

Summary

All _URC_HANDLER_FOUND return values need to set landingPad
and its value does not matter for _URC_CONTINUE_UNWIND. So we
can always set landingPad to unify code.

For an exception specification (ttypeIndex < 0), we can check _UA_FORCE_UNWIND first.

The so-called type 3 search (actions & _UA_CLEANUP_PHASE && !(actions & _UA_HANDLER_FRAME))
is actually conceptually wrong. For a catch handler or an unmatched dynamic
exception specification, _UA_HANDLER_FOUND should be returned immediately. It
still appeared to work because the ttypeIndex==0 case would return
_UA_HANDLER_FOUND at a later time.

This patch fixes the conceptual error and simplifies the code by handling type 3
the same way as type 2 (which is also what libsupc++ does).
The only difference between phase 1 and phase 2 is what to do with a cleanup
(actionEntry==0, or a ttypeIndex==0 is found in the action record chain):
phase 1 returns _URC_CONTINUE_UNWIND while phase 2 returns _URC_HANDLER_FOUND.

[MaskRay]

Fix throw nullptr (which requires to adjust the pointer.

[MaskRay]

😳

[MaskRay]

😳

[MaskRay]

@mstorsjo Perhaps you can test this as well:)

(I have tested it on x86-64 and an ARM qemu-user environ)

[MaskRay]

In D93190#2485484, @MaskRay wrote:

@mstorsjo Perhaps you can test this as well:)

(I have tested it on x86-64 and an ARM qemu-user environ)

😳

[mstorsjo]

In D93190#2485484, @MaskRay wrote:

@mstorsjo Perhaps you can test this as well:)

Seems good on mingw on i686 (dwarf), x86_64 (SEH), armv7 (dwarf), aarch64 (SEH).

[MaskRay]

In D93190#2491125, @mstorsjo wrote:

In D93190#2485484, @MaskRay wrote:

@mstorsjo Perhaps you can test this as well:)

Seems good on mingw on i686 (dwarf), x86_64 (SEH), armv7 (dwarf), aarch64 (SEH).

Thanks!

May I get a LGTM from an owner? :)

[ldionne]

libcxxabi/src/cxa_personality.cpp

776–778

I don't understand why we're allowed to remove this. Can you please explain? The same question applies to other removals of call_terminate below.

[MaskRay]

libcxxabi/src/cxa_personality.cpp

738

For regular unwinding: if the search phase returns _URC_HANDLER_FOUND (catch (...) always matches), the cleanup phase will get _UA_HANDLER_FRAME and will match as well. The check here is dead.

For _UA_FORCE_UNWIND, there is no search phase. The code path is skipped.

776–778

For regular unwinding: if the search phase returns _URC_HANDLER_FOUND (can_catch returns true), the cleanup phase will match as well. These are some additional sanity checks but they are not useful and just clutter up the code. Neither libsupc++ nor libcxxrt has the additional checks.

For _UA_FORCE_UNWIND, there is no search phase. The code path is skipped.

808

For regular unwinding: if the search phase returns _URC_HANDLER_FOUND (the exception specifier cannot match the propagated exception), the cleanup phase will have exception_spec_can_catch returning true as well. The check here is dead.

For _UA_FORCE_UNWIND, there is no search phase. The code path is skipped.

[smeenai]

@compnerd is already part of the libc++abi group, but I'm also adding him explicitly in case he has the time to look, since he has a lot of relevant experience.

[MaskRay]

In D93190#2491770, @smeenai wrote:

@compnerd is already part of the libc++abi group, but I'm also adding him explicitly in case he has the time to look, since he has a lot of relevant experience.

Ping @compnerd :)

[compnerd]

I like the clean up here, but, I think there is some value in retaining the defensive sanity checks. You are correct that in most cases, this should never be hit in a proper unwind, but because this may be run in a last chance scenario, having the defensive checks is good for catching things going wrong.

libcxxabi/src/cxa_personality.cpp

738

Correct, this was a sanity check path. I think that rewriting it as an explicit check and terminate (which is annotated as being cold even) would be nice to retain. At the very least, we should assert that.

[MaskRay]

Add assert

[MaskRay]

fix an assert and comment

[MaskRay]

forgot to include a paren change in the previous diff

[compnerd]

Looks reasonable enough to me. Please ensure that @ldionne is okay with it as well.

[ldionne]

This really isn't in my area, so I'm deferring to @compnerd . Please feel free to go ahead. :-)

[MaskRay]

Check _UA_FORCE_UNWIND first for exception specification

libcxxabi/src/cxa_personality.cpp

 #ifndef __USING_SJLJ_EXCEPTIONS__
             if (landingPad == 0)
             {
                 // No handler here
                 results.reason = _URC_CONTINUE_UNWIND;
                 return;
             }
             landingPad = (uintptr_t)lpStart + landingPad;
+            results.landingPad = landingPad;
 #else  // __USING_SJLJ_EXCEPTIONS__
             ++landingPad;
 #endif  // __USING_SJLJ_EXCEPTIONS__
             if (actionEntry == 0)
             {
                 // Found a cleanup
-                // If this is a type 1 or type 2 search, there are no handlers
-                // If this is a type 3 search, you want to install the cleanup.
-                if ((actions & _UA_CLEANUP_PHASE) && !(actions & _UA_HANDLER_FRAME))
-                {
-                    results.ttypeIndex = 0;  // Redundant but clarifying
-                    results.landingPad = landingPad;
-                    results.reason = _URC_HANDLER_FOUND;
-                    return;
-                }
-                // No handler here
-                results.reason = _URC_CONTINUE_UNWIND;
+                results.reason = actions & _UA_SEARCH_PHASE
+                                     ? _URC_CONTINUE_UNWIND
+                                     : _URC_HANDLER_FOUND;
                 return;
             }
             // Convert 1-based byte offset into
             const uint8_t* action = actionTableStart + (actionEntry - 1);
+            bool hasCleanup = false;
             // Scan action entries until you find a matching handler, cleanup, or the end of action list
             while (true)
             {
                 const uint8_t* actionRecord = action;
                 int64_t ttypeIndex = readSLEB128(&action);
                 if (ttypeIndex > 0)
                 {
                     // Found a catch, does it actually catch?
                     // First check for catch (...)
                     const __shim_type_info* catchType =
                         get_shim_type_info(static_cast<uint64_t>(ttypeIndex),
                                            classInfo, ttypeEncoding,
                                            native_exception, unwind_exception);
                     if (catchType == 0)
                     {
-                        // Found catch (...) catches everything, including foreign exceptions
-                        // If this is a type 1 search save state and return _URC_HANDLER_FOUND
-                        // If this is a type 2 search save state and return _URC_HANDLER_FOUND
-                        // If this is a type 3 search !_UA_FORCE_UNWIND, we should have found this in phase 1!
-                        // If this is a type 3 search _UA_FORCE_UNWIND, ignore handler and continue scan
-                        if ((actions & _UA_SEARCH_PHASE) || (actions & _UA_HANDLER_FRAME))
-                        {
-                            // Save state and return _URC_HANDLER_FOUND
-                            results.ttypeIndex = ttypeIndex;
-                            results.actionRecord = actionRecord;
-                            results.landingPad = landingPad;
-                            results.adjustedPtr = get_thrown_object_ptr(unwind_exception);
-                            results.reason = _URC_HANDLER_FOUND;
-                            return;
-                        }
-                        else if (!(actions & _UA_FORCE_UNWIND))
-                        {
-                            // It looks like the exception table has changed
-                            //    on us.  Likely stack corruption!
-                            call_terminate(native_exception, unwind_exception);
-                        }
-                    }
+                        // Found catch (...) catches everything, including
+                        // foreign exceptions. This is search phase, cleanup
+                        // phase with foreign exception, or forced unwinding.
+                        assert(actions & (_UA_SEARCH_PHASE | _UA_HANDLER_FRAME |
+                                          _UA_FORCE_UNWIND));
+                        results.adjustedPtr =
+                            get_thrown_object_ptr(unwind_exception);
                     // Else this is a catch (T) clause and will never
                     //    catch a foreign exception
                     else if (native_exception)
                     {
                         __cxa_exception* exception_header = (__cxa_exception*)(unwind_exception+1) - 1;
                         void* adjustedPtr = get_thrown_object_ptr(unwind_exception);
                         const __shim_type_info* excpType =
                             static_cast<const __shim_type_info*>(exception_header->exceptionType);
                         if (adjustedPtr == 0 || excpType == 0)
                         {
                             // Something very bad happened
                             call_terminate(native_exception, unwind_exception);
                         }
                         if (catchType->can_catch(excpType, adjustedPtr))
                         {
-                            // Found a matching handler
-                            // If this is a type 1 search save state and return _URC_HANDLER_FOUND
-                            // If this is a type 3 search and !_UA_FORCE_UNWIND, we should have found this in phase 1!
-                            // If this is a type 3 search and _UA_FORCE_UNWIND, ignore handler and continue scan
-                            if (actions & _UA_SEARCH_PHASE)
-                            {
-                                // Save state and return _URC_HANDLER_FOUND
-                                results.ttypeIndex = ttypeIndex;
-                                results.actionRecord = actionRecord;
-                                results.landingPad = landingPad;
-                                results.adjustedPtr = adjustedPtr;
-                                results.reason = _URC_HANDLER_FOUND;
-                                return;
-                            }
-                            else if (!(actions & _UA_FORCE_UNWIND))
-                            {
-                                // It looks like the exception table has changed
-                                //    on us.  Likely stack corruption!
-                                call_terminate(native_exception, unwind_exception);
-                            }
-                        }
+                            // Found a matching handler. This is either search
+                            // phase or forced unwinding.
+                            assert(actions &
+                                   (_UA_SEARCH_PHASE | _UA_FORCE_UNWIND));
                     }
                     // Scan next action ...
                 }
                 else if (ttypeIndex < 0)
                 {
-                    // Found an exception spec.  If this is a foreign exception,
-                    //   it is always caught.
-                    if (native_exception)
-                    {
+                    // Found an exception specification.
+                    if (actions & _UA_FORCE_UNWIND) {
+                        // Skip if forced unwinding.
+                    } else if (native_exception) {
                         // Does the exception spec catch this native exception?
                         __cxa_exception* exception_header = (__cxa_exception*)(unwind_exception+1) - 1;
                         void* adjustedPtr = get_thrown_object_ptr(unwind_exception);
                         const __shim_type_info* excpType =
                             static_cast<const __shim_type_info*>(exception_header->exceptionType);
                         if (adjustedPtr == 0 || excpType == 0)
                         {
                             // Something very bad happened
                             call_terminate(native_exception, unwind_exception);
                         }
                         if (exception_spec_can_catch(ttypeIndex, classInfo,
                                                      ttypeEncoding, excpType,
                                                      adjustedPtr, unwind_exception))
                         {
-                            // native exception caught by exception spec
-                            // If this is a type 1 search, save state and return _URC_HANDLER_FOUND
-                            // If this is a type 3 search !_UA_FORCE_UNWIND, we should have found this in phase 1!
-                            // If this is a type 3 search _UA_FORCE_UNWIND, ignore handler and continue scan
-                            if (actions & _UA_SEARCH_PHASE)
-                            {
-                                // Save state and return _URC_HANDLER_FOUND
-                                results.ttypeIndex = ttypeIndex;
-                                results.actionRecord = actionRecord;
-                                results.landingPad = landingPad;
-                                results.adjustedPtr = adjustedPtr;
-                                results.reason = _URC_HANDLER_FOUND;
-                                return;
-                            }
-                            else if (!(actions & _UA_FORCE_UNWIND))
-                            {
-                                // It looks like the exception table has changed
-                                //    on us.  Likely stack corruption!
-                                call_terminate(native_exception, unwind_exception);
-                            }
-                        }
-                    }
-                    else
-                    {
+                            // Native exception caught by exception
+                            // specification.
+                            assert(actions & _UA_SEARCH_PHASE);
+                    } else {
                         // foreign exception caught by exception spec
-                        // If this is a type 1 search, save state and return _URC_HANDLER_FOUND
-                        // If this is a type 2 search, save state and return _URC_HANDLER_FOUND
-                        // If this is a type 3 search !_UA_FORCE_UNWIND, we should have found this in phase 1!
-                        // If this is a type 3 search _UA_FORCE_UNWIND, ignore handler and continue scan
-                        if ((actions & _UA_SEARCH_PHASE) || (actions & _UA_HANDLER_FRAME))
-                        {
-                            // Save state and return _URC_HANDLER_FOUND
-                            results.ttypeIndex = ttypeIndex;
-                            results.actionRecord = actionRecord;
-                            results.landingPad = landingPad;
-                            results.adjustedPtr = get_thrown_object_ptr(unwind_exception);
-                            results.reason = _URC_HANDLER_FOUND;
-                            return;
-                        }
-                        else if (!(actions & _UA_FORCE_UNWIND))
-                        {
-                            // It looks like the exception table has changed
-                            //    on us.  Likely stack corruption!
-                            call_terminate(native_exception, unwind_exception);
-                        }
-                    }
+                        results.adjustedPtr =
+                            get_thrown_object_ptr(unwind_exception);
                     // Scan next action ...
-                }
-                else  // ttypeIndex == 0
-                {
-                    // Found a cleanup
-                    // If this is a type 1 search, ignore it and continue scan
-                    // If this is a type 2 search, ignore it and continue scan
-                    // If this is a type 3 search, save state and return _URC_HANDLER_FOUND
-                    if ((actions & _UA_CLEANUP_PHASE) && !(actions & _UA_HANDLER_FRAME))
-                    {
-                        // Save state and return _URC_HANDLER_FOUND
-                        results.ttypeIndex = ttypeIndex;
-                        results.actionRecord = actionRecord;
-                        results.landingPad = landingPad;
-                        results.adjustedPtr = get_thrown_object_ptr(unwind_exception);
-                        results.reason = _URC_HANDLER_FOUND;
-                        return;
-                    }
+                } else {
+                    hasCleanup = true;
                 }
                 const uint8_t* temp = action;
                 int64_t actionOffset = readSLEB128(&temp);
                 if (actionOffset == 0)
                 {
-                    // End of action list, no matching handler or cleanup found
-                    results.reason = _URC_CONTINUE_UNWIND;
+                    // End of action list. If this is phase 2 and we have found
+                    // a cleanup (ttypeIndex=0), return _URC_HANDLER_FOUND;
+                    // otherwise return _URC_CONTINUE_UNWIND.
+                    results.reason = hasCleanup && actions & _UA_CLEANUP_PHASE
+                                         ? _URC_HANDLER_FOUND
+                                         : _URC_CONTINUE_UNWIND;
                     return;
                 }
                 // Go to next action
                 action += actionOffset;
             }  // there is no break out of this loop, only return
         }
 #ifndef __USING_SJLJ_EXCEPTIONS__
         else if (ipOffset < start)