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

[Alexey Samsonov]

================
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";
 
----------------
Kostya Serebryany wrote:
> 80 chars
Done

================
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"),
----------------
Kostya Serebryany wrote:
> maybe asan-check-lifetime? 
Done

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

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

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1092
@@ +1091,3 @@
+  Type *AllocaType = Alloca->getType();
+  Type *Int8PtrTy = Type::getInt8PtrTy(AllocaType->getContext());
+
----------------
Kostya Serebryany wrote:
> IRBuilder has IRB.getInt8PtrTy()
What's the difference between this and creating (some) IRBuilder to getInt8PtrTy from it?

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

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

================
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);
----------------
Kostya Serebryany wrote:
> Why? 
> Don't we unpoison these things twice? 
No, we _poison_ stuff at llvm.lifetime.end and should unpoison it back here.


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