Addresses #30950.
Any targets that require BUILD_ON_FUZZING=ON (currently only p2p_headers_presync) to work properly can now set require_build_for_fuzzing as an option. If BUILD_FOR_FUZZING is not on and you try to run a target that has this option, then there’s a message printed upon exit.
With this change, the CI will be able to run the fuzz test runner without any timeouts/failures.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process. A summary of reviews will appear here.
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.
-DBUILD_FUZZ_BINARY=ON but not BUILD_FOR_FUZZING. Then run the fuzz test runner and you should see the message in place of the p2p_headers_presync test.
FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION is required, is there any value in registering p2p_headers_presync as a target if it’s not set? Maybe a new FUZZ_BUILD_ONLY_TARGET or so, which would conditionally register?
158+// The FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION macro is set when BUILD_FOR_FUZZING=ON, so
159+// it's used as a proxy to detect fuzzing-only build mode.
160+#if !defined(FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION)
161+ if (it->second.opts.require_build_for_fuzzing) {
162+ std::cout << "The fuzz target " << g_fuzz_target << " requires BUILD_FOR_FUZZING=ON" << std::endl;
163+ std::exit(EXIT_SUCCESS);
I think this patch is still incomplete. This will just turn the timeout into an early error. Obviously a fast error is better than a long timeout, but it would be better to exclude the test from the non-fuzz-only builds in the CI and mention in the pull request description that require_build_for_fuzzing targets must be excluded manually in non-fuzz-only builds.
An alternative would be to not register it, as explained by Cory.
133 g_testing_setup = setup.get();
134 }
135 } // namespace
136
137-FUZZ_TARGET(p2p_headers_presync, .init = initialize)
138+FUZZ_BUILD_ONLY_TARGET(p2p_headers_presync, .init = initialize)
style nit: Seems fine, but I preferred the previous approach where all fuzz targets are registered with the same macro. This should be possible by guarding the FuzzFrameworkRegisterTarget behind a if constexpr (opts.require_build_for_fuzzing and not build_for_fuzzing).
This should also make #30882 easier to implement, because it doesn’t have to deal with two macros.
Agreed, that’s a better way to do it. Let me know what you think about d470af336a3f66230e9c68ffc924caad0ad84980. Another possibility I was thinking of was something like:
0#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
1#define CONDITIONAL_REGISTER(name, target, opts) FuzzFrameworkRegisterTarget(name, target, opts)
2#else
3#define CONDITIONAL_REGISTER(name, target, opts)
4 do {
5 if (!(opts).requires_fuzz_build) {
6 FuzzFrameworkRegisterTarget(name, target, opts);
7 }
8 } while(0)
9#endif
Is there an approach I’m missing?
DETAIL_FUZZ seems better. And not as many macros.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/31178187447
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
37
38+#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
39+constexpr bool should_register = true;
40+#else
41+constexpr bool should_register = false;
42+#endif
0constexpr bool build_for_fuzzing
1#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
2 {true};
3#else
4 {false};
5#endif
nit: Could rename, so that it can be re-used in the future for other stuff, if needed?
Labels
Tests