Skip to content

Commit 4351742

Browse files
grandizzygrandizzy
authored
fix(coverage): use first opcode for if block with statements (#8615)
* fix(coverage): use first opcode for if block anchor * Better naming
1 parent 78f86c3 commit 4351742

File tree

5 files changed

+147
-42
lines changed

5 files changed

+147
-42
lines changed

‎crates/evm/coverage/src/analysis.rs‎

Lines changed: 26 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -197,29 +197,25 @@ impl<'a> ContractVisitor<'a> {
197197
// Add branch coverage items only if one of true/branch bodies contains
198198
// statements.
199199
if has_statements(&true_body) || has_statements(&false_body) {
200-
// Add the coverage item for branch 0 (true body).
201-
// The relevant source range for the true branch is the `if(...)`
202-
// statement itself and the true body of the if statement.
203-
//
204-
// The false body of the statement is processed as its own thing.
205-
// If this source range is not processed like this, it is virtually
206-
// impossible to correctly map instructions back to branches that
207-
// include more complex logic like conditional logic.
200+
// The branch instruction is mapped to the first opcode within the true
201+
// body source range.
208202
self.push_item_kind(
209-
CoverageItemKind::Branch { branch_id, path_id: 0 },
210-
&ast::LowFidelitySourceLocation {
211-
start: node.src.start,
212-
length: true_body.src.length.map(|length| {
213-
true_body.src.start - node.src.start + length
214-
}),
215-
index: node.src.index,
203+
CoverageItemKind::Branch {
204+
branch_id,
205+
path_id: 0,
206+
is_first_opcode: true,
216207
},
208+
&true_body.src,
217209
);
218210
// Add the coverage item for branch 1 (false body).
219211
// The relevant source range for the false branch is the `else`
220212
// statement itself and the false body of the else statement.
221213
self.push_item_kind(
222-
CoverageItemKind::Branch { branch_id, path_id: 1 },
214+
CoverageItemKind::Branch {
215+
branch_id,
216+
path_id: 1,
217+
is_first_opcode: false,
218+
},
223219
&ast::LowFidelitySourceLocation {
224220
start: node.src.start,
225221
length: false_body.src.length.map(|length| {
@@ -270,7 +266,10 @@ impl<'a> ContractVisitor<'a> {
270266
// branch ID as we do
271267
self.branch_id += 1;
272268

273-
self.push_item_kind(CoverageItemKind::Branch { branch_id, path_id: 0 }, &node.src);
269+
self.push_item_kind(
270+
CoverageItemKind::Branch { branch_id, path_id: 0, is_first_opcode: false },
271+
&node.src,
272+
);
274273
self.visit_block(body)?;
275274

276275
Ok(())
@@ -369,11 +368,19 @@ impl<'a> ContractVisitor<'a> {
369368
let branch_id = self.branch_id;
370369
self.branch_id += 1;
371370
self.push_item_kind(
372-
CoverageItemKind::Branch { branch_id, path_id: 0 },
371+
CoverageItemKind::Branch {
372+
branch_id,
373+
path_id: 0,
374+
is_first_opcode: false,
375+
},
373376
&node.src,
374377
);
375378
self.push_item_kind(
376-
CoverageItemKind::Branch { branch_id, path_id: 1 },
379+
CoverageItemKind::Branch {
380+
branch_id,
381+
path_id: 1,
382+
is_first_opcode: false,
383+
},
377384
&node.src,
378385
);
379386
}

‎crates/evm/coverage/src/anchors.rs‎

Lines changed: 24 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -24,27 +24,34 @@ pub fn find_anchors(
2424
}
2525

2626
let item = &items[item_id];
27+
let find_anchor_by_first_opcode = |item: &CoverageItem| match find_anchor_simple(
28+
source_map, ic_pc_map, item_id, &item.loc,
29+
) {
30+
Ok(anchor) => Some(anchor),
31+
Err(e) => {
32+
warn!("Could not find anchor for item {item}: {e}");
33+
None
34+
}
35+
};
2736
match item.kind {
28-
CoverageItemKind::Branch { path_id, .. } => {
29-
match find_anchor_branch(bytecode, source_map, item_id, &item.loc) {
30-
Ok(anchors) => match path_id {
31-
0 => Some(anchors.0),
32-
1 => Some(anchors.1),
33-
_ => panic!("Too many paths for branch"),
34-
},
35-
Err(e) => {
36-
warn!("Could not find anchor for item {item}: {e}");
37-
None
37+
CoverageItemKind::Branch { path_id, is_first_opcode, .. } => {
38+
if is_first_opcode {
39+
find_anchor_by_first_opcode(item)
40+
} else {
41+
match find_anchor_branch(bytecode, source_map, item_id, &item.loc) {
42+
Ok(anchors) => match path_id {
43+
0 => Some(anchors.0),
44+
1 => Some(anchors.1),
45+
_ => panic!("Too many paths for branch"),
46+
},
47+
Err(e) => {
48+
warn!("Could not find anchor for item {item}: {e}");
49+
None
50+
}
3851
}
3952
}
4053
}
41-
_ => match find_anchor_simple(source_map, ic_pc_map, item_id, &item.loc) {
42-
Ok(anchor) => Some(anchor),
43-
Err(e) => {
44-
warn!("Could not find anchor for item {item}: {e}");
45-
None
46-
}
47-
},
54+
_ => find_anchor_by_first_opcode(item),
4855
}
4956
})
5057
.collect()

‎crates/evm/coverage/src/lib.rs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -294,6 +294,8 @@ pub enum CoverageItemKind {
294294
///
295295
/// The first path has ID 0, the next ID 1, and so on.
296296
path_id: usize,
297+
/// If true, then the branch anchor is the first opcode within the branch source range.
298+
is_first_opcode: bool,
297299
},
298300
/// A branch in the code.
299301
SinglePathBranch {
@@ -326,7 +328,7 @@ impl Display for CoverageItem {
326328
CoverageItemKind::Statement => {
327329
write!(f, "Statement")?;
328330
}
329-
CoverageItemKind::Branch { branch_id, path_id } => {
331+
CoverageItemKind::Branch { branch_id, path_id, .. } => {
330332
write!(f, "Branch (branch: {branch_id}, path: {path_id})")?;
331333
}
332334
CoverageItemKind::SinglePathBranch { branch_id } => {

‎crates/forge/src/coverage.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ impl<'a> CoverageReporter for LcovReporter<'a> {
109109
CoverageItemKind::Line => {
110110
writeln!(self.destination, "DA:{line},{hits}")?;
111111
}
112-
CoverageItemKind::Branch { branch_id, path_id } => {
112+
CoverageItemKind::Branch { branch_id, path_id, .. } => {
113113
writeln!(
114114
self.destination,
115115
"BRDA:{line},{branch_id},{path_id},{}",

‎crates/forge/tests/cli/coverage.rs‎

Lines changed: 93 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1094,10 +1094,10 @@ contract AContractTest is DSTest {
10941094
.assert_success()
10951095
.stdout_eq(str![[r#"
10961096
...
1097-
| File | % Lines | % Statements | % Branches | % Funcs |
1098-
|-------------------|---------------|--------------|--------------|---------------|
1099-
| src/AContract.sol | 100.00% (4/4) | 50.00% (2/4) | 50.00% (2/4) | 100.00% (1/1) |
1100-
| Total | 100.00% (4/4) | 50.00% (2/4) | 50.00% (2/4) | 100.00% (1/1) |
1097+
| File | % Lines | % Statements | % Branches | % Funcs |
1098+
|-------------------|--------------|--------------|--------------|---------------|
1099+
| src/AContract.sol | 50.00% (2/4) | 50.00% (2/4) | 50.00% (2/4) | 100.00% (1/1) |
1100+
| Total | 50.00% (2/4) | 50.00% (2/4) | 50.00% (2/4) | 100.00% (1/1) |
11011101
11021102
"#]]);
11031103

@@ -1113,3 +1113,92 @@ contract AContractTest is DSTest {
11131113
"#]],
11141114
);
11151115
});
1116+
1117+
// https://github.com/foundry-rs/foundry/issues/8604
1118+
forgetest!(test_branch_with_calldata_reads, |prj, cmd| {
1119+
prj.insert_ds_test();
1120+
prj.add_source(
1121+
"AContract.sol",
1122+
r#"
1123+
contract AContract {
1124+
event IsTrue(bool isTrue);
1125+
event IsFalse(bool isFalse);
1126+
1127+
function execute(bool[] calldata isTrue) external {
1128+
for (uint256 i = 0; i < isTrue.length; i++) {
1129+
if (isTrue[i]) {
1130+
emit IsTrue(isTrue[i]);
1131+
} else {
1132+
emit IsFalse(!isTrue[i]);
1133+
}
1134+
}
1135+
}
1136+
}
1137+
"#,
1138+
)
1139+
.unwrap();
1140+
1141+
prj.add_source(
1142+
"AContractTest.sol",
1143+
r#"
1144+
import "./test.sol";
1145+
import {AContract} from "./AContract.sol";
1146+
1147+
contract AContractTest is DSTest {
1148+
function testTrueCoverage() external {
1149+
AContract a = new AContract();
1150+
bool[] memory isTrue = new bool[](1);
1151+
isTrue[0] = true;
1152+
a.execute(isTrue);
1153+
}
1154+
1155+
function testFalseCoverage() external {
1156+
AContract a = new AContract();
1157+
bool[] memory isFalse = new bool[](1);
1158+
isFalse[0] = false;
1159+
a.execute(isFalse);
1160+
}
1161+
}
1162+
"#,
1163+
)
1164+
.unwrap();
1165+
1166+
// Assert 50% coverage for true branches.
1167+
cmd.arg("coverage")
1168+
.args(["--mt".to_string(), "testTrueCoverage".to_string()])
1169+
.assert_success()
1170+
.stdout_eq(str![[r#"
1171+
...
1172+
| File | % Lines | % Statements | % Branches | % Funcs |
1173+
|-------------------|--------------|--------------|--------------|---------------|
1174+
| src/AContract.sol | 75.00% (3/4) | 80.00% (4/5) | 50.00% (1/2) | 100.00% (1/1) |
1175+
| Total | 75.00% (3/4) | 80.00% (4/5) | 50.00% (1/2) | 100.00% (1/1) |
1176+
1177+
"#]]);
1178+
1179+
// Assert 50% coverage for false branches.
1180+
cmd.forge_fuse()
1181+
.arg("coverage")
1182+
.args(["--mt".to_string(), "testFalseCoverage".to_string()])
1183+
.assert_success()
1184+
.stdout_eq(str![[r#"
1185+
...
1186+
| File | % Lines | % Statements | % Branches | % Funcs |
1187+
|-------------------|--------------|--------------|--------------|---------------|
1188+
| src/AContract.sol | 50.00% (2/4) | 80.00% (4/5) | 50.00% (1/2) | 100.00% (1/1) |
1189+
| Total | 50.00% (2/4) | 80.00% (4/5) | 50.00% (1/2) | 100.00% (1/1) |
1190+
1191+
"#]]);
1192+
1193+
// Assert 100% coverage (true/false branches properly covered).
1194+
cmd.forge_fuse().arg("coverage").args(["--summary".to_string()]).assert_success().stdout_eq(
1195+
str![[r#"
1196+
...
1197+
| File | % Lines | % Statements | % Branches | % Funcs |
1198+
|-------------------|---------------|---------------|---------------|---------------|
1199+
| src/AContract.sol | 100.00% (4/4) | 100.00% (5/5) | 100.00% (2/2) | 100.00% (1/1) |
1200+
| Total | 100.00% (4/4) | 100.00% (5/5) | 100.00% (2/2) | 100.00% (1/1) |
1201+
1202+
"#]],
1203+
);
1204+
});

0 commit comments

Comments
 (0)