Re: Git v2.46.0 and --allow-multiple-definition linker flag

From: Jeff King <peff@peff.net>
Date: Jan 14, 2025, 11:03 UTC
Message-ID: <20250114110323.GD882468@coredump.intra.peff.net>
In-Reply-To: <egtxf4f3dufiz56g276lt4qtediarj5kkuqbv222edrwcgf5dk@ocnbky74w3tv>

On Mon, Jan 13, 2025 at 09:54:07AM -0800, Josh Steadmon wrote:

Show 5 quoted lines

> As Junio says, a short term fix would be to build with
> LINK_FUZZ_PROGRAMS="". A better solution would be to make
> config.mak.uname smarter about whether to enable this by default. I see
> that we use "detect-compiler" in config.mak.dev, would it make sense to
> check this in config.mak.uname as well?

It feels like the original sin here is defining main() in our library in the first place, if there are programs that may not want it. But we don't actually put it into libgit.a; the object file is just mentioned in the $(GITLIBS) variable of the Makefile.

We could split that out like this:

diff --git a/Makefile b/Makefile
index 97e8385b66..f098ca5a5c 100644
--- a/Makefile
+++ b/Makefile
@@ -1371,7 +1371,8 @@ UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/lib-oid.o
 UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/lib-reftable.o
 
 # xdiff and reftable libs may in turn depend on what is in libgit.a
-GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(LIB_FILE)
+GITLIBS = $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(LIB_FILE)
+PROGRAM_GITLIBS = common-main.o $(GITLIBS)
 EXTLIBS =
 
 GIT_USER_AGENT = git/$(GIT_VERSION)

and then depend on $(PROGRAM_GITLIBS) as appropriate. Or if oss-fuzz is
the only special case here, then perhaps we could just teach it to
suppress the extra main:

diff --git a/Makefile b/Makefile
index 97e8385b66..06431170de 100644
--- a/Makefile
+++ b/Makefile
@@ -3852,9 +3852,8 @@ FUZZ_CXXFLAGS ?= $(ALL_CFLAGS)
 .PHONY: fuzz-all
 fuzz-all: $(FUZZ_PROGRAMS)
 
-$(FUZZ_PROGRAMS): %: %.o oss-fuzz/dummy-cmd-main.o $(GITLIBS) GIT-LDFLAGS
+$(FUZZ_PROGRAMS): %: %.o $(filter-out common-main.o,$(GITLIBS)) GIT-LDFLAGS
 	$(QUIET_LINK)$(FUZZ_CXX) $(FUZZ_CXXFLAGS) -o $@ $(ALL_LDFLAGS) \
-		-Wl,--allow-multiple-definition \
 		$(filter %.o,$^) $(filter %.a,$^) $(LIBS) $(LIB_FUZZING_ENGINE)
 
 $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_OBJS) \

You'd need two fixes to go with that:

  1. common-main was originally supposed to just be about overriding
     main(). But it has since grown common_exit(), which does not have
     the same linking properties (we override exit() calls at the macro
     layer instead). That should be defined in libgit.a somewhere.

  2. When building the test-compile fuzzers, now you'll need to
     provide an actual main() function. I guess we can determine that by
     the presence of LIB_FUZZING_ENGINE? So maybe:

diff --git a/Makefile b/Makefile
index 97e8385b66..ba3faf9b9e 100644
--- a/Makefile
+++ b/Makefile
@@ -3852,9 +3852,15 @@ FUZZ_CXXFLAGS ?= $(ALL_CFLAGS)
 .PHONY: fuzz-all
 fuzz-all: $(FUZZ_PROGRAMS)
 
-$(FUZZ_PROGRAMS): %: %.o oss-fuzz/dummy-cmd-main.o $(GITLIBS) GIT-LDFLAGS
+ifdef LIB_FUZZING_ENGINE
+# assume the fuzzing engine supplies main()
+FUZZ_GITLIBS = $(filter-out common-main.o, $(GITLIBS))
+else
+FUZZ_GITLIBS = oss-fuzz/dummy-cmd-main.o $(GITLIBS)
+endif
+
+$(FUZZ_PROGRAMS): %: %.o $(FUZZ_GITLIBS) GIT-LDFLAGS
 	$(QUIET_LINK)$(FUZZ_CXX) $(FUZZ_CXXFLAGS) -o $@ $(ALL_LDFLAGS) \
-		-Wl,--allow-multiple-definition \
 		$(filter %.o,$^) $(filter %.a,$^) $(LIBS) $(LIB_FUZZING_ENGINE)
 
 $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_OBJS) \

-Peff