[PATCH] D29908: Disallow returning a __block variable via a move

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak created this revision.

This patch disables moving a __block variable to return it from a function, which was causing a crash that occurred when compiling and executing the following code:

  #include <dispatch/dispatch.h>
  #include <memory>
  #include <cstdio>
  
  class A {
  public:
    A(int x) : internal(x) {}
    int internal;
  };
  
  dispatch_semaphore_t sema;
  dispatch_queue_t q;
  
  std::shared_ptr<A> dispatch_function(int x) {
    __block std::shared_ptr<A> ret = std::shared_ptr<A>(new A(x));
    dispatch_async(q, ^{
      printf("%d\n", ret->internal); // segfaults here
      dispatch_semaphore_signal(sema);
    });
    return ret;
  }
  
  int main() {
    q = dispatch_queue_create("com.example.MyCustomQueue", NULL);
    sema = dispatch_semaphore_create(0);
    dispatch_function(100);
    dispatch_semaphore_wait(sema, DISPATCH_TIME_FOREVER);
    dispatch_release(sema);
    return 0;
  }

This is how the crash occurs:

1. When dispatch_async is called, the block passed to it is copied to the heap, and when that happens the shared_ptr is moved to the heap. Note that the shared_ptr is moved, not copied, in @__Block_byref_object_copy_ because Sema::CheckCompleteVariableDeclaration passes " /*AllowNRVO=*/true" to the call to PerformMoveOrCopyInitialization. I'm not sure whether this is correct or not, but even after changing it to pass AllowNRVO=false, the code still crashes.

2. "ret" is moved to the return aggregate. Since "ret" is a __block variable, it refers to the shared_ptr on the heap, not on the stack, so the one on the heap is moved.

3. The destructor for the shared_ptr on the stack is called when dispatch_function exits.

4. When dispatch_function returns in "main", the returned shared_ptr is immediately destructed, and the object it points to is destructed too when the reference count drops to zero.

5. The code crashes when the block passed to dispatch_async is executed since the object ret  used to point to has been destructed.

An alternate solution that fixes the crash is to somehow pass followForward=false to CodeGenFunction::emitBlockByrefAddress so that it emits the address of the shared_ptr on the stack, not on the heap, but I guess that is not always correct and can break some other code.

rdar://problem/28181080


https://reviews.llvm.org/D29908

Files:
  lib/Sema/SemaStmt.cpp
  test/SemaObjCXX/blocks.mm


Index: test/SemaObjCXX/blocks.mm
===================================================================
--- test/SemaObjCXX/blocks.mm
+++ test/SemaObjCXX/blocks.mm
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -fblocks -Wno-objc-root-class %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fblocks -Wno-objc-root-class -std=c++11 %s
 @protocol NSObject;
 
 void bar(id(^)(void));
@@ -144,3 +144,17 @@
 
   template void f<X>(X);
 }
+
+namespace MoveBlockVariable {
+struct B0 {
+};
+
+struct B1 { // expected-note 2 {{candidate constructor (the implicit}}
+  B1(B0&&); // expected-note {{candidate constructor not viable}}
+};
+
+B1 test_move() {
+  __block B0 b;
+  return b; // expected-error {{no viable conversion from returned value of type 'MoveBlockVariable::B0' to function return type 'MoveBlockVariable::B1'}}
+}
+}
Index: lib/Sema/SemaStmt.cpp
===================================================================
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -2743,15 +2743,15 @@
   // ...automatic...
   if (!VD->hasLocalStorage()) return false;
 
+  // __block variables can't be allocated in a way that permits NRVO.
+  if (VD->hasAttr<BlocksAttr>()) return false;
+
   if (AllowParamOrMoveConstructible)
     return true;
 
   // ...non-volatile...
   if (VD->getType().isVolatileQualified()) return false;
 
-  // __block variables can't be allocated in a way that permits NRVO.
-  if (VD->hasAttr<BlocksAttr>()) return false;
-
   // Variables with higher required alignment than their type's ABI
   // alignment cannot use NRVO.
   if (!VD->getType()->isDependentType() && VD->hasAttr<AlignedAttr>() &&


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D29908.88249.patch
Type: text/x-patch
Size: 1634 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170213/17544e1f/attachment.bin>