feat(coverage): better branch handling #2133
Conversation
It's possible to have multiple functions defined in the same source file with the same name. For example, you could define two tokens in the same file - each would have a transfer function, so we need to ensure the function names are unique by prepending the contract name.
onbjerg
changed the title
evm/src/coverage/visitor.rs
Outdated
| .unwrap_or(loc.start); | ||
| .ok_or_else(|| { | ||
| eyre::eyre!("could not find anchor: no matching instruction in range") | ||
| })?; |
There was a problem hiding this comment.
This is more "brittle" in the sense that we don't have a fallback, but the prior fallback made 0 sense so it's better this way
onbjerg
force-pushed
the
onbjerg/coverage-branches
branch
from
f18c406
to
a71e740
Compare
onbjerg
force-pushed
the
onbjerg/coverage-branches
branch
from
a71e740
to
d982559
Compare
onbjerg
force-pushed
the
onbjerg/coverage-branches
branch
from
765a39b
to
34080de
Compare
|
I've run this on Lil Web3 and my coverage testing repo and it seems to find some uncovered branches, and those seem to line up with what is actually uncovered. For example, in Lil Web3 it found some branches that have not been tested :) |
| let element = if let Some(element) = source_map.get(pc - cumulative_push_size) { | ||
| element | ||
| } else { | ||
| // NOTE(onbjerg): For some reason the last few bytes of the bytecode do not have | ||
| // a source map associated, so at that point we just stop searching | ||
| break | ||
| }; |
There was a problem hiding this comment.
This is a bit weird, but I'm unsure if it's intentional?
evm/src/coverage/visitor.rs
Outdated
| /// The source code that contains the AST being walked. | ||
| source: String, | ||
| /// Source maps for this specific source file, keyed by the contract name. | ||
| source_maps: HashMap<String, SourceMap>, | ||
| /// Bytecodes for this specific source file, keyed by the contract name. | ||
| bytecodes: HashMap<String, Cow<'a, Bytes>>, |
There was a problem hiding this comment.
Bytes is cheaply cloneable because it's basically just an AtomicPointer and a * u8 pointer to the data, so no need to use cow, I think
There was a problem hiding this comment.
the cow is from ethers-rs 😄 but yes i agree
There was a problem hiding this comment.
oh, maybe I should change that, I probably introduced this by copy-pasting the other functions...
Some small refactoring and solution for #1967
Motivation
We want good branch coverage in
forge coverage, which means detecting the following scenarios:assertandrequireshould result in branching (one branch reverts, the other one doesn't)Solution
assertandrequirerequire us to inspect the bytecode that the source map identifies as representing them. Generally, this bytecode follows the format:That is, we need to find bytecode that identifies an
assertor arequireand look for a JUMPI preceded by a PUSH. The first branch is the instruction directly after JUMPI, because this branch will eventually end in a REVERT. The second branch is the instruction at the PC we pushed before JUMPI. Both of these locations need to be translated to instruction counters (i.e. program counters minus any push bytes leading up to that program counter) because this is how the source maps are indexed.For if statements, the general format is somewhat the same:
There is a special case though: if you have more complex conditions for
assert,requireor your if statement, then there might be multiple sections of bytecode that match the same template. The last piece of bytecode that matches this template seems to be the "main" jump, so we always need to find the last JUMPI to find the branches.Closes #1967