Hi, <div><br></div><div>I haven't heard any more comments. </div><div>Eric Christopher confirmed that the only concern is having the instrumentation pass w/o the run-time library. </div><div>I do want to contribute run-time as well. </div>
<div>How do we proceed? </div><div><br></div><div>Let me describe the whole thing once more. </div><div><br></div><div><div>AddressSanitizer finds </div><div> - Out-of-bounds accesses to heap, stack and globals</div><div>
- Use-after-free (heap)</div><div>The slowdown is typically within 2x: <a href="http://code.google.com/p/address-sanitizer/wiki/PerformanceNumbers">http://code.google.com/p/address-sanitizer/wiki/PerformanceNumbers</a></div>
</div><div>It also has an experimental feature to detect use-after-return (stack). </div><div><br></div><div>The tool currently works on x86 Linux and Mac, both 32- and 64- bits. </div><div>The Linux port is absolutely stable (one can browse the web using instrumented Chromium for hours). </div>
<div>The Mac port is slightly less stable (Chromium tests work, but the browser hiccups infrequently).</div><div><br></div><div>The tool consists of two parts: instrumentation pass (~1KLOC) and run-time library (~3KLOC). </div>
<div><br></div><div>The instrumentation pass being reviewed in this mail thread only supports detection of heap bugs. </div><div>Other parts (~400 more LOC) will add stack, globals, and blacklisting. </div><div><br></div>
<div>The run-time library replaces malloc/free/etc, intercepts other libc functions (e.g. pthread_create) and does error reporting.</div><div>The library and test code uses google coding conventions.</div><div>The integration tests use gtest.</div>
<div><br></div><div>I'd like to have the run-time available as part of LLVM, and I would agree on any reasonable way of doing so in the following order of preference:</div><div> - use svn:externals to point to the existing repository at <a href="http://code.google.com/p/address-sanitizer/source/checkout">http://code.google.com/p/address-sanitizer/source/checkout</a></div>
<div> - copy the code to LLVM repository and periodically take the fresh versions from c.g.c repository. </div><div> - use the LLVM repository as the primary repository (here I am concerned with the tool chain: we are too used to the features provided by c.g.c.)</div>
<div>Please suggest what suits LLVM the most.</div><div><br></div><div>Thanks, </div><div><br></div><div>--kcc <br><br><div class="gmail_quote">On Mon, Sep 12, 2011 at 11:48 AM, Kostya Serebryany <span dir="ltr"><<a href="mailto:kcc@google.com">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;"><div>FYI</div><div>I've added noreturn attribute to the __asan_report_error call. It doesn't cause compile problems in cpu2006 any more. </div>
<div>The good news is that the slowdown (compared to assembly with ud2) on cpu2006 ranges between 1% and 8%. </div>
<div>And we may recover from it if __asan_report_error does not cause stack frame to be created in leaf functions. </div><div><br></div><div>--kcc </div><div class="HOEnZb"><div></div><div class="h5"><div><br><div><br><div class="gmail_quote">
On Fri, Sep 9, 2011 at 12:48 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 Thu, Sep 8, 2011 at 5:38 PM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@apple.com" target="_blank">echristo@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div></div><div><br><div><div>On Sep 8, 2011, at 5:14 PM, Kostya Serebryany wrote:</div><br><blockquote type="cite"><br><br><div class="gmail_quote">On Thu, Sep 8, 2011 at 4:43 PM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@apple.com" target="_blank">echristo@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div></div><div><br>
On Sep 8, 2011, at 4:35 PM, Chandler Carruth wrote:<br>
<br>
> FYI,<br>
><br>
> I've looked again at this patch recently, and talked to several others about it...<br>
><br>
> I think there are some big issues with the implementation, bit I think they could probably be addressed. The one that I see the most is the x86 assembly just being dumped as a blob. I think that really needs to be resolved before this can move forward.<br>
><br>
> It seems like this should be do-able in some way with the llvm.trap intrinsic. If anything, we might need another intrinsic or an extension of llvm.trap that allows feeding the SIGILL signal handler the data arguments it needs; I'd need to read up on exactly what is required for that to figure out how best to represent that in IR, but it shouldn't be that hard.<br>
><br>
> That said, maybe to improve the discussion of the patch and make reviewing it easier, you could just switch to emitting the call to the runtime library function in all cases? That should be a strictly simpler patch, and then how to improve performance by avoiding the runtime call can be a followup.<br>
><br>
<br>
</div></div>This encompasses most of my concerns with the patch.<br>
<div><br>
> Maybe others could comment on other implementation issues? I'm sadly not an expert at LLVM...<br>
<br>
</div>In a glance I didn't see a lot, but the other bits were somewhat hiding it. How about when we get a new patch we can take a bit more of a look at it.<br></blockquote><div><br></div><div>I dropped the assembly part and updated the patch at <a href="http://codereview.appspot.com/4867059/" target="_blank">http://codereview.appspot.com/4867059/</a> (also in attachment).</div>
<div>We can figure out how to make it more efficient later (something like llvm.trap intrinsic with a parameter).</div><div><br></div><div>This patch is 570 LOC plus few lines of trivial changes and a test.</div><div>It implements only basic functionality: detection of out-of-bounds and use-after-free in heap.</div>
<div><br></div><div>The complete version of LLVM pass is ~1000 LOC (<a href="http://code.google.com/p/address-sanitizer/source/browse/trunk/llvm/AddressSanitizer.cpp" target="_blank">http://code.google.com/p/address-sanitizer/source/browse/trunk/llvm/AddressSanitizer.cpp</a>).</div>
<div>It also handles out-of-bounds for stack and global objects and (recently added) stack use-after-return bugs. </div></div></blockquote><br></div></div></div><div>Some somewhat trivial issues:</div><div><br></div><div>
<div>a) Please go ahead and hide the non-user visible flags.</div></div></div></blockquote></div><div>done </div><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word">
<div><div><br></div><div>b) Not a big fan of this style of avoid 80-col wrap:</div><div><br></div><div><div>+Instruction *AddressSanitizer::generateCrashCode(</div><div>+ IRBuilder<> &IRB, Value *Addr, int TelltaleValue) {</div>
</div></div></div></blockquote></div><div>done </div><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><div>
<br></div><div>c) + // which will be lowered to a cusom assembly.</div>
<div><br></div><div>s/cusom/custom</div><div><br></div><div>d) </div><div><br></div><div><div>+ if (TypeSize != 8 && TypeSize != 16 &&</div><div>+ TypeSize != 32 && TypeSize != 64 && TypeSize != 128) {</div>
<div>+ // TODO(kcc): do something better.</div><div>+ return;</div><div>+ }</div></div><div><br></div><div>What's the reason behind this?</div></div></div></div></blockquote><div><br></div></div><div>Changed the comment.</div>
<div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><div><br></div><div>Perhaps not so trivial:</div><div><br></div>
<div>a) Where are the asan_* functions? Where are they supposed to live?</div><div><br></div><div>I'll probably have more, but it's a start and the discussion on the library functions probably should happen sooner rather than later.</div>
<div><br></div><span><font color="#888888"><div>-eric</div></font></span></div></div></div></blockquote></div></div><br>
</blockquote></div><br></div></div>
</div></div></blockquote></div><br></div>