[LLVMdev] Curiosity about transform changes under Sanitizers (Was: [PATCH] Disable branch folding with MemorySanitizer)

[David Blaikie]

On Tue, Nov 19, 2013 at 8:38 AM, 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.
>

Hmm, OK. I can see how the optimization is reasonable. I'll have to read
up/chat more about how TSan works to understand how it gets caught up by
this.


> 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?
>

The load widening issue you mentioned in another reply on the same thread.


>
> --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
>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20131119/9897e6e7/attachment.html>