[cfe-commits] [libcxxabi] r147547 - in /libcxxabi/trunk/src: cxa_exception.cpp cxa_personality.cpp

[Howard Hinnant]

On Jan 10, 2012, at 2:46 AM, Sebastian Redl wrote:

> 
> On 04.01.2012, at 21:49, Howard Hinnant wrote:
> 
>> Author: hhinnant
>> Date: Wed Jan  4 14:49:43 2012
>> New Revision: 147547
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=147547&view=rev
>> Log:
>> Just getting started on the personality routine.  This is just a skeleton.  Still learning how to fill it in...
> 
> I know you've done some commits since then, so some comments below might be out-of-date, but merging the patches is more trouble, so here goes.
> I'll look at the later commits later.
> 
>> 
>> +// Requires:  version == 1
>> +//            actions == _UA_SEARCH_PHASE, or
>> +//                    == _UA_CLEANUP_PHASE, or
>> +//                    == _UA_CLEANUP_PHASE | _UA_HANDLER_FRAME, or
>> +//                    == _UA_CLEANUP_PHASE | _UA_FORCE_UNWIND
>> +//            exceptionObject != nullptr
>> +//            context != nullptr
>> +_Unwind_Reason_Code
>> +__gxx_personality_v0(int version, _Unwind_Action actions, uint64_t exceptionClass,
>> +                     _Unwind_Exception* exceptionObject, _Unwind_Context* context)
>> +{
>> +    if (version == 1 && exceptionObject != 0 && context != 0)
>> +    {
> 
> Does it make sense to invert this condition, return early, and reduce indentation? You'd still need the return at the end.

<shrug> For relatively short routines I've grown to prefer this style.  But it isn't a big deal either way.

> 
>> +        bool native_exception = (exceptionClass & 0xFFF0) == 0x432B2B00;
> 
> I don't see how this could ever be true. Is the bitmask wrong? Also, a named constant on the rhs would be nice.

Yes, it was wrong and has since been fixed.  You're correct that I should encapsulate this.

> 
>> +        bool force_unwind = actions & _UA_FORCE_UNWIND;
>> +        if (native_exception && !force_unwind)
>> +        {
>> +            if (actions & _UA_SEARCH_PHASE)
>> +            {
>> +                if (actions & _UA_CLEANUP_PHASE)
>> +                    return _URC_FATAL_PHASE1_ERROR;
>> +                if (contains_handler(exceptionObject, context))
>> +                    return _URC_HANDLER_FOUND
>> +                return _URC_CONTINUE_UNWIND;
>> +            }
>> +            if (actions & _UA_CLEANUP_PHASE)
>> +            {
>> +                if (actions & _UA_HANDLER_FRAME)
>> +                {
>> +                    // return _URC_INSTALL_CONTEXT or _URC_FATAL_PHASE2_ERROR
>> +                    return transfer_control_to_landing_pad(context);
>> +                }
>> +                // return _URC_CONTINUE_UNWIND or _URC_FATAL_PHASE2_ERROR
>> +                return perform_cleanup();
>> +            }
>> +        }
>> +        else // foreign exception or force_unwind
>> +        {
>> +            if (actions & _UA_SEARCH_PHASE)
>> +            {
>> +                if (actions & _UA_CLEANUP_PHASE)
>> +                    return _URC_FATAL_PHASE1_ERROR;
>> +                return _URC_CONTINUE_UNWIND;
>> +            }
>> +            if (actions & _UA_CLEANUP_PHASE)
>> +            {
>> +                if (actions & _UA_HANDLER_FRAME)
>> +                    return _URC_FATAL_PHASE2_ERROR;
>> +                // return _URC_CONTINUE_UNWIND or _URC_FATAL_PHASE2_ERROR
>> +                return perform_cleanup();
>> +            }
>> +        }
>> +    }
> 
> Feels to me that this would be cleaner if you swapped the nesting - do the if (native_exception && !force_unwind) at the innermost place it's needed.
> bool catchable = native_exception && !force_unwind;
> if (actions & SEARCH) {
>  if (actions & CLEANUP)
>    return error;
>  if (catchable && containsHandler(eO, context))
>    return found;
>  return continue;
> }
> etc.

I probably will reorganize this.  I'm still learning what the function is supposed to do and have yet to learn exactly how to handle foreign exceptions and forced unwinding.

> 
> By the way, I believe GCC allows catching foreign exceptions by installing a handler for _native_exception or something like that.

Hmm... Any public documentation on that?  I also suspect, but do not know for sure at the moment, that a foreign exception can be caught by catch (...).

Thanks,
Howard