<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Nov 19, 2013 at 10:08 AM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">----- Original Message -----<br>
> From: "Kostya Serebryany" <<a href="mailto:kcc@google.com">kcc@google.com</a>><br>
> To: "Michael M Kuperstein" <<a href="mailto:michael.m.kuperstein@intel.com">michael.m.kuperstein@intel.com</a>><br>
> Cc: "LLVM Developers Mailing List" <<a href="mailto:llvmdev@cs.uiuc.edu">llvmdev@cs.uiuc.edu</a>><br>
> Sent: Tuesday, November 19, 2013 11:45:39 AM<br>
> Subject: Re: [LLVMdev] Curiosity about transform changes under Sanitizers (Was: [PATCH] Disable branch folding with<br>
> MemorySanitizer)<br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
</div><div class="im">> On Tue, Nov 19, 2013 at 9:05 PM, Kuperstein, Michael M <<br>
> <a href="mailto:michael.m.kuperstein@intel.com">michael.m.kuperstein@intel.com</a> > wrote:<br>
><br>
><br>
> 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<br>
> model is to allow a wide array of compiler transformations -<br>
> including speculative loads - for non-atomic variables.<br>
> I believe what's most likely happening (without looking at the<br>
> Mozilla source) is that the original program contains a C++ data<br>
> race, and the transformation exposes it to TSan.<br>
><br>
><br>
><br>
> The original program is race-free.<br>
> I've posted a minimized reproducer that actually triggers a tsan<br>
> false positive at O1 here:<br>
> <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><br>
><br>
><br>
<br>
</div>Here's my problem:<br>
<br>
int g;<br>
int foo(int cond) {<br>
if (cond)<br>
return g;<br>
return 0;<br>
}<br>
<br>
is being transformed into this (at the IR level):<br>
<br>
int g;<br>
int foo(int cond) {<br>
return cond ? g : 0;<br></blockquote><div><br></div><div>This doesn't have the same semantics as the IR transformed 'select', though, does it?<br><br>In IR 'g' is unconditionally loaded before the select. In C++, 'g' is completely unevaluated if 'cond' is false. </div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
}<br>
<br>
but I could have just as easily made that transformation at the C++ level as well, and I don't see that as introducing a data race here (in practice). When you have a load that is used only conditionally, TSAN should only record it when the condition is true. IMHO, to do otherwise, is a bug in TSAN. Furthermore, if you try and do this by disabling all optimizations that transform conditionally-executed blocks into selects, you'll have a lot more work to do (because you'll also need to handle every place in the backend that does if conversion).<br>
<span class="HOEnZb"><font color="#888888"><br>
-Hal<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
><br>
><br>
><br>
> -----Original Message-----<br>
> From: <a href="mailto:llvmdev-bounces@cs.uiuc.edu">llvmdev-bounces@cs.uiuc.edu</a> [mailto:<br>
> <a href="mailto:llvmdev-bounces@cs.uiuc.edu">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<br>
> Sanitizers (Was: [PATCH] Disable branch folding with<br>
> 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<br>
> 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">kcc@google.com</a> ><br>
> wrote:<br>
> ><br>
> ><br>
> ><br>
> > On Tue, Nov 19, 2013 at 8:25 PM, David Blaikie < <a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a><br>
> > > 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><br>
> > ) 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<br>
> > 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 <<br>
> >> <a href="mailto:kcc@google.com">kcc@google.com</a> > wrote:<br>
> >>><br>
> >>> And we've been just informed by the mozilla folks about yet<br>
> >>> 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 <<br>
> >>> <a href="mailto:kcc@google.com">kcc@google.com</a> ><br>
> >>> wrote:<br>
> >>>><br>
> >>>><br>
> >>>><br>
> >>>><br>
> >>>> On Tue, Nov 19, 2013 at 1:27 AM, David Blaikie <<br>
> >>>> <a href="mailto:dblaikie@gmail.com">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<br>
> >>>> are<br>
> >>>> not fighting a false positive, but rather a debug-info-damaging<br>
> >>>> optimization.<br>
> >>>><br>
> >>>> --kcc<br>
> >>>><br>
> >>>>><br>
> >>>>> If not, does this need some deeper discussion about<br>
> >>>>> 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">eugenis@google.com</a> ><br>
> >>>>> wrote:<br>
> >>>>>><br>
> >>>>>> Branch folding optimization often leads to confusing MSan<br>
> >>>>>> 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<br>
> >>>>>> 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<br>
> >>>>>> reports.<br>
> >>>>>> Use of<br>
> >>>>>> + // uninitialized value in this branch instruction is<br>
> >>>>>> reported<br>
> >>>>>> + with<br>
> >>>>>> the<br>
> >>>>>> + // predecessor's debug location.<br>
> >>>>>> + if<br>
> >>>>>> (BB->getParent()->hasFnAttribute(Attribute::SanitizeMemory)<br>
> >>>>>> &&<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<br>
> >>>>>> sanitize_memory.<br>
> >>>>>> +define void @caller(i32 %x, i32 %y) sanitize_memory { ;<br>
> >>>>>> CHECK:<br>
> >>>>>> +define void @caller(i32 [[X:%.*]], i32 [[Y:%.*]]) ; CHECK:<br>
> >>>>>> icmp<br>
> >>>>>> +slt i32 {{.*}}[[X]] ; CHECK: icmp slt i32 {{.*}}[[Y]] ;<br>
> >>>>>> CHECK:<br>
> >>>>>> +ret void<br>
><br>
><br>
> >>>>>> +<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">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">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">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">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>
> 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.<br>
><br>
><br>
><br>
> _______________________________________________<br>
> LLVM Developers mailing list<br>
> <a href="mailto:LLVMdev@cs.uiuc.edu">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>
</div></div><div class="im HOEnZb">--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</div><div class="HOEnZb"><div class="h5">_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:LLVMdev@cs.uiuc.edu">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>
</div></div></blockquote></div><br></div></div>