Re: 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  wrote:
> On 15 February 2017 at 11:50, Hans Wennborg  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 
>> wrote:
>> > Hans,
>> >
>> > Can this be merged to 4.0 too?
>> >
>> >> On Feb 14, 2017, at 9:15 PM, Akira Hatanaka via cfe-commits
>> >>  wrote:
>> >>
>> >> Author: ahatanak
>> >> Date: Tue Feb 14 23:15:28 2017
>> >> New Revision: 295150
>> >>
>> >> URL: http://llvm.org/viewvc/llvm-project?rev=295150=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=295149=295150=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()) 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()) return false;
>> >> -
>> >>   // Variables with higher required alignment than their type's ABI
>> >>   // alignment cannot use NRVO.
>> >>   if (!VD->getType()->isDependentType() && VD->hasAttr()
>> >> &&
>> >>
>> >> Modified: cfe/trunk/test/SemaObjCXX/blocks.mm
>> >> URL:
>> >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaObjCXX/blocks.mm?rev=295150=295149=295150=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);
>> >> }
>> >> +
>> >> +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

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

[Richard Smith via cfe-commits]

On 15 February 2017 at 11:50, Hans Wennborg  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 
> 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=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=295149=295150=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()) 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()) return false;
> >> -
> >>   // Variables with higher required alignment than their type's ABI
> >>   // alignment cannot use NRVO.
> >>   if (!VD->getType()->isDependentType() && VD->hasAttr()
> &&
> >>
> >> Modified: cfe/trunk/test/SemaObjCXX/blocks.mm
> >> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/
> SemaObjCXX/blocks.mm?rev=295150=295149=295150=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);
> >> }
> >> +
> >> +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

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

[Hans Wennborg via cfe-commits]

+Richard for risk/reward analysis.

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  wrote:
> Hans,
>
> Can this be merged to 4.0 too?
>
>> On Feb 14, 2017, at 9:15 PM, Akira Hatanaka via cfe-commits 
>>  wrote:
>>
>> Author: ahatanak
>> Date: Tue Feb 14 23:15:28 2017
>> New Revision: 295150
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=295150=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=295149=295150=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()) 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()) return false;
>> -
>>   // Variables with higher required alignment than their type's ABI
>>   // alignment cannot use NRVO.
>>   if (!VD->getType()->isDependentType() && VD->hasAttr() &&
>>
>> Modified: cfe/trunk/test/SemaObjCXX/blocks.mm
>> URL: 
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaObjCXX/blocks.mm?rev=295150=295149=295150=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);
>> }
>> +
>> +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

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

[Akira Hatanaka via cfe-commits]

Hans,

Can this be merged to 4.0 too?

> On Feb 14, 2017, at 9:15 PM, Akira Hatanaka via cfe-commits 
>  wrote:
> 
> Author: ahatanak
> Date: Tue Feb 14 23:15:28 2017
> New Revision: 295150
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=295150=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=295149=295150=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()) 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()) return false;
> -
>   // Variables with higher required alignment than their type's ABI
>   // alignment cannot use NRVO.
>   if (!VD->getType()->isDependentType() && VD->hasAttr() &&
> 
> Modified: cfe/trunk/test/SemaObjCXX/blocks.mm
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaObjCXX/blocks.mm?rev=295150=295149=295150=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);
> }
> +
> +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