Call objc_retainBlock before passing a block as a variadic argument

[ahatanak]

Summary

Copy the block to the heap before passing it to the callee in case the block escapes in the callee.

rdar://problem/55683462

[rjmccall]

Patch looks good, but could you describe this behavior in the ARC documentation?

[ahatanak]

Explain why this change is needed in ARC documentation.

[rjmccall]

clang/docs/AutomaticReferenceCounting.rst

1866

"to a non-block pointer type (such as `id`)", please. Otherwise looks good. I didn't realize that this conversion behavior wasn't documented at all, so thank you for adding it.

[ahatanak]

clang/docs/AutomaticReferenceCounting.rst

1866

I thought it was documented too and searched for it, but couldn't find it anywhere in the ARC doc.

clang/docs/AutomaticReferenceCounting.rst

 by initializing the heap copy with the result of moving from the stack copy.
 
 With the exception of retains done as part of initializing a ``__strong``
 parameter variable or reading a ``__weak`` variable, whenever these semantics
 call for retaining a value of block-pointer type, it has the effect of a
 ``Block_copy``.  The optimizer may remove such copies when it sees that the
 result is used only as an argument to a call.
 
+When a block pointer type is converted to a non-block pointer type (such as
+``id``), ``Block_copy`` is called. This is necessary because a block allocated
+on the stack won't get copied to the heap when the non-block pointer escapes.
+A block pointer is implicitly converted to ``id`` when it is passed to a
+function as a variadic argument.
+
 .. _arc.misc.exceptions:
 
 Exceptions
 ----------
 
 By default in Objective C, ARC is not exception-safe for normal releases:
 
 * It does not end the lifetime of ``__strong`` variables when their scopes are

clang/lib/Sema/SemaExpr.cpp

if (Proto->getReturnType() == Context.UnknownAnyTy && FDecl &&
         AllArgs.push_back(arg.get());
       }
 
     // Otherwise do argument promotion, (C99 6.5.2.2p7).
     } else {
       for (Expr *A : Args.slice(ArgIx)) {
         ExprResult Arg = DefaultVariadicArgumentPromotion(A, CallType, FDecl);
         Invalid |= Arg.isInvalid();
+        // Copy blocks to the heap.
+        if (A->getType()->isBlockPointerType())
+          maybeExtendBlockObject(Arg);
         AllArgs.push_back(Arg.get());
       }
     }
 
     // Check for array bounds violations.
     for (Expr *A : Args.slice(ArgIx))
       CheckArrayAccess(A);
   }

clang/test/CodeGenObjC/arc-blocks.m

 // CHECK-UNOPT: store i8* [[RETAINED]], i8** [[BLOCKCAPTURED]]
 // CHECK-UNOPT: call void @llvm.objc.storeStrong(i8** [[CAPTUREFIELD]], i8* null)
 
 void test20_callee(void (^)());
 void test20(const id x) {
   test20_callee(^{ (void)x; });
 }
 
+// CHECK-LABEL: define void @test21(
+// CHECK: %[[V6:.*]] = call i8* @llvm.objc.retainBlock(
+// CHECK: %[[V7:.*]] = bitcast i8* %[[V6]] to void ()*
+// CHECK: call void (i32, ...) @test21_callee(i32 1, void ()* %[[V7]]),
+
+void test21_callee(int n, ...);
+void test21(id x) {
+  test21_callee(1, ^{ (void)x; });
+}
+
 // CHECK: attributes [[NUW]] = { nounwind }
 // CHECK-UNOPT: attributes [[NUW]] = { nounwind }