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

feat(coverage): caching for coverage #9366

Draft
wants to merge 24 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
39c5922
fix(`coverage`): write coverage artifacts to separate dir
yash-atreya Nov 20, 2024
215ac18
Merge branch 'master' into yash/fix-8840
yash-atreya Nov 21, 2024
20b09e8
adjust cache path for coverage
yash-atreya Nov 21, 2024
581cf4c
partial test
yash-atreya Nov 21, 2024
a0d7e19
enable cache
yash-atreya Nov 21, 2024
c59364a
don't assume recompilation
yash-atreya Nov 21, 2024
66cfb3d
fix(`coverage`): dont recompile when cache exists
yash-atreya Nov 21, 2024
730b26d
Merge branch 'master' into yash/fix-8840
yash-atreya Nov 21, 2024
0eadcfa
account for cached compiled artifacts at once
yash-atreya Nov 22, 2024
9902adf
test with multi solc versions
yash-atreya Nov 22, 2024
0433863
fix(`forge clean`): remove cache/coverage dir
yash-atreya Nov 22, 2024
e90a354
add test
klkvr Nov 22, 2024
7d639e6
fix(`coverage`): account for `build_id` in source identification (#9…
yash-atreya Nov 27, 2024
2b1af19
fix
yash-atreya Nov 29, 2024
498e427
rm version as key in report
yash-atreya Nov 29, 2024
06f01dc
rm version key from ContractId and report.items
yash-atreya Nov 29, 2024
38db689
fix: scale anchor.items_id
yash-atreya Nov 29, 2024
a6edce9
fix: rm dependence of coverage on version
yash-atreya Nov 29, 2024
8297739
fix: respect artifact.build_id while processing hitmaps
yash-atreya Nov 29, 2024
13d3fd7
fix: respect element idx
yash-atreya Nov 29, 2024
973cfe0
key source_ids by version and path
yash-atreya Dec 2, 2024
2eaab46
Merge branch 'master' into yash/fix-8840
yash-atreya Dec 2, 2024
1624587
nit
yash-atreya Dec 2, 2024
860ff71
Merge branch 'master' into yash/fix-8840
yash-atreya Dec 13, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions crates/config/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -985,6 +985,8 @@ impl Config {
};
remove_test_dir(&self.fuzz.failure_persist_dir);
remove_test_dir(&self.invariant.failure_persist_dir);
remove_test_dir(&Some(self.cache_path.clone().join("coverage")));
remove_test_dir(&Some(self.cache_path.clone()));

// Remove snapshot directory.
let snapshot_dir = project.root().join(&self.snapshots);
Expand Down
1 change: 0 additions & 1 deletion crates/evm/coverage/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,5 @@ foundry-evm-core.workspace = true
alloy-primitives.workspace = true
eyre.workspace = true
revm.workspace = true
semver.workspace = true
tracing.workspace = true
rayon.workspace = true
41 changes: 33 additions & 8 deletions crates/evm/coverage/src/analysis.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
use super::{CoverageItem, CoverageItemKind, SourceLocation};
use alloy_primitives::map::HashMap;
use core::fmt;
use foundry_common::TestFunctionExt;
use foundry_compilers::artifacts::ast::{self, Ast, Node, NodeType};
use rayon::prelude::*;
use std::sync::Arc;
use std::{borrow::Cow, sync::Arc};

/// A visitor that walks the AST of a single contract and finds coverage items.
#[derive(Clone, Debug)]
pub struct ContractVisitor<'a> {
/// The source ID of the contract.
source_id: usize,
source_id: SourceIdentifier,
/// The source code that contains the AST being walked.
source: &'a str,

Expand All @@ -26,7 +27,7 @@ pub struct ContractVisitor<'a> {
}

impl<'a> ContractVisitor<'a> {
pub fn new(source_id: usize, source: &'a str, contract_name: &'a Arc<str>) -> Self {
pub fn new(source_id: SourceIdentifier, source: &'a str, contract_name: &'a Arc<str>) -> Self {
Self { source_id, source, contract_name, branch_id: 0, last_line: 0, items: Vec::new() }
}

Expand Down Expand Up @@ -473,7 +474,7 @@ impl<'a> ContractVisitor<'a> {
let loc_start =
self.source.char_indices().map(|(i, _)| i).nth(loc.start).unwrap_or_default();
SourceLocation {
source_id: self.source_id,
source_id: self.source_id.clone(),
contract_name: self.contract_name.clone(),
start: loc.start as u32,
length: loc.length.map(|x| x as u32),
Expand Down Expand Up @@ -538,7 +539,7 @@ impl<'a> SourceAnalyzer<'a> {
.sources
.sources
.par_iter()
.flat_map_iter(|(&source_id, SourceFile { source, ast })| {
.flat_map_iter(|(source_id, SourceFile { source, ast })| {
ast.nodes.iter().map(move |node| {
if !matches!(node.node_type, NodeType::ContractDefinition) {
return Ok(vec![]);
Expand All @@ -556,7 +557,7 @@ impl<'a> SourceAnalyzer<'a> {
.attribute("name")
.ok_or_else(|| eyre::eyre!("Contract has no name"))?;

let mut visitor = ContractVisitor::new(source_id, source, &name);
let mut visitor = ContractVisitor::new(source_id.clone(), source, &name);
visitor.visit_contract(node)?;
let mut items = visitor.items;

Expand All @@ -583,7 +584,31 @@ impl<'a> SourceAnalyzer<'a> {
#[derive(Debug, Default)]
pub struct SourceFiles<'a> {
/// The versioned sources.
pub sources: HashMap<usize, SourceFile<'a>>,
/// Keyed by build_id and source_id.
pub sources: HashMap<SourceIdentifier, SourceFile<'a>>,
}

/// Serves as a unique identifier for sources across multiple compiler runs.
#[derive(Clone, Debug, PartialEq, Eq, Hash)]
pub struct SourceIdentifier {
/// Source ID is unique for each source file per compilation job but may not be across
/// different jobs.
pub source_id: usize,
/// Artifact build id is same for all sources in a single compilation job. But always unique
/// across different jobs.
pub build_id: String,
}

impl fmt::Display for SourceIdentifier {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "source_id={} build_id={}", self.source_id, self.build_id,)
}
}

impl SourceIdentifier {
pub fn new(source_id: usize, build_id: String) -> Self {
Self { source_id, build_id }
}
}

/// The source code and AST of a file.
Expand All @@ -592,5 +617,5 @@ pub struct SourceFile<'a> {
/// The source code.
pub source: String,
/// The AST of the source code.
pub ast: &'a Ast,
pub ast: Cow<'a, Ast>,
}
13 changes: 10 additions & 3 deletions crates/evm/coverage/src/anchors.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use crate::analysis::SourceIdentifier;

use super::{CoverageItem, CoverageItemKind, ItemAnchor, SourceLocation};
use alloy_primitives::map::{DefaultHashBuilder, HashMap, HashSet};
use eyre::ensure;
Expand All @@ -11,12 +13,16 @@ pub fn find_anchors(
source_map: &SourceMap,
ic_pc_map: &IcPcMap,
items: &[CoverageItem],
items_by_source_id: &HashMap<usize, Vec<usize>>,
items_by_source_id: &HashMap<SourceIdentifier, Vec<usize>>,
source_id: &SourceIdentifier,
) -> Vec<ItemAnchor> {
let mut seen = HashSet::with_hasher(DefaultHashBuilder::default());
source_map
.iter()
.filter_map(|element| items_by_source_id.get(&(element.index()? as usize)))
.filter_map(|element| {
items_by_source_id
.get(&SourceIdentifier::new(element.index()? as usize, source_id.build_id.clone()))
})
.flatten()
.filter_map(|&item_id| {
if !seen.insert(item_id) {
Expand Down Expand Up @@ -171,7 +177,8 @@ pub fn find_anchor_branch(
/// Calculates whether `element` is within the range of the target `location`.
fn is_in_source_range(element: &SourceElement, location: &SourceLocation) -> bool {
// Source IDs must match.
let source_ids_match = element.index().is_some_and(|a| a as usize == location.source_id);
let source_ids_match =
element.index().is_some_and(|a| a as usize == location.source_id.source_id);
if !source_ids_match {
return false;
}
Expand Down
66 changes: 31 additions & 35 deletions crates/evm/coverage/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ extern crate foundry_common;
extern crate tracing;

use alloy_primitives::{map::HashMap, Bytes, B256};
use analysis::SourceIdentifier;
use eyre::{Context, Result};
use foundry_compilers::artifacts::sourcemap::SourceMap;
use semver::Version;
use std::{
collections::BTreeMap,
fmt::Display,
Expand All @@ -36,29 +36,31 @@ pub use inspector::CoverageCollector;
#[derive(Clone, Debug, Default)]
pub struct CoverageReport {
/// A map of source IDs to the source path.
pub source_paths: HashMap<(Version, usize), PathBuf>,
pub source_paths: HashMap<SourceIdentifier, PathBuf>,
/// A map of source paths to source IDs.
pub source_paths_to_ids: HashMap<(Version, PathBuf), usize>,
pub source_paths_to_ids: HashMap<PathBuf, SourceIdentifier>,
yash-atreya marked this conversation as resolved.
Show resolved Hide resolved
/// All coverage items for the codebase, keyed by the compiler version.
pub items: HashMap<Version, Vec<CoverageItem>>,
pub items: HashMap<SourceIdentifier, Vec<CoverageItem>>,
Copy link
Member

Choose a reason for hiding this comment

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

this should now be a Vec<CoverageItem> I guess given that we no longer have a version-level separation

/// All item anchors for the codebase, keyed by their contract ID.
pub anchors: HashMap<ContractId, (Vec<ItemAnchor>, Vec<ItemAnchor>)>,
/// All the bytecode hits for the codebase.
pub bytecode_hits: HashMap<ContractId, HitMap>,
/// The bytecode -> source mappings.
pub source_maps: HashMap<ContractId, (SourceMap, SourceMap)>,
/// Anchor Item ID by source ID
pub item_ids_by_source_id: HashMap<SourceIdentifier, Vec<usize>>,
Copy link
Member

Choose a reason for hiding this comment

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

this seems unused?

}

impl CoverageReport {
/// Add a source file path.
pub fn add_source(&mut self, version: Version, source_id: usize, path: PathBuf) {
self.source_paths.insert((version.clone(), source_id), path.clone());
self.source_paths_to_ids.insert((version, path), source_id);
pub fn add_source(&mut self, source_id: SourceIdentifier, path: PathBuf) {
self.source_paths.insert(source_id.clone(), path.clone());
self.source_paths_to_ids.insert(path, source_id);
}

/// Get the source ID for a specific source file path.
pub fn get_source_id(&self, version: Version, path: PathBuf) -> Option<usize> {
self.source_paths_to_ids.get(&(version, path)).copied()
pub fn get_source_id(&self, path: PathBuf) -> Option<SourceIdentifier> {
self.source_paths_to_ids.get(&path).cloned()
}

/// Add the source maps.
Expand All @@ -70,8 +72,10 @@ impl CoverageReport {
}

/// Add coverage items to this report.
pub fn add_items(&mut self, version: Version, items: impl IntoIterator<Item = CoverageItem>) {
self.items.entry(version).or_default().extend(items);
pub fn add_items(&mut self, items: impl IntoIterator<Item = CoverageItem>) {
for item in items.into_iter() {
self.items.entry(item.loc.source_id.clone()).or_default().push(item);
}
}

/// Add anchors to this report.
Expand All @@ -86,11 +90,9 @@ impl CoverageReport {
pub fn summary_by_file(&self) -> impl Iterator<Item = (PathBuf, CoverageSummary)> {
let mut summaries = BTreeMap::new();

for (version, items) in self.items.iter() {
for (source_id, items) in self.items.iter() {
for item in items {
let Some(path) =
self.source_paths.get(&(version.clone(), item.loc.source_id)).cloned()
else {
let Some(path) = self.source_paths.get(source_id).cloned() else {
continue;
};
*summaries.entry(path).or_default() += item;
Expand All @@ -104,11 +106,9 @@ impl CoverageReport {
pub fn items_by_source(&self) -> impl Iterator<Item = (PathBuf, Vec<CoverageItem>)> {
let mut items_by_source: BTreeMap<_, Vec<_>> = BTreeMap::new();

for (version, items) in self.items.iter() {
for (source_id, items) in self.items.iter() {
for item in items {
let Some(path) =
self.source_paths.get(&(version.clone(), item.loc.source_id)).cloned()
else {
let Some(path) = self.source_paths.get(source_id).cloned() else {
continue;
};
items_by_source.entry(path).or_default().push(item.clone());
Expand Down Expand Up @@ -142,8 +142,12 @@ impl CoverageReport {
for anchor in anchors {
if let Some(&hits) = hit_map.hits.get(&anchor.instruction) {
self.items
.get_mut(&contract_id.version)
.and_then(|items| items.get_mut(anchor.item_id))
.get_mut(&contract_id.source_id)
.and_then(|items| {
let scaled_item_id = anchor.item_id % items.len();
Copy link
Member

Choose a reason for hiding this comment

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

what is this for?

Copy link
Member

Choose a reason for hiding this comment

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

this doesn't look correct. right now items are keyed by the source_id they are from but this tries to get it by the source id of the contract where anchor was found


items.get_mut(scaled_item_id)
})
.expect("Anchor refers to non-existent coverage item")
.hits += hits;
}
Expand All @@ -157,12 +161,9 @@ impl CoverageReport {
/// This function should only be called after all the sources were used, otherwise, the output
/// will be missing the ones that are dependent on them.
pub fn filter_out_ignored_sources(&mut self, filter: impl Fn(&Path) -> bool) {
self.items.retain(|version, items| {
items.retain(|item| {
self.source_paths
.get(&(version.clone(), item.loc.source_id))
.map(|path| filter(path))
.unwrap_or(false)
self.items.retain(|source_id, items| {
items.retain(|_item| {
self.source_paths.get(source_id).map(|path| filter(path)).unwrap_or(false)
});
!items.is_empty()
});
Expand Down Expand Up @@ -258,18 +259,13 @@ impl HitMap {
/// A unique identifier for a contract
#[derive(Clone, Debug, PartialEq, Eq, Hash)]
pub struct ContractId {
pub version: Version,
pub source_id: usize,
pub source_id: SourceIdentifier,
pub contract_name: Arc<str>,
}

impl Display for ContractId {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(
f,
"Contract \"{}\" (solc {}, source ID {})",
self.contract_name, self.version, self.source_id
)
write!(f, "Contract \"{}\" source ID {}", self.contract_name, self.source_id)
}
}

Expand Down Expand Up @@ -348,7 +344,7 @@ impl Display for CoverageItem {
#[derive(Clone, Debug)]
pub struct SourceLocation {
/// The source ID.
pub source_id: usize,
pub source_id: SourceIdentifier,
/// The contract this source range is in.
pub contract_name: Arc<str>,
/// Start byte in the source code.
Expand Down
Loading