Brodes/guard flow parsing k

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

@@ -5,9 +5,54 @@
 
 import cpp
 import semmle.code.cpp.ir.IR
+private import semmle.code.cpp.ir.ValueNumbering
 private import semmle.code.cpp.ir.implementation.raw.internal.TranslatedExpr
 private import semmle.code.cpp.ir.implementation.raw.internal.InstructionTag
 
+/**
+ * Variant of valueNumber that accounts for conversions.
+ * Note: this predicate is defined in IRGuards to take advantage of
+ * IRGuards definition of int_value.
+ */
+ValueNumber valueNumberWithConversions(Instruction instr) {
+  exists(ValueNumber vn | vn = valueNumber(instr) |
+    // GVN using valueNumber directly
+    result = vn
+    or
+    // GVN for the ConvertInstruction conversion
+    result = valueNumberWithConversions(vn.getAnInstruction().(ConvertInstruction).getUnary())
+    or
+    // GVN for the CompareNEInstruction conversion for a comparison with 0 (conversion of value to boolean in c)
+    exists(CompareNEInstruction cmp | cmp = vn.getAnInstruction() |
+      result = valueNumberWithConversions(cmp.getLeft()) and
+      int_value(cmp.getRight()) = 0
+    )
+  )
+}
+
+/**
+ * Returns `instr` or any instruction used to define `instr`.
+ */
+pragma[inline]
+private Instruction getDerivedInstruction(Instruction instr) {
+  result = instr
+  or
+  result = valueNumber(instr).getAnInstruction().(StoreInstruction).getSourceValue()
+  or
+  result = derivedThroughConversion(valueNumber(instr))
+}
+
+private Instruction derivedThroughConversion(ValueNumber vn) {
+  // GVN for the ConvertInstruction conversion
+  result = getDerivedInstruction(vn.getAnInstruction().(ConvertInstruction).getUnary())
+  or
+  // GVN for the CompareNEInstruction conversion for a comparison with 0 (conversion of value to boolean in c)
+  exists(CompareNEInstruction comp | comp = vn.getAnInstruction() |
+    result = getDerivedInstruction(comp.getLeft()) and
+    int_value(comp.getRight()) = 0
+  )
+}
+
 /**
  * Holds if `block` consists of an `UnreachedInstruction`.
  *
@@ -538,7 +583,8 @@ class IRGuardCondition extends Instruction {
   cached
   predicate ensuresLt(Operand left, Operand right, int k, IRBlock block, boolean isLessThan) {
     exists(AbstractValue value |
-      compares_lt(this, left, right, k, isLessThan, value) and this.valueControls(block, value)
+      compares_lt(this, left, right, k, isLessThan, value) and
+      this.valueControls(block, value)
     )
   }
 
@@ -549,7 +595,8 @@ class IRGuardCondition extends Instruction {
   cached
   predicate ensuresLt(Operand op, int k, IRBlock block, boolean isLessThan) {
     exists(AbstractValue value |
-      compares_lt(this, op, k, isLessThan, value) and this.valueControls(block, value)
+      compares_lt(this, op, k, isLessThan, value) and
+      this.valueControls(block, value)
     )
   }
 
@@ -601,7 +648,8 @@ class IRGuardCondition extends Instruction {
   cached
   predicate ensuresEq(Operand left, Operand right, int k, IRBlock block, boolean areEqual) {
     exists(AbstractValue value |
-      compares_eq(this, left, right, k, areEqual, value) and this.valueControls(block, value)
+      compares_eq(this, left, right, k, areEqual, value) and
+      this.valueControls(block, value)
     )
   }
 
@@ -612,7 +660,8 @@ class IRGuardCondition extends Instruction {
   cached
   predicate ensuresEq(Operand op, int k, IRBlock block, boolean areEqual) {
     exists(AbstractValue value |
-      unary_compares_eq(this, op, k, areEqual, false, value) and this.valueControls(block, value)
+      unary_compares_eq(this, op, k, areEqual, false, value) and
+      this.valueControls(block, value)
     )
   }
 
@@ -742,27 +791,30 @@ private Instruction getBranchForCondition(Instruction guard) {
 private predicate compares_eq(
   Instruction test, Operand left, Operand right, int k, boolean areEqual, AbstractValue value
 ) {
-  /* The simple case where the test *is* the comparison so areEqual = testIsTrue xor eq. */
-  exists(AbstractValue v | simple_comparison_eq(test, left, right, k, v) |
-    areEqual = true and value = v
+  exists(Instruction derived | derived = getDerivedInstruction(test) |
+    /* The simple case where the test *is* the comparison so areEqual = testIsTrue xor eq. */
+    exists(AbstractValue v | simple_comparison_eq(derived, left, right, k, v) |
+      areEqual = true and value = v
+      or
+      areEqual = false and value = v.getDualValue()
+    )
     or
-    areEqual = false and value = v.getDualValue()
-  )
-  or
-  // I think this is handled by forwarding in controlsBlock.
-  //or
-  //logical_comparison_eq(test, left, right, k, areEqual, testIsTrue)
-  /* a == b + k => b == a - k */
-  exists(int mk | k = -mk | compares_eq(test, right, left, mk, areEqual, value))
-  or
-  complex_eq(test, left, right, k, areEqual, value)
-  or
-  /* (x is true => (left == right + k)) => (!x is false => (left == right + k)) */
-  exists(AbstractValue dual | value = dual.getDualValue() |
-    compares_eq(test.(LogicalNotInstruction).getUnary(), left, right, k, areEqual, dual)
+    // I think this is handled by forwarding in controlsBlock.
+    //or
+    //logical_comparison_eq(test, left, right, k, areEqual, testIsTrue)
+    /* a == b + k => b == a - k */
+    exists(int mk | k = -mk | compares_eq(derived, right, left, mk, areEqual, value))
+    or
+    complex_eq(derived, left, right, k, areEqual, value)
+    or
+    /* (x is true => (left == right + k)) => (!x is false => (left == right + k)) */
+    exists(AbstractValue dual | value = dual.getDualValue() |
+      compares_eq(derived.(LogicalNotInstruction).getUnary(), left, right, k, areEqual, dual)
+    )
+    or
+    compares_eq(derived.(BuiltinExpectCallInstruction).getCondition(), left, right, k, areEqual,
+      value)
   )
-  or
-  compares_eq(test.(BuiltinExpectCallInstruction).getCondition(), left, right, k, areEqual, value)
 }
 
 /**
@@ -803,38 +855,41 @@ private predicate compares_eq(
 private predicate unary_compares_eq(
   Instruction test, Operand op, int k, boolean areEqual, boolean inNonZeroCase, AbstractValue value
 ) {
-  /* The simple case where the test *is* the comparison so areEqual = testIsTrue xor eq. */
-  exists(AbstractValue v |
-    unary_simple_comparison_eq(test, k, inNonZeroCase, v) and op.getDef() = test
-  |
-    areEqual = true and value = v
+  exists(Instruction derived | derived = getDerivedInstruction(test) |
+    /* The simple case where the test *is* the comparison so areEqual = testIsTrue xor eq. */
+    exists(AbstractValue v |
+      unary_simple_comparison_eq(derived, k, inNonZeroCase, v) and op.getDef() = derived
+    |
+      areEqual = true and value = v
+      or
+      areEqual = false and value = v.getDualValue()
+    )
     or
-    areEqual = false and value = v.getDualValue()
-  )
-  or
-  unary_complex_eq(test, op, k, areEqual, inNonZeroCase, value)
-  or
-  /* (x is true => (op == k)) => (!x is false => (op == k)) */
-  exists(AbstractValue dual, boolean inNonZeroCase0 |
-    value = dual.getDualValue() and
-    unary_compares_eq(test.(LogicalNotInstruction).getUnary(), op, k, inNonZeroCase0, areEqual, dual)
-  |
-    k = 0 and inNonZeroCase = inNonZeroCase0
+    unary_complex_eq(derived, op, k, areEqual, inNonZeroCase, value)
     or
-    k != 0 and inNonZeroCase = true
-  )
-  or
-  // ((test is `areEqual` => op == const + k2) and const == `k1`) =>
-  // test is `areEqual` => op == k1 + k2
-  inNonZeroCase = false and
-  exists(int k1, int k2, ConstantInstruction const |
-    compares_eq(test, op, const.getAUse(), k2, areEqual, value) and
-    int_value(const) = k1 and
-    k = k1 + k2
+    /* (x is true => (op == k)) => (!x is false => (op == k)) */
+    exists(AbstractValue dual, boolean inNonZeroCase0 |
+      value = dual.getDualValue() and
+      unary_compares_eq(derived.(LogicalNotInstruction).getUnary(), op, k, inNonZeroCase0, areEqual,
+        dual)
+    |
+      k = 0 and inNonZeroCase = inNonZeroCase0
+      or
+      k != 0 and inNonZeroCase = true
+    )
+    or
+    // ((test is `areEqual` => op == const + k2) and const == `k1`) =>
+    // test is `areEqual` => op == k1 + k2
+    inNonZeroCase = false and
+    exists(int k1, int k2, Instruction const |
+      compares_eq(derived, op, const.getAUse(), k2, areEqual, value) and
+      int_value(const) = k1 and
+      k = k1 + k2
+    )
+    or
+    unary_compares_eq(derived.(BuiltinExpectCallInstruction).getCondition(), op, k, areEqual,
+      inNonZeroCase, value)
   )
-  or
-  unary_compares_eq(test.(BuiltinExpectCallInstruction).getCondition(), op, k, areEqual,
-    inNonZeroCase, value)
 }
 
 /** Rearrange various simple comparisons into `left == right + k` form. */
@@ -868,6 +923,18 @@ private predicate unary_simple_comparison_eq(
     inNonZeroCase = false
   )
   or
+  (
+    exists(BinaryLogicalOperation e | e.getAnOperand() = test.getUnconvertedResultExpression())
+    or
+    exists(UnaryLogicalOperation e | e.getAnOperand() = test.getUnconvertedResultExpression())
+    or
+    exists(IfStmt i | i.getCondition() = test.getUnconvertedResultExpression())
+    or
+    exists(Loop i | i.getCondition() = test.getUnconvertedResultExpression())
+    or
+    exists(ConditionalExpr c | c.getCondition() = test.getUnconvertedResultExpression())
+    // Todo recursive conditionalExpr in guard
+  ) and
   // Any instruction with an integral type could potentially be part of a
   // check for nullness when used in a guard. So we include all integral
   // typed instructions here. However, since some of these instructions are
@@ -912,6 +979,8 @@ private predicate unary_simple_comparison_eq(
     value.(BooleanValue).getValue() = false and
     inNonZeroCase = false
   )
+  // Has to be in a boolean operator (operanad of OR or AND)
+  // has to be a 'guard' (loop, if, switch, ternary)
 }
 
 /** A call to the builtin operation `__builtin_expect`. */
@@ -997,37 +1066,44 @@ private predicate unary_complex_eq(
 private predicate compares_lt(
   Instruction test, Operand left, Operand right, int k, boolean isLt, AbstractValue value
 ) {
-  /* In the simple case, the test is the comparison, so isLt = testIsTrue */
-  simple_comparison_lt(test, left, right, k) and
-  value.(BooleanValue).getValue() = isLt
-  or
-  complex_lt(test, left, right, k, isLt, value)
-  or
-  /* (not (left < right + k)) => (left >= right + k) */
-  exists(boolean isGe | isLt = isGe.booleanNot() | compares_ge(test, left, right, k, isGe, value))
-  or
-  /* (x is true => (left < right + k)) => (!x is false => (left < right + k)) */
-  exists(AbstractValue dual | value = dual.getDualValue() |
-    compares_lt(test.(LogicalNotInstruction).getUnary(), left, right, k, isLt, dual)
+  exists(Instruction derived | derived = getDerivedInstruction(test) |
+    // exists(thing | thing = derived ... )
+    /* In the simple case, the test is the comparison, so isLt = testIsTrue */
+    simple_comparison_lt(derived, left, right, k) and
+    value.(BooleanValue).getValue() = isLt
+    or
+    complex_lt(derived, left, right, k, isLt, value)
+    or
+    /* (not (left < right + k)) => (left >= right + k) */
+    exists(boolean isGe | isLt = isGe.booleanNot() |
+      compares_ge(derived, left, right, k, isGe, value)
+    )
+    or
+    /* (x is true => (left < right + k)) => (!x is false => (left < right + k)) */
+    exists(AbstractValue dual | value = dual.getDualValue() |
+      compares_lt(derived.(LogicalNotInstruction).getUnary(), left, right, k, isLt, dual)
+    )
   )
 }
 
 /** Holds if `op < k` evaluates to `isLt` given that `test` evaluates to `value`. */
 private predicate compares_lt(Instruction test, Operand op, int k, boolean isLt, AbstractValue value) {
-  unary_simple_comparison_lt(test, k, isLt, value) and
-  op.getDef() = test
-  or
-  complex_lt(test, op, k, isLt, value)
-  or
-  /* (x is true => (op < k)) => (!x is false => (op < k)) */
-  exists(AbstractValue dual | value = dual.getDualValue() |
-    compares_lt(test.(LogicalNotInstruction).getUnary(), op, k, isLt, dual)
-  )
-  or
-  exists(int k1, int k2, ConstantInstruction const |
-    compares_lt(test, op, const.getAUse(), k2, isLt, value) and
-    int_value(const) = k1 and
-    k = k1 + k2
+  exists(Instruction derived | derived = getDerivedInstruction(test) |
+    unary_simple_comparison_lt(derived, k, isLt, value) and
+    op.getDef() = derived
+    or
+    complex_lt(derived, op, k, isLt, value)
+    or
+    /* (x is true => (op < k)) => (!x is false => (op < k)) */
+    exists(AbstractValue dual | value = dual.getDualValue() |
+      compares_lt(derived.(LogicalNotInstruction).getUnary(), op, k, isLt, dual)
+    )
+    or
+    exists(int k1, int k2, Instruction const |
+      compares_lt(derived, op, const.getAUse(), k2, isLt, value) and
+      int_value(const) = k1 and
+      k = k1 + k2
+    )
   )
 }
 
@@ -1352,5 +1428,9 @@ private class IntegerOrPointerConstantInstruction extends ConstantInstruction {
 
 /** The int value of integer constant expression. */
 private int int_value(Instruction i) {
-  result = i.(IntegerOrPointerConstantInstruction).getValue().toInt()
+  result =
+    valueNumber(i).getAnInstruction().(IntegerOrPointerConstantInstruction).getValue().toInt()
+  or
+  // handle conversions
+  result = int_value(valueNumber(i).getAnInstruction().(ConvertInstruction).getUnary())
 }

cpp/ql/test/library-tests/controlflow/guards-ir/test.c

@@ -184,3 +184,14 @@ int abort_test(int x) {
         abort();
     }
 }
+
+void boolean_flow(int x){
+    int b = x > 5;
+
+    if(!b){
+    }
+
+    if(b&&x<100){
+
+    }
+}