C++: performance and termination fixes

Author: Robert Marsh

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

@@ -216,7 +216,7 @@ private predicate uniqueValueNumber(Instruction instr, FunctionIR funcIR) {
 /**
  * Gets the value number assigned to `instr`, if any. Returns at most one result.
  */
-ValueNumber valueNumber(Instruction instr) {
+cached ValueNumber valueNumber(Instruction instr) {
   result = nonUniqueValueNumber(instr) or
   exists(FunctionIR funcIR |
     uniqueValueNumber(instr, funcIR) and

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

@@ -10,7 +10,8 @@ private newtype TBound =
       (
         i.getResultType() instanceof IntegralType or
         i.getResultType() instanceof PointerType
-      )
+      ) and
+      not vn.getAnInstruction() instanceof ConstantInstruction
     |
       i instanceof PhiInstruction
       or

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

@@ -197,6 +197,7 @@ class CondReason extends Reason, TCondReason {
  * Holds if a cast from `fromtyp` to `totyp` can be ignored for the purpose of
  * range analysis.
  */
+pragma[inline]
 private predicate safeCast(IntegralType fromtyp, IntegralType totyp) {
   fromtyp.getSize() < totyp.getSize() and
   (
@@ -256,7 +257,7 @@ private class NarrowingCastInstruction extends ConvertInstruction {
  * - `upper = true`  : `i <= op + delta`
  * - `upper = false` : `i >= op + delta`
  */
-private predicate boundFlowStep(Instruction i, Operand op, int delta, boolean upper) {
+private predicate boundFlowStep(Instruction i, NonPhiOperand op, int delta, boolean upper) {
   valueFlowStep(i, op, delta) and
   (upper = true or upper = false)
   or
@@ -425,12 +426,27 @@ private predicate boundedPhiOperand(
   )
 }
 
+/** Holds if `v != e + delta` at `pos`. */
+private predicate unequalFlowStep(
+ Operand op1, Operand op2, int delta, Reason reason
+) {
+  exists(IRGuardCondition guard, boolean testIsTrue |
+    guard = eqFlowCond(valueNumberOfOperand(op1), op2, delta, false, testIsTrue) and
+    guard.controls(op1.getInstruction().getBlock(), testIsTrue) and
+    reason = TCondReason(guard)
+  )
+}
+
 /**
  * Holds if `op != b + delta` at `pos`.
  */
 private predicate unequalOperand(Operand op, Bound b, int delta, Reason reason) {
-  // TODO: implement this
-  none()
+  exists(Operand op2, int d1, int d2 |
+    unequalFlowStep(op, op2, delta, reason) and
+    boundedNonPhiOperand(op2, b, d2, true, _, _, _) and
+    boundedNonPhiOperand(op2, b, d2, true, _, _, _) and
+    delta = d1 + d2
+  )
 }
 
 private predicate boundedPhiCandValidForEdge(
@@ -544,50 +560,47 @@ private predicate boundedCastExpr(
 private predicate boundedInstruction(
   Instruction i, Bound b, int delta, boolean upper, boolean fromBackEdge, int origdelta, Reason reason
 ) {
-  i instanceof PhiInstruction and
-  forex(PhiOperand op | op = i.getAnOperand() |
-    boundedPhiCandValidForEdge(i, b, delta, upper, fromBackEdge, origdelta, reason, op)
-  )
-  or
-  i = b.getInstruction(delta) and
-  (upper = true or upper = false) and
-  fromBackEdge = false and
-  origdelta = delta and
-  reason = TNoReason()
-  or
-  exists(Operand mid, int d1, int d2 |
-    boundFlowStep(i, mid, d1, upper) and
-    boundedNonPhiOperand(mid, b, d2, upper, fromBackEdge, origdelta, reason) and
-    delta = d1 + d2 and
-    not exists(getValue(getConstantValue(i)))
-  )
-  or
-  exists(Operand mid, int factor, int d |
-    boundFlowStepMul(i, mid, factor) and
-    boundedNonPhiOperand(mid, b, d, upper, fromBackEdge, origdelta, reason) and
-    b instanceof ZeroBound and
-    delta = d*factor and
-    not exists(getValue(getConstantValue(i)))
-  )
-  or
-  exists(Operand mid, int factor, int d |
-    boundFlowStepDiv(i, mid, factor) and
-    boundedNonPhiOperand(mid, b, d, upper, fromBackEdge, origdelta, reason) and
-    d >= 0 and
-    b instanceof ZeroBound and
-    delta = d / factor and
-    not exists(getValue(getConstantValue(i)))
-  )
-  or
-  exists(NarrowingCastInstruction cast |
-    cast = i and
-    safeNarrowingCast(cast, upper.booleanNot()) and
-    boundedCastExpr(cast, b, delta, upper, fromBackEdge, origdelta, reason)
+  isReducibleCFG(i.getFunction()) and
+  (
+    i instanceof PhiInstruction and
+    forex(PhiOperand op | op = i.getAnOperand() |
+      boundedPhiCandValidForEdge(i, b, delta, upper, fromBackEdge, origdelta, reason, op)
+    )
+    or
+    i = b.getInstruction(delta) and
+    (upper = true or upper = false) and
+    fromBackEdge = false and
+    origdelta = delta and
+    reason = TNoReason()
+    or
+    exists(Operand mid, int d1, int d2 |
+      boundFlowStep(i, mid, d1, upper) and
+      boundedNonPhiOperand(mid, b, d2, upper, fromBackEdge, origdelta, reason) and
+      delta = d1 + d2 and
+      not exists(getValue(getConstantValue(i)))
+    )
+    or
+    exists(Operand mid, int factor, int d |
+      boundFlowStepMul(i, mid, factor) and
+      boundedNonPhiOperand(mid, b, d, upper, fromBackEdge, origdelta, reason) and
+      b instanceof ZeroBound and
+      delta = d*factor and
+      not exists(getValue(getConstantValue(i)))
+    )
+    or
+    exists(Operand mid, int factor, int d |
+      boundFlowStepDiv(i, mid, factor) and
+      boundedNonPhiOperand(mid, b, d, upper, fromBackEdge, origdelta, reason) and
+      d >= 0 and
+      b instanceof ZeroBound and
+      delta = d / factor and
+      not exists(getValue(getConstantValue(i)))
+    )
+    or
+    exists(NarrowingCastInstruction cast |
+      cast = i and
+      safeNarrowingCast(cast, upper.booleanNot()) and
+      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

@@ -62,3 +62,25 @@ predicate valueFlowStep(Instruction i, Operand op, int delta) {
       -getValue(getConstantValue(x.getDefinitionInstruction()))
   )
 }
+
+predicate isReducibleCFG(Function f) {
+  not exists(LabelStmt l, GotoStmt goto |
+    goto.getTarget() = l and
+    l.getLocation().isBefore(goto.getLocation()) and
+    l.getEnclosingFunction() = f
+  ) and
+  not exists(LabelStmt ls, Loop l |
+    ls.getParent*() = l and
+    l.getEnclosingFunction() = f
+  ) and
+  not exists(SwitchCase cs |
+    cs.getSwitchStmt().getStmt() != cs.getParentStmt() and
+    cs.getEnclosingFunction() = f
+  )
+}
+
+predicate backEdge(PhiInstruction phi, PhiOperand op) {
+  phi.getAnOperand() = op and
+  phi.getBlock().dominates(op.getPredecessorBlock())
+  // TODO: identify backedges during IR construction
+}

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

@@ -35,6 +35,11 @@
 | test.cpp:97:10:97:10 | Load: x | file://:0:0:0:0 | 0 | 1 | false | CompareLT: ... < ... | test.cpp:94:7:94:11 | test.cpp:94:7:94:11 |
 | test.cpp:100:10:100:10 | Load: x | file://:0:0:0:0 | 0 | 1 | true | CompareLE: ... <= ... | test.cpp:99:7:99:12 | test.cpp:99:7:99:12 |
 | test.cpp:102:10:102:10 | Load: x | file://:0:0:0:0 | 0 | 2 | false | CompareLE: ... <= ... | test.cpp:99:7:99:12 | test.cpp:99:7:99:12 |
-| test.cpp:106:5:106:10 | Phi: test10 | test.cpp:113:3:113:6 | Phi: call to sink | -1 | true | CompareLT: ... < ... | test.cpp:114:18:114:22 | test.cpp:114:18:114:22 |
-| test.cpp:106:5:106:10 | Phi: test10 | test.cpp:114:18:114:18 | Phi: i | 0 | false | NoReason | file://:0:0:0:0 | file://:0:0:0:0 |
-| test.cpp:106:5:106:10 | Phi: test10 | test.cpp:114:18:114:18 | Phi: i | 0 | true | NoReason | file://:0:0:0:0 | file://:0:0:0:0 |
+| test.cpp:107:5:107:10 | Phi: test10 | test.cpp:114:3:114:6 | Phi: call to sink | -1 | true | CompareLT: ... < ... | test.cpp:115:18:115:22 | test.cpp:115:18:115:22 |
+| test.cpp:140:10:140:10 | Store: i | file://:0:0:0:0 | 0 | 1 | false | NoReason | file://:0:0:0:0 | file://:0:0:0:0 |
+| test.cpp:140:10:140:10 | Store: i | test.cpp:135:16:135:16 | InitializeParameter: x | 0 | false | CompareLT: ... < ... | test.cpp:139:11:139:15 | test.cpp:139:11:139:15 |
+| test.cpp:140:10:140:10 | Store: i | test.cpp:138:5:138:5 | Phi: i | 1 | false | NoReason | file://:0:0:0:0 | file://:0:0:0:0 |
+| test.cpp:140:10:140:10 | Store: i | test.cpp:138:5:138:5 | Phi: i | 1 | true | NoReason | file://:0:0:0:0 | file://:0:0:0:0 |
+| test.cpp:149:10:149:10 | Store: i | file://:0:0:0:0 | 0 | 1 | false | NoReason | file://:0:0:0:0 | file://:0:0:0:0 |
+| test.cpp:149:10:149:10 | Store: i | test.cpp:147:5:147:5 | Phi: i | 1 | false | NoReason | file://:0:0:0:0 | file://:0:0:0:0 |
+| test.cpp:149:10:149:10 | Store: i | test.cpp:147:5:147:5 | Phi: i | 1 | true | NoReason | file://:0:0:0:0 | file://:0:0:0:0 |

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

@@ -103,6 +103,7 @@ void test9(int x) {
   }
 }
 
+// Phi nodes as bounds
 int test10(int y, int z, bool use_y) {
   int x;
   if(use_y) {
@@ -115,3 +116,35 @@ int test10(int y, int z, bool use_y) {
     return i;
   }
 }
+
+// Irreducible CFGs
+int test11(int y, int x) {
+  int i = 0;
+  if (x < y) {
+    x = y;
+  } else {
+    goto inLoop;
+  }
+  for(i = 0; i < x; i++) {
+    inLoop:
+    sink(i);
+  }
+}
+
+// do-while
+int test12(int x) {
+  int i = 0;
+  do {
+    i++;
+  } while(i < x);
+  return i;
+}
+
+// do while false
+int test13(int x) {
+  int i = 0;
+  do {
+    i++;
+  } while(false);
+  return i;
+}