Disallow returning a __block variable via a move

[ahatanak]

Summary

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

[ahatanak]

Posting John's review comment, which was sent to the review I abandoned:

"Hmm. The behavior I think we actually want here is that we should move out of the __block variable but not perform NRVO, essentially like we would if the variable were a parameter.

The block byref copy helper should always be doing a move if it can, regardless of whether the function is returned. That's independent of this, though."

[ahatanak]

In D29908#676589, @ahatanak wrote:

Posting John's review comment, which was sent to the review I abandoned:

"Hmm. The behavior I think we actually want here is that we should move out of the __block variable but not perform NRVO, essentially like we would if the variable were a parameter.

I think that's what's happening. Because of the changes made in r274291, NRVO isn't performed, but the move constructor instead of the copy constructor is called to return the shared_ptr. The shared_ptr in the heap is moved to returned shared_ptr (%agg.result), so when the block function passed to dispatch_async is executed, it segfaults because the object "ret" used to point to has been destructed.

If that's the case, should we conclude that clang is behaving as expected?

The block byref copy helper should always be doing a move if it can, regardless of whether the function is returned. That's independent of this, though."

[rjmccall]

In D29908#676601, @ahatanak wrote:

In D29908#676589, @ahatanak wrote:

Posting John's review comment, which was sent to the review I abandoned:

"Hmm. The behavior I think we actually want here is that we should move out of the __block variable but not perform NRVO, essentially like we would if the variable were a parameter.

I think that's what's happening. Because of the changes made in r274291, NRVO isn't performed, but the move constructor instead of the copy constructor is called to return the shared_ptr. The shared_ptr in the heap is moved to returned shared_ptr (%agg.result), so when the block function passed to dispatch_async is executed, it segfaults because the object "ret" used to point to has been destructed.

If that's the case, should we conclude that clang is behaving as expected?

The block byref copy helper should always be doing a move if it can, regardless of whether the function is returned. That's independent of this, though."

Oh, I see. Just to clarify: it doesn't matter that the object "ret" used to point to has been destructed, what matters is that "ret" now holds a null reference because it's been moved out of.

I retract my comment; I agree there's a bug here. We should not implicitly move out of block variables during a return because we cannot assume that the variable will no longer be used. Please update the comment to correctly identify this as the reasoning; it has nothing to do with whether block variables technically *can* support NRVO.

[ahatanak]

Update comment.

[ahatanak]

In D29908#676788, @rjmccall wrote:

Oh, I see. Just to clarify: it doesn't matter that the object "ret" used to point to has been destructed, what matters is that "ret" now holds a null reference because it's been moved out of.

Yes, that's right.

I retract my comment; I agree there's a bug here. We should not implicitly move out of block variables during a return because we cannot assume that the variable will no longer be used. Please update the comment to correctly identify this as the reasoning; it has nothing to do with whether block variables technically *can* support NRVO.

OK, thanks. I was wondering whether there is something we should or can do in IRGen to make it work, but it looks like we have to just disable moving out of block variables.

I'll commit this patch later today.

cfe/trunk/lib/Sema/SemaStmt.cpp

bool Sema::isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD,
   if (VD->getKind() != Decl::Var &&
       !(AllowParamOrMoveConstructible && VD->getKind() == Decl::ParmVar))
     return false;
   if (VD->isExceptionVariable()) return false;
 
   // ...automatic...
   if (!VD->hasLocalStorage()) return false;
 
+  // Return false if VD is a __block variable. We don't want to implicitly move
+  // out of a __block variable during a return because we cannot assume the
+  // variable will no longer be used.
+  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>() &&
       Context.getDeclAlign(VD) > Context.getTypeAlignInChars(VD->getType()))
     return false;
 
   return true;
 }

cfe/trunk/test/SemaObjCXX/blocks.mm

-// 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));
 void foo(id <NSObject>(^objectCreationBlock)(void)) {
     return bar(objectCreationBlock); // OK
 }
 
 void bar2(id(*)(void));
namespace DependentReturn {
 
   struct X { };
   void operator+(X, X);
   bool operator==(X, X);
   bool operator!=(X, X);
 
   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'}}
+}
+}