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

[Chandler Carruth]

  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.

  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.

  Anyways, thanks for working on this! Very excited to get this functionality.


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

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


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