Differential D18618
[ObjC] Pop all cleanups created in CodeGenFunction::EmitObjCForCollectionStmt Authored by ahatanak on Mar 30 2016, 12:54 PM.
Details 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 TimelineComment Actions 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. Comment Actions 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. Comment Actions 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). Comment Actions Changed the patch based on review comments. Added code to enter RunCleanupsScope before the condition is evaluated and force its cleanup before emitting LoopEnd. |