Skip to content

Commit 4226270

Browse files
criemencriemen
authored
Merge branch 'main' into criemen/rust-bzlmod-new
2 parents 496fff4 + 73caa48 commit 4226270

File tree

13 files changed

+261
-18
lines changed

13 files changed

+261
-18
lines changed

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* 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‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -762,6 +762,8 @@ private predicate compares_eq(
762762
exists(AbstractValue dual | value = dual.getDualValue() |
763763
compares_eq(test.(LogicalNotInstruction).getUnary(), left, right, k, areEqual, dual)
764764
)
765+
or
766+
compares_eq(test.(BuiltinExpectCallInstruction).getCondition(), left, right, k, areEqual, value)
765767
}
766768

767769
/**
@@ -831,6 +833,9 @@ private predicate unary_compares_eq(
831833
int_value(const) = k1 and
832834
k = k1 + k2
833835
)
836+
or
837+
unary_compares_eq(test.(BuiltinExpectCallInstruction).getCondition(), op, k, areEqual,
838+
inNonZeroCase, value)
834839
}
835840

836841
/** Rearrange various simple comparisons into `left == right + k` form. */
@@ -910,12 +915,68 @@ private predicate unary_simple_comparison_eq(
910915
)
911916
}
912917

918+
/** A call to the builtin operation `__builtin_expect`. */
919+
private class BuiltinExpectCallInstruction extends CallInstruction {
920+
BuiltinExpectCallInstruction() { this.getStaticCallTarget().hasName("__builtin_expect") }
921+
922+
/** Gets the condition of this call. */
923+
Instruction getCondition() {
924+
// The first parameter of `__builtin_expect` has type `long`. So we skip
925+
// the conversion when inferring guards.
926+
result = this.getArgument(0).(ConvertInstruction).getUnary()
927+
}
928+
}
929+
930+
/**
931+
* Holds if `left == right + k` is `areEqual` if `cmp` evaluates to `value`,
932+
* and `cmp` is an instruction that compares the value of
933+
* `__builtin_expect(left == right + k, _)` to `0`.
934+
*/
935+
private predicate builtin_expect_eq(
936+
CompareInstruction cmp, Operand left, Operand right, int k, boolean areEqual, AbstractValue value
937+
) {
938+
exists(BuiltinExpectCallInstruction call, Instruction const, AbstractValue innerValue |
939+
int_value(const) = 0 and
940+
cmp.hasOperands(call.getAUse(), const.getAUse()) and
941+
compares_eq(call.getCondition(), left, right, k, areEqual, innerValue)
942+
|
943+
cmp instanceof CompareNEInstruction and
944+
value = innerValue
945+
or
946+
cmp instanceof CompareEQInstruction and
947+
value.getDualValue() = innerValue
948+
)
949+
}
950+
913951
private predicate complex_eq(
914952
CompareInstruction cmp, Operand left, Operand right, int k, boolean areEqual, AbstractValue value
915953
) {
916954
sub_eq(cmp, left, right, k, areEqual, value)
917955
or
918956
add_eq(cmp, left, right, k, areEqual, value)
957+
or
958+
builtin_expect_eq(cmp, left, right, k, areEqual, value)
959+
}
960+
961+
/**
962+
* Holds if `op == k` is `areEqual` if `cmp` evaluates to `value`, and `cmp` is
963+
* an instruction that compares the value of `__builtin_expect(op == k, _)` to `0`.
964+
*/
965+
private predicate unary_builtin_expect_eq(
966+
CompareInstruction cmp, Operand op, int k, boolean areEqual, boolean inNonZeroCase,
967+
AbstractValue value
968+
) {
969+
exists(BuiltinExpectCallInstruction call, Instruction const, AbstractValue innerValue |
970+
int_value(const) = 0 and
971+
cmp.hasOperands(call.getAUse(), const.getAUse()) and
972+
unary_compares_eq(call.getCondition(), op, k, areEqual, inNonZeroCase, innerValue)
973+
|
974+
cmp instanceof CompareNEInstruction and
975+
value = innerValue
976+
or
977+
cmp instanceof CompareEQInstruction and
978+
value.getDualValue() = innerValue
979+
)
919980
}
920981

921982
private predicate unary_complex_eq(
@@ -924,6 +985,8 @@ private predicate unary_complex_eq(
924985
unary_sub_eq(test, op, k, areEqual, inNonZeroCase, value)
925986
or
926987
unary_add_eq(test, op, k, areEqual, inNonZeroCase, value)
988+
or
989+
unary_builtin_expect_eq(test, op, k, areEqual, inNonZeroCase, value)
927990
}
928991

929992
/*

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,3 +45,7 @@
4545
| test.cpp:122:9:122:9 | b |
4646
| test.cpp:125:13:125:20 | ! ... |
4747
| test.cpp:125:14:125:17 | call to safe |
48+
| test.cpp:131:6:131:21 | call to __builtin_expect |
49+
| test.cpp:135:6:135:21 | call to __builtin_expect |
50+
| test.cpp:141:6:141:21 | call to __builtin_expect |
51+
| test.cpp:145:6:145:21 | call to __builtin_expect |

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

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,10 +164,46 @@
164164
| 126 | call to test3_condition != 0 when ... && ... is true |
165165
| 126 | call to test3_condition != 0 when call to test3_condition is true |
166166
| 126 | call to test3_condition == 0 when call to test3_condition is false |
167+
| 131 | ... + ... != a+0 when call to __builtin_expect is false |
168+
| 131 | ... + ... == a+0 when call to __builtin_expect is true |
169+
| 131 | a != ... + ...+0 when call to __builtin_expect is false |
170+
| 131 | a != b+42 when call to __builtin_expect is false |
171+
| 131 | a == ... + ...+0 when call to __builtin_expect is true |
172+
| 131 | a == b+42 when call to __builtin_expect is true |
167173
| 131 | b != 0 when b is true |
174+
| 131 | b != a+-42 when call to __builtin_expect is false |
168175
| 131 | b == 0 when b is false |
176+
| 131 | b == a+-42 when call to __builtin_expect is true |
177+
| 131 | call to __builtin_expect != 0 when call to __builtin_expect is true |
178+
| 131 | call to __builtin_expect == 0 when call to __builtin_expect is false |
179+
| 135 | ... + ... != a+0 when call to __builtin_expect is true |
180+
| 135 | ... + ... == a+0 when call to __builtin_expect is false |
181+
| 135 | a != ... + ...+0 when call to __builtin_expect is true |
182+
| 135 | a != b+42 when call to __builtin_expect is true |
183+
| 135 | a == ... + ...+0 when call to __builtin_expect is false |
184+
| 135 | a == b+42 when call to __builtin_expect is false |
185+
| 135 | b != a+-42 when call to __builtin_expect is true |
186+
| 135 | b == a+-42 when call to __builtin_expect is false |
187+
| 135 | call to __builtin_expect != 0 when call to __builtin_expect is true |
188+
| 135 | call to __builtin_expect == 0 when call to __builtin_expect is false |
169189
| 137 | 0 != 0 when 0 is true |
170190
| 137 | 0 == 0 when 0 is false |
191+
| 141 | 42 != a+0 when call to __builtin_expect is false |
192+
| 141 | 42 == a+0 when call to __builtin_expect is true |
193+
| 141 | a != 42 when call to __builtin_expect is false |
194+
| 141 | a != 42+0 when call to __builtin_expect is false |
195+
| 141 | a == 42 when call to __builtin_expect is true |
196+
| 141 | a == 42+0 when call to __builtin_expect is true |
197+
| 141 | call to __builtin_expect != 0 when call to __builtin_expect is true |
198+
| 141 | call to __builtin_expect == 0 when call to __builtin_expect is false |
199+
| 145 | 42 != a+0 when call to __builtin_expect is true |
200+
| 145 | 42 == a+0 when call to __builtin_expect is false |
201+
| 145 | a != 42 when call to __builtin_expect is true |
202+
| 145 | a != 42+0 when call to __builtin_expect is true |
203+
| 145 | a == 42 when call to __builtin_expect is false |
204+
| 145 | a == 42+0 when call to __builtin_expect is false |
205+
| 145 | call to __builtin_expect != 0 when call to __builtin_expect is true |
206+
| 145 | call to __builtin_expect == 0 when call to __builtin_expect is false |
171207
| 146 | ! ... != 0 when ! ... is true |
172208
| 146 | ! ... == 0 when ! ... is false |
173209
| 146 | x != 0 when ! ... is false |

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,3 +104,7 @@
104104
| test.cpp:122:9:122:9 | b | true | 125 | 125 |
105105
| test.cpp:125:13:125:20 | ! ... | true | 125 | 125 |
106106
| test.cpp:125:14:125:17 | call to safe | false | 125 | 125 |
107+
| test.cpp:131:6:131:21 | call to __builtin_expect | true | 131 | 132 |
108+
| test.cpp:135:6:135:21 | call to __builtin_expect | true | 135 | 136 |
109+
| test.cpp:141:6:141:21 | call to __builtin_expect | true | 141 | 142 |
110+
| test.cpp:145:6:145:21 | call to __builtin_expect | true | 145 | 146 |

‎cpp/ql/test/library-tests/controlflow/guards/GuardsEnsure.expected‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,18 @@ binary
159159
| test.cpp:105:6:105:14 | ... != ... | test.cpp:105:11:105:14 | 0.0 | != | test.cpp:105:6:105:6 | f | 0 | 105 | 106 |
160160
| test.cpp:111:6:111:14 | ... != ... | test.cpp:111:6:111:6 | i | != | test.cpp:111:11:111:14 | 0.0 | 0 | 111 | 112 |
161161
| test.cpp:111:6:111:14 | ... != ... | test.cpp:111:11:111:14 | 0.0 | != | test.cpp:111:6:111:6 | i | 0 | 111 | 112 |
162+
| test.cpp:131:6:131:21 | call to __builtin_expect | test.cpp:131:23:131:23 | a | == | test.cpp:131:28:131:28 | b | 42 | 131 | 132 |
163+
| test.cpp:131:6:131:21 | call to __builtin_expect | test.cpp:131:23:131:23 | a | == | test.cpp:131:28:131:33 | ... + ... | 0 | 131 | 132 |
164+
| test.cpp:131:6:131:21 | call to __builtin_expect | test.cpp:131:28:131:28 | b | == | test.cpp:131:23:131:23 | a | -42 | 131 | 132 |
165+
| test.cpp:131:6:131:21 | call to __builtin_expect | test.cpp:131:28:131:33 | ... + ... | == | test.cpp:131:23:131:23 | a | 0 | 131 | 132 |
166+
| test.cpp:135:6:135:21 | call to __builtin_expect | test.cpp:135:23:135:23 | a | != | test.cpp:135:28:135:28 | b | 42 | 135 | 136 |
167+
| test.cpp:135:6:135:21 | call to __builtin_expect | test.cpp:135:23:135:23 | a | != | test.cpp:135:28:135:33 | ... + ... | 0 | 135 | 136 |
168+
| test.cpp:135:6:135:21 | call to __builtin_expect | test.cpp:135:28:135:28 | b | != | test.cpp:135:23:135:23 | a | -42 | 135 | 136 |
169+
| test.cpp:135:6:135:21 | call to __builtin_expect | test.cpp:135:28:135:33 | ... + ... | != | test.cpp:135:23:135:23 | a | 0 | 135 | 136 |
170+
| test.cpp:141:6:141:21 | call to __builtin_expect | test.cpp:141:23:141:23 | a | == | test.cpp:141:28:141:29 | 42 | 0 | 141 | 142 |
171+
| test.cpp:141:6:141:21 | call to __builtin_expect | test.cpp:141:28:141:29 | 42 | == | test.cpp:141:23:141:23 | a | 0 | 141 | 142 |
172+
| test.cpp:145:6:145:21 | call to __builtin_expect | test.cpp:145:23:145:23 | a | != | test.cpp:145:28:145:29 | 42 | 0 | 145 | 146 |
173+
| test.cpp:145:6:145:21 | call to __builtin_expect | test.cpp:145:28:145:29 | 42 | != | test.cpp:145:23:145:23 | a | 0 | 145 | 146 |
162174
unary
163175
| test.c:7:9:7:13 | ... > ... | test.c:7:9:7:9 | x | < | 1 | 10 | 11 |
164176
| test.c:7:9:7:13 | ... > ... | test.c:7:9:7:9 | x | >= | 1 | 7 | 9 |
@@ -270,3 +282,9 @@ unary
270282
| test.cpp:122:9:122:9 | b | test.cpp:122:9:122:9 | b | != | 0 | 125 | 125 |
271283
| test.cpp:125:13:125:20 | ! ... | test.cpp:125:13:125:20 | ! ... | != | 0 | 125 | 125 |
272284
| test.cpp:125:14:125:17 | call to safe | test.cpp:125:14:125:17 | call to safe | == | 0 | 125 | 125 |
285+
| test.cpp:131:6:131:21 | call to __builtin_expect | test.cpp:131:6:131:21 | call to __builtin_expect | != | 0 | 131 | 132 |
286+
| test.cpp:135:6:135:21 | call to __builtin_expect | test.cpp:135:6:135:21 | call to __builtin_expect | != | 0 | 135 | 136 |
287+
| test.cpp:141:6:141:21 | call to __builtin_expect | test.cpp:141:6:141:21 | call to __builtin_expect | != | 0 | 141 | 142 |
288+
| test.cpp:141:6:141:21 | call to __builtin_expect | test.cpp:141:23:141:23 | a | == | 42 | 141 | 142 |
289+
| test.cpp:145:6:145:21 | call to __builtin_expect | test.cpp:145:6:145:21 | call to __builtin_expect | != | 0 | 145 | 146 |
290+
| test.cpp:145:6:145:21 | call to __builtin_expect | test.cpp:145:23:145:23 | a | != | 42 | 145 | 146 |

‎cpp/ql/test/library-tests/controlflow/guards/test.cpp‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,4 +125,24 @@ void test(bool b)
125125
if (!safe(x)) return;
126126
}
127127
use(x);
128+
}
129+
130+
void binary_test_builtin_expected(int a, int b) {
131+
if(__builtin_expect(a == b + 42, 0)) {
132+
use(a);
133+
}
134+
135+
if(__builtin_expect(a != b + 42, 0)) {
136+
use(a);
137+
}
138+
}
139+
140+
void unary_test_builtin_expected(int a) {
141+
if(__builtin_expect(a == 42, 0)) {
142+
use(a);
143+
}
144+
145+
if(__builtin_expect(a != 42, 0)) {
146+
use(a);
147+
}
128148
}

‎go/extractor/project/BUILD.bazel‎

Lines changed: 4 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎go/extractor/project/project.go‎

Lines changed: 23 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,17 @@ type GoModule struct {
3636
Module *modfile.File // The parsed contents of the `go.mod` file
3737
}
3838

39+
// Tries to find the Go toolchain version required for this module.
40+
func (module *GoModule) RequiredGoVersion() util.SemVer {
41+
if module.Module != nil && module.Module.Toolchain != nil {
42+
return util.NewSemVer(module.Module.Toolchain.Name)
43+
} else if module.Module != nil && module.Module.Go != nil {
44+
return util.NewSemVer(module.Module.Go.Version)
45+
} else {
46+
return tryReadGoDirective(module.Path)
47+
}
48+
}
49+
3950
// Represents information about a Go project workspace: this may either be a folder containing
4051
// a `go.work` file or a collection of `go.mod` files.
4152
type GoWorkspace struct {
@@ -54,24 +65,23 @@ type GoVersionInfo = util.SemVer
5465
// 1. The Go version specified in the `go.work` file, if any.
5566
// 2. The greatest Go version specified in any `go.mod` file, if any.
5667
func (workspace *GoWorkspace) RequiredGoVersion() util.SemVer {
57-
if workspace.WorkspaceFile != nil && workspace.WorkspaceFile.Go != nil {
58-
// If we have parsed a `go.work` file, return the version number from it.
68+
// If we have parsed a `go.work` file, we prioritise versions from it over those in individual `go.mod`
69+
// files. We are interested in toolchain versions, so if there is an explicit toolchain declaration in
70+
// a `go.work` file, we use that. Otherwise, we fall back to the language version in the `go.work` file
71+
// and use that as toolchain version. If we didn't parse a `go.work` file, then we try to find the
72+
// greatest version contained in `go.mod` files.
73+
if workspace.WorkspaceFile != nil && workspace.WorkspaceFile.Toolchain != nil {
74+
return util.NewSemVer(workspace.WorkspaceFile.Toolchain.Name)
75+
} else if workspace.WorkspaceFile != nil && workspace.WorkspaceFile.Go != nil {
5976
return util.NewSemVer(workspace.WorkspaceFile.Go.Version)
6077
} else if workspace.Modules != nil && len(workspace.Modules) > 0 {
6178
// Otherwise, if we have `go.work` files, find the greatest Go version in those.
6279
var greatestVersion util.SemVer = nil
6380
for _, module := range workspace.Modules {
64-
if module.Module != nil && module.Module.Go != nil {
65-
// If we have parsed the file, retrieve the version number we have already obtained.
66-
modVersion := util.NewSemVer(module.Module.Go.Version)
67-
if greatestVersion == nil || modVersion.IsNewerThan(greatestVersion) {
68-
greatestVersion = modVersion
69-
}
70-
} else {
71-
modVersion := tryReadGoDirective(module.Path)
72-
if modVersion != nil && (greatestVersion == nil || modVersion.IsNewerThan(greatestVersion)) {
73-
greatestVersion = modVersion
74-
}
81+
modVersion := module.RequiredGoVersion()
82+
83+
if modVersion != nil && (greatestVersion == nil || modVersion.IsNewerThan(greatestVersion)) {
84+
greatestVersion = modVersion
7585
}
7686
}
7787

‎go/extractor/project/project_test.go‎

Lines changed: 79 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"path/filepath"
55
"testing"
66

7+
"github.com/github/codeql-go/extractor/util"
78
"golang.org/x/mod/modfile"
89
)
910

@@ -28,14 +29,18 @@ func TestStartsWithAnyOf(t *testing.T) {
2829
testStartsWithAnyOf(t, filepath.Join("foo", "bar"), filepath.Join("foo", "baz"), false)
2930
}
3031

31-
func testHasInvalidToolchainVersion(t *testing.T, contents string) bool {
32-
modFile, err := modfile.Parse("test.go", []byte(contents), nil)
32+
func parseModFile(t *testing.T, contents string) *modfile.File {
33+
modFile, err := modfile.Parse("go.mod", []byte(contents), nil)
3334

3435
if err != nil {
3536
t.Errorf("Unable to parse %s: %s.\n", contents, err.Error())
3637
}
3738

38-
return hasInvalidToolchainVersion(modFile)
39+
return modFile
40+
}
41+
42+
func testHasInvalidToolchainVersion(t *testing.T, contents string) bool {
43+
return hasInvalidToolchainVersion(parseModFile(t, contents))
3944
}
4045

4146
func TestHasInvalidToolchainVersion(t *testing.T) {
@@ -62,3 +67,74 @@ func TestHasInvalidToolchainVersion(t *testing.T) {
6267
}
6368
}
6469
}
70+
71+
func parseWorkFile(t *testing.T, contents string) *modfile.WorkFile {
72+
workFile, err := modfile.ParseWork("go.work", []byte(contents), nil)
73+
74+
if err != nil {
75+
t.Errorf("Unable to parse %s: %s.\n", contents, err.Error())
76+
}
77+
78+
return workFile
79+
}
80+
81+
func TestRequiredGoVersion(t *testing.T) {
82+
type ModVersionPair struct {
83+
FileContents string
84+
ExpectedVersion string
85+
}
86+
87+
modules := []ModVersionPair{
88+
{"go 1.20", "v1.20"},
89+
{"go 1.21.2", "v1.21.2"},
90+
{"go 1.21rc1", "v1.21.0-rc1"},
91+
{"go 1.21rc1\ntoolchain go1.22.0", "v1.22.0"},
92+
{"go 1.21rc1\ntoolchain go1.22rc1", "v1.22.0-rc1"},
93+
}
94+
95+
for _, testData := range modules {
96+
// `go.mod` and `go.work` files have mostly the same format
97+
modFile := parseModFile(t, testData.FileContents)
98+
workFile := parseWorkFile(t, testData.FileContents)
99+
mod := GoModule{
100+
Path: "test", // irrelevant
101+
Module: modFile,
102+
}
103+
work := GoWorkspace{
104+
WorkspaceFile: workFile,
105+
}
106+
107+
result := mod.RequiredGoVersion()
108+
if result == nil {
109+
t.Errorf(
110+
"Expected mod.RequiredGoVersion() to return %s for the below `go.mod` file, but got nothing:\n%s",
111+
testData.ExpectedVersion,
112+
testData.FileContents,
113+
)
114+
} else if result != util.NewSemVer(testData.ExpectedVersion) {
115+
t.Errorf(
116+
"Expected mod.RequiredGoVersion() to return %s for the below `go.mod` file, but got %s:\n%s",
117+
testData.ExpectedVersion,
118+
result,
119+
testData.FileContents,
120+
)
121+
}
122+
123+
result = work.RequiredGoVersion()
124+
if result == nil {
125+
t.Errorf(
126+
"Expected mod.RequiredGoVersion() to return %s for the below `go.work` file, but got nothing:\n%s",
127+
testData.ExpectedVersion,
128+
testData.FileContents,
129+
)
130+
} else if result != util.NewSemVer(testData.ExpectedVersion) {
131+
t.Errorf(
132+
"Expected mod.RequiredGoVersion() to return %s for the below `go.work` file, but got %s:\n%s",
133+
testData.ExpectedVersion,
134+
result,
135+
testData.FileContents,
136+
)
137+
}
138+
}
139+
140+
}

0 commit comments

Comments
 (0)