Fix over-release of return value of lambda implicitly converted to block/function pointer

[danzimm]

Summary

Clang allows automatic conversion of lambdas to ObjC blocks and C function pointers (the latter only when nothing is captured). If a lambda returns an ObjC object with arc enabled then it gets over-released. You can see the bad behavior in the program below:

typedef id (^Go)();

int main() {
  Go g = []() -> id {
    return [NSObject new];
  };
  id sss = nil;
  @autoreleasepool {
    id ss = g();
    sss = ss;
  }
  NSLog(@"%@", sss);
  return 0;
}

(and if you change the Go typedef to be a function pointer then the same error occurs, which is a crash).

The test I wrote relies on the optimizer inlining the lambda invocation -- I'm not sure if that's an ok assumption to make. The IR generated previous to this patch is kind of fun:

define internal i8* @___Z5test0P11objc_object_block_invoke(i8* nocapture readonly %.block_descriptor) #0 {
entry:
  %block.capture.addr = getelementptr inbounds i8, i8* %.block_descriptor, i64 32
  %0 = bitcast i8* %block.capture.addr to i8**
  %1 = load i8*, i8** %0, align 8, !tbaa !7
  %2 = tail call i8* @objc_retain(i8* %1) #2
  %3 = tail call i8* @objc_autoreleaseReturnValue(i8* %1) #2
  %4 = tail call i8* @objc_autoreleaseReturnValue(i8* %1) #2
  ret i8* %1
}

[trentxintong]

@gottesmm can you please take a look ? Thanks.

[gottesmm]

I do not work on objcarc any longer. +CC Duncan.

[dexonsmith]

Akira and/or John should take a look instead of me.

[gottesmm]

SGTM.

[rjmccall]

Alternative #1 is actually the right option — only it shouldn't be an objc_retain, it should be an objc_retainAutoreleasedReturnValue.

[danzimm]

@rjmccall ah right, good point -- I think it's ok to elide that objc_retainAutoreleasedReturnValue/objc_autoreleaseReturnValue pair in this case though, since all of this is generated within clang itself (and thus we can make the guarentee that the object will properly live until the return of the enclosing block) -- it reduces retain count churn this way too

[danzimm]

Or do we need to abide by those semantics strictly here? Could you expand on why that is, if that's the case?

[rjmccall]

In D41050#952720, @danzimm wrote:

Or do we need to abide by those semantics strictly here? Could you expand on why that is, if that's the case?

The autoreleased-return-value optimization cannot be understood purely in terms of its impact on the retain count; it's a hand-off from the callee to the caller the requires a specific code-generation pattern in order to work.

Here, calling retainAutoreleasedRV in the block is necessary in order to prevent the return value from the lambda from actually being autoreleased. The block then owns a retain on the return value and must call autoreleaseRV in order to balance that out and transfer the object to its caller. The caller will hopefully also call retainAutoreleasedRV, thus completing the transfer of the return value all the way to the ultimate caller without actually performing any retains or releases.

John.

[danzimm]

Call objc_retainAutoreleasedReturnValue after invoking a wrapped lambda

[danzimm]

@rjmccall aha, right - thanks for explaining that to me (sorry for the bad logic I had earlier).

It turns out this was also broken for a lambda that was auto-converted to a function pointer who returned an ObjC object. I changed the test case to reflect the more general situation, but I'm still unsure about the quality of the tests. Is it ok to rely on optimization passes to inline certain calls?

[danzimm]

Remove unnecessary change of braces

[rjmccall]

We do try to avoid relying on optimizer behavior in our tests. I don't know why you need that here.

[danzimm]

Change tests to use non-O2 generated IR. It looks like the combined objc_retainAutoreleasedReturnValue/objc_autoreleaseReturnValue calls annihilate each other and we just get a call/ret.

[danzimm]

Ah, just found test/Transforms/ObjCARC/rv.ll test3:

; Delete a redundant retainRV,autoreleaseRV when forwaring a call result
; directly to a return value.

; CHECK-LABEL: define i8* @test3(
; CHECK: call i8* @returner()
; CHECK-NEXT: ret i8* %call
define i8* @test3() {
entry:
  %call = call i8* @returner()
  %0 = call i8* @objc_retainAutoreleasedReturnValue(i8* %call) nounwind
  %1 = call i8* @objc_autoreleaseReturnValue(i8* %0) nounwind
  ret i8* %1
}

[rjmccall]

In D41050#953917, @danzimm wrote:

Change tests to use non-O2 generated IR. It looks like the combined objc_retainAutoreleasedReturnValue/objc_autoreleaseReturnValue calls annihilate each other and we just get a call/ret.

Is that really happening at -O0? Maybe you need to add -disable-llvm-optzns to the test line if so.

[danzimm]

In D41050#954668, @rjmccall wrote:

In D41050#953917, @danzimm wrote:

Change tests to use non-O2 generated IR. It looks like the combined objc_retainAutoreleasedReturnValue/objc_autoreleaseReturnValue calls annihilate each other and we just get a call/ret.

Is that really happening at -O0? Maybe you need to add -disable-llvm-optzns to the test line if so.

Unfortunately it looks like adding -disable-llvm-optzns and/or -disable-llvm-passes (I also manually added -O0 since I don't know what clang defaults to) doesn't generate the explicit call to objc_retainAutoreleasedReturnValue. Should we add a flag to disable that merging?

[danzimm]

I just dug into how the ARC optimization pass is invoked... it really shouldn't be invoked if -disable-llvm-passes is passed (or -disable-llvm-optzns which appears to just alias the first). Can you verify that my command is sane:

/Users/danzimm/oss/build/bin/clang -cc1 -internal-isystem /Users/danzimm/oss/build/lib/clang/6.0.0/include -nostdsysteminc -triple x86_64-apple-macosx10.12.0 -disable-llvm-passes -emit-llvm -fblocks -fobjc-arc -std=c++11 -O0 -o - /Users/danzimm/oss/llvm/tools/clang/test/CodeGenObjCXX/arc-forwarded-lambda-call.mm

[danzimm]

Pass -disable-llvm-passes with -O3 so that we can test that the IR we generate actually is generated

[danzimm]

Remove unnecessary checks from tests (sorry for unbatched updates)

[rjmccall]

Heh, alright, that works. It's unfortunate that -disable-llvm-passes doesn't suppress running the pass under -O0; that seems like an oversight.

Anyway, LGTM.

[dexonsmith]

In D41050#954863, @rjmccall wrote:

Heh, alright, that works. It's unfortunate that -disable-llvm-passes doesn't suppress running the pass under -O0; that seems like an oversight.

Anyway, LGTM.

I suspect -O0 just generates different IR. You can confirm what passes are executed by adding -mllvm -debug-pass=Executions.

[rjmccall]

Oh, yes, that could be.

[danzimm]

@dexonsmith Here are my results after passing those extra flags with -O3 (/Users/danzimm/oss/build/bin/clang -cc1 -internal-isystem /Users/danzimm/oss/build/lib/clang/6.0.0/include -nostdsysteminc -triple x86_64-apple-macosx10.12.0 -emit-llvm -disable-llvm-passes -O3 -fblocks -fobjc-arc -fobjc-runtime-has-weak -std=c++11 -mllvm -debug-pass=Executions -o ~/foo.ll /Users/danzimm/oss/llvm/tools/clang/test/CodeGenObjCXX/arc-forwarded-lambda-call.mm):

Pass Arguments:  -tti
Target Transform Information
  FunctionPass Manager
Pass Arguments:  -tti
Target Transform Information
  ModulePass Manager
    Print Module IR
[2017-12-14 09:47:12.252472000] 0x7f87b90016f0   Executing Pass 'Print Module IR' on Module '/Users/danzimm/oss/llvm/tools/clang/test/CodeGenObjCXX/arc-forwarded-lambda-call.mm'...
[2017-12-14 09:47:12.254702000] 0x7f87b90016f0    Freeing Pass 'Print Module IR' on Module '/Users/danzimm/oss/llvm/tools/clang/test/CodeGenObjCXX/arc-forwarded-lambda-call.mm'...
Pass Arguments:  -tti
Target Transform Information
  ModulePass Manager

and with -O0 (/Users/danzimm/oss/build/bin/clang -cc1 -internal-isystem /Users/danzimm/oss/build/lib/clang/6.0.0/include -nostdsysteminc -triple x86_64-apple-macosx10.12.0 -emit-llvm -disable-llvm-passes -O0 -fblocks -fobjc-arc -fobjc-runtime-has-weak -std=c++11 -mllvm -debug-pass=Executions -o ~/foo.ll /Users/danzimm/oss/llvm/tools/clang/test/CodeGenObjCXX/arc-forwarded-lambda-call.mm):

Pass Arguments:  -tti
Target Transform Information
  FunctionPass Manager
Pass Arguments:  -tti
Target Transform Information
  ModulePass Manager
    Print Module IR
[2017-12-14 09:47:20.884147000] 0x7ff71ed097d0   Executing Pass 'Print Module IR' on Module '/Users/danzimm/oss/llvm/tools/clang/test/CodeGenObjCXX/arc-forwarded-lambda-call.mm'...
[2017-12-14 09:47:20.886182000] 0x7ff71ed097d0    Freeing Pass 'Print Module IR' on Module '/Users/danzimm/oss/llvm/tools/clang/test/CodeGenObjCXX/arc-forwarded-lambda-call.mm'...
Pass Arguments:  -tti
Target Transform Information
  ModulePass Manager

[danzimm]

Also, I don't have commit access. Could someone else commit this for me?

[rjmccall]

Sure, r320721.

lib/CodeGen/CGBlocks.cpp

for (const auto &CI : blockDecl->captures()) {
     setAddrOfLocalVar(variable, alloca);
   }
 
   // Save a spot to insert the debug information for all the DeclRefExprs.
   llvm::BasicBlock *entry = Builder.GetInsertBlock();
   llvm::BasicBlock::iterator entry_ptr = Builder.GetInsertPoint();
   --entry_ptr;
 
-  if (IsLambdaConversionToBlock)
+  if (IsLambdaConversionToBlock) {
     EmitLambdaBlockInvokeBody();
-  else {
+  } else {
     PGO.assignRegionCounters(GlobalDecl(blockDecl), fn);
     incrementProfileCounter(blockDecl->getBody());
     EmitStmt(blockDecl->getBody());
   }
 
   // Remember where we were...
   llvm::BasicBlock *resume = Builder.GetInsertBlock();
 

lib/CodeGen/CGClass.cpp

void CodeGenFunction::EmitForwardingCallToLambda(
   // the call can't be variadic anyway --- it's impossible to forward
   // variadic arguments.
 
   // Now emit our call.
   auto callee = CGCallee::forDirect(calleePtr, callOperator);
   RValue RV = EmitCall(calleeFnInfo, callee, returnSlot, callArgs);
 
   // If necessary, copy the returned value into the slot.
-  if (!resultType->isVoidType() && returnSlot.isNull())
+  if (!resultType->isVoidType() && returnSlot.isNull()) {
+    if (getLangOpts().ObjCAutoRefCount && resultType->isObjCRetainableType()) {
+      RV = RValue::get(EmitARCRetainAutoreleasedReturnValue(RV.getScalarVal()));
+    }
     EmitReturnOfRValue(RV, resultType);
-  else
+  } else
     EmitBranchThroughCleanup(ReturnBlock);
 }
 
 void CodeGenFunction::EmitLambdaBlockInvokeBody() {
   const BlockDecl *BD = BlockInfo->getBlockDecl();
   const VarDecl *variable = BD->capture_begin()->getVariable();
   const CXXRecordDecl *Lambda = variable->getType()->getAsCXXRecordDecl();
   const CXXMethodDecl *CallOp = Lambda->getLambdaCallOperator();

test/CodeGenObjCXX/arc-forwarded-lambda-call.mm

+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.12.0 -emit-llvm -fblocks -fobjc-arc -fobjc-runtime-has-weak -std=c++11 -O2 -o - %s | FileCheck %s
+
+void test0(id x) {
+  extern void test0_helper(id (^)(void));
+  test0_helper([=]() { return x; });
+  // CHECK-LABEL: define internal i8* @___Z5test0P11objc_object_block_invoke
+  // CHECK: {{%.*}} = tail call i8* @objc_retain(i8* [[T0:%.*]])
+  // CHECK-NEXT: {{%.*}} = tail call i8* @objc_autoreleaseReturnValue(i8* [[T0]])
+  // CHECK-NEXT: ret i8* [[T0]]
+}
+
+id test1_rv;
+
+void test1() {
+  extern void test1_helper(id (*)(void));
+  test1_helper([](){ return test1_rv; });
+  // CHECK-LABEL: define internal i8* @"_ZZ5test1vEN3$_18__invokeEv"
+  // CHECK: {{%.*}} = tail call i8* @objc_retain(i8* [[T0:%.*]])
+  // CHECK-NEXT: {{%.*}} = tail call i8* @objc_autoreleaseReturnValue(i8* [[T0]])
+  // CHECK-NEXT: ret i8* [[T0]]
+}