r295150 - [Sema] Disallow returning a __block variable via a move.

[Hans Wennborg via cfe-commits]

On Wed, Feb 15, 2017 at 12:17 PM, Richard Smith <richardsmith at google.com> wrote:
> On 15 February 2017 at 11:50, Hans Wennborg <hans at 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 <ahatanaka at 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 at 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 at lists.llvm.org
>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> >
>
>