On 15 February 2017 at 11:50, Hans Wennborg <h...@chromium.org> wrote:
> +Richard for risk/reward analysis. > This is an extremely safe change, and fixes what amounts to a subtle miscompile. I think we should take it. > r274291 was also in 3.9, so this isn't strictly speaking a regression. > > On Wed, Feb 15, 2017 at 11:43 AM, Akira Hatanaka <ahatan...@apple.com> > wrote: > > Hans, > > > > Can this be merged to 4.0 too? > > > >> On Feb 14, 2017, at 9:15 PM, Akira Hatanaka via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> > >> Author: ahatanak > >> Date: Tue Feb 14 23:15:28 2017 > >> New Revision: 295150 > >> > >> URL: http://llvm.org/viewvc/llvm-project?rev=295150&view=rev > >> Log: > >> [Sema] Disallow returning a __block variable via a move. > >> > >> r274291 made changes to prefer calling a move constructor to calling a > >> copy constructor when returning from a function. This caused programs to > >> crash when a __block variable in the heap was moved out and used later. > >> > >> This commit fixes the bug by disallowing moving out of __block variables > >> implicitly. > >> > >> rdar://problem/28181080 > >> > >> Differential Revision: https://reviews.llvm.org/D29908 > >> > >> Modified: > >> cfe/trunk/lib/Sema/SemaStmt.cpp > >> cfe/trunk/test/SemaObjCXX/blocks.mm > >> > >> Modified: cfe/trunk/lib/Sema/SemaStmt.cpp > >> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/ > SemaStmt.cpp?rev=295150&r1=295149&r2=295150&view=diff > >> ============================================================ > ================== > >> --- cfe/trunk/lib/Sema/SemaStmt.cpp (original) > >> +++ cfe/trunk/lib/Sema/SemaStmt.cpp Tue Feb 14 23:15:28 2017 > >> @@ -2743,15 +2743,17 @@ bool Sema::isCopyElisionCandidate(QualTy > >> // ...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>() > && > >> > >> Modified: cfe/trunk/test/SemaObjCXX/blocks.mm > >> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ > SemaObjCXX/blocks.mm?rev=295150&r1=295149&r2=295150&view=diff > >> ============================================================ > ================== > >> --- cfe/trunk/test/SemaObjCXX/blocks.mm (original) > >> +++ cfe/trunk/test/SemaObjCXX/blocks.mm Tue Feb 14 23:15:28 2017 > >> @@ -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 @@ namespace DependentReturn { > >> > >> 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'}} > >> +} > >> +} > >> > >> > >> _______________________________________________ > >> cfe-commits mailing list > >> cfe-commits@lists.llvm.org > >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits