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

Kostya Serebryany kcc at google.com
Thu Nov 29 01:22:28 PST 2012


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.

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


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

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)


>
>
> ================
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121129/512cc714/attachment.html>


More information about the llvm-commits mailing list