fix conflict between AddressSanitizer and load widening (GVN)

[nlewycky1, 2012-02-06 22:20:28]

Go ahead and commit once you've resolved the comments Duncan and I made.

http://codereview.appspot.com/5630068/diff/2001/lib/Analysis/MemoryDependence...
File lib/Analysis/MemoryDependenceAnalysis.cpp (right):

http://codereview.appspot.com/5630068/diff/2001/lib/Analysis/MemoryDependence...
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 = ...".

http://codereview.appspot.com/5630068/diff/2001/lib/Analysis/MemoryDependence...
lib/Analysis/MemoryDependenceAnalysis.cpp:329: // While this is safe in a
regular build, Address Safety analysys tools
"analysis"

[kcc, 2012-02-06 22:34:19]

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/MemoryDependence...
> File lib/Analysis/MemoryDependenceAnalysis.cpp (right):
> 
>
http://codereview.appspot.com/5630068/diff/2001/lib/Analysis/MemoryDependence...
> 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. 


> 
>
http://codereview.appspot.com/5630068/diff/2001/lib/Analysis/MemoryDependence...
> lib/Analysis/MemoryDependenceAnalysis.cpp:329: // While this is safe in a
> regular build, Address Safety analysys tools
> "analysis"
ok

[nlewycky1, 2012-02-06 22:37:39]

[+llvm-commits, silly codereview.]

On 6 February 2012 14:34, <konstantin.s.serebryany@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.

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://...
> +
> +
> +; 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}
> +
> +@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;
>
>
>

[kcc1, 2012-02-06 22:53:45]

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

> [+llvm-commits, silly codereview.]
>
> On 6 February 2012 14:34, <konstantin.s.serebryany@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://...
>> +
>> +
>> +; 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}
>> +
>> +@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@cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>