The FUZZ_TARGET macros have many issues:
Fix all issues by using only a single macro.
This refactor does not change behavior.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--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.
Concept ACK
11 | @@ -12,7 +12,7 @@ 12 | #include <string> 13 | #include <vector> 14 | 15 | -FUZZ_TARGET(blockfilter) 16 | +FUZZ_TARGET(blockfilter, {})
nit: would be cool if we could avoid these empty braces somehow
Yes, it is possible. For example:
diff --git a/src/test/fuzz/blockfilter.cpp b/src/test/fuzz/blockfilter.cpp
index aa06af549a..3adc114515 100644
--- a/src/test/fuzz/blockfilter.cpp
+++ b/src/test/fuzz/blockfilter.cpp
@@ -12,7 +12,7 @@
#include <string>
#include <vector>
-FUZZ_TARGET(blockfilter, {})
+FUZZ_TARGET(blockfilter)
{
FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
const std::optional<BlockFilter> block_filter = ConsumeDeserializable<BlockFilter>(fuzzed_data_provider);
diff --git a/src/test/fuzz/fuzz.h b/src/test/fuzz/fuzz.h
index 7c6ac0ccaa..409c721730 100644
--- a/src/test/fuzz/fuzz.h
+++ b/src/test/fuzz/fuzz.h
@@ -38,7 +38,7 @@ void FuzzFrameworkRegisterTarget(std::string_view name, TypeTestOneInput target,
struct name##_Before_Main { \
name##_Before_Main() \
{ \
- FuzzFrameworkRegisterTarget(#name, name##_fuzz_target, __VA_ARGS__); \
+ FuzzFrameworkRegisterTarget(#name, name##_fuzz_target, {__VA_ARGS__}); \
} \
} const static g_##name##_before_main; \
void name##_fuzz_target(FuzzBufferType buffer)
diff --git a/src/test/fuzz/script_assets_test_minimizer.cpp b/src/test/fuzz/script_assets_test_minimizer.cpp
index 29d2595266..7862be2f21 100644
--- a/src/test/fuzz/script_assets_test_minimizer.cpp
+++ b/src/test/fuzz/script_assets_test_minimizer.cpp
@@ -186,7 +186,7 @@ void Test(const std::string& str)
void test_init() {}
-FUZZ_TARGET(script_assets_test_minimizer, {.init = test_init, .hidden = true})
+FUZZ_TARGET(script_assets_test_minimizer, .init = test_init, .hidden = true)
{
if (buffer.size() < 2 || buffer.back() != '\n' || buffer[buffer.size() - 2] != ',') return;
const std::string str((const char*)buffer.data(), buffer.size() - 2);
diff --git a/src/wallet/test/fuzz/coincontrol.cpp b/src/wallet/test/fuzz/coincontrol.cpp
index 36b2bd089b..0f71f28df2 100644
--- a/src/wallet/test/fuzz/coincontrol.cpp
+++ b/src/wallet/test/fuzz/coincontrol.cpp
@@ -20,7 +20,7 @@ void initialize_coincontrol()
g_setup = testing_setup.get();
}
-FUZZ_TARGET(coincontrol, {.init = initialize_coincontrol})
+FUZZ_TARGET(coincontrol, .init = initialize_coincontrol)
{
FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
const auto& node = g_setup->m_node;
Do you want me to push that?
Yes, I think that'd be nice, but also don't have a super strong preference
Concept ACK
Adding a new option requires doubling the number of existing macros in the worst case.
I've run into this before and it is indeed pretty annoying.
38 | -#define FUZZ_TARGET_INIT(name, init_fun) \ 39 | - FUZZ_TARGET_INIT_HIDDEN(name, init_fun, false) 40 | +void FuzzFrameworkRegisterTarget(std::string_view name, TypeTestOneInput target, FuzzTargetOptions opts); 41 | 42 | -#define FUZZ_TARGET_INIT_HIDDEN(name, init_fun, hidden) \ 43 | +#define FUZZ_TARGET(name, ...) \
I believe that before C++20 this actually requires at least one argument after name, technically speaking (it's been a widely-adopted extension to allow that, however).
Currently all call sites do pass at least one token. However, #28065 (review) wouldn't, I guess.
Oh, yes, I commented incorrectly. This was meant as a response to that comment.
Thanks for the comments. Fixed everything.
59 | @@ -60,9 +60,9 @@ std::map<std::string_view, std::tuple<TypeTestOneInput, TypeInitialize, TypeHidd 60 | return g_fuzz_targets; 61 | } 62 | 63 | -void FuzzFrameworkRegisterTarget(std::string_view name, TypeTestOneInput target, TypeInitialize init, TypeHidden hidden) 64 | +void FuzzFrameworkRegisterTarget(std::string_view name, TypeTestOneInput target, FuzzTargetOptions opts) 65 | { 66 | - const auto it_ins = FuzzTargets().try_emplace(name, std::move(target), std::move(init), hidden); 67 | + const auto it_ins = FuzzTargets().try_emplace(name, std::move(target), std::move(opts.init), opts.hidden);
nit: I guess we could have g_fuzz_targets store the opts directly? Would mean less churn when adding an option.
Thanks, done. Also used C++ 17 structured binding declaration to get the map key-value.
Code review ACK fa6fa1f7b0120c13be0a515072065db82855b111
* This allows to reduce the number of total macros.
* Also, adding a new option no longer requires doubling the number of
macros in the worst case.
-BEGIN VERIFY SCRIPT-
ren() { sed --regexp-extended -i "s|$1|$2|g" $(git grep -l --extended-regexp "$1"); }
# Replace FUZZ_TARGET_INIT
ren 'FUZZ_TARGET_INIT\((.+), (.+)\)' 'FUZZ_TARGET(\1, .init = \2)'
# Delete unused FUZZ_TARGET_INIT
sed -i -e '37,39d' src/test/fuzz/fuzz.h
-END VERIFY SCRIPT-69 | 70 | -void FuzzFrameworkRegisterTarget(std::string_view name, TypeTestOneInput target, TypeInitialize init, TypeHidden hidden) 71 | +void FuzzFrameworkRegisterTarget(std::string_view name, TypeTestOneInput target, FuzzTargetOptions opts) 72 | { 73 | - const auto it_ins = FuzzTargets().try_emplace(name, std::move(target), std::move(init), hidden); 74 | + const auto it_ins{FuzzTargets().try_emplace(name, FuzzTarget /* temporary can be dropped in C++20 */ {std::move(target), std::move(opts)})};
const auto it_ins{FuzzTargets().try_emplace(name, std::move(target), std::move(opts))};
Haven't tracked down which C++20 LWG paper this was. Let me know if someone finds something.
ACK fa6dfaaf45bde465969fa7d8fa6b6574a497a6ca