{"thread":{"id":"61056","subject":"[PATCH 0/2] fuzz: build fuzzers by default on Linux","startedAt":"2024-03-05T21:12:03Z","lastAt":"2024-04-24T19:07:40Z","messageCount":23,"participants":["Josh Steadmon","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"489990","messageId":"cover.1709673020.git.steadmon@google.com","threadId":"61056","inReplyTo":null,"subject":"[PATCH 0/2] fuzz: build fuzzers by default on Linux","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-03-05T21:11:58Z","receivedAt":"2024-03-05T21:12:03Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Increase our protection against fuzzer bit-rot by making sure we can\nlink the fuzz test executables on Linux. Patch 1 is a small CI config\nimprovement to fix compiler feature detection. Patch 2 is the Makefile /\nconfig.mak.uname change to add the executables to `make all` on Linux.\n\n\nJosh Steadmon (2):\n ci: also define CXX environment variable\n fuzz: link fuzz programs with `make all` on Linux\n\n .github/workflows/main.yml | 12 ++++++++++++\n Makefile | 14 +++++++++++---\n config.mak.uname | 1 +\n 3 files changed, 24 insertions(+), 3 deletions(-)\n\n\nbase-commit: b387623c12f3f4a376e4d35a610fd3e55d7ea907\n-- \n2.44.0.278.ge034bb2e1d-goog\n\n"},{"id":"489991","messageId":"75f98cbf98005b0a069977096ec5501f2f7830fe.1709673020.git.steadmon@google.com","threadId":"61056","inReplyTo":"cover.1709673020.git.steadmon@google.com","subject":"[PATCH 1/2] ci: also define CXX environment variable","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-03-05T21:11:59Z","receivedAt":"2024-03-05T21:12:05Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"In a future commit, we will build the fuzzer executables as part of the\ndefault 'make all' target, which requires a C++ compiler. If we do not\nexplicitly set CXX, it defaults to g++ on GitHub CI. However, this can\nlead to incorrect feature detection when CC=clang, since the\n'detect-compiler' script only looks at CC. Fix the issue by always\nsetting CXX to match CC in our CI config.\n\nWe only plan on building fuzzers on Linux, so none of the other CI\nconfigs need a similar adjustment.\n\nSigned-off-by: Josh Steadmon \n---\n .github/workflows/main.yml | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\nindex 683a2d633e..83945a3235 100644\n--- a/.github/workflows/main.yml\n+++ b/.github/workflows/main.yml\n@@ -265,42 +265,54 @@ jobs:\n vector:\n - jobname: linux-sha256\n cc: clang\n+ cxx: clang++\n pool: ubuntu-latest\n - jobname: linux-reftable\n cc: clang\n+ cxx: clang++\n pool: ubuntu-latest\n - jobname: linux-gcc\n cc: gcc\n+ cxx: g++\n cc_package: gcc-8\n pool: ubuntu-20.04\n - jobname: linux-TEST-vars\n cc: gcc\n+ cxx: g++\n cc_package: gcc-8\n pool: ubuntu-20.04\n - jobname: osx-clang\n cc: clang\n+ cxx: clang++\n pool: macos-13\n - jobname: osx-reftable\n cc: clang\n+ cxx: clang++\n pool: macos-13\n - jobname: osx-gcc\n cc: gcc\n+ cxx: g++\n cc_package: gcc-13\n pool: macos-13\n - jobname: linux-gcc-default\n cc: gcc\n+ cxx: g++\n pool: ubuntu-latest\n - jobname: linux-leaks\n cc: gcc\n+ cxx: g++\n pool: ubuntu-latest\n - jobname: linux-reftable-leaks\n cc: gcc\n+ cxx: g++\n pool: ubuntu-latest\n - jobname: linux-asan-ubsan\n cc: clang\n+ cxx: clang++\n pool: ubuntu-latest\n env:\n CC: ${{matrix.vector.cc}}\n+ CXX: ${{matrix.vector.cxx}}\n CC_PACKAGE: ${{matrix.vector.cc_package}}\n jobname: ${{matrix.vector.jobname}}\n runs_on_pool: ${{matrix.vector.pool}}\n-- \n2.44.0.278.ge034bb2e1d-goog\n\n"},{"id":"489992","messageId":"eef15e3d3da3ca6953fa8bf3ade190da8e68bf46.1709673020.git.steadmon@google.com","threadId":"61056","inReplyTo":"cover.1709673020.git.steadmon@google.com","subject":"[PATCH 2/2] fuzz: link fuzz programs with `make all` on Linux","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-03-05T21:12:00Z","receivedAt":"2024-03-05T21:12:07Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Since 5e47215080 (fuzz: add basic fuzz testing target., 2018-10-12), we\nhave compiled object files for the fuzz tests as part of the default\n'make all' target. This helps prevent bit-rot in lesser-used parts of\nthe codebase, by making sure that incompatible changes are caught at\nbuild time.\n\nHowever, since we never linked the fuzzer executables, this did not\nprotect us from link-time errors. As of 8b9a42bf48 (fuzz: fix fuzz test\nbuild rules, 2024-01-19), it's now possible to link the fuzzer\nexecutables without using a fuzzing engine and a variety of\ncompiler-specific (and compiler-version-specific) flags, at least on\nLinux. So let's add a platform-specific option in config.mak.uname to\nlink the executables as part of the default `make all` target.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Josh Steadmon \n---\n Makefile | 14 +++++++++++---\n config.mak.uname | 1 +\n 2 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 4e255c81f2..f74e96d7c2 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -409,6 +409,9 @@ include shared.mak\n # to the \"\" of the corresponding `compat/fsmonitor/fsm-settings-.c`\n # that implements the `fsm_os_settings__*()` routines.\n #\n+# Define LINK_FUZZ_PROGRAMS if you want `make all` to also build the fuzz test\n+# programs in oss-fuzz/.\n+#\n # === Optional library: libintl ===\n #\n # Define NO_GETTEXT if you don't want Git output to be translated.\n@@ -763,9 +766,6 @@ FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o\n .PHONY: fuzz-objs\n fuzz-objs: $(FUZZ_OBJS)\n \n-# Always build fuzz objects even if not testing, to prevent bit-rot.\n-all:: $(FUZZ_OBJS)\n-\n FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS)))\n \n # Empty...\n@@ -2368,6 +2368,14 @@ ifndef NO_TCLTK\n endif\n \t$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'\n \n+# Build fuzz programs if possible, or at least compile the object files; even\n+# without the necessary fuzzing support, this prevents bit-rot.\n+ifdef LINK_FUZZ_PROGRAMS\n+all:: $(FUZZ_PROGRAMS)\n+else\n+all:: $(FUZZ_OBJS)\n+endif\n+\n please_set_SHELL_PATH_to_a_more_modern_shell:\n \t@$$(:)\n \ndiff --git a/config.mak.uname b/config.mak.uname\nindex dacc95172d..6579c36a99 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -68,6 +68,7 @@ ifeq ($(uname_S),Linux)\n \tifneq ($(findstring .el7.,$(uname_R)),)\n \t\tBASIC_CFLAGS += -std=c99\n \tendif\n+\tLINK_FUZZ_PROGRAMS = YesPlease\n endif\n ifeq ($(uname_S),GNU/kFreeBSD)\n \tHAVE_ALLOCA_H = YesPlease\n-- \n2.44.0.278.ge034bb2e1d-goog\n\n"},{"id":"490018","messageId":"xmqqv860z7fv.fsf@gitster.g","threadId":"61056","inReplyTo":"75f98cbf98005b0a069977096ec5501f2f7830fe.1709673020.git.steadmon@google.com","subject":"Re: [PATCH 1/2] ci: also define CXX environment variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-05T21:37:24Z","receivedAt":"2024-03-05T21:37:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> In a future commit, we will build the fuzzer executables as part of the\n> default 'make all' target, which requires a C++ compiler. If we do not\n> explicitly set CXX, it defaults to g++ on GitHub CI. However, this can\n> lead to incorrect feature detection when CC=clang, since the\n> 'detect-compiler' script only looks at CC. Fix the issue by always\n> setting CXX to match CC in our CI config.\n>\n> We only plan on building fuzzers on Linux, so none of the other CI\n> configs need a similar adjustment.\n\nSounds fair. It's not like we as the project decides to never build\nfuzzers on macOS and will forbid others from doing so. Those who\nare not part of \"we\" are welcome to add support to build fuzzers on\nother platforms. So perhaps\n\n We only plan on building fuzzers on Linux with the next patch,\n so for now, only adjust configuration for the Linux CI jobs.\n\nmay convey our intention better to our future selves.\n\nThanks.\n"},{"id":"490021","messageId":"xmqqplw8z73y.fsf@gitster.g","threadId":"61056","inReplyTo":"eef15e3d3da3ca6953fa8bf3ade190da8e68bf46.1709673020.git.steadmon@google.com","subject":"Re: [PATCH 2/2] fuzz: link fuzz programs with `make all` on Linux","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-05T21:44:33Z","receivedAt":"2024-03-05T21:44:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> Since 5e47215080 (fuzz: add basic fuzz testing target., 2018-10-12), we\n> have compiled object files for the fuzz tests as part of the default\n> 'make all' target. This helps prevent bit-rot in lesser-used parts of\n> the codebase, by making sure that incompatible changes are caught at\n> build time.\n>\n> However, since we never linked the fuzzer executables, this did not\n> protect us from link-time errors. As of 8b9a42bf48 (fuzz: fix fuzz test\n> build rules, 2024-01-19), it's now possible to link the fuzzer\n> executables without using a fuzzing engine and a variety of\n> compiler-specific (and compiler-version-specific) flags, at least on\n> Linux. So let's add a platform-specific option in config.mak.uname to\n> link the executables as part of the default `make all` target.\n>\n> Suggested-by: Junio C Hamano \n> Signed-off-by: Josh Steadmon \n> ---\n> Makefile | 14 +++++++++++---\n> config.mak.uname | 1 +\n> 2 files changed, 12 insertions(+), 3 deletions(-)\n>\n> diff --git a/Makefile b/Makefile\n> index 4e255c81f2..f74e96d7c2 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -409,6 +409,9 @@ include shared.mak\n> # to the \"\" of the corresponding `compat/fsmonitor/fsm-settings-.c`\n> # that implements the `fsm_os_settings__*()` routines.\n> #\n> +# Define LINK_FUZZ_PROGRAMS if you want `make all` to also build the fuzz test\n> +# programs in oss-fuzz/.\n> +#\n> # === Optional library: libintl ===\n> #\n> # Define NO_GETTEXT if you don't want Git output to be translated.\n> @@ -763,9 +766,6 @@ FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o\n> .PHONY: fuzz-objs\n> fuzz-objs: $(FUZZ_OBJS)\n> \n> -# Always build fuzz objects even if not testing, to prevent bit-rot.\n> -all:: $(FUZZ_OBJS)\n> -\n> FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS)))\n> \n> # Empty...\n> @@ -2368,6 +2368,14 @@ ifndef NO_TCLTK\n> endif\n> \t$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'\n> \n> +# Build fuzz programs if possible, or at least compile the object files; even\n> +# without the necessary fuzzing support, this prevents bit-rot.\n> +ifdef LINK_FUZZ_PROGRAMS\n> +all:: $(FUZZ_PROGRAMS)\n> +else\n> +all:: $(FUZZ_OBJS)\n> +endif\n\nIt would have been easier on the eyes if we had the fuzz things\ntogether, perhaps like this simplified version? We build FUZZ_OBJS\neither way, and when the LINK_FUZZ_PROGRAMS is requested, we follow\nthe fuzz-all recipe, too.\n\ndiff --git c/Makefile w/Makefile\nindex 4e255c81f2..46e457a7a8 100644\n--- c/Makefile\n+++ w/Makefile\n@@ -409,6 +409,9 @@ include shared.mak\n # to the \"\" of the corresponding `compat/fsmonitor/fsm-settings-.c`\n # that implements the `fsm_os_settings__*()` routines.\n #\n+# Define LINK_FUZZ_PROGRAMS if you want `make all` to also build the fuzz test\n+# programs in oss-fuzz/.\n+#\n # === Optional library: libintl ===\n #\n # Define NO_GETTEXT if you don't want Git output to be translated.\n@@ -766,6 +769,12 @@ fuzz-objs: $(FUZZ_OBJS)\n # Always build fuzz objects even if not testing, to prevent bit-rot.\n all:: $(FUZZ_OBJS)\n \n+# Build fuzz programs, even without the necessary fuzzing support,\n+# this prevents bit-rot.\n+ifdef LINK_FUZZ_PROGRAMS\n+all:: fuzz-all\n+endif\n+\n FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS)))\n \n # Empty...\n"},{"id":"490046","messageId":"20240306005057.GC3797463@coredump.intra.peff.net","threadId":"61056","inReplyTo":"75f98cbf98005b0a069977096ec5501f2f7830fe.1709673020.git.steadmon@google.com","subject":"Re: [PATCH 1/2] ci: also define CXX environment variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-06T00:50:57Z","receivedAt":"2024-03-06T00:50:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 05, 2024 at 01:11:59PM -0800, Josh Steadmon wrote:\n\n> In a future commit, we will build the fuzzer executables as part of the\n> default 'make all' target, which requires a C++ compiler. If we do not\n> explicitly set CXX, it defaults to g++ on GitHub CI. However, this can\n> lead to incorrect feature detection when CC=clang, since the\n> 'detect-compiler' script only looks at CC. Fix the issue by always\n> setting CXX to match CC in our CI config.\n> \n> We only plan on building fuzzers on Linux, so none of the other CI\n> configs need a similar adjustment.\n\nDoes this mean that after your patch 2, running:\n\n make CC=clang\n\nmay have problems on Linux, because it will now try to link fuzzers\nusing g++, even though everything else is built with clang (and ditto\nthe detect-compiler used it)?\n\n-Peff\n"},{"id":"490050","messageId":"20240306010016.GA3811328@coredump.intra.peff.net","threadId":"61056","inReplyTo":"20240306005057.GC3797463@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] ci: also define CXX environment variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-06T01:00:16Z","receivedAt":"2024-03-06T01:00:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 05, 2024 at 07:50:58PM -0500, Jeff King wrote:\n\n> On Tue, Mar 05, 2024 at 01:11:59PM -0800, Josh Steadmon wrote:\n> \n> > In a future commit, we will build the fuzzer executables as part of the\n> > default 'make all' target, which requires a C++ compiler. If we do not\n> > explicitly set CXX, it defaults to g++ on GitHub CI. However, this can\n> > lead to incorrect feature detection when CC=clang, since the\n> > 'detect-compiler' script only looks at CC. Fix the issue by always\n> > setting CXX to match CC in our CI config.\n> > \n> > We only plan on building fuzzers on Linux, so none of the other CI\n> > configs need a similar adjustment.\n> \n> Does this mean that after your patch 2, running:\n> \n> make CC=clang\n> \n> may have problems on Linux, because it will now try to link fuzzers\n> using g++, even though everything else is built with clang (and ditto\n> the detect-compiler used it)?\n\nAlso, if the answer is \"yes\": do we really need a c++ linker here? My\nunderstanding from reading \"git log -SCXX Makefile\" is that when using\noss-fuzz, you'd sometimes want to pass c++ specific things in\nFUZZ_CXXFLAGS. But we're not using that here, and are just making sure\nthat things can be linked. Can we just use $(CC) by default here, then?\n\nSomething like:\n\ndiff --git a/Makefile b/Makefile\nindex f74e96d7c2..3f09d75f46 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3861,17 +3861,18 @@ cover_db_html: cover_db\n #\n # An example command to build against libFuzzer from LLVM 11.0.0:\n #\n-# make CC=clang CXX=clang++ \\\n+# make CC=clang FUZZ_CXX=clang++ \\\n # CFLAGS=\"-fsanitize=fuzzer-no-link,address\" \\\n # LIB_FUZZING_ENGINE=\"-fsanitize=fuzzer,address\" \\\n # fuzz-all\n #\n+FUZZ_CXX ?= $(CC)\n FUZZ_CXXFLAGS ?= $(ALL_CFLAGS)\n \n .PHONY: fuzz-all\n \n $(FUZZ_PROGRAMS): %: %.o oss-fuzz/dummy-cmd-main.o $(GITLIBS) GIT-LDFLAGS\n-\t$(QUIET_LINK)$(CXX) $(FUZZ_CXXFLAGS) -o $@ $(ALL_LDFLAGS) \\\n+\t$(QUIET_LINK)$(FUZZ_CXX) $(FUZZ_CXXFLAGS) -o $@ $(ALL_LDFLAGS) \\\n \t\t-Wl,--allow-multiple-definition \\\n \t\t$(filter %.o,$^) $(filter %.a,$^) $(LIBS) $(LIB_FUZZING_ENGINE)\n \n\n-Peff\n"},{"id":"491611","messageId":"xmqq1q7w8xx6.fsf@gitster.g","threadId":"61056","inReplyTo":"cover.1709673020.git.steadmon@google.com","subject":"Re: [PATCH 0/2] fuzz: build fuzzers by default on Linux","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-26T21:51:01Z","receivedAt":"2024-03-26T21:51:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> Increase our protection against fuzzer bit-rot by making sure we can\n> link the fuzz test executables on Linux. Patch 1 is a small CI config\n> improvement to fix compiler feature detection. Patch 2 is the Makefile /\n> config.mak.uname change to add the executables to `make all` on Linux.\n\nThis has seen a handful of review comments but they haven't been\nresponded nor resulted in a new round. Can we wrap this up anytime\nsoon?\n\nWe would expect a review comment to be at least responded to either\nrebut or admit the issues raised. It may be that a reviewer's point\nwere missing the mark and the patches themselves were perfectly\nfine.\n\nBut all other cases, even when the reviewer's comment were missing\nthe mark, such a confusion may have been the result of the patch\ntext or the proposed log message being unclear. Of course, the\nreview comments may have been pointing out an actionable issue.\nThey would hopefully lead to an improved version of the patches\nposted sometime later, so that we can conclude a topic and move\nahead.\n\nThanks.\n\n"},{"id":"492651","messageId":"ZhW0BOYHyPkZbgbd@google.com","threadId":"61056","inReplyTo":"xmqqv860z7fv.fsf@gitster.g","subject":"Re: [PATCH 1/2] ci: also define CXX environment variable","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-04-09T21:32:52Z","receivedAt":"2024-04-09T21:32:58Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.03.05 13:37, Junio C Hamano wrote:\n> Josh Steadmon writes:\n> \n> > In a future commit, we will build the fuzzer executables as part of the\n> > default 'make all' target, which requires a C++ compiler. If we do not\n> > explicitly set CXX, it defaults to g++ on GitHub CI. However, this can\n> > lead to incorrect feature detection when CC=clang, since the\n> > 'detect-compiler' script only looks at CC. Fix the issue by always\n> > setting CXX to match CC in our CI config.\n> >\n> > We only plan on building fuzzers on Linux, so none of the other CI\n> > configs need a similar adjustment.\n> \n> Sounds fair. It's not like we as the project decides to never build\n> fuzzers on macOS and will forbid others from doing so. Those who\n> are not part of \"we\" are welcome to add support to build fuzzers on\n> other platforms. So perhaps\n> \n> We only plan on building fuzzers on Linux with the next patch,\n> so for now, only adjust configuration for the Linux CI jobs.\n> \n> may convey our intention better to our future selves.\n> \n> Thanks.\n\nReworded as suggested. Sorry for letting this series sit without\nattention for so long. Will send V2 soon.\n"},{"id":"492652","messageId":"ZhW0bm3gAxuvMnzi@google.com","threadId":"61056","inReplyTo":"xmqq1q7w8xx6.fsf@gitster.g","subject":"Re: [PATCH 0/2] fuzz: build fuzzers by default on Linux","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-04-09T21:34:38Z","receivedAt":"2024-04-09T21:34:44Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.03.26 14:51, Junio C Hamano wrote:\n> Josh Steadmon writes:\n> \n> > Increase our protection against fuzzer bit-rot by making sure we can\n> > link the fuzz test executables on Linux. Patch 1 is a small CI config\n> > improvement to fix compiler feature detection. Patch 2 is the Makefile /\n> > config.mak.uname change to add the executables to `make all` on Linux.\n> \n> This has seen a handful of review comments but they haven't been\n> responded nor resulted in a new round. Can we wrap this up anytime\n> soon?\n> \n> We would expect a review comment to be at least responded to either\n> rebut or admit the issues raised. It may be that a reviewer's point\n> were missing the mark and the patches themselves were perfectly\n> fine.\n> \n> But all other cases, even when the reviewer's comment were missing\n> the mark, such a confusion may have been the result of the patch\n> text or the proposed log message being unclear. Of course, the\n> review comments may have been pointing out an actionable issue.\n> They would hopefully lead to an improved version of the patches\n> posted sometime later, so that we can conclude a topic and move\n> ahead.\n> \n> Thanks.\n\nSorry for letting this sit for so long. I'll be addressing comments and\nsending a V2 soon.\n"},{"id":"492662","messageId":"ZhW6BM9V-Rto_CW4@google.com","threadId":"61056","inReplyTo":"xmqqplw8z73y.fsf@gitster.g","subject":"Re: [PATCH 2/2] fuzz: link fuzz programs with `make all` on Linux","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-04-09T21:58:28Z","receivedAt":"2024-04-09T21:58:34Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.03.05 13:44, Junio C Hamano wrote:\n> Josh Steadmon writes:\n> \n> > Since 5e47215080 (fuzz: add basic fuzz testing target., 2018-10-12), we\n> > have compiled object files for the fuzz tests as part of the default\n> > 'make all' target. This helps prevent bit-rot in lesser-used parts of\n> > the codebase, by making sure that incompatible changes are caught at\n> > build time.\n> >\n> > However, since we never linked the fuzzer executables, this did not\n> > protect us from link-time errors. As of 8b9a42bf48 (fuzz: fix fuzz test\n> > build rules, 2024-01-19), it's now possible to link the fuzzer\n> > executables without using a fuzzing engine and a variety of\n> > compiler-specific (and compiler-version-specific) flags, at least on\n> > Linux. So let's add a platform-specific option in config.mak.uname to\n> > link the executables as part of the default `make all` target.\n> >\n> > Suggested-by: Junio C Hamano \n> > Signed-off-by: Josh Steadmon \n> > ---\n> > Makefile | 14 +++++++++++---\n> > config.mak.uname | 1 +\n> > 2 files changed, 12 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/Makefile b/Makefile\n> > index 4e255c81f2..f74e96d7c2 100644\n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -409,6 +409,9 @@ include shared.mak\n> > # to the \"\" of the corresponding `compat/fsmonitor/fsm-settings-.c`\n> > # that implements the `fsm_os_settings__*()` routines.\n> > #\n> > +# Define LINK_FUZZ_PROGRAMS if you want `make all` to also build the fuzz test\n> > +# programs in oss-fuzz/.\n> > +#\n> > # === Optional library: libintl ===\n> > #\n> > # Define NO_GETTEXT if you don't want Git output to be translated.\n> > @@ -763,9 +766,6 @@ FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o\n> > .PHONY: fuzz-objs\n> > fuzz-objs: $(FUZZ_OBJS)\n> > \n> > -# Always build fuzz objects even if not testing, to prevent bit-rot.\n> > -all:: $(FUZZ_OBJS)\n> > -\n> > FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS)))\n> > \n> > # Empty...\n> > @@ -2368,6 +2368,14 @@ ifndef NO_TCLTK\n> > endif\n> > \t$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'\n> > \n> > +# Build fuzz programs if possible, or at least compile the object files; even\n> > +# without the necessary fuzzing support, this prevents bit-rot.\n> > +ifdef LINK_FUZZ_PROGRAMS\n> > +all:: $(FUZZ_PROGRAMS)\n> > +else\n> > +all:: $(FUZZ_OBJS)\n> > +endif\n> \n> It would have been easier on the eyes if we had the fuzz things\n> together, perhaps like this simplified version? We build FUZZ_OBJS\n> either way, and when the LINK_FUZZ_PROGRAMS is requested, we follow\n> the fuzz-all recipe, too.\n\nWe need the LINK_FUZZ_PROGRAMS conditional to happen after we import\nconfig.mak.uname (line 1434 in my V1). We also need to define FUZZ_OBJS\nprior to adding it to OBJECTS (line 2698 in V1). I can move all of the\nfuzz-definition within that range, keeping everything in one place at\nthe cost of a larger diff. I'll do that for V2, but if you prefer\notherwise please let me know.\n\nAlthough I'm not 100% sure that we even need to add FUZZ_OBJS to\nOBJECTS, so let me check that tomorrow. If not, then I can move\neverything to the bottom of the Makefile where we also define fuzz-all\nand the build rules for FUZZ_PROGRAMS.\n\n\n> diff --git c/Makefile w/Makefile\n> index 4e255c81f2..46e457a7a8 100644\n> --- c/Makefile\n> +++ w/Makefile\n> @@ -409,6 +409,9 @@ include shared.mak\n> # to the \"\" of the corresponding `compat/fsmonitor/fsm-settings-.c`\n> # that implements the `fsm_os_settings__*()` routines.\n> #\n> +# Define LINK_FUZZ_PROGRAMS if you want `make all` to also build the fuzz test\n> +# programs in oss-fuzz/.\n> +#\n> # === Optional library: libintl ===\n> #\n> # Define NO_GETTEXT if you don't want Git output to be translated.\n> @@ -766,6 +769,12 @@ fuzz-objs: $(FUZZ_OBJS)\n> # Always build fuzz objects even if not testing, to prevent bit-rot.\n> all:: $(FUZZ_OBJS)\n> \n> +# Build fuzz programs, even without the necessary fuzzing support,\n> +# this prevents bit-rot.\n> +ifdef LINK_FUZZ_PROGRAMS\n> +all:: fuzz-all\n> +endif\n> +\n> FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS)))\n> \n> # Empty...\n"},{"id":"492741","messageId":"Zhb7YwMdtNKzpCSw@google.com","threadId":"61056","inReplyTo":"ZhW6BM9V-Rto_CW4@google.com","subject":"Re: [PATCH 2/2] fuzz: link fuzz programs with `make all` on Linux","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-04-10T20:49:39Z","receivedAt":"2024-04-10T20:49:46Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.04.09 14:58, Josh Steadmon wrote:\n> On 2024.03.05 13:44, Junio C Hamano wrote:\n> > Josh Steadmon writes:\n> > \n> > > Since 5e47215080 (fuzz: add basic fuzz testing target., 2018-10-12), we\n> > > have compiled object files for the fuzz tests as part of the default\n> > > 'make all' target. This helps prevent bit-rot in lesser-used parts of\n> > > the codebase, by making sure that incompatible changes are caught at\n> > > build time.\n> > >\n> > > However, since we never linked the fuzzer executables, this did not\n> > > protect us from link-time errors. As of 8b9a42bf48 (fuzz: fix fuzz test\n> > > build rules, 2024-01-19), it's now possible to link the fuzzer\n> > > executables without using a fuzzing engine and a variety of\n> > > compiler-specific (and compiler-version-specific) flags, at least on\n> > > Linux. So let's add a platform-specific option in config.mak.uname to\n> > > link the executables as part of the default `make all` target.\n> > >\n> > > Suggested-by: Junio C Hamano \n> > > Signed-off-by: Josh Steadmon \n> > > ---\n> > > Makefile | 14 +++++++++++---\n> > > config.mak.uname | 1 +\n> > > 2 files changed, 12 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/Makefile b/Makefile\n> > > index 4e255c81f2..f74e96d7c2 100644\n> > > --- a/Makefile\n> > > +++ b/Makefile\n> > > @@ -409,6 +409,9 @@ include shared.mak\n> > > # to the \"\" of the corresponding `compat/fsmonitor/fsm-settings-.c`\n> > > # that implements the `fsm_os_settings__*()` routines.\n> > > #\n> > > +# Define LINK_FUZZ_PROGRAMS if you want `make all` to also build the fuzz test\n> > > +# programs in oss-fuzz/.\n> > > +#\n> > > # === Optional library: libintl ===\n> > > #\n> > > # Define NO_GETTEXT if you don't want Git output to be translated.\n> > > @@ -763,9 +766,6 @@ FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o\n> > > .PHONY: fuzz-objs\n> > > fuzz-objs: $(FUZZ_OBJS)\n> > > \n> > > -# Always build fuzz objects even if not testing, to prevent bit-rot.\n> > > -all:: $(FUZZ_OBJS)\n> > > -\n> > > FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS)))\n> > > \n> > > # Empty...\n> > > @@ -2368,6 +2368,14 @@ ifndef NO_TCLTK\n> > > endif\n> > > \t$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'\n> > > \n> > > +# Build fuzz programs if possible, or at least compile the object files; even\n> > > +# without the necessary fuzzing support, this prevents bit-rot.\n> > > +ifdef LINK_FUZZ_PROGRAMS\n> > > +all:: $(FUZZ_PROGRAMS)\n> > > +else\n> > > +all:: $(FUZZ_OBJS)\n> > > +endif\n> > \n> > It would have been easier on the eyes if we had the fuzz things\n> > together, perhaps like this simplified version? We build FUZZ_OBJS\n> > either way, and when the LINK_FUZZ_PROGRAMS is requested, we follow\n> > the fuzz-all recipe, too.\n> \n> We need the LINK_FUZZ_PROGRAMS conditional to happen after we import\n> config.mak.uname (line 1434 in my V1). We also need to define FUZZ_OBJS\n> prior to adding it to OBJECTS (line 2698 in V1). I can move all of the\n> fuzz-definition within that range, keeping everything in one place at\n> the cost of a larger diff. I'll do that for V2, but if you prefer\n> otherwise please let me know.\n> \n> Although I'm not 100% sure that we even need to add FUZZ_OBJS to\n> OBJECTS, so let me check that tomorrow. If not, then I can move\n> everything to the bottom of the Makefile where we also define fuzz-all\n> and the build rules for FUZZ_PROGRAMS.\n\nIt turns out we do need FUZZ_OBJS in OBJECTS so that we define a build\nrule, otherwise the Makefile doesn't know how to compile the fuzzer\nobjects. So V2 will have most of the fuzzer rules in the line\n(1434,2698) range.\n"},{"id":"492742","messageId":"xmqqh6g9rl5k.fsf@gitster.g","threadId":"61056","inReplyTo":"Zhb7YwMdtNKzpCSw@google.com","subject":"Re: [PATCH 2/2] fuzz: link fuzz programs with `make all` on Linux","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-10T20:57:11Z","receivedAt":"2024-04-10T20:57:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> It turns out we do need FUZZ_OBJS in OBJECTS so that we define a build\n> rule, otherwise the Makefile doesn't know how to compile the fuzzer\n> objects. So V2 will have most of the fuzzer rules in the line\n> (1434,2698) range.\n\nThe reasoning and the conclusion sound sensible.\nThanks.\n"},{"id":"492743","messageId":"Zhb9ioeICP6FRJlu@google.com","threadId":"61056","inReplyTo":"20240306010016.GA3811328@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] ci: also define CXX environment variable","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-04-10T20:58:50Z","receivedAt":"2024-04-10T20:58:56Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.03.05 20:00, Jeff King wrote:\n> On Tue, Mar 05, 2024 at 07:50:58PM -0500, Jeff King wrote:\n> \n> > On Tue, Mar 05, 2024 at 01:11:59PM -0800, Josh Steadmon wrote:\n> > \n> > > In a future commit, we will build the fuzzer executables as part of the\n> > > default 'make all' target, which requires a C++ compiler. If we do not\n> > > explicitly set CXX, it defaults to g++ on GitHub CI. However, this can\n> > > lead to incorrect feature detection when CC=clang, since the\n> > > 'detect-compiler' script only looks at CC. Fix the issue by always\n> > > setting CXX to match CC in our CI config.\n> > > \n> > > We only plan on building fuzzers on Linux, so none of the other CI\n> > > configs need a similar adjustment.\n> > \n> > Does this mean that after your patch 2, running:\n> > \n> > make CC=clang\n> > \n> > may have problems on Linux, because it will now try to link fuzzers\n> > using g++, even though everything else is built with clang (and ditto\n> > the detect-compiler used it)?\n> \n> Also, if the answer is \"yes\": do we really need a c++ linker here? My\n> understanding from reading \"git log -SCXX Makefile\" is that when using\n> oss-fuzz, you'd sometimes want to pass c++ specific things in\n> FUZZ_CXXFLAGS. But we're not using that here, and are just making sure\n> that things can be linked. Can we just use $(CC) by default here, then?\n> \n> Something like:\n> \n> diff --git a/Makefile b/Makefile\n> index f74e96d7c2..3f09d75f46 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -3861,17 +3861,18 @@ cover_db_html: cover_db\n> #\n> # An example command to build against libFuzzer from LLVM 11.0.0:\n> #\n> -# make CC=clang CXX=clang++ \\\n> +# make CC=clang FUZZ_CXX=clang++ \\\n> # CFLAGS=\"-fsanitize=fuzzer-no-link,address\" \\\n> # LIB_FUZZING_ENGINE=\"-fsanitize=fuzzer,address\" \\\n> # fuzz-all\n> #\n> +FUZZ_CXX ?= $(CC)\n> FUZZ_CXXFLAGS ?= $(ALL_CFLAGS)\n> \n> .PHONY: fuzz-all\n> \n> $(FUZZ_PROGRAMS): %: %.o oss-fuzz/dummy-cmd-main.o $(GITLIBS) GIT-LDFLAGS\n> -\t$(QUIET_LINK)$(CXX) $(FUZZ_CXXFLAGS) -o $@ $(ALL_LDFLAGS) \\\n> +\t$(QUIET_LINK)$(FUZZ_CXX) $(FUZZ_CXXFLAGS) -o $@ $(ALL_LDFLAGS) \\\n> \t\t-Wl,--allow-multiple-definition \\\n> \t\t$(filter %.o,$^) $(filter %.a,$^) $(LIBS) $(LIB_FUZZING_ENGINE)\n> \n> \n> -Peff\n\nIndeed, it does break, and this is a good fix. Thanks for the catch!\n"},{"id":"492744","messageId":"20240410211100.GA2276041@coredump.intra.peff.net","threadId":"61056","inReplyTo":"ZhW6BM9V-Rto_CW4@google.com","subject":"Re: [PATCH 2/2] fuzz: link fuzz programs with `make all` on Linux","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-10T21:11:00Z","receivedAt":"2024-04-10T21:11:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 09, 2024 at 02:58:28PM -0700, Josh Steadmon wrote:\n\n> > It would have been easier on the eyes if we had the fuzz things\n> > together, perhaps like this simplified version? We build FUZZ_OBJS\n> > either way, and when the LINK_FUZZ_PROGRAMS is requested, we follow\n> > the fuzz-all recipe, too.\n> \n> We need the LINK_FUZZ_PROGRAMS conditional to happen after we import\n> config.mak.uname (line 1434 in my V1). We also need to define FUZZ_OBJS\n> prior to adding it to OBJECTS (line 2698 in V1). I can move all of the\n> fuzz-definition within that range, keeping everything in one place at\n> the cost of a larger diff. I'll do that for V2, but if you prefer\n> otherwise please let me know.\n> \n> Although I'm not 100% sure that we even need to add FUZZ_OBJS to\n> OBJECTS, so let me check that tomorrow. If not, then I can move\n> everything to the bottom of the Makefile where we also define fuzz-all\n> and the build rules for FUZZ_PROGRAMS.\n\nThe conditional has to be read handled while reading the Makefile, but\nas a \"simple\" variable, OBJECTS isn't expanded until the whole Makefile\nhas been read. So for example this out-of-order definition works:\n\ndiff --git a/Makefile b/Makefile\nindex 533eaae612..5dbf1935a1 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -755,6 +755,7 @@ ETAGS_TARGET = TAGS\n # If you add a new fuzzer, please also make sure to run it in\n # ci/run-build-and-minimal-fuzzers.sh so that we make sure it still links and\n # runs in the future.\n+OBJECTS += $(FUZZ_OBJS)\n FUZZ_OBJS += oss-fuzz/dummy-cmd-main.o\n FUZZ_OBJS += oss-fuzz/fuzz-commit-graph.o\n FUZZ_OBJS += oss-fuzz/fuzz-config.o\n@@ -2695,7 +2696,6 @@ OBJECTS += $(SCALAR_OBJS)\n OBJECTS += $(PROGRAM_OBJS)\n OBJECTS += $(TEST_OBJS)\n OBJECTS += $(XDIFF_OBJS)\n-OBJECTS += $(FUZZ_OBJS)\n OBJECTS += $(REFTABLE_OBJS) $(REFTABLE_TEST_OBJS)\n OBJECTS += $(UNIT_TEST_OBJS)\n \n\nNow whether that is useful for organizing the Makefile, I don't know,\nbut I thought I'd throw it out there in case it helps you.\n\n-Peff\n"},{"id":"492791","messageId":"cover.1712858920.git.steadmon@google.com","threadId":"61056","inReplyTo":"cover.1709673020.git.steadmon@google.com","subject":"[PATCH v2 0/2] fuzz: build fuzzers by default on Linux","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-04-11T18:14:23Z","receivedAt":"2024-04-11T18:14:27Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Please note: this has been rebased onto the current 'master' [436d4e5b14\n(The seventeenth batch, 2024-04-10)] in order to resolve a conflict with\nthe recently merged bt/fuzz-config-parse series.\n\nIncrease our protection against fuzzer bit-rot by making sure we can\nlink the fuzz test executables on Linux. Patch 1 is a small CI config\nimprovement to fix compiler feature detection. Patch 2 is the Makefile /\nconfig.mak.uname change to add the executables to `make all` on Linux.\n\nChanges in V2:\n* Rebased onto master\n* Fixed compiler mismatch issue when we override CC but not CXX\n* Consolidated some of the fuzzer Makefile definitions in one location\n\n\nJosh Steadmon (2):\n ci: also define CXX environment variable\n fuzz: link fuzz programs with `make all` on Linux\n\n .github/workflows/main.yml | 12 +++++++\n Makefile | 51 +++++++++++++++++------------\n ci/run-build-and-minimal-fuzzers.sh | 2 +-\n config.mak.uname | 1 +\n 4 files changed, 44 insertions(+), 22 deletions(-)\n\nRange-diff against v1:\n1: 75f98cbf98 ! 1: e55b691272 ci: also define CXX environment variable\n @@ Commit message\n 'detect-compiler' script only looks at CC. Fix the issue by always\n setting CXX to match CC in our CI config.\n \n - We only plan on building fuzzers on Linux, so none of the other CI\n - configs need a similar adjustment.\n + We only plan on building fuzzers on Linux with the next patch, so for\n + now, only adjust configuration for the Linux CI jobs.\n \n \n2: eef15e3d3d < -: ---------- fuzz: link fuzz programs with `make all` on Linux\n-: ---------- > 2: 8846a7766a fuzz: link fuzz programs with `make all` on Linux\n\nbase-commit: 436d4e5b14df49870a897f64fe92c0ddc7017e4c\n-- \n2.44.0.683.g7961c838ac-goog\n\n"},{"id":"492792","messageId":"e55b6912725fa478134c7a67a9e4aeab7dca2c57.1712858920.git.steadmon@google.com","threadId":"61056","inReplyTo":"cover.1712858920.git.steadmon@google.com","subject":"[PATCH v2 1/2] ci: also define CXX environment variable","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-04-11T18:14:24Z","receivedAt":"2024-04-11T18:14:29Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"In a future commit, we will build the fuzzer executables as part of the\ndefault 'make all' target, which requires a C++ compiler. If we do not\nexplicitly set CXX, it defaults to g++ on GitHub CI. However, this can\nlead to incorrect feature detection when CC=clang, since the\n'detect-compiler' script only looks at CC. Fix the issue by always\nsetting CXX to match CC in our CI config.\n\nWe only plan on building fuzzers on Linux with the next patch, so for\nnow, only adjust configuration for the Linux CI jobs.\n\nSigned-off-by: Josh Steadmon \n---\n .github/workflows/main.yml | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\nindex 3428773b09..d9986256e6 100644\n--- a/.github/workflows/main.yml\n+++ b/.github/workflows/main.yml\n@@ -265,42 +265,54 @@ jobs:\n vector:\n - jobname: linux-sha256\n cc: clang\n+ cxx: clang++\n pool: ubuntu-latest\n - jobname: linux-reftable\n cc: clang\n+ cxx: clang++\n pool: ubuntu-latest\n - jobname: linux-gcc\n cc: gcc\n+ cxx: g++\n cc_package: gcc-8\n pool: ubuntu-20.04\n - jobname: linux-TEST-vars\n cc: gcc\n+ cxx: g++\n cc_package: gcc-8\n pool: ubuntu-20.04\n - jobname: osx-clang\n cc: clang\n+ cxx: clang++\n pool: macos-13\n - jobname: osx-reftable\n cc: clang\n+ cxx: clang++\n pool: macos-13\n - jobname: osx-gcc\n cc: gcc\n+ cxx: g++\n cc_package: gcc-13\n pool: macos-13\n - jobname: linux-gcc-default\n cc: gcc\n+ cxx: g++\n pool: ubuntu-latest\n - jobname: linux-leaks\n cc: gcc\n+ cxx: g++\n pool: ubuntu-latest\n - jobname: linux-reftable-leaks\n cc: gcc\n+ cxx: g++\n pool: ubuntu-latest\n - jobname: linux-asan-ubsan\n cc: clang\n+ cxx: clang++\n pool: ubuntu-latest\n env:\n CC: ${{matrix.vector.cc}}\n+ CXX: ${{matrix.vector.cxx}}\n CC_PACKAGE: ${{matrix.vector.cc_package}}\n jobname: ${{matrix.vector.jobname}}\n runs_on_pool: ${{matrix.vector.pool}}\n-- \n2.44.0.683.g7961c838ac-goog\n\n"},{"id":"492793","messageId":"8846a7766a1e14373272f7115d37a3b774f51a71.1712858920.git.steadmon@google.com","threadId":"61056","inReplyTo":"cover.1712858920.git.steadmon@google.com","subject":"[PATCH v2 2/2] fuzz: link fuzz programs with `make all` on Linux","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-04-11T18:14:25Z","receivedAt":"2024-04-11T18:14:31Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Since 5e47215080 (fuzz: add basic fuzz testing target., 2018-10-12), we\nhave compiled object files for the fuzz tests as part of the default\n'make all' target. This helps prevent bit-rot in lesser-used parts of\nthe codebase, by making sure that incompatible changes are caught at\nbuild time.\n\nHowever, since we never linked the fuzzer executables, this did not\nprotect us from link-time errors. As of 8b9a42bf48 (fuzz: fix fuzz test\nbuild rules, 2024-01-19), it's now possible to link the fuzzer\nexecutables without using a fuzzing engine and a variety of\ncompiler-specific (and compiler-version-specific) flags, at least on\nLinux. So let's add a platform-specific option in config.mak.uname to\nlink the executables as part of the default `make all` target.\n\nSince linking the fuzzer executables without a fuzzing engine does not\nrequire a C++ compiler, we can change the FUZZ_PROGRAMS build rule to\nuse $(CC) by default. This avoids compiler mis-match issues when\noverriding $(CC) but not $(CXX). When we *do* want to actually link with\na fuzzing engine, we can set $(FUZZ_CXX). The build instructions in the\nCI fuzz-smoke-test job and in the Makefile comment have been updated\naccordingly.\n\nWhile we're at it, we can consolidate some of the fuzzer build\ninstructions into one location in the Makefile.\n\nSuggested-by: Junio C Hamano \nHelped-by: Jeff King \nSigned-off-by: Josh Steadmon \n---\n Makefile | 51 +++++++++++++++++------------\n ci/run-build-and-minimal-fuzzers.sh | 2 +-\n config.mak.uname | 1 +\n 3 files changed, 32 insertions(+), 22 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex c43c1bd1a0..b9e97ad3b9 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -409,6 +409,9 @@ include shared.mak\n # to the \"\" of the corresponding `compat/fsmonitor/fsm-settings-.c`\n # that implements the `fsm_os_settings__*()` routines.\n #\n+# Define LINK_FUZZ_PROGRAMS if you want `make all` to also build the fuzz test\n+# programs in oss-fuzz/.\n+#\n # === Optional library: libintl ===\n #\n # Define NO_GETTEXT if you don't want Git output to be translated.\n@@ -752,23 +755,6 @@ SCRIPTS = $(SCRIPT_SH_GEN) \\\n \n ETAGS_TARGET = TAGS\n \n-# If you add a new fuzzer, please also make sure to run it in\n-# ci/run-build-and-minimal-fuzzers.sh so that we make sure it still links and\n-# runs in the future.\n-FUZZ_OBJS += oss-fuzz/dummy-cmd-main.o\n-FUZZ_OBJS += oss-fuzz/fuzz-commit-graph.o\n-FUZZ_OBJS += oss-fuzz/fuzz-config.o\n-FUZZ_OBJS += oss-fuzz/fuzz-date.o\n-FUZZ_OBJS += oss-fuzz/fuzz-pack-headers.o\n-FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o\n-.PHONY: fuzz-objs\n-fuzz-objs: $(FUZZ_OBJS)\n-\n-# Always build fuzz objects even if not testing, to prevent bit-rot.\n-all:: $(FUZZ_OBJS)\n-\n-FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS)))\n-\n # Empty...\n EXTRA_PROGRAMS =\n \n@@ -2372,6 +2358,29 @@ ifndef NO_TCLTK\n endif\n \t$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'\n \n+# If you add a new fuzzer, please also make sure to run it in\n+# ci/run-build-and-minimal-fuzzers.sh so that we make sure it still links and\n+# runs in the future.\n+FUZZ_OBJS += oss-fuzz/dummy-cmd-main.o\n+FUZZ_OBJS += oss-fuzz/fuzz-commit-graph.o\n+FUZZ_OBJS += oss-fuzz/fuzz-config.o\n+FUZZ_OBJS += oss-fuzz/fuzz-date.o\n+FUZZ_OBJS += oss-fuzz/fuzz-pack-headers.o\n+FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o\n+.PHONY: fuzz-objs\n+fuzz-objs: $(FUZZ_OBJS)\n+\n+# Always build fuzz objects even if not testing, to prevent bit-rot.\n+all:: $(FUZZ_OBJS)\n+\n+FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS)))\n+\n+# Build fuzz programs when possible, even without the necessary fuzzing support,\n+# to prevent bit-rot.\n+ifdef LINK_FUZZ_PROGRAMS\n+all:: $(FUZZ_PROGRAMS)\n+endif\n+\n please_set_SHELL_PATH_to_a_more_modern_shell:\n \t@$$(:)\n \n@@ -3857,22 +3866,22 @@ cover_db_html: cover_db\n #\n # An example command to build against libFuzzer from LLVM 11.0.0:\n #\n-# make CC=clang CXX=clang++ \\\n+# make CC=clang FUZZ_CXX=clang++ \\\n # CFLAGS=\"-fsanitize=fuzzer-no-link,address\" \\\n # LIB_FUZZING_ENGINE=\"-fsanitize=fuzzer,address\" \\\n # fuzz-all\n #\n+FUZZ_CXX ?= $(CC)\n FUZZ_CXXFLAGS ?= $(ALL_CFLAGS)\n \n .PHONY: fuzz-all\n+fuzz-all: $(FUZZ_PROGRAMS)\n \n $(FUZZ_PROGRAMS): %: %.o oss-fuzz/dummy-cmd-main.o $(GITLIBS) GIT-LDFLAGS\n-\t$(QUIET_LINK)$(CXX) $(FUZZ_CXXFLAGS) -o $@ $(ALL_LDFLAGS) \\\n+\t$(QUIET_LINK)$(FUZZ_CXX) $(FUZZ_CXXFLAGS) -o $@ $(ALL_LDFLAGS) \\\n \t\t-Wl,--allow-multiple-definition \\\n \t\t$(filter %.o,$^) $(filter %.a,$^) $(LIBS) $(LIB_FUZZING_ENGINE)\n \n-fuzz-all: $(FUZZ_PROGRAMS)\n-\n $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS\n \t$(call mkdir_p_parent_template)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \\\ndiff --git a/ci/run-build-and-minimal-fuzzers.sh b/ci/run-build-and-minimal-fuzzers.sh\nindex a51076d18d..797d65c661 100755\n--- a/ci/run-build-and-minimal-fuzzers.sh\n+++ b/ci/run-build-and-minimal-fuzzers.sh\n@@ -7,7 +7,7 @@\n \n group \"Build fuzzers\" make \\\n \tCC=clang \\\n-\tCXX=clang++ \\\n+\tFUZZ_CXX=clang++ \\\n \tCFLAGS=\"-fsanitize=fuzzer-no-link,address\" \\\n \tLIB_FUZZING_ENGINE=\"-fsanitize=fuzzer,address\" \\\n \tfuzz-all\ndiff --git a/config.mak.uname b/config.mak.uname\nindex d0dcca2ec5..9107b4ae2b 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -68,6 +68,7 @@ ifeq ($(uname_S),Linux)\n \tifneq ($(findstring .el7.,$(uname_R)),)\n \t\tBASIC_CFLAGS += -std=c99\n \tendif\n+\tLINK_FUZZ_PROGRAMS = YesPlease\n endif\n ifeq ($(uname_S),GNU/kFreeBSD)\n \tHAVE_ALLOCA_H = YesPlease\n-- \n2.44.0.683.g7961c838ac-goog\n\n"},{"id":"492808","messageId":"xmqq1q7br33h.fsf@gitster.g","threadId":"61056","inReplyTo":"8846a7766a1e14373272f7115d37a3b774f51a71.1712858920.git.steadmon@google.com","subject":"Re: [PATCH v2 2/2] fuzz: link fuzz programs with `make all` on Linux","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-11T21:39:30Z","receivedAt":"2024-04-11T21:39:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> @@ -752,23 +755,6 @@ SCRIPTS = $(SCRIPT_SH_GEN) \\\n> \n> - ...\n> -# Always build fuzz objects even if not testing, to prevent bit-rot.\n> -all:: $(FUZZ_OBJS)\n> -...\n> -FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS)))\n> -\n> # Empty...\n> EXTRA_PROGRAMS =\n\nAs Peff said earlier, I suspect there is no need to move things for\ndependencies (make rules are somewhat declarative), but grouping all\nthe things related to fuzzing is a good idea, so I am OK with the\nnew location.\n\n> +# Always build fuzz objects even if not testing, to prevent bit-rot.\n> +all:: $(FUZZ_OBJS)\n> +\n> +FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS)))\n> +\n> +# Build fuzz programs when possible, even without the necessary fuzzing support,\n> +# to prevent bit-rot.\n> +ifdef LINK_FUZZ_PROGRAMS\n> +all:: $(FUZZ_PROGRAMS)\n> +endif\n\nOK.\n\nWill queue. Thanks.\n"},{"id":"492827","messageId":"20240412042247.GA1077925@coredump.intra.peff.net","threadId":"61056","inReplyTo":"e55b6912725fa478134c7a67a9e4aeab7dca2c57.1712858920.git.steadmon@google.com","subject":"Re: [PATCH v2 1/2] ci: also define CXX environment variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-12T04:22:47Z","receivedAt":"2024-04-12T04:22:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 11, 2024 at 11:14:24AM -0700, Josh Steadmon wrote:\n\n> In a future commit, we will build the fuzzer executables as part of the\n> default 'make all' target, which requires a C++ compiler. If we do not\n> explicitly set CXX, it defaults to g++ on GitHub CI. However, this can\n> lead to incorrect feature detection when CC=clang, since the\n> 'detect-compiler' script only looks at CC. Fix the issue by always\n> setting CXX to match CC in our CI config.\n\nSince you took my suggestion in patch 2, this \"which requires a C++\ncompiler\" is no longer true, is it? And I don't think we'd even look at\nthe CXX variable at all, since it's now FUZZ_CXX.\n\nSo this patch can just be dropped, I'd think.\n\n-Peff\n"},{"id":"493423","messageId":"ba9d24c6445de309226bf7c165499f1969807fef.1713982389.git.steadmon@google.com","threadId":"61056","inReplyTo":"cover.1709673020.git.steadmon@google.com","subject":"[PATCH v3] fuzz: link fuzz programs with `make all` on Linux","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-04-24T18:14:42Z","receivedAt":"2024-04-24T18:14:45Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Since 5e47215080 (fuzz: add basic fuzz testing target., 2018-10-12), we\nhave compiled object files for the fuzz tests as part of the default\n'make all' target. This helps prevent bit-rot in lesser-used parts of\nthe codebase, by making sure that incompatible changes are caught at\nbuild time.\n\nHowever, since we never linked the fuzzer executables, this did not\nprotect us from link-time errors. As of 8b9a42bf48 (fuzz: fix fuzz test\nbuild rules, 2024-01-19), it's now possible to link the fuzzer\nexecutables without using a fuzzing engine and a variety of\ncompiler-specific (and compiler-version-specific) flags, at least on\nLinux. So let's add a platform-specific option in config.mak.uname to\nlink the executables as part of the default `make all` target.\n\nSince linking the fuzzer executables without a fuzzing engine does not\nrequire a C++ compiler, we can change the FUZZ_PROGRAMS build rule to\nuse $(CC) by default. This avoids compiler mis-match issues when\noverriding $(CC) but not $(CXX). When we *do* want to actually link with\na fuzzing engine, we can set $(FUZZ_CXX). The build instructions in the\nCI fuzz-smoke-test job and in the Makefile comment have been updated\naccordingly.\n\nWhile we're at it, we can consolidate some of the fuzzer build\ninstructions into one location in the Makefile.\n\nSuggested-by: Junio C Hamano \nHelped-by: Jeff King \nSigned-off-by: Josh Steadmon \n---\nChanges in V3:\n* Dropped CI config patch; no longer needed since we don't use CXX in\n fuzzer build rules anymore\n\nChanges in V2:\n* Rebased onto master\n* Fixed compiler mismatch issue when we override CC but not CXX\n* Consolidated some of the fuzzer Makefile definitions in one location\n\nRange-diff against v2:\n1: e55b691272 < -: ---------- ci: also define CXX environment variable\n2: 8846a7766a = 1: ba9d24c644 fuzz: link fuzz programs with `make all` on Linux\n\n Makefile | 51 +++++++++++++++++------------\n ci/run-build-and-minimal-fuzzers.sh | 2 +-\n config.mak.uname | 1 +\n 3 files changed, 32 insertions(+), 22 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex c43c1bd1a0..b9e97ad3b9 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -409,6 +409,9 @@ include shared.mak\n # to the \"\" of the corresponding `compat/fsmonitor/fsm-settings-.c`\n # that implements the `fsm_os_settings__*()` routines.\n #\n+# Define LINK_FUZZ_PROGRAMS if you want `make all` to also build the fuzz test\n+# programs in oss-fuzz/.\n+#\n # === Optional library: libintl ===\n #\n # Define NO_GETTEXT if you don't want Git output to be translated.\n@@ -752,23 +755,6 @@ SCRIPTS = $(SCRIPT_SH_GEN) \\\n \n ETAGS_TARGET = TAGS\n \n-# If you add a new fuzzer, please also make sure to run it in\n-# ci/run-build-and-minimal-fuzzers.sh so that we make sure it still links and\n-# runs in the future.\n-FUZZ_OBJS += oss-fuzz/dummy-cmd-main.o\n-FUZZ_OBJS += oss-fuzz/fuzz-commit-graph.o\n-FUZZ_OBJS += oss-fuzz/fuzz-config.o\n-FUZZ_OBJS += oss-fuzz/fuzz-date.o\n-FUZZ_OBJS += oss-fuzz/fuzz-pack-headers.o\n-FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o\n-.PHONY: fuzz-objs\n-fuzz-objs: $(FUZZ_OBJS)\n-\n-# Always build fuzz objects even if not testing, to prevent bit-rot.\n-all:: $(FUZZ_OBJS)\n-\n-FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS)))\n-\n # Empty...\n EXTRA_PROGRAMS =\n \n@@ -2372,6 +2358,29 @@ ifndef NO_TCLTK\n endif\n \t$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'\n \n+# If you add a new fuzzer, please also make sure to run it in\n+# ci/run-build-and-minimal-fuzzers.sh so that we make sure it still links and\n+# runs in the future.\n+FUZZ_OBJS += oss-fuzz/dummy-cmd-main.o\n+FUZZ_OBJS += oss-fuzz/fuzz-commit-graph.o\n+FUZZ_OBJS += oss-fuzz/fuzz-config.o\n+FUZZ_OBJS += oss-fuzz/fuzz-date.o\n+FUZZ_OBJS += oss-fuzz/fuzz-pack-headers.o\n+FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o\n+.PHONY: fuzz-objs\n+fuzz-objs: $(FUZZ_OBJS)\n+\n+# Always build fuzz objects even if not testing, to prevent bit-rot.\n+all:: $(FUZZ_OBJS)\n+\n+FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS)))\n+\n+# Build fuzz programs when possible, even without the necessary fuzzing support,\n+# to prevent bit-rot.\n+ifdef LINK_FUZZ_PROGRAMS\n+all:: $(FUZZ_PROGRAMS)\n+endif\n+\n please_set_SHELL_PATH_to_a_more_modern_shell:\n \t@$$(:)\n \n@@ -3857,22 +3866,22 @@ cover_db_html: cover_db\n #\n # An example command to build against libFuzzer from LLVM 11.0.0:\n #\n-# make CC=clang CXX=clang++ \\\n+# make CC=clang FUZZ_CXX=clang++ \\\n # CFLAGS=\"-fsanitize=fuzzer-no-link,address\" \\\n # LIB_FUZZING_ENGINE=\"-fsanitize=fuzzer,address\" \\\n # fuzz-all\n #\n+FUZZ_CXX ?= $(CC)\n FUZZ_CXXFLAGS ?= $(ALL_CFLAGS)\n \n .PHONY: fuzz-all\n+fuzz-all: $(FUZZ_PROGRAMS)\n \n $(FUZZ_PROGRAMS): %: %.o oss-fuzz/dummy-cmd-main.o $(GITLIBS) GIT-LDFLAGS\n-\t$(QUIET_LINK)$(CXX) $(FUZZ_CXXFLAGS) -o $@ $(ALL_LDFLAGS) \\\n+\t$(QUIET_LINK)$(FUZZ_CXX) $(FUZZ_CXXFLAGS) -o $@ $(ALL_LDFLAGS) \\\n \t\t-Wl,--allow-multiple-definition \\\n \t\t$(filter %.o,$^) $(filter %.a,$^) $(LIBS) $(LIB_FUZZING_ENGINE)\n \n-fuzz-all: $(FUZZ_PROGRAMS)\n-\n $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS\n \t$(call mkdir_p_parent_template)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \\\ndiff --git a/ci/run-build-and-minimal-fuzzers.sh b/ci/run-build-and-minimal-fuzzers.sh\nindex a51076d18d..797d65c661 100755\n--- a/ci/run-build-and-minimal-fuzzers.sh\n+++ b/ci/run-build-and-minimal-fuzzers.sh\n@@ -7,7 +7,7 @@\n \n group \"Build fuzzers\" make \\\n \tCC=clang \\\n-\tCXX=clang++ \\\n+\tFUZZ_CXX=clang++ \\\n \tCFLAGS=\"-fsanitize=fuzzer-no-link,address\" \\\n \tLIB_FUZZING_ENGINE=\"-fsanitize=fuzzer,address\" \\\n \tfuzz-all\ndiff --git a/config.mak.uname b/config.mak.uname\nindex d0dcca2ec5..9107b4ae2b 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -68,6 +68,7 @@ ifeq ($(uname_S),Linux)\n \tifneq ($(findstring .el7.,$(uname_R)),)\n \t\tBASIC_CFLAGS += -std=c99\n \tendif\n+\tLINK_FUZZ_PROGRAMS = YesPlease\n endif\n ifeq ($(uname_S),GNU/kFreeBSD)\n \tHAVE_ALLOCA_H = YesPlease\n\nbase-commit: 436d4e5b14df49870a897f64fe92c0ddc7017e4c\n-- \n2.44.0.769.g3c40516874-goog\n\n"},{"id":"493424","messageId":"4srqrveqk2f5wwrkfivzx7ipj6txgsohdtl76ybyvg6e2vrrcx@gglr37gka774","threadId":"61056","inReplyTo":"20240412042247.GA1077925@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/2] ci: also define CXX environment variable","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-04-24T18:15:03Z","receivedAt":"2024-04-24T18:15:09Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.04.12 00:22, Jeff King wrote:\n> On Thu, Apr 11, 2024 at 11:14:24AM -0700, Josh Steadmon wrote:\n> \n> > In a future commit, we will build the fuzzer executables as part of the\n> > default 'make all' target, which requires a C++ compiler. If we do not\n> > explicitly set CXX, it defaults to g++ on GitHub CI. However, this can\n> > lead to incorrect feature detection when CC=clang, since the\n> > 'detect-compiler' script only looks at CC. Fix the issue by always\n> > setting CXX to match CC in our CI config.\n> \n> Since you took my suggestion in patch 2, this \"which requires a C++\n> compiler\" is no longer true, is it? And I don't think we'd even look at\n> the CXX variable at all, since it's now FUZZ_CXX.\n> \n> So this patch can just be dropped, I'd think.\n> \n> -Peff\n\nDone in V3.\n"},{"id":"493426","messageId":"xmqqv846y3yj.fsf@gitster.g","threadId":"61056","inReplyTo":"ba9d24c6445de309226bf7c165499f1969807fef.1713982389.git.steadmon@google.com","subject":"Re: [PATCH v3] fuzz: link fuzz programs with `make all` on Linux","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-24T19:07:32Z","receivedAt":"2024-04-24T19:07:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> Since linking the fuzzer executables without a fuzzing engine does not\n> require a C++ compiler, we can change the FUZZ_PROGRAMS build rule to\n> use $(CC) by default. This avoids compiler mis-match issues when\n> overriding $(CC) but not $(CXX). When we *do* want to actually link with\n> a fuzzing engine, we can set $(FUZZ_CXX). The build instructions in the\n> CI fuzz-smoke-test job and in the Makefile comment have been updated\n> accordingly.\n>\n> While we're at it, we can consolidate some of the fuzzer build\n> instructions into one location in the Makefile.\n\nLooks good to me. Will replace and let's mark it for 'next'.\n\nI do not recall suggesting anything concrete on this one, though ;-)\n\nThanks.\n\n> Suggested-by: Junio C Hamano \n> Helped-by: Jeff King \n> Signed-off-by: Josh Steadmon \n> ---\n> Changes in V3:\n> * Dropped CI config patch; no longer needed since we don't use CXX in\n> fuzzer build rules anymore\n"}]}