Adds an optional integer coverage_check argument to deterministic-fuzz-coverage:
0: run both checks, preserving the current default behavior1: only check each corpus input individually2: only check all corpus inputs in one processThis makes it easier to isolate per-input nondeterminism from cross-input state leakage. Good for debugging.
<!--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/35809.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | Crypt-iQ |
| Stale ACK | jeanpablojp |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
tACK e01ca9bacdeb860291b1824cac7fb099a3b41111
Built the tool and ran all three modes against an instrumented binary I put together here, all green, and the check catches nondeterminism on both paths. Mode 0 came out identical to master.
Silly nit: could the motivation go into the commit message?
tACK e01ca9b
Built the tool and ran all three modes against an instrumented binary I put together here, all green, and the check catches nondeterminism on both paths. Mode 0 came out identical to master.
Silly nit: could the motivation go into the commit message?
Sorry for the delay, updated.
36 | + )) 37 | + })?, 38 | + None => 0, 39 | + }; 40 | + match mode { 41 | + 0 => Ok(Self::Both),
Would be cleaner to use properly named strings for this enum? E.g:
"both" => Ok(Self::Both),
"single" => Ok(Self::IndividualInputs),
"combined" => Ok(Self::AllInputs),
other => Err(exit_help(&format!(
"Invalid coverage check mode '{other}'. Expected 'both', 'single', or 'combined'"
))),
Possibly this could just be a single bool without any extra class and with proper named args:
let mut check_individual = true; // default is "both"
// skip the pos args (everything after is a named arg)
for arg in env::args().skip(5) {
if let Some(value) = arg.strip_prefix("--mode=") {
check_individual = match value {
"both" => true,
"combined" => false,
other => return Err(help(&format!(
"Invalid mode '{other}'. Expected 'both' or 'combined'"
))),
};
}else {
return Err(help(&format!("Too many args, or unknown named arg: {arg}")));
}
}
The rationale being that there should be no reason to skip the full combined check, only the expensive single check. Otherwise, two boolean flags are still simpler than a full enum class for this?
The rationale being that there should be no reason to skip the full combined check, only the expensive single check. Otherwise, two boolean flags are still simpler than a full enum class for this?
The motivation to propose this PR is when I fixed a coverage issue, I run the tool to verify if it worked, and it's really time-consuming. Forgot which target I used, but I recall the combined run itself is time-consuming too, let's go wtih the former one?
"both" => Ok(Self::Both),
"single" => Ok(Self::IndividualInputs),
"combined" => Ok(Self::AllInputs),
other => Err(exit_help(&format!(
"Invalid coverage check mode '{other}'. Expected 'both', 'single', or 'combined'"
))),
Seems fine, but would be good to use named args at some point 😅
Add an optional coverage_check argument to deterministic-fuzz-coverage.
The `both` mode runs both checks and preserves the current default
behavior. The `single` mode checks each corpus input individually.
The `combined` mode checks all corpus inputs in a single process.
This makes it easier to distinguish per-input nondeterminism from
cross-input state leakage. Good for debugging.
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
21 | @@ -22,9 +22,13 @@ repository must have been cloned. Finally, a fuzz target has to be picked 22 | before running the tool: 23 | 24 | ``` 25 | -cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/build_dir $PWD/qa-assets/fuzz_corpora fuzz_target_name 26 | +cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/build_dir $PWD/qa-assets/fuzz_corpora fuzz_target_name [parallelism] [coverage_check] 27 | ``` 28 | 29 | +The optional `coverage_check` argument controls which checks are run: `both` 30 | +runs both checks, `single` only checks each input individually, and `combined`
nit: could be more descriptive for somebody reading this and say: both runs each input individually and all inputs in one go, or put it at the end after combined
crACK 754a9e908ff828341d8eee657584b8ebd31f58e3