[llvm-commits] AddressSanitizer, first patch

[Eric Christopher]

On Sep 8, 2011, at 5:14 PM, Kostya Serebryany wrote:

> 
> 
> On Thu, Sep 8, 2011 at 4:43 PM, Eric Christopher <echristo at apple.com> wrote:
> 
> On Sep 8, 2011, at 4:35 PM, Chandler Carruth wrote:
> 
> > FYI,
> >
> > I've looked again at this patch recently, and talked to several others about it...
> >
> > 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.
> >
> > 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.
> >
> > 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.
> >
> 
> This encompasses most of my concerns with the patch.
> 
> > Maybe others could comment on other implementation issues? I'm sadly not an expert at LLVM...
> 
> 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.
> 
> I dropped the assembly part and updated the patch at http://codereview.appspot.com/4867059/ (also in attachment).
> We can figure out how to make it more efficient later (something like llvm.trap intrinsic with a parameter).
> 
> This patch is 570 LOC plus few lines of trivial changes and a test.
> It implements only basic functionality: detection of out-of-bounds and use-after-free in heap.
> 
> The complete version of LLVM pass is ~1000 LOC (http://code.google.com/p/address-sanitizer/source/browse/trunk/llvm/AddressSanitizer.cpp).
> It also handles out-of-bounds for stack and global objects and (recently added) stack use-after-return bugs. 

Some somewhat trivial issues:

a) Please go ahead and hide the non-user visible flags.

b) Not a big fan of this style of avoid 80-col wrap:

+Instruction *AddressSanitizer::generateCrashCode(
+    IRBuilder<> &IRB, Value *Addr, int TelltaleValue) {

c) +  // which will be lowered to a cusom assembly.

s/cusom/custom

d) 

+  if (TypeSize != 8 && TypeSize != 16 &&
+      TypeSize != 32 && TypeSize != 64 && TypeSize != 128) {
+    // TODO(kcc): do something better.
+    return;
+  }

What's the reason behind this?

Perhaps not so trivial:

a) Where are the asan_* functions? Where are they supposed to live?

I'll probably have more, but it's a start and the discussion on the library functions probably should happen sooner rather than later.

-eric
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110908/68d2bce9/attachment.html>