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

[Nick Lewycky]

[+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.

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;
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120206/3f57b044/attachment.html>