This is an archive of the discontinued LLVM Phabricator instance.

[ObjC] Pop all cleanups created in CodeGenFunction::EmitObjCForCollectionStmt
ClosedPublic

Authored by ahatanak on Mar 30 2016, 12:54 PM.

Details

Summary

This patch fixes a bug where EmitObjCForCollectionStmt doesn't pop cleanups for captures.

For example, in the following for-in loop, a block which captures self is passed to foo1:

for (id x in [self foo1:^{ use(self); }]) {

use(x);
break;

}

The current code in EmitObjCForCollectionStmt doesn't pop the cleanup for the captured self before completing IR generation for the for-in loop. This causes code-gen to generate incorrect IR in which self gets released twice.

rdar://problem/16865751

Diff Detail

Event Timeline

ahatanak updated this revision to Diff 52106.Mar 30 2016, 12:54 PM
ahatanak retitled this revision from to [ObjC] Pop all cleanups created in CodeGenFunction::EmitObjCForCollectionStmt .
ahatanak updated this object.
ahatanak added a reviewer: rjmccall.
ahatanak added a subscriber: cfe-commits.
rjmccall edited edge metadata.Mar 30 2016, 5:13 PM

If multiple cleanups can be entered, the appropriate thing to do is enter a RunCleanupsScope of some sort and then pop it at the right time. But you should find out why the cleanup is being entered — it's possible that it's not being properly bound to a full-expression, in which case cleaning it up at the end of the collection is inappropriate, because it actually needs to be cleaned up before the loop body is ever entered.

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.

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

ahatanak updated this revision to Diff 52609.Apr 4 2016, 1:25 PM
ahatanak edited edge metadata.

Changed the patch based on review comments.

Added code to enter RunCleanupsScope before the condition is evaluated and force its cleanup before emitting LoopEnd.

Also, fixed the regex in test case so that the test passes on release builds too.

LGTM, thanks!

This revision was automatically updated to reflect the committed changes.
ahatanak edited subscribers, added: kubamracek; removed: cfe-commits.Apr 12 2017, 3:18 PM