From: Jeff King <peff@peff•net>
To: Josh Steadmon <steadmon@google•com>
Cc: Junio C Hamano <gitster@pobox•com>,
git@vger•kernel.org, James Mills <prologic@shortcircuit•net.au>
Subject: Re: Git v2.46.0 and --allow-multiple-definition linker flag
Date: Tue, 14 Jan 2025 06:03:23 -0500 [thread overview]
Message-ID: <20250114110323.GD882468@coredump.intra.peff.net> (raw)
In-Reply-To: <egtxf4f3dufiz56g276lt4qtediarj5kkuqbv222edrwcgf5dk@ocnbky74w3tv>
On Mon, Jan 13, 2025 at 09:54:07AM -0800, Josh Steadmon wrote:
> 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
prev parent reply other threads:[~2025-01-14 11:03 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-12-21 23:23 Git v2.46.0 and --allow-multiple-definition linker flag James Mills
2024-12-22 2:30 ` Junio C Hamano
2024-12-22 2:47 ` Junio C Hamano
2025-01-13 17:54 ` Josh Steadmon
2025-01-14 11:03 ` Jeff King [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250114110323.GD882468@coredump.intra.peff.net \
--to=peff@peff$(echo .)net \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=prologic@shortcircuit$(echo .)net.au \
--cc=steadmon@google$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox