Merge pull request #18490 from MathiasVP/generate-int-to-bool-conversion-instructions-2

C++: Generate int-to-bool conversions in C code

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

@@ -233,6 +233,114 @@ private class GuardConditionFromBinaryLogicalOperator extends GuardConditionImpl
   }
 }
 
+/**
+ * Holds if `ir` controls `block`, meaning that `block` is only
+ * entered if the value of this condition is `v`. This helper
+ * predicate does not necessarily hold for binary logical operations like
+ * `&&` and `||`. See the detailed explanation on predicate `controls`.
+ */
+private predicate controlsBlock(IRGuardCondition ir, BasicBlock controlled, AbstractValue v) {
+  exists(IRBlock irb |
+    ir.valueControls(irb, v) and
+    nonExcludedIRAndBasicBlock(irb, controlled) and
+    not isUnreachedBlock(irb)
+  )
+}
+
+private class GuardConditionFromNotExpr extends GuardConditionImpl {
+  IRGuardCondition ir;
+
+  GuardConditionFromNotExpr() {
+    // Users often expect the `x` in `!x` to also be a guard condition. But
+    // from the perspective of the IR the `x` is just the left-hand side of a
+    // comparison against 0 so it's not included as a normal
+    // `IRGuardCondition`. So to align with user expectations we make that `x`
+    // a `GuardCondition`.
+    exists(NotExpr notExpr |
+      this = notExpr.getOperand() and
+      ir.getUnconvertedResultExpression() = notExpr
+    )
+  }
+
+  override predicate valueControls(BasicBlock controlled, AbstractValue v) {
+    // This condition must determine the flow of control; that is, this
+    // node must be a top-level condition.
+    controlsBlock(ir, controlled, v.getDualValue())
+  }
+
+  pragma[inline]
+  override predicate comparesLt(Expr left, Expr right, int k, boolean isLessThan, boolean testIsTrue) {
+    exists(Instruction li, Instruction ri |
+      li.getUnconvertedResultExpression() = left and
+      ri.getUnconvertedResultExpression() = right and
+      ir.comparesLt(li.getAUse(), ri.getAUse(), k, isLessThan, testIsTrue.booleanNot())
+    )
+  }
+
+  pragma[inline]
+  override predicate comparesLt(Expr e, int k, boolean isLessThan, AbstractValue value) {
+    exists(Instruction i |
+      i.getUnconvertedResultExpression() = e and
+      ir.comparesLt(i.getAUse(), k, isLessThan, value.getDualValue())
+    )
+  }
+
+  pragma[inline]
+  override predicate ensuresLt(Expr left, Expr right, int k, BasicBlock block, boolean isLessThan) {
+    exists(Instruction li, Instruction ri, boolean testIsTrue |
+      li.getUnconvertedResultExpression() = left and
+      ri.getUnconvertedResultExpression() = right and
+      ir.comparesLt(li.getAUse(), ri.getAUse(), k, isLessThan, testIsTrue.booleanNot()) and
+      this.controls(block, testIsTrue)
+    )
+  }
+
+  pragma[inline]
+  override predicate ensuresLt(Expr e, int k, BasicBlock block, boolean isLessThan) {
+    exists(Instruction i, AbstractValue value |
+      i.getUnconvertedResultExpression() = e and
+      ir.comparesLt(i.getAUse(), k, isLessThan, value.getDualValue()) and
+      this.valueControls(block, value)
+    )
+  }
+
+  pragma[inline]
+  override predicate comparesEq(Expr left, Expr right, int k, boolean areEqual, boolean testIsTrue) {
+    exists(Instruction li, Instruction ri |
+      li.getUnconvertedResultExpression() = left and
+      ri.getUnconvertedResultExpression() = right and
+      ir.comparesEq(li.getAUse(), ri.getAUse(), k, areEqual, testIsTrue.booleanNot())
+    )
+  }
+
+  pragma[inline]
+  override predicate ensuresEq(Expr left, Expr right, int k, BasicBlock block, boolean areEqual) {
+    exists(Instruction li, Instruction ri, boolean testIsTrue |
+      li.getUnconvertedResultExpression() = left and
+      ri.getUnconvertedResultExpression() = right and
+      ir.comparesEq(li.getAUse(), ri.getAUse(), k, areEqual, testIsTrue.booleanNot()) and
+      this.controls(block, testIsTrue)
+    )
+  }
+
+  pragma[inline]
+  override predicate comparesEq(Expr e, int k, boolean areEqual, AbstractValue value) {
+    exists(Instruction i |
+      i.getUnconvertedResultExpression() = e and
+      ir.comparesEq(i.getAUse(), k, areEqual, value.getDualValue())
+    )
+  }
+
+  pragma[inline]
+  override predicate ensuresEq(Expr e, int k, BasicBlock block, boolean areEqual) {
+    exists(Instruction i, AbstractValue value |
+      i.getUnconvertedResultExpression() = e and
+      ir.comparesEq(i.getAUse(), k, areEqual, value.getDualValue()) and
+      this.valueControls(block, value)
+    )
+  }
+}
+
 /**
  * A Boolean condition in the AST that guards one or more basic blocks and has a corresponding IR
  * instruction.
@@ -245,7 +353,7 @@ private class GuardConditionFromIR extends GuardConditionImpl {
   override predicate valueControls(BasicBlock controlled, AbstractValue v) {
     // This condition must determine the flow of control; that is, this
     // node must be a top-level condition.
-    this.controlsBlock(controlled, v)
+    controlsBlock(ir, controlled, v)
   }
 
   pragma[inline]
@@ -319,20 +427,6 @@ private class GuardConditionFromIR extends GuardConditionImpl {
       this.valueControls(block, value)
     )
   }
-
-  /**
-   * Holds if this condition controls `block`, meaning that `block` is only
-   * entered if the value of this condition is `v`. This helper
-   * predicate does not necessarily hold for binary logical operations like
-   * `&&` and `||`. See the detailed explanation on predicate `controls`.
-   */
-  private predicate controlsBlock(BasicBlock controlled, AbstractValue v) {
-    exists(IRBlock irb |
-      ir.valueControls(irb, v) and
-      nonExcludedIRAndBasicBlock(irb, controlled) and
-      not isUnreachedBlock(irb)
-    )
-  }
 }
 
 private predicate excludeAsControlledInstruction(Instruction instr) {
@@ -588,7 +682,7 @@ class IRGuardCondition extends Instruction {
   /** Holds if (determined by this guard) `op == k` evaluates to `areEqual` if this expression evaluates to `value`. */
   pragma[inline]
   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)
   }
 
   /**
@@ -610,7 +704,7 @@ class IRGuardCondition extends Instruction {
   pragma[inline]
   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)
     )
   }
@@ -636,7 +730,7 @@ class IRGuardCondition extends Instruction {
   pragma[inline]
   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)
     )
   }
@@ -847,77 +941,59 @@ private module Cached {
     compares_eq(test.(BuiltinExpectCallValueNumber).getCondition(), left, right, k, areEqual, value)
   }
 
+  private predicate isConvertedBool(Instruction instr) {
+    instr.getResultIRType() instanceof IRBooleanType
+    or
+    isConvertedBool(instr.(ConvertInstruction).getUnary())
+    or
+    isConvertedBool(instr.(BuiltinExpectCallInstruction).getCondition())
+  }
+
   /**
    * 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.
    */
   cached
   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)
+    exists(CompareValueNumber cmp, Operand left, Operand right, AbstractValue v |
+      test = cmp and
+      cmp.hasOperands(left, right) and
+      isConvertedBool(left.getDef()) and
+      int_value(right.getDef()) = 0 and
+      unary_compares_eq(valueNumberOfOperand(left), op, k, areEqual, v)
+    |
+      cmp instanceof CompareNEValueNumber and
+      v = value
+      or
+      cmp instanceof CompareEQValueNumber and
+      v.getDualValue() = value
+    )
+    or
+    unary_compares_eq(test.(BuiltinExpectCallValueNumber).getCondition(), op, k, areEqual, value)
   }
 
   /** Rearrange various simple comparisons into `left == right + k` form. */
@@ -939,74 +1015,64 @@ private module Cached {
    * 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)
+  private predicate mayBranchOn(Instruction instr) {
+    exists(ConditionalBranchInstruction branch | branch.getCondition() = instr)
     or
     // If `!x` is a relevant unary comparison then so is `x`.
     exists(LogicalNotInstruction logicalNot |
-      isRelevantUnaryComparisonOperand(unique( | | logicalNot.getAUse())) and
-      logicalNot.getUnaryOperand() = op
+      mayBranchOn(logicalNot) and
+      logicalNot.getUnary() = instr
     )
     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()
+      mayBranchOn(copy) and
+      instr = copy.getSourceValue()
     )
     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()
+      mayBranchOn(phi) and
+      instr = phi.getAnInput()
     )
     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()
+      mayBranchOn(call) and
+      instr = call.getCondition()
     )
   }
 
   /** 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())
+    )
+    or
+    exists(BooleanValue bv |
+      bv = value and
+      mayBranchOn(op.getDef()) and
+      op = test.getAUse()
+    |
+      k = 0 and
+      bv.getValue() = false
+      or
+      k = 1 and
+      bv.getValue() = true
     )
   }
 
@@ -1061,13 +1127,12 @@ private module Cached {
    * 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
@@ -1078,14 +1143,13 @@ private module Cached {
   }
 
   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)
   }
 
   /*
@@ -1347,20 +1411,17 @@ private module Cached {
 
   // 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
@@ -1415,12 +1476,10 @@ private module Cached {
 
   // 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
@@ -1429,9 +1488,8 @@ private module Cached {
       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,23 @@ 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 `getInstructionConvertedResultExpression`
+    // is the `BoolConversion` (by the logic in the disjunct above). Thus,
+    // calling `getInstructionUnconvertedResultExpression` 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. This ensures that calling
+    // `getInstructionUnconvertedResultExpression` on the `CompareNEInstruction`
+    // gives `x` in both the C case and C++ case.
+    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

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

@@ -187,7 +187,24 @@ class TranslatedValueCondition extends TranslatedCondition, TTranslatedValueCond
 
   final override predicate handlesDestructorsExplicitly() { none() } // TODO: this needs to be revisted when we get unnamed destructors
 
+  private Type getValueExprType() {
+    result = this.getValueExpr().getExprType().getUnspecifiedType()
+  }
+
+  predicate shouldGenerateCompareNE() { not this.getValueExprType() instanceof BoolType }
+
   override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType resultType) {
+    this.shouldGenerateCompareNE() and
+    (
+      tag = ValueConditionCompareTag() and
+      opcode instanceof Opcode::CompareNE and
+      resultType = getBoolType()
+      or
+      tag = ValueConditionConstantTag() and
+      opcode instanceof Opcode::Constant and
+      resultType = getTypeForPRValue(this.getValueExprType())
+    )
+    or
     tag = ValueConditionConditionalBranchTag() and
     opcode instanceof Opcode::ConditionalBranch and
     resultType = getVoidType()
@@ -195,11 +212,24 @@ class TranslatedValueCondition extends TranslatedCondition, TTranslatedValueCond
 
   override Instruction getChildSuccessorInternal(TranslatedElement child, EdgeKind kind) {
     child = this.getValueExpr() and
-    result = this.getInstruction(ValueConditionConditionalBranchTag()) and
-    kind instanceof GotoEdge
+    kind instanceof GotoEdge and
+    if this.shouldGenerateCompareNE()
+    then result = this.getInstruction(ValueConditionConstantTag())
+    else result = this.getInstruction(ValueConditionConditionalBranchTag())
   }
 
   override Instruction getInstructionSuccessorInternal(InstructionTag tag, EdgeKind kind) {
+    this.shouldGenerateCompareNE() and
+    (
+      tag = ValueConditionConstantTag() and
+      kind instanceof GotoEdge and
+      result = this.getInstruction(ValueConditionCompareTag())
+      or
+      tag = ValueConditionCompareTag() and
+      kind instanceof GotoEdge and
+      result = this.getInstruction(ValueConditionConditionalBranchTag())
+    )
+    or
     tag = ValueConditionConditionalBranchTag() and
     (
       kind instanceof TrueEdge and
@@ -211,9 +241,26 @@ class TranslatedValueCondition extends TranslatedCondition, TTranslatedValueCond
   }
 
   override Instruction getInstructionRegisterOperand(InstructionTag tag, OperandTag operandTag) {
+    this.shouldGenerateCompareNE() and
+    tag = ValueConditionCompareTag() and
+    (
+      operandTag instanceof LeftOperandTag and
+      result = this.getValueExpr().getResult()
+      or
+      operandTag instanceof RightOperandTag and
+      result = this.getInstruction(ValueConditionConstantTag())
+    )
+    or
     tag = ValueConditionConditionalBranchTag() and
     operandTag instanceof ConditionOperandTag and
-    result = this.getValueExpr().getResult()
+    if this.shouldGenerateCompareNE()
+    then result = this.getInstruction(ValueConditionCompareTag())
+    else result = this.getValueExpr().getResult()
+  }
+
+  override string getInstructionConstantValue(InstructionTag tag) {
+    tag = ValueConditionConstantTag() and
+    result = "0"
   }
 
   private TranslatedExpr getValueExpr() { result = getTranslatedExpr(expr) }

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

@@ -59,10 +59,19 @@ abstract class TranslatedExpr extends TranslatedElement {
 
   final CppType getResultType() {
     if this.isResultGLValue()
-    then result = getTypeForGLValue(expr.getType())
-    else result = getTypeForPRValue(expr.getType())
+    then result = getTypeForGLValue(this.getExprType())
+    else result = getTypeForPRValue(this.getExprType())
   }
 
+  /**
+   * Gets the type of `expr`.
+   *
+   * This predicate can be overwritten in subclasses to modify the result
+   * of `getResultType` which determines the type of the instruction that
+   * generates the result of `expr`.
+   */
+  Type getExprType() { result = expr.getType() }
+
   /**
    * Holds if the result of this `TranslatedExpr` is a glvalue.
    */
@@ -1262,9 +1271,10 @@ abstract class TranslatedSingleInstructionExpr extends TranslatedNonConstantExpr
 
 class TranslatedUnaryExpr extends TranslatedSingleInstructionExpr {
   TranslatedUnaryExpr() {
-    expr instanceof NotExpr or
-    expr instanceof ComplementExpr or
-    expr instanceof UnaryPlusExpr or
+    expr instanceof ComplementExpr
+    or
+    expr instanceof UnaryPlusExpr
+    or
     expr instanceof UnaryMinusExpr
   }
 
@@ -1298,8 +1308,6 @@ class TranslatedUnaryExpr extends TranslatedSingleInstructionExpr {
   }
 
   final override Opcode getOpcode() {
-    expr instanceof NotExpr and result instanceof Opcode::LogicalNot
-    or
     expr instanceof ComplementExpr and result instanceof Opcode::BitComplement
     or
     expr instanceof UnaryPlusExpr and result instanceof Opcode::CopyValue
@@ -1312,6 +1320,100 @@ class TranslatedUnaryExpr extends TranslatedSingleInstructionExpr {
   }
 }
 
+/**
+ * The IR translation of a `NotExpr`.
+ *
+ * In C++ an operation such as `!x` where `x` is an `int` will generate
+ * ```
+ * r1(glval<int>) = VariableAddress[x] :
+ * r2(int)        = Load               : &r1
+ * r3(int)        = Constant[0]        :
+ * r4(bool)       = CompareNE          : r2, r3
+ * r5(bool)       = LogicalNot         : r4
+ * ```
+ * since C does not do implicit int-to-bool casts we need to generate the
+ * `Constant[0]`, `CompareNE`, and `LogicalNot` instructions manually, but
+ * we simplify this and generate `Constant[0]`, `CompareEQ` instead.
+ */
+class TranslatedNotExpr extends TranslatedNonConstantExpr {
+  override NotExpr expr;
+
+  override Type getExprType() { result instanceof BoolType }
+
+  private Type getOperandType() { result = this.getOperand().getExprType().getUnspecifiedType() }
+
+  predicate shouldGenerateEq() { not this.getOperandType() instanceof BoolType }
+
+  final override Instruction getFirstInstruction(EdgeKind kind) {
+    result = this.getOperand().getFirstInstruction(kind)
+  }
+
+  override Instruction getALastInstructionInternal() {
+    result = this.getInstruction(NotExprOperationTag())
+  }
+
+  final override TranslatedElement getChildInternal(int id) {
+    id = 0 and result = this.getOperand()
+  }
+
+  override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType resultType) {
+    this.shouldGenerateEq() and
+    tag = NotExprConstantTag() and
+    opcode instanceof Opcode::Constant and
+    resultType = getTypeForPRValue(this.getOperandType())
+    or
+    resultType = getBoolType() and
+    tag = NotExprOperationTag() and
+    if this.shouldGenerateEq()
+    then opcode instanceof Opcode::CompareEQ
+    else opcode instanceof Opcode::LogicalNot
+  }
+
+  final override Instruction getInstructionSuccessorInternal(InstructionTag tag, EdgeKind kind) {
+    tag = NotExprOperationTag() and
+    result = this.getParent().getChildSuccessor(this, kind)
+    or
+    tag = NotExprConstantTag() and
+    kind instanceof GotoEdge and
+    result = this.getInstruction(NotExprOperationTag())
+  }
+
+  final override Instruction getChildSuccessorInternal(TranslatedElement child, EdgeKind kind) {
+    child = this.getOperand() and
+    kind instanceof GotoEdge and
+    if this.shouldGenerateEq()
+    then result = this.getInstruction(NotExprConstantTag())
+    else result = this.getInstruction(NotExprOperationTag())
+  }
+
+  final override Instruction getInstructionRegisterOperand(InstructionTag tag, OperandTag operandTag) {
+    tag = NotExprOperationTag() and
+    if this.shouldGenerateEq()
+    then (
+      result = this.getOperand().getResult() and
+      operandTag instanceof LeftOperandTag
+      or
+      result = this.getInstruction(NotExprConstantTag()) and
+      operandTag instanceof RightOperandTag
+    ) else (
+      operandTag instanceof UnaryOperandTag and
+      result = this.getOperand().getResult()
+    )
+  }
+
+  private TranslatedExpr getOperand() {
+    result = getTranslatedExpr(expr.getOperand().getFullyConverted())
+  }
+
+  final override Instruction getResult() { result = this.getInstruction(NotExprOperationTag()) }
+
+  override string getInstructionConstantValue(InstructionTag tag) {
+    this.shouldGenerateEq() and
+    tag = NotExprConstantTag() and
+    result = "0"
+  }
+}
+
 /**
  * IR translation of a `co_await` or `co_yield` expression.
  *
@@ -1754,6 +1856,12 @@ class TranslatedBinaryOperation extends TranslatedSingleInstructionExpr {
     result = comparisonOpcode(expr)
   }
 
+  override Type getExprType() {
+    if exists(comparisonOpcode(expr))
+    then result instanceof BoolType
+    else result = super.getExprType()
+  }
+
   override int getInstructionElementSize(InstructionTag tag) {
     tag = OnlyInstructionTag() and
     exists(Opcode opcode |
@@ -2857,6 +2965,10 @@ class TranslatedBinaryConditionalExpr extends TranslatedConditionalExpr {
     result = this.getCondition().getFirstInstruction(kind)
   }
 
+  private Type getConditionType() {
+    result = this.getCondition().getExprType().getUnspecifiedType()
+  }
+
   override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType resultType) {
     super.hasInstruction(opcode, tag, resultType)
     or
@@ -2864,11 +2976,35 @@ class TranslatedBinaryConditionalExpr extends TranslatedConditionalExpr {
     tag = ValueConditionConditionalBranchTag() and
     opcode instanceof Opcode::ConditionalBranch and
     resultType = getVoidType()
+    or
+    exists(Type t |
+      t = this.getConditionType() and
+      not t instanceof BoolType
+    |
+      tag = ValueConditionConditionalConstantTag() and
+      opcode instanceof Opcode::Constant and
+      resultType = getTypeForPRValue(t)
+      or
+      tag = ValueConditionConditionalCompareTag() and
+      opcode instanceof Opcode::CompareNE and
+      resultType = getBoolType()
+    )
   }
 
   override Instruction getInstructionSuccessorInternal(InstructionTag tag, EdgeKind kind) {
     result = super.getInstructionSuccessorInternal(tag, kind)
     or
+    not this.getConditionType() instanceof BoolType and
+    (
+      tag = ValueConditionConditionalConstantTag() and
+      kind instanceof GotoEdge and
+      result = this.getInstruction(ValueConditionConditionalCompareTag())
+      or
+      tag = ValueConditionConditionalCompareTag() and
+      kind instanceof GotoEdge and
+      result = this.getInstruction(ValueConditionConditionalBranchTag())
+    )
+    or
     tag = ValueConditionConditionalBranchTag() and
     (
       kind instanceof TrueEdge and
@@ -2884,7 +3020,19 @@ class TranslatedBinaryConditionalExpr extends TranslatedConditionalExpr {
     or
     tag = ValueConditionConditionalBranchTag() and
     operandTag instanceof ConditionOperandTag and
-    result = this.getCondition().getResult()
+    if this.getConditionType() instanceof BoolType
+    then result = this.getCondition().getResult()
+    else result = this.getInstruction(ValueConditionConditionalCompareTag())
+    or
+    not this.getConditionType() instanceof BoolType and
+    tag = ValueConditionConditionalCompareTag() and
+    (
+      operandTag instanceof LeftOperandTag and
+      result = this.getCondition().getResult()
+      or
+      operandTag instanceof RightOperandTag and
+      result = this.getInstruction(ValueConditionConditionalConstantTag())
+    )
   }
 
   override Instruction getChildSuccessorInternal(TranslatedElement child, EdgeKind kind) {
@@ -2892,7 +3040,9 @@ class TranslatedBinaryConditionalExpr extends TranslatedConditionalExpr {
     or
     kind instanceof GotoEdge and
     child = this.getCondition() and
-    result = this.getInstruction(ValueConditionConditionalBranchTag())
+    if this.getConditionType() instanceof BoolType
+    then result = this.getInstruction(ValueConditionConditionalBranchTag())
+    else result = this.getInstruction(ValueConditionConditionalConstantTag())
   }
 
   private TranslatedExpr getCondition() {
@@ -2909,6 +3059,11 @@ class TranslatedBinaryConditionalExpr extends TranslatedConditionalExpr {
     // always converting the "then" operand to `bool`, which is almost always the wrong type.
     result = getTranslatedExpr(expr.getThen().getExplicitlyConverted())
   }
+
+  override string getInstructionConstantValue(InstructionTag tag) {
+    tag = ValueConditionConditionalConstantTag() and
+    result = "0"
+  }
 }
 
 /**