Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(coverage): proper instruction for 1st branch anchor #8512

Merged
merged 3 commits into from
Jul 25, 2024

Conversation

grandizzy
Copy link
Collaborator

Motivation

Re https://t.me/foundry_support/55383
PR #8414 fixed the branch coverage report (issue reported in #7784 ) by properly mapping if/else source bodies

match node.attribute::<Node>("falseBody") {
// Both if/else statements.
Some(false_body) => {
// Add the coverage item for branch 1 (false body).
// The relevant source range for the false branch is the `else` statement
// itself and the false body of the else statement.

but it also made the wrong assumption that the first branch opcode after JUMPI should be pc + 1

ItemAnchor {
item_id,
// The first branch is the opcode directly after JUMPI
instruction: pc + 1,
},

This is not true because the evaluation is done for the opcode before JUMPI (so JUMPI is pc + 1 and next opcode is pc + 2)

// Check if we are in the source range we are interested in, and if the next opcode
// is a JUMPI
if is_in_source_range(element, loc) && bytecode[pc + 1] == opcode::JUMPI {

Solution

Proper source mappings fixes #7784 and #4294 (require and assert branches properly reported now), hence this PR reverts the instruction for first branch and adds back branching for asserts
Added unit tests for all require/assert branches
#4309 #4310 and #4315 tests were passing with wrong branch opcode, will follow up with a fix for them

@grandizzy grandizzy marked this pull request as ready for review July 24, 2024 16:49
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool

@grandizzy grandizzy merged commit 644bb31 into foundry-rs:master Jul 25, 2024
20 checks passed
@grandizzy grandizzy deleted the fix-coverage branch July 25, 2024 08:08
benwjhack pushed a commit to CompassLabs/foundry-test that referenced this pull request Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

forge coverage produces incorrect output
2 participants