[PATCH] D64451: [PoisonChecking] Validate inbounds annotation on getelementptr where possible

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Three comments, though I need to look at it when I'm actually awake.



================
Comment at: lib/Transforms/Instrumentation/PoisonChecking.cpp:208
+    // TODO: expand this and integrate w/MemoryBuiltins.h
+    return isa<AllocaInst>(V);
+  };
----------------
`GlobalVariable` is another easy one, and `isMallocLikeFn`.


================
Comment at: lib/Transforms/Instrumentation/PoisonChecking.cpp:213
+  for (unsigned i = 1; i < GEP.getNumOperands(); i++)
+    IsNonNegativeOffset &= isKnownNonNegative(GEP.getOperand(i), DL);
+
----------------
You use `GetUnderlyingObject` to get a base which is used in checks later but you look at the indices of this GEP only.

I think that is not OK, e.g., if you have two geps, one all positive indices and the one before negative ones.


================
Comment at: lib/Transforms/Instrumentation/PoisonChecking.cpp:234
+                                   "upper.limit");
+    Checks.push_back(B.CreateICmp(ICmpInst::ICMP_UGT, Addr, UpperLimit));
+  }
----------------
> The in bounds addresses for an allocated object are all the addresses that point into the object, plus the address one byte past the end.

You might need to adjust `SizeInBytes` by one, though it's late.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64451