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

[Sebastian Redl]

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.

> +        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.

> +        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.

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

Sebastian