[PATCH 0/2] fuzz: build fuzzers by default on Linux

[PATCH 0/2] fuzz: build fuzzers by default on Linux

[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.


Josh Steadmon (2):
  ci: also define CXX environment variable
  fuzz: link fuzz programs with `make all` on Linux

 .github/workflows/main.yml | 12 ++++++++++++
 Makefile                   | 14 +++++++++++---
 config.mak.uname           |  1 +
 3 files changed, 24 insertions(+), 3 deletions(-)


base-commit: b387623c12f3f4a376e4d35a610fd3e55d7ea907
-- 
2.44.0.278.ge034bb2e1d-goog

[PATCH 1/2] ci: also define CXX environment variable

[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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 .github/workflows/main.yml | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 683a2d633e..83945a3235 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -265,42 +265,54 @@ jobs:
         vector:
           - jobname: linux-sha256
             cc: clang
+            cxx: clang++
             pool: ubuntu-latest
           - jobname: linux-reftable
             cc: clang
+            cxx: clang++
             pool: ubuntu-latest
           - jobname: linux-gcc
             cc: gcc
+            cxx: g++
             cc_package: gcc-8
             pool: ubuntu-20.04
           - jobname: linux-TEST-vars
             cc: gcc
+            cxx: g++
             cc_package: gcc-8
             pool: ubuntu-20.04
           - jobname: osx-clang
             cc: clang
+            cxx: clang++
             pool: macos-13
           - jobname: osx-reftable
             cc: clang
+            cxx: clang++
             pool: macos-13
           - jobname: osx-gcc
             cc: gcc
+            cxx: g++
             cc_package: gcc-13
             pool: macos-13
           - jobname: linux-gcc-default
             cc: gcc
+            cxx: g++
             pool: ubuntu-latest
           - jobname: linux-leaks
             cc: gcc
+            cxx: g++
             pool: ubuntu-latest
           - jobname: linux-reftable-leaks
             cc: gcc
+            cxx: g++
             pool: ubuntu-latest
           - jobname: linux-asan-ubsan
             cc: clang
+            cxx: clang++
             pool: ubuntu-latest
     env:
       CC: ${{matrix.vector.cc}}
+      CXX: ${{matrix.vector.cxx}}
       CC_PACKAGE: ${{matrix.vector.cc_package}}
       jobname: ${{matrix.vector.jobname}}
       runs_on_pool: ${{matrix.vector.pool}}
-- 
2.44.0.278.ge034bb2e1d-goog

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

[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 <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
+
 please_set_SHELL_PATH_to_a_more_modern_shell:
 	@$$(:)
 
diff --git a/config.mak.uname b/config.mak.uname
index dacc95172d..6579c36a99 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -68,6 +68,7 @@ ifeq ($(uname_S),Linux)
 	ifneq ($(findstring .el7.,$(uname_R)),)
 		BASIC_CFLAGS += -std=c99
 	endif
+	LINK_FUZZ_PROGRAMS = YesPlease
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
 	HAVE_ALLOCA_H = YesPlease
-- 
2.44.0.278.ge034bb2e1d-goog

Re: [PATCH 1/2] ci: also define CXX environment variable

[Junio C Hamano]

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.

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

[Junio C Hamano]

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...

Re: [PATCH 1/2] ci: also define CXX environment variable

[Jeff King]

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)?

-Peff

Re: [PATCH 1/2] ci: also define CXX environment variable

[Jeff King]

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

Re: [PATCH 0/2] fuzz: build fuzzers by default on Linux

[Junio C Hamano]

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.

Re: [PATCH 1/2] ci: also define CXX environment variable

[Josh Steadmon]

On 2024.03.05 13:37, Junio C Hamano wrote:
> 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.

Reworded as suggested. Sorry for letting this series sit without
attention for so long. Will send V2 soon.

Re: [PATCH 0/2] fuzz: build fuzzers by default on Linux

[Josh Steadmon]

On 2024.03.26 14:51, Junio C Hamano wrote:
> 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.

Sorry for letting this sit for so long. I'll be addressing comments and
sending a V2 soon.

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

[Josh Steadmon]

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.


> 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...

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

[Josh Steadmon]

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.

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

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> 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.

The reasoning and the conclusion sound sensible.
Thanks.

Re: [PATCH 1/2] ci: also define CXX environment variable

[Josh Steadmon]

On 2024.03.05 20:00, Jeff King wrote:
> 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

Indeed, it does break, and this is a good fix. Thanks for the catch!

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

[Jeff King]

On Tue, Apr 09, 2024 at 02:58:28PM -0700, Josh Steadmon wrote:

> > 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.

The conditional has to be read handled while reading the Makefile, but
as a "simple" variable, OBJECTS isn't expanded until the whole Makefile
has been read. So for example this out-of-order definition works:

diff --git a/Makefile b/Makefile
index 533eaae612..5dbf1935a1 100644
--- a/Makefile
+++ b/Makefile
@@ -755,6 +755,7 @@ ETAGS_TARGET = TAGS
 # If you add a new fuzzer, please also make sure to run it in
 # ci/run-build-and-minimal-fuzzers.sh so that we make sure it still links and
 # runs in the future.
+OBJECTS += $(FUZZ_OBJS)
 FUZZ_OBJS += oss-fuzz/dummy-cmd-main.o
 FUZZ_OBJS += oss-fuzz/fuzz-commit-graph.o
 FUZZ_OBJS += oss-fuzz/fuzz-config.o
@@ -2695,7 +2696,6 @@ OBJECTS += $(SCALAR_OBJS)
 OBJECTS += $(PROGRAM_OBJS)
 OBJECTS += $(TEST_OBJS)
 OBJECTS += $(XDIFF_OBJS)
-OBJECTS += $(FUZZ_OBJS)
 OBJECTS += $(REFTABLE_OBJS) $(REFTABLE_TEST_OBJS)
 OBJECTS += $(UNIT_TEST_OBJS)
 

Now whether that is useful for organizing the Makefile, I don't know,
but I thought I'd throw it out there in case it helps you.

-Peff

[PATCH v2 0/2] fuzz: build fuzzers by default on Linux

[Josh Steadmon]

Please note: this has been rebased onto the current 'master' [436d4e5b14
(The seventeenth batch, 2024-04-10)] in order to resolve a conflict with
the recently merged bt/fuzz-config-parse series.

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.

Changes in V2:
* Rebased onto master
* Fixed compiler mismatch issue when we override CC but not CXX
* Consolidated some of the fuzzer Makefile definitions in one location


Josh Steadmon (2):
  ci: also define CXX environment variable
  fuzz: link fuzz programs with `make all` on Linux

 .github/workflows/main.yml          | 12 +++++++
 Makefile                            | 51 +++++++++++++++++------------
 ci/run-build-and-minimal-fuzzers.sh |  2 +-
 config.mak.uname                    |  1 +
 4 files changed, 44 insertions(+), 22 deletions(-)

Range-diff against v1:
1:  75f98cbf98 ! 1:  e55b691272 ci: also define CXX environment variable
    @@ Commit message
         '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.
    +    We only plan on building fuzzers on Linux with the next patch, so for
    +    now, only adjust configuration for the Linux CI jobs.
     
     
2:  eef15e3d3d < -:  ---------- fuzz: link fuzz programs with `make all` on Linux
-:  ---------- > 2:  8846a7766a fuzz: link fuzz programs with `make all` on Linux

base-commit: 436d4e5b14df49870a897f64fe92c0ddc7017e4c
-- 
2.44.0.683.g7961c838ac-goog

[PATCH v2 1/2] ci: also define CXX environment variable

[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 with the next patch, so for
now, only adjust configuration for the Linux CI jobs.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 .github/workflows/main.yml | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 3428773b09..d9986256e6 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -265,42 +265,54 @@ jobs:
         vector:
           - jobname: linux-sha256
             cc: clang
+            cxx: clang++
             pool: ubuntu-latest
           - jobname: linux-reftable
             cc: clang
+            cxx: clang++
             pool: ubuntu-latest
           - jobname: linux-gcc
             cc: gcc
+            cxx: g++
             cc_package: gcc-8
             pool: ubuntu-20.04
           - jobname: linux-TEST-vars
             cc: gcc
+            cxx: g++
             cc_package: gcc-8
             pool: ubuntu-20.04
           - jobname: osx-clang
             cc: clang
+            cxx: clang++
             pool: macos-13
           - jobname: osx-reftable
             cc: clang
+            cxx: clang++
             pool: macos-13
           - jobname: osx-gcc
             cc: gcc
+            cxx: g++
             cc_package: gcc-13
             pool: macos-13
           - jobname: linux-gcc-default
             cc: gcc
+            cxx: g++
             pool: ubuntu-latest
           - jobname: linux-leaks
             cc: gcc
+            cxx: g++
             pool: ubuntu-latest
           - jobname: linux-reftable-leaks
             cc: gcc
+            cxx: g++
             pool: ubuntu-latest
           - jobname: linux-asan-ubsan
             cc: clang
+            cxx: clang++
             pool: ubuntu-latest
     env:
       CC: ${{matrix.vector.cc}}
+      CXX: ${{matrix.vector.cxx}}
       CC_PACKAGE: ${{matrix.vector.cc_package}}
       jobname: ${{matrix.vector.jobname}}
       runs_on_pool: ${{matrix.vector.pool}}
-- 
2.44.0.683.g7961c838ac-goog

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

[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.

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>
---
 Makefile                            | 51 +++++++++++++++++------------
 ci/run-build-and-minimal-fuzzers.sh |  2 +-
 config.mak.uname                    |  1 +
 3 files changed, 32 insertions(+), 22 deletions(-)

diff --git a/Makefile b/Makefile
index c43c1bd1a0..b9e97ad3b9 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.
@@ -752,23 +755,6 @@ SCRIPTS = $(SCRIPT_SH_GEN) \
 
 ETAGS_TARGET = TAGS
 
-# If you add a new fuzzer, please also make sure to run it in
-# ci/run-build-and-minimal-fuzzers.sh so that we make sure it still links and
-# runs in the future.
-FUZZ_OBJS += oss-fuzz/dummy-cmd-main.o
-FUZZ_OBJS += oss-fuzz/fuzz-commit-graph.o
-FUZZ_OBJS += oss-fuzz/fuzz-config.o
-FUZZ_OBJS += oss-fuzz/fuzz-date.o
-FUZZ_OBJS += oss-fuzz/fuzz-pack-headers.o
-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...
 EXTRA_PROGRAMS =
 
@@ -2372,6 +2358,29 @@ ifndef NO_TCLTK
 endif
 	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'
 
+# If you add a new fuzzer, please also make sure to run it in
+# ci/run-build-and-minimal-fuzzers.sh so that we make sure it still links and
+# runs in the future.
+FUZZ_OBJS += oss-fuzz/dummy-cmd-main.o
+FUZZ_OBJS += oss-fuzz/fuzz-commit-graph.o
+FUZZ_OBJS += oss-fuzz/fuzz-config.o
+FUZZ_OBJS += oss-fuzz/fuzz-date.o
+FUZZ_OBJS += oss-fuzz/fuzz-pack-headers.o
+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)))
+
+# Build fuzz programs when possible, even without the necessary fuzzing support,
+# to prevent bit-rot.
+ifdef LINK_FUZZ_PROGRAMS
+all:: $(FUZZ_PROGRAMS)
+endif
+
 please_set_SHELL_PATH_to_a_more_modern_shell:
 	@$$(:)
 
@@ -3857,22 +3866,22 @@ 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-all: $(FUZZ_PROGRAMS)
 
 $(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)
 
-fuzz-all: $(FUZZ_PROGRAMS)
-
 $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS
 	$(call mkdir_p_parent_template)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
diff --git a/ci/run-build-and-minimal-fuzzers.sh b/ci/run-build-and-minimal-fuzzers.sh
index a51076d18d..797d65c661 100755
--- a/ci/run-build-and-minimal-fuzzers.sh
+++ b/ci/run-build-and-minimal-fuzzers.sh
@@ -7,7 +7,7 @@
 
 group "Build fuzzers" make \
 	CC=clang \
-	CXX=clang++ \
+	FUZZ_CXX=clang++ \
 	CFLAGS="-fsanitize=fuzzer-no-link,address" \
 	LIB_FUZZING_ENGINE="-fsanitize=fuzzer,address" \
 	fuzz-all
diff --git a/config.mak.uname b/config.mak.uname
index d0dcca2ec5..9107b4ae2b 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -68,6 +68,7 @@ ifeq ($(uname_S),Linux)
 	ifneq ($(findstring .el7.,$(uname_R)),)
 		BASIC_CFLAGS += -std=c99
 	endif
+	LINK_FUZZ_PROGRAMS = YesPlease
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
 	HAVE_ALLOCA_H = YesPlease
-- 
2.44.0.683.g7961c838ac-goog

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

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> @@ -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 =

As Peff said earlier, I suspect there is no need to move things for
dependencies (make rules are somewhat declarative), but grouping all
the things related to fuzzing is a good idea, so I am OK with the
new location.

> +# 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

OK.

Will queue.  Thanks.

Re: [PATCH v2 1/2] ci: also define CXX environment variable

[Jeff King]

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

[PATCH v3] fuzz: link fuzz programs with `make all` on Linux

[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.

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

Changes in V2:
* Rebased onto master
* Fixed compiler mismatch issue when we override CC but not CXX
* Consolidated some of the fuzzer Makefile definitions in one location

Range-diff against v2:
1:  e55b691272 < -:  ---------- ci: also define CXX environment variable
2:  8846a7766a = 1:  ba9d24c644 fuzz: link fuzz programs with `make all` on Linux

 Makefile                            | 51 +++++++++++++++++------------
 ci/run-build-and-minimal-fuzzers.sh |  2 +-
 config.mak.uname                    |  1 +
 3 files changed, 32 insertions(+), 22 deletions(-)

diff --git a/Makefile b/Makefile
index c43c1bd1a0..b9e97ad3b9 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.
@@ -752,23 +755,6 @@ SCRIPTS = $(SCRIPT_SH_GEN) \
 
 ETAGS_TARGET = TAGS
 
-# If you add a new fuzzer, please also make sure to run it in
-# ci/run-build-and-minimal-fuzzers.sh so that we make sure it still links and
-# runs in the future.
-FUZZ_OBJS += oss-fuzz/dummy-cmd-main.o
-FUZZ_OBJS += oss-fuzz/fuzz-commit-graph.o
-FUZZ_OBJS += oss-fuzz/fuzz-config.o
-FUZZ_OBJS += oss-fuzz/fuzz-date.o
-FUZZ_OBJS += oss-fuzz/fuzz-pack-headers.o
-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...
 EXTRA_PROGRAMS =
 
@@ -2372,6 +2358,29 @@ ifndef NO_TCLTK
 endif
 	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'
 
+# If you add a new fuzzer, please also make sure to run it in
+# ci/run-build-and-minimal-fuzzers.sh so that we make sure it still links and
+# runs in the future.
+FUZZ_OBJS += oss-fuzz/dummy-cmd-main.o
+FUZZ_OBJS += oss-fuzz/fuzz-commit-graph.o
+FUZZ_OBJS += oss-fuzz/fuzz-config.o
+FUZZ_OBJS += oss-fuzz/fuzz-date.o
+FUZZ_OBJS += oss-fuzz/fuzz-pack-headers.o
+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)))
+
+# Build fuzz programs when possible, even without the necessary fuzzing support,
+# to prevent bit-rot.
+ifdef LINK_FUZZ_PROGRAMS
+all:: $(FUZZ_PROGRAMS)
+endif
+
 please_set_SHELL_PATH_to_a_more_modern_shell:
 	@$$(:)
 
@@ -3857,22 +3866,22 @@ 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-all: $(FUZZ_PROGRAMS)
 
 $(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)
 
-fuzz-all: $(FUZZ_PROGRAMS)
-
 $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS
 	$(call mkdir_p_parent_template)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
diff --git a/ci/run-build-and-minimal-fuzzers.sh b/ci/run-build-and-minimal-fuzzers.sh
index a51076d18d..797d65c661 100755
--- a/ci/run-build-and-minimal-fuzzers.sh
+++ b/ci/run-build-and-minimal-fuzzers.sh
@@ -7,7 +7,7 @@
 
 group "Build fuzzers" make \
 	CC=clang \
-	CXX=clang++ \
+	FUZZ_CXX=clang++ \
 	CFLAGS="-fsanitize=fuzzer-no-link,address" \
 	LIB_FUZZING_ENGINE="-fsanitize=fuzzer,address" \
 	fuzz-all
diff --git a/config.mak.uname b/config.mak.uname
index d0dcca2ec5..9107b4ae2b 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -68,6 +68,7 @@ ifeq ($(uname_S),Linux)
 	ifneq ($(findstring .el7.,$(uname_R)),)
 		BASIC_CFLAGS += -std=c99
 	endif
+	LINK_FUZZ_PROGRAMS = YesPlease
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
 	HAVE_ALLOCA_H = YesPlease

base-commit: 436d4e5b14df49870a897f64fe92c0ddc7017e4c
-- 
2.44.0.769.g3c40516874-goog

Re: [PATCH v2 1/2] ci: also define CXX environment variable

[Josh Steadmon]

On 2024.04.12 00:22, Jeff King wrote:
> 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

Done in V3.

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

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> 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.

Looks good to me.  Will replace and let's mark it for 'next'.

I do not recall suggesting anything concrete on this one, though ;-)

Thanks.

> 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