[PATCH] D60617: MSan: handle llvm.lifetime.start intrinsic

[Alexander Potapenko via Phabricator via llvm-commits]

glider marked 4 inline comments as done.
glider added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1037
+  SmallSet<AllocaInst *, 16> AllocaSet;
+  SmallVector<std::pair<IntrinsicInst *, AllocaInst *>, 16> LifetimeStartList;
   SmallVector<StoreInst *, 16> StoreList;
----------------
eugenis wrote:
> This does not have to be so complicated.
> Get rid of FallbackAllocaSet; instead remove instructions from AllocaSet as they are being poisoned at lifetime calls, then poison remaining alloca instructions.
> 
> This way if untraceable lifetime was seen, you can simply skip the lifetime poisoning step.
> 
> Please rename InstrumentOnlyAllocas to something that does not suggest the we are instrumenting _only_ allocas (and nothing else).
> 
Agreed, we don't need FallbackAllocaSet, we can just poison allocas from LifetimeStartList as if they were in AllocaSet (see the updated patch).


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:2574
+    if (Len->isMinusOne())
+      return;
+
----------------
eugenis wrote:
> What's wrong with variable length allocas?
You're right, fixed this.


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1300
+      Instruction *InsertAt = InstrumentLifetimeStart ? Item.first : nullptr;
+      instrumentAlloca(*Item.second, InsertAt);
+    }
----------------
Reused `instrumentAlloca()` here instead of calling KMSAN/userspace-specific functions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60617/new/

https://reviews.llvm.org/D60617