This is almost a "refactor" to tidy up the error messages. Apart from the messages, the behavior of the tools is identical.
This was requested in #31901 (review).
Previously, the tool would abort the program early on some errors. Now, the tool propagates an std::result::Result::Err up to main via an early return. Getting rid of the aborts also allows to drop the RUST_BACKTRACE env setting.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32074.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | hodlinator, janb84 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
53 | match output {
54 | Ok(output) if output.status.success() => {}
55 | - _ => {
56 | - exit_help(&format!("The tool {} is not installed", tool));
57 | - }
58 | + _ => Err(exit_help(&format!("The tool {} is not installed", tool)))?,
info: Would have expected:
_ => return Err(exit_help(&format!("The tool {} is not installed", tool))),
? feels like returning is an open question, but after Err() it's always true... maybe this is still idiomatic though. ? in other places implies a possible return, so probably just takes some getting used to.
I found Err(...)? nicer as an early-return statement, but no strong opinion
90 | + Err(exit_help("--help requested"))?; 91 | } 92 | - let corpora_dir = args 93 | - .get(2) 94 | - .unwrap_or_else(|| exit_help("Must set fuzz corpora dir")); 95 | + let corpora_dir = args.get(2).ok_or(exit_help("Must set fuzz corpora dir"))?;
nit: Unrelated to this change, but would be nice to avoid "shadowing".
let corpora_dir = Path::new(args.get(2).ok_or(exit_help("Must set fuzz corpora dir"))?);
In Rust shadowing is a feature, but happy to change, no strong opinion.
Thread #32074 (review):
I like shadowing for if let Some(foo) = foo, in other cases it ups the code complexity in my book.
But this is not an egregious case. Fine with leaving as-is unless others agree with me.
@hodlinator I'll wait for you to give the ack/review on this, and once you are happy that your feedback is fully addressed, I'll move it out of draft
196 | @@ -175,29 +197,36 @@ fn deterministic_coverage( 197 | // 198 | // Also, This can catch issues where several fuzz inputs are non-deterministic, but the sum of 199 | // their overall coverage trace remains the same across runs and thus remains undetected. 200 | + println!("Check each fuzz input individually ..."); 201 | for entry in entries {
Unrelated, but while looking for faults with this PR and trying to trigger errors, I got distracted into making this run in parallel. Earlier version triggered OOM-killer and this one is still in a super-unbaked state. (Output should be buffered and sequenced, parallization logic should be refined, file naming uniqueness-approach is bad, etc).
<details><summary>diff</summary>
diff --git a/contrib/devtools/deterministic-fuzz-coverage/src/main.rs b/contrib/devtools/deterministic-fuzz-coverage/src/main.rs
index 423393181c..171afbbed7 100644
--- a/contrib/devtools/deterministic-fuzz-coverage/src/main.rs
+++ b/contrib/devtools/deterministic-fuzz-coverage/src/main.rs
@@ -2,11 +2,13 @@
// Distributed under the MIT software license, see the accompanying
// file COPYING or https://opensource.org/license/mit/.
+use std::collections::VecDeque;
use std::env;
use std::fs::{read_dir, File};
use std::path::{Path, PathBuf};
use std::process::{Command, ExitCode};
use std::str;
+use std::thread;
/// A type for a complete and readable error message.
type AppError = String;
@@ -106,8 +108,6 @@ fn deterministic_coverage(
fuzz_target: &str,
) -> AppResult {
let using_libfuzzer = using_libfuzzer(fuzz_exe)?;
- let profraw_file = build_dir.join("fuzz_det_cov.profraw");
- let profdata_file = build_dir.join("fuzz_det_cov.profdata");
let corpus_dir = corpora_dir.join(fuzz_target);
let mut entries = read_dir(&corpus_dir)
.map_err(|err| {
@@ -121,7 +121,13 @@ fn deterministic_coverage(
.collect::<Vec<_>>();
entries.sort_by_key(|entry| entry.file_name());
let run_single = |run_id: u8, entry: &Path| -> Result<PathBuf, AppError> {
- let cov_txt_path = build_dir.join(format!("fuzz_det_cov.show.{run_id}.txt"));
+ let file_name = match entry.file_name() {
+ Some(f) => Ok(f.to_string_lossy()),
+ None => Err("noooo".to_string()),
+ }?;
+ let cov_txt_path = build_dir.join(format!("fuzz_det_cov.show.{fuzz_target}.{file_name}.{run_id}.txt"));
+ let profraw_file = build_dir.join(format!("fuzz_det_cov.{fuzz_target}.{file_name}.profraw"));
+ let profdata_file = build_dir.join(format!("fuzz_det_cov.{fuzz_target}.{file_name}.profdata"));
if !{
{
let mut cmd = Command::new(fuzz_exe);
@@ -197,20 +203,44 @@ The coverage was not deterministic between runs.
//
// Also, This can catch issues where several fuzz inputs are non-deterministic, but the sum of
// their overall coverage trace remains the same across runs and thus remains undetected.
- println!("Check each fuzz input individually ...");
- for entry in entries {
- let entry = entry.path();
- if !entry.is_file() {
- Err(format!("{} should be a file", entry.display()))?;
+ println!("Check each fuzz input individually, {} inputs...", entries.len());
+ thread::scope(|s| -> AppResult {
+ use std::thread::ScopedJoinHandle;
+ let mut handles = VecDeque::<ScopedJoinHandle<_>>::new();
+ for entry in entries {
+ let entry = entry.path();
+ if !entry.is_file() {
+ return Err(format!("{} should be a file", entry.display()));
+ }
+ let entry = entry.to_path_buf();
+ handles.push_back(s.spawn(move || -> AppResult {
+ let cov_txt_base = run_single(0, &entry)?;
+ let cov_txt_repeat = run_single(1, &entry)?;
+ check_diff(
+ &cov_txt_base,
+ &cov_txt_repeat,
+ &format!("The fuzz target input was {}.", entry.display()),
+ )
+ }));
+ if handles.len() >= std::thread::available_parallelism().map_err(|e| format!("Failed checking available_parallelism: {e}"))?.into() {
+ match handles.remove(0) {
+ Some(front) => match front.join() {
+ Err(e) => Err(format!("err: {:?}", e)),
+ Ok(result) => result,
+ },
+ None => Ok({})
+ }?
+ }
}
- let cov_txt_base = run_single(0, &entry)?;
- let cov_txt_repeat = run_single(1, &entry)?;
- check_diff(
- &cov_txt_base,
- &cov_txt_repeat,
- &format!("The fuzz target input was {}.", entry.display()),
- )?;
- }
+ for h in handles {
+ match h.join() {
+ Err(e) => Err(format!("err: {:?}", e)),
+ Ok(result) => result,
+ }?
+ }
+
+ Ok(())
+ })?;
// Finally, check that running over all fuzz inputs in one process is deterministic as well.
// This can catch issues where mutable global state is leaked from one fuzz input execution to
// the next.
Nice. I think this is sufficient as an approach for a dev-only tool. Happy to push it here, or review your follow-up. Just let me know.
Thread #32074 (review): If you want to implement some version of it I'm happy to review, or we can defer it, not sure when I'll be able to get it into good shape.
If you want to implement some version of it I'm happy to review, or we can defer it, not sure when I'll be able to get it into good shape.
Done in #32158. I ended up only taking thread::scope and VecDeque and restructured the rest. Lmk if this sounds good, or if you want to be listed as co-author.
20 | + Err(err) => { 21 | + eprintln!("{}", err); 22 | + ExitCode::FAILURE 23 | + } 24 | + } 25 | +}
This is slightly boilerplate-y but it's a price I'm willing to pay for the rest of the diff.
Might be less annoying to have at the bottom of the file so it's out of the way?
thx, done
9 | +use std::path::{Path, PathBuf}; 10 | +use std::process::{Command, ExitCode}; 11 | use std::str; 12 | 13 | +/// A type for a complete and readable error message. 14 | +type AppError = String;
Tempting to suggest
struct AppError(String);
But it does add a fair amount of noise where it's instantiated, so I understand we might want to go a bit less type-safe for these tools.
Yeah, the string ends up in Err eventually, so it should be type-safe enough for nothing to go wrong. At least I can't see any risk here.
@hodlinator I'll wait for you to give the ack/review on this, and once you are happy that your feedback is fully addressed, I'll move it out of draft
Can't promise I won't find any more issues, but this turned out surprisingly well IMO. Thanks for taking the time to work on it!
rebased with a move-only change, and replied to all feedback
144 | - let cov_file = File::create(&cov_txt_path).expect("Failed to create coverage txt file"); 145 | - assert!(Command::new(LLVM_COV) 146 | + .map_err(|e| format!("{LLVM_PROFDATA} merge failed with {e}"))? 147 | + .success() 148 | + { 149 | + Err(format!("{LLVM_PROFDATA} merge failed"))?;
nit: Tried running without coverage support compiled in:
₿ cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/build_fuzz ../qa-assets/fuzz_corpora/ process_message
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.03s
Running `contrib/devtools/deterministic-fuzz-coverage/target/debug/deterministic-fuzz-coverage /home/hodlinator/bitcoin/build_fuzz ../qa-assets/fuzz_corpora/ process_message`
Check if using libFuzzer ...
Check each fuzz input individually ...
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 1580959840
INFO: Loaded 1 modules (543273 inline 8-bit counters): 543273 [0x559d31989d40, 0x559d31a0e769),
INFO: Loaded 1 PC tables (543273 PCs): 543273 [0x559d31a0e770,0x559d32258a00),
/home/hodlinator/bitcoin/build_fuzz/bin/fuzz: Running 1 inputs 1 time(s) each.
Running: ../qa-assets/fuzz_corpora/process_message/00023ad97f6f03d0899bfa81d8ff7d095d8b4ee9
Executed ../qa-assets/fuzz_corpora/process_message/00023ad97f6f03d0899bfa81d8ff7d095d8b4ee9 in 2 ms
***
*** NOTE: fuzzing was not performed, you have only
*** executed the target code on a fixed set of inputs.
***
error: /home/hodlinator/bitcoin/build_fuzz/fuzz_det_cov.profraw: No such file or directory
llvm-profdata merge failed
Could change to:
Err(format!("{LLVM_PROFDATA} merge failed. This can be a sign of compiling without code coverage support."))?;
Same for unit tests.
thx, done!
ACK fab6fe3c1676202bd9469c3e9f9454911c92602b
Removes the use of assert!()s, expect(), unwrap_or_else(), error backtraces and exit(1) in favor of more Rust-idiomatic user/environment error handling through Result<>.
Heavy use of map_err() and ? to enable early returns on error results in quite a small diff and pleasant code.
Tested running fuzz-util on non-existent build-dir with expected error. Tried running fuzz executable without coverage support (see inline comment). Ran with coverage and found deterministic uint160_deserialize and non-deterministic process_message.
Tested unit test miniscript_tests for determinism successfully.
re-ACK fa7a40d952ad7588f45f6229aeae754b02fdfb55
See prior ACK for high level take on the change.
New changes:
Perhaps it’s worth adding a note to clarify that the fuzz tool should be allowed to run until it completes. Given the nature of fuzz tests, someone might assume that, unlike fuzzing runs with "runs=1," this process will continue indefinitely, and thus a few "succeeded against 1 file in xs" messages might seem sufficient.
thx, maybe in a follow-up. With two acks, this seems close to merge
thx, maybe in a follow-up. With two acks, this seems close to merge
Agreed ! Lets not invalidate the ACKS over this.
ACK fa7a40d
A
