C++: Generate int-to-bool conversion instructions in C code

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

@@ -681,7 +681,7 @@ class IRGuardCondition extends Instruction {
   /** Holds if (determined by this guard) `op == k` evaluates to `areEqual` if this expression evaluates to `value`. */
   cached
   predicate comparesEq(Operand op, int k, boolean areEqual, AbstractValue value) {
-    unary_compares_eq(valueNumber(this), op, k, areEqual, false, value)
+    unary_compares_eq(valueNumber(this), op, k, areEqual, value)
   }
 
   /**
@@ -703,7 +703,7 @@ class IRGuardCondition extends Instruction {
   cached
   predicate ensuresEq(Operand op, int k, IRBlock block, boolean areEqual) {
     exists(AbstractValue value |
-      unary_compares_eq(valueNumber(this), op, k, areEqual, false, value) and
+      unary_compares_eq(valueNumber(this), op, k, areEqual, value) and
       this.valueControls(block, value)
     )
   }
@@ -729,7 +729,7 @@ class IRGuardCondition extends Instruction {
   cached
   predicate ensuresEqEdge(Operand op, int k, IRBlock pred, IRBlock succ, boolean areEqual) {
     exists(AbstractValue value |
-      unary_compares_eq(valueNumber(this), op, k, areEqual, false, value) and
+      unary_compares_eq(valueNumber(this), op, k, areEqual, value) and
       this.valueControlsEdge(pred, succ, value)
     )
   }
@@ -866,72 +866,34 @@ private predicate compares_eq(
 
 /**
  * Holds if `op == k` is `areEqual` given that `test` is equal to `value`.
- *
- * Many internal predicates in this file have a `inNonZeroCase` column.
- * Ideally, the `k` column would be a type such as `Option<int>::Option`, to
- * represent whether we have a concrete value `k` such that `op == k`, or whether
- * we only know that `op != 0`.
- * However, cannot instantiate `Option` with an infinite type. Thus the boolean
- * `inNonZeroCase` is used to distinquish the `Some` (where we have a concrete
- * value `k`) and `None` cases (where we only know that `op != 0`).
- *
- * Thus, if `inNonZeroCase = true` then `op != 0` and the value of `k` is
- * meaningless.
- *
- * To see why `inNonZeroCase` is needed consider the following C program:
- * ```c
- * char* p = ...;
- * if(p) {
- *   use(p);
- * }
- * ```
- * in C++ there would be an int-to-bool conversion on `p`. However, since C
- * does not have booleans there is no conversion. We want to be able to
- * conclude that `p` is non-zero in the true branch, so we need to give `k`
- * some value. However, simply setting `k = 1` would make the rest of the
- * analysis think that `k == 1` holds inside the branch. So we distinquish
- * between the above case and
- * ```c
- * if(p == 1) {
- *   use(p)
- * }
- * ```
- * by setting `inNonZeroCase` to `true` in the former case, but not in the
- * latter.
  */
 private predicate unary_compares_eq(
-  ValueNumber test, Operand op, int k, boolean areEqual, boolean inNonZeroCase, AbstractValue value
+  ValueNumber test, Operand op, int k, boolean areEqual, AbstractValue value
 ) {
   /* The simple case where the test *is* the comparison so areEqual = testIsTrue xor eq. */
-  exists(AbstractValue v | unary_simple_comparison_eq(test, op, k, inNonZeroCase, v) |
+  exists(AbstractValue v | unary_simple_comparison_eq(test, op, k, v) |
     areEqual = true and value = v
     or
     areEqual = false and value = v.getDualValue()
   )
   or
-  unary_complex_eq(test, op, k, areEqual, inNonZeroCase, value)
+  unary_complex_eq(test, op, k, areEqual, value)
   or
   /* (x is true => (op == k)) => (!x is false => (op == k)) */
-  exists(AbstractValue dual, boolean inNonZeroCase0 |
+  exists(AbstractValue dual |
     value = dual.getDualValue() and
-    unary_compares_eq(test.(LogicalNotValueNumber).getUnary(), op, k, inNonZeroCase0, areEqual, dual)
-  |
-    k = 0 and inNonZeroCase = inNonZeroCase0
-    or
-    k != 0 and inNonZeroCase = true
+    unary_compares_eq(test.(LogicalNotValueNumber).getUnary(), op, k, areEqual, dual)
   )
   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(test, op, const.getAUse(), k2, areEqual, value) and
     int_value(const) = k1 and
     k = k1 + k2
   )
   or
-  unary_compares_eq(test.(BuiltinExpectCallValueNumber).getCondition(), op, k, areEqual,
-    inNonZeroCase, value)
+  unary_compares_eq(test.(BuiltinExpectCallValueNumber).getCondition(), op, k, areEqual, value)
 }
 
 /** Rearrange various simple comparisons into `left == right + k` form. */
@@ -949,78 +911,24 @@ private predicate simple_comparison_eq(
   value.(BooleanValue).getValue() = false
 }
 
-/**
- * Holds if `op` is an operand that is eventually used in a unary comparison
- * with a constant.
- */
-private predicate isRelevantUnaryComparisonOperand(Operand op) {
-  // Base case: `op` is an operand of a `CompareEQInstruction` or `CompareNEInstruction`,
-  // and the other operand is a constant.
-  exists(CompareInstruction eq, Instruction instr |
-    eq.hasOperands(op, instr.getAUse()) and
-    exists(int_value(instr))
-  |
-    eq instanceof CompareEQInstruction
-    or
-    eq instanceof CompareNEInstruction
-  )
-  or
-  // C doesn't have int-to-bool conversions, so `if(x)` will just generate:
-  // r2_1(glval<int>) = VariableAddress[x]
-  // r2_2(int)        = Load[x]                : &:r2_1, m1_6
-  // v2_3(void)       = ConditionalBranch      : r2_2
-  exists(ConditionalBranchInstruction branch | branch.getConditionOperand() = op)
-  or
-  // If `!x` is a relevant unary comparison then so is `x`.
-  exists(LogicalNotInstruction logicalNot |
-    isRelevantUnaryComparisonOperand(unique( | | logicalNot.getAUse())) and
-    logicalNot.getUnaryOperand() = op
-  )
-  or
-  // If `y` is a relevant unary comparison and `y = x` then so is `x`.
-  not op.isDefinitionInexact() and
-  exists(CopyInstruction copy |
-    isRelevantUnaryComparisonOperand(unique( | | copy.getAUse())) and
-    op = copy.getSourceValueOperand()
-  )
-  or
-  // If phi(x1, x2) is a relevant unary comparison then so are `x1` and `x2`.
-  not op.isDefinitionInexact() and
-  exists(PhiInstruction phi |
-    isRelevantUnaryComparisonOperand(unique( | | phi.getAUse())) and
-    op = phi.getAnInputOperand()
-  )
-  or
-  // If `__builtin_expect(x)` is a relevant unary comparison then so is `x`.
-  exists(BuiltinExpectCallInstruction call |
-    isRelevantUnaryComparisonOperand(unique( | | call.getAUse())) and
-    op = call.getConditionOperand()
-  )
-}
-
 /** Rearrange various simple comparisons into `op == k` form. */
 private predicate unary_simple_comparison_eq(
-  ValueNumber test, Operand op, int k, boolean inNonZeroCase, AbstractValue value
+  ValueNumber test, Operand op, int k, AbstractValue value
 ) {
   exists(CaseEdge case, SwitchConditionValueNumber condition |
     condition = test and
     op = condition.getExpressionOperand() and
     case = value.(MatchValue).getCase() and
     exists(condition.getSuccessor(case)) and
-    case.getValue().toInt() = k and
-    inNonZeroCase = false
+    case.getValue().toInt() = k
   )
   or
-  isRelevantUnaryComparisonOperand(op) and
-  op.getDef() = test.getAnInstruction() and
-  (
-    k = 1 and
+  exists(Instruction const | int_value(const) = k |
     value.(BooleanValue).getValue() = true and
-    inNonZeroCase = true
+    test.(CompareEQValueNumber).hasOperands(op, const.getAUse())
     or
-    k = 0 and
     value.(BooleanValue).getValue() = false and
-    inNonZeroCase = false
+    test.(CompareNEValueNumber).hasOperands(op, const.getAUse())
   )
 }
 
@@ -1074,13 +982,12 @@ private predicate complex_eq(
  * an instruction that compares the value of `__builtin_expect(op == k, _)` to `0`.
  */
 private predicate unary_builtin_expect_eq(
-  CompareValueNumber cmp, Operand op, int k, boolean areEqual, boolean inNonZeroCase,
-  AbstractValue value
+  CompareValueNumber cmp, Operand op, int k, boolean areEqual, AbstractValue value
 ) {
   exists(BuiltinExpectCallValueNumber call, Instruction const, AbstractValue innerValue |
     int_value(const) = 0 and
     cmp.hasOperands(call.getAUse(), const.getAUse()) and
-    unary_compares_eq(call.getCondition(), op, k, areEqual, inNonZeroCase, innerValue)
+    unary_compares_eq(call.getCondition(), op, k, areEqual, innerValue)
   |
     cmp instanceof CompareNEValueNumber and
     value = innerValue
@@ -1091,13 +998,13 @@ private predicate unary_builtin_expect_eq(
 }
 
 private predicate unary_complex_eq(
-  ValueNumber test, Operand op, int k, boolean areEqual, boolean inNonZeroCase, AbstractValue value
+  ValueNumber test, Operand op, int k, boolean areEqual, AbstractValue value
 ) {
-  unary_sub_eq(test, op, k, areEqual, inNonZeroCase, value)
+  unary_sub_eq(test, op, k, areEqual, value)
   or
-  unary_add_eq(test, op, k, areEqual, inNonZeroCase, value)
+  unary_add_eq(test, op, k, areEqual, value)
   or
-  unary_builtin_expect_eq(test, op, k, areEqual, inNonZeroCase, value)
+  unary_builtin_expect_eq(test, op, k, areEqual, value)
 }
 
 /*
@@ -1357,19 +1264,17 @@ private predicate sub_eq(
 
 // op - x == c => op == (c+x)
 private predicate unary_sub_eq(
-  ValueNumber test, Operand op, int k, boolean areEqual, boolean inNonZeroCase, AbstractValue value
+  ValueNumber test, Operand op, int k, boolean areEqual, AbstractValue value
 ) {
-  inNonZeroCase = false and
   exists(SubInstruction sub, int c, int x |
-    unary_compares_eq(test, sub.getAUse(), c, areEqual, inNonZeroCase, value) and
+    unary_compares_eq(test, sub.getAUse(), c, areEqual, value) and
     op = sub.getLeftOperand() and
     x = int_value(sub.getRight()) and
     k = c + x
   )
   or
-  inNonZeroCase = false and
   exists(PointerSubInstruction sub, int c, int x |
-    unary_compares_eq(test, sub.getAUse(), c, areEqual, inNonZeroCase, value) and
+    unary_compares_eq(test, sub.getAUse(), c, areEqual, value) and
     op = sub.getLeftOperand() and
     x = int_value(sub.getRight()) and
     k = c + x
@@ -1424,12 +1329,10 @@ private predicate add_eq(
 
 // left + x == right + c => left == right + (c-x)
 private predicate unary_add_eq(
-  ValueNumber test, Operand left, int k, boolean areEqual, boolean inNonZeroCase,
-  AbstractValue value
+  ValueNumber test, Operand left, int k, boolean areEqual, AbstractValue value
 ) {
-  inNonZeroCase = false and
   exists(AddInstruction lhs, int c, int x |
-    unary_compares_eq(test, lhs.getAUse(), c, areEqual, inNonZeroCase, value) and
+    unary_compares_eq(test, lhs.getAUse(), c, areEqual, value) and
     (
       left = lhs.getLeftOperand() and x = int_value(lhs.getRight())
       or
@@ -1438,9 +1341,8 @@ private predicate unary_add_eq(
     k = c - x
   )
   or
-  inNonZeroCase = false and
   exists(PointerAddInstruction lhs, int c, int x |
-    unary_compares_eq(test, lhs.getAUse(), c, areEqual, inNonZeroCase, value) and
+    unary_compares_eq(test, lhs.getAUse(), c, areEqual, value) and
     (
       left = lhs.getLeftOperand() and x = int_value(lhs.getRight())
       or

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/IRConstruction.qll

@@ -171,6 +171,20 @@ module Raw {
       // forwarded the result of another translated expression.
       instruction = translatedExpr.getInstruction(_)
     )
+    or
+    // Consider the snippet `if(x) { ... }` where `x` is an integer.
+    // In C++ there is a `BoolConversion` conversion on `x` which generates a
+    // `CompareNEInstruction` whose `getInstructionUnconvertedResultExpression`
+    // is the `BoolConversion`. Thus, calling `getInstructionConvertedResultExpression` on
+    // the `CompareNEInstruction` gives `x` in C++ code.
+    // However, in C there is no such conversion to return. So instead we have
+    // to map the result of `getInstructionConvertedResultExpression` on
+    // the `CompareNEInstruction` to `x` manually.
+    exists(TranslatedValueCondition translatedValueCondition |
+      translatedValueCondition = getTranslatedCondition(result) and
+      translatedValueCondition.shouldGenerateCompareNE() and
+      instruction = translatedValueCondition.getInstruction(ValueConditionCompareTag())
+    )
   }
 
   cached

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/InstructionTag.qll

@@ -38,7 +38,11 @@ newtype TInstructionTag =
   AllocationSizeTag() or
   AllocationElementSizeTag() or
   AllocationExtentConvertTag() or
+  ValueConditionCompareTag() or
+  ValueConditionConstantTag() or
   ValueConditionConditionalBranchTag() or
+  ValueConditionConditionalConstantTag() or
+  ValueConditionConditionalCompareTag() or
   ConditionValueTrueTempAddressTag() or
   ConditionValueTrueConstantTag() or
   ConditionValueTrueStoreTag() or
@@ -49,6 +53,8 @@ newtype TInstructionTag =
   ConditionValueResultLoadTag() or
   BoolConversionConstantTag() or
   BoolConversionCompareTag() or
+  NotExprOperationTag() or
+  NotExprConstantTag() or
   ResultCopyTag() or
   LoadTag() or // Implicit load due to lvalue-to-rvalue conversion
   CatchTag() or
@@ -167,6 +173,14 @@ string getInstructionTagId(TInstructionTag tag) {
   or
   tag = ValueConditionConditionalBranchTag() and result = "ValCondCondBranch"
   or
+  tag = ValueConditionConditionalConstantTag() and result = "ValueConditionConditionalConstant"
+  or
+  tag = ValueConditionConditionalCompareTag() and result = "ValueConditionConditionalCompare"
+  or
+  tag = ValueConditionCompareTag() and result = "ValCondCondCompare"
+  or
+  tag = ValueConditionConstantTag() and result = "ValCondConstant"
+  or
   tag = ConditionValueTrueTempAddressTag() and result = "CondValTrueTempAddr"
   or
   tag = ConditionValueTrueConstantTag() and result = "CondValTrueConst"
@@ -187,6 +201,10 @@ string getInstructionTagId(TInstructionTag tag) {
   or
   tag = BoolConversionCompareTag() and result = "BoolConvComp"
   or
+  tag = NotExprOperationTag() and result = "NotExprOperation"
+  or
+  tag = NotExprConstantTag() and result = "NotExprWithBoolConversionConstant"
+  or
   tag = ResultCopyTag() and result = "ResultCopy"
   or
   tag = LoadTag() and result = "Load" // Implicit load due to lvalue-to-rvalue conversion