C++: respond to further PR comments

cpp/ql/src/semmle/code/cpp/controlflow/IRGuards.qll

@@ -295,7 +295,7 @@ class IRGuardCondition extends Instruction {
      * return x;
      * ```
      */
-    predicate controlsEdgeDirectly(ConditionalBranchInstruction branch, IRBlock succ, boolean testIsTrue) {
+    predicate hasBranchEdge(ConditionalBranchInstruction branch, IRBlock succ, boolean testIsTrue) {
       branch.getCondition() = this and
       (
         testIsTrue = true and

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll

@@ -751,6 +751,9 @@ class BinaryInstruction extends Instruction {
     result = getAnOperand().(RightOperand).getDefinitionInstruction()
   }
   
+  /**
+   * Holds if this instruction's operands are `op1` and `op2`, in either order.
+   */
   final predicate hasOperands(Operand op1, Operand op2) {
     op1 = getAnOperand().(LeftOperand) and op2 = getAnOperand().(RightOperand)
     or

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/ValueNumbering.qll

@@ -83,6 +83,10 @@ class ValueNumber extends TValueNumber {
       instr order by instr.getBlock().getDisplayIndex(), instr.getDisplayIndexInBlock()
     )
   }
+  
+  final Operand getAUse() {
+    this = valueNumber(result.getDefinitionInstruction())
+  }
 }
 
 /**
@@ -220,6 +224,13 @@ ValueNumber valueNumber(Instruction instr) {
   )
 }
 
+/**
+ * Gets the value number assigned to `instr`, if any. Returns at most one result.
+ */
+ValueNumber valueNumberOfOperand(Operand op) {
+  result = valueNumber(op.getDefinitionInstruction())
+}
+
 /**
  * Gets the value number assigned to `instr`, if any, unless that instruction is assigned a unique
  * value number.

cpp/ql/src/semmle/code/cpp/rangeanalysis/Bound.qll

@@ -6,10 +6,6 @@ private newtype TBound =
   TBoundInstruction(Instruction i) {
     i.getResultType() instanceof IntegralType or
     i.getResultType() instanceof PointerType
-  } or
-  TBoundOperand(Operand o) {
-    o.getDefinitionInstruction().getResultType() instanceof IntegralType or
-    o.getDefinitionInstruction().getResultType() instanceof PointerType
   }
 
 /**
@@ -52,24 +48,4 @@ class InstructionBound extends Bound, TBoundInstruction {
   override predicate hasLocationInfo(string path, int sl, int sc, int el, int ec) {
     getInstruction().getLocation().hasLocationInfo(path, sl, sc, el, ec)
   }
-}
-
-/**
- * A bound corresponding to the value of an `Operand`.
- */
-class OperandBound extends Bound, TBoundOperand {
-  Operand getOperand() {
-    this = TBoundOperand(result)
-  }
-
-  override Instruction getInstruction(int delta) {
-    this = TBoundOperand(result.getAUse()) and
-    delta = 0
-  } 
-
-  override string toString() { result = getOperand().toString() }
-
-  override predicate hasLocationInfo(string path, int sl, int sc, int el, int ec) {
-    getOperand().getLocation().hasLocationInfo(path, sl, sc, el, ec)
-  }
-}
+}

cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeAnalysis.qll

@@ -124,14 +124,10 @@ import RangeAnalysisPublic
  * - `isEq = true`  : `i == bound + delta`
  * - `isEq = false` : `i != bound + delta`
  */
-private IRGuardCondition eqFlowCond(Operand op, Operand bound, int delta,
+private IRGuardCondition eqFlowCond(ValueNumber vn, Operand bound, int delta,
   boolean isEq, boolean testIsTrue)
 {
-  exists(Operand compared |
-    result.ensuresEq(compared, bound, delta, op.getInstruction().getBlock(), isEq) and
-    result.controls(bound.getInstruction().getBlock(), testIsTrue) and
-    valueNumber(compared.getDefinitionInstruction()) = valueNumber (op.getDefinitionInstruction())
-  )
+  result.comparesEq(vn.getAUse(), bound, delta, isEq, testIsTrue)
 }
 
 /**
@@ -147,8 +143,9 @@ private predicate boundFlowStepSsa(
   reason = TNoReason() and
   delta = 0
   or*/
-  exists(IRGuardCondition guard |
-    guard = boundFlowCond(op2, op1, delta, upper, _) and
+  exists(IRGuardCondition guard, boolean testIsTrue |
+    guard = boundFlowCond(valueNumberOfOperand(op2), op1, delta, upper, testIsTrue) and
+    guard.controls(op2.getInstruction().getBlock(), testIsTrue) and
     reason = TCondReason(guard)
   )
 }
@@ -160,37 +157,19 @@ private predicate boundFlowStepSsa(
  * - `upper = true`  : `op <= bound + delta`
  * - `upper = false` : `op >= bound + delta`
  */
-private IRGuardCondition boundFlowCond(NonPhiOperand op, NonPhiOperand bound, int delta, boolean upper,
+private IRGuardCondition boundFlowCond(ValueNumber vn, NonPhiOperand bound, int delta, boolean upper,
   boolean testIsTrue)
 {
-  exists(Operand compared |
-    result.comparesLt(compared, bound, delta, upper, testIsTrue) and
-    result.controls(op.getInstruction().getBlock(), testIsTrue) and
-    valueNumber(compared.getDefinitionInstruction()) = valueNumber(op.getDefinitionInstruction())
-  )
-  // TODO: strengthening through modulus library
-}
-
-/**
- * Gets a condition that tests whether `op` is bounded by `bound + delta`.
- * 
- * - `upper = true`  : `op <= bound + delta`
- * - `upper = false` : `op >= bound + delta`
- */
-private IRGuardCondition boundFlowCondPhi(PhiOperand op, NonPhiOperand bound, int delta, boolean upper,
-  boolean testIsTrue)
-{
-  exists(Operand compared |
-    result.comparesLt(compared, bound, delta, upper, testIsTrue) and
-    result.controlsEdgeDirectly(op.getPredecessorBlock().getLastInstruction(), op.getInstruction().getBlock(), testIsTrue) and
-    valueNumber(compared.getDefinitionInstruction()) = valueNumber (op.getDefinitionInstruction())
+  exists(int d |
+    result.comparesLt(vn.getAUse(), bound, d, upper, testIsTrue) and
+    // strengthen from x < y to x <= y-1
+    if upper = true
+      then delta = d-1
+      else delta = d
   )
   or
-  exists(Operand compared |
-    result.comparesLt(compared, bound, delta, upper, testIsTrue) and
-    result.controls(op.getPredecessorBlock(), testIsTrue) and
-    valueNumber(compared.getDefinitionInstruction()) = valueNumber (op.getDefinitionInstruction())
-  )
+  result = eqFlowCond(vn, bound, delta, true, testIsTrue) and
+  (upper = true or upper = false)
   // TODO: strengthening through modulus library
 }
 
@@ -239,6 +218,9 @@ private predicate safeCast(IntegralType fromtyp, IntegralType totyp) {
 private class SafeCastInstruction extends ConvertInstruction {
   SafeCastInstruction() {
     safeCast(getResultType(), getOperand().getResultType())
+    or
+    getResultType() instanceof PointerType and
+    getOperand().getResultType() instanceof PointerType
   }
 }
 
@@ -402,8 +384,13 @@ private predicate boundFlowStepPhi(
   reason = TNoReason() and
   delta = 0
   or
-  exists(IRGuardCondition guard |
-    guard = boundFlowCondPhi(op2, op1, delta, upper, _) and
+  exists(IRGuardCondition guard, boolean testIsTrue |
+    guard = boundFlowCond(valueNumberOfOperand(op2), op1, delta, upper, testIsTrue) and
+    (
+      guard.hasBranchEdge(op2.getPredecessorBlock().getLastInstruction(), op2.getInstruction().getBlock(), testIsTrue)
+      or
+      guard.controls(op2.getPredecessorBlock(), testIsTrue)
+    ) and
     reason = TCondReason(guard)
   )
 }
@@ -472,12 +459,10 @@ private int weakenDelta(boolean upper, int delta) {
 
 private predicate boundedPhiInp(
   PhiInstruction phi, PhiOperand op, Bound b, int delta, boolean upper, boolean fromBackEdge,
-  int origdelta, Reason reason 
+  int origdelta, Reason reason
 ) {
   phi.getAnOperand() = op and
   exists(int d, boolean fromBackEdge0 |
-    boundedInstruction(op.getDefinitionInstruction(), b, d, upper, fromBackEdge0, origdelta, reason)
-    or
     boundedPhiOperand(op, b, d, upper, fromBackEdge0, origdelta, reason)
     or
     b.(InstructionBound).getInstruction() = op.getDefinitionInstruction() and
@@ -564,6 +549,7 @@ private predicate boundedInstruction(
     boundedPhiCandValidForEdge(i, b, delta, upper, fromBackEdge, origdelta, reason, op)
   )
   or
+  not i instanceof PhiInstruction and
   i = b.getInstruction(delta) and
   (upper = true or upper = false) and
   fromBackEdge = false and

cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeUtils.qll

@@ -44,4 +44,21 @@ predicate valueFlowStep(Instruction i, Operand op, int delta) {
     |
     delta = -getValue(getConstantValue(x.getDefinitionInstruction()))
   )
+  or
+  exists(Operand x |
+    i.(PointerAddInstruction).getAnOperand() = op and
+    i.(PointerAddInstruction).getAnOperand() = x and
+    op  != x
+    |
+    delta = i.(PointerAddInstruction).getElementSize() *
+      getValue(getConstantValue(x.getDefinitionInstruction()))
+  )
+  or
+  exists(Operand x |
+    i.(PointerSubInstruction).getAnOperand().(LeftOperand) = op and
+    i.(PointerSubInstruction).getAnOperand().(RightOperand) = x
+    |
+    delta = i.(PointerSubInstruction).getElementSize() * 
+      -getValue(getConstantValue(x.getDefinitionInstruction()))
+  )
 }