Merge pull request #20218 from MathiasVP/fix-guard-conditions-for-likely

C++: Improvements to `IRGuard`s

Author: jketema

cpp/ql/lib/change-notes/2025-08-13-guards.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The guards libraries (`semmle.code.cpp.controlflow.Guards` and `semmle.code.cpp.controlflow.IRGuards`) have been improved to recognize more guards.

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

@@ -936,6 +936,77 @@ private module Cached {
     ValueNumber getUnary() { result.getAnInstruction() = instr.getUnary() }
   }
 
+  signature predicate sinkSig(Instruction instr);
+
+  private module BooleanInstruction<sinkSig/1 isSink> {
+    /**
+     * Holds if `i1` flows to `i2` in a single step and `i2` is not an
+     * instruction that produces a value of Boolean type.
+     */
+    private predicate stepToNonBoolean(Instruction i1, Instruction i2) {
+      not i2.getResultIRType() instanceof IRBooleanType and
+      (
+        i2.(CopyInstruction).getSourceValue() = i1
+        or
+        i2.(ConvertInstruction).getUnary() = i1
+        or
+        i2.(BuiltinExpectCallInstruction).getArgument(0) = i1
+      )
+    }
+
+    private predicate rev(Instruction instr) {
+      isSink(instr)
+      or
+      exists(Instruction instr1 |
+        rev(instr1) and
+        stepToNonBoolean(instr, instr1)
+      )
+    }
+
+    private predicate hasBooleanType(Instruction instr) {
+      instr.getResultIRType() instanceof IRBooleanType
+    }
+
+    private predicate fwd(Instruction instr) {
+      rev(instr) and
+      (
+        hasBooleanType(instr)
+        or
+        exists(Instruction instr0 |
+          fwd(instr0) and
+          stepToNonBoolean(instr0, instr)
+        )
+      )
+    }
+
+    private predicate prunedStep(Instruction i1, Instruction i2) {
+      fwd(i1) and
+      fwd(i2) and
+      stepToNonBoolean(i1, i2)
+    }
+
+    private predicate stepsPlus(Instruction i1, Instruction i2) =
+      doublyBoundedFastTC(prunedStep/2, hasBooleanType/1, isSink/1)(i1, i2)
+
+    /**
+     * Gets the Boolean-typed instruction that defines `instr` before any
+     * integer conversions are applied, if any.
+     */
+    Instruction get(Instruction instr) {
+      isSink(instr) and
+      (
+        result = instr
+        or
+        stepsPlus(result, instr)
+      ) and
+      hasBooleanType(result)
+    }
+  }
+
+  private predicate isUnaryComparesEqLeft(Instruction instr) {
+    unary_compares_eq(_, instr.getAUse(), 0, _, _)
+  }
+
   /**
    * Holds if `left == right + k` is `areEqual` given that test is `testIsTrue`.
    *
@@ -966,14 +1037,19 @@ private module Cached {
     )
     or
     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())
+    exists(Operand l, BooleanValue bv |
+      // 1. test = value -> int(l) = 0 is !bv
+      unary_compares_eq(test, l, 0, bv.getValue().booleanNot(), value) and
+      // 2. l = bv -> left + right is areEqual
+      compares_eq(valueNumber(BooleanInstruction<isUnaryComparesEqLeft/1>::get(l.getDef())), left,
+        right, k, areEqual, bv)
+      // We want this to hold:
+      // `test = value -> left + right is areEqual`
+      // Applying 2 we need to show:
+      // `test = value -> l = bv`
+      // And `l = bv` holds by `int(l) = 0 is !bv`
+    )
   }
 
   /**
@@ -1006,19 +1082,11 @@ private module Cached {
       k = k1 + k2
     )
     or
-    exists(CompareValueNumber cmp, Operand left, Operand right, AbstractValue v |
-      test = cmp and
-      pragma[only_bind_into](cmp)
-          .hasOperands(pragma[only_bind_into](left), pragma[only_bind_into](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
+    // See argument for why this is correct in compares_eq
+    exists(Operand l, BooleanValue bv |
+      unary_compares_eq(test, l, 0, bv.getValue().booleanNot(), value) and
+      unary_compares_eq(valueNumber(BooleanInstruction<isUnaryComparesEqLeft/1>::get(l.getDef())),
+        op, k, areEqual, bv)
     )
     or
     unary_compares_eq(test.(BuiltinExpectCallValueNumber).getCondition(), op, k, areEqual, value)
@@ -1116,70 +1184,26 @@ private module Cached {
     )
   }
 
+  private predicate isBuiltInExpectArg(Instruction instr) {
+    instr = any(BuiltinExpectCallInstruction buildinExpect).getArgument(0)
+  }
+
   /** A call to the builtin operation `__builtin_expect`. */
   private class BuiltinExpectCallInstruction extends CallInstruction {
     BuiltinExpectCallInstruction() { this.getStaticCallTarget().hasName("__builtin_expect") }
 
     /** Gets the condition of this call. */
-    Instruction getCondition() { result = this.getConditionOperand().getDef() }
-
-    Operand getConditionOperand() {
-      // The first parameter of `__builtin_expect` has type `long`. So we skip
-      // the conversion when inferring guards.
-      result = this.getArgument(0).(ConvertInstruction).getUnaryOperand()
+    Instruction getCondition() {
+      result = BooleanInstruction<isBuiltInExpectArg/1>::get(this.getArgument(0))
     }
   }
 
-  /**
-   * Holds if `left == right + k` is `areEqual` if `cmp` evaluates to `value`,
-   * and `cmp` is an instruction that compares the value of
-   * `__builtin_expect(left == right + k, _)` to `0`.
-   */
-  private predicate builtin_expect_eq(
-    CompareValueNumber cmp, Operand left, Operand right, 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
-      compares_eq(call.getCondition(), left, right, k, areEqual, innerValue)
-    |
-      cmp instanceof CompareNEValueNumber and
-      value = innerValue
-      or
-      cmp instanceof CompareEQValueNumber and
-      value.getDualValue() = innerValue
-    )
-  }
-
   private predicate complex_eq(
     ValueNumber cmp, Operand left, Operand right, int k, boolean areEqual, AbstractValue value
   ) {
     sub_eq(cmp, left, right, k, areEqual, value)
     or
     add_eq(cmp, left, right, k, areEqual, value)
-    or
-    builtin_expect_eq(cmp, left, right, k, areEqual, value)
-  }
-
-  /**
-   * Holds if `op == k` is `areEqual` if `cmp` evaluates to `value`, and `cmp` is
-   * 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, 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, innerValue)
-    |
-      cmp instanceof CompareNEValueNumber and
-      value = innerValue
-      or
-      cmp instanceof CompareEQValueNumber and
-      value.getDualValue() = innerValue
-    )
   }
 
   private predicate unary_complex_eq(
@@ -1188,8 +1212,6 @@ private module Cached {
     unary_sub_eq(test, op, k, areEqual, value)
     or
     unary_add_eq(test, op, k, areEqual, value)
-    or
-    unary_builtin_expect_eq(test, op, k, areEqual, value)
   }
 
   /*
@@ -1215,6 +1237,15 @@ private module Cached {
     exists(AbstractValue dual | value = dual.getDualValue() |
       compares_lt(test.(LogicalNotValueNumber).getUnary(), left, right, k, isLt, dual)
     )
+    or
+    compares_lt(test.(BuiltinExpectCallValueNumber).getCondition(), left, right, k, isLt, value)
+    or
+    // See argument for why this is correct in compares_eq
+    exists(Operand l, BooleanValue bv |
+      unary_compares_eq(test, l, 0, bv.getValue().booleanNot(), value) and
+      compares_lt(valueNumber(BooleanInstruction<isUnaryComparesEqLeft/1>::get(l.getDef())), left,
+        right, k, isLt, bv)
+    )
   }
 
   /** Holds if `op < k` evaluates to `isLt` given that `test` evaluates to `value`. */
@@ -1234,6 +1265,15 @@ private module Cached {
       int_value(const) = k1 and
       k = k1 + k2
     )
+    or
+    compares_lt(test.(BuiltinExpectCallValueNumber).getCondition(), op, k, isLt, value)
+    or
+    // See argument for why this is correct in compares_eq
+    exists(Operand l, BooleanValue bv |
+      unary_compares_eq(test, l, 0, bv.getValue().booleanNot(), value) and
+      compares_lt(valueNumber(BooleanInstruction<isUnaryComparesEqLeft/1>::get(l.getDef())), op, k,
+        isLt, bv)
+    )
   }
 
   /** `(a < b + k) => (b > a - k) => (b >= a + (1-k))` */

cpp/ql/test/library-tests/controlflow/guards/Guards.expected

@@ -44,6 +44,8 @@
 | test.c:198:8:198:8 | b |
 | test.c:206:7:206:8 | ! ... |
 | test.c:206:8:206:8 | c |
+| test.c:215:6:215:18 | call to __builtin_expect |
+| test.c:219:9:219:22 | call to __builtin_expect |
 | test.cpp:18:8:18:10 | call to get |
 | test.cpp:31:7:31:13 | ... == ... |
 | test.cpp:42:13:42:20 | call to getABool |
@@ -92,3 +94,15 @@
 | test.cpp:241:9:241:43 | ... && ... |
 | test.cpp:241:22:241:30 | ... == ... |
 | test.cpp:241:35:241:43 | ... == ... |
+| test.cpp:247:6:247:18 | ... == ... |
+| test.cpp:253:6:253:18 | ... != ... |
+| test.cpp:260:6:260:18 | ... == ... |
+| test.cpp:266:6:266:18 | ... != ... |
+| test.cpp:273:6:273:17 | ... == ... |
+| test.cpp:279:6:279:17 | ... != ... |
+| test.cpp:287:6:287:19 | ... == ... |
+| test.cpp:293:6:293:19 | ... != ... |
+| test.cpp:300:6:300:19 | ... == ... |
+| test.cpp:306:6:306:19 | ... != ... |
+| test.cpp:312:6:312:18 | ... == ... |
+| test.cpp:318:6:318:18 | ... != ... |