Merge remote-tracking branch 'origin/main' into michaelrfairhurst/side-effects-6-rule-28-3-1-predicate-side-effects

Author: MichaelRFairhurst

.github/copilot-instructions.md

@@ -0,0 +1,60 @@
+---
+description: 'Code review guidelines for GitHub copilot in this project'
+applyTo: '**'
+excludeAgent: ["coding-agent"]
+---
+
+# Code Review Instructions
+
+A change note is required for any pull request which modifies:
+- The structure or layout of the release artifacts.
+- The evaluation performance (memory, execution time) of an existing query.
+- The results of an existing query in any circumstance.
+
+If the pull request only adds new rule queries, a change note is not required.
+Confirm that either a change note is not required or the change note is required and has been added.
+
+For PRs that add new queries or modify existing queries, also consider the following review checklist:
+- Confirm that the output format of shared queries is valid.
+- Have all the relevant rule package description files been checked in?
+- Have you verified that the metadata properties of each new query is set appropriately?
+- Do all the unit tests contain both "COMPLIANT" and "NON_COMPLIANT" cases?
+- Are all the alerts in the expected file annotated as NON_COMPLIANT in the test source file?
+- Are the alert messages properly formatted and consistent with the style guide?
+- Does the query have an appropriate level of in-query comments/documentation?
+- Does the query not reinvent features in the standard library?
+- Can the query be simplified further (not golfed!).
+
+In your review output, list only those checklist items that are not satisfied or are uncertain, but also report any other problems you find outside this checklist; do not mention checklist items that clearly pass.
+
+## Validating tests and .expected files
+
+The test infrastructure for CodeQL that we use in this project involves the creation of a test directory with the following structure:
+- Test root is `some/path/test/path/to/feature` (mirrors `some/path/src/path/to/query`)
+- At least one test `c` or `c++` file, typically named `test.c`/`test.cpp`, with lines annotated `// COMPLIANT` or `// NON_COMPLIANT`
+- A `.ql` file with test query logic, or a `.qlref` file referring to the production query logic
+- A matching `FOO.expected` file to go with each `FOO.ql` or `FOO.qlref`, containing the test query results for the test `c` or `c++` files
+- Note that some test directories simply have a `testref` file, to document that a certain query is tested in a different directory.
+
+As a code reviewer, it is critical to ensure that the results in the `.expected` file match the comments in the test file.
+
+The `.expected` file uses a columnar format:
+- For example, a basic row may look like `| test.cpp:8:22:8:37 | element | message |`.
+- For a query with `select x, "test"`, the columns are | x.getLocation() | x.toString() | "test" |`
+- An alert with placeholders will use `$@` in the message, and have additional `element`/`string` columns for placeholder, e.g. `| test.cpp:8:22:8:37 | ... + ... | Invalid add of $@. | test.cpp:7:5:7:12 | my_var | deprecated variable my_var |`.
+- Remember, there is one `.expected` file for each `.ql` or `.qlref` file.
+- Each `.expected` file will contain the results for all test c/cpp files.
+- The `toString()` format of QL objects is deliberately terse for performance reasons.
+- For certain queries such as "path problems", the results may be grouped into categories via text lines with the category name, e.g. `nodes` and `edges` and `problems`.
+
+Reviewing tests in this style can be tedious and error prone, but fundamental to the effectiveness of our TDD requirements in this project.
+
+When reviewing tests, it is critical to:
+- Check that each `NON_COMPLIANT` case in the test file has a row in the correct `.expected` file referring to the correct location.
+- Check that each row in each `.expected` file has a `NON_COMPLIANT` case in the test file at the correct location.
+- Check that there are no `.expected` rows that refer to test code cases marked as `COMPLIANT`, or with no comment
+- Note that it is OK if the locations of the comment are not precisely aligned with the alert 
+- Check that the alert message and placeholders are accurate and understandable.
+- Check that the locations do not refer to files in the standard library, as these have issues in GitHub's Code Scanning UI and complicate our compiler compatibility tests.
+- Consider the "test coverage" of the query, are each of its logical statements effectively exercised  individually, collectively? The test should neither be overly bloated nor under specified.
+- Consider the edge cases of the language itself, will the analysis work in non-trivial cases, are all relevant language concepts tested here? This doesn't need to be exhaustive, but it should be thoughfully thorough.

c/cert/src/rules/EXP30-C/DependenceOnOrderOfScalarEvaluationForSideEffects.ql

@@ -21,15 +21,14 @@ import codingstandards.c.cert
 import codingstandards.cpp.SideEffect
 import codingstandards.c.Ordering
 import codingstandards.c.orderofevaluation.VariableAccessOrdering
+import Ordering::Make<VariableAccessInFullExpressionOrdering> as FullExpressionOrdering
 
-from
-  VariableAccessInFullExpressionOrdering config, FullExpr e, ScalarVariable v, VariableEffect ve,
-  VariableAccess va1, VariableAccess va2
+from FullExpr e, ScalarVariable v, VariableEffect ve, VariableAccess va1, VariableAccess va2
 where
   not isExcluded(e, SideEffects1Package::dependenceOnOrderOfScalarEvaluationForSideEffectsQuery()) and
   e = va1.(ConstituentExpr).getFullExpr() and
   va1 = ve.getAnAccess() and
-  config.isUnsequenced(va1, va2) and
+  FullExpressionOrdering::isUnsequenced(va1, va2) and
   v = va1.getTarget()
 select e, "Scalar object referenced by $@ has a $@ that is unsequenced in relative to another $@.",
   v, v.getName(), ve, "side-effect", va2, "side-effect or value computation"

c/common/src/codingstandards/c/Objects.qll

@@ -1,5 +1,5 @@
 import cpp
-import codingstandards.c.StorageDuration
+import codingstandards.cpp.lifetimes.StorageDuration
 import codingstandards.c.IdentifierLinkage
 import semmle.code.cpp.valuenumbering.HashCons
 import codingstandards.cpp.Clvalues

c/common/src/codingstandards/c/Ordering.qll

@@ -1,97 +1,84 @@
 import cpp
-import codingstandards.cpp.SideEffect
 import codingstandards.c.Expr
 import codingstandards.cpp.Variable
 
 module Ordering {
-  abstract class Configuration extends string {
-    bindingset[this]
-    Configuration() { any() }
+  private import codingstandards.cpp.Ordering as CppCommonOrdering
+  import CppCommonOrdering::OrderingBase
 
-    abstract predicate isCandidate(Expr e1, Expr e2);
+  module CConfigBase {
+    class EvaluationNode = Expr;
 
-    /**
-     * Holds if `e1` is sequenced before `e2` as defined by Annex C in ISO/IEC 9899:2011
-     * This limits to expression and we do not consider the sequence points that are not amenable to modelling:
-     * - before a library function returns (see 7.1.4 point 3).
-     * - after the actions associated with each formatted I/O function conversion specifier (see 7.21.6 point 1 & 7.29.2 point 1).
-     * - between the expr before and after a call to a comparison function,
-     *   between any call to a comparison function, and any movement of the objects passed
-     *   as arguments to that call (see 7.22.5 point 5).
-     */
-    predicate isSequencedBefore(Expr e1, Expr e2) {
-      isCandidate(e1, e2) and
-      not e1 = e2 and
+    pragma[inline]
+    Expr toExpr(Expr e) { result = e }
+
+    pragma[inline]
+    predicate sequencingEdge(Expr e1, Expr e2) { c11Ordering(e1, e2) }
+  }
+
+  pragma[inline]
+  predicate c11Ordering(Expr e1, Expr e2) {
+    // 6.5.2.2 point 10 - The evaluation of the function designator and the actual arguments are sequenced
+    // before the actual call.
+    exists(Call call |
       (
-        // 6.5.2.2 point 10 - The evaluation of the function designator and the actual arguments are sequenced
-        // before the actual call.
-        exists(Call call |
-          (
-            call.getAnArgument().getAChild*() = e1
-            or
-            // Postfix expression designating the called function
-            // We current only handle call through function pointers because the postfix expression
-            // of regular function calls is not available. That is, identifying `f` in `f(...)`
-            call.(ExprCall).getExpr().getAChild*() = e1
-          ) and
-          call.getTarget() = e2.getEnclosingFunction()
-        )
-        or
-        // 6.5.13 point 4 & 6.5.14 point 4 - The operators guarantee left-to-right evaluation and there is
-        // a sequence point between the first and second operand if the latter is evaluated.
-        exists(BinaryLogicalOperation blop |
-          blop instanceof LogicalAndExpr or blop instanceof LogicalOrExpr
-        |
-          blop.getLeftOperand().getAChild*() = e1 and blop.getRightOperand().getAChild*() = e2
-        )
-        or
-        // 6.5.17 point 2 - There is a sequence point between the left operand and the right operand.
-        exists(CommaExpr ce, Expr lhs, Expr rhs |
-          lhs = ce.getLeftOperand() and
-          rhs = ce.getRightOperand()
-        |
-          lhs.getAChild*() = e1 and rhs.getAChild*() = e2
-        )
+        call.getAnArgument().getAChild*() = e1
         or
-        // 6.5.15 point 4 - There is a sequence point between the first operand and the evaluation of the second or third.
-        exists(ConditionalExpr cond |
-          cond.getCondition().getAChild*() = e1 and
-          (cond.getThen().getAChild*() = e2 or cond.getElse().getAChild*() = e2)
-        )
-        or
-        // Between the evaluation of a full expression and the next to be evaluated full expression.
-        // Note we don't strictly check if `e2` is the next to be evaluated full expression and rely on the
-        // `isCandidate` configuration to minimze the scope or related full expressions.
-        e1 instanceof FullExpr and e2 instanceof FullExpr
-        or
-        // The side effect of updating the stored value of the left operand is sequenced after the value computations of the left and right operands.
-        // See 6.5.16
-        e2.(Assignment).getAnOperand().getAChild*() = e1
-        or
-        // There is a sequence point after a full declarator as described in 6.7.6 point 3.
-        exists(DeclStmt declStmt, int i, int j | i < j |
-          declStmt
-              .getDeclarationEntry(i)
-              .(VariableDeclarationEntry)
-              .getVariable()
-              .getInitializer()
-              .getExpr()
-              .getAChild*() = e1 and
-          declStmt
-              .getDeclarationEntry(j)
-              .(VariableDeclarationEntry)
-              .getVariable()
-              .getInitializer()
-              .getExpr()
-              .getAChild*() = e2
-        )
-      )
-    }
-
-    predicate isUnsequenced(Expr e1, Expr e2) {
-      isCandidate(e1, e2) and
-      not isSequencedBefore(e1, e2) and
-      not isSequencedBefore(e2, e1)
-    }
+        // Postfix expression designating the called function
+        // We current only handle call through function pointers because the postfix expression
+        // of regular function calls is not available. That is, identifying `f` in `f(...)`
+        call.(ExprCall).getExpr().getAChild*() = e1
+      ) and
+      call.getTarget() = e2.getEnclosingFunction()
+    )
+    or
+    // 6.5.13 point 4 & 6.5.14 point 4 - The operators guarantee left-to-right evaluation and there is
+    // a sequence point between the first and second operand if the latter is evaluated.
+    exists(BinaryLogicalOperation blop |
+      blop instanceof LogicalAndExpr or blop instanceof LogicalOrExpr
+    |
+      blop.getLeftOperand().getAChild*() = e1 and blop.getRightOperand().getAChild*() = e2
+    )
+    or
+    // 6.5.17 point 2 - There is a sequence point between the left operand and the right operand.
+    exists(CommaExpr ce, Expr lhs, Expr rhs |
+      lhs = ce.getLeftOperand() and
+      rhs = ce.getRightOperand()
+    |
+      lhs.getAChild*() = e1 and rhs.getAChild*() = e2
+    )
+    or
+    // 6.5.15 point 4 - There is a sequence point between the first operand and the evaluation of the second or third.
+    exists(ConditionalExpr cond |
+      cond.getCondition().getAChild*() = e1 and
+      (cond.getThen().getAChild*() = e2 or cond.getElse().getAChild*() = e2)
+    )
+    or
+    // Between the evaluation of a full expression and the next to be evaluated full expression.
+    // Note we don't strictly check if `e2` is the next to be evaluated full expression and rely on the
+    // `isCandidate` configuration to minimze the scope or related full expressions.
+    e1 instanceof FullExpr and e2 instanceof FullExpr
+    or
+    // The side effect of updating the stored value of the left operand is sequenced after the value computations of the left and right operands.
+    // See 6.5.16
+    e2.(Assignment).getAnOperand().getAChild*() = e1
+    or
+    // There is a sequence point after a full declarator as described in 6.7.6 point 3.
+    exists(DeclStmt declStmt, int i, int j | i < j |
+      declStmt
+          .getDeclarationEntry(i)
+          .(VariableDeclarationEntry)
+          .getVariable()
+          .getInitializer()
+          .getExpr()
+          .getAChild*() = e1 and
+      declStmt
+          .getDeclarationEntry(j)
+          .(VariableDeclarationEntry)
+          .getVariable()
+          .getInitializer()
+          .getExpr()
+          .getAChild*() = e2
+    )
   }
 }

c/common/src/codingstandards/c/orderofevaluation/VariableAccessOrdering.qll

@@ -1,10 +1,11 @@
 import cpp
 import codingstandards.c.Ordering
+import codingstandards.c.SideEffects
 
-class VariableAccessInFullExpressionOrdering extends Ordering::Configuration {
-  VariableAccessInFullExpressionOrdering() { this = "VariableAccessInFullExpressionOrdering" }
+module VariableAccessInFullExpressionOrdering implements Ordering::ConfigSig {
+  import Ordering::CConfigBase
 
-  override predicate isCandidate(Expr e1, Expr e2) { isCandidate(_, e1, e2) }
+  predicate isCandidate(Expr e1, Expr e2) { isCandidate(_, e1, e2) }
 }
 
 pragma[noinline]

c/misra/src/rules/RULE-13-2/UnsequencedAtomicReads.ql

@@ -19,10 +19,10 @@ import codingstandards.c.Ordering
 import codingstandards.c.orderofevaluation.VariableAccessOrdering
 import codingstandards.cpp.StdFunctionOrMacro
 
-class AtomicAccessInFullExpressionOrdering extends Ordering::Configuration {
-  AtomicAccessInFullExpressionOrdering() { this = "AtomicAccessInFullExpressionOrdering" }
+module AtomicAccessInFullExpressionOrdering implements Ordering::ConfigSig {
+  import Ordering::CConfigBase
 
-  override predicate isCandidate(Expr e1, Expr e2) {
+  predicate isCandidate(Expr e1, Expr e2) {
     exists(AtomicVariableAccess a, AtomicVariableAccess b, FullExpr e | a = e1 and b = e2 |
       a.getTarget() = b.getTarget() and
       a.getARead().(ConstituentExpr).getFullExpr() = e and
@@ -91,9 +91,11 @@ class AtomicVariableAccess extends VariableAccess {
   }
 }
 
+import Ordering::Make<AtomicAccessInFullExpressionOrdering> as AtomicOrdering
+
 from
-  AtomicAccessInFullExpressionOrdering config, FullExpr e, Variable v, AtomicVariableAccess va1,
-  AtomicVariableAccess va2, Expr va1Read, Expr va2Read
+  FullExpr e, Variable v, AtomicVariableAccess va1, AtomicVariableAccess va2, Expr va1Read,
+  Expr va2Read
 where
   not isExcluded(e, SideEffects3Package::unsequencedAtomicReadsQuery()) and
   va1Read = va1.getARead() and
@@ -103,7 +105,7 @@ where
   // for instance in gcc where atomic functions expand to StmtExprs, which have clear sequences.
   // In this case, the result of `getARead()` for a pair of atomic function calls may be
   // unsequenced even though the `VariableAccess`es within those calls are not.
-  config.isUnsequenced(va1Read, va2Read) and
+  AtomicOrdering::isUnsequenced(va1Read, va2Read) and
   v = va1.getTarget() and
   v = va2.getTarget() and
   // Exclude cases where the variable is assigned a value tainted by the other variable access.

c/misra/src/rules/RULE-13-2/UnsequencedSideEffects.ql

@@ -33,10 +33,10 @@ predicate partOfFullExpr(VariableEffectOrAccess e, FullExpr fe) {
   )
 }
 
-class ConstituentExprOrdering extends Ordering::Configuration {
-  ConstituentExprOrdering() { this = "ConstituentExprOrdering" }
+module ConstituentExprOrderingConfig implements Ordering::ConfigSig {
+  import Ordering::CConfigBase
 
-  override predicate isCandidate(Expr e1, Expr e2) {
+  predicate isCandidate(Expr e1, Expr e2) {
     exists(FullExpr fe |
       partOfFullExpr(e1, fe) and
       partOfFullExpr(e2, fe)
@@ -172,9 +172,11 @@ predicate inConditionalElse(ConditionalExpr ce, Expr e) {
   )
 }
 
+import Ordering::Make<ConstituentExprOrderingConfig> as ConstituentExprOrdering
+
 predicate isUnsequencedEffect(
-  ConstituentExprOrdering orderingConfig, FullExpr fullExpr, VariableEffect variableEffect1,
-  VariableAccess va1, VariableAccess va2, Locatable placeHolder, string label
+  FullExpr fullExpr, VariableEffect variableEffect1, VariableAccess va1, VariableAccess va2,
+  Locatable placeHolder, string label
 ) {
   // The two access are scoped to the same full expression.
   sameFullExpr(fullExpr, va1, va2) and
@@ -190,22 +192,22 @@ predicate isUnsequencedEffect(
       (
         va1.getEnclosingStmt() = variableEffect1.getEnclosingStmt() and
         va2.getEnclosingStmt() = variableEffect2.getEnclosingStmt() and
-        orderingConfig.isUnsequenced(variableEffect1, variableEffect2)
+        ConstituentExprOrdering::isUnsequenced(variableEffect1, variableEffect2)
         or
         va1.getEnclosingStmt() = variableEffect1.getEnclosingStmt() and
         not va2.getEnclosingStmt() = variableEffect2.getEnclosingStmt() and
         exists(Call call |
           call.getAnArgument() = va2 and call.getEnclosingStmt() = va1.getEnclosingStmt()
         |
-          orderingConfig.isUnsequenced(variableEffect1, call)
+          ConstituentExprOrdering::isUnsequenced(variableEffect1, call)
         )
         or
         not va1.getEnclosingStmt() = variableEffect1.getEnclosingStmt() and
         va2.getEnclosingStmt() = variableEffect2.getEnclosingStmt() and
         exists(Call call |
           call.getAnArgument() = va1 and call.getEnclosingStmt() = va2.getEnclosingStmt()
         |
-          orderingConfig.isUnsequenced(call, variableEffect2)
+          ConstituentExprOrdering::isUnsequenced(call, variableEffect2)
         )
       ) and
       // Break the symmetry of the ordering relation by requiring that the first expression is located before the second.
@@ -222,13 +224,13 @@ predicate isUnsequencedEffect(
     ) and
     (
       va1.getEnclosingStmt() = variableEffect1.getEnclosingStmt() and
-      orderingConfig.isUnsequenced(variableEffect1, va2)
+      ConstituentExprOrdering::isUnsequenced(variableEffect1, va2)
       or
       not va1.getEnclosingStmt() = variableEffect1.getEnclosingStmt() and
       exists(Call call |
         call.getAnArgument() = va1 and call.getEnclosingStmt() = va2.getEnclosingStmt()
       |
-        orderingConfig.isUnsequenced(call, va2)
+        ConstituentExprOrdering::isUnsequenced(call, va2)
       )
     ) and
     // The read is not used to compute the effect on the variable.
@@ -240,10 +242,10 @@ predicate isUnsequencedEffect(
 }
 
 from
-  ConstituentExprOrdering orderingConfig, FullExpr fullExpr, VariableEffect variableEffect1,
-  VariableAccess va1, VariableAccess va2, Locatable placeHolder, string label
+  FullExpr fullExpr, VariableEffect variableEffect1, VariableAccess va1, VariableAccess va2,
+  Locatable placeHolder, string label
 where
   not isExcluded(fullExpr, SideEffects3Package::unsequencedSideEffectsQuery()) and
-  isUnsequencedEffect(orderingConfig, fullExpr, variableEffect1, va1, va2, placeHolder, label)
+  isUnsequencedEffect(fullExpr, variableEffect1, va1, va2, placeHolder, label)
 select fullExpr, "The expression contains unsequenced $@ to $@ and $@ to $@.", variableEffect1,
   "side effect", va1, va1.getTarget().getName(), placeHolder, label, va2, va2.getTarget().getName()