[llvm-commits] fix conflict between AddressSanitizer and load widening (GVN) (issue 5630068)

[Kostya Serebryany]

On Mon, Feb 6, 2012 at 2:37 PM, Nick Lewycky <nlewycky at google.com> wrote:

> [+llvm-commits, silly codereview.]
>
> On 6 February 2012 14:34, <konstantin.s.serebryany at gmail.com> wrote:
>
>> Reviewers: nlewycky1,
>>
>> Message:
>>
>> On 2012/02/06 22:20:28, nlewycky1 wrote:
>>
>>> Go ahead and commit once you've resolved the comments Duncan and I
>>>
>> made.
>>
>>
>> http://codereview.appspot.com/**5630068/diff/2001/lib/**Analysis/**
>> MemoryDependenceAnalysis.cpp<http://codereview.appspot.com/5630068/diff/2001/lib/Analysis/MemoryDependenceAnalysis.cpp>
>>
>>> File lib/Analysis/**MemoryDependenceAnalysis.cpp (right):
>>>
>>
>>
>> http://codereview.appspot.com/**5630068/diff/2001/lib/**Analysis/**
>> MemoryDependenceAnalysis.cpp#**newcode327<http://codereview.appspot.com/5630068/diff/2001/lib/Analysis/MemoryDependenceAnalysis.cpp#newcode327>
>>
>>> lib/Analysis/**MemoryDependenceAnalysis.cpp:**327:
>>> LI->getParent()->getParent()->**hasFnAttr(Attribute::**AddressSafety)) {
>>> LI->getParent()->getParent()->**hasFnAttr(Attribute::**AddressSafety)
>>> is a
>>> loop-invariant, please hoist it into "bool MayWidenPastEnd = ...".
>>>
>>
>> Hm?
>> This is a loop invariant, but it is not computed on every iteration.
>> Sometimes it is never computed.
>> If I move it outside of the loop it is likely to be computed more often.
>
>
> Oops, you're right. Short-circuiting and all. :)
>
> Please commit.
>


r149925, thanks!


>
> Nick
>
>
>>
>>
>>
>>
>>
>> http://codereview.appspot.com/**5630068/diff/2001/lib/**Analysis/**
>> MemoryDependenceAnalysis.cpp#**newcode329<http://codereview.appspot.com/5630068/diff/2001/lib/Analysis/MemoryDependenceAnalysis.cpp#newcode329>
>>
>>> lib/Analysis/**MemoryDependenceAnalysis.cpp:**329: // While this is safe
>>>
>> in a
>>
>>> regular build, Address Safety analysys tools
>>> "analysis"
>>>
>> ok
>>
>>
>>
>>
>>
>> Please review this at http://codereview.appspot.com/**5630068/<http://codereview.appspot.com/5630068/>
>>
>> Affected files:
>>  M     lib/Analysis/**MemoryDependenceAnalysis.cpp
>>  A     test/Instrumentation/**AddressSanitizer/asan-vs-gvn.**ll
>>
>>
>> Index: test/Instrumentation/**AddressSanitizer/asan-vs-gvn.**ll
>> ==============================**==============================**=======
>> --- test/Instrumentation/**AddressSanitizer/asan-vs-gvn.**ll
>>  (revision 0)
>> +++ test/Instrumentation/**AddressSanitizer/asan-vs-gvn.**ll
>>  (revision 0)
>> @@ -0,0 +1,43 @@
>> +; RUN: opt < %s -basicaa -gvn -asan -S | FileCheck %s
>> +; ASAN conflicts with load widening iff the widened load accesses data
>> out of bounds
>> +; (while the original unwidened loads do not).
>> +; http://code.google.com/p/**address-sanitizer/issues/**detail?id=20#c1<http://code.google.com/p/address-sanitizer/issues/detail?id=20#c1>
>> +
>> +
>> +; 32-bit little endian target.
>> +target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-**
>> i16:16:16-i32:32:32-i64:32:64-**f32:32:32-f64:32:64-v64:64:64-**
>> v128:128:128-a0:0:64-f80:128:**128-n8:16:32"
>> +
>> +%struct_of_7_bytes_4_aligned = type { i32, i8, i8, i8}
>> +
>> + at f = global %struct_of_7_bytes_4_aligned zeroinitializer, align 4
>> +
>> +; Accessing bytes 4 and 6, not ok to widen to i32 if address_safety is
>> set.
>> +
>> +define i32 @test_widening_bad(i8* %P) nounwind ssp noredzone
>> address_safety {
>> +entry:
>> +  %tmp = load i8* getelementptr inbounds (%struct_of_7_bytes_4_aligned*
>> @f, i64 0, i32 1), align 4
>> +  %conv = zext i8 %tmp to i32
>> +  %tmp1 = load i8* getelementptr inbounds (%struct_of_7_bytes_4_aligned*
>> @f, i64 0, i32 3), align 1
>> +  %conv2 = zext i8 %tmp1 to i32
>> +  %add = add nsw i32 %conv, %conv2
>> +  ret i32 %add
>> +; CHECK: @test_widening_bad
>> +; CHECK: __asan_report_load1
>> +; CHECK: __asan_report_load1
>> +; CHECK-ret i32
>> +}
>> +
>> +;; Accessing byets 4 and 5. Ok to widen to i16.
>> +
>> +define i32 @test_widening_ok(i8* %P) nounwind ssp noredzone
>> address_safety {
>> +entry:
>> +  %tmp = load i8* getelementptr inbounds (%struct_of_7_bytes_4_aligned*
>> @f, i64 0, i32 1), align 4
>> +  %conv = zext i8 %tmp to i32
>> +  %tmp1 = load i8* getelementptr inbounds (%struct_of_7_bytes_4_aligned*
>> @f, i64 0, i32 2), align 1
>> +  %conv2 = zext i8 %tmp1 to i32
>> +  %add = add nsw i32 %conv, %conv2
>> +  ret i32 %add
>> +; CHECK: @test_widening_ok
>> +; CHECK: __asan_report_load1
>> +; CHECK-ret i32
>> +}
>> Index: lib/Analysis/**MemoryDependenceAnalysis.cpp
>> ==============================**==============================**=======
>> --- lib/Analysis/**MemoryDependenceAnalysis.cpp   (revision 149872)
>> +++ lib/Analysis/**MemoryDependenceAnalysis.cpp   (working copy)
>> @@ -323,6 +323,14 @@
>>         !TD.fitsInLegalInteger(**NewLoadByteSize*8))
>>       return 0;
>>
>> +    if (LIOffs+NewLoadByteSize > MemLocEnd &&
>> +        LI->getParent()->getParent()->**hasFnAttr(Attribute::**AddressSafety))
>> {
>> +      // We will be reading past the location accessed by the original
>> program.
>>
>> +      // While this is safe in a regular build, Address Safety analysys
>> tools
>> +      // may start reporting false warnings. So, don't do widening.
>> +      return 0;
>> +    }
>> +
>>     // If a load of this width would include all of MemLoc, then we
>> succeed.
>>     if (LIOffs+NewLoadByteSize >= MemLocEnd)
>>       return NewLoadByteSize;
>>
>>
>>
>
> _______________________________________________
> 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-commits/attachments/20120206/54c8af68/attachment.html>