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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: lib/Transforms/Instrumentation/PoisonChecking.cpp:208
+    // TODO: expand this and integrate w/MemoryBuiltins.h
+    return isa<AllocaInst>(V);
+  };
----------------
reames wrote:
> 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.  
That is fine with me.


================
Comment at: lib/Transforms/Instrumentation/PoisonChecking.cpp:234
+                                   "upper.limit");
+    Checks.push_back(B.CreateICmp(ICmpInst::ICMP_UGT, Addr, UpperLimit));
+  }
----------------
reames wrote:
> 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?
Yes. An offset of one in the right direction, compared to what you have now, is required. Either change is fine.


================
Comment at: lib/Transforms/Instrumentation/PoisonChecking.cpp:237
+  if (GenLowerCheck)
+    Checks.push_back(B.CreateICmp(ICmpInst::ICMP_ULT, Addr, Base));
+}
----------------
reames wrote:
> 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?  
I would have thought the reasoning described by @nlopes is only valid for `inbounds` GEPs. "Normal" GEPs should be allowed to go out-of-bounds without producing poison (IMHO).


================
Comment at: lib/Transforms/Instrumentation/PoisonChecking.cpp:217
+  for (unsigned i = 1; i < GEP.getNumOperands() && IsNonNegativeOffset; i++)
+    IsNonNegativeOffset &= isKnownNonNegative(GEP.getOperand(i), DL);
+
----------------
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;
```



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

https://reviews.llvm.org/D64451