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

[Richard Smith via cfe-commits]

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.


> 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170215/a78542fe/attachment.html>