[PATCH] D18618: [ObjC] Pop all cleanups created in CodeGenFunction::EmitObjCForCollectionStmt

[John McCall via cfe-commits]

rjmccall added a comment.

In http://reviews.llvm.org/D18618#387847, @ahatanak wrote:

> In CodeGenFunction::EmitARCRetainScalarExpr, if I move the declaration of "scope" above the call to enterFullExpression, the cleanup is popped before the loop body is entered right after the method returning the collection (foo1 in the test case) is called.
>
> if (const ExprWithCleanups *cleanups = dyn_cast<ExprWithCleanups>(e)) {
>
>   enterFullExpression(cleanups);
>   RunCleanupsScope scope(*this);
>   return EmitARCRetainScalarExpr(cleanups->getSubExpr());
>
> }
>
> The cleanup is entered in enterBlockScope which is transitively called from enterFullExpression. EmitARCRetainScalarExpr is called at CGObjC.cpp:1491. The Collection for ExprObjCForCollectionStmt is an ExprWithCleanups which has the Block passed to foo1 attached to it and the cleanups for the captures of the Block are entered in enterBlockScope.


I see.

We start at a cleanup depth D.  LoopEnd is being created at that depth and registered as a break target, meaning that a break within the loop will branch directly to LoopEnd.getBlock() through any enclosing cleanups down to D.  The code pushes exactly one cleanup explicitly, and only in ARC, and so it is assuming that it can simply pop that cleanup on the normal exit out of the loop and the cleanup stack will go back to depth D.  Unfortunately, this is incorrect because we need to evaluate a full-expression, and while normally that will not introduce additional cleanups in the enclosing scope, it can because of block scoping.

Block scoping is somewhat strange: a block lives for the lifetime of the scope enclosing the full-expression containing the block literal.  This is why the operations in EmitARCRetainScalarExpr are ordered the way they are.  However, arguably the enclosing scope of the collection expression of an ObjC foreach loop ought to be tied to the collection statement itself rather than that statement's enclosing scope.  That would be consistent with how we interpret the scoping rules of other control-flow statements: we push a scope before evaluating the condition (and exit it before emitting the continuation block) regardless of whether the condition actually declares a variable.

So I think the correct solution is to follow the model of EmitWhileStmt: enter a RunCleanupsScope immediately before evaluating the condition and force its cleanups immediately before branching to LoopEnd.getBlock() along the normal exit path (unconditionally, not just in ARC mode).


http://reviews.llvm.org/D18618