C++: SimpleRangeAnalysis for MulExpr by constant

Author: jbj

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

@@ -156,8 +156,65 @@ float safeFloor(float v) {
   result = v
 }
 
+/** A `MulExpr` where exactly one operand is constant. */
+private class MulByConstantExpr extends MulExpr {
+  float constant;
+  Expr operand;
+
+  MulByConstantExpr() {
+    exists(Expr constantExpr |
+      this.hasOperands(constantExpr, operand) and
+      constant = constantExpr.getFullyConverted().getValue().toFloat() and
+      not exists(operand.getFullyConverted().getValue().toFloat())
+    )
+  }
+
+  /** Gets the value of the constant operand. */
+  float getConstant() { result = constant }
+
+  /** Gets the non-constant operand. */
+  Expr getOperand() { result = operand }
+}
+
 private class UnsignedMulExpr extends MulExpr {
-  UnsignedMulExpr() { this.getType().(IntegralType).isUnsigned() }
+  UnsignedMulExpr() {
+    this.getType().(IntegralType).isUnsigned() and
+    // Avoid overlap. It should be slightly cheaper to analyze
+    // `MulByConstantExpr`.
+    not this instanceof MulByConstantExpr
+  }
+}
+
+/**
+ * Holds if `expr` is effectively a multiplication of `operand` with the
+ * positive constant `positive`.
+ */
+private predicate multipliesByPositive(Expr expr, Expr operand, float positive) {
+  operand = expr.(MulByConstantExpr).getOperand() and
+  positive = expr.(MulByConstantExpr).getConstant() and
+  positive >= 0.0 // includes positive zero
+  or
+  operand = expr.(UnaryPlusExpr).getOperand() and
+  positive = 1.0
+  or
+  operand = expr.(CommaExpr).getRightOperand() and
+  positive = 1.0
+  or
+  operand = expr.(StmtExpr).getResultExpr() and
+  positive = 1.0
+}
+
+/**
+ * Holds if `expr` is effectively a multiplication of `operand` with the
+ * negative constant `negative`.
+ */
+private predicate multipliesByNegative(Expr expr, Expr operand, float negative) {
+  operand = expr.(MulByConstantExpr).getOperand() and
+  negative = expr.(MulByConstantExpr).getConstant() and
+  negative < 0.0 // includes negative zero
+  or
+  operand = expr.(UnaryMinusExpr).getOperand() and
+  negative = -1.0
 }
 
 private class UnsignedAssignMulExpr extends AssignMulExpr {
@@ -172,9 +229,9 @@ private predicate analyzableExpr(Expr e) {
   (
     exists(getValue(e).toFloat())
     or
-    e instanceof UnaryPlusExpr
+    multipliesByPositive(e, _, _)
     or
-    e instanceof UnaryMinusExpr
+    multipliesByNegative(e, _, _)
     or
     e instanceof MinExpr
     or
@@ -200,10 +257,6 @@ private predicate analyzableExpr(Expr e) {
     or
     e instanceof RemExpr
     or
-    e instanceof CommaExpr
-    or
-    e instanceof StmtExpr
-    or
     // A conversion is analyzable, provided that its child has an arithmetic
     // type. (Sometimes the child is a reference type, and so does not get
     // any bounds.) Rather than checking whether the type of the child is
@@ -272,12 +325,14 @@ private predicate defDependsOnDef(
  * the structure of `getLowerBoundsImpl` and `getUpperBoundsImpl`.
  */
 private predicate exprDependsOnDef(Expr e, RangeSsaDefinition srcDef, StackVariable srcVar) {
-  exists(UnaryMinusExpr negateExpr | e = negateExpr |
-    exprDependsOnDef(negateExpr.getOperand(), srcDef, srcVar)
+  exists(Expr operand |
+    multipliesByNegative(e, operand, _) and
+    exprDependsOnDef(operand, srcDef, srcVar)
   )
   or
-  exists(UnaryPlusExpr plusExpr | e = plusExpr |
-    exprDependsOnDef(plusExpr.getOperand(), srcDef, srcVar)
+  exists(Expr operand |
+    multipliesByPositive(e, operand, _) and
+    exprDependsOnDef(operand, srcDef, srcVar)
   )
   or
   exists(MinExpr minExpr | e = minExpr | exprDependsOnDef(minExpr.getAnOperand(), srcDef, srcVar))
@@ -316,14 +371,6 @@ private predicate exprDependsOnDef(Expr e, RangeSsaDefinition srcDef, StackVaria
   or
   exists(RemExpr remExpr | e = remExpr | exprDependsOnDef(remExpr.getAnOperand(), srcDef, srcVar))
   or
-  exists(CommaExpr commaExpr | e = commaExpr |
-    exprDependsOnDef(commaExpr.getRightOperand(), srcDef, srcVar)
-  )
-  or
-  exists(StmtExpr stmtExpr | e = stmtExpr |
-    exprDependsOnDef(stmtExpr.getResultExpr(), srcDef, srcVar)
-  )
-  or
   exists(Conversion convExpr | e = convExpr | exprDependsOnDef(convExpr.getExpr(), srcDef, srcVar))
   or
   // unsigned `&`
@@ -592,15 +639,16 @@ deprecated predicate positive_overflow(Expr expr) { exprMightOverflowPositively(
 
 /** Only to be called by `getTruncatedLowerBounds`. */
 private float getLowerBoundsImpl(Expr expr) {
-  exists(UnaryPlusExpr plusExpr |
-    expr = plusExpr and
-    result = getFullyConvertedLowerBounds(plusExpr.getOperand())
+  exists(Expr operand, float operandLow, float positive |
+    multipliesByPositive(expr, operand, positive) and
+    operandLow = getFullyConvertedLowerBounds(operand) and
+    result = positive * operandLow
   )
   or
-  exists(UnaryMinusExpr negateExpr, float xHigh |
-    expr = negateExpr and
-    xHigh = getFullyConvertedUpperBounds(negateExpr.getOperand()) and
-    result = -xHigh
+  exists(Expr operand, float operandHigh, float negative |
+    multipliesByNegative(expr, operand, negative) and
+    operandHigh = getFullyConvertedUpperBounds(operand) and
+    result = negative * operandHigh
   )
   or
   exists(MinExpr minExpr |
@@ -732,16 +780,6 @@ private float getLowerBoundsImpl(Expr expr) {
     )
   )
   or
-  exists(CommaExpr commaExpr |
-    expr = commaExpr and
-    result = getFullyConvertedLowerBounds(commaExpr.getRightOperand())
-  )
-  or
-  exists(StmtExpr stmtExpr |
-    expr = stmtExpr and
-    result = getFullyConvertedLowerBounds(stmtExpr.getResultExpr())
-  )
-  or
   // If the conversion is to an arithmetic type then we just return the
   // lower bound of the child. We do not need to handle truncation and
   // overflow here, because that is done in `getTruncatedLowerBounds`.
@@ -775,15 +813,16 @@ private float getLowerBoundsImpl(Expr expr) {
 
 /** Only to be called by `getTruncatedUpperBounds`. */
 private float getUpperBoundsImpl(Expr expr) {
-  exists(UnaryPlusExpr plusExpr |
-    expr = plusExpr and
-    result = getFullyConvertedUpperBounds(plusExpr.getOperand())
+  exists(Expr operand, float operandHigh, float positive |
+    multipliesByPositive(expr, operand, positive) and
+    operandHigh = getFullyConvertedUpperBounds(operand) and
+    result = positive * operandHigh
   )
   or
-  exists(UnaryMinusExpr negateExpr, float xLow |
-    expr = negateExpr and
-    xLow = getFullyConvertedLowerBounds(negateExpr.getOperand()) and
-    result = -xLow
+  exists(Expr operand, float operandLow, float negative |
+    multipliesByNegative(expr, operand, negative) and
+    operandLow = getFullyConvertedLowerBounds(operand) and
+    result = negative * operandLow
   )
   or
   exists(MaxExpr maxExpr |
@@ -913,16 +952,6 @@ private float getUpperBoundsImpl(Expr expr) {
     )
   )
   or
-  exists(CommaExpr commaExpr |
-    expr = commaExpr and
-    result = getFullyConvertedUpperBounds(commaExpr.getRightOperand())
-  )
-  or
-  exists(StmtExpr stmtExpr |
-    expr = stmtExpr and
-    result = getFullyConvertedUpperBounds(stmtExpr.getResultExpr())
-  )
-  or
   // If the conversion is to an arithmetic type then we just return the
   // upper bound of the child. We do not need to handle truncation and
   // overflow here, because that is done in `getTruncatedUpperBounds`.

cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/lowerBound.expected

@@ -510,6 +510,25 @@
 | test.c:504:3:504:9 | ulconst | 10 |
 | test.c:505:10:505:16 | uiconst | 40 |
 | test.c:505:20:505:26 | ulconst | 40 |
+| test.c:509:7:509:7 | i | -2147483648 |
+| test.c:509:18:509:18 | i | -1 |
+| test.c:510:5:510:5 | i | -2147483648 |
+| test.c:510:13:510:13 | i | -1 |
+| test.c:511:9:511:9 | i | -5 |
+| test.c:513:5:513:5 | i | -2147483648 |
+| test.c:513:9:513:9 | i | -5 |
+| test.c:514:9:514:9 | i | -30 |
+| test.c:516:5:516:5 | i | -30 |
+| test.c:517:9:517:9 | i | -2147483648 |
+| test.c:519:5:519:5 | i | -2147483648 |
+| test.c:520:9:520:9 | i | -2147483648 |
+| test.c:522:7:522:7 | i | -2147483648 |
+| test.c:523:5:523:5 | i | -2147483648 |
+| test.c:523:9:523:9 | i | -1 |
+| test.c:524:9:524:9 | i | 1 |
+| test.c:526:3:526:3 | i | -2147483648 |
+| test.c:526:7:526:7 | i | -2147483648 |
+| test.c:527:10:527:10 | i | -2147483648 |
 | test.cpp:10:7:10:7 | b | -2147483648 |
 | test.cpp:11:5:11:5 | x | -2147483648 |
 | test.cpp:13:10:13:10 | x | -2147483648 |

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

@@ -504,3 +504,25 @@ unsigned long mul_assign(unsigned int ui) {
   ulconst *= 4;
   return uiconst + ulconst; // 40 .. 40 for both
 }
+
+int mul_by_constant(int i, int j) {
+  if (i >= -1 && i <= 2) {
+    i = 5 * i;
+    out(i); // -5 .. 10
+
+    i = i * -3;
+    out(i); // -30 .. 15
+
+    i *= 7;
+    out(i); // -210 .. 105 [BUG: not supported]
+
+    i *= -11;
+    out(i); // -1155 .. 2310 [BUG: not supported]
+  }
+  if (i == -1) {
+    i = i * (int)0xffFFffFF; // fully converted literal is -1
+    out(i); // 1 .. 1
+  }
+  i = i * -1;
+  return i; // -2^31 .. 2^31-1
+}

cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/upperBound.expected

@@ -510,6 +510,25 @@
 | test.c:504:3:504:9 | ulconst | 10 |
 | test.c:505:10:505:16 | uiconst | 40 |
 | test.c:505:20:505:26 | ulconst | 40 |
+| test.c:509:7:509:7 | i | 2147483647 |
+| test.c:509:18:509:18 | i | 2147483647 |
+| test.c:510:5:510:5 | i | 2147483647 |
+| test.c:510:13:510:13 | i | 2 |
+| test.c:511:9:511:9 | i | 10 |
+| test.c:513:5:513:5 | i | 2147483647 |
+| test.c:513:9:513:9 | i | 10 |
+| test.c:514:9:514:9 | i | 15 |
+| test.c:516:5:516:5 | i | 15 |
+| test.c:517:9:517:9 | i | 2147483647 |
+| test.c:519:5:519:5 | i | 2147483647 |
+| test.c:520:9:520:9 | i | 2147483647 |
+| test.c:522:7:522:7 | i | 2147483647 |
+| test.c:523:5:523:5 | i | 2147483647 |
+| test.c:523:9:523:9 | i | -1 |
+| test.c:524:9:524:9 | i | 1 |
+| test.c:526:3:526:3 | i | 2147483647 |
+| test.c:526:7:526:7 | i | 2147483647 |
+| test.c:527:10:527:10 | i | 2147483647 |
 | test.cpp:10:7:10:7 | b | 2147483647 |
 | test.cpp:11:5:11:5 | x | 2147483647 |
 | test.cpp:13:10:13:10 | x | 2147483647 |