<div>Chandler, </div>I've committed the changes that merge asan crash callbacks (under a flag). <div><div><br></div><div>% cat ~/tmp/store2.cc </div><div>void foo(int *a, int *b) {</div><div> *a = 0;</div><div> *b = 1;</div>
<div>}</div></div><div>% clang -O2 -faddress-sanitizer -S -o - -mllvm -asan-merge-callbacks=1 ~/tmp/store2.cc -emit-llvm 2>&1</div><div>...</div><div><div>crash_bb-w-4: ; preds = %16, %21</div>
<div> %14 = phi i64 [ %7, %21 ], [ %0, %16 ]</div><div> %15 = phi i64 [ 846930886, %21 ], [ 1804289383, %16 ]</div><div> call void @__asan_report_store4(i64 %14, i64 %15) noreturn nounwind</div><div> unreachable</div>
</div><div>...</div><div><br></div><div>This produces 5% bigger code: </div><div><div><div>1.08<span class="Apple-tab-span" style="white-space:pre"> </span>400.perlbench</div><div>1.08<span class="Apple-tab-span" style="white-space:pre"> </span>401.bzip2</div>
<div>1.07<span class="Apple-tab-span" style="white-space:pre"> </span>403.gcc</div><div>1.00<span class="Apple-tab-span" style="white-space:pre"> </span>429.mcf</div><div>1.01<span class="Apple-tab-span" style="white-space:pre"> </span>445.gobmk</div>
<div>1.08<span class="Apple-tab-span" style="white-space:pre"> </span>456.hmmer</div><div>1.03<span class="Apple-tab-span" style="white-space:pre"> </span>458.sjeng</div><div>1.02<span class="Apple-tab-span" style="white-space:pre"> </span>462.libquantum</div>
<div>1.08<span class="Apple-tab-span" style="white-space:pre"> </span>464.h264ref</div><div>1.05<span class="Apple-tab-span" style="white-space:pre"> </span>473.astar</div><div>1.07<span class="Apple-tab-span" style="white-space:pre"> </span>483.xalancbmk</div>
<div>1.04<span class="Apple-tab-span" style="white-space:pre"> </span>433.milc</div><div>1.08<span class="Apple-tab-span" style="white-space:pre"> </span>444.namd</div><div>1.07<span class="Apple-tab-span" style="white-space:pre"> </span>447.dealII</div>
<div>1.08<span class="Apple-tab-span" style="white-space:pre"> </span>450.soplex</div><div>1.07<span class="Apple-tab-span" style="white-space:pre"> </span>453.povray</div><div>1.00<span class="Apple-tab-span" style="white-space:pre"> </span>470.lbm</div>
<div>1.06<span class="Apple-tab-span" style="white-space:pre"> </span>482.sphinx3</div></div></div><div><br></div><div>This also produces 1%-4% slower code (I've run only 3 benchmarks)</div><div><div> 400.perlbench, 1061.00, 1072.00, 1.01</div>
<div> 401.bzip2, 910.00, 923.00, 1.01</div><div> 403.gcc, 623.00, 645.00, 1.04</div></div><div><br></div><div>Besides, the reproducer from <a href="http://llvm.org/bugs/show_bug.cgi?id=13225">http://llvm.org/bugs/show_bug.cgi?id=13225</a> builds 3x slower!</div>
<div>I guess that these extra PHI nodes produce much more stress for CG than the extra BBs. </div><div><div>% time clang -O2 -c -w -mllvm -asan-merge-callbacks=0 -mllvm -asan-max-ins-per-bb=99999999 -faddress-sanitizer big-global.cc </div>
<div>TIME: real: 8.970; user: 8.690; system: 0.100</div><div>% time clang -O2 -c -w -mllvm -asan-merge-callbacks=1 -mllvm -asan-max-ins-per-bb=99999999 -faddress-sanitizer big-global.cc </div><div>TIME: real: 25.629; user: 25.350; system: 0.080</div>
<div>% </div></div><div><br></div><div><br></div><div>The current implementation passes a random constant instead of the PC to the callback. </div><div>Getting the real PC might end up even more expensive. </div><div>We can not get the PC on x86_32 at all, so we will need non-trivial changes in the run-time to accept the module offset. </div>
<div><br></div><div>Finally, this adds 150 lines to the asan compiler module. </div><div><br></div><div>So, I am tempted to rollback all these changes. </div><div>WDYT? </div><div><br></div><div>There is another problem: we are avoiding the callback merging by sheer luck. </div>
<div>A tiny change in splitBlockAndInsertIfThen will cause it to happen and will break asan error reports:</div><div><br></div><div><div>--- lib/Transforms/Instrumentation/AddressSanitizer.cpp (revision 160361)</div><div>
+++ lib/Transforms/Instrumentation/AddressSanitizer.cpp (working copy)</div><div>@@ -276,7 +276,7 @@</div><div> BranchInst *CheckTerm = 0;</div><div> if (!ThenBlock) {</div><div> LLVMContext &C = Head->getParent()->getParent()->getContext();</div>
<div>- ThenBlock = BasicBlock::Create(C, "", Head->getParent());</div><div>+ ThenBlock = BasicBlock::Create(C, "", Head->getParent(), Tail);</div><div> CheckTerm = BranchInst::Create(Tail, ThenBlock);</div>
<div> }</div><div> BranchInst *HeadNewTerm =</div></div><div><br></div><div>Any suggestions? </div><div><br></div><div>--kcc <br><br><div class="gmail_quote">On Mon, Jul 16, 2012 at 5:34 PM, Kostya Serebryany <span dir="ltr"><<a href="mailto:kcc@google.com" target="_blank">kcc@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This change has been reverted by r<span style="color:rgb(34,34,34);font-size:13px;font-family:arial,sans-serif">160256 and asan bots are green, but the problem still exists. </span><div>
<br>
</div><div>Current situation: given N memory access the generated code looks like:</div><div>if (*shadow1) __asan_report_read4(addr1); // no-return</div><div>if (*shadow2) __asan_report_read4(addr2); // no-return</div><div>
if (*shadow3) __asan_report_read4(addr3); // no-return</div><div><br></div><div>This causes compile-time problems in CG and produces significant amount of code (which is not hot, but hurts i-cache). </div><div>First Chandler's suggestion was to replace this code with</div>
<div><br></div><div>if (*shadow1) { arg = addr1; goto report_read4; }</div><div>if (*shadow2) { arg = addr2; goto report_read4; }</div><div>if (*shadow3) { arg = addr3; goto report_read4; }</div><div>return;</div><div>report_read4:</div>
<div> __asan_report_read4(arg);</div><div><br></div><div>The problem here is that now there is a single __asan_report_read4 for multiple memory accesses and asan run-time can not correctly report the top stack frame</div>
<div>(it gets the PC using __builtin_return_address and then passes this PC to the symbolizer). </div><div><br></div><div><br></div><div>Second Chandler's idea is to pass the PC as the second parameter to __asan_report_read4: </div>
<div><div>if (*shadow1) { arg = addr1; pc = current_pc(); goto report_read4; }</div><div>if (*shadow2) { arg = addr2; pc = current_pc(); goto report_read4; }</div><div>if (*shadow3) { arg = addr3; pc = current_pc(); goto report_read4; }</div>
<div>return;</div><div>report_read4:</div><div> __asan_report_read4(arg, pc);</div></div><div><br></div><div>The problems here: </div><div> 1. There is not LLVM intrinsic to get current_pc(). This is probably solvable. </div>
<div> 2. This adds an instruction on the main path. Hard to say how much it will cost, but not zero. I'll have to experiment. </div><div> 3. On x86_32 this is even worse, because one can not get the PC directly from the register and this becomes prohibitively slow. We can probably use a module offset instead of real PC... So, it will have to be reflected in the appropriate LLVM intrinsic. </div>
<div><br></div><div>So, I'll implement the merging of __asan_report_* functions under a flag and continue experimenting... </div><div><br></div><div>--kcc </div><div class="HOEnZb"><div class="h5"><div><br></div><div>
<div class="gmail_quote">On Mon, Jul 16, 2012 at 1:41 PM, Kostya Serebryany <span dir="ltr"><<a href="mailto:kcc@google.com" target="_blank">kcc@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br><br><div class="gmail_quote"><div>On Mon, Jul 16, 2012 at 1:39 PM, Kostya Serebryany <span dir="ltr"><<a href="mailto:kcc@google.com" target="_blank">kcc@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
FYI <div>This broke some of the asan tests because multiple calls to __asan_report_* got merged into one. </div><div>Every call to __asan_report_* has unique debug info and merging them leads to incorrect error messages. </div>
<div>Chandler and I are investigating. </div><div><br></div><div>The simple way to reproduce: </div><div>% cd llvm/projects/compiler-rt/lib/asan</div><div>% clang -g -O1 -faddress-sanitizer output_tests/global-overflow.cc && ./a.out 2>&1 | ./scripts/asan_symbolize.py /</div>
<div>Should print: </div><div> #0 0x403420 in main global-overflow.cc:9</div><div>Instead it prints: </div><div> #0 0x403420 in main global-overflow.cc:14</div></blockquote><div><br></div></div><div>Another test case: </div>
<div>clang -O1 -S -o - -faddress-sanitizer output_tests/large_func_test.cc | grep __asan_report</div><div><br></div><div>Before the patch, it would print 21 lines with __asan_report_load4. </div><div>Now it prints only 3 (hm, why not one!?). </div>
<div><br></div><div>--kcc </div><div><div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><br></div><div><br></div><div>Please avoid making other changes to AddressSanitizer.cpp before we resolve this issue. </div>
<div><br></div><div>--kcc <div><div><br><br><div class="gmail_quote">On Mon, Jul 16, 2012 at 12:58 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: chandlerc<br>
Date: Mon Jul 16 03:58:53 2012<br>
New Revision: 160254<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=160254&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=160254&view=rev</a><br>
Log:<br>
Teach AddressSanitizer to create basic blocks in a more natural order.<br>
This is particularly useful to the backend code generators which try to<br>
process things in the incoming function order.<br>
<br>
Also, cleanup some uses of IRBuilder to be a bit simpler and more clear.<br>
<br>
Modified:<br>
llvm/trunk/lib/Transforms/Instrumentation/AddressSanitizer.cpp<br>
llvm/trunk/test/Instrumentation/AddressSanitizer/basic.ll<br>
<br>
Modified: llvm/trunk/lib/Transforms/Instrumentation/AddressSanitizer.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Instrumentation/AddressSanitizer.cpp?rev=160254&r1=160253&r2=160254&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Instrumentation/AddressSanitizer.cpp?rev=160254&r1=160253&r2=160254&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/Instrumentation/AddressSanitizer.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/Instrumentation/AddressSanitizer.cpp Mon Jul 16 03:58:53 2012<br>
@@ -230,17 +230,17 @@<br>
// Returns the ThenBlock's terminator.<br>
static BranchInst *splitBlockAndInsertIfThen(Value *Cmp) {<br>
Instruction *SplitBefore = cast<Instruction>(Cmp)->getNextNode();<br>
+<br>
+ // Create three basic blocks, with the middle block empty, by splitting twice.<br>
BasicBlock *Head = SplitBefore->getParent();<br>
- BasicBlock *Tail = Head->splitBasicBlock(SplitBefore);<br>
+ BasicBlock *Then = Head->splitBasicBlock(SplitBefore);<br>
+ BasicBlock *Tail = Then->splitBasicBlock(SplitBefore);<br>
+<br>
TerminatorInst *HeadOldTerm = Head->getTerminator();<br>
- LLVMContext &C = Head->getParent()->getParent()->getContext();<br>
- BasicBlock *ThenBlock = BasicBlock::Create(C, "", Head->getParent());<br>
- BranchInst *HeadNewTerm =<br>
- BranchInst::Create(/*ifTrue*/ThenBlock, /*ifFalse*/Tail, Cmp);<br>
- ReplaceInstWithInst(HeadOldTerm, HeadNewTerm);<br>
+ IRBuilder<>(HeadOldTerm).CreateCondBr(Cmp, Then, Tail);<br>
+ HeadOldTerm->eraseFromParent();<br>
<br>
- BranchInst *CheckTerm = BranchInst::Create(Tail, ThenBlock);<br>
- return CheckTerm;<br>
+ return cast<BranchInst>(Then->getTerminator());<br>
}<br>
<br>
Value *AddressSanitizer::memToShadow(Value *Shadow, IRBuilder<> &IRB) {<br>
@@ -387,28 +387,28 @@<br>
Value *Cmp = IRB.CreateICmpNE(ShadowValue, CmpVal);<br>
<br>
Instruction *CheckTerm = splitBlockAndInsertIfThen(Cmp);<br>
- IRBuilder<> IRB2(CheckTerm);<br>
+ IRB.SetInsertPoint(CheckTerm);<br>
<br>
size_t Granularity = 1 << MappingScale;<br>
if (TypeSize < 8 * Granularity) {<br>
// Addr & (Granularity - 1)<br>
- Value *LastAccessedByte = IRB2.CreateAnd(<br>
+ Value *LastAccessedByte = IRB.CreateAnd(<br>
AddrLong, ConstantInt::get(IntptrTy, Granularity - 1));<br>
// (Addr & (Granularity - 1)) + size - 1<br>
if (TypeSize / 8 > 1)<br>
- LastAccessedByte = IRB2.CreateAdd(<br>
+ LastAccessedByte = IRB.CreateAdd(<br>
LastAccessedByte, ConstantInt::get(IntptrTy, TypeSize / 8 - 1));<br>
// (uint8_t) ((Addr & (Granularity-1)) + size - 1)<br>
- LastAccessedByte = IRB2.CreateIntCast(<br>
+ LastAccessedByte = IRB.CreateIntCast(<br>
LastAccessedByte, IRB.getInt8Ty(), false);<br>
// ((uint8_t) ((Addr & (Granularity-1)) + size - 1)) >= ShadowValue<br>
- Value *Cmp2 = IRB2.CreateICmpSGE(LastAccessedByte, ShadowValue);<br>
+ Value *Cmp2 = IRB.CreateICmpSGE(LastAccessedByte, ShadowValue);<br>
<br>
CheckTerm = splitBlockAndInsertIfThen(Cmp2);<br>
+ IRB.SetInsertPoint(CheckTerm);<br>
}<br>
<br>
- IRBuilder<> IRB1(CheckTerm);<br>
- Instruction *Crash = generateCrashCode(IRB1, AddrLong, IsWrite, TypeSize);<br>
+ Instruction *Crash = generateCrashCode(IRB, AddrLong, IsWrite, TypeSize);<br>
Crash->setDebugLoc(OrigIns->getDebugLoc());<br>
ReplaceInstWithInst(CheckTerm, new UnreachableInst(*C));<br>
}<br>
<br>
Modified: llvm/trunk/test/Instrumentation/AddressSanitizer/basic.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Instrumentation/AddressSanitizer/basic.ll?rev=160254&r1=160253&r2=160254&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Instrumentation/AddressSanitizer/basic.ll?rev=160254&r1=160253&r2=160254&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Instrumentation/AddressSanitizer/basic.ll (original)<br>
+++ llvm/trunk/test/Instrumentation/AddressSanitizer/basic.ll Mon Jul 16 03:58:53 2012<br>
@@ -16,11 +16,6 @@<br>
; CHECK: icmp ne i8<br>
; CHECK: br i1 %{{.*}}, label %{{.*}}, label %{{.*}}<br>
;<br>
-; The actual load comes next because ASan adds the last instrumentation block<br>
-; to the end of the function.<br>
-; CHECK: %tmp1 = load i32* %a<br>
-; CHECK: ret i32 %tmp1<br>
-;<br>
; First instrumentation block refines the shadow test.<br>
; CHECK: and i64 %[[LOAD_ADDR]], 7<br>
; CHECK: add i64 %{{.*}}, 3<br>
@@ -28,9 +23,13 @@<br>
; CHECK: icmp sge i8 %{{.*}}, %[[LOAD_SHADOW]]<br>
; CHECK: br i1 %{{.*}}, label %{{.*}}, label %{{.*}}<br>
;<br>
-; Final instrumentation block reports the error.<br>
+; Second instrumentation block reports the error.<br>
; CHECK: call void @__asan_report_load4(i64 %[[LOAD_ADDR]]) noreturn<br>
; CHECK: unreachable<br>
+;<br>
+; Finally the instrumented load.<br>
+; CHECK: %tmp1 = load i32* %a<br>
+; CHECK: ret i32 %tmp1<br>
<br>
entry:<br>
%tmp1 = load i32* %a<br>
@@ -48,11 +47,6 @@<br>
; CHECK: icmp ne i8<br>
; CHECK: br i1 %{{.*}}, label %{{.*}}, label %{{.*}}<br>
;<br>
-; The actual store comes next because ASan adds the last instrumentation block<br>
-; to the end of the function.<br>
-; CHECK: store i32 42, i32* %a<br>
-; CHECK: ret void<br>
-;<br>
; First instrumentation block refines the shadow test.<br>
; CHECK: and i64 %[[STORE_ADDR]], 7<br>
; CHECK: add i64 %{{.*}}, 3<br>
@@ -60,9 +54,13 @@<br>
; CHECK: icmp sge i8 %{{.*}}, %[[STORE_SHADOW]]<br>
; CHECK: br i1 %{{.*}}, label %{{.*}}, label %{{.*}}<br>
;<br>
-; Final instrumentation block reports the error.<br>
+; Second instrumentation block reports the error.<br>
; CHECK: call void @__asan_report_store4(i64 %[[STORE_ADDR]]) noreturn<br>
; CHECK: unreachable<br>
+;<br>
+; Finally the instrumented store.<br>
+; CHECK: store i32 42, i32* %a<br>
+; CHECK: ret void<br>
<br>
entry:<br>
store i32 42, i32* %a<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div></div>
</blockquote></div></div></div><br>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>