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

[Philip Reames via Phabricator via llvm-commits]

reames marked 3 inline comments as done.
reames added a comment.

Address comments in advance of patch refresh.



================
Comment at: lib/Transforms/Instrumentation/PoisonChecking.cpp:217
+  for (unsigned i = 1; i < GEP.getNumOperands() && IsNonNegativeOffset; i++)
+    IsNonNegativeOffset &= isKnownNonNegative(GEP.getOperand(i), DL);
+
----------------
jdoerfert wrote:
> While I now thing this is correct, we might want to make it less restrictive using existing functionality to accumulate and strip pointer stuff, e.g., along the lines of:
> 
> ```
> const Value *Base = GEP->stripAndAccumulateConstantOffsets(const DataLayout &DL,
>                                                  APInt &Offset,
>                                                  bool AllowNonInbounds);
> IsNonNegativeOffset = (Base == Obj) && Offset >= 0;
> ```
> 
Your proposal is less powerful than the existing code as it's restricted to constants where the current code is didn't.  Did you have a particular missed case in mind?


================
Comment at: lib/Transforms/Instrumentation/PoisonChecking.cpp:234
+                                   "upper.limit");
+    Checks.push_back(B.CreateICmp(ICmpInst::ICMP_UGT, Addr, UpperLimit));
+  }
----------------
aqjune wrote:
> Is this case covered as well?
> ```
> a = alloca i32 // 4 bytes
> p1 = gep a, -1
> p2 = gep inbounds p1, 2
> ```
> p2 is poison because p1 is not in-bounds pointer.
> UpperCheck will succeed because Addr is smaller than a + 4.
Hadn't been handled in the original patch, see forthcoming refresh.


================
Comment at: lib/Transforms/Instrumentation/PoisonChecking.cpp:280
+    break;
+  }
   };
----------------
jdoerfert wrote:
> aqjune wrote:
> > A silly question: where is the poison-ness of operands of GEP checked at?
> I think this is relates to your other question: nowhere yet (I think)
> 
> Though, this is a really good point. The check for an inbound gep could include one to ensure a valid base pointer. How to do that is tricky though. If the base allocation is known, one can substract the base pointer from the base allocation and do `diff >= 0 && diff <= length`. If the base allocation is not known, we would need to track more information throughout the program. There are different ways to do this but I guess the first case should be covered next.
I think nlopes question was actually about one of the operands being directly poison.  Which is handled via the propagatesFullPoison code.  


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

https://reviews.llvm.org/D64451