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

[Kostya Serebryany]

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:74
@@ -73,1 +73,3 @@
+static const char *kAsanStackPoisonMemoryName = "__asan_stack_poison_memory";
+static const char *kAsanStackUnpoisonMemoryName = "__asan_stack_unpoison_memory";
 
----------------
80 chars

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:141
@@ -138,1 +140,3 @@
 
+static cl::opt<bool> ClUseLifetime("asan-use-lifetime",
+       cl::desc("Use llvm.lifetime.intrinsics to insert extra checks"),
----------------
maybe asan-check-lifetime? 

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1246
@@ +1245,3 @@
+
+void AddressSanitizer::poisonAlloca(Value *V, uint64_t Size,
+                                    IRBuilder<> IRB) {
----------------
two functions with identical body. Maybe merge into one and pass Func as a param? 

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1050
@@ -1023,1 +1049,3 @@
 
+void AddressSanitizer::handleValueLifetime(Value *V, Value *Origin,
+                                           PoisonedAllocaMap &PoisonMap) {
----------------
The whole thing deserves a wide comment describing the algorithm. 

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1055
@@ +1054,3 @@
+       ++UI) {
+    if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(*UI)) {
+      Intrinsic::ID ID = II->getIntrinsicID();
----------------
I typically prefer 
  if (not-good) continue;
  lots-of-code
instead of 
  if (good) {
    lots-of-code
  }

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1078
@@ +1077,3 @@
+          // Update the maximal poisoning for alloca.
+          if (PoisonMap[Origin] < SizeValue)
+            PoisonMap[Origin] = SizeValue;
----------------
Does this mean we may have the same Alloca have different scopes?
This also deserves a comment. 

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1092
@@ +1091,3 @@
+  Type *AllocaType = Alloca->getType();
+  Type *Int8PtrTy = Type::getInt8PtrTy(AllocaType->getContext());
+
----------------
IRBuilder has IRB.getInt8PtrTy()

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1225
@@ -1139,1 +1224,3 @@
     PoisonStack(ArrayRef<AllocaInst*>(AllocaVec), IRBRet, ShadowBase, false);
+    // Unpoison all local variables poisoned in llvm.lifetime analysis.
+    unpoisonAllocas(PoisonedAlloca, IRBRet);
----------------
Why? 
Don't we unpoison these things twice? 


http://llvm-reviews.chandlerc.com/D140