[LLVMdev] Curiosity about transform changes under Sanitizers (Was: [PATCH] Disable branch folding with MemorySanitizer)
Hal Finkel
hfinkel at anl.gov
Tue Nov 19 09:07:09 PST 2013
----- Original Message -----
> From: "Evgeniy Stepanov" <eugeni.stepanov at gmail.com>
> To: "Kostya Serebryany" <kcc at google.com>
> Cc: "LLVM Developers Mailing List" <llvmdev at cs.uiuc.edu>
> Sent: Tuesday, November 19, 2013 10:55:11 AM
> Subject: Re: [LLVMdev] Curiosity about transform changes under Sanitizers (Was: [PATCH] Disable branch folding with
> MemorySanitizer)
>
> The root cause of those issues is the fact that sanitizers verify
> C++-level semantics with LLVM IR level instrumentation. For example,
> 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.
So you're saying that *if* the load had been unconditional in the original C++ program, then it would have been a data race? That does not sound right: the data race is not a property of the load itself, it is a property of the use of that data.
-Hal
>
>
> On Tue, Nov 19, 2013 at 8:38 PM, Kostya Serebryany <kcc at google.com>
> wrote:
> >
> >
> >
> > On Tue, Nov 19, 2013 at 8:25 PM, David Blaikie <dblaikie at gmail.com>
> > wrote:
> >>
> >> Just moving this branch of the thread out of the review because I
> >> don't
> >> want to derail the review thread...
> >>
> >> Kostya - why are these two cases not optimization bugs in general?
> >> (why do
> >> they only affect sanitizers?)
> >
> >
> > The recent case from mozilla
> > (https://code.google.com/p/thread-sanitizer/issues/detail?id=40#c2)
> > is a
> > legal
> > optimization -- it hoists a safe load (i.e. a load which is known
> > not to
> > fail) out of conditional branch.
> > It reduces the number of basic blocks and branches, and so I think
> > it's good
> > in general.
> > I can't imagine a case where this optimization will break a valid
> > program.
> > Which is the second one you are referring to?
> >
> > --kcc
> >
> >>
> >>
> >>
> >>
> >> On Mon, Nov 18, 2013 at 8:37 PM, Kostya Serebryany
> >> <kcc at google.com> wrote:
> >>>
> >>> And we've been just informed by the mozilla folks about yet
> >>> another case
> >>> of optimization being hostile to sanitizers:
> >>> hoisting a safe load out of conditional branch introduces a race
> >>> which
> >>> tsan happily reports.
> >>> https://code.google.com/p/thread-sanitizer/issues/detail?id=40#c2
> >>>
> >>> --kcc
> >>>
> >>>
> >>> On Tue, Nov 19, 2013 at 8:06 AM, Kostya Serebryany
> >>> <kcc at google.com>
> >>> wrote:
> >>>>
> >>>>
> >>>>
> >>>>
> >>>> On Tue, Nov 19, 2013 at 1:27 AM, David Blaikie
> >>>> <dblaikie at gmail.com>
> >>>> wrote:
> >>>>>
> >>>>> Do we have precedence for this kind of change (where sanitizers
> >>>>> affect
> >>>>> optimizations in arbitrary internal ways - not simply by
> >>>>> enabling/disabling
> >>>>> certain passes)?
> >>>>
> >>>>
> >>>> Yes. AddressSanitizer and ThreadSanitizer disable load widening
> >>>> that
> >>>> would create a partially out-of-bounds or a racy access.
> >>>> See lib/Analysis/MemoryDependenceAnalysis.cpp (search for
> >>>> Attribute::SanitizeAddress and Attribute::SanitizeThread).
> >>>> This case with MemorySanitizer is slightly different because we
> >>>> are not
> >>>> fighting a false positive, but rather a debug-info-damaging
> >>>> optimization.
> >>>>
> >>>> --kcc
> >>>>
> >>>>>
> >>>>> If not, does this need some deeper discussion about
> >>>>> alternatives (is it
> >>>>> important that we be able to produce equivalent code without
> >>>>> the sanitizers
> >>>>> enabled?)?
> >>>>>
> >>>>>
> >>>>> On Mon, Nov 18, 2013 at 7:02 AM, Evgeniy Stepanov
> >>>>> <eugenis at google.com>
> >>>>> wrote:
> >>>>>>
> >>>>>> Branch folding optimization often leads to confusing MSan
> >>>>>> reports due
> >>>>>> to lost debug info.
> >>>>>> For example,
> >>>>>> 1: if (x < 0)
> >>>>>> 2: if (y < 0)
> >>>>>> 3: do_something();
> >>>>>> is transformed into something like
> >>>>>> %0 = and i32 %y, %x
> >>>>>> %1 = icmp slt i32 %0, 0
> >>>>>> br i1 %1, label %if.then2, label %if.end3
> >>>>>> where all 3 instructions are associated with line 1.
> >>>>>>
> >>>>>> This patch disables folding of conditional branches in
> >>>>>> functions with
> >>>>>> sanitize_memory attribute.
> >>>>>>
> >>>>>> http://llvm-reviews.chandlerc.com/D2214
> >>>>>>
> >>>>>> Files:
> >>>>>> lib/Transforms/Utils/SimplifyCFG.cpp
> >>>>>> test/Transforms/SimplifyCFG/branch-fold-msan.ll
> >>>>>>
> >>>>>> Index: lib/Transforms/Utils/SimplifyCFG.cpp
> >>>>>> ===================================================================
> >>>>>> --- lib/Transforms/Utils/SimplifyCFG.cpp
> >>>>>> +++ lib/Transforms/Utils/SimplifyCFG.cpp
> >>>>>> @@ -1967,6 +1967,13 @@
> >>>>>> bool llvm::FoldBranchToCommonDest(BranchInst *BI) {
> >>>>>> BasicBlock *BB = BI->getParent();
> >>>>>>
> >>>>>> + // This optimization results in confusing MemorySanitizer
> >>>>>> reports.
> >>>>>> Use of
> >>>>>> + // uninitialized value in this branch instruction is
> >>>>>> reported with
> >>>>>> the
> >>>>>> + // predecessor's debug location.
> >>>>>> + if
> >>>>>> (BB->getParent()->hasFnAttribute(Attribute::SanitizeMemory)
> >>>>>> &&
> >>>>>> + BI->isConditional())
> >>>>>> + return false;
> >>>>>> +
> >>>>>> Instruction *Cond = 0;
> >>>>>> if (BI->isConditional())
> >>>>>> Cond = dyn_cast<Instruction>(BI->getCondition());
> >>>>>> Index: test/Transforms/SimplifyCFG/branch-fold-msan.ll
> >>>>>> ===================================================================
> >>>>>> --- test/Transforms/SimplifyCFG/branch-fold-msan.ll
> >>>>>> +++ test/Transforms/SimplifyCFG/branch-fold-msan.ll
> >>>>>> @@ -0,0 +1,29 @@
> >>>>>> +; RUN: opt < %s -simplifycfg -S | FileCheck %s
> >>>>>> +
> >>>>>> +declare void @callee()
> >>>>>> +
> >>>>>> +; Test that conditional branches are not folded with
> >>>>>> sanitize_memory.
> >>>>>> +define void @caller(i32 %x, i32 %y) sanitize_memory {
> >>>>>> +; CHECK: define void @caller(i32 [[X:%.*]], i32 [[Y:%.*]])
> >>>>>> +; CHECK: icmp slt i32 {{.*}}[[X]]
> >>>>>> +; CHECK: icmp slt i32 {{.*}}[[Y]]
> >>>>>> +; CHECK: ret void
> >>>>>> +
> >>>>>> +entry:
> >>>>>> + %cmp = icmp slt i32 %x, 0
> >>>>>> + br i1 %cmp, label %if.then, label %if.end3
> >>>>>> +
> >>>>>> +if.then: ; preds =
> >>>>>> %entry
> >>>>>> + %cmp1 = icmp slt i32 %y, 0
> >>>>>> + br i1 %cmp1, label %if.then2, label %if.end
> >>>>>> +
> >>>>>> +if.then2: ; preds =
> >>>>>> %if.then
> >>>>>> + call void @callee()
> >>>>>> + br label %if.end
> >>>>>> +
> >>>>>> +if.end: ; preds =
> >>>>>> %if.then2, %if.then
> >>>>>> + br label %if.end3
> >>>>>> +
> >>>>>> +if.end3: ; preds =
> >>>>>> %if.end,
> >>>>>> %entry
> >>>>>> + ret void
> >>>>>> +}
> >>>>>>
> >>>>>> _______________________________________________
> >>>>>> llvm-commits mailing list
> >>>>>> llvm-commits at cs.uiuc.edu
> >>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >>>>>>
> >>>>>
> >>>>>
> >>>>> _______________________________________________
> >>>>> llvm-commits mailing list
> >>>>> llvm-commits at cs.uiuc.edu
> >>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >>>>>
> >>>>
> >>>
> >>
> >
> >
> > _______________________________________________
> > LLVM Developers mailing list
> > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
> >
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
--
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory
More information about the llvm-dev
mailing list