<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 15 February 2017 at 11:50, Hans Wennborg <span dir="ltr"><<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">+Richard for risk/reward analysis.<br></blockquote><div><br></div><div>This is an extremely safe change, and fixes what amounts to a subtle miscompile. I think we should take it.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
r274291 was also in 3.9, so this isn't strictly speaking a regression.<br>
<br>
On Wed, Feb 15, 2017 at 11:43 AM, Akira Hatanaka <<a href="mailto:ahatanaka@apple.com">ahatanaka@apple.com</a>> wrote:<br>
> Hans,<br>
><br>
> Can this be merged to 4.0 too?<br>
><br>
>> On Feb 14, 2017, at 9:15 PM, Akira Hatanaka via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>> Author: ahatanak<br>
>> Date: Tue Feb 14 23:15:28 2017<br>
>> New Revision: 295150<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=295150&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=295150&view=rev</a><br>
>> Log:<br>
>> [Sema] Disallow returning a __block variable via a move.<br>
>><br>
>> r274291 made changes to prefer calling a move constructor to calling a<br>
>> copy constructor when returning from a function. This caused programs to<br>
>> crash when a __block variable in the heap was moved out and used later.<br>
>><br>
>> This commit fixes the bug by disallowing moving out of __block variables<br>
>> implicitly.<br>
>><br>
>> rdar://problem/28181080<br>
>><br>
>> Differential Revision: <a href="https://reviews.llvm.org/D29908" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D29908</a><br>
>><br>
>> Modified:<br>
>> cfe/trunk/lib/Sema/SemaStmt.<wbr>cpp<br>
>> cfe/trunk/test/SemaObjCXX/<a href="http://blocks.mm" rel="noreferrer" target="_blank">bloc<wbr>ks.mm</a><br>
>><br>
>> Modified: cfe/trunk/lib/Sema/SemaStmt.<wbr>cpp<br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaStmt.cpp?rev=295150&r1=295149&r2=295150&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/lib/Sema/<wbr>SemaStmt.cpp?rev=295150&r1=<wbr>295149&r2=295150&view=diff</a><br>
>> ==============================<wbr>==============================<wbr>==================<br>
>> --- cfe/trunk/lib/Sema/SemaStmt.<wbr>cpp (original)<br>
>> +++ cfe/trunk/lib/Sema/SemaStmt.<wbr>cpp Tue Feb 14 23:15:28 2017<br>
>> @@ -2743,15 +2743,17 @@ bool Sema::isCopyElisionCandidate(<wbr>QualTy<br>
>> // ...automatic...<br>
>> if (!VD->hasLocalStorage()) return false;<br>
>><br>
>> + // Return false if VD is a __block variable. We don't want to implicitly move<br>
>> + // out of a __block variable during a return because we cannot assume the<br>
>> + // variable will no longer be used.<br>
>> + if (VD->hasAttr<BlocksAttr>()) return false;<br>
>> +<br>
>> if (<wbr>AllowParamOrMoveConstructible)<br>
>> return true;<br>
>><br>
>> // ...non-volatile...<br>
>> if (VD->getType().<wbr>isVolatileQualified()) return false;<br>
>><br>
>> - // __block variables can't be allocated in a way that permits NRVO.<br>
>> - if (VD->hasAttr<BlocksAttr>()) return false;<br>
>> -<br>
>> // Variables with higher required alignment than their type's ABI<br>
>> // alignment cannot use NRVO.<br>
>> if (!VD->getType()-><wbr>isDependentType() && VD->hasAttr<AlignedAttr>() &&<br>
>><br>
>> Modified: cfe/trunk/test/SemaObjCXX/<a href="http://blocks.mm" rel="noreferrer" target="_blank">bloc<wbr>ks.mm</a><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaObjCXX/blocks.mm?rev=295150&r1=295149&r2=295150&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/test/<wbr>SemaObjCXX/blocks.mm?rev=<wbr>295150&r1=295149&r2=295150&<wbr>view=diff</a><br>
>> ==============================<wbr>==============================<wbr>==================<br>
>> --- cfe/trunk/test/SemaObjCXX/<a href="http://blocks.mm" rel="noreferrer" target="_blank">bloc<wbr>ks.mm</a> (original)<br>
>> +++ cfe/trunk/test/SemaObjCXX/<a href="http://blocks.mm" rel="noreferrer" target="_blank">bloc<wbr>ks.mm</a> Tue Feb 14 23:15:28 2017<br>
>> @@ -1,4 +1,4 @@<br>
>> -// RUN: %clang_cc1 -fsyntax-only -verify -fblocks -Wno-objc-root-class %s<br>
>> +// RUN: %clang_cc1 -fsyntax-only -verify -fblocks -Wno-objc-root-class -std=c++11 %s<br>
>> @protocol NSObject;<br>
>><br>
>> void bar(id(^)(void));<br>
>> @@ -144,3 +144,17 @@ namespace DependentReturn {<br>
>><br>
>> template void f<X>(X);<br>
>> }<br>
>> +<br>
>> +namespace MoveBlockVariable {<br>
>> +struct B0 {<br>
>> +};<br>
>> +<br>
>> +struct B1 { // expected-note 2 {{candidate constructor (the implicit}}<br>
>> + B1(B0&&); // expected-note {{candidate constructor not viable}}<br>
>> +};<br>
>> +<br>
>> +B1 test_move() {<br>
>> + __block B0 b;<br>
>> + return b; // expected-error {{no viable conversion from returned value of type 'MoveBlockVariable::B0' to function return type 'MoveBlockVariable::B1'}}<br>
>> +}<br>
>> +}<br>
>><br>
>><br>
>> ______________________________<wbr>_________________<br>
>> cfe-commits mailing list<br>
>> <a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
><br>
</blockquote></div><br></div></div>