Merge branch 'main' into main

Author: mbaluda

cpp/ql/lib/change-notes/2024-10-06-builtin-expect.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The "Guards" library (`semmle.code.cpp.controlflow.Guards`) now also infers guards from calls to the builtin operation `__builtin_expect`. As a result, some queries may produce fewer false positives.

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

@@ -375,6 +375,33 @@ cached
 class IRGuardCondition extends Instruction {
   Instruction branch;
 
+  /*
+   * An `IRGuardCondition` supports reasoning about four different kinds of
+   * relations:
+   * 1. A unary equality relation of the form `e == k`
+   * 2. A binary equality relation of the form `e1 == e2 + k`
+   * 3. A unary inequality relation of the form `e < k`
+   * 4. A binary inequality relation of the form `e1 < e2 + k`
+   *
+   * where `k` is a constant.
+   *
+   * Furthermore, the unary relations (i.e., case 1 and case 3) are also
+   * inferred from `switch` statement guards: equality relations are inferred
+   * from the unique `case` statement, if any, and inequality relations are
+   * inferred from the [case range](https://gcc.gnu.org/onlinedocs/gcc/Case-Ranges.html)
+   * gcc extension.
+   *
+   * The implementation of all four follows the same structure: Each relation
+   * has a cached user-facing predicate that. For example,
+   * `GuardCondition::comparesEq` calls `compares_eq`. This predicate has
+   * several cases that recursively decompose the relation to bring it to a
+   * canonical form (i.e., a relation of the form `e1 == e2 + k`). The base
+   * case for this relation (i.e., `simple_comparison_eq`) handles
+   * `CompareEQInstruction`s and `CompareNEInstruction`, and recursive
+   * predicates (e.g., `complex_eq`) rewrites larger expressions such as
+   * `e1 + k1 == e2 + k2` into canonical the form `e1 == e2 + (k2 - k1)`.
+   */
+
   cached
   IRGuardCondition() { branch = getBranchForCondition(this) }
 
@@ -735,6 +762,8 @@ private predicate compares_eq(
   exists(AbstractValue dual | value = dual.getDualValue() |
     compares_eq(test.(LogicalNotInstruction).getUnary(), left, right, k, areEqual, dual)
   )
+  or
+  compares_eq(test.(BuiltinExpectCallInstruction).getCondition(), left, right, k, areEqual, value)
 }
 
 /**
@@ -776,7 +805,9 @@ 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, op, k, inNonZeroCase, v) |
+  exists(AbstractValue v |
+    unary_simple_comparison_eq(test, k, inNonZeroCase, v) and op.getDef() = test
+  |
     areEqual = true and value = v
     or
     areEqual = false and value = v.getDualValue()
@@ -802,6 +833,9 @@ private predicate unary_compares_eq(
     int_value(const) = k1 and
     k = k1 + k2
   )
+  or
+  unary_compares_eq(test.(BuiltinExpectCallInstruction).getCondition(), op, k, areEqual,
+    inNonZeroCase, value)
 }
 
 /** Rearrange various simple comparisons into `left == right + k` form. */
@@ -821,45 +855,55 @@ private predicate simple_comparison_eq(
   value.(BooleanValue).getValue() = false
 }
 
-/**
- * Holds if `test` is an instruction that is part of test that eventually is
- * used in a conditional branch.
- */
-private predicate relevantUnaryComparison(Instruction test) {
-  not test instanceof CompareInstruction and
-  exists(IRType type, ConditionalBranchInstruction branch |
-    type instanceof IRAddressType or type instanceof IRIntegerType
-  |
-    type = test.getResultIRType() and
-    branch.getCondition() = test
-  )
-  or
-  exists(LogicalNotInstruction logicalNot |
-    relevantUnaryComparison(logicalNot) and
-    test = logicalNot.getUnary()
-  )
-}
-
 /**
  * Rearrange various simple comparisons into `op == k` form.
  */
 private predicate unary_simple_comparison_eq(
-  Instruction test, Operand op, int k, boolean inNonZeroCase, AbstractValue value
+  Instruction test, int k, boolean inNonZeroCase, AbstractValue value
 ) {
   exists(SwitchInstruction switch, CaseEdge case |
     test = switch.getExpression() and
-    op.getDef() = test and
     case = value.(MatchValue).getCase() and
     exists(switch.getSuccessor(case)) and
     case.getValue().toInt() = k and
     inNonZeroCase = false
   )
   or
-  // There's no implicit CompareInstruction in files compiled as C since C
-  // doesn't have implicit boolean conversions. So instead we check whether
-  // there's a branch on a value of pointer or integer type.
-  relevantUnaryComparison(test) and
-  op.getDef() = test 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
+  // already included as guards in other cases, we exclude those here.
+  // These are instructions that compute a binary equality or inequality
+  // relation. For example, the following:
+  // ```cpp
+  // if(a == b + 42) { ... }
+  // ```
+  // generates the following IR:
+  // ```
+  // r1(glval<int>) = VariableAddress[a]     :
+  // r2(int)        = Load[a]                : &:r1, m1
+  // r3(glval<int>) = VariableAddress[b]     :
+  // r4(int)        = Load[b]                : &:r3, m2
+  // r5(int)        = Constant[42]           :
+  // r6(int)        = Add                    : r4, r5
+  // r7(bool)       = CompareEQ              : r2, r6
+  // v1(void)       = ConditionalBranch      : r7
+  // ```
+  // and since `r7` is an integral typed instruction this predicate could
+  // include a case for when `r7` evaluates to true (in which case we would
+  // infer that `r6` was non-zero, and a case for when `r7` evaluates to false
+  // (in which case we would infer that `r6` was zero).
+  // However, since `a == b + 42` is already supported when reasoning about
+  // binary equalities we exclude those cases here.
+  not test.isGLValue() and
+  not simple_comparison_eq(test, _, _, _, _) and
+  not simple_comparison_lt(test, _, _, _) and
+  not test = any(SwitchInstruction switch).getExpression() and
+  (
+    test.getResultIRType() instanceof IRAddressType or
+    test.getResultIRType() instanceof IRIntegerType or
+    test.getResultIRType() instanceof IRBooleanType
+  ) and
   (
     k = 1 and
     value.(BooleanValue).getValue() = true and
@@ -871,12 +915,68 @@ private predicate unary_simple_comparison_eq(
   )
 }
 
+/** 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() {
+    // The first parameter of `__builtin_expect` has type `long`. So we skip
+    // the conversion when inferring guards.
+    result = this.getArgument(0).(ConvertInstruction).getUnary()
+  }
+}
+
+/**
+ * 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(
+  CompareInstruction cmp, Operand left, Operand right, int k, boolean areEqual, AbstractValue value
+) {
+  exists(BuiltinExpectCallInstruction 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 CompareNEInstruction and
+    value = innerValue
+    or
+    cmp instanceof CompareEQInstruction and
+    value.getDualValue() = innerValue
+  )
+}
+
 private predicate complex_eq(
   CompareInstruction 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(
+  CompareInstruction cmp, Operand op, int k, boolean areEqual, boolean inNonZeroCase,
+  AbstractValue value
+) {
+  exists(BuiltinExpectCallInstruction 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)
+  |
+    cmp instanceof CompareNEInstruction and
+    value = innerValue
+    or
+    cmp instanceof CompareEQInstruction and
+    value.getDualValue() = innerValue
+  )
 }
 
 private predicate unary_complex_eq(
@@ -885,6 +985,8 @@ private predicate unary_complex_eq(
   unary_sub_eq(test, op, k, areEqual, inNonZeroCase, value)
   or
   unary_add_eq(test, op, k, areEqual, inNonZeroCase, value)
+  or
+  unary_builtin_expect_eq(test, op, k, areEqual, inNonZeroCase, value)
 }
 
 /*
@@ -913,7 +1015,8 @@ private predicate compares_lt(
 
 /** 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) {
-  simple_comparison_lt(test, op, k, isLt, value)
+  unary_simple_comparison_lt(test, k, isLt, value) and
+  op.getDef() = test
   or
   complex_lt(test, op, k, isLt, value)
   or
@@ -960,12 +1063,11 @@ private predicate simple_comparison_lt(CompareInstruction cmp, Operand left, Ope
 }
 
 /** Rearrange various simple comparisons into `op < k` form. */
-private predicate simple_comparison_lt(
-  Instruction test, Operand op, int k, boolean isLt, AbstractValue value
+private predicate unary_simple_comparison_lt(
+  Instruction test, int k, boolean isLt, AbstractValue value
 ) {
   exists(SwitchInstruction switch, CaseEdge case |
     test = switch.getExpression() and
-    op.getDef() = test and
     case = value.(MatchValue).getCase() and
     exists(switch.getSuccessor(case)) and
     case.getMaxValue() > case.getMinValue()

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

@@ -74,6 +74,8 @@ astGuardsCompare
 | 34 | j >= 10+0 when ... < ... is false |
 | 42 | 10 < j+1 when ... < ... is false |
 | 42 | 10 >= j+1 when ... < ... is true |
+| 42 | call to getABool != 0 when call to getABool is true |
+| 42 | call to getABool == 0 when call to getABool is false |
 | 42 | j < 10+0 when ... < ... is true |
 | 42 | j >= 10+0 when ... < ... is false |
 | 44 | 0 < z+0 when ... > ... is true |
@@ -537,6 +539,8 @@ astGuardsEnsure_const
 | test.cpp:31:7:31:13 | ... == ... | test.cpp:31:7:31:7 | x | != | -1 | 34 | 34 |
 | test.cpp:31:7:31:13 | ... == ... | test.cpp:31:7:31:7 | x | == | -1 | 30 | 30 |
 | test.cpp:31:7:31:13 | ... == ... | test.cpp:31:7:31:7 | x | == | -1 | 31 | 32 |
+| test.cpp:42:13:42:20 | call to getABool | test.cpp:42:13:42:20 | call to getABool | != | 0 | 43 | 45 |
+| test.cpp:42:13:42:20 | call to getABool | test.cpp:42:13:42:20 | call to getABool | == | 0 | 53 | 53 |
 irGuards
 | test.c:7:9:7:13 | CompareGT: ... > ... |
 | test.c:17:8:17:12 | CompareLT: ... < ... |
@@ -613,6 +617,8 @@ irGuardsCompare
 | 34 | j >= 10+0 when CompareLT: ... < ... is false |
 | 42 | 10 < j+1 when CompareLT: ... < ... is false |
 | 42 | 10 >= j+1 when CompareLT: ... < ... is true |
+| 42 | call to getABool != 0 when Call: call to getABool is true |
+| 42 | call to getABool == 0 when Call: call to getABool is false |
 | 42 | j < 10 when CompareLT: ... < ... is true |
 | 42 | j < 10+0 when CompareLT: ... < ... is true |
 | 42 | j >= 10 when CompareLT: ... < ... is false |
@@ -1081,3 +1087,5 @@ irGuardsEnsure_const
 | test.cpp:31:7:31:13 | CompareEQ: ... == ... | test.cpp:31:7:31:7 | Load: x | != | -1 | 34 | 34 |
 | test.cpp:31:7:31:13 | CompareEQ: ... == ... | test.cpp:31:7:31:7 | Load: x | == | -1 | 30 | 30 |
 | test.cpp:31:7:31:13 | CompareEQ: ... == ... | test.cpp:31:7:31:7 | Load: x | == | -1 | 32 | 32 |
+| test.cpp:42:13:42:20 | Call: call to getABool | test.cpp:42:13:42:20 | Call: call to getABool | != | 0 | 44 | 44 |
+| test.cpp:42:13:42:20 | Call: call to getABool | test.cpp:42:13:42:20 | Call: call to getABool | == | 0 | 53 | 53 |

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

@@ -42,3 +42,10 @@
 | test.cpp:99:6:99:6 | f |
 | test.cpp:105:6:105:14 | ... != ... |
 | test.cpp:111:6:111:14 | ... != ... |
+| test.cpp:122:9:122:9 | b |
+| test.cpp:125:13:125:20 | ! ... |
+| test.cpp:125:14:125:17 | call to safe |
+| test.cpp:131:6:131:21 | call to __builtin_expect |
+| test.cpp:135:6:135:21 | call to __builtin_expect |
+| test.cpp:141:6:141:21 | call to __builtin_expect |
+| test.cpp:145:6:145:21 | call to __builtin_expect |

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

@@ -44,6 +44,8 @@
 | 34 | j >= 10+0 when ... < ... is false |
 | 42 | 10 < j+1 when ... < ... is false |
 | 42 | 10 >= j+1 when ... < ... is true |
+| 42 | call to getABool != 0 when call to getABool is true |
+| 42 | call to getABool == 0 when call to getABool is false |
 | 42 | j < 10 when ... < ... is true |
 | 42 | j < 10+0 when ... < ... is true |
 | 42 | j >= 10 when ... < ... is false |
@@ -149,16 +151,59 @@
 | 111 | 0.0 == i+0 when ... != ... is false |
 | 111 | i != 0.0+0 when ... != ... is true |
 | 111 | i == 0.0+0 when ... != ... is false |
+| 122 | b != 0 when b is true |
+| 122 | b == 0 when b is false |
+| 125 | ! ... != 0 when ! ... is true |
+| 125 | ! ... == 0 when ! ... is false |
+| 125 | call to safe != 0 when ! ... is false |
+| 125 | call to safe != 0 when call to safe is true |
+| 125 | call to safe == 0 when call to safe is false |
 | 126 | 1 != 0 when 1 is true |
 | 126 | 1 != 0 when ... && ... is true |
 | 126 | 1 == 0 when 1 is false |
 | 126 | call to test3_condition != 0 when ... && ... is true |
 | 126 | call to test3_condition != 0 when call to test3_condition is true |
 | 126 | call to test3_condition == 0 when call to test3_condition is false |
+| 131 | ... + ... != a+0 when call to __builtin_expect is false |
+| 131 | ... + ... == a+0 when call to __builtin_expect is true |
+| 131 | a != ... + ...+0 when call to __builtin_expect is false |
+| 131 | a != b+42 when call to __builtin_expect is false |
+| 131 | a == ... + ...+0 when call to __builtin_expect is true |
+| 131 | a == b+42 when call to __builtin_expect is true |
 | 131 | b != 0 when b is true |
+| 131 | b != a+-42 when call to __builtin_expect is false |
 | 131 | b == 0 when b is false |
+| 131 | b == a+-42 when call to __builtin_expect is true |
+| 131 | call to __builtin_expect != 0 when call to __builtin_expect is true |
+| 131 | call to __builtin_expect == 0 when call to __builtin_expect is false |
+| 135 | ... + ... != a+0 when call to __builtin_expect is true |
+| 135 | ... + ... == a+0 when call to __builtin_expect is false |
+| 135 | a != ... + ...+0 when call to __builtin_expect is true |
+| 135 | a != b+42 when call to __builtin_expect is true |
+| 135 | a == ... + ...+0 when call to __builtin_expect is false |
+| 135 | a == b+42 when call to __builtin_expect is false |
+| 135 | b != a+-42 when call to __builtin_expect is true |
+| 135 | b == a+-42 when call to __builtin_expect is false |
+| 135 | call to __builtin_expect != 0 when call to __builtin_expect is true |
+| 135 | call to __builtin_expect == 0 when call to __builtin_expect is false |
 | 137 | 0 != 0 when 0 is true |
 | 137 | 0 == 0 when 0 is false |
+| 141 | 42 != a+0 when call to __builtin_expect is false |
+| 141 | 42 == a+0 when call to __builtin_expect is true |
+| 141 | a != 42 when call to __builtin_expect is false |
+| 141 | a != 42+0 when call to __builtin_expect is false |
+| 141 | a == 42 when call to __builtin_expect is true |
+| 141 | a == 42+0 when call to __builtin_expect is true |
+| 141 | call to __builtin_expect != 0 when call to __builtin_expect is true |
+| 141 | call to __builtin_expect == 0 when call to __builtin_expect is false |
+| 145 | 42 != a+0 when call to __builtin_expect is true |
+| 145 | 42 == a+0 when call to __builtin_expect is false |
+| 145 | a != 42 when call to __builtin_expect is true |
+| 145 | a != 42+0 when call to __builtin_expect is true |
+| 145 | a == 42 when call to __builtin_expect is false |
+| 145 | a == 42+0 when call to __builtin_expect is false |
+| 145 | call to __builtin_expect != 0 when call to __builtin_expect is true |
+| 145 | call to __builtin_expect == 0 when call to __builtin_expect is false |
 | 146 | ! ... != 0 when ! ... is true |
 | 146 | ! ... == 0 when ! ... is false |
 | 146 | x != 0 when ! ... is false |

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

@@ -100,3 +100,11 @@
 | test.cpp:99:6:99:6 | f | true | 99 | 100 |
 | test.cpp:105:6:105:14 | ... != ... | true | 105 | 106 |
 | test.cpp:111:6:111:14 | ... != ... | true | 111 | 112 |
+| test.cpp:122:9:122:9 | b | true | 123 | 125 |
+| test.cpp:122:9:122:9 | b | true | 125 | 125 |
+| test.cpp:125:13:125:20 | ! ... | true | 125 | 125 |
+| test.cpp:125:14:125:17 | call to safe | false | 125 | 125 |
+| test.cpp:131:6:131:21 | call to __builtin_expect | true | 131 | 132 |
+| test.cpp:135:6:135:21 | call to __builtin_expect | true | 135 | 136 |
+| test.cpp:141:6:141:21 | call to __builtin_expect | true | 141 | 142 |
+| test.cpp:145:6:145:21 | call to __builtin_expect | true | 145 | 146 |