C++: respond to PR comments, add some TODOs

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

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

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

@@ -55,7 +55,7 @@ class InstructionBound extends Bound, TBoundInstruction {
 }
 
 /**
- * A bound corrseponding to the value of an `Operand`.
+ * A bound corresponding to the value of an `Operand`.
  */
 class OperandBound extends Bound, TBoundOperand {
   Operand getOperand() {
@@ -72,4 +72,4 @@ class OperandBound extends Bound, TBoundOperand {
   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

@@ -1,14 +1,14 @@
 /**
  * Provides classes and predicates for range analysis.
  *
- * An inferred bound can either be a specific integer, the abstract value of an
- * SSA variable, or the abstract value of an interesting expression. The latter
- * category includes array lengths that are not SSA variables.
+ * An inferred bound can either be a specific integer or the abstract value of
+ * an IR `Instruction`.
  *
  * If an inferred bound relies directly on a condition, then this condition is
  * reported as the reason for the bound.
  */
 
+// TODO: update the following comment
 /*
  * This library tackles range analysis as a flow problem. Consider e.g.:
  * ```
@@ -118,7 +118,7 @@ private import RangeAnalysisCache
 import RangeAnalysisPublic
 
 /**
- * Gets a condition that tests whether `i` equals `bound + delta` at `use`.
+ * Gets a condition that tests whether `op` equals `bound + delta`.
  *
  * If the condition evaluates to `testIsTrue`:
  * - `isEq = true`  : `i == bound + delta`
@@ -142,11 +142,11 @@ private IRGuardCondition eqFlowCond(Operand op, Operand bound, int delta,
 private predicate boundFlowStepSsa(
   NonPhiOperand op2, Operand op1, int delta, boolean upper, Reason reason
 ) {
-  op2.getDefinitionInstruction().getAnOperand().(CopySourceOperand) = op1 and
+  /*op2.getDefinitionInstruction().getAnOperand().(CopySourceOperand) = op1 and
   (upper = true or upper = false) and
   reason = TNoReason() and
   delta = 0
-  or
+  or*/
   exists(IRGuardCondition guard |
     guard = boundFlowCond(op2, op1, delta, upper, _) and
     reason = TCondReason(guard)
@@ -182,7 +182,7 @@ private IRGuardCondition boundFlowCondPhi(PhiOperand op, NonPhiOperand bound, in
 {
   exists(Operand compared |
     result.comparesLt(compared, bound, delta, upper, testIsTrue) and
-    result.controlsEdge(op.getPredecessorBlock().getLastInstruction(), op.getInstruction().getBlock(), testIsTrue) and
+    result.controlsEdgeDirectly(op.getPredecessorBlock().getLastInstruction(), op.getInstruction().getBlock(), testIsTrue) and
     valueNumber(compared.getDefinitionInstruction()) = valueNumber (op.getDefinitionInstruction())
   )
   or
@@ -219,12 +219,21 @@ class CondReason extends Reason, TCondReason {
  * range analysis.
  */
 private predicate safeCast(IntegralType fromtyp, IntegralType totyp) {
-  fromtyp.getSize() <= totyp.getSize() and
+  fromtyp.getSize() < totyp.getSize() and
   (
     fromtyp.isUnsigned()
     or
     totyp.isSigned()
+  ) or
+  fromtyp.getSize() <= totyp.getSize() and
+  (
+    fromtyp.isSigned() and
+    totyp.isSigned()
+    or
+    fromtyp.isUnsigned() and
+    totyp.isUnsigned()
   )
+  // TODO: infer safety using sign analysis?
 }
 
 private class SafeCastInstruction extends ConvertInstruction {
@@ -349,15 +358,17 @@ private predicate boundFlowStepDiv(Instruction i1, Operand op, int factor) {
   )
 }
 
+/**
+ * Holds if `b` is a valid bound for `op`
+ */
 pragma[noinline]
 private predicate boundedNonPhiOperand(NonPhiOperand op, Bound b, int delta, boolean upper,
   boolean fromBackEdge, int origdelta, Reason reason
 ) {
-  exists(NonPhiOperand op2, int d1, int d2, Reason r1, Reason r2 |
-    boundFlowStepSsa(op, op2, d1, upper, r1) and
-    boundedNonPhiOperand(op2, b, d2, upper, fromBackEdge, origdelta, r2) and
-    delta = d1 + d2 and
-    (if r1 instanceof NoReason then reason = r2 else reason = r1)
+  exists(NonPhiOperand op2, int d1, int d2 |
+    boundFlowStepSsa(op, op2, d1, upper, reason) and
+    boundedNonPhiOperand(op2, b, d2, upper, fromBackEdge, origdelta, _) and
+    delta = d1 + d2
   )
   or
   boundedInstruction(op.getDefinitionInstruction(), b, delta, upper, fromBackEdge, origdelta, reason)
@@ -402,7 +413,7 @@ private predicate boundedPhiOperand(
   PhiOperand op, Bound b, int delta, boolean upper, boolean fromBackEdge, int origdelta,
   Reason reason
 ) {
-  exists(NonPhiOperand op2, int d1, int d2, Reason r1, Reason r2 | // should mid be an `Operand`?
+  exists(NonPhiOperand op2, int d1, int d2, Reason r1, Reason r2 |
     boundFlowStepPhi(op, op2, d1, upper, r1) and
     boundedNonPhiOperand(op2, b, d2, upper, fromBackEdge, origdelta, r2) and
     delta = d1 + d2 and
@@ -431,6 +442,7 @@ private predicate boundedPhiOperand(
  * Holds if `op != b + delta` at `pos`.
  */
 private predicate unequalOperand(Operand op, Bound b, int delta, Reason reason) {
+  // TODO: implement this
   none()
 }
 
@@ -588,3 +600,9 @@ private predicate boundedInstruction(
     boundedCastExpr(cast, b, delta, upper, fromBackEdge, origdelta, reason)
   )
 }
+
+predicate backEdge(PhiInstruction phi, PhiOperand op) {
+  phi.getAnOperand() = op and
+  phi.getBlock().dominates(op.getPredecessorBlock())
+  // TODO: identify backedges during IR construction
+}

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

@@ -45,10 +45,3 @@ predicate valueFlowStep(Instruction i, Operand op, int delta) {
     delta = -getValue(getConstantValue(x.getDefinitionInstruction()))
   )
 }
-
-predicate backEdge(PhiInstruction phi, PhiOperand op) {
-  phi.getAnOperand() = op and
-  (
-    phi.getBlock().dominates(op.getPredecessorBlock())
-  )
-}

cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/RangeAnalysis.ql

@@ -3,10 +3,14 @@ import semmle.code.cpp.ir.IR
 import semmle.code.cpp.controlflow.IRGuards
 import semmle.code.cpp.ir.ValueNumbering
 
-query predicate instructionBounds(Instruction i, Bound b, int delta, boolean upper, Reason reason) {
-  boundedInstruction(i, b, delta, upper, reason)
-}
-
-query predicate operandBounds(Operand op, Bound b, int delta, boolean upper, Reason reason) {
-  boundedOperand(op, b, delta, upper, reason)
+query predicate instructionBounds(Instruction i, Bound b, int delta, boolean upper, Reason reason) 
+{
+  i instanceof LoadInstruction and
+  boundedInstruction(i, b, delta, upper, reason) and
+  (
+    b.(InstructionBound).getInstruction() instanceof InitializeParameterInstruction or
+    b.(InstructionBound).getInstruction() instanceof CallInstruction or
+    b instanceof ZeroBound
+  ) and
+  not valueNumber(b.(InstructionBound).getInstruction()) = valueNumber(i)
 }

cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/test.cpp

@@ -1,4 +1,7 @@
 
+void sink(...);
+int source();
+
 // Guards, inference, critical edges
 int test1(int x, int y) {
   if (x < y) {
@@ -18,12 +21,12 @@ int test2(int x, int y) {
 }
 
 // for loops
-int test3(int x, void *p) {
+int test3(int x, int *p) {
   int i;
   for(i = 0; i < x; i++) {
     p[i];
   }
-  for(i = x; i > 0; i++) {
+  for(i = x; i > 0; i--) {
     p[i];
   }
 }
@@ -36,3 +39,15 @@ int test4(int *begin, int *end) {
   }
 }
 
+int test5(int x, int y, int z) {
+  if (y < z) {
+    if (x < y) {
+      sink(x);
+    }
+  }
+  if (x < y) {
+    if (y < z) {
+      sink(x); // x < z is not inferred here
+    }
+  }
+}