Re: [PATCH 2/2] fuzz: link fuzz programs with `make all` on Linux

[Josh Steadmon <steadmon@google.com>]

On 2024.04.09 14:58, Josh Steadmon wrote:
> 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 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.