Merge pull request #14656 from aschackmull/shared/range-utils

Rangeanalysis: Share ssaRead predicate

shared/rangeanalysis/codeql/rangeanalysis/RangeAnalysis.qll

@@ -106,6 +106,8 @@ signature module Semantic {
 
   class ShiftRightUnsignedExpr extends BinaryExpr;
 
+  default predicate isAssignOp(BinaryExpr bin) { none() }
+
   class RelationalExpr extends Expr {
     Expr getLesserOperand();
 
@@ -126,9 +128,15 @@ signature module Semantic {
 
   class NegateExpr extends UnaryExpr;
 
-  class AddOneExpr extends UnaryExpr;
+  class PreIncExpr extends UnaryExpr;
 
-  class SubOneExpr extends UnaryExpr;
+  class PreDecExpr extends UnaryExpr;
+
+  class PostIncExpr extends UnaryExpr;
+
+  class PostDecExpr extends UnaryExpr;
+
+  class CopyValueExpr extends UnaryExpr;
 
   class ConditionalExpr extends Expr {
     Expr getBranchExpr(boolean branch);
@@ -168,7 +176,9 @@ signature module Semantic {
 
   class SsaPhiNode extends SsaVariable;
 
-  class SsaExplicitUpdate extends SsaVariable;
+  class SsaExplicitUpdate extends SsaVariable {
+    Expr getDefiningExpr();
+  }
 
   class SsaReadPosition {
     predicate hasReadOfVar(SsaVariable v);
@@ -240,14 +250,6 @@ signature module DeltaSig {
 }
 
 signature module LangSig<Semantic Sem, DeltaSig D> {
-  /**
-   * Holds if the specified expression should be excluded from the result of `ssaRead()`.
-   *
-   * This predicate is to keep the results identical to the original Java implementation. It should be
-   * removed once we have the new implementation matching the old results exactly.
-   */
-  predicate ignoreSsaReadCopy(Sem::Expr e);
-
   /**
    * Holds if `e >= bound` (if `upper = false`) or `e <= bound` (if `upper = true`).
    */
@@ -266,40 +268,6 @@ signature module LangSig<Semantic Sem, DeltaSig D> {
    */
   predicate ignoreExprBound(Sem::Expr e);
 
-  /**
-   * Ignore any inferred zero lower bound on this expression.
-   *
-   * This predicate is to keep the results identical to the original Java implementation. It should be
-   * removed once we have the new implementation matching the old results exactly.
-   */
-  predicate ignoreZeroLowerBound(Sem::Expr e);
-
-  /**
-   * Holds if the specified expression should be excluded from the result of `ssaRead()`.
-   *
-   * This predicate is to keep the results identical to the original Java implementation. It should be
-   * removed once we have the new implementation matching the old results exactly.
-   */
-  predicate ignoreSsaReadArithmeticExpr(Sem::Expr e);
-
-  /**
-   * Holds if the specified variable should be excluded from the result of `ssaRead()`.
-   *
-   * This predicate is to keep the results identical to the original Java implementation. It should be
-   * removed once we have the new implementation matching the old results exactly.
-   */
-  predicate ignoreSsaReadAssignment(Sem::SsaVariable v);
-
-  /**
-   * Adds additional results to `ssaRead()` that are specific to Java.
-   *
-   * This predicate handles propagation of offsets for post-increment and post-decrement expressions
-   * in exactly the same way as the old Java implementation. Once the new implementation matches the
-   * old one, we should remove this predicate and propagate deltas for all similar patterns, whether
-   * or not they come from a post-increment/decrement expression.
-   */
-  Sem::Expr specificSsaRead(Sem::SsaVariable v, D::Delta delta);
-
   /**
    * Holds if the value of `dest` is known to be `src + delta`.
    */
@@ -327,8 +295,6 @@ signature module LangSig<Semantic Sem, DeltaSig D> {
 }
 
 signature module UtilSig<Semantic Sem, DeltaSig DeltaParam> {
-  Sem::Expr semSsaRead(Sem::SsaVariable v, DeltaParam::Delta delta);
-
   Sem::Guard semEqFlowCond(
     Sem::SsaVariable v, Sem::Expr e, DeltaParam::Delta delta, boolean isEq, boolean testIsTrue
   );
@@ -379,7 +345,7 @@ signature module BoundSig<LocationSig Location, Semantic Sem, DeltaSig D> {
   class SemZeroBound extends SemBound;
 
   class SemSsaBound extends SemBound {
-    Sem::SsaVariable getAVariable();
+    Sem::SsaVariable getVariable();
   }
 }
 
@@ -399,6 +365,7 @@ module RangeStage<
   private import OverflowParam
   private import SignAnalysis
   private import ModulusAnalysis
+  private import internal.RangeUtils::MakeUtils<Sem, D>
 
   /**
    * An expression that does conversion, boxing, or unboxing
@@ -512,11 +479,11 @@ module RangeStage<
   private predicate boundCondition(
     Sem::RelationalExpr comp, Sem::SsaVariable v, Sem::Expr e, D::Delta delta, boolean upper
   ) {
-    comp.getLesserOperand() = semSsaRead(v, delta) and
+    comp.getLesserOperand() = ssaRead(v, delta) and
     e = comp.getGreaterOperand() and
     upper = true
     or
-    comp.getGreaterOperand() = semSsaRead(v, delta) and
+    comp.getGreaterOperand() = ssaRead(v, delta) and
     e = comp.getLesserOperand() and
     upper = false
     or
@@ -524,7 +491,7 @@ module RangeStage<
       // (v - d) - e < c
       comp.getLesserOperand() = sub and
       comp.getGreaterOperand() = c and
-      sub.getLeftOperand() = semSsaRead(v, d) and
+      sub.getLeftOperand() = ssaRead(v, d) and
       sub.getRightOperand() = e and
       upper = true and
       delta = D::fromFloat(D::toFloat(d) + c.getIntValue())
@@ -532,7 +499,7 @@ module RangeStage<
       // (v - d) - e > c
       comp.getGreaterOperand() = sub and
       comp.getLesserOperand() = c and
-      sub.getLeftOperand() = semSsaRead(v, d) and
+      sub.getLeftOperand() = ssaRead(v, d) and
       sub.getRightOperand() = e and
       upper = false and
       delta = D::fromFloat(D::toFloat(d) + c.getIntValue())
@@ -541,7 +508,7 @@ module RangeStage<
       comp.getLesserOperand() = sub and
       comp.getGreaterOperand() = c and
       sub.getLeftOperand() = e and
-      sub.getRightOperand() = semSsaRead(v, d) and
+      sub.getRightOperand() = ssaRead(v, d) and
       upper = false and
       delta = D::fromFloat(D::toFloat(d) - c.getIntValue())
       or
@@ -549,7 +516,7 @@ module RangeStage<
       comp.getGreaterOperand() = sub and
       comp.getLesserOperand() = c and
       sub.getLeftOperand() = e and
-      sub.getRightOperand() = semSsaRead(v, d) and
+      sub.getRightOperand() = ssaRead(v, d) and
       upper = true and
       delta = D::fromFloat(D::toFloat(d) - c.getIntValue())
     )
@@ -667,7 +634,7 @@ module RangeStage<
     Sem::SsaVariable v1, Sem::SsaVariable v2, float delta
   ) {
     exists(Sem::Guard guardEq, D::Delta d1, D::Delta d2, boolean eqIsTrue |
-      guardEq = semEqFlowCond(v1, semSsaRead(v2, d1), d2, true, eqIsTrue) and
+      guardEq = semEqFlowCond(v1, ssaRead(v2, d1), d2, true, eqIsTrue) and
       delta = D::toFloat(d2) - D::toFloat(d1) and
       guardEq.directlyControls(result, eqIsTrue)
     )
@@ -950,7 +917,7 @@ module RangeStage<
       or
       boundedPhi(inp, b, d, upper, fromBackEdge0, origdelta, reason)
       or
-      b.(SemSsaBound).getAVariable() = inp and
+      b.(SemSsaBound).getVariable() = inp and
       d = D::fromFloat(0) and
       (upper = true or upper = false) and
       fromBackEdge0 = false and
@@ -1000,7 +967,7 @@ module RangeStage<
     Sem::SsaPhiNode phi, Sem::SsaVariable inp, Sem::SsaReadPositionPhiInputEdge edge, boolean upper
   ) {
     exists(D::Delta d, SemSsaBound phibound |
-      phibound.getAVariable() = phi and
+      phibound.getVariable() = phi and
       boundedPhiInp(phi, inp, edge, phibound, d, upper, _, _, _) and
       (
         upper = true and D::toFloat(d) <= 0
@@ -1091,9 +1058,7 @@ module RangeStage<
     or
     upper = false and
     b = D::fromInt(0) and
-    semPositive(e.(Sem::BitAndExpr).getAnOperand()) and
-    // REVIEW: We let the language opt out here to preserve original results.
-    not ignoreZeroLowerBound(e)
+    semPositive(e.(Sem::BitAndExpr).getAnOperand())
   }
 
   /**
@@ -1188,12 +1153,12 @@ module RangeStage<
     positively = false and
     (
       expr instanceof Sem::NegateExpr or
-      expr instanceof Sem::SubOneExpr or
+      expr instanceof Sem::PreDecExpr or
       getTrackedType(expr.(Sem::DivExpr)) instanceof Sem::FloatingPointType
     )
     or
     positively = true and
-    expr instanceof Sem::AddOneExpr
+    expr instanceof Sem::PreIncExpr
   }
 
   /**

shared/rangeanalysis/codeql/rangeanalysis/internal/RangeUtils.qll

@@ -0,0 +1,37 @@
+private import codeql.rangeanalysis.RangeAnalysis
+
+module MakeUtils<Semantic Lang, DeltaSig D> {
+  /**
+   * Gets an expression that equals `v - d`.
+   */
+  Lang::Expr ssaRead(Lang::SsaVariable v, D::Delta delta) {
+    result = v.getAUse() and delta = D::fromInt(0)
+    or
+    exists(D::Delta d1, Lang::ConstantIntegerExpr c |
+      result.(Lang::AddExpr).hasOperands(ssaRead(v, d1), c) and
+      delta = D::fromFloat(D::toFloat(d1) - c.getIntValue()) and
+      // In the scope of `x += ..`, which is SSA translated as `x2 = x1 + ..`,
+      // the variable `x1` is shadowed by `x2`, so there's no need to view this
+      // as a read of `x1`.
+      not Lang::isAssignOp(result)
+    )
+    or
+    exists(Lang::SubExpr sub, D::Delta d1, Lang::ConstantIntegerExpr c |
+      result = sub and
+      sub.getLeftOperand() = ssaRead(v, d1) and
+      sub.getRightOperand() = c and
+      delta = D::fromFloat(D::toFloat(d1) + c.getIntValue()) and
+      not Lang::isAssignOp(result)
+    )
+    or
+    result = v.(Lang::SsaExplicitUpdate).getDefiningExpr() and
+    if result instanceof Lang::PostIncExpr
+    then delta = D::fromFloat(1) // x++ === ++x - 1
+    else
+      if result instanceof Lang::PostDecExpr
+      then delta = D::fromFloat(-1) // x-- === --x + 1
+      else delta = D::fromFloat(0)
+    or
+    result.(Lang::CopyValueExpr).getOperand() = ssaRead(v, delta)
+  }
+}