[LLVMdev] Curiosity about transform changes under Sanitizers (Was: [PATCH] Disable branch folding with MemorySanitizer)
Kostya Serebryany
kcc at google.com
Tue Nov 19 09:52:01 PST 2013
On Tue, Nov 19, 2013 at 8:48 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> 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.
>
That was https://code.google.com/p/address-sanitizer/issues/detail?id=20
(asan)
If you have e.g. "char a[3]" aligned by 4 and you load a[0] and a[2], it
is tempting to load 4 bytes from (int*)a instead.
This is legal from LLVM point of view, but it makes asan think that we
overflow 'a' by one byte to the right.
In tsan it's even worse.
Consider we have "char a[4]" aligned by 4, and we load a[1] and a[3].
Then the optimizer replaces it with one load from (int*)a.
But another thread is reading a[2]. There was no race on C++ level, but we
now have a race on LLVM level.
Note that valgrind suffers from all the same issues which makes it nearly
unusable at large scale with anything other than -O0.
--kcc
>
>
>>
>> --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/87ce3fa4/attachment.html>
More information about the llvm-dev
mailing list