patch, 2 partsfuzz: 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.

> Josh Steadmon <steadmon@google.com> writes:
> 
> > 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.

> On Tue, Mar 05, 2024 at 01:11:59PM -0800, Josh Steadmon wrote:
> 
> > 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)?

> On Tue, Mar 05, 2024 at 07:50:58PM -0500, Jeff King wrote:
> 
> > On Tue, Mar 05, 2024 at 01:11:59PM -0800, Josh Steadmon wrote:
> > 
> > > 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)?
> 
> 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:
> 
> diff --git a/Makefile b/Makefile
> index f74e96d7c2..3f09d75f46 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -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)
>  
>  .PHONY: fuzz-all
>  
>  $(FUZZ_PROGRAMS): %: %.o oss-fuzz/dummy-cmd-main.o $(GITLIBS) GIT-LDFLAGS
> -	$(QUIET_LINK)$(CXX) $(FUZZ_CXXFLAGS) -o $@ $(ALL_LDFLAGS) \
> +	$(QUIET_LINK)$(FUZZ_CXX) $(FUZZ_CXXFLAGS) -o $@ $(ALL_LDFLAGS) \
>  		-Wl,--allow-multiple-definition \
>  		$(filter %.o,$^) $(filter %.a,$^) $(LIBS) $(LIB_FUZZING_ENGINE)
>  
> 
> -Peff

> 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 <gitster@pobox.com>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
>  Makefile         | 14 +++++++++++---
>  config.mak.uname |  1 +
>  2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 4e255c81f2..f74e96d7c2 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -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)'
>  
> +# 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

> Josh Steadmon <steadmon@google.com> writes:
> 
> > 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 <gitster@pobox.com>
> > Signed-off-by: Josh Steadmon <steadmon@google.com>
> > ---
> >  Makefile         | 14 +++++++++++---
> >  config.mak.uname |  1 +
> >  2 files changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 4e255c81f2..f74e96d7c2 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -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)'
> >  
> > +# 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.

> diff --git c/Makefile w/Makefile
> index 4e255c81f2..46e457a7a8 100644
> --- c/Makefile
> +++ w/Makefile
> @@ -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...

> On 2024.03.05 13:44, Junio C Hamano wrote:
> > Josh Steadmon <steadmon@google.com> writes:
> > 
> > > 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 <gitster@pobox.com>
> > > Signed-off-by: Josh Steadmon <steadmon@google.com>
> > > ---
> > >  Makefile         | 14 +++++++++++---
> > >  config.mak.uname |  1 +
> > >  2 files changed, 12 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/Makefile b/Makefile
> > > index 4e255c81f2..f74e96d7c2 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -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)'
> > >  
> > > +# 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 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.

> Josh Steadmon <steadmon@google.com> writes:
> 
> > 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.

> On Thu, Apr 11, 2024 at 11:14:24AM -0700, Josh Steadmon wrote:
> 
> > 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

> @@ -752,23 +755,6 @@ SCRIPTS = $(SCRIPT_SH_GEN) \
>  
> - ...
> -# 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...
>  EXTRA_PROGRAMS =

> +# 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)))
> +
> +# Build fuzz programs when possible, even without the necessary fuzzing support,
> +# to prevent bit-rot.
> +ifdef LINK_FUZZ_PROGRAMS
> +all:: $(FUZZ_PROGRAMS)
> +endif

> 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 <gitster@pobox.com>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
> Changes in V3:
> * Dropped CI config patch; no longer needed since we don't use CXX in
>   fuzzer build rules anymore