build: Make G_FUZZING constexpr, require -DBUILD_FOR

maflcko commented at 1:00 pm on October 31, 2024: member

g_fuzzing is used inside Assume at runtime, causing significant overhead in hot paths. See #31178

One could simply remove the g_fuzzing check from the Assume, but this would make fuzzing a bit less useful. Also, it would be unclear if g_fuzzing adds a runtime overhead in other code paths today or in the future.

Fix all issues by making G_FUZZING equal to the build option BUILD_FOR_FUZZING, and for consistency in fuzzing, require it to be set when executing any fuzz target.

Fixes #31178

Temporarily this drops fuzzing from two CI tasks, but they can be re-added in a follow-up with something like https://github.com/bitcoin/bitcoin/pull/31073

ci: Temporarily disable macOS/Windows fuzz step

The fuzz binary is still compiled. This is required for the next commit.

fae3cf0ffa

Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to execute a fuzz target fafbf8acf4

DrahtBot commented at 1:00 pm on October 31, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31191.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	dergoegge, marcofleon, davidgumberg, ryanofsky

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31161 (cmake: Set top-level target output locations by hebasto)

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.

maflcko renamed this:
~~Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz~~
build: Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz
on Oct 31, 2024

DrahtBot added the label Build system on Oct 31, 2024

in src/util/check.h:53 in fafbf8acf4

49@@ -44,7 +50,7 @@ void assertion_fail(std::string_view file, int line, std::string_view func, std:
50 template <bool IS_ASSERT, typename T>
51 constexpr T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] const char* file, [[maybe_unused]] int line, [[maybe_unused]] const char* func, [[maybe_unused]] const char* assertion)
52 {
53-    if (IS_ASSERT || std::is_constant_evaluated() || g_fuzzing
54+    if (IS_ASSERT || std::is_constant_evaluated() || G_FUZZING

dergoegge commented at 1:13 pm on October 31, 2024:

0    if constexpr (IS_ASSERT || std::is_constant_evaluated() || G_FUZZING

marcofleon commented at 2:23 pm on October 31, 2024:

Do we need the std::is_constant_evaluated() then? I’m getting this warning everywhere when I build

0../../../../src/util/check.h:53:32: warning: 'std::is_constant_evaluated' will always evaluate to 'true' in a manifestly constant-evaluated expression [-Wconstant-evaluated]
1   53 |     if constexpr (IS_ASSERT || std::is_constant_evaluated() || G_FUZZING
2      |                                ^
3../../../../src/util/check.h:53:32: warning: 'std::is_constant_evaluated' will always evaluate to 'true' in a manifestly constant-evaluated expression [-Wconstant-evaluated]

maflcko commented at 2:54 pm on October 31, 2024:

Yeah, the two are exclusive. It is not possible to add constexpr here and use std::is_constant_evaluated() correctly. The meaning of std::is_constant_evaluated() in this context denotes whether the Assume() was called in a constexpr context, not whether the if condition was evaluated constant.

maflcko commented at 2:55 pm on October 31, 2024:

(resolving for now)

dergoegge approved

dergoegge commented at 1:13 pm on October 31, 2024: member

utACK fafbf8acf419d5e2ca307e5804099361ca7471af

maflcko force-pushed on Oct 31, 2024

DrahtBot added the label CI failed on Oct 31, 2024

DrahtBot commented at 2:41 pm on October 31, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/32337137613

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.

maflcko force-pushed on Oct 31, 2024

marcofleon commented at 3:50 pm on October 31, 2024: contributor

Tested ACK fafbf8acf419d5e2ca307e5804099361ca7471af

DrahtBot removed the label CI failed on Oct 31, 2024

davidgumberg commented at 10:49 pm on October 31, 2024: contributor

Tested ACK https://github.com/bitcoin/bitcoin/pull/31191/commits/fafbf8acf419d5e2ca307e5804099361ca7471af

This solves #31178

./build/src/bench/bench_bitcoin -filter=LinearizeOptimallyExample11 -min-time=30000

branch	ns/op	op/s	err%	ins/op	cyc/op	IPC	bra/op	miss%	total	benchmark
master (f07a533dfcb)	380,031,566.62	2.63	0.3%	7,324,884,836.62	1,321,463,005.62	5.543	898,388,779.12	0.2%	32.69	`LinearizeOptimallyExample11`
branch (fafbf8acf)	301,497,436.82	3.32	0.2%	4,990,499,128.40	1,046,022,020.91	4.771	352,054,052.20	0.3%	32.53	`LinearizeOptimallyExample11`

It also seems to reasonably solve #30950 & #31057 which were the motivation of #31093 which introduced the regression.

I don’t have a view on this, but just want to document that this PR does not address the described use case of building for fuzzing without building the fuzz binary (https://github.com/bitcoin/bitcoin/issues/31057#issuecomment-2400458885) which is what motivated making g_fuzzing a runtime check.

0BUILD_FOR_FUZZING:BOOL=OFF
1BUILD_FUZZ_BINARY:BOOL=ON
I build with these options to be able to be able to know if changes not related to fuzzing will break the build.

maflcko commented at 7:08 am on November 1, 2024: member

I don’t have a view on this, but just want to document that this PR does not address the described use case of building for fuzzing without building the fuzz binary (#31057 (comment)) which is what motivated making g_fuzzing a runtime check.
0BUILD_FOR_FUZZING:BOOL=OFF
1BUILD_FUZZ_BINARY:BOOL=ON
I build with these options to be able to be able to know if changes not related to fuzzing will break the build.

Can you explain this a bit better? This pull request does not change anything about being able to build with these options and be able to see if the fuzz binary build breaks.

Specifically,

Everything in an if constexpr is still looked at by the compiler and any code issues should be detected, even with the above build options, before and after this pull request. See also #29315 for a different change using if constexpr for that purpose (as opposed to preprocessor directives).
All fuzz pre-run options such as PRINT_ALL_FUZZ_TARGETS_AND_ABORT, or WRITE_ALL_FUZZ_TARGETS_AND_ABORT, etc will still work normally before and after this pull request.
Only executing a fuzz target with the above build options is not possible, which is the reason for the first temporary CI commit.

Maybe I am misunderstanding, so it would help to share exact steps to reproduce of the use case that you are claiming is not addressed.

ryanofsky commented at 1:22 pm on November 1, 2024: contributor

Code review ACK fafbf8acf419d5e2ca307e5804099361ca7471af but approach -0, because this approach means libraries built for fuzz testing do not function correctly if used in a release, and libraries built for releases are mostly useless for fuzz testing. So I would like to at least consider other solutions to this problem even if we go with this one.

#31178 makes it pretty clear that if we want to be able to write Assume() statements in hot paths, we need to be able to compile them out in release builds to avoid impacting performance, and leave them compiled into builds that are used for fuzzing. So I do think the build system should support compiling libraries differently for fuzz testing and releases. But I don’t think the build system should go so far as to make libraries built for fuzz testing and libraries built for releases mutually incompatible.

Also I don’t really buy the idea that if you put an Assume() statement in a hot path, you just be to assume it has no cost and that compiler will optimize it out. I think majority of Assume() statements are not in hot paths, and main use-case for Assume() is not to be a faster Assert() that you use in performance-critical code, but to be a safer Assert() that you can use to catch bugs during development, but will not crash the entire program and provide a terrible user experience when bugs (inevitably) occur in production.

So I think a better alternative to this PR might just be to provide a better alternative to Assume(). I think most uses of Assume() are fine as they are, but a few really are on hot paths, and I tried adding a simple counter to identify them in the linearizeoptimallyexample11 benchmark. If I disable these checks, the speedup is equivalent to this PR without drawbacks of this PR. The change I would propose based on this is:

  0--- a/CMakeLists.txt
  1+++ b/CMakeLists.txt
  2@@ -225,6 +225,7 @@ if(BUILD_FOR_FUZZING)
  3   set(BUILD_GUI_TESTS OFF)
  4   set(BUILD_BENCH OFF)
  5   set(BUILD_FUZZ_BINARY ON)
  6+  target_compile_definitions(core_interface INTERFACE ENABLE_SLOWCHECK)
  7 endif()
  8 
  9 include(ProcessConfigurations)
 10--- a/cmake/module/ProcessConfigurations.cmake
 11+++ b/cmake/module/ProcessConfigurations.cmake
 12@@ -126,6 +126,7 @@ target_compile_definitions(core_interface_debug INTERFACE
 13   DEBUG_LOCKCONTENTION
 14   RPC_DOC_CHECK
 15   ABORT_ON_FAILED_ASSUME
 16+  ENABLE_SLOWCHECK
 17 )
 18 # We leave assertions on.
 19 if(MSVC)
 20--- a/src/cluster_linearize.h
 21+++ b/src/cluster_linearize.h
 22@@ -337,7 +337,7 @@ struct SetInfo
 23     /** Add a transaction to this SetInfo (which must not yet be in it). */
 24     void Set(const DepGraph<SetType>& depgraph, ClusterIndex pos) noexcept
 25     {
 26-        Assume(!transactions[pos]);
 27+        SLOWCHECK(!transactions[pos]);
 28         transactions.Set(pos);
 29         feerate += depgraph.FeeRate(pos);
 30     }
 31--- a/src/util/bitset.h
 32+++ b/src/util/bitset.h
 33@@ -281,7 +281,7 @@ class MultiIntBitSet
 34         /** Progress to the next 1 bit (only if != IteratorEnd). */
 35         constexpr Iterator& operator++() noexcept
 36         {
 37-            Assume(m_idx < N);
 38+            SLOWCHECK(m_idx < N);
 39             m_val &= m_val - I{1U};
 40             if (m_val == 0) {
 41                 while (true) {
 42@@ -301,7 +301,7 @@ class MultiIntBitSet
 43         /** Get the current bit position (only if != IteratorEnd). */
 44         constexpr unsigned operator*() const noexcept
 45         {
 46-            Assume(m_idx < N);
 47+            SLOWCHECK(m_idx < N);
 48             return m_pos;
 49         }
 50     };
 51@@ -316,7 +316,7 @@ public:
 52     /** Set a bit to 1. */
 53     void constexpr Set(unsigned pos) noexcept
 54     {
 55-        Assume(pos < MAX_SIZE);
 56+        SLOWCHECK(pos < MAX_SIZE);
 57         m_val[pos / LIMB_BITS] |= I{1U} << (pos % LIMB_BITS);
 58     }
 59     /** Set a bit to the specified value. */
 60--- a/src/util/check.h
 61+++ b/src/util/check.h
 62@@ -81,6 +81,12 @@ constexpr T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] con
 63 /**
 64  * Assume is the identity function.
 65  *
 66+ * Assume() should be used instead of Assert() in cases where if the condition
 67+ * is not true, it indicates there is a bug, and you want to handle the bug by
 68+ * crashing in debug builds and in fuzz tests, but want to avoid unnecessary
 69+ * crashes in release builds and handle it with logging, warning, or other
 70+ * fallback behavior.
 71+ *
 72  * - Should be used to run non-fatal checks. In debug builds it behaves like
 73  *   Assert()/assert() to notify developers and testers about non-fatal errors.
 74  *   In production it doesn't warn or log anything.
 75@@ -90,6 +96,24 @@ constexpr T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] con
 76  */
 77 #define Assume(val) inline_assertion_check<false>(val, __FILE__, __LINE__, __func__, #val)
 78 
 79+/**
 80+ * SLOWCHECK() can be used to perform checks that are enabled in debug and fuzz
 81+ * builds but are skipped in release builds. It is meant to be used for checks
 82+ * that are slow, either because they occur in hot code paths, or because the
 83+ * checks themselves are expensive. Assert() should be preferred for critical
 84+ * checks which are not in performance-sensitive code.
 85+ *
 86+ * SLOWCHECK() is basically equivalent to the traditional C/C++ assert() macro
 87+ * For historic reasons, Bitcoin Core cannot be compiled with NDEBUG so there is
 88+ * no way to skip assert() checks in release builds, and SLOWCHECK() restores
 89+ * this lost functionality.
 90+ */
 91+#ifdef ENABLE_SLOWCHECK
 92+#define SLOWCHECK(val) assert(val);
 93+#else
 94+#define SLOWCHECK(val) assert(1 || (val));
 95+#endif
 96+
 97 /**
 98  * NONFATAL_UNREACHABLE() is a macro that is used to mark unreachable code. It throws a NonFatalCheckError.
 99  */
100--- a/src/util/vecdeque.h
101+++ b/src/util/vecdeque.h
102@@ -74,7 +74,7 @@ class VecDeque
103     /** What index in the buffer does logical entry number pos have? */
104     size_t BufferIndex(size_t pos) const noexcept
105     {
106-        Assume(pos < m_capacity);
107+        SLOWCHECK(pos < m_capacity);
108         // The expression below is used instead of the more obvious (pos + m_offset >= m_capacity),
109         // because the addition there could in theory overflow with very large deques.
110         if (pos >= m_capacity - m_offset) {

I would curious to know from @sipa and others if they think this approach makes sense and does not add too much of a burden.

Specifically results I saw testing this with the linearizeoptimallyexample11 benchmark were 1.42 op/sec on master, 1.77 op/sec reverting 9f243cd7fa6654e3b71ba6bff82cceed547c5d53 (#31093), 1.77 op/sec cherry-picking fafbf8acf419d5e2ca307e5804099361ca7471af from this PR, and 1.83 op/sec with the proposed change.

maflcko commented at 2:40 pm on November 1, 2024: member

this approach means libraries built for fuzz testing do not function correctly if used in a release, and libraries built for releases are mostly useless for fuzz testing.

I don’t think it is a supported use-case to take libraries from one build (with different build options) and drop them into another build (with different build options) and expect it to work, or be a supported use-case. This has also been the case up until two weeks ago, before commit 9f243cd7fa6654e3b71ba6bff82cceed547c5d53. Also your SLOWCHECK suggestion doesn’t seem to change the fact that mix-matching differently compiled libraries can result in an unsafe or less useful binary using those libraries.

Also I don’t really buy the idea that if you put an Assume() statement in a hot path, you just be to assume it has no cost and that compiler will optimize it out.

I don’t think I’ve claimed this. It has always been the assumption that Assume(expr), Assert(expr), and assert(expr) have a runtime cost of at least (void)(expr). This is also explained in the dev notes: “the expression is always evaluated.” On current master, as explained by you in #31178 (comment), the runtime cost of Assume(expr) in some cases increased by an additional check of bool{g_fuzzing}. This change here is simply restoring what has been the assumption in all previous releases of Bitcoin Core.

I think majority of Assume() statements are not in hot paths, and main use-case for Assume() is not to be a faster Assert() that you use in performance-critical code

I agree and I don’t think I’ve claimed otherwise. I just don’t see the use-case to mix-match libraries from different builds and I think this change makes sense even if you go ahead with your SLOWCHECK idea.

SLOWCHECK

It may be best to submit this as a separate pull request. I think it is fine if this one keeps sitting for a few more weeks and it can be closed/merged, possibly depending on the result of your SLOWCHECK pull request.

ryanofsky commented at 3:38 pm on November 1, 2024: contributor

I don’t think it is a supported use-case to take libraries from one build (with different build options) and drop them into another build

Agree it should not be a generally supported use case. And I do think it’s nice to have a BUILD_FOR_FUZZING option that enables the best options for builds specifically doing fuzzing.

But for normal builds, I don’t think the whole codebase should need to be recompiled with different options just to get a useful fuzz binary. And I don’t think it is good for BUILD_FOR_FUZZING option to create strange libraries that seem functional but skip proof of work checks. I did ACK this PR in case we think these problems are worth having so Assume() can be optimized out in hot code paths. But I think these are negative consequences of this PR, even if worth the tradeoff.

Also I don’t really buy the idea that if you put an Assume() statement in a hot path, you just be to assume it has no cost and that compiler will optimize it out.

I don’t think I’ve claimed this.

Yes sorry, the comments above about Assume() usage were meant for @sipa who wrote “I have been using Assume in many places assuming it’ll be optimized out entirely in production code” and similar things in the other issues.

It may be best to submit this as a separate pull request.

Yes, will do if it seems like something that is helpful.

maflcko commented at 4:45 pm on November 1, 2024: member

And I don’t think it is good for BUILD_FOR_FUZZING option to create strange libraries that seem functional but skip proof of work checks.

I don’t think this conceptual issue is addressed by your alternative SLOWCHECK suggestion. If this was a supported use-case, someone could link a fuzzing-compiled SLOWCHECK library that seems functional, but is possibly slow, or may even crash in production, when normally it should not. So your use-case would also be a reason against SLOWCHECK. I don’t think your use-case is possible to support, so I don’t think it should be used as a reason for or against a change.

davidgumberg commented at 10:31 pm on November 1, 2024: contributor

Everything in an if constexpr is still looked at by the compiler and any code issues should be detected.

Didn’t think of this, that makes sense to me. I also may have misunderstood or misrepresented the use-case/concern I tried to describe, but it’s not my own and I don’t have any view on it, so it seems silly that I even mentioned it.

I still ACK https://github.com/bitcoin/bitcoin/pull/31191/commits/fafbf8acf419d5e2ca307e5804099361ca7471af for fixing the regression measured in #31178.

ryanofsky commented at 2:27 pm on November 4, 2024: contributor

The use-case which I think this breaks and should be supported is the ability to use fuzzing in a normal build instead of a dedicated build.

As an analogy, if you want use a debugger, the best place to use it is in a dedicated debug build, but you should also be able to generate debug symbols and use debuggers with some limitations in release builds. Or if you want to check memory safety, the best way to do it may be to run MSan or valgrind in dedicated builds, but it should also be possible to use ASan in normal builds. Similarly, if you want to run fuzz tests, the best way to run them is in a dedicated build with -DBUILD_FOR_FUZZING=ON, but I think you should also be able to run fuzz tests in a normal build with other functioning executables. This flexibility lets you have a single build supporting most developer tools, even if it is not the ideal build for running every tool. I think it lowers the barrier to start trying new tools if using them only requires toggling build options not creating entirely new build configurations.

AFAICT before #31093, libraries with BUILD_FOR_FUZZING did not function normally because they skipped proof of work checks, and libraries without BUILD_FOR_FUZZING were not very useful for fuzzing because they skipped most Assume() checks. #31093 fixed both of those issues and this PR breaks them again. Maybe this is acceptable, but doesn’t seem good, and an alternate approach like the one in #31191 (comment) might be better. I’d be happy to open a PR with that approach it makes sense, or to learn that I am wrong and it doesn’t make sense, or to get feedback that it is not a good solution for other reasons.

I also think this PR is ok, but I don’t like the tradeoff it is making, so I’m just pointing out the disadvantages I see here.

marcofleon commented at 8:53 pm on November 4, 2024: contributor

I think it lowers the barrier to start trying new tools if using them only requires toggling build options not creating entirely new build configurations.

I see what you’re saying about tools being more accessible if they’re able to be used within a single build. However, I do think with cmake it’s straightforward to maintain separate builds for different purposes. And fuzz testing might be involved enough to warrant its own build.

I don’t see many advantages to fuzzing in a normal build that isn’t optimized for it. If we’re still able to verify that the fuzz binary builds properly and run pre-fuzz checks, as mentioned in #31191 (comment), then to me it feels reasonable to require BUILD_FOR_FUZZING when we intend to do some actual fuzz testing.

I also think it’s cleaner to isolate fuzz-specific code paths (like skipping the PoW check) to a dedicated build. Of course, I’m open to reviewing other potential solutions.

ryanofsky merged this on Nov 5, 2024

ryanofsky closed this on Nov 5, 2024

ryanofsky commented at 11:08 am on November 5, 2024: contributor

Went ahead and merged this since it has enough ACKs, and it sounds like the concerns I have about drawbacks of this PR don’t seem to be shared by the other reviewers. I think it would be nice if fuzzing could be turned on without turning every else off, and this approach will probably make me a little less likely to start writing fuzz tests, and prefer writing unit tests instead for convenience. But status after this PR isn’t worse than the status before #31093, and this approach can always be revisited later.

maflcko deleted the branch on Nov 5, 2024

maflcko commented at 11:44 am on November 5, 2024: member

this approach will probably make me a little less likely to start writing fuzz tests

I understand your concerns, but did you actually encounter them in reality? Generally, for writing fuzz tests, you’ll want to go with a fuzz engine (and ideally sanitizers). Otherwise, generating fuzz inputs and exploring the fuzz target reachable code will be harder. Once you link a fuzz engine, you’ll have to go with a separate fuzz build anyway.

My understanding is that the only real-world developer use-case this affects is running the fuzz target(s) on a fixed set of fuzz input(s). However, this is used rarely or even not at all recently by developers (I think), and was regularly only done in CI.

ryanofsky commented at 12:22 pm on November 5, 2024: contributor

I understand your concerns, but did you actually encounter them in reality?

Yes, I’ve never written or run a fuzz test because they require separate setup and because I’m reasonably happy writing unit tests. If I could run fuzz tests in my default build, I’d be more likely to write fuzz tests as an alternative to unit tests. I understand fuzz tests are not always an alternative to unit tests, but I’ve written a lot of unit tests where I’m just handcrafting inputs to test edge cases, and then checking invariants, and probably fuzz tests could save effort and do a better job. Not just letting developers add -fsanitize=fuzzer,address to their flags and run an extra binary seems like adding an unnecessary obstacle to starting to use the fuzzer. Maybe the obstacle has to exist for reasons I’m too uninformed to know about, but from the outside treating fuzzing compiler options differently than other compiler options seems confusing and unnecessarily complex.

sipa commented at 12:31 pm on November 5, 2024: member

@ryanofsky As long as we’re talking about libfuzzer-based fuzzing (which is what we mostly use in this project, though not exclusively), the -fsanitize=fuzzer option does two things:

It instruments the code to keep track of branch/code coverage in a libfuzzer-compatible way
It links with libfuzzer, adding the fuzzer main() function.

These pretty much inherently require a separate build: you can’t do fuzzing without (1), because the mutations that libfuzzer makes to the corpus are based on coverage information. And in the other direction, (2) is incompatible with a normal build because it adds its own main function.

Now, in theory there is -fsanitizer=fuzzer-no-link which only adds (1) but not (2). I suspect (but have not tried) that it’s possible to construct a functional bitcoind compiled this way, which wouldn’t do any fuzzing, but involves .o files which are compatible with both fuzzing and production use. Of course, they’d take a very significant performance hit due to the instrumentation added, so I don’t know how much value that has.

FWIW, for me personally, the overhead of needing a separate configure for fuzzing has reduced significantly since the introduction of cmake, as I just have a separate build directory for it.

maflcko commented at 12:43 pm on November 5, 2024: member

Yes, I’ve never written or run a fuzz test because they require separate setup

I see. I think what you are asking for could be achieved by reverting this pull request, possibly hiding the performance impact behind -DCMAKE_BUILD_TYPE=Debug (or another cmake dev-only flag). Then, you could use the fuzz executable from the “normal” build (without sanitizers and a fuzz engine) for blackbox fuzzing in AFL++ QEMU mode (or something similar). However, for me personally just going with a separate build with libFuzzer was easier than trying to set up blackbox fuzzing.

ryanofsky commented at 1:05 pm on November 5, 2024: contributor

Thanks for clarifications and ideas. Iit sounds like doing what I want would require fuzzer-no-link instead of fuzzer but otherwise it should be possible for BUILD_FOR_FUZZING to just enable extra instrumentation and not make every library and every other binary unusable. To me this just seems like a most straightforward way and least surprising way of organizing the build, that would make fuzzing easier to get started with by just being able to turn on an option that doesn’t break everything else. I can see why the current approach is working for people, though, even if it seems not very cleanly designed.

build: Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz #31191

Code Coverage & Benchmarks

Reviews

Conflicts