[cfe-commits] r167534 - in /cfe/trunk: lib/CodeGen/CGException.cpp lib/CodeGen/CGObjCGNU.cpp lib/CodeGen/CodeGenFunction.h test/CodeGenObjC/gnu-exceptions.m

[John McCall]

On Nov 12, 2012, at 6:53 PM, David Chisnall <csdavec at swan.ac.uk> wrote:
> On 11 Nov 2012, at 00:41, John McCall wrote:
>> If you're going to do this, you need to have two different EH-resume blocks.
>> The way it's written right now, whichever you ask for first is going to be what
>> you get, even for later requests with a different parameter.
>> 
>> But really, I don't understand why you need this parameter at all.  The only
>> place you're passing false is when asking for the block to branch to
>> when an exception-specification fails to trigger.  This is actually still the
>> "falling off the end of the EH scope" case, and should still be emitted with
>> a resume instruction.  I don't think there's any 'resume' case where you
>> actually want to call the rethrow function.
> 
> In that case, I'm confused.  The case that I have failing is that resuming an exception caught by an Objective-C catch (including a catchall) should use the throw function, but resuming one caught in an @finally directive or via a cleanup attribute.

Let me try to break this down.  I'm sure that you know a lot of this, but it's worth being clear.

LLVM's resume instruction is for resuming the current unwind process because the current function is done performing whatever cleanup work it requires.  That's why it has special semantics for the inliner:  the definition of "the current function" changes, and more stuff may need to happen before resuming out of the function (or the landing pad might even handle the exception now).  It's really just for resuming after all the cleanups have run.*

I assume that by "resuming an exception caught by an Objective-C catch" you mean rethrowing it with a "@throw;" statement.  It is generally inappropriate to use 'resume' for this because the rethrown exception might be caught by an enclosing handler in the function, or there might be more cleanups to run before leaving the function — in fact, in C++ (and ObjC with Darwin's personality) that's always true, because catch statements are always run inside a cleanup to perform __{cxa,objc}_end_catch.  But more directly, it is specifically impossible to use 'resume' for "@throw;" with your personality because you actually terminate the current unwind process before entering a landing pad that will handle the exception.  Therefore you will always need to start a new unwind process in this case.

For @finally statements, Clang actually emits them differently from how GCC does:  we use a catch-all, not a cleanup.  First, we want to provide a language guarantee that the @finally is executed;  if there are no handlers active on the stack, not all platforms run cleanups before calling the uncaught exception handler.  Second, a @finally block can actually terminate the current unwind process by leaving the block other than by falling off the end, and that's inappropriate for a cleanup.  So resuming out of a @finally is actually more like an "@throw;" than a resume — especially because, again, the @finally is not necessarily the outermost EH scope in the function.

> Do you have any suggestions for fixing this correctly?  It was working correctly in clang 3.0, but was broken during the EH refactoring at some subsequent point.

1)  The EH resume block should always be using an actual 'resume'.
2)  You need to make sure that rethrowing an exception when falling out of @finally is done with your throw function.

John.

* There's a subtlety here.  In a context that doesn't require cleanups, an actual 'resume' instruction should always be unreachable, because the personality shouldn't ever land at a landing pad if the exception won't be handled and there are no cleanups.  However, optimization can make that logical point in the function reachable, because it might be inlined into a function that needs cleanups or provides more handlers.  So we emit code pessimistically assuming that we might be inlined, and we just hope that the backend will clean that up later (which it currently does not).