[PATCH] D41050: Fix over-release of return value of lambda implicitly converted to block

[Dan Zimmerman via Phabricator via cfe-commits]

danzimm created this revision.
Herald added subscribers: cfe-commits, kosarev.

Clang has a pretty cool feature right now that will allow you to use a lambda as a block. Unfortunately there's a bug in this conversion if the return value of the block is an ObjC object and arc is enabled -- the return value gets over-released. An example of this causing a crash that shouldn't occur can be seen 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;
  }

It looks like when a lambda is auto-converted to a block it generates a block that just wraps the lambda and forwards all args to it and the return value of the lambda back to be the return value of the block. Currently if the block returns an ObjC object both the lambda that's generated and the block that wraps it autorelease the return value, which effectively over-releases the return value. There are a few options that I considered and one that stuck out to me:

1. Add a retain call on the return value after invoking the enclosed lambda -- this way we maintain proper retain count
2. Stop autoreleasing the return value of the lambda
3. Stop autoreleasing the return value of the block

I opted against the first case since it would mean unnecessary extra reference count churn. I didn't see an easy way to implement the second option as the enclosing lambda is generated as a global function definition, without the metadata that a block encloses it (and thus there's no easy way that I could see to differentiate the lambda-converted-to-block function definition from other global function definitions). Thus I came to the third option which is easily implementable since the codegen for blocks must recognize whether or not it encloses a lambda (as it changes what it generates if it does).

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
  }


Repository:
  rC Clang

https://reviews.llvm.org/D41050

Files:
  lib/CodeGen/CGBlocks.cpp
  test/CodeGenObjCXX/arc-block-lambda-conversion.mm


Index: test/CodeGenObjCXX/arc-block-lambda-conversion.mm
===================================================================
--- /dev/null
+++ test/CodeGenObjCXX/arc-block-lambda-conversion.mm
@@ -0,0 +1,10 @@
+// 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]]
+}
Index: lib/CodeGen/CGBlocks.cpp
===================================================================
--- lib/CodeGen/CGBlocks.cpp
+++ lib/CodeGen/CGBlocks.cpp
@@ -1450,9 +1450,12 @@
   llvm::BasicBlock::iterator entry_ptr = Builder.GetInsertPoint();
   --entry_ptr;
 
-  if (IsLambdaConversionToBlock)
+  if (IsLambdaConversionToBlock) {
+    // The lambda that's generated will emit a call to
+    // objc_autoreleaseReturnValue for us if necessary
+    AutoreleaseResult = false;
     EmitLambdaBlockInvokeBody();
-  else {
+  } else {
     PGO.assignRegionCounters(GlobalDecl(blockDecl), fn);
     incrementProfileCounter(blockDecl->getBody());
     EmitStmt(blockDecl->getBody());


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D41050.126280.patch
Type: text/x-patch
Size: 1408 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171209/87103d03/attachment.bin>