Skip to content

Commit c808c96

Browse files
Merge branch 'main' into michaelrfairhurst/implement-package-preprocessor
2 parents a83e513 + 743d8ec commit c808c96

File tree

165 files changed

+4944
-1011
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

165 files changed

+4944
-1011
lines changed

.github/copilot-instructions.md

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
---
2+
description: 'Code review guidelines for GitHub copilot in this project'
3+
applyTo: '**'
4+
excludeAgent: ["coding-agent"]
5+
---
6+
7+
# Code Review Instructions
8+
9+
A change note is required for any pull request which modifies:
10+
- The structure or layout of the release artifacts.
11+
- The evaluation performance (memory, execution time) of an existing query.
12+
- The results of an existing query in any circumstance.
13+
14+
If the pull request only adds new rule queries, a change note is not required.
15+
Confirm that either a change note is not required or the change note is required and has been added.
16+
17+
For PRs that add new queries or modify existing queries, also consider the following review checklist:
18+
- Confirm that the output format of shared queries is valid.
19+
- Have all the relevant rule package description files been checked in?
20+
- Have you verified that the metadata properties of each new query is set appropriately?
21+
- Do all the unit tests contain both "COMPLIANT" and "NON_COMPLIANT" cases?
22+
- Are all the alerts in the expected file annotated as NON_COMPLIANT in the test source file?
23+
- Are the alert messages properly formatted and consistent with the style guide?
24+
- Does the query have an appropriate level of in-query comments/documentation?
25+
- Does the query not reinvent features in the standard library?
26+
- Can the query be simplified further (not golfed!).
27+
28+
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.
29+
30+
## Validating tests and .expected files
31+
32+
The test infrastructure for CodeQL that we use in this project involves the creation of a test directory with the following structure:
33+
- Test root is `some/path/test/path/to/feature` (mirrors `some/path/src/path/to/query`)
34+
- At least one test `c` or `c++` file, typically named `test.c`/`test.cpp`, with lines annotated `// COMPLIANT` or `// NON_COMPLIANT`
35+
- A `.ql` file with test query logic, or a `.qlref` file referring to the production query logic
36+
- 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
37+
- Note that some test directories simply have a `testref` file, to document that a certain query is tested in a different directory.
38+
39+
As a code reviewer, it is critical to ensure that the results in the `.expected` file match the comments in the test file.
40+
41+
The `.expected` file uses a columnar format:
42+
- For example, a basic row may look like `| test.cpp:8:22:8:37 | element | message |`.
43+
- For a query with `select x, "test"`, the columns are | x.getLocation() | x.toString() | "test" |`
44+
- 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 |`.
45+
- Remember, there is one `.expected` file for each `.ql` or `.qlref` file.
46+
- Each `.expected` file will contain the results for all test c/cpp files.
47+
- The `toString()` format of QL objects is deliberately terse for performance reasons.
48+
- 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`.
49+
50+
Reviewing tests in this style can be tedious and error prone, but fundamental to the effectiveness of our TDD requirements in this project.
51+
52+
When reviewing tests, it is critical to:
53+
- Check that each `NON_COMPLIANT` case in the test file has a row in the correct `.expected` file referring to the correct location.
54+
- Check that each row in each `.expected` file has a `NON_COMPLIANT` case in the test file at the correct location.
55+
- Check that there are no `.expected` rows that refer to test code cases marked as `COMPLIANT`, or with no comment
56+
- Note that it is OK if the locations of the comment are not precisely aligned with the alert
57+
- Check that the alert message and placeholders are accurate and understandable.
58+
- 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.
59+
- 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.

.github/workflows/code-scanning-pack-gen.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ jobs:
111111
zip -r codeql-coding-standards/code-scanning-cpp-query-pack.zip codeql-coding-standards/c/ codeql-coding-standards/cpp/ codeql-coding-standards/.codeqlmanifest.json codeql-coding-standards/supported_codeql_configs.json codeql-coding-standards/scripts/configuration codeql-coding-standards/scripts/reports codeql-coding-standards/scripts/shared codeql-coding-standards/scripts/guideline_recategorization codeql-coding-standards/schemas
112112
113113
- name: Upload GHAS Query Pack
114-
uses: actions/upload-artifact@v5
114+
uses: actions/upload-artifact@v6
115115
with:
116116
name: code-scanning-cpp-query-pack.zip
117117
path: code-scanning-cpp-query-pack.zip
@@ -132,7 +132,7 @@ jobs:
132132
codeql pack bundle --output=report-coding-standards.tgz cpp/report/src
133133
134134
- name: Upload qlpack bundles
135-
uses: actions/upload-artifact@v5
135+
uses: actions/upload-artifact@v6
136136
with:
137137
name: coding-standards-codeql-packs
138138
path: '*-coding-standards.tgz'

.github/workflows/codeql_unit_tests.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ jobs:
153153
file.close()
154154
155155
- name: Upload test results
156-
uses: actions/upload-artifact@v5
156+
uses: actions/upload-artifact@v6
157157
with:
158158
name: ${{ matrix.language }}-test-results-${{ runner.os }}-${{ matrix.codeql_cli }}-${{ matrix.codeql_standard_library_ident }}
159159
path: |
@@ -173,7 +173,7 @@ jobs:
173173
script: |
174174
core.setFailed('Test run job failed')
175175
- name: Collect test results
176-
uses: actions/download-artifact@v6
176+
uses: actions/download-artifact@v7
177177

178178
- name: Validate test results
179179
run: |

.github/workflows/extra-rule-validation.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,13 +46,13 @@ jobs:
4646
run: scripts/util/Test-SharedImplementationsHaveTestCases.ps1 -Language c -CIMode
4747

4848

49-
- uses: actions/upload-artifact@v5
49+
- uses: actions/upload-artifact@v6
5050
if: failure()
5151
with:
5252
name: missing-test-report.csv
5353
path: MissingTestReport*.csv
5454

55-
- uses: actions/upload-artifact@v5
55+
- uses: actions/upload-artifact@v6
5656
if: failure()
5757
with:
5858
name: test-report.csv

.github/workflows/generate-html-docs.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ jobs:
3737
python scripts/documentation/generate_iso26262_docs.py coding-standards-html-docs
3838
3939
- name: Upload HTML documentation
40-
uses: actions/upload-artifact@v5
40+
uses: actions/upload-artifact@v6
4141
with:
4242
name: coding-standards-docs-${{ github.sha }}
4343
path: coding-standards-html-docs/

.github/workflows/standard_library_upgrade_tests.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ jobs:
145145
}, test_summary_file)
146146
147147
- name: Upload test results
148-
uses: actions/upload-artifact@v5
148+
uses: actions/upload-artifact@v6
149149
with:
150150
name: test-results-${{runner.os}}-${{matrix.codeql_cli}}-${{matrix.codeql_standard_library_ident}}
151151
path: |
@@ -164,7 +164,7 @@ jobs:
164164
python-version: "3.9"
165165

166166
- name: Collect test results
167-
uses: actions/download-artifact@v6
167+
uses: actions/download-artifact@v7
168168

169169
- name: Validate test results
170170
shell: python

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

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,15 +21,14 @@ import codingstandards.c.cert
2121
import codingstandards.cpp.SideEffect
2222
import codingstandards.c.Ordering
2323
import codingstandards.c.orderofevaluation.VariableAccessOrdering
24+
import Ordering::Make<VariableAccessInFullExpressionOrdering> as FullExpressionOrdering
2425

25-
from
26-
VariableAccessInFullExpressionOrdering config, FullExpr e, ScalarVariable v, VariableEffect ve,
27-
VariableAccess va1, VariableAccess va2
26+
from FullExpr e, ScalarVariable v, VariableEffect ve, VariableAccess va1, VariableAccess va2
2827
where
2928
not isExcluded(e, SideEffects1Package::dependenceOnOrderOfScalarEvaluationForSideEffectsQuery()) and
3029
e = va1.(ConstituentExpr).getFullExpr() and
3130
va1 = ve.getAnAccess() and
32-
config.isUnsequenced(va1, va2) and
31+
FullExpressionOrdering::isUnsequenced(va1, va2) and
3332
v = va1.getTarget()
3433
select e, "Scalar object referenced by $@ has a $@ that is unsequenced in relative to another $@.",
3534
v, v.getName(), ve, "side-effect", va2, "side-effect or value computation"
Lines changed: 73 additions & 86 deletions
Original file line numberDiff line numberDiff line change
@@ -1,97 +1,84 @@
11
import cpp
2-
import codingstandards.cpp.SideEffect
32
import codingstandards.c.Expr
43
import codingstandards.cpp.Variable
54

65
module Ordering {
7-
abstract class Configuration extends string {
8-
bindingset[this]
9-
Configuration() { any() }
6+
private import codingstandards.cpp.Ordering as CppCommonOrdering
7+
import CppCommonOrdering::OrderingBase
108

11-
abstract predicate isCandidate(Expr e1, Expr e2);
9+
module CConfigBase {
10+
class EvaluationNode = Expr;
1211

13-
/**
14-
* Holds if `e1` is sequenced before `e2` as defined by Annex C in ISO/IEC 9899:2011
15-
* This limits to expression and we do not consider the sequence points that are not amenable to modelling:
16-
* - before a library function returns (see 7.1.4 point 3).
17-
* - after the actions associated with each formatted I/O function conversion specifier (see 7.21.6 point 1 & 7.29.2 point 1).
18-
* - between the expr before and after a call to a comparison function,
19-
* between any call to a comparison function, and any movement of the objects passed
20-
* as arguments to that call (see 7.22.5 point 5).
21-
*/
22-
predicate isSequencedBefore(Expr e1, Expr e2) {
23-
isCandidate(e1, e2) and
24-
not e1 = e2 and
12+
pragma[inline]
13+
Expr toExpr(Expr e) { result = e }
14+
15+
pragma[inline]
16+
predicate sequencingEdge(Expr e1, Expr e2) { c11Ordering(e1, e2) }
17+
}
18+
19+
pragma[inline]
20+
predicate c11Ordering(Expr e1, Expr e2) {
21+
// 6.5.2.2 point 10 - The evaluation of the function designator and the actual arguments are sequenced
22+
// before the actual call.
23+
exists(Call call |
2524
(
26-
// 6.5.2.2 point 10 - The evaluation of the function designator and the actual arguments are sequenced
27-
// before the actual call.
28-
exists(Call call |
29-
(
30-
call.getAnArgument().getAChild*() = e1
31-
or
32-
// Postfix expression designating the called function
33-
// We current only handle call through function pointers because the postfix expression
34-
// of regular function calls is not available. That is, identifying `f` in `f(...)`
35-
call.(ExprCall).getExpr().getAChild*() = e1
36-
) and
37-
call.getTarget() = e2.getEnclosingFunction()
38-
)
39-
or
40-
// 6.5.13 point 4 & 6.5.14 point 4 - The operators guarantee left-to-right evaluation and there is
41-
// a sequence point between the first and second operand if the latter is evaluated.
42-
exists(BinaryLogicalOperation blop |
43-
blop instanceof LogicalAndExpr or blop instanceof LogicalOrExpr
44-
|
45-
blop.getLeftOperand().getAChild*() = e1 and blop.getRightOperand().getAChild*() = e2
46-
)
47-
or
48-
// 6.5.17 point 2 - There is a sequence point between the left operand and the right operand.
49-
exists(CommaExpr ce, Expr lhs, Expr rhs |
50-
lhs = ce.getLeftOperand() and
51-
rhs = ce.getRightOperand()
52-
|
53-
lhs.getAChild*() = e1 and rhs.getAChild*() = e2
54-
)
25+
call.getAnArgument().getAChild*() = e1
5526
or
56-
// 6.5.15 point 4 - There is a sequence point between the first operand and the evaluation of the second or third.
57-
exists(ConditionalExpr cond |
58-
cond.getCondition().getAChild*() = e1 and
59-
(cond.getThen().getAChild*() = e2 or cond.getElse().getAChild*() = e2)
60-
)
61-
or
62-
// Between the evaluation of a full expression and the next to be evaluated full expression.
63-
// Note we don't strictly check if `e2` is the next to be evaluated full expression and rely on the
64-
// `isCandidate` configuration to minimze the scope or related full expressions.
65-
e1 instanceof FullExpr and e2 instanceof FullExpr
66-
or
67-
// The side effect of updating the stored value of the left operand is sequenced after the value computations of the left and right operands.
68-
// See 6.5.16
69-
e2.(Assignment).getAnOperand().getAChild*() = e1
70-
or
71-
// There is a sequence point after a full declarator as described in 6.7.6 point 3.
72-
exists(DeclStmt declStmt, int i, int j | i < j |
73-
declStmt
74-
.getDeclarationEntry(i)
75-
.(VariableDeclarationEntry)
76-
.getVariable()
77-
.getInitializer()
78-
.getExpr()
79-
.getAChild*() = e1 and
80-
declStmt
81-
.getDeclarationEntry(j)
82-
.(VariableDeclarationEntry)
83-
.getVariable()
84-
.getInitializer()
85-
.getExpr()
86-
.getAChild*() = e2
87-
)
88-
)
89-
}
90-
91-
predicate isUnsequenced(Expr e1, Expr e2) {
92-
isCandidate(e1, e2) and
93-
not isSequencedBefore(e1, e2) and
94-
not isSequencedBefore(e2, e1)
95-
}
27+
// Postfix expression designating the called function
28+
// We current only handle call through function pointers because the postfix expression
29+
// of regular function calls is not available. That is, identifying `f` in `f(...)`
30+
call.(ExprCall).getExpr().getAChild*() = e1
31+
) and
32+
call.getTarget() = e2.getEnclosingFunction()
33+
)
34+
or
35+
// 6.5.13 point 4 & 6.5.14 point 4 - The operators guarantee left-to-right evaluation and there is
36+
// a sequence point between the first and second operand if the latter is evaluated.
37+
exists(BinaryLogicalOperation blop |
38+
blop instanceof LogicalAndExpr or blop instanceof LogicalOrExpr
39+
|
40+
blop.getLeftOperand().getAChild*() = e1 and blop.getRightOperand().getAChild*() = e2
41+
)
42+
or
43+
// 6.5.17 point 2 - There is a sequence point between the left operand and the right operand.
44+
exists(CommaExpr ce, Expr lhs, Expr rhs |
45+
lhs = ce.getLeftOperand() and
46+
rhs = ce.getRightOperand()
47+
|
48+
lhs.getAChild*() = e1 and rhs.getAChild*() = e2
49+
)
50+
or
51+
// 6.5.15 point 4 - There is a sequence point between the first operand and the evaluation of the second or third.
52+
exists(ConditionalExpr cond |
53+
cond.getCondition().getAChild*() = e1 and
54+
(cond.getThen().getAChild*() = e2 or cond.getElse().getAChild*() = e2)
55+
)
56+
or
57+
// Between the evaluation of a full expression and the next to be evaluated full expression.
58+
// Note we don't strictly check if `e2` is the next to be evaluated full expression and rely on the
59+
// `isCandidate` configuration to minimze the scope or related full expressions.
60+
e1 instanceof FullExpr and e2 instanceof FullExpr
61+
or
62+
// The side effect of updating the stored value of the left operand is sequenced after the value computations of the left and right operands.
63+
// See 6.5.16
64+
e2.(Assignment).getAnOperand().getAChild*() = e1
65+
or
66+
// There is a sequence point after a full declarator as described in 6.7.6 point 3.
67+
exists(DeclStmt declStmt, int i, int j | i < j |
68+
declStmt
69+
.getDeclarationEntry(i)
70+
.(VariableDeclarationEntry)
71+
.getVariable()
72+
.getInitializer()
73+
.getExpr()
74+
.getAChild*() = e1 and
75+
declStmt
76+
.getDeclarationEntry(j)
77+
.(VariableDeclarationEntry)
78+
.getVariable()
79+
.getInitializer()
80+
.getExpr()
81+
.getAChild*() = e2
82+
)
9683
}
9784
}

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
11
import cpp
22
import codingstandards.c.Ordering
3+
import codingstandards.c.SideEffects
34

4-
class VariableAccessInFullExpressionOrdering extends Ordering::Configuration {
5-
VariableAccessInFullExpressionOrdering() { this = "VariableAccessInFullExpressionOrdering" }
5+
module VariableAccessInFullExpressionOrdering implements Ordering::ConfigSig {
6+
import Ordering::CConfigBase
67

7-
override predicate isCandidate(Expr e1, Expr e2) { isCandidate(_, e1, e2) }
8+
predicate isCandidate(Expr e1, Expr e2) { isCandidate(_, e1, e2) }
89
}
910

1011
pragma[noinline]
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
| test.c:36:3:36:18 | ... = ... | An object $@ assigned to overlapping object $@. | test.c:36:9:36:10 | m2 | m2 | test.c:36:17:36:18 | m1 | m1 |

0 commit comments

Comments
 (0)