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

[Philip Reames via Phabricator via llvm-commits]

reames planned changes to this revision.
reames marked 4 inline comments as done.
reames added inline comments.


================
Comment at: lib/Transforms/Instrumentation/PoisonChecking.cpp:208
+    // TODO: expand this and integrate w/MemoryBuiltins.h
+    return isa<AllocaInst>(V);
+  };
----------------
jdoerfert wrote:
> `GlobalVariable` is another easy one, and `isMallocLikeFn`.
GlobalVariable was a bit unclear to me since I haven't studied the rules for conflicting definitions in different modules.

isMallocLikeFn is definitely valid, but requires TLI (which this pass currently doesn't have).  I was planning on doing that in a follow up.  


================
Comment at: lib/Transforms/Instrumentation/PoisonChecking.cpp:213
+  for (unsigned i = 1; i < GEP.getNumOperands(); i++)
+    IsNonNegativeOffset &= isKnownNonNegative(GEP.getOperand(i), DL);
+
----------------
jdoerfert wrote:
> 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.
Good catch!  Yeah, definitely needs fixed.


================
Comment at: lib/Transforms/Instrumentation/PoisonChecking.cpp:234
+                                   "upper.limit");
+    Checks.push_back(B.CreateICmp(ICmpInst::ICMP_UGT, Addr, UpperLimit));
+  }
----------------
jdoerfert wrote:
> > 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.
This should be handled by using UGT not UGE right?


================
Comment at: lib/Transforms/Instrumentation/PoisonChecking.cpp:237
+  if (GenLowerCheck)
+    Checks.push_back(B.CreateICmp(ICmpInst::ICMP_ULT, Addr, Base));
+}
----------------
nlopes wrote:
> Doing these pointer comparisons isn't legal, and LLVM can fold "out_of_bonds_ptr >= valid_ptr" to poison.
> You need to cast those pointers to integers and do the comparison there: offset >= 0 && offset <= obj_size.  You may want to lower the GEP in IR right away (there's a function for that in ValueTracking or somewhere else, I forget)?
Can you cite where in the LangRef it says this?  I think you're quite possibly right, but a) I didn't find the wording with a quick search and b) the output seems to survive the optimizer in simple cases which is maybe a hint we don't implement it that way?  


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64451