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>() &&
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>() &&
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits