<div class="gmail_quote">[+llvm-commits, silly codereview.]</div><div class="gmail_quote"><br></div><div class="gmail_quote">On 6 February 2012 14:34, <span dir="ltr"><<a href="mailto:konstantin.s.serebryany@gmail.com">konstantin.s.serebryany@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Reviewers: nlewycky1,<br>
<br>
Message:<div class="im"><br>
On 2012/02/06 22:20:28, nlewycky1 wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Go ahead and commit once you've resolved the comments Duncan and I<br>
</blockquote>
made.<br>
<br>
<br>
<a href="http://codereview.appspot.com/5630068/diff/2001/lib/Analysis/MemoryDependenceAnalysis.cpp" target="_blank">http://codereview.appspot.com/<u></u>5630068/diff/2001/lib/<u></u>Analysis/<u></u>MemoryDependenceAnalysis.cpp</a><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
File lib/Analysis/<u></u>MemoryDependenceAnalysis.cpp (right):<br>
</blockquote>
<br>
<br>
<a href="http://codereview.appspot.com/5630068/diff/2001/lib/Analysis/MemoryDependenceAnalysis.cpp#newcode327" target="_blank">http://codereview.appspot.com/<u></u>5630068/diff/2001/lib/<u></u>Analysis/<u></u>MemoryDependenceAnalysis.cpp#<u></u>newcode327</a><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
lib/Analysis/<u></u>MemoryDependenceAnalysis.cpp:<u></u>327:<br>
LI->getParent()->getParent()-><u></u>hasFnAttr(Attribute::<u></u>AddressSafety)) {<br>
LI->getParent()->getParent()-><u></u>hasFnAttr(Attribute::<u></u>AddressSafety) is a<br>
loop-invariant, please hoist it into "bool MayWidenPastEnd = ...".<br>
</blockquote>
<br></div>
Hm?<br>
This is a loop invariant, but it is not computed on every iteration.<br>
Sometimes it is never computed.<br>
If I move it outside of the loop it is likely to be computed more often.</blockquote><div><br></div><div>Oops, you're right. Short-circuiting and all. :)</div><div><br></div><div>Please commit.</div><div><br></div><div>
Nick</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im"><br>
<br>
<br>
<br>
<br>
<br>
<a href="http://codereview.appspot.com/5630068/diff/2001/lib/Analysis/MemoryDependenceAnalysis.cpp#newcode329" target="_blank">http://codereview.appspot.com/<u></u>5630068/diff/2001/lib/<u></u>Analysis/<u></u>MemoryDependenceAnalysis.cpp#<u></u>newcode329</a><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
lib/Analysis/<u></u>MemoryDependenceAnalysis.cpp:<u></u>329: // While this is safe<br>
</blockquote>
in a<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
regular build, Address Safety analysys tools<br>
"analysis"<br>
</blockquote></div>
ok<br>
<br>
<br>
<br>
<br>
<br>
Please review this at <a href="http://codereview.appspot.com/5630068/" target="_blank">http://codereview.appspot.com/<u></u>5630068/</a><br>
<br>
Affected files:<br>
M lib/Analysis/<u></u>MemoryDependenceAnalysis.cpp<br>
A test/Instrumentation/<u></u>AddressSanitizer/asan-vs-gvn.<u></u>ll<br>
<br>
<br>
Index: test/Instrumentation/<u></u>AddressSanitizer/asan-vs-gvn.<u></u>ll<br>
==============================<u></u>==============================<u></u>=======<br>
--- test/Instrumentation/<u></u>AddressSanitizer/asan-vs-gvn.<u></u>ll (revision 0)<br>
+++ test/Instrumentation/<u></u>AddressSanitizer/asan-vs-gvn.<u></u>ll (revision 0)<br>
@@ -0,0 +1,43 @@<br>
+; RUN: opt < %s -basicaa -gvn -asan -S | FileCheck %s<br>
+; ASAN conflicts with load widening iff the widened load accesses data out of bounds<br>
+; (while the original unwidened loads do not).<br>
+; <a href="http://code.google.com/p/address-sanitizer/issues/detail?id=20#c1" target="_blank">http://code.google.com/p/<u></u>address-sanitizer/issues/<u></u>detail?id=20#c1</a><br>
+<br>
+<br>
+; 32-bit little endian target.<br>
+target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-<u></u>i16:16:16-i32:32:32-i64:32:64-<u></u>f32:32:32-f64:32:64-v64:64:64-<u></u>v128:128:128-a0:0:64-f80:128:<u></u>128-n8:16:32"<br>
+<br>
+%struct_of_7_bytes_4_aligned = type { i32, i8, i8, i8}<br>
+<br>
+@f = global %struct_of_7_bytes_4_aligned zeroinitializer, align 4<br>
+<br>
+; Accessing bytes 4 and 6, not ok to widen to i32 if address_safety is set.<br>
+<br>
+define i32 @test_widening_bad(i8* %P) nounwind ssp noredzone address_safety {<br>
+entry:<br>
+ %tmp = load i8* getelementptr inbounds (%struct_of_7_bytes_4_aligned* @f, i64 0, i32 1), align 4<br>
+ %conv = zext i8 %tmp to i32<br>
+ %tmp1 = load i8* getelementptr inbounds (%struct_of_7_bytes_4_aligned* @f, i64 0, i32 3), align 1<br>
+ %conv2 = zext i8 %tmp1 to i32<br>
+ %add = add nsw i32 %conv, %conv2<br>
+ ret i32 %add<br>
+; CHECK: @test_widening_bad<br>
+; CHECK: __asan_report_load1<br>
+; CHECK: __asan_report_load1<br>
+; CHECK-ret i32<br>
+}<br>
+<br>
+;; Accessing byets 4 and 5. Ok to widen to i16.<br>
+<br>
+define i32 @test_widening_ok(i8* %P) nounwind ssp noredzone address_safety {<br>
+entry:<br>
+ %tmp = load i8* getelementptr inbounds (%struct_of_7_bytes_4_aligned* @f, i64 0, i32 1), align 4<br>
+ %conv = zext i8 %tmp to i32<br>
+ %tmp1 = load i8* getelementptr inbounds (%struct_of_7_bytes_4_aligned* @f, i64 0, i32 2), align 1<br>
+ %conv2 = zext i8 %tmp1 to i32<br>
+ %add = add nsw i32 %conv, %conv2<br>
+ ret i32 %add<br>
+; CHECK: @test_widening_ok<br>
+; CHECK: __asan_report_load1<br>
+; CHECK-ret i32<br>
+}<br>
Index: lib/Analysis/<u></u>MemoryDependenceAnalysis.cpp<br>
==============================<u></u>==============================<u></u>=======<br>
--- lib/Analysis/<u></u>MemoryDependenceAnalysis.cpp (revision 149872)<br>
+++ lib/Analysis/<u></u>MemoryDependenceAnalysis.cpp (working copy)<br>
@@ -323,6 +323,14 @@<br>
!TD.fitsInLegalInteger(<u></u>NewLoadByteSize*8))<br>
return 0;<br>
<br>
+ if (LIOffs+NewLoadByteSize > MemLocEnd &&<br>
+ LI->getParent()->getParent()-><u></u>hasFnAttr(Attribute::<u></u>AddressSafety)) {<br>
+ // We will be reading past the location accessed by the original program.<div class="im"><br>
+ // While this is safe in a regular build, Address Safety analysys tools<br></div>
+ // may start reporting false warnings. So, don't do widening.<br>
+ return 0;<br>
+ }<br>
+<br>
// If a load of this width would include all of MemLoc, then we succeed.<br>
if (LIOffs+NewLoadByteSize >= MemLocEnd)<br>
return NewLoadByteSize;<br>
<br>
<br>
</blockquote></div><br>