[llvm-commits] AddressSanitizer, first patch

[Kostya Serebryany]

On Thu, Sep 8, 2011 at 5:38 PM, Eric Christopher <echristo at apple.com> wrote:

>
> 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.
>
done

>
> b) Not a big fan of this style of avoid 80-col wrap:
>
> +Instruction *AddressSanitizer::generateCrashCode(
> +    IRBuilder<> &IRB, Value *Addr, int TelltaleValue) {
>
done

>
> 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?
>

Changed the comment.


>
> 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/20110909/f93ee41e/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: issue4867059_14009.diff
Type: text/x-patch
Size: 23474 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110909/f93ee41e/attachment.bin>