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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
