<div style="font-family: arial, helvetica, sans-serif; font-size: 10pt"><br><br><div class="gmail_quote">On Thu, Nov 29, 2012 at 11:59 AM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
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.<br></blockquote><div><br></div><div>Yep! </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
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.<br></blockquote><div><br></div><div>
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. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Anyways, thanks for working on this! Very excited to get this functionality.<br></blockquote><div><br></div><div>My only worry is about performance: whether the additional slowdown will allow us to enable the new feature by default. Will see... </div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
<br>
================<br>
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1055<br>
@@ +1054,3 @@<br>
+// (3) if %size is constant, unpoison underlying shadow memory<br>
+// for llvm.lifetime.start and poison it for llvm.lifetime.end.<br>
+// (4) store the maximal value of %size argument for each %alloca<br>
----------------<br>
</div><div class="im">Alexey Samsonov wrote:<br>
> Kostya Serebryany wrote:<br>
> > Do we actually need to unpoison on llvm.lifetime.start ?<br>
> > Isn't the memory unpoisoned already?<br>
> 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...<br>
</div>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.<br></blockquote><div><br></div><div>
Ok, agree. We need to unpoison at llvm.lifetime.start. Please reflect the reason in comments. </div><div>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)</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
================<br>
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1057<br>
@@ +1056,3 @@<br>
+// (4) store the maximal value of %size argument for each %alloca<br>
+// to make sure we unpoison all the poisoned memory at function exit.<br>
+void AddressSanitizer::handleValueLifetime(Value *V, Value *Origin,<br>
----------------<br>
</div><div class="im">Alexey Samsonov wrote:<br>
> Kostya Serebryany wrote:<br>
> > I am still not getting this.<br>
> > Why would any alloca be mentioned in multiple llvm.lifetime.start?<br>
> > Does this happen?<br>
> This is allowed, but unlikely. No, I haven't seen such cases.<br>
</div>Imagine this code with very tight lifetime markers emitted by Clang:<br>
<br>
void f() {<br>
// ... some interesting code ...<br>
for (...) {<br>
// .. some code that cannot access 'arr'<br>
if (...) {<br>
char arr[42];<br>
// ... some code using 'arr' ...<br>
}<br>
// .. some more code that cannot access 'arr'<br>
}<br>
}<br>
<br>
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.<br></blockquote><div><br></div><div>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)</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
<br>
================<br>
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1055<br>
@@ +1054,3 @@<br>
+// (3) if %size is constant, unpoison underlying shadow memory<br>
+// for llvm.lifetime.start and poison it for llvm.lifetime.end.<br>
+// (4) store the maximal value of %size argument for each %alloca<br>
----------------<br>
</div>Chandler Carruth wrote:<br>
<div class="im">> Alexey Samsonov wrote:<br>
> > Kostya Serebryany wrote:<br>
> > > Do we actually need to unpoison on llvm.lifetime.start ?<br>
> > > Isn't the memory unpoisoned already?<br>
> > 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...<br>
</div>> 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.<br>
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.<br>
<br>
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.<br>
<br>
<br>
<a href="http://llvm-reviews.chandlerc.com/D140" target="_blank">http://llvm-reviews.chandlerc.com/D140</a><br>
</blockquote></div><br></div>