[llvm-commits] [PATCH] AddressSanitizer: handle llvm.lifetime intrinsics (LLVM part)

[Alexey Samsonov]

On Thu, Nov 29, 2012 at 1:22 AM, Kostya Serebryany <kcc at google.com> wrote:

>
>
> On Thu, Nov 29, 2012 at 11:59 AM, Chandler Carruth <chandlerc at gmail.com>wrote:
>
>>
>>   Just FYI, I think it would be good to have Nick look at this as he
>> worked on the design and implementation of the lifetime markers.
>>
>
> Yep!
>
>
>>
>>   One question I had -- why do this after merging the allocas? My gut
>> instinct was that it would've been simpler to do it when the allocas are
>> nice and distinct.... but maybe not.
>>
>
> Not sure how it can help us. It may hurt us though, because before we've
> merged allocas we can not be 100% sure that a particular alloca will get
> merged.
>

True. We should only do lifetime analysis for allocas we replace, and at
that point it makes little difference which value to use - alloca we're
replacing right now,
or the replacement.


>
>>   Anyways, thanks for working on this! Very excited to get this
>> functionality.
>>
>
> My only worry is about performance: whether the additional slowdown will
> allow us to enable the new feature by default. Will see...
>
>
>>
>>
>> ================
>> Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1055
>> @@ +1054,3 @@
>> +// (3) if %size is constant, unpoison underlying shadow memory
>> +//     for llvm.lifetime.start and poison it for llvm.lifetime.end.
>> +// (4) store the maximal value of %size argument for each %alloca
>> ----------------
>> Alexey Samsonov wrote:
>> > Kostya Serebryany wrote:
>> > > Do we actually need to unpoison on llvm.lifetime.start ?
>> > > Isn't the memory unpoisoned already?
>> > What about the loops? Two function calls for each local variable for
>> each loop iteration ultimately suck, we need to think of some optimizations
>> here...
>> The fix to this should be the same as the fix to all of the
>> local-alloca-instrumentation I'm working on... I wouldn't worry about this
>> just yet, it should fall out later.
>>
>
> Ok, agree. We need to unpoison at llvm.lifetime.start. Please reflect the
> reason in comments.
>

Done.


> As for poisoning at the function entry -- it may cost us too much (imagine
> a cold basic block with life-time markers in a hot function)
>

Didn't understand this.


>
>
>>
>> ================
>> Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1057
>> @@ +1056,3 @@
>> +// (4) store the maximal value of %size argument for each %alloca
>> +//     to make sure we unpoison all the poisoned memory at function exit.
>> +void AddressSanitizer::handleValueLifetime(Value *V, Value *Origin,
>> ----------------
>> Alexey Samsonov wrote:
>> > Kostya Serebryany wrote:
>> > > I am still not getting this.
>> > > Why would any alloca be mentioned in multiple llvm.lifetime.start?
>> > > Does this happen?
>> > This is allowed, but unlikely. No, I haven't seen such cases.
>> Imagine this code with very tight lifetime markers emitted by Clang:
>>
>>   void f() {
>>     // ... some interesting code ...
>>     for (...) {
>>       // .. some code that cannot access 'arr'
>>       if (...) {
>>         char arr[42];
>>         // ... some code using 'arr' ...
>>       }
>>       // .. some more code that cannot access 'arr'
>>     }
>>   }
>>
>> If we unroll the loop, we would want to have a single alloca with N
>> distinct start -> end lifetimes for the N loop iterations unrolled.
>>
>
Ah, thanks for the example!


>
> Got it. Still, if we do a single unpoison call for the entire alloca, we
> don't need to remember sizes. (and no need for the Map object)
>

You're right, removed the map for good.


>
>
>>
>>
>> ================
>> Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1055
>> @@ +1054,3 @@
>> +// (3) if %size is constant, unpoison underlying shadow memory
>> +//     for llvm.lifetime.start and poison it for llvm.lifetime.end.
>> +// (4) store the maximal value of %size argument for each %alloca
>> ----------------
>> Chandler Carruth wrote:
>> > Alexey Samsonov wrote:
>> > > Kostya Serebryany wrote:
>> > > > Do we actually need to unpoison on llvm.lifetime.start ?
>> > > > Isn't the memory unpoisoned already?
>> > > What about the loops? Two function calls for each local variable for
>> each loop iteration ultimately suck, we need to think of some optimizations
>> here...
>> > The fix to this should be the same as the fix to all of the
>> local-alloca-instrumentation I'm working on... I wouldn't worry about this
>> just yet, it should fall out later.
>> Regarding your comment Kostya, if we have a lifetime start and end, we
>> shouldn't have the memory unpoisoned to start with. It should start
>> poisoned and become unpoisoned at the lifetime start.
>>
>> The tricky thing is that LLVM's allocas are at the start of the function,
>> but the lifetime of a local variable might not start for some time within
>> the function. We'll still probably catch more compiler bugs than anything
>> else here due to the scoping rules, but hey, nothing wrong with that.
>>
>>
>> http://llvm-reviews.chandlerc.com/D140
>>
>
>

-- 
Alexey Samsonov, MSK
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121129/4e016c1d/attachment.html>