<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Nov 19, 2013 at 9:56 PM, Kuperstein, Michael M <span dir="ltr"><<a href="mailto:michael.m.kuperstein@intel.com" target="_blank">michael.m.kuperstein@intel.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div lang="EN-US" link="blue" vlink="purple">
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">What I’m trying to say is that according to my understanding of the C++11 memory model, even in that small reproducer, the store to g and the load from g are
in fact a data race.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">(This is regardless of the fact the load is protected by a branch that is not taken.)</span></p></div>
</div></blockquote><div><br></div><div>My understanding of the standard is quite the opposite. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple">
<div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> Kostya Serebryany [mailto:<a href="mailto:kcc@google.com" target="_blank">kcc@google.com</a>]
<br>
<b>Sent:</b> Tuesday, November 19, 2013 19:46<br>
<b>To:</b> Kuperstein, Michael M<br>
<b>Cc:</b> Evgeniy Stepanov; LLVM Developers Mailing List</span></p><div><div class="h5"><br>
<b>Subject:</b> Re: [LLVMdev] Curiosity about transform changes under Sanitizers (Was: [PATCH] Disable branch folding with MemorySanitizer)<u></u><u></u></div></div><p></p><div><div class="h5">
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal" style="line-height:115%"><u></u> <u></u></p>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt;line-height:115%"><u></u> <u></u></p>
<div>
<p class="MsoNormal" style="line-height:115%">On Tue, Nov 19, 2013 at 9:05 PM, Kuperstein, Michael M <<a href="mailto:michael.m.kuperstein@intel.com" target="_blank">michael.m.kuperstein@intel.com</a>> wrote:<u></u><u></u></p>
<p class="MsoNormal" style="line-height:115%">My $0.02 - I'm not sure the transformation introduces a data race.<br>
<br>
To the best of my understanding, the point of the C++11/C11 memory model is to allow a wide array of compiler transformations - including speculative loads - for non-atomic variables.<br>
I believe what's most likely happening (without looking at the Mozilla source) is that the original program contains a C++ data race, and the transformation exposes it to TSan.<u></u><u></u></p>
<div>
<p class="MsoNormal" style="line-height:115%"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal" style="line-height:115%">The original program is race-free. <u></u><u></u></p>
</div>
<div>
<p class="MsoNormal" style="line-height:115%">I've posted a minimized reproducer that actually triggers a tsan false positive at O1 here: <u></u><u></u></p>
</div>
<div>
<p class="MsoNormal" style="line-height:115%"><a href="https://code.google.com/p/thread-sanitizer/issues/detail?id=40#c5" target="_blank">https://code.google.com/p/thread-sanitizer/issues/detail?id=40#c5</a><u></u><u></u></p>
</div>
<div>
<p class="MsoNormal" style="line-height:115%"> <u></u><u></u></p>
</div>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0cm 0cm 0cm 6.0pt;margin-left:4.8pt;margin-right:0cm">
<div>
<div>
<p class="MsoNormal" style="line-height:115%"><br>
-----Original Message-----<br>
From: <a href="mailto:llvmdev-bounces@cs.uiuc.edu" target="_blank">llvmdev-bounces@cs.uiuc.edu</a> [mailto:<a href="mailto:llvmdev-bounces@cs.uiuc.edu" target="_blank">llvmdev-bounces@cs.uiuc.edu</a>] On Behalf Of Evgeniy Stepanov<br>
Sent: Tuesday, November 19, 2013 18:55<br>
To: Kostya Serebryany<br>
Cc: LLVM Developers Mailing List<br>
Subject: Re: [LLVMdev] Curiosity about transform changes under Sanitizers (Was: [PATCH] Disable branch folding with MemorySanitizer)<br>
<br>
The root cause of those issues is the fact that sanitizers verify<br>
C++-level semantics with LLVM IR level instrumentation. For example,<br>
speculative loads are OK in IR if it can be proved that the load won't trap, but in C++ it would be a data race.<br>
<br>
<br>
On Tue, Nov 19, 2013 at 8:38 PM, Kostya Serebryany <<a href="mailto:kcc@google.com" target="_blank">kcc@google.com</a>> wrote:<br>
><br>
><br>
><br>
> On Tue, Nov 19, 2013 at 8:25 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>> Just moving this branch of the thread out of the review because I<br>
>> don't want to derail the review thread...<br>
>><br>
>> Kostya - why are these two cases not optimization bugs in general?<br>
>> (why do they only affect sanitizers?)<br>
><br>
><br>
> The recent case from mozilla<br>
> (<a href="https://code.google.com/p/thread-sanitizer/issues/detail?id=40#c2" target="_blank">https://code.google.com/p/thread-sanitizer/issues/detail?id=40#c2</a>) is<br>
> a legal optimization -- it hoists a safe load (i.e. a load which is<br>
> known not to<br>
> fail) out of conditional branch.<br>
> It reduces the number of basic blocks and branches, and so I think<br>
> it's good in general.<br>
> I can't imagine a case where this optimization will break a valid program.<br>
> Which is the second one you are referring to?<br>
><br>
> --kcc<br>
><br>
>><br>
>><br>
>><br>
>><br>
>> On Mon, Nov 18, 2013 at 8:37 PM, Kostya Serebryany <<a href="mailto:kcc@google.com" target="_blank">kcc@google.com</a>> wrote:<br>
>>><br>
>>> And we've been just informed by the mozilla folks about yet another<br>
>>> case of optimization being hostile to sanitizers:<br>
>>> hoisting a safe load out of conditional branch introduces a race<br>
>>> which tsan happily reports.<br>
>>> <a href="https://code.google.com/p/thread-sanitizer/issues/detail?id=40#c2" target="_blank">
https://code.google.com/p/thread-sanitizer/issues/detail?id=40#c2</a><br>
>>><br>
>>> --kcc<br>
>>><br>
>>><br>
>>> On Tue, Nov 19, 2013 at 8:06 AM, Kostya Serebryany <<a href="mailto:kcc@google.com" target="_blank">kcc@google.com</a>><br>
>>> wrote:<br>
>>>><br>
>>>><br>
>>>><br>
>>>><br>
>>>> On Tue, Nov 19, 2013 at 1:27 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>><br>
>>>> wrote:<br>
>>>>><br>
>>>>> Do we have precedence for this kind of change (where sanitizers<br>
>>>>> affect optimizations in arbitrary internal ways - not simply by<br>
>>>>> enabling/disabling certain passes)?<br>
>>>><br>
>>>><br>
>>>> Yes. AddressSanitizer and ThreadSanitizer disable load widening<br>
>>>> that would create a partially out-of-bounds or a racy access.<br>
>>>> See lib/Analysis/MemoryDependenceAnalysis.cpp (search for<br>
>>>> Attribute::SanitizeAddress and Attribute::SanitizeThread).<br>
>>>> This case with MemorySanitizer is slightly different because we are<br>
>>>> not fighting a false positive, but rather a debug-info-damaging optimization.<br>
>>>><br>
>>>> --kcc<br>
>>>><br>
>>>>><br>
>>>>> If not, does this need some deeper discussion about alternatives<br>
>>>>> (is it important that we be able to produce equivalent code<br>
>>>>> without the sanitizers enabled?)?<br>
>>>>><br>
>>>>><br>
>>>>> On Mon, Nov 18, 2013 at 7:02 AM, Evgeniy Stepanov<br>
>>>>> <<a href="mailto:eugenis@google.com" target="_blank">eugenis@google.com</a>><br>
>>>>> wrote:<br>
>>>>>><br>
>>>>>> Branch folding optimization often leads to confusing MSan reports<br>
>>>>>> due to lost debug info.<br>
>>>>>> For example,<br>
>>>>>> 1: if (x < 0)<br>
>>>>>> 2: if (y < 0)<br>
>>>>>> 3: do_something();<br>
>>>>>> is transformed into something like<br>
>>>>>> %0 = and i32 %y, %x<br>
>>>>>> %1 = icmp slt i32 %0, 0<br>
>>>>>> br i1 %1, label %if.then2, label %if.end3 where all 3<br>
>>>>>> instructions are associated with line 1.<br>
>>>>>><br>
>>>>>> This patch disables folding of conditional branches in functions<br>
>>>>>> with sanitize_memory attribute.<br>
>>>>>><br>
>>>>>> <a href="http://llvm-reviews.chandlerc.com/D2214" target="_blank">http://llvm-reviews.chandlerc.com/D2214</a><br>
>>>>>><br>
>>>>>> Files:<br>
>>>>>> lib/Transforms/Utils/SimplifyCFG.cpp<br>
>>>>>> test/Transforms/SimplifyCFG/branch-fold-msan.ll<br>
>>>>>><br>
>>>>>> Index: lib/Transforms/Utils/SimplifyCFG.cpp<br>
>>>>>> =================================================================<br>
>>>>>> ==<br>
>>>>>> --- lib/Transforms/Utils/SimplifyCFG.cpp<br>
>>>>>> +++ lib/Transforms/Utils/SimplifyCFG.cpp<br>
>>>>>> @@ -1967,6 +1967,13 @@<br>
>>>>>> bool llvm::FoldBranchToCommonDest(BranchInst *BI) {<br>
>>>>>> BasicBlock *BB = BI->getParent();<br>
>>>>>><br>
>>>>>> + // This optimization results in confusing MemorySanitizer reports.<br>
>>>>>> Use of<br>
>>>>>> + // uninitialized value in this branch instruction is reported<br>
>>>>>> + with<br>
>>>>>> the<br>
>>>>>> + // predecessor's debug location.<br>
>>>>>> + if (BB->getParent()->hasFnAttribute(Attribute::SanitizeMemory) &&<br>
>>>>>> + BI->isConditional())<br>
>>>>>> + return false;<br>
>>>>>> +<br>
>>>>>> Instruction *Cond = 0;<br>
>>>>>> if (BI->isConditional())<br>
>>>>>> Cond = dyn_cast<Instruction>(BI->getCondition());<br>
>>>>>> Index: test/Transforms/SimplifyCFG/branch-fold-msan.ll<br>
>>>>>> =================================================================<br>
>>>>>> ==<br>
>>>>>> --- test/Transforms/SimplifyCFG/branch-fold-msan.ll<br>
>>>>>> +++ test/Transforms/SimplifyCFG/branch-fold-msan.ll<br>
>>>>>> @@ -0,0 +1,29 @@<br>
>>>>>> +; RUN: opt < %s -simplifycfg -S | FileCheck %s<br>
>>>>>> +<br>
>>>>>> +declare void @callee()<br>
>>>>>> +<br>
>>>>>> +; Test that conditional branches are not folded with sanitize_memory.<u></u><u></u></p>
</div>
</div>
<p class="MsoNormal" style="line-height:115%">>>>>>> +define void @caller(i32 %x, i32 %y) sanitize_memory { ; CHECK:<br>
>>>>>> +define void @caller(i32 [[X:%.*]], i32 [[Y:%.*]]) ; CHECK: icmp<br>
>>>>>> +slt i32 {{.*}}[[X]] ; CHECK: icmp slt i32 {{.*}}[[Y]] ; CHECK:<br>
>>>>>> +ret void<u></u><u></u></p>
<div>
<div>
<p class="MsoNormal" style="line-height:115%">>>>>>> +<br>
>>>>>> +entry:<br>
>>>>>> + %cmp = icmp slt i32 %x, 0<br>
>>>>>> + br i1 %cmp, label %if.then, label %if.end3<br>
>>>>>> +<br>
>>>>>> +if.then: ; preds = %entry<br>
>>>>>> + %cmp1 = icmp slt i32 %y, 0<br>
>>>>>> + br i1 %cmp1, label %if.then2, label %if.end<br>
>>>>>> +<br>
>>>>>> +if.then2: ; preds = %if.then<br>
>>>>>> + call void @callee()<br>
>>>>>> + br label %if.end<br>
>>>>>> +<br>
>>>>>> +if.end: ; preds =<br>
>>>>>> %if.then2, %if.then<br>
>>>>>> + br label %if.end3<br>
>>>>>> +<br>
>>>>>> +if.end3: ; preds = %if.end,<br>
>>>>>> %entry<br>
>>>>>> + ret void<br>
>>>>>> +}<br>
>>>>>><br>
>>>>>> _______________________________________________<br>
>>>>>> llvm-commits mailing list<br>
>>>>>> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
>>>>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>>>>>><br>
>>>>><br>
>>>>><br>
>>>>> _______________________________________________<br>
>>>>> llvm-commits mailing list<br>
>>>>> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
>>>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>>>>><br>
>>>><br>
>>><br>
>><br>
><br>
><br>
> _______________________________________________<br>
> LLVM Developers mailing list<br>
> <a href="mailto:LLVMdev@cs.uiuc.edu" target="_blank">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">
http://llvm.cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
><br>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:LLVMdev@cs.uiuc.edu" target="_blank">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">
http://llvm.cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><u></u><u></u></p>
</div>
</div>
<p class="MsoNormal" style="margin-bottom:12.0pt;line-height:115%">---------------------------------------------------------------------<br>
Intel Israel (74) Limited<br>
<br>
This e-mail and any attachments may contain confidential material for<br>
the sole use of the intended recipient(s). Any review or distribution<br>
by others is strictly prohibited. If you are not the intended<br>
recipient, please contact the sender and delete all copies.<u></u><u></u></p>
</blockquote>
</div>
<p class="MsoNormal" style="line-height:115%"><u></u> <u></u></p>
</div>
</div>
</div></div></div><div><div class="h5">
<p>---------------------------------------------------------------------<br>
Intel Israel (74) Limited</p>
<p>This e-mail and any attachments may contain confidential material for<br>
the sole use of the intended recipient(s). Any review or distribution<br>
by others is strictly prohibited. If you are not the intended<br>
recipient, please contact the sender and delete all copies.</p></div></div></div>
</blockquote></div><br></div></div>