fuzz: build fuzzers by default on Linux

In a future commit, we will build the fuzzer executables as part of the default 'make all' target, which requires a C++ compiler. If we do not explicitly set CXX, it defaults to g++ on GitHub CI. However, this can lead to incorrect feature detection when CC=clang, since the 'detect-compiler' script only looks at CC. Fix the issue by always setting CXX to match CC in our CI config.

We only plan on building fuzzers on Linux, so none of the other CI configs need a similar adjustment.

Quoted 9 lines · Josh Steadmon

In a future commit, we will build the fuzzer executables as part of the default 'make all' target, which requires a C++ compiler. If we do not explicitly set CXX, it defaults to g++ on GitHub CI. However, this can lead to incorrect feature detection when CC=clang, since the 'detect-compiler' script only looks at CC. Fix the issue by always setting CXX to match CC in our CI config.

We only plan on building fuzzers on Linux, so none of the other CI configs need a similar adjustment.

Sounds fair. It's not like we as the project decides to never build fuzzers on macOS and will forbid others from doing so. Those who are not part of "we" are welcome to add support to build fuzzers on other platforms. So perhaps

We only plan on building fuzzers on Linux with the next patch, so for now, only adjust configuration for the Linux CI jobs.

may convey our intention better to our future selves.

Thanks.

In a future commit, we will build the fuzzer executables as part of the default 'make all' target, which requires a C++ compiler. If we do not explicitly set CXX, it defaults to g++ on GitHub CI. However, this can lead to incorrect feature detection when CC=clang, since the 'detect-compiler' script only looks at CC. Fix the issue by always setting CXX to match CC in our CI config.

We only plan on building fuzzers on Linux, so none of the other CI configs need a similar adjustment.

Quoted 9 lines · Josh Steadmon

In a future commit, we will build the fuzzer executables as part of the default 'make all' target, which requires a C++ compiler. If we do not explicitly set CXX, it defaults to g++ on GitHub CI. However, this can lead to incorrect feature detection when CC=clang, since the 'detect-compiler' script only looks at CC. Fix the issue by always setting CXX to match CC in our CI config.

We only plan on building fuzzers on Linux, so none of the other CI configs need a similar adjustment.

Does this mean that after your patch 2, running:

make CC=clang

may have problems on Linux, because it will now try to link fuzzers using g++, even though everything else is built with clang (and ditto the detect-compiler used it)?

Quoted 12 lines · Jeff King

Quoted 9 lines · Josh Steadmon

In a future commit, we will build the fuzzer executables as part of the default 'make all' target, which requires a C++ compiler. If we do not explicitly set CXX, it defaults to g++ on GitHub CI. However, this can lead to incorrect feature detection when CC=clang, since the 'detect-compiler' script only looks at CC. Fix the issue by always setting CXX to match CC in our CI config.

We only plan on building fuzzers on Linux, so none of the other CI configs need a similar adjustment.

Quoted 7 lines

Does this mean that after your patch 2, running:

make CC=clang

may have problems on Linux, because it will now try to link fuzzers using g++, even though everything else is built with clang (and ditto the detect-compiler used it)?

Also, if the answer is "yes": do we really need a c++ linker here? My understanding from reading "git log -SCXX Makefile" is that when using oss-fuzz, you'd sometimes want to pass c++ specific things in FUZZ_CXXFLAGS. But we're not using that here, and are just making sure that things can be linked. Can we just use $(CC) by default here, then?

Something like:

Makefile +2 -1

index f74e96d7c2..3f09d75f46 100644
@@ -3861,17 +3861,18 @@ cover_db_html: cover_db
 #
 # An example command to build against libFuzzer from LLVM 11.0.0:
 #
-# make CC=clang CXX=clang++ \
+# make CC=clang FUZZ_CXX=clang++ \
 #      CFLAGS="-fsanitize=fuzzer-no-link,address" \
 #      LIB_FUZZING_ENGINE="-fsanitize=fuzzer,address" \
 #      fuzz-all
 #
+FUZZ_CXX ?= $(CC)
 FUZZ_CXXFLAGS ?= $(ALL_CFLAGS)
 

Since 5e47215080 (fuzz: add basic fuzz testing target., 2018-10-12), we have compiled object files for the fuzz tests as part of the default 'make all' target. This helps prevent bit-rot in lesser-used parts of the codebase, by making sure that incompatible changes are caught at build time.

However, since we never linked the fuzzer executables, this did not protect us from link-time errors. As of 8b9a42bf48 (fuzz: fix fuzz test build rules, 2024-01-19), it's now possible to link the fuzzer executables without using a fuzzing engine and a variety of compiler-specific (and compiler-version-specific) flags, at least on Linux. So let's add a platform-specific option in config.mak.uname to link the executables as part of the default make all target.

Suggested-by Junio C Hamano <[email protected]>

Signed-off-by Josh Steadmon <[email protected]>

---
 Makefile         | 14 +++++++++++---
 config.mak.uname |  1 +
 2 files changed, 12 insertions(+), 3 deletions(-)

Makefile +3 -3

index 4e255c81f2..f74e96d7c2 100644
@@ -409,6 +409,9 @@ include shared.mak
 # to the "<name>" of the corresponding `compat/fsmonitor/fsm-settings-<name>.c`
 # that implements the `fsm_os_settings__*()` routines.
 #
+# Define LINK_FUZZ_PROGRAMS if you want `make all` to also build the fuzz test
+# programs in oss-fuzz/.
+#
 # === Optional library: libintl ===
 #
 # Define NO_GETTEXT if you don't want Git output to be translated.

@@ -763,9 +766,6 @@ FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o
 .PHONY: fuzz-objs
 fuzz-objs: $(FUZZ_OBJS)
 
-# Always build fuzz objects even if not testing, to prevent bit-rot.
-all:: $(FUZZ_OBJS)
-
 FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS)))
 
 # Empty...

Quoted 49 lines · Josh Steadmon

Since 5e47215080 (fuzz: add basic fuzz testing target., 2018-10-12), we have compiled object files for the fuzz tests as part of the default 'make all' target. This helps prevent bit-rot in lesser-used parts of the codebase, by making sure that incompatible changes are caught at build time.

However, since we never linked the fuzzer executables, this did not protect us from link-time errors. As of 8b9a42bf48 (fuzz: fix fuzz test build rules, 2024-01-19), it's now possible to link the fuzzer executables without using a fuzzing engine and a variety of compiler-specific (and compiler-version-specific) flags, at least on Linux. So let's add a platform-specific option in config.mak.uname to link the executables as part of the default make all target.

Suggested-by Junio C Hamano <[email protected]>

Signed-off-by Josh Steadmon <[email protected]>

---
 Makefile         | 14 +++++++++++---
 config.mak.uname |  1 +
 2 files changed, 12 insertions(+), 3 deletions(-)

Makefile +3 -3

index 4e255c81f2..f74e96d7c2 100644
@@ -409,6 +409,9 @@ include shared.mak
 # to the "<name>" of the corresponding `compat/fsmonitor/fsm-settings-<name>.c`
 # that implements the `fsm_os_settings__*()` routines.
 #
+# Define LINK_FUZZ_PROGRAMS if you want `make all` to also build the fuzz test
+# programs in oss-fuzz/.
+#
 # === Optional library: libintl ===
 #
 # Define NO_GETTEXT if you don't want Git output to be translated.

@@ -763,9 +766,6 @@ FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o
 .PHONY: fuzz-objs
 fuzz-objs: $(FUZZ_OBJS)
 
-# Always build fuzz objects even if not testing, to prevent bit-rot.
-all:: $(FUZZ_OBJS)
-
 FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS)))
 
 # Empty...

@@ -2368,6 +2368,14 @@ ifndef NO_TCLTK
 endif
 	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'
 

Makefile ifndef NO_TCLTK · line 2377

Quoted 7 lines

Makefile +7 -0

+# Build fuzz programs if possible, or at least compile the object files; even
+# without the necessary fuzzing support, this prevents bit-rot.
+ifdef LINK_FUZZ_PROGRAMS
+all:: $(FUZZ_PROGRAMS)
+else
+all:: $(FUZZ_OBJS)
+endif

It would have been easier on the eyes if we had the fuzz things together, perhaps like this simplified version? We build FUZZ_OBJS either way, and when the LINK_FUZZ_PROGRAMS is requested, we follow the fuzz-all recipe, too.

Makefile +9 -0

index 4e255c81f2..46e457a7a8 100644
@@ -409,6 +409,9 @@ include shared.mak
 # to the "<name>" of the corresponding `compat/fsmonitor/fsm-settings-<name>.c`
 # that implements the `fsm_os_settings__*()` routines.
 #
+# Define LINK_FUZZ_PROGRAMS if you want `make all` to also build the fuzz test
+# programs in oss-fuzz/.
+#
 # === Optional library: libintl ===
 #
 # Define NO_GETTEXT if you don't want Git output to be translated.

@@ -766,6 +769,12 @@ fuzz-objs: $(FUZZ_OBJS)
 # Always build fuzz objects even if not testing, to prevent bit-rot.
 all:: $(FUZZ_OBJS)
 
+# Build fuzz programs, even without the necessary fuzzing support,
+# this prevents bit-rot.
+ifdef LINK_FUZZ_PROGRAMS
+all:: fuzz-all
+endif
+
 FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS)))
 
 # Empty...

Quoted 51 lines · Junio C Hamano

Quoted 49 lines · Josh Steadmon

Since 5e47215080 (fuzz: add basic fuzz testing target., 2018-10-12), we have compiled object files for the fuzz tests as part of the default 'make all' target. This helps prevent bit-rot in lesser-used parts of the codebase, by making sure that incompatible changes are caught at build time.

However, since we never linked the fuzzer executables, this did not protect us from link-time errors. As of 8b9a42bf48 (fuzz: fix fuzz test build rules, 2024-01-19), it's now possible to link the fuzzer executables without using a fuzzing engine and a variety of compiler-specific (and compiler-version-specific) flags, at least on Linux. So let's add a platform-specific option in config.mak.uname to link the executables as part of the default make all target.

Suggested-by Junio C Hamano <[email protected]>

Signed-off-by Josh Steadmon <[email protected]>

---
 Makefile         | 14 +++++++++++---
 config.mak.uname |  1 +
 2 files changed, 12 insertions(+), 3 deletions(-)

Makefile +3 -3

index 4e255c81f2..f74e96d7c2 100644
@@ -409,6 +409,9 @@ include shared.mak
 # to the "<name>" of the corresponding `compat/fsmonitor/fsm-settings-<name>.c`
 # that implements the `fsm_os_settings__*()` routines.
 #
+# Define LINK_FUZZ_PROGRAMS if you want `make all` to also build the fuzz test
+# programs in oss-fuzz/.
+#
 # === Optional library: libintl ===
 #
 # Define NO_GETTEXT if you don't want Git output to be translated.

@@ -763,9 +766,6 @@ FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o
 .PHONY: fuzz-objs
 fuzz-objs: $(FUZZ_OBJS)
 
-# Always build fuzz objects even if not testing, to prevent bit-rot.
-all:: $(FUZZ_OBJS)
-
 FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS)))
 
 # Empty...

@@ -2368,6 +2368,14 @@ ifndef NO_TCLTK
 endif
 	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'
 

Quoted 12 lines

Makefile ifndef NO_TCLTK · line 2377

Quoted 7 lines

Makefile +7 -0

+# Build fuzz programs if possible, or at least compile the object files; even
+# without the necessary fuzzing support, this prevents bit-rot.
+ifdef LINK_FUZZ_PROGRAMS
+all:: $(FUZZ_PROGRAMS)
+else
+all:: $(FUZZ_OBJS)
+endif

It would have been easier on the eyes if we had the fuzz things together, perhaps like this simplified version? We build FUZZ_OBJS either way, and when the LINK_FUZZ_PROGRAMS is requested, we follow the fuzz-all recipe, too.

We need the LINK_FUZZ_PROGRAMS conditional to happen after we import config.mak.uname (line 1434 in my V1). We also need to define FUZZ_OBJS prior to adding it to OBJECTS (line 2698 in V1). I can move all of the fuzz-definition within that range, keeping everything in one place at the cost of a larger diff. I'll do that for V2, but if you prefer otherwise please let me know.

Although I'm not 100% sure that we even need to add FUZZ_OBJS to OBJECTS, so let me check that tomorrow. If not, then I can move everything to the bottom of the Makefile where we also define fuzz-all and the build rules for FUZZ_PROGRAMS.

It turns out we do need FUZZ_OBJS in OBJECTS so that we define a build rule, otherwise the Makefile doesn't know how to compile the fuzzer objects. So V2 will have most of the fuzzer rules in the line (1434,2698) range.

Quoted 4 lines

It would have been easier on the eyes if we had the fuzz things together, perhaps like this simplified version? We build FUZZ_OBJS either way, and when the LINK_FUZZ_PROGRAMS is requested, we follow the fuzz-all recipe, too.

We need the LINK_FUZZ_PROGRAMS conditional to happen after we import config.mak.uname (line 1434 in my V1). We also need to define FUZZ_OBJS prior to adding it to OBJECTS (line 2698 in V1). I can move all of the fuzz-definition within that range, keeping everything in one place at the cost of a larger diff. I'll do that for V2, but if you prefer otherwise please let me know.

Although I'm not 100% sure that we even need to add FUZZ_OBJS to OBJECTS, so let me check that tomorrow. If not, then I can move everything to the bottom of the Makefile where we also define fuzz-all and the build rules for FUZZ_PROGRAMS.

Increase our protection against fuzzer bit-rot by making sure we can link the fuzz test executables on Linux. Patch 1 is a small CI config improvement to fix compiler feature detection. Patch 2 is the Makefile / config.mak.uname change to add the executables to make all on Linux.

Quoted 4 lines · Josh Steadmon

Increase our protection against fuzzer bit-rot by making sure we can link the fuzz test executables on Linux. Patch 1 is a small CI config improvement to fix compiler feature detection. Patch 2 is the Makefile / config.mak.uname change to add the executables to make all on Linux.

This has seen a handful of review comments but they haven't been responded nor resulted in a new round. Can we wrap this up anytime soon?

We would expect a review comment to be at least responded to either rebut or admit the issues raised. It may be that a reviewer's point were missing the mark and the patches themselves were perfectly fine.

But all other cases, even when the reviewer's comment were missing the mark, such a confusion may have been the result of the patch text or the proposed log message being unclear. Of course, the review comments may have been pointing out an actionable issue. They would hopefully lead to an improved version of the patches posted sometime later, so that we can conclude a topic and move ahead.

Thanks.

In a future commit, we will build the fuzzer executables as part of the default 'make all' target, which requires a C++ compiler. If we do not explicitly set CXX, it defaults to g++ on GitHub CI. However, this can lead to incorrect feature detection when CC=clang, since the 'detect-compiler' script only looks at CC. Fix the issue by always setting CXX to match CC in our CI config.

Quoted 6 lines · Josh Steadmon

In a future commit, we will build the fuzzer executables as part of the default 'make all' target, which requires a C++ compiler. If we do not explicitly set CXX, it defaults to g++ on GitHub CI. However, this can lead to incorrect feature detection when CC=clang, since the 'detect-compiler' script only looks at CC. Fix the issue by always setting CXX to match CC in our CI config.

Since you took my suggestion in patch 2, this "which requires a C++ compiler" is no longer true, is it? And I don't think we'd even look at the CXX variable at all, since it's now FUZZ_CXX.

So this patch can just be dropped, I'd think.

-Peff

Since linking the fuzzer executables without a fuzzing engine does not require a C++ compiler, we can change the FUZZ_PROGRAMS build rule to use $(CC) by default. This avoids compiler mis-match issues when overriding $(CC) but not $(CXX). When we *do* want to actually link with a fuzzing engine, we can set $(FUZZ_CXX). The build instructions in the CI fuzz-smoke-test job and in the Makefile comment have been updated accordingly.

While we're at it, we can consolidate some of the fuzzer build instructions into one location in the Makefile.

Suggested-by Junio C Hamano <[email protected]>

Helped-by Jeff King <[email protected]>

Signed-off-by Josh Steadmon <[email protected]>

---
Changes in V3:
* Dropped CI config patch; no longer needed since we don't use CXX in fuzzer build rules anymore