On Wed, Feb 15, 2017 at 12:17 PM, Richard Smith <richardsm...@google.com> wrote: > 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.
Very good; merged in r295234. > >> >> 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
