[PATCH] D139811: sanmd: improve precision of UAR analysis

[Marco Elver via Phabricator via llvm-commits]

melver added inline comments.


================
Comment at: compiler-rt/test/metadata/uar.cpp:79
+  FN(local_array);                                                             \
   /**/
 
----------------
Add test for explicit __builtin_alloca ?


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp:274
 
+static bool hasUseAfterReturnUnsafeUses(Value &V) {
+  for (User *U : V.users()) {
----------------
dvyukov wrote:
> dvyukov wrote:
> > I was looking at isSafeAndProfitableToSinkLoad:
> > https://github.com/llvm/llvm-project/blob/881076cde29699ef2a458c9d957ebcd49ded8893/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp#L621
> > 
> > and isAllocaPromotable:
> > https://github.com/llvm/llvm-project/blob/af4e856fa71c6b5086aeda79bfcd954e98cef591/llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp#L63
> Btw, isSafeAndProfitableToSinkLoad is broken. All allocas now sink to lifetime intrinsics. So it will conclude all allocas are address-taken. This immediately popped up in our end-to-end C++ tests, but wasn't detected by the InstCombine bitcode tests.
All this is already in a big anonymous namespace, so static not needed.


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp:274
 
+static bool hasUseAfterReturnUnsafeUses(Value &V) {
+  for (User *U : V.users()) {
----------------
melver wrote:
> dvyukov wrote:
> > dvyukov wrote:
> > > I was looking at isSafeAndProfitableToSinkLoad:
> > > https://github.com/llvm/llvm-project/blob/881076cde29699ef2a458c9d957ebcd49ded8893/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp#L621
> > > 
> > > and isAllocaPromotable:
> > > https://github.com/llvm/llvm-project/blob/af4e856fa71c6b5086aeda79bfcd954e98cef591/llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp#L63
> > Btw, isSafeAndProfitableToSinkLoad is broken. All allocas now sink to lifetime intrinsics. So it will conclude all allocas are address-taken. This immediately popped up in our end-to-end C++ tests, but wasn't detected by the InstCombine bitcode tests.
> All this is already in a big anonymous namespace, so static not needed.
Could file bug (https://github.com/llvm/llvm-project/issues) and point out that it needs the isLifetimeStartOrEnd() check, but we don't quite know if it's intended?


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp:280-293
+    if (isa<LoadInst>(U))
+      continue;
+    if (auto *SI = dyn_cast<StoreInst>(U)) {
+      // If storing TO the alloca, then the address isn't taken.
+      if (SI->getOperand(1) == &V)
+        continue;
+    }
----------------
LoadInst, StoreInst, GetElementPtrInst and BitCastInst all are derived from Instruction, so these checks can be moved in the 'if (... dyn_cast<Instruction>' branch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139811