[llvm-commits] AddressSanitizer, first patch

[Eli Friedman]

On Thu, Sep 8, 2011 at 5:14 PM, Kostya Serebryany <kcc at google.com> 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.

A couple review comments:

AddressSanitizer::appendToGlobalCtors is messy at best; is there
really no better way to get the asan init function to run when it
needs to?

Please get rid of blockHasException and the dependent code; the
eh_exception intrinsic is going away very soon, and you shouldn't run
into similar issues with the new exception handling framework.

+  GlobalValue *asan_mapping_offset =
+      new GlobalVariable(M, IntPtrTy, true, GlobalValue::LinkOnceODRLinkage,
+                     ConstantInt::get(IntPtrTy, 1ULL << MappingOffsetLog),
+                     "__asan_mapping_offset");
+  GlobalValue *asan_mapping_scale =
+      new GlobalVariable(M, IntPtrTy, true, GlobalValue::LinkOnceODRLinkage,
+                         ConstantInt::get(IntPtrTy, MappingScale),
+                         "__asan_mapping_scale");

If you use WeakLinkage for these, you won't need the fake loads.

-Eli