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

[David Blaikie]

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

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/91c24893/attachment.html>