Add snapshot test support for coverage implementation (#3368)

Add a snapshot-based test to our coverage implementation. This adds coverage for a bunch of code which did not previously have it, and allows us to easily see that coverage is being generated as expected.

A fix was made to the recording code to remove the use of `TargetAllowList` as it was easy to misconfigure `CoverageRecorder` when using it. The use of `TargetAllowList` is now only a container struct in our generic coverage task.

src/agent/coverage/Cargo.toml

@@ -4,6 +4,9 @@ version = "0.1.0"
 edition = "2021"
 license = "MIT"
 
+[features]
+slow-tests = []
+
 [dependencies]
 anyhow = { version = "1.0", features = ["backtrace"] }
 cobertura = { path = "../cobertura" }
@@ -32,3 +35,8 @@ procfs = { version = "0.12", default-features = false, features = ["flate2"] }
 clap = { version = "4.3", features = ["derive"] }
 env_logger = "0.10.0"
 pretty_assertions = "1.4.0"
+insta = { version = "1.31.0", features = ["glob"] }
+coverage = { path = "../coverage" }
+cc = "1.0"
+tempfile = "3.7.0"
+dunce = "1.0"

src/agent/coverage/examples/record.rs

@@ -2,10 +2,10 @@ use std::process::Command;
 use std::sync::Arc;
 use std::time::Duration;
 
-use anyhow::{bail, Result};
+use anyhow::{bail, Context, Result};
 use clap::Parser;
 use cobertura::CoberturaCoverage;
-use coverage::allowlist::{AllowList, TargetAllowList};
+use coverage::allowlist::AllowList;
 use coverage::binary::{BinaryCoverage, DebugInfoCache};
 use coverage::record::{CoverageRecorder, Recorded};
 use debuggable_module::load_module::LoadModule;
@@ -57,19 +57,21 @@ fn main() -> Result<()> {
         .map(Duration::from_millis)
         .unwrap_or(DEFAULT_TIMEOUT);
 
-    let mut allowlist = TargetAllowList::default();
+    let module_allowlist = args
+        .module_allowlist
+        .map(AllowList::load)
+        .unwrap_or_else(|| Ok(AllowList::default()))
+        .context("loading module allowlist")?;
 
-    if let Some(path) = &args.module_allowlist {
-        allowlist.modules = AllowList::load(path)?;
-    }
-
-    if let Some(path) = &args.source_allowlist {
-        allowlist.source_files = AllowList::load(path)?;
-    }
+    let source_allowlist = args
+        .source_allowlist
+        .map(AllowList::load)
+        .unwrap_or_else(|| Ok(AllowList::default()))
+        .context("loading source allowlist")?;
 
     let mut coverage = BinaryCoverage::default();
     let loader = Arc::new(Loader::new());
-    let cache = Arc::new(DebugInfoCache::new(allowlist.source_files.clone()));
+    let cache = Arc::new(DebugInfoCache::new(source_allowlist.clone()));
 
     let t = std::time::Instant::now();
     precache_target(&args.command[0], &loader, &cache)?;
@@ -84,7 +86,7 @@ fn main() -> Result<()> {
 
             let t = std::time::Instant::now();
             let recorded = CoverageRecorder::new(cmd)
-                .allowlist(allowlist.clone())
+                .module_allowlist(module_allowlist.clone())
                 .loader(loader.clone())
                 .debuginfo_cache(cache.clone())
                 .timeout(timeout)
@@ -102,7 +104,7 @@ fn main() -> Result<()> {
 
         let t = std::time::Instant::now();
         let recorded = CoverageRecorder::new(cmd)
-            .allowlist(allowlist.clone())
+            .module_allowlist(module_allowlist)
             .loader(loader)
             .debuginfo_cache(cache)
             .timeout(timeout)
@@ -118,8 +120,8 @@ fn main() -> Result<()> {
 
     match args.output {
         OutputFormat::ModOff => dump_modoff(&coverage)?,
-        OutputFormat::Source => dump_source_line(&coverage, allowlist.source_files)?,
-        OutputFormat::Cobertura => dump_cobertura(&coverage, allowlist.source_files)?,
+        OutputFormat::Source => dump_source_line(&coverage, source_allowlist)?,
+        OutputFormat::Cobertura => dump_cobertura(&coverage, source_allowlist)?,
     }
 
     Ok(())

src/agent/coverage/src/allowlist.rs

@@ -5,31 +5,6 @@ use anyhow::Result;
 use regex::{Regex, RegexSet};
 use std::path::Path;
 
-#[derive(Clone, Debug, Default)]
-pub struct TargetAllowList {
-    pub modules: AllowList,
-    pub source_files: AllowList,
-}
-
-impl TargetAllowList {
-    pub fn new(modules: AllowList, source_files: AllowList) -> Self {
-        Self {
-            modules,
-            source_files,
-        }
-    }
-
-    #[allow(clippy::field_reassign_with_default)]
-    pub fn extend(&self, other: &Self) -> Self {
-        let mut new = Self::default();
-
-        new.modules = self.modules.extend(&other.modules);
-        new.source_files = self.source_files.extend(&other.source_files);
-
-        new
-    }
-}
-
 #[derive(Clone, Debug)]
 pub struct AllowList {
     allow: RegexSet,

src/agent/coverage/src/binary.rs

@@ -11,7 +11,7 @@ pub use debuggable_module::{block, path::FilePath, Offset};
 use symbolic::debuginfo::Object;
 use symbolic::symcache::{SymCache, SymCacheConverter};
 
-use crate::allowlist::{AllowList, TargetAllowList};
+use crate::allowlist::AllowList;
 
 #[derive(Clone, Debug, Default, Eq, PartialEq)]
 pub struct BinaryCoverage {
@@ -214,7 +214,7 @@ impl CachedDebugInfo {
 
 pub fn find_coverage_sites(
     module: &dyn Module,
-    allowlist: &TargetAllowList,
+    source_allowlist: &AllowList,
 ) -> Result<ModuleBinaryCoverage> {
     let debuginfo = module.debuginfo()?;
 
@@ -232,7 +232,7 @@ pub fn find_coverage_sites(
     for function in debuginfo.functions() {
         if let Some(location) = symcache.lookup(function.offset.0).next() {
             if let Some(file) = location.file() {
-                if !allowlist.source_files.is_allowed(file.full_path()) {
+                if !source_allowlist.is_allowed(file.full_path()) {
                     debug!(
                         "skipping sweep of `{}:{}` due to excluded source path `{}`",
                         module.executable_path(),
@@ -253,7 +253,7 @@ pub fn find_coverage_sites(
 
                     // Apply allowlists per block, to account for inlining. The `location` values
                     // here describe the top of the inline-inclusive call stack.
-                    if !allowlist.source_files.is_allowed(path) {
+                    if !source_allowlist.is_allowed(path) {
                         continue;
                     }
 

src/agent/coverage/src/lib.rs

@@ -12,7 +12,7 @@ pub mod source;
 mod timer;
 
 #[doc(inline)]
-pub use allowlist::{AllowList, TargetAllowList};
+pub use allowlist::AllowList;
 
 #[doc(inline)]
 pub use record::{CoverageRecorder, Recorded};

src/agent/coverage/src/record.rs

@@ -8,8 +8,8 @@ use std::time::Duration;
 use anyhow::Result;
 use debuggable_module::loader::Loader;
 
-use crate::allowlist::TargetAllowList;
 use crate::binary::{BinaryCoverage, DebugInfoCache};
+use crate::AllowList;
 
 #[cfg(target_os = "linux")]
 pub mod linux;
@@ -18,7 +18,7 @@ pub mod linux;
 pub mod windows;
 
 pub struct CoverageRecorder {
-    allowlist: TargetAllowList,
+    module_allowlist: AllowList,
     cache: Arc<DebugInfoCache>,
     cmd: Command,
     loader: Arc<Loader>,
@@ -30,22 +30,20 @@ impl CoverageRecorder {
         cmd.stdout(Stdio::piped());
         cmd.stderr(Stdio::piped());
 
-        let allowlist = TargetAllowList::default();
-        let cache = Arc::new(DebugInfoCache::new(allowlist.source_files.clone()));
         let loader = Arc::new(Loader::new());
         let timeout = Duration::from_secs(5);
 
         Self {
-            allowlist,
-            cache,
+            module_allowlist: AllowList::default(),
+            cache: Arc::new(DebugInfoCache::new(AllowList::default())),
             cmd,
             loader,
             timeout,
         }
     }
 
-    pub fn allowlist(mut self, allowlist: TargetAllowList) -> Self {
-        self.allowlist = allowlist;
+    pub fn module_allowlist(mut self, module_allowlist: AllowList) -> Self {
+        self.module_allowlist = module_allowlist;
         self
     }
 
@@ -82,7 +80,7 @@ impl CoverageRecorder {
             let child_pid = child_pid.clone();
 
             timer::timed(self.timeout, move || {
-                let mut recorder = LinuxRecorder::new(&loader, self.allowlist, &self.cache);
+                let mut recorder = LinuxRecorder::new(&loader, self.module_allowlist, &self.cache);
                 let mut dbg = Debugger::new(&mut recorder);
                 let child = dbg.spawn(self.cmd)?;
 
@@ -128,7 +126,8 @@ impl CoverageRecorder {
         let loader = self.loader.clone();
 
         crate::timer::timed(self.timeout, move || {
-            let mut recorder = WindowsRecorder::new(&loader, self.allowlist, &self.cache);
+            let mut recorder =
+                WindowsRecorder::new(&loader, self.module_allowlist, self.cache.as_ref());
             let (mut dbg, child) = Debugger::init(self.cmd, &mut recorder)?;
             dbg.run(&mut recorder)?;
 

src/agent/coverage/src/record/linux.rs

@@ -14,11 +14,11 @@ use pete::Tracee;
 pub mod debugger;
 use debugger::{DebugEventHandler, DebuggerContext, ModuleImage};
 
-use crate::allowlist::TargetAllowList;
+use crate::allowlist::AllowList;
 use crate::binary::{BinaryCoverage, DebugInfoCache};
 
 pub struct LinuxRecorder<'cache, 'data> {
-    allowlist: TargetAllowList,
+    module_allowlist: AllowList,
     cache: &'cache DebugInfoCache,
     pub coverage: BinaryCoverage,
     loader: &'data Loader,
@@ -28,14 +28,14 @@ pub struct LinuxRecorder<'cache, 'data> {
 impl<'cache, 'data> LinuxRecorder<'cache, 'data> {
     pub fn new(
         loader: &'data Loader,
-        allowlist: TargetAllowList,
+        module_allowlist: AllowList,
         cache: &'cache DebugInfoCache,
     ) -> Self {
         let coverage = BinaryCoverage::default();
         let modules = BTreeMap::new();
 
         Self {
-            allowlist,
+            module_allowlist,
             cache,
             coverage,
             loader,
@@ -80,7 +80,7 @@ impl<'cache, 'data> LinuxRecorder<'cache, 'data> {
 
         let path = image.path();
 
-        if !self.allowlist.modules.is_allowed(path) {
+        if !self.module_allowlist.is_allowed(path) {
             debug!("not inserting denylisted module: {path}");
             return Ok(());
         }

src/agent/coverage/src/record/windows.rs

@@ -13,8 +13,8 @@ use debuggable_module::windows::WindowsModule;
 use debuggable_module::{Module, Offset};
 use debugger::{BreakpointId, BreakpointType, DebugEventHandler, Debugger, ModuleLoadInfo};
 
-use crate::allowlist::TargetAllowList;
 use crate::binary::{BinaryCoverage, DebugInfoCache};
+use crate::AllowList;
 
 // For a new module image, we defer setting coverage breakpoints until exit from one of these
 // functions (when present). This avoids breaking hotpatching routines in the ASan interceptor
@@ -26,7 +26,7 @@ const PROCESS_IMAGE_DEFERRAL_TRIGGER: &str = "__asan::AsanInitInternal(";
 const LIBRARY_IMAGE_DEFERRAL_TRIGGER: &str = "DllMain(";
 
 pub struct WindowsRecorder<'cache, 'data> {
-    allowlist: TargetAllowList,
+    module_allowlist: AllowList,
     breakpoints: Breakpoints,
     cache: &'cache DebugInfoCache,
     deferred_breakpoints: BTreeMap<BreakpointId, (Breakpoint, DeferralState)>,
@@ -39,7 +39,7 @@ pub struct WindowsRecorder<'cache, 'data> {
 impl<'cache, 'data> WindowsRecorder<'cache, 'data> {
     pub fn new(
         loader: &'data Loader,
-        allowlist: TargetAllowList,
+        module_allowlist: AllowList,
         cache: &'cache DebugInfoCache,
     ) -> Self {
         let breakpoints = Breakpoints::default();
@@ -49,7 +49,7 @@ impl<'cache, 'data> WindowsRecorder<'cache, 'data> {
         let stop_error = None;
 
         Self {
-            allowlist,
+            module_allowlist,
             breakpoints,
             cache,
             deferred_breakpoints,
@@ -60,12 +60,12 @@ impl<'cache, 'data> WindowsRecorder<'cache, 'data> {
         }
     }
 
-    pub fn allowlist(&self) -> &TargetAllowList {
-        &self.allowlist
+    pub fn module_allowlist(&self) -> &AllowList {
+        &self.module_allowlist
     }
 
-    pub fn allowlist_mut(&mut self) -> &mut TargetAllowList {
-        &mut self.allowlist
+    pub fn module_allowlist_mut(&mut self) -> &mut AllowList {
+        &mut self.module_allowlist
     }
 
     fn try_on_create_process(&mut self, dbg: &mut Debugger, module: &ModuleLoadInfo) -> Result<()> {
@@ -163,7 +163,7 @@ impl<'cache, 'data> WindowsRecorder<'cache, 'data> {
             return Ok(());
         }
 
-        if !self.allowlist.modules.is_allowed(&path) {
+        if !self.module_allowlist.is_allowed(&path) {
             debug!("not inserting denylisted module: {path}");
             return Ok(());
         }

src/agent/coverage/src/source.rs

@@ -51,14 +51,14 @@ impl From<Line> for u32 {
 
 pub fn binary_to_source_coverage(
     binary: &BinaryCoverage,
-    allowlist: impl Into<Option<AllowList>>,
+    source_allowlist: impl Into<Option<AllowList>>,
 ) -> Result<SourceCoverage> {
     use std::collections::btree_map::Entry;
 
     use symbolic::debuginfo::Object;
     use symbolic::symcache::{SymCache, SymCacheConverter};
 
-    let allowlist = allowlist.into().unwrap_or_default();
+    let source_allowlist = source_allowlist.into().unwrap_or_default();
     let loader = Loader::new();
 
     let mut source = SourceCoverage::default();
@@ -106,7 +106,7 @@ pub fn binary_to_source_coverage(
 
                         if let Some(file) = location.file() {
                             // Only include relevant inlinees.
-                            if !allowlist.is_allowed(&file.full_path()) {
+                            if !source_allowlist.is_allowed(&file.full_path()) {
                                 continue;
                             }