From 710fa3311b5b05e10f3b6ee32c6e611d25af5371 Mon Sep 17 00:00:00 2001 From: "Brian R. Murphy" <132495859+brmataptos@users.noreply.github.com> Date: Fri, 1 Nov 2024 21:41:39 -0700 Subject: [PATCH 1/6] Improve error messages for coverage file issues --- crates/aptos/Cargo.toml | 2 +- crates/aptos/src/common/types.rs | 2 +- crates/aptos/src/move_tool/coverage.rs | 2 +- .../function_generator.rs | 3 +- .../move-bytecode-source-map/src/utils.rs | 20 ++-- .../move/tools/move-cli/src/base/coverage.rs | 2 +- .../move/tools/move-cli/src/base/test.rs | 4 +- .../move/tools/move-cli/src/test/mod.rs | 2 +- .../src/bin/coverage-summaries.rs | 100 ++++++++++++++---- .../move-coverage/src/bin/source-coverage.rs | 23 ++-- .../tools/move-coverage/src/coverage_map.rs | 53 ++++++---- .../move/tools/move-package/Cargo.toml | 2 +- .../move/tools/move-package/src/lib.rs | 4 +- 13 files changed, 149 insertions(+), 70 deletions(-) diff --git a/crates/aptos/Cargo.toml b/crates/aptos/Cargo.toml index 40b119763b4f2..6372696b598ef 100644 --- a/crates/aptos/Cargo.toml +++ b/crates/aptos/Cargo.toml @@ -13,7 +13,7 @@ repository = { workspace = true } rust-version = { workspace = true } [dependencies] -anyhow = { workspace = true } +anyhow = { workspace = true, features = [ "backtrace" ] } aptos-api-types = { workspace = true } aptos-backup-cli = { workspace = true } aptos-bitvec = { workspace = true } diff --git a/crates/aptos/src/common/types.rs b/crates/aptos/src/common/types.rs index 4fe5fa58c3a02..8a72e3b3f8687 100644 --- a/crates/aptos/src/common/types.rs +++ b/crates/aptos/src/common/types.rs @@ -18,7 +18,7 @@ use crate::{ genesis::git::from_yaml, move_tool::{ArgWithType, FunctionArgType, MemberId}, }; -use anyhow::{bail, Context}; +use anyhow::{bail, Context, Result}; use aptos_api_types::ViewFunction; use aptos_crypto::{ ed25519::{Ed25519PrivateKey, Ed25519PublicKey, Ed25519Signature}, diff --git a/crates/aptos/src/move_tool/coverage.rs b/crates/aptos/src/move_tool/coverage.rs index 366b6bb9472f9..53add94da75b6 100644 --- a/crates/aptos/src/move_tool/coverage.rs +++ b/crates/aptos/src/move_tool/coverage.rs @@ -184,7 +184,7 @@ fn compile_coverage( let path = move_options.get_package_path()?; let coverage_map = - CoverageMap::from_binary_file(path.join(".coverage_map.mvcov")).map_err(|err| { + CoverageMap::from_binary_file(&path.join(".coverage_map.mvcov")).map_err(|err| { CliError::UnexpectedError(format!("Failed to retrieve coverage map {}", err)) })?; let package = config diff --git a/third_party/move/move-compiler-v2/src/file_format_generator/function_generator.rs b/third_party/move/move-compiler-v2/src/file_format_generator/function_generator.rs index e35c6257716cb..9f2457f25cf3a 100644 --- a/third_party/move/move-compiler-v2/src/file_format_generator/function_generator.rs +++ b/third_party/move/move-compiler-v2/src/file_format_generator/function_generator.rs @@ -108,12 +108,13 @@ impl<'a> FunctionGenerator<'a> { fun_env: FunctionEnv<'b>, acquires_list: &BTreeSet, ) { + let id_loc = fun_env.get_id_loc(); let loc = fun_env.get_id_loc(); let function = gen.function_index(ctx, &loc, &fun_env); let visibility = fun_env.visibility(); let fun_count = gen.module.function_defs.len(); let def_idx = FunctionDefinitionIndex::new(ctx.checked_bound( - &loc, + &id_loc, fun_count, MAX_FUNCTION_DEF_COUNT, "defined function", diff --git a/third_party/move/move-ir-compiler/move-bytecode-source-map/src/utils.rs b/third_party/move/move-ir-compiler/move-bytecode-source-map/src/utils.rs index f73d1203451c3..7a262f4f14ce9 100644 --- a/third_party/move/move-ir-compiler/move-bytecode-source-map/src/utils.rs +++ b/third_party/move/move-ir-compiler/move-bytecode-source-map/src/utils.rs @@ -3,7 +3,7 @@ // SPDX-License-Identifier: Apache-2.0 use crate::source_map::SourceMap; -use anyhow::{format_err, Result}; +use anyhow::{Context, Result}; use move_ir_types::location::Loc; use std::{fs::File, io::Read, path::Path}; @@ -13,9 +13,17 @@ pub type Errors = Vec; pub fn source_map_from_file(file_path: &Path) -> Result { let mut bytes = Vec::new(); File::open(file_path) - .ok() - .and_then(|mut file| file.read_to_end(&mut bytes).ok()) - .ok_or_else(|| format_err!("Error while reading in source map information"))?; - bcs::from_bytes::(&bytes) - .map_err(|_| format_err!("Error deserializing into source map")) + .and_then(|mut file| file.read_to_end(&mut bytes)) + .with_context(|| { + format!( + "Reading in source map information for file {}", + file_path.to_string_lossy(), + ) + })?; + bcs::from_bytes::(&bytes).with_context(|| { + format!( + "Deserializing source map information for file {}", + file_path.to_string_lossy() + ) + }) } diff --git a/third_party/move/tools/move-cli/src/base/coverage.rs b/third_party/move/tools/move-cli/src/base/coverage.rs index 07f95117c0613..51a514f31cba1 100644 --- a/third_party/move/tools/move-cli/src/base/coverage.rs +++ b/third_party/move/tools/move-cli/src/base/coverage.rs @@ -60,7 +60,7 @@ pub struct Coverage { impl Coverage { pub fn execute(self, path: Option, config: BuildConfig) -> anyhow::Result<()> { let path = reroot_path(path)?; - let coverage_map = CoverageMap::from_binary_file(path.join(".coverage_map.mvcov"))?; + let coverage_map = CoverageMap::from_binary_file(&path.join(".coverage_map.mvcov"))?; let package = config.compile_package(&path, &mut Vec::new())?; let modules: Vec<_> = package .root_modules() diff --git a/third_party/move/tools/move-cli/src/base/test.rs b/third_party/move/tools/move-cli/src/base/test.rs index 955e32a8ed8f0..81b5452411da3 100644 --- a/third_party/move/tools/move-cli/src/base/test.rs +++ b/third_party/move/tools/move-cli/src/base/test.rs @@ -343,8 +343,8 @@ pub fn run_move_unit_tests_with_factory, + #[clap(long = "modules-dir", short = 'm')] + pub modules_dir: Option, /// Optional path for summaries. Printed to stdout if not present. #[clap(long = "summary-path", short = 'o')] pub summary_path: Option, /// Whether function coverage summaries should be displayed #[clap(long = "summarize-functions", short = 'f')] pub summarize_functions: bool, - /// The path to the standard library binary directory for Move + /// The path to the standard library binary directory for Move (e.g., `.../build/MoveStdlib/bytecode_modules`) #[clap(long = "stdlib-path", short = 's')] pub stdlib_path: Option, /// Whether path coverage should be derived (default is instruction coverage) @@ -50,31 +54,68 @@ struct Args { pub csv_output: bool, } -fn get_modules(args: &Args) -> Vec { - let mut modules = Vec::new(); - if let Some(stdlib_path) = &args.stdlib_path { - let stdlib_modules = fs::read_dir(stdlib_path).unwrap().map(|file| { - let bytes = fs::read(file.unwrap().path()).unwrap(); - CompiledModule::deserialize(&bytes).expect("Module blob can't be deserialized") - }); - modules.extend(stdlib_modules); +fn maybe_add_modules( + modules: &mut Vec, + modules_dir: &Option, +) -> anyhow::Result<()> { + if let Some(modules_path) = modules_dir { + let new_modules_files = fs::read_dir(modules_path) + .with_context(|| format!("Reading module directory {}", modules_path.to_string()))?; + let new_modules: Result, _> = new_modules_files + .map(|dirent_or_error| { + dirent_or_error + .with_context(|| { + format!( + "Iterating over module directory {}", + modules_path.to_string() + ) + }) + .and_then(|file| { + fs::read(file.path()) + .map_err(anyhow::Error::from) + .and_then(|bytes| { + CompiledModule::deserialize(&bytes).with_context(|| { + format!("Reading file {}", file.path().to_string_lossy()) + }) + }) + }) + }) + .collect(); + let mut new_modules = new_modules?; + modules.append(&mut new_modules); } + Ok(()) +} + +fn get_modules(args: &Args) -> anyhow::Result> { + let mut modules = Vec::new(); + maybe_add_modules(&mut modules, &args.stdlib_path)?; + maybe_add_modules(&mut modules, &args.modules_dir)?; if let Some(module_binary_path) = &args.module_binary_path { - let bytecode_bytes = fs::read(module_binary_path).expect("Unable to read bytecode file"); - let compiled_module = CompiledModule::deserialize(&bytecode_bytes) - .expect("Module blob can't be deserialized"); + let bytecode_bytes = fs::read(module_binary_path).with_context(|| { + format!( + "Failed to get_modules for module path {}", + module_binary_path + ) + })?; + let compiled_module = CompiledModule::deserialize(&bytecode_bytes).with_context(|| { + format!( + "Filed to get_modules for module path {}", + module_binary_path + ) + })?; modules.push(compiled_module); } if modules.is_empty() { - panic!("No modules provided for coverage checking") + bail!("No modules provided for coverage checking") } - modules + Ok(modules) } -fn main() { +fn main() -> anyhow::Result<()> { let args = Args::parse(); let input_trace_path = Path::new(&args.input_trace_path); @@ -86,12 +127,19 @@ fn main() { None => Box::new(io::stdout()), }; - let modules = get_modules(&args); + let modules = get_modules(&args)?; if args.derive_path_coverage { let trace_map = if args.is_raw_trace_file { - TraceMap::from_trace_file(input_trace_path) + TraceMap::from_trace_file(&input_trace_path).with_context(|| { + format!("Reading trace file {}", input_trace_path.to_string_lossy()) + })? } else { - TraceMap::from_binary_file(input_trace_path) + TraceMap::from_binary_file(&input_trace_path).with_context(|| { + format!( + "Reading binary coverage file {}", + input_trace_path.to_string_lossy() + ) + })? }; if !args.csv_output { format_human_summary( @@ -111,9 +159,16 @@ fn main() { } } else { let coverage_map = if args.is_raw_trace_file { - CoverageMap::from_trace_file(input_trace_path) + CoverageMap::from_trace_file(&input_trace_path).with_context(|| { + format!("Reading trace file {}", input_trace_path.to_string_lossy(),) + })? } else { - CoverageMap::from_binary_file(input_trace_path).unwrap() + CoverageMap::from_binary_file(&input_trace_path).with_context(|| { + format!( + "Reading binary coverage file {}", + input_trace_path.to_string_lossy(), + ) + })? }; let unified_exec_map = coverage_map.to_unified_exec_map(); if !args.csv_output { @@ -133,6 +188,7 @@ fn main() { ) } } + Ok(()) } #[test] diff --git a/third_party/move/tools/move-coverage/src/bin/source-coverage.rs b/third_party/move/tools/move-coverage/src/bin/source-coverage.rs index 36bdc5b0e6754..19ab305570084 100644 --- a/third_party/move/tools/move-coverage/src/bin/source-coverage.rs +++ b/third_party/move/tools/move-coverage/src/bin/source-coverage.rs @@ -4,6 +4,7 @@ #![forbid(unsafe_code)] +use anyhow::{Context, Result}; use clap::Parser; use move_binary_format::file_format::CompiledModule; use move_bytecode_source_map::utils::source_map_from_file; @@ -50,23 +51,29 @@ struct Args { pub tag: TextIndicator, } -fn main() { +fn main() -> Result<()> { let args = Args::parse(); let source_map_extension = SOURCE_MAP_EXTENSION; let coverage_map = if args.is_raw_trace_file { - CoverageMap::from_trace_file(&args.input_trace_path) + CoverageMap::from_trace_file(&args.input_trace_path)? } else { - CoverageMap::from_binary_file(&args.input_trace_path).unwrap() + CoverageMap::from_binary_file(&args.input_trace_path)? }; - let bytecode_bytes = fs::read(&args.module_binary_path).expect("Unable to read bytecode file"); - let compiled_module = - CompiledModule::deserialize(&bytecode_bytes).expect("Module blob can't be deserialized"); + let bytecode_bytes = fs::read(&args.module_binary_path) + .with_context(|| format!("Reading module binary file {}", args.module_binary_path))?; + let compiled_module = CompiledModule::deserialize(&bytecode_bytes) + .with_context(|| format!("Deserializing file {}", args.module_binary_path))?; let source_map = source_map_from_file( &Path::new(&args.module_binary_path).with_extension(source_map_extension), ) - .unwrap(); + .with_context(|| { + format!( + "Reading source map from file {}{}", + args.module_binary_path, source_map_extension, + ) + })?; let source_path = Path::new(&args.source_file_path); let source_cov = SourceCoverageBuilder::new(&compiled_module, &coverage_map, &source_map); @@ -81,7 +88,7 @@ fn main() { let source_coverage = source_cov.compute_source_coverage(source_path); source_coverage .output_source_coverage(&mut coverage_writer, args.color, args.tag) - .unwrap(); + .with_context(|| format!("Outputting source coverage")) } #[test] diff --git a/third_party/move/tools/move-coverage/src/coverage_map.rs b/third_party/move/tools/move-coverage/src/coverage_map.rs index b22e3217dd4b1..45bb7dca7062f 100644 --- a/third_party/move/tools/move-coverage/src/coverage_map.rs +++ b/third_party/move/tools/move-coverage/src/coverage_map.rs @@ -4,7 +4,7 @@ #![forbid(unsafe_code)] -use anyhow::{format_err, Result}; +use anyhow::{format_err, Context, Result}; use move_binary_format::file_format::{CodeOffset, CompiledModule}; use move_core_types::{ account_address::AccountAddress, @@ -61,12 +61,12 @@ impl CoverageMap { /// Takes in a file containing a raw VM trace, and returns an updated coverage map. pub fn update_coverage_from_trace_file + std::fmt::Debug>( mut self, - filename: P, - ) -> Self { - let file = File::open(&filename) - .unwrap_or_else(|_| panic!("Unable to open coverage trace file '{:?}'", filename)); + filename: &P, + ) -> Result { + let file = File::open(filename) + .map_err(|e| format_err!("Unable to open coverage trace file {:?}: {}", filename, e))?; for line in BufReader::new(file).lines() { - let line = line.unwrap(); + let line = line?; let mut splits = line.split(','); // Use a dummy key so that the data structure of the coverage map does not need to be changed let exec_id = "dummy_exec_id"; @@ -87,26 +87,29 @@ impl CoverageMap { assert_eq!(context_segs.pop().unwrap(), "Script",); } } - self + Ok(self) } /// Takes in a file containing a raw VM trace, and returns a coverage map. - pub fn from_trace_file + std::fmt::Debug>(filename: P) -> Self { + pub fn from_trace_file + std::fmt::Debug>(filename: &P) -> Result { let empty_module_map = CoverageMap { exec_maps: BTreeMap::new(), }; - empty_module_map.update_coverage_from_trace_file(filename) + empty_module_map + .update_coverage_from_trace_file(filename) + .with_context(|| format!("Updating coverage from trace file {:?}", filename)) } /// Takes in a file containing a serialized coverage map and returns a coverage map. - pub fn from_binary_file + std::fmt::Debug>(filename: P) -> Result { + pub fn from_binary_file + std::fmt::Debug>(filename: &P) -> Result { let mut bytes = Vec::new(); - File::open(&filename) + File::open(filename) .map_err(|e| format_err!("{}: Coverage map file '{:?}' doesn't exist", e, filename))? .read_to_end(&mut bytes) .ok() .ok_or_else(|| format_err!("Unable to read coverage map"))?; - bcs::from_bytes(&bytes).map_err(|_| format_err!("Error deserializing coverage map")) + bcs::from_bytes(&bytes) + .with_context(|| format!("Deserializing coverage map from binary file {:?}", filename)) } // add entries in a cascading manner @@ -267,10 +270,13 @@ impl ExecCoverageMapWithModules { impl TraceMap { /// Takes in a file containing a raw VM trace, and returns an updated coverage map. - pub fn update_from_trace_file>(mut self, filename: P) -> Self { - let file = File::open(filename).unwrap(); + pub fn update_from_trace_file + std::fmt::Debug>( + mut self, + filename: &P, + ) -> Result { + let file = File::open(filename)?; for line in BufReader::new(file).lines() { - let line = line.unwrap(); + let line = line?; let mut splits = line.split(','); // Use a dummy key so that the data structure of the coverage map does not need to be changed let exec_id = "dummy_exec_id"; @@ -291,11 +297,11 @@ impl TraceMap { assert_eq!(context_segs.pop().unwrap(), "Script",); } } - self + Ok(self) } // Takes in a file containing a raw VM trace, and returns a parsed trace. - pub fn from_trace_file>(filename: P) -> Self { + pub fn from_trace_file + std::fmt::Debug>(filename: &P) -> Result { let trace_map = TraceMap { exec_maps: BTreeMap::new(), }; @@ -303,16 +309,14 @@ impl TraceMap { } // Takes in a file containing a serialized trace and deserialize it. - pub fn from_binary_file>(filename: P) -> Self { + pub fn from_binary_file + std::fmt::Debug>(filename: &P) -> Result { let mut bytes = Vec::new(); File::open(filename) .ok() .and_then(|mut file| file.read_to_end(&mut bytes).ok()) - .ok_or_else(|| format_err!("Error while reading in coverage map binary")) - .unwrap(); + .ok_or_else(|| format_err!("Error while reading in coverage map binary"))?; bcs::from_bytes(&bytes) - .map_err(|_| format_err!("Error deserializing into coverage map")) - .unwrap() + .with_context(|| format!("Deserializing {:?} into coverage map", filename)) } // add entries in a cascading manner @@ -334,7 +338,10 @@ impl TraceMap { } } -pub fn output_map_to_file>(file_name: P, data: &M) -> Result<()> { +pub fn output_map_to_file + std::fmt::Debug>( + file_name: &P, + data: &M, +) -> Result<()> { let bytes = bcs::to_bytes(data)?; let mut file = File::create(file_name)?; file.write_all(&bytes)?; diff --git a/third_party/move/tools/move-package/Cargo.toml b/third_party/move/tools/move-package/Cargo.toml index 7c57f7cffc06b..5f7fb7612b257 100644 --- a/third_party/move/tools/move-package/Cargo.toml +++ b/third_party/move/tools/move-package/Cargo.toml @@ -8,7 +8,7 @@ publish = false edition = "2021" [dependencies] -anyhow = { workspace = true } +anyhow = { workspace = true, features = ["backtrace"] } clap = { workspace = true, features = ["derive"] } colored = { workspace = true } itertools = { workspace = true } diff --git a/third_party/move/tools/move-package/src/lib.rs b/third_party/move/tools/move-package/src/lib.rs index 75f7d112f1eae..9caad33624885 100644 --- a/third_party/move/tools/move-package/src/lib.rs +++ b/third_party/move/tools/move-package/src/lib.rs @@ -211,8 +211,8 @@ impl BuildConfig { let config = self.compiler_config.clone(); // Need clone because of mut self let resolved_graph = self.resolution_graph_for_package(path, writer)?; let mutx = PackageLock::lock(); - let ret = - BuildPlan::create(resolved_graph)?.compile_no_exit(&config, external_checks, writer); + let plan = BuildPlan::create(resolved_graph)?; + let ret = plan.compile_no_exit(&config, external_checks, writer); mutx.unlock(); ret } From d077a0a8abf93181cf5c9703995115c1d39bb4da Mon Sep 17 00:00:00 2001 From: "Brian R. Murphy" <132495859+brmataptos@users.noreply.github.com> Date: Thu, 22 Aug 2024 17:10:13 -0700 Subject: [PATCH 2/6] original test case is working, with a lot of debugging still --- .../tools/move-coverage/src/source_coverage.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/third_party/move/tools/move-coverage/src/source_coverage.rs b/third_party/move/tools/move-coverage/src/source_coverage.rs index cba696461cda6..e65c43a051a21 100644 --- a/third_party/move/tools/move-coverage/src/source_coverage.rs +++ b/third_party/move/tools/move-coverage/src/source_coverage.rs @@ -183,6 +183,10 @@ impl<'a> SourceCoverageBuilder<'a> { coverage_map: &CoverageMap, source_map: &'a SourceMap, ) -> Self { + eprintln!("coverage_map is {:#?}", coverage_map); + eprintln!("source_map is {:#?}", source_map); + eprintln!("module is {:#?}", module); + let module_name = module.self_id(); let unified_exec_map = coverage_map.to_unified_exec_map(); let module_map = unified_exec_map @@ -247,6 +251,7 @@ impl<'a> SourceCoverageBuilder<'a> { coverage.map(|x| (fn_name, x)) }) .collect(); + eprintln!("uncovered_locations is {:#?}", uncovered_locations); Self { uncovered_locations, @@ -255,6 +260,7 @@ impl<'a> SourceCoverageBuilder<'a> { } pub fn compute_source_coverage(&self, file_path: &Path) -> SourceCoverage { + eprintln!("Reading file {}", file_path.display()); let file_contents = fs::read_to_string(file_path).unwrap(); assert!( self.source_map.check(&file_contents), @@ -273,6 +279,13 @@ impl<'a> SourceCoverageBuilder<'a> { let end_loc = files.location(file_id, span.end()).unwrap(); let start_line = start_loc.line.0; let end_line = end_loc.line.0; + eprintln!( + "Looking at span = ({}, {}), line = ({}, {})", + span.start(), + span.end(), + start_line, + end_line + ); let segments = uncovered_segments .entry(start_line) .or_insert_with(Vec::new); @@ -305,6 +318,7 @@ impl<'a> SourceCoverageBuilder<'a> { let mut annotated_lines = Vec::new(); for (line_number, mut line) in file_contents.lines().map(|x| x.to_owned()).enumerate() { + eprintln!("looking at {}, {}", line_number, line); match uncovered_segments.get(&(line_number as u32)) { None => annotated_lines.push(vec![StringSegment::Covered(line)]), Some(segments) => { @@ -315,6 +329,7 @@ impl<'a> SourceCoverageBuilder<'a> { for segment in segments { match segment { AbstractSegment::Bounded { start, end } => { + eprintln!("Bounded {}, {}, cursor = {}", start, end, cursor); let length = end - start; let (before, after) = line.split_at((start - cursor) as usize); let (uncovered, rest) = after.split_at(length as usize); @@ -324,12 +339,14 @@ impl<'a> SourceCoverageBuilder<'a> { cursor = *end; }, AbstractSegment::BoundedRight { end } => { + eprintln!("BoundedRight {}, cursor = {}", end, cursor); let (uncovered, rest) = line.split_at((end - cursor) as usize); line_acc.push(StringSegment::Uncovered(uncovered.to_string())); line = rest.to_string(); cursor = *end; }, AbstractSegment::BoundedLeft { start } => { + eprintln!("BoundedLeft {}, cursor = {}", start, cursor); let (before, after) = line.split_at((start - cursor) as usize); line_acc.push(StringSegment::Covered(before.to_string())); line_acc.push(StringSegment::Uncovered(after.to_string())); From 2cb0783b6fd96dd5ba6003b72c5d964462237a0e Mon Sep 17 00:00:00 2001 From: "Brian R. Murphy" <132495859+brmataptos@users.noreply.github.com> Date: Sun, 3 Nov 2024 12:33:18 -0800 Subject: [PATCH 3/6] add a bit more debugging output --- .../tools/move-coverage/src/coverage_map.rs | 13 +++++++- .../move-coverage/src/source_coverage.rs | 31 ++++++++++++++++++- 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/third_party/move/tools/move-coverage/src/coverage_map.rs b/third_party/move/tools/move-coverage/src/coverage_map.rs index 45bb7dca7062f..4e67c7aaf6a6f 100644 --- a/third_party/move/tools/move-coverage/src/coverage_map.rs +++ b/third_party/move/tools/move-coverage/src/coverage_map.rs @@ -130,10 +130,20 @@ impl CoverageMap { pub fn to_unified_exec_map(&self) -> ExecCoverageMap { let mut unified_map = ExecCoverageMap::new(String::new()); - for (_, exec_map) in self.exec_maps.iter() { + eprintln!("to_unified_exec_map"); + for (_key, exec_map) in self.exec_maps.iter() { + eprintln!("looking at key {}", _key); for ((module_addr, module_name), module_map) in exec_map.module_maps.iter() { for (func_name, func_map) in module_map.function_maps.iter() { for (pc, count) in func_map.iter() { + eprintln!( + "Adding item ({}, {}, {}, {}, {}) to unified_map", + module_addr.short_str_lossless(), + module_name.clone().into_string(), + func_name.clone().into_string(), + pc, + count, + ); unified_map.insert_multi( *module_addr, module_name.clone(), @@ -145,6 +155,7 @@ impl CoverageMap { } } } + eprintln!("finished to_unified_exec_map"); unified_map } } diff --git a/third_party/move/tools/move-coverage/src/source_coverage.rs b/third_party/move/tools/move-coverage/src/source_coverage.rs index e65c43a051a21..59aa38eb534de 100644 --- a/third_party/move/tools/move-coverage/src/source_coverage.rs +++ b/third_party/move/tools/move-coverage/src/source_coverage.rs @@ -280,7 +280,8 @@ impl<'a> SourceCoverageBuilder<'a> { let start_line = start_loc.line.0; let end_line = end_loc.line.0; eprintln!( - "Looking at span = ({}, {}), line = ({}, {})", + "Looking at key = {}, span = ({}, {}), line = ({}, {})", + _key.clone().into_string(), span.start(), span.end(), start_line, @@ -324,6 +325,15 @@ impl<'a> SourceCoverageBuilder<'a> { Some(segments) => { // Note: segments are already pre-sorted by construction so don't need to be // resorted. + eprintln!( + "Segments for line {} are: {}", + line_number, + segments + .iter() + .map(|seg| format!("{:?}", seg).to_string()) + .collect::>() + .join(", ") + ); let mut line_acc = Vec::new(); let mut cursor = 0; for segment in segments { @@ -420,6 +430,25 @@ fn merge_spans(file_hash: FileHash, cov: FunctionSourceCoverage) -> Vec { return vec![]; } + let mut last_loc: Option = None; + for loc in cov.uncovered_locations.iter() { + if loc.file_hash() != file_hash { + if let Some(last_loc) = last_loc { + eprintln!( + "dropping loc ({}, {}, {}) after ({}, {}, {})", + loc.file_hash(), + loc.start(), + loc.end(), + last_loc.file_hash(), + loc.start(), + loc.end(), + ); + } + last_loc = None; + } else { + last_loc = Some(*loc); + } + } let mut covs: Vec<_> = cov .uncovered_locations .iter() From 0cb33c0c14202ea582512f4004dc612565839c2b Mon Sep 17 00:00:00 2001 From: "Brian R. Murphy" <132495859+brmataptos@users.noreply.github.com> Date: Sat, 9 Nov 2024 19:41:03 -0800 Subject: [PATCH 4/6] Get coverage working better, add --show-locs flag to aptos move disassemble --- crates/aptos/src/move_tool/bytecode.rs | 51 +- crates/aptos/src/move_tool/coverage.rs | 12 +- third_party/move/move-compiler-v2/src/lib.rs | 9 +- .../move/move-ir/types/src/location.rs | 89 +++ third_party/move/move-model/src/model.rs | 1 + .../move/tools/move-cli/src/base/coverage.rs | 11 +- .../move-coverage/src/source_coverage.rs | 575 ++++++++++++++---- .../move-disassembler/src/disassembler.rs | 41 +- 8 files changed, 654 insertions(+), 135 deletions(-) diff --git a/crates/aptos/src/move_tool/bytecode.rs b/crates/aptos/src/move_tool/bytecode.rs index 6a662300b20db..6efe59a70fe7e 100644 --- a/crates/aptos/src/move_tool/bytecode.rs +++ b/crates/aptos/src/move_tool/bytecode.rs @@ -27,7 +27,7 @@ use move_disassembler::disassembler::{Disassembler, DisassemblerOptions}; use move_ir_types::location::Spanned; use std::{ fs, - path::{Path, PathBuf}, + path::{Component, Path, PathBuf}, process::Command, }; @@ -63,6 +63,9 @@ pub struct BytecodeCommand { #[clap(long)] pub is_script: bool, + #[clap(long)] + pub show_locs: bool, + #[clap(flatten)] input: BytecodeCommandInput, @@ -203,11 +206,43 @@ impl BytecodeCommand { fn disassemble(&self, bytecode_path: &Path) -> Result { let bytecode_bytes = read_from_file(bytecode_path)?; - let move_path = bytecode_path.with_extension(MOVE_EXTENSION); - let source_map_path = bytecode_path.with_extension(SOURCE_MAP_EXTENSION); - let source = fs::read_to_string(move_path).ok(); - let source_map = source_map_from_file(&source_map_path).ok(); + let source = { + let move_path = bytecode_path.with_extension(MOVE_EXTENSION); + if let Ok(source) = fs::read_to_string(move_path.clone()) { + Some(source) + } else { + let move_path = move_path + .components() + .map(|elt| { + if elt.as_os_str() == "bytecode_modules" { + Component::Normal("sources".as_ref()) + } else { + elt.clone() + } + }) + .collect::(); + fs::read_to_string(move_path).ok() + } + }; + let source_map = { + let source_map_path = bytecode_path.with_extension(SOURCE_MAP_EXTENSION); + if let Ok(source_map) = source_map_from_file(&source_map_path) { + Some(source_map) + } else { + let source_map_path = source_map_path + .components() + .map(|elt| { + if elt.as_os_str() == "bytecode_modules" { + Component::Normal("source_maps".as_ref()) + } else { + elt.clone() + } + }) + .collect::(); + source_map_from_file(&source_map_path).ok() + } + }; let disassembler_options = DisassemblerOptions { print_code: true, @@ -215,6 +250,7 @@ impl BytecodeCommand { print_basic_blocks: true, print_locals: true, print_bytecode_stats: false, + show_locs: self.show_locs, }; let no_loc = Spanned::unsafe_no_loc(()).loc; let module: CompiledModule; @@ -232,6 +268,11 @@ impl BytecodeCommand { let mut source_mapping = if let Some(s) = source_map { SourceMapping::new(s, bytecode) } else { + if self.show_locs { + return Err(CliError::UnexpectedError( + "Missing source_map with option show-locs".to_string(), + )); + } SourceMapping::new_from_view(bytecode, no_loc) .context("Unable to build dummy source mapping")? }; diff --git a/crates/aptos/src/move_tool/coverage.rs b/crates/aptos/src/move_tool/coverage.rs index 53add94da75b6..b85aad965a277 100644 --- a/crates/aptos/src/move_tool/coverage.rs +++ b/crates/aptos/src/move_tool/coverage.rs @@ -124,7 +124,17 @@ impl CliCommand<()> for SourceCoverage { }) => (module, source_map), _ => panic!("Should all be modules"), }; - let source_coverage = SourceCoverageBuilder::new(module, &coverage_map, source_map); + let packages: Vec<_> = package + .root_modules() + .map(|unit| match &unit.unit { + CompiledUnit::Module(NamedCompiledModule { + module, source_map, .. + }) => (module, source_map), + _ => panic!("Should all be modules"), + }) + .collect(); + let source_coverage = + SourceCoverageBuilder::new(module, &coverage_map, source_map, packages); let source_coverage = source_coverage.compute_source_coverage(source_path); let output_result = source_coverage.output_source_coverage(&mut std::io::stdout(), self.color, self.tag); diff --git a/third_party/move/move-compiler-v2/src/lib.rs b/third_party/move/move-compiler-v2/src/lib.rs index 84eda127b9f03..81a63ad02e406 100644 --- a/third_party/move/move-compiler-v2/src/lib.rs +++ b/third_party/move/move-compiler-v2/src/lib.rs @@ -504,14 +504,7 @@ pub fn bytecode_pipeline(env: &GlobalEnv) -> FunctionTargetPipeline { pub fn disassemble_compiled_units(units: &[CompiledUnit]) -> anyhow::Result { let disassembled_units: anyhow::Result> = units .iter() - .map(|unit| { - let view = match unit { - CompiledUnit::Module(module) => BinaryIndexedView::Module(&module.module), - CompiledUnit::Script(script) => BinaryIndexedView::Script(&script.script), - }; - Disassembler::from_view(view, location::Loc::new(FileHash::empty(), 0, 0)) - .and_then(|d| d.disassemble()) - }) + .map(|unit| Disassembler::from_unit(unit).disassemble()) .collect(); Ok(disassembled_units?.concat()) } diff --git a/third_party/move/move-ir/types/src/location.rs b/third_party/move/move-ir/types/src/location.rs index f97cd2e752606..cc47f30e82093 100644 --- a/third_party/move/move-ir/types/src/location.rs +++ b/third_party/move/move-ir/types/src/location.rs @@ -36,6 +36,7 @@ pub struct Loc { impl Loc { pub fn new(file_hash: FileHash, start: ByteIndex, end: ByteIndex) -> Loc { + assert!(start <= end); Loc { file_hash, start, @@ -61,6 +62,94 @@ impl Loc { end: self.end as usize, } } + + pub fn contains(&self, other: &Self) -> bool { + if self.file_hash != other.file_hash { + false + } else { + self.start <= other.start && other.end <= self.end + } + } + + pub fn overlaps(&self, other: &Self) -> bool { + if self.file_hash != other.file_hash { + false + } else { + // [a, b] overlaps? [c, d] + // a <= b + // c <= d + other.start <= self.end && self.start <= other.end + // c <= b + // a <= d + // One of these: + // [a <= (c <= b] <= d) + // (c <= [a <= b] <= d) + // (c <= [a <= d) <= b] + } + } + + pub fn overlaps_or_abuts(&self, other: &Self) -> bool { + if self.file_hash != other.file_hash { + false + } else { + // [a, b] overlaps? [c, d] + // a <= b + // c <= d + other.start <= self.end + 1 && self.start <= other.end + 1 + // c <= b + 1 + // a <= d + 1 + // One of these: + // a <= c <= b+1 <= d+1 + // c <= a <= b+1 <= d+1 + // c <= a <= d+1 <= b+1 + } + } + + pub fn try_merge(&mut self, other: &Self) -> bool { + if self.overlaps_or_abuts(other) { + self.start = std::cmp::min(self.start, other.start); + self.end = std::cmp::max(self.end, other.end); + true + } else { + false + } + } + + // if other overlaps with this, then subtract it out. + pub fn subtract(self, other: &Self) -> Vec { + if !self.overlaps(other) { + vec![self] + } else { + if other.start <= self.start { + if self.end <= other.end { + vec![] + } else { + vec![Loc { + start: other.end + 1, + ..self + }] + } + } else { + if self.end <= other.end { + vec![Loc { + end: other.start - 1, + ..self + }] + } else { + vec![ + Loc { + end: other.start - 1, + ..self + }, + Loc { + start: other.end + 1, + ..self + }, + ] + } + } + } + } } impl PartialOrd for Loc { diff --git a/third_party/move/move-model/src/model.rs b/third_party/move/move-model/src/model.rs index 9428d08ec9617..e63c4a9a98963 100644 --- a/third_party/move/move-model/src/model.rs +++ b/third_party/move/move-model/src/model.rs @@ -3337,6 +3337,7 @@ impl<'env> ModuleEnv<'env> { print_basic_blocks: true, print_locals: true, print_bytecode_stats: false, + show_locs: false, }); Some( disas diff --git a/third_party/move/tools/move-cli/src/base/coverage.rs b/third_party/move/tools/move-cli/src/base/coverage.rs index 51a514f31cba1..f5049555be014 100644 --- a/third_party/move/tools/move-cli/src/base/coverage.rs +++ b/third_party/move/tools/move-cli/src/base/coverage.rs @@ -79,8 +79,17 @@ impl Coverage { }) => (module, source_map), _ => panic!("Should all be modules"), }; + let packages: Vec<_> = package + .root_modules() + .map(|unit| match &unit.unit { + CompiledUnit::Module(NamedCompiledModule { + module, source_map, .. + }) => (module, source_map), + _ => panic!("Should all be modules"), + }) + .collect(); let source_coverage_builder = - SourceCoverageBuilder::new(module, &coverage_map, source_map); + SourceCoverageBuilder::new(module, &coverage_map, source_map, packages); let source_coverage = source_coverage_builder.compute_source_coverage(source_path); source_coverage .output_source_coverage(&mut std::io::stdout(), self.color, self.tag) diff --git a/third_party/move/tools/move-coverage/src/source_coverage.rs b/third_party/move/tools/move-coverage/src/source_coverage.rs index 59aa38eb534de..b2ab82212e1a6 100644 --- a/third_party/move/tools/move-coverage/src/source_coverage.rs +++ b/third_party/move/tools/move-coverage/src/source_coverage.rs @@ -6,7 +6,7 @@ use crate::coverage_map::CoverageMap; use clap::ValueEnum; -use codespan::{Files, Span}; +use codespan::{FileId, Files, Span}; use colored::{self, Colorize}; use move_binary_format::{ access::ModuleAccess, @@ -15,11 +15,11 @@ use move_binary_format::{ }; use move_bytecode_source_map::source_map::SourceMap; use move_command_line_common::files::FileHash; -use move_core_types::identifier::Identifier; use move_ir_types::location::Loc; use serde::Serialize; use std::{ - collections::BTreeMap, + cmp::{Ordering, PartialOrd}, + collections::{BTreeMap, BTreeSet}, fmt::{Display, Formatter}, fs, io::{self, Write}, @@ -38,20 +38,172 @@ pub struct FunctionSourceCoverage { pub uncovered_locations: Vec, } +/// Source-level positive coverage information for a function. +#[derive(Clone, Debug, Serialize)] +pub struct FunctionSourceCoverage2 { + /// Is this a native function? + /// If so, then `uncovered_locations` is empty. + pub fn_is_native: bool, + + /// List of source locations in the function that were covered. + pub covered_locations: Vec, + + /// List of all source locations in the function. + pub uncovered_locations: Vec, +} + +fn minimize_locations(mut locs: Vec) -> Vec { + locs.sort(); + let mut result = vec![]; + let mut locs_iter = locs.into_iter(); + if let Some(mut current_loc) = locs_iter.next() { + for next_loc in locs_iter { + let loc_tmp = current_loc; + if !current_loc.try_merge(&next_loc) { + eprintln!("Not merging {:?} with {:?}", loc_tmp, next_loc); + result.push(current_loc); + current_loc = next_loc; + } + } + result.push(current_loc); + } + result +} + +// locs1 and locs2 should each be sorted or this won't work. +fn subtract_locations(locs1: BTreeSet, locs2: &BTreeSet) -> Vec { + let mut result = vec![]; + let mut locs1_iter = locs1.into_iter(); + let mut locs2_iter = locs2.iter(); + if let Some(mut current_loc1) = locs1_iter.next() { + if let Some(mut current_loc2) = locs2_iter.next() { + loop { + if current_loc1.overlaps(current_loc2) { + let mut diff = current_loc1.subtract(current_loc2); + if let Some(new_loc1) = diff.pop() { + result.append(&mut diff); + current_loc1 = new_loc1; + // continue + } else { + // diff was empty, get a new loc1 + if let Some(new_loc1) = locs1_iter.next() { + current_loc1 = new_loc1; + // retry loc2 + // continue + } else { + // no more loc1, return + return result; + } + } + } else { + // no overlap + if current_loc1 <= *current_loc2 { + // loc1 is done. save it and get a new one. + result.push(current_loc1); + if let Some(new_loc1) = locs1_iter.next() { + current_loc1 = new_loc1; + // continue + } else { + // No more loc1, return + return result; + } + } else { + // loc1 might have more overlaps, try another loc2 + if let Some(new_loc2) = locs2_iter.next() { + current_loc2 = new_loc2; + // continue + } else { + // loc2 is finished but loc1 is not, + // finish adding all loc1 + result.push(current_loc1); + for loc1 in locs1_iter { + result.push(loc1); + } + return result; + } + } + } + } + } + result.push(current_loc1); + } + result +} + /// Builder for the source code coverage. #[derive(Debug, Serialize)] pub struct SourceCoverageBuilder<'a> { /// Mapping from function name to the source-level uncovered locations for that function. - pub uncovered_locations: BTreeMap, + pub uncovered_locations: Vec, + pub covered_locations: Vec, source_map: &'a SourceMap, } -#[derive(Debug, Serialize, Eq, PartialEq, Ord, PartialOrd)] +#[derive(Debug, Serialize, Eq, PartialEq)] pub enum AbstractSegment { - Bounded { start: u32, end: u32 }, - BoundedRight { end: u32 }, - BoundedLeft { start: u32 }, + Bounded { + is_covered: bool, + start: u32, + end: u32, + }, + BoundedRight { + is_covered: bool, + end: u32, + }, + BoundedLeft { + is_covered: bool, + start: u32, + }, + // Unbounded {}, +} + +impl AbstractSegment { + fn is_covered(&self) -> bool { + use AbstractSegment::*; + match self { + Bounded { is_covered, .. } + | BoundedRight { is_covered, .. } + | BoundedLeft { is_covered, .. } => *is_covered, + } + } + + fn get_start(&self) -> u32 { + use AbstractSegment::*; + match self { + Bounded { start, .. } => *start, + BoundedRight { .. } => 0u32, + BoundedLeft { start, .. } => *start, + // Unbounded {} => 0u32, + } + } + + fn get_number(&self) -> u8 { + use AbstractSegment::*; + match self { + Bounded { .. } => 0, + BoundedRight { .. } => 1, + BoundedLeft { .. } => 2, + // Unbounded {} => 3, + } + } +} + +impl PartialOrd for AbstractSegment { + fn partial_cmp(&self, other: &AbstractSegment) -> Option { + Some(self.cmp(other)) + } +} + +impl Ord for AbstractSegment { + fn cmp(&self, other: &AbstractSegment) -> Ordering { + use Ordering::*; + match self.get_start().cmp(&other.get_start()) { + Less => Less, + Greater => Greater, + Equal => self.get_number().cmp(&other.get_number()), + } + } } /// Option to control use of color escape codes in coverage output @@ -168,6 +320,7 @@ impl ColorChoice { pub enum StringSegment { Covered(String), Uncovered(String), + NotCounted(String), } pub type AnnotatedLine = Vec; @@ -182,83 +335,270 @@ impl<'a> SourceCoverageBuilder<'a> { module: &CompiledModule, coverage_map: &CoverageMap, source_map: &'a SourceMap, + packages: Vec<(&CompiledModule, &'a SourceMap)>, ) -> Self { eprintln!("coverage_map is {:#?}", coverage_map); eprintln!("source_map is {:#?}", source_map); eprintln!("module is {:#?}", module); + let module_loc = source_map.definition_location; + let module_name = module.self_id(); let unified_exec_map = coverage_map.to_unified_exec_map(); let module_map = unified_exec_map .module_maps .get(&(*module_name.address(), module_name.name().to_owned())); - let uncovered_locations: BTreeMap = module - .function_defs() - .iter() - .enumerate() - .flat_map(|(function_def_idx, function_def)| { - let fn_handle = module.function_handle_at(function_def.function); - let fn_name = module.identifier_at(fn_handle.name).to_owned(); - let function_def_idx = FunctionDefinitionIndex(function_def_idx as u16); - - // If the function summary doesn't exist then that function hasn't been called yet. - let coverage = match &function_def.code { - None => Some(FunctionSourceCoverage { - fn_is_native: true, - uncovered_locations: Vec::new(), - }), - Some(code_unit) => { - module_map.map(|fn_map| match fn_map.function_maps.get(&fn_name) { + eprintln!("unified_exec_map is {:#?}", &unified_exec_map); + eprintln!("module_map is {:#?}", &module_map); + + eprintln!("Computing covered_locations"); + + let mut fun_coverage: Vec = Vec::new(); + for (module, source_map) in packages.iter() { + let module_name = module.self_id(); + let module_map = unified_exec_map + .module_maps + .get(&(*module_name.address(), module_name.name().to_owned())); + if let Some(module_map) = module_map { + for (function_def_idx, function_def) in module.function_defs().iter().enumerate() { + let fn_handle = module.function_handle_at(function_def.function); + let fn_name = module.identifier_at(fn_handle.name).to_owned(); + let function_def_idx = FunctionDefinitionIndex(function_def_idx as u16); + let function_map = source_map + .get_function_source_map(function_def_idx) + .unwrap(); + let _function_loc = function_map.definition_location; + let function_covered_locations: FunctionSourceCoverage2 = match &function_def + .code + { + None => FunctionSourceCoverage2 { + fn_is_native: true, + covered_locations: vec![], + uncovered_locations: vec![], + }, + Some(code_unit) => match module_map.function_maps.get(&fn_name) { None => { - let function_map = source_map - .get_function_source_map(function_def_idx) - .unwrap(); - let mut uncovered_locations = - vec![function_map.definition_location]; - uncovered_locations.extend(function_map.code_map.values()); - - FunctionSourceCoverage { + let locations: Vec<_> = (0..code_unit.code.len()) + .filter_map(|code_offset| { + if let Ok(loc) = source_map.get_code_location( + function_def_idx, + code_offset as CodeOffset, + ) { + Some(loc) + } else { + None + } + }) + .collect(); + FunctionSourceCoverage2 { fn_is_native: false, - uncovered_locations, + covered_locations: vec![], + uncovered_locations: locations, } }, Some(function_coverage) => { - let uncovered_locations: Vec<_> = (0..code_unit.code.len()) - .flat_map(|code_offset| { - if !function_coverage.contains_key(&(code_offset as u64)) { - Some( - source_map - .get_code_location( - function_def_idx, - code_offset as CodeOffset, - ) - .unwrap(), - ) + let (fun_cov, fun_uncov): (Vec<_>, Vec<_>) = (0..code_unit + .code + .len()) + .filter_map(|code_offset| { + if let Ok(loc) = source_map.get_code_location( + function_def_idx, + code_offset as CodeOffset, + ) { + if function_coverage.contains_key(&(code_offset as u64)) + { + Some((loc, true)) + } else { + Some((loc, false)) + } } else { None } }) + .partition(|(_loc, test)| *test); + + let covered_locations: Vec<_> = fun_cov + .iter() + .filter_map( + |(loc, covered)| if *covered { Some(*loc) } else { None }, + ) + .collect(); + let uncovered_locations: Vec<_> = fun_uncov + .iter() + .filter_map( + |(loc, covered)| if !*covered { Some(*loc) } else { None }, + ) .collect(); - FunctionSourceCoverage { + + FunctionSourceCoverage2 { fn_is_native: false, + covered_locations, uncovered_locations, } }, - }) - }, - }; - coverage.map(|x| (fn_name, x)) + }, + }; + fun_coverage.push(function_covered_locations); + } + } + } + eprintln!("fun_coverage is {:#?}", fun_coverage); + + // Filter locations for this module and build 2 sets: covered and uncovered locations + // Note that Move 1 compiler sets module_loc = location of symbol in definition, so if + // there are multiple modules in one file we may leave others in the picture. + eprintln!("module_loc is {:?}", module_loc); + let module_file_hash = module_loc.file_hash(); + eprintln!("module_file_hash is {:?}", module_file_hash); + + let (covered, uncovered): (BTreeSet, BTreeSet) = fun_coverage + .iter() + .map( + |FunctionSourceCoverage2 { + covered_locations, + uncovered_locations, + .. + }| { + let cov: BTreeSet<_> = covered_locations + .iter() + .filter(|loc| loc.file_hash() == module_file_hash) + .cloned() + .collect(); + let uncov: BTreeSet<_> = uncovered_locations + .iter() + .filter(|loc| loc.file_hash() == module_file_hash) + .cloned() + .collect(); + (cov, uncov) + }, + ) + .reduce(|(c1, u1), (c2, u2)| { + ( + c1.union(&c2).cloned().collect(), + u1.union(&u2).cloned().collect(), + ) }) - .collect(); - eprintln!("uncovered_locations is {:#?}", uncovered_locations); + .unwrap_or_else(|| (BTreeSet::new(), BTreeSet::new())); + + eprintln!("covered 0 is {:#?}", covered); + eprintln!("uncovered 0 is {:#?}", uncovered); + + // Some locations contain others, but should not subsume them, e.g.: + // 31: if (p) { + // 32: ... + // 33: } else { + // 34: ... + // 35 } + // May have 3 corresponding bytecodes, with locations: + // L1: 31-35 + // L2: 32 + // L3: 34 + // If L1 is covered but L3 is not, then we want to subtract + // L3 from the covered region. + // + // OTOH, we may see L3 in multiple runs, once covered and once + // not. So we need to + // (1) subtract identical covered locations from uncovered locations + // (2) subtract uncovered locations from any covered locations they overlap + // (3) subtract covered locations from any uncovered lcoations they overlap + + // (1) + let uncovered: BTreeSet<_> = uncovered.difference(&covered).cloned().collect(); + + eprintln!("uncovered 1 is {:#?}", uncovered); + + // (2) + let covered: Vec<_> = subtract_locations(covered, &uncovered); + + eprintln!("covered 2a is {:#?}", covered); + + let covered: Vec<_> = minimize_locations(covered); + + eprintln!("covered 2b is {:#?}", covered); + + let uncovered: Vec<_> = uncovered.into_iter().collect(); + eprintln!("uncovered 2a is {:#?}", uncovered); + + let uncovered: Vec<_> = minimize_locations(uncovered); + + eprintln!("uncovered 2b is {:#?}", uncovered); + + let uncovered: BTreeSet<_> = uncovered.into_iter().collect(); + let covered: BTreeSet<_> = covered.into_iter().collect(); + + // (3) + let uncovered: Vec<_> = subtract_locations(uncovered, &covered); + let covered: Vec<_> = covered.into_iter().collect(); + + eprintln!("uncovered 3 is {:#?}", uncovered); + eprintln!("uncovered is {:#?}", uncovered); Self { - uncovered_locations, + uncovered_locations: uncovered, + covered_locations: covered, source_map, } } + // Converts a (sorted) list of non-overlapping `Loc` into a + // map from line number to set of `AsbtractSegment` for each line + // of the file `file_id` in fileset `files`. + fn locs_to_segments( + &self, + files: &mut Files, + file_id: FileId, + spans: Vec, + is_covered: bool, + ) -> BTreeMap> { + let mut segments = BTreeMap::new(); + + for span in spans.into_iter() { + let start_loc = files.location(file_id, span.start()).unwrap(); + let end_loc = files.location(file_id, span.end()).unwrap(); + let start_line = start_loc.line.0; + let end_line = end_loc.line.0; + eprintln!( + "Looking at span = ({}, {}), line = ({}, {})", + span.start(), + span.end(), + start_line, + end_line + ); + + let line_segments = segments.entry(start_line).or_insert_with(Vec::new); + if start_line == end_line { + let segment = AbstractSegment::Bounded { + start: start_loc.column.0, + end: end_loc.column.0, + is_covered, + }; + if !line_segments.contains(&segment) { + line_segments.push(segment); + } + } else { + line_segments.push(AbstractSegment::BoundedLeft { + start: start_loc.column.0, + is_covered, + }); + for i in start_line + 1..end_line { + let line_segments = segments.entry(i).or_insert_with(Vec::new); + line_segments.push(AbstractSegment::BoundedLeft { + start: 0, + is_covered, + }); + } + let last_line_segments = segments.entry(end_line).or_insert_with(Vec::new); + last_line_segments.push(AbstractSegment::BoundedRight { + end: end_loc.column.0, + is_covered, + }); + } + } + segments.values_mut().for_each(|v| v.sort()); + segments + } + pub fn compute_source_coverage(&self, file_path: &Path) -> SourceCoverage { eprintln!("Reading file {}", file_path.display()); let file_contents = fs::read_to_string(file_path).unwrap(); @@ -267,64 +607,41 @@ impl<'a> SourceCoverageBuilder<'a> { "File contents {} out of sync with source map", file_path.display() ); - let file_hash = self.source_map.definition_location.file_hash(); let mut files = Files::new(); let file_id = files.add(file_path.as_os_str().to_os_string(), file_contents.clone()); - let mut uncovered_segments = BTreeMap::new(); + let covs: Vec<_> = self + .covered_locations + .iter() + .map(|loc| Span::new(loc.start(), loc.end())) + .collect(); + let uncovs: Vec<_> = self + .uncovered_locations + .iter() + .map(|loc| Span::new(loc.start(), loc.end())) + .collect(); - for (_key, fn_cov) in self.uncovered_locations.iter() { - for span in merge_spans(file_hash, fn_cov.clone()).into_iter() { - let start_loc = files.location(file_id, span.start()).unwrap(); - let end_loc = files.location(file_id, span.end()).unwrap(); - let start_line = start_loc.line.0; - let end_line = end_loc.line.0; - eprintln!( - "Looking at key = {}, span = ({}, {}), line = ({}, {})", - _key.clone().into_string(), - span.start(), - span.end(), - start_line, - end_line - ); - let segments = uncovered_segments - .entry(start_line) - .or_insert_with(Vec::new); - if start_line == end_line { - let segment = AbstractSegment::Bounded { - start: start_loc.column.0, - end: end_loc.column.0, - }; - // TODO: There is some issue with the source map where we have multiple spans - // from different functions. This can be seen in the source map for `Roles.move` - if !segments.contains(&segment) { - segments.push(segment); - } - } else { - segments.push(AbstractSegment::BoundedLeft { - start: start_loc.column.0, - }); - for i in start_line + 1..end_line { - let segment = uncovered_segments.entry(i).or_insert_with(Vec::new); - segment.push(AbstractSegment::BoundedLeft { start: 0 }); - } - let last_segment = uncovered_segments.entry(end_line).or_insert_with(Vec::new); - last_segment.push(AbstractSegment::BoundedRight { - end: end_loc.column.0, - }); - } + let mut uncovered_segments = self.locs_to_segments(&mut files, file_id, uncovs, false); + let mut covered_segments = self.locs_to_segments(&mut files, file_id, covs, true); + + covered_segments.values_mut().for_each(|v| v.sort()); + for (key, value) in covered_segments.iter_mut() { + match uncovered_segments.get_mut(key) { + None => {}, + Some(value2) => { + value.append(value2); + value.sort(); + }, } } - uncovered_segments.values_mut().for_each(|v| v.sort()); let mut annotated_lines = Vec::new(); for (line_number, mut line) in file_contents.lines().map(|x| x.to_owned()).enumerate() { eprintln!("looking at {}, {}", line_number, line); - match uncovered_segments.get(&(line_number as u32)) { - None => annotated_lines.push(vec![StringSegment::Covered(line)]), + let line_no_u32 = line_number as u32; + match covered_segments.get(&line_no_u32) { + None => annotated_lines.push(vec![StringSegment::NotCounted(line)]), Some(segments) => { - // Note: segments are already pre-sorted by construction so don't need to be - // resorted. eprintln!( "Segments for line {} are: {}", line_number, @@ -336,43 +653,69 @@ impl<'a> SourceCoverageBuilder<'a> { ); let mut line_acc = Vec::new(); let mut cursor = 0; + // Note: segments are already pre-sorted by construction so don't need to be + // resorted. + // let mut segment_start: Option = None; + // let mut segment_end: Option = None; for segment in segments { match segment { - AbstractSegment::Bounded { start, end } => { + AbstractSegment::Bounded { + start, + end, + is_covered, + } => { eprintln!("Bounded {}, {}, cursor = {}", start, end, cursor); + let length = end - start; let (before, after) = line.split_at((start - cursor) as usize); - let (uncovered, rest) = after.split_at(length as usize); - line_acc.push(StringSegment::Covered(before.to_string())); - line_acc.push(StringSegment::Uncovered(uncovered.to_string())); + let (covered, rest) = after.split_at(length as usize); + line_acc.push(StringSegment::NotCounted(before.to_string())); + line_acc.push( + if *is_covered { + StringSegment::Covered(covered.to_string()) + } else { + StringSegment::Uncovered(covered.to_string()) + }, + ); line = rest.to_string(); cursor = *end; }, - AbstractSegment::BoundedRight { end } => { + AbstractSegment::BoundedRight { end, is_covered } => { eprintln!("BoundedRight {}, cursor = {}", end, cursor); let (uncovered, rest) = line.split_at((end - cursor) as usize); - line_acc.push(StringSegment::Uncovered(uncovered.to_string())); + line_acc.push( + if *is_covered { + StringSegment::Covered(uncovered.to_string()) + } else { + StringSegment::Uncovered(uncovered.to_string()) + }, + ); line = rest.to_string(); cursor = *end; }, - AbstractSegment::BoundedLeft { start } => { + AbstractSegment::BoundedLeft { start, is_covered } => { eprintln!("BoundedLeft {}, cursor = {}", start, cursor); let (before, after) = line.split_at((start - cursor) as usize); - line_acc.push(StringSegment::Covered(before.to_string())); - line_acc.push(StringSegment::Uncovered(after.to_string())); + line_acc.push(StringSegment::NotCounted(before.to_string())); + line_acc.push( + if *is_covered { + StringSegment::Covered(after.to_string()) + } else { + StringSegment::Uncovered(after.to_string()) + }, + ); line = "".to_string(); cursor = 0; }, } } if !line.is_empty() { - line_acc.push(StringSegment::Covered(line)) + line_acc.push(StringSegment::NotCounted(line)) } annotated_lines.push(line_acc) }, - } + }; } - SourceCoverage { annotated_lines } } } @@ -402,13 +745,18 @@ impl SourceCoverage { let has_uncovered = line .iter() .any(|string_segment| matches!(string_segment, StringSegment::Uncovered(_))); + let has_covered = line + .iter() + .any(|string_segment| matches!(string_segment, StringSegment::Covered(_))); write!( output_writer, "{} ", if has_uncovered { "-".to_string().red() - } else { + } else if has_covered { "+".to_string().green() + } else { + " ".to_string().normal() } )?; } @@ -416,6 +764,7 @@ impl SourceCoverage { match string_segment { StringSegment::Covered(s) => write!(output_writer, "{}", s.green())?, StringSegment::Uncovered(s) => write!(output_writer, "{}", s.bold().red())?, + StringSegment::NotCounted(s) => write!(output_writer, "{}", s.normal())?, } } writeln!(output_writer)?; diff --git a/third_party/move/tools/move-disassembler/src/disassembler.rs b/third_party/move/tools/move-disassembler/src/disassembler.rs index 3dedaa3f942b1..3348852ba2023 100644 --- a/third_party/move/tools/move-disassembler/src/disassembler.rs +++ b/third_party/move/tools/move-disassembler/src/disassembler.rs @@ -48,6 +48,10 @@ pub struct DisassemblerOptions { /// Print bytecode statistics for the module. #[clap(long = "print-bytecode-stats")] pub print_bytecode_stats: bool, + + /// Show source locations for bytecodes. + #[clap(long = "show-locs")] + pub show_locs: bool, } impl DisassemblerOptions { @@ -58,6 +62,7 @@ impl DisassemblerOptions { print_basic_blocks: true, print_locals: true, print_bytecode_stats: false, + show_locs: false, } } } @@ -297,12 +302,12 @@ impl<'a> Disassembler<'a> { instruction: String, ) -> String { if self.coverage_map.is_none() { - return format!("\t{}: {}", pc, instruction); + return format!("\t{:5}: {}", pc, instruction); } let coverage = function_coverage_map.and_then(|map| map.get(&(pc as u64))); match coverage { - Some(coverage) => format!("[{}]\t{}: {}", coverage, pc, instruction).green(), - None => format!("\t{}: {}", pc, instruction).red(), + Some(coverage) => format!("[{}]\t{:5}: {}", coverage, pc, instruction).green(), + None => format!("\t{}: {:5}", pc, instruction).red(), } .to_string() } @@ -1158,17 +1163,39 @@ impl<'a> Disassembler<'a> { let instrs: Vec = code .code .iter() - .map(|instruction| { - self.disassemble_instruction( + .enumerate() + .map(|(idx, instruction)| { + match self.disassemble_instruction( parameters, instruction, locals_sigs, function_source_map, decl_location, - ) + ) { + Ok(instr_str) => { + if self.options.show_locs { + if let Some(loc) = function_source_map.get_code_location(idx as u16) { + let hash = + (format!("{}", loc.file_hash()).to_string())[..8].to_string(); + Ok(format!( + "{:60} {}:{}-{}", + instr_str, + hash, + loc.start(), + loc.end() + ) + .to_string()) + } else { + Ok(instr_str) + } + } else { + Ok(instr_str) + } + }, + Err(err) => Err(err), + } }) .collect::>>()?; - let mut instrs: Vec = instrs .into_iter() .enumerate() From 32cbef476d278ccd7fc802bc99760459ff076670 Mon Sep 17 00:00:00 2001 From: "Brian R. Murphy" <132495859+brmataptos@users.noreply.github.com> Date: Mon, 11 Nov 2024 22:02:09 -0800 Subject: [PATCH 5/6] show env_pipeline pass outputs if MVC_LOG=debug, show dot files if MVC_DUMP=true also add more debug logging of Loc information --- .../move-compiler-v2/src/env_pipeline/mod.rs | 10 ++++---- .../function_generator.rs | 24 ++++++++++++------- third_party/move/move-compiler-v2/src/lib.rs | 2 +- .../move-compiler/src/command_line/mod.rs | 5 ++++ .../bytecode/src/function_target.rs | 1 + .../move-coverage/src/source_coverage.rs | 12 +++++----- 6 files changed, 34 insertions(+), 20 deletions(-) diff --git a/third_party/move/move-compiler-v2/src/env_pipeline/mod.rs b/third_party/move/move-compiler-v2/src/env_pipeline/mod.rs index d589973479f13..5d52ecdb87048 100644 --- a/third_party/move/move-compiler-v2/src/env_pipeline/mod.rs +++ b/third_party/move/move-compiler-v2/src/env_pipeline/mod.rs @@ -4,7 +4,7 @@ //! This module contains a set of transformers and analyzers of global environment. //! Those can be arranged in a pipeline and executed in sequence. -use log::trace; +use log::debug; use move_model::model::GlobalEnv; use std::io::Write; @@ -43,10 +43,10 @@ impl<'a> EnvProcessorPipeline<'a> { /// Runs the pipeline. Running will be ended if any of the steps produces an error. /// The function returns true if all steps succeeded without errors. pub fn run(&self, env: &mut GlobalEnv) -> bool { - trace!("before env processor pipeline: {}", env.dump_env()); + debug!("before env processor pipeline: {}", env.dump_env()); for (name, proc) in &self.processors { proc(env); - trace!("after env processor {}", name); + debug!("after env processor {}: {}", name, env.dump_env()); if env.has_errors() { return false; } @@ -58,13 +58,13 @@ impl<'a> EnvProcessorPipeline<'a> { /// only. pub fn run_and_record(&self, env: &mut GlobalEnv, w: &mut impl Write) -> anyhow::Result { let msg = format!("before env processor pipeline:\n{}\n", env.dump_env()); - trace!("{}", msg); + debug!("{}", msg); writeln!(w, "// -- Model dump {}", msg)?; for (name, proc) in &self.processors { proc(env); if !env.has_errors() { let msg = format!("after env processor {}:\n{}\n", name, env.dump_env()); - trace!("{}", msg); + debug!("{}", msg); writeln!(w, "// -- Model dump {}", msg)?; } else { return Ok(false); diff --git a/third_party/move/move-compiler-v2/src/file_format_generator/function_generator.rs b/third_party/move/move-compiler-v2/src/file_format_generator/function_generator.rs index 9f2457f25cf3a..c2edfe4faef52 100644 --- a/third_party/move/move-compiler-v2/src/file_format_generator/function_generator.rs +++ b/third_party/move/move-compiler-v2/src/file_format_generator/function_generator.rs @@ -12,6 +12,7 @@ use crate::{ livevar_analysis_processor::LiveVarAnnotation, }, }; +use log::debug; use move_binary_format::{ file_format as FF, file_format::{CodeOffset, FunctionDefinitionIndex}, @@ -209,6 +210,12 @@ impl<'a> FunctionGenerator<'a> { for i in 0..bytecode.len() { let code_offset = i as FF::CodeOffset; let bc = &bytecode[i]; + debug!( + "Generating code for bytecode {:?} ({}) at offset {}", + bc, + bc.display(&ctx.fun, &BTreeMap::new()), + i + ); let bytecode_ctx = BytecodeContext { fun_ctx: ctx, code_offset, @@ -260,16 +267,17 @@ impl<'a> FunctionGenerator<'a> { /// Generate file-format bytecode from a stackless bytecode and an optional next bytecode /// for peephole optimizations. fn gen_bytecode(&mut self, ctx: &BytecodeContext, bc: &Bytecode, next_bc: Option<&Bytecode>) { + let loc = ctx.fun_ctx.fun.get_bytecode_loc(ctx.attr_id); + let ir_loc = ctx.fun_ctx.module.env.to_ir_loc(&loc); + let offset = self.code.len(); + let func_name = ctx.fun_ctx.fun.func_env.get_full_name_str(); + debug!( + "Setting location for {}:{} with attr_id {:?} to {:?} (from {:?})", + func_name, offset, ctx.attr_id, ir_loc, loc + ); self.gen .source_map - .add_code_mapping( - ctx.fun_ctx.def_idx, - self.code.len() as FF::CodeOffset, - ctx.fun_ctx - .module - .env - .to_ir_loc(&ctx.fun_ctx.fun.get_bytecode_loc(ctx.attr_id)), - ) + .add_code_mapping(ctx.fun_ctx.def_idx, offset as FF::CodeOffset, ir_loc) .expect(SOURCE_MAP_OK); match bc { Bytecode::Assign(_, dest, source, mode) => { diff --git a/third_party/move/move-compiler-v2/src/lib.rs b/third_party/move/move-compiler-v2/src/lib.rs index 81a63ad02e406..4e1d9a2fdbef7 100644 --- a/third_party/move/move-compiler-v2/src/lib.rs +++ b/third_party/move/move-compiler-v2/src/lib.rs @@ -122,7 +122,7 @@ where &env, &mut targets, &dump_base_name, - false, + command_line::get_move_compiler_dump_from_env(), &pipeline::register_formatters, || !env.has_errors(), ) diff --git a/third_party/move/move-compiler/src/command_line/mod.rs b/third_party/move/move-compiler/src/command_line/mod.rs index c829b905d5f36..49ae3e5ff3cff 100644 --- a/third_party/move/move-compiler/src/command_line/mod.rs +++ b/third_party/move/move-compiler/src/command_line/mod.rs @@ -77,3 +77,8 @@ pub const MVC_BACKTRACE_ENV_VAR: &str = "MVC_BACKTRACE"; pub fn get_move_compiler_backtrace_from_env() -> bool { read_bool_env_var(MOVE_COMPILER_BACKTRACE_ENV_VAR) || read_bool_env_var(MVC_BACKTRACE_ENV_VAR) } + +// Flag to dump bytecode files +pub fn get_move_compiler_dump_from_env() -> bool { + read_bool_env_var(MOVE_COMPILER_DUMP_ENV_VAR) || read_bool_env_var(MVC_DUMP_ENV_VAR) +} diff --git a/third_party/move/move-model/bytecode/src/function_target.rs b/third_party/move/move-model/bytecode/src/function_target.rs index 04ecc24bbbfa7..64d7d9ce63184 100644 --- a/third_party/move/move-model/bytecode/src/function_target.rs +++ b/third_party/move/move-model/bytecode/src/function_target.rs @@ -452,6 +452,7 @@ impl<'env> FunctionTarget<'env> { // add location if verbose { + texts.push(format!(" # attr_id {:?}", attr_id)); texts.push(format!( " # {}", self.get_bytecode_loc(attr_id).display(self.global_env()) diff --git a/third_party/move/tools/move-coverage/src/source_coverage.rs b/third_party/move/tools/move-coverage/src/source_coverage.rs index b2ab82212e1a6..db99b408e4c96 100644 --- a/third_party/move/tools/move-coverage/src/source_coverage.rs +++ b/third_party/move/tools/move-coverage/src/source_coverage.rs @@ -115,17 +115,16 @@ fn subtract_locations(locs1: BTreeSet, locs2: &BTreeSet) -> Vec { } else { // loc2 is finished but loc1 is not, // finish adding all loc1 - result.push(current_loc1); - for loc1 in locs1_iter { - result.push(loc1); - } - return result; + break; } } } } } result.push(current_loc1); + for loc1 in locs1_iter { + result.push(loc1); + } } result } @@ -732,9 +731,10 @@ impl SourceCoverage { TextIndicator::Explicit | TextIndicator::On => { write!( output_writer, - "Code coverage per line of code:\n {} indicates the line is not executable or is fully covered during execution\n {} indicates the line is executable but NOT fully covered during execution\nSource code follows:\n", + "Code coverage per line of code:\n {} indicates the line is covered during execution\n {} indicates the line is executable but NOT fully covered during execution\n {} indicates the line is either test code or is not executable\nSource code follows:\n", "+".to_string().green(), "-".to_string().bold().red(), + " ".to_string(), )?; true }, From 953e569b862da840b4de995c559d07477ebc70b5 Mon Sep 17 00:00:00 2001 From: "Brian R. Murphy" <132495859+brmataptos@users.noreply.github.com> Date: Thu, 14 Nov 2024 12:08:14 -0800 Subject: [PATCH 6/6] dump more information about attr_id and source_map during function_generator for red_black_map::add WARNING: debug log output will now be 721MB. --- .../src/file_format_generator/function_generator.rs | 13 +++++++++++-- .../src/file_format_generator/module_generator.rs | 9 +++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/third_party/move/move-compiler-v2/src/file_format_generator/function_generator.rs b/third_party/move/move-compiler-v2/src/file_format_generator/function_generator.rs index c2edfe4faef52..0d40a30546e62 100644 --- a/third_party/move/move-compiler-v2/src/file_format_generator/function_generator.rs +++ b/third_party/move/move-compiler-v2/src/file_format_generator/function_generator.rs @@ -211,10 +211,11 @@ impl<'a> FunctionGenerator<'a> { let code_offset = i as FF::CodeOffset; let bc = &bytecode[i]; debug!( - "Generating code for bytecode {:?} ({}) at offset {}", + "Generating code for bytecode {:?} ({}) at offset {} with attr_id {:?}", bc, bc.display(&ctx.fun, &BTreeMap::new()), - i + i, + bc.get_attr_id(), ); let bytecode_ctx = BytecodeContext { fun_ctx: ctx, @@ -252,6 +253,14 @@ impl<'a> FunctionGenerator<'a> { } } + let func_name = ctx.fun.func_env.get_full_name_str(); + if func_name == "red_black_map::add" { + debug!( + "After Function {} source_map is {:#?}", + func_name, self.gen.source_map + ); + } + // Deliver result let locals = self.gen.signature( &ctx.module, diff --git a/third_party/move/move-compiler-v2/src/file_format_generator/module_generator.rs b/third_party/move/move-compiler-v2/src/file_format_generator/module_generator.rs index ab7c9b862dbd0..e55f905656e8a 100644 --- a/third_party/move/move-compiler-v2/src/file_format_generator/module_generator.rs +++ b/third_party/move/move-compiler-v2/src/file_format_generator/module_generator.rs @@ -12,6 +12,7 @@ use crate::{ }; use codespan_reporting::diagnostic::Severity; use itertools::Itertools; +use log::debug; use move_binary_format::{ file_format as FF, file_format::{AccessKind, FunctionHandle, ModuleHandle, StructDefinitionIndex, TableIndex}, @@ -211,6 +212,14 @@ impl ModuleGenerator { FunctionGenerator::run(self, ctx, fun_env, acquires_list); } + let module_name = module_env.get_full_name_str(); + if module_name == "0xabc::red_black_map" { + debug!( + "After generating module {}, source_map is {:#?}", + module_name, self.source_map + ); + } + // At handles of friend modules for mid in module_env.get_friend_modules() { let handle = self.module_handle(ctx, &module_env.get_loc(), &ctx.env.get_module(mid));