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

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

[James Mills]

Hi Folks 👋

Been a while since I've emailed one of the large(r) open source lists,
so please excuse me I may be a bit rusty.

For some years now I've been building a very tiny custom LInux
distribution called µLInux (micro Linux) over at
https://git.mills.io/prologic/ulinux -- I primarily target the x86_64
architecture and its intended use is mostly as lightweight VM(s) that
boot really quickly but are otherwise populated with "just enough"
UNIX tools, a small package manager and few other bits to get
interesting things going.

Today I noticed that when upgrading the zlib (✅) and git (❌) ports
that the latest version of Git v2.47.2 does not link and I observe the
following linker error, mostly what looks like a missing
capability/flag of the TinyC compiler that is used as the linker
(LD=tcc):

tcc: error: unsupported linker option '--allow-multiple-definition'

I bisected the Git releases and traced the introduction of this new
flag to v2.46.0

I can't find any details of this flag really or when this was
introduced in the GNU binutils and so far I haven't asked if the Tiny
C devs intend to support this option (yet).

Just wondering, can we consider backing this out? From my very brief
research on the web this seems to allow the linker to permit multiple
definitions of a function. I'm not even sure why that would be
desirable to be honest (but I haven't been a C/C++ developer basically
ever, I know enough to be dangerous!)

Kind regards

James

-- James Mills / prologic

Join Yarn.social today! The only decentralised social media that
respects your privacy and freedoms!

E:    prologic@shortcircuit.net.au
W:    prologic.shortcircuit.net.au
Blog: Read my Blog
Yarn: @prologic@twtxt.net

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

[Junio C Hamano]

James Mills <prologic@shortcircuit.net.au> writes:

> tcc: error: unsupported linker option '--allow-multiple-definition'
>
> I bisected the Git releases and traced the introduction of this new
> flag to v2.46.0
>
> I can't find any details of this flag really or when this was
> introduced in the GNU binutils and so far I haven't asked if the Tiny
> C devs intend to support this option (yet).

Would

    $ make LINK_FUZZ_PROGRAMS=""

help?

The platform-specific tweak defined in config.mak.uname file assumes
that you have glibc plus gcc or clang with usual binutils niceties
once you claim that you are Linux.  It lumps all different variants
of Linux into a single ball of wax and defines LINK_FUZZ_PROGRAMS
Makefile macro, which is a bit unfortunate.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> James Mills <prologic@shortcircuit.net.au> writes:
>
>> tcc: error: unsupported linker option '--allow-multiple-definition'
>>
>> I bisected the Git releases and traced the introduction of this new
>> flag to v2.46.0
>>
>> I can't find any details of this flag really or when this was
>> introduced in the GNU binutils and so far I haven't asked if the Tiny
>> C devs intend to support this option (yet).
>
> Would
>
>     $ make LINK_FUZZ_PROGRAMS=""
>
> help?
>
> The platform-specific tweak defined in config.mak.uname file assumes
> that you have glibc plus gcc or clang with usual binutils niceties
> once you claim that you are Linux.  It lumps all different variants
> of Linux into a single ball of wax and defines LINK_FUZZ_PROGRAMS
> Makefile macro, which is a bit unfortunate.

Having said that, I am not sure if the commit that introduced the
fuzz-all linkage rules, which is 8b9a42bf (fuzz: fix fuzz test build
rules, 2024-01-19), needed to do so in the first place.  With this
trivial patch at the end of the message applied on top of 8b9a42bf
(or more recent 'master', for that matter), "make fuzz-all" seems to
link things just fine.

Note that "make CC=clang fuzz-all" in the older code used to fail
due to something else from gcc+ (which recent build no longer makes
mandatory), but in today's code with gcc/clang/ld available locally,
the linker flag does not seem to be needed for "make fuzz-all".

Josh, do you offhand know what made us add the option?

Thanks.

diff --git c/Makefile w/Makefile
index c0cbed69d8..5af8935968 100644
--- c/Makefile
+++ w/Makefile
@@ -3848,7 +3848,6 @@ FUZZ_CXXFLAGS ?= $(ALL_CFLAGS)
 
 $(FUZZ_PROGRAMS): %: %.o oss-fuzz/dummy-cmd-main.o $(GITLIBS) GIT-LDFLAGS
 	$(QUIET_LINK)$(CXX) $(FUZZ_CXXFLAGS) -o $@ $(ALL_LDFLAGS) \
-		-Wl,--allow-multiple-definition \
 		$(filter %.o,$^) $(filter %.a,$^) $(LIBS) $(LIB_FUZZING_ENGINE)
 
 fuzz-all: $(FUZZ_PROGRAMS)

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

[Josh Steadmon]

On 2024.12.21 18:47, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > James Mills <prologic@shortcircuit.net.au> writes:
> >
> >> tcc: error: unsupported linker option '--allow-multiple-definition'
> >>
> >> I bisected the Git releases and traced the introduction of this new
> >> flag to v2.46.0
> >>
> >> I can't find any details of this flag really or when this was
> >> introduced in the GNU binutils and so far I haven't asked if the Tiny
> >> C devs intend to support this option (yet).
> >
> > Would
> >
> >     $ make LINK_FUZZ_PROGRAMS=""
> >
> > help?
> >
> > The platform-specific tweak defined in config.mak.uname file assumes
> > that you have glibc plus gcc or clang with usual binutils niceties
> > once you claim that you are Linux.  It lumps all different variants
> > of Linux into a single ball of wax and defines LINK_FUZZ_PROGRAMS
> > Makefile macro, which is a bit unfortunate.
> 
> Having said that, I am not sure if the commit that introduced the
> fuzz-all linkage rules, which is 8b9a42bf (fuzz: fix fuzz test build
> rules, 2024-01-19), needed to do so in the first place.  With this
> trivial patch at the end of the message applied on top of 8b9a42bf
> (or more recent 'master', for that matter), "make fuzz-all" seems to
> link things just fine.
> 
> Note that "make CC=clang fuzz-all" in the older code used to fail
> due to something else from gcc+ (which recent build no longer makes
> mandatory), but in today's code with gcc/clang/ld available locally,
> the linker flag does not seem to be needed for "make fuzz-all".
> 
> Josh, do you offhand know what made us add the option?
> 
> Thanks.
> 
> diff --git c/Makefile w/Makefile
> index c0cbed69d8..5af8935968 100644
> --- c/Makefile
> +++ w/Makefile
> @@ -3848,7 +3848,6 @@ FUZZ_CXXFLAGS ?= $(ALL_CFLAGS)
>  
>  $(FUZZ_PROGRAMS): %: %.o oss-fuzz/dummy-cmd-main.o $(GITLIBS) GIT-LDFLAGS
>  	$(QUIET_LINK)$(CXX) $(FUZZ_CXXFLAGS) -o $@ $(ALL_LDFLAGS) \
> -		-Wl,--allow-multiple-definition \
>  		$(filter %.o,$^) $(filter %.a,$^) $(LIBS) $(LIB_FUZZING_ENGINE)
>  
>  fuzz-all: $(FUZZ_PROGRAMS)

Sorry for the delayed response, I'm finally back to work after holidays
/ sick leave.

Unfortunately, the --allow-multiple-definitions flag is still required.

When simply running "make fuzz-all" with no other args (with or without
the patch above), we do not actually produce a working fuzzer:

$ make clean
$ make fuzz-all
$ ./oss-fuzz/fuzz-date
BUG: oss-fuzz/dummy-cmd-main.c:12: We should not execute cmd_main() from a fuzz target
zsh: IOT instruction  ./oss-fuzz/fuzz-date

This is because we're linking with common-main.o and
oss-fuzz/dummy-cmd-main.o. commain-main.o:main() calls cmd_main(), which
it finds in oss-fuzz/dummy-cmd-main.o. If we instead build with a
fuzzing engine enabled:

$ make clean
$ make CC=clang FUZZ_CXX=clang++ \
     CFLAGS="-fsanitize=fuzzer-no-link,address" \
     LIB_FUZZING_ENGINE="-fsanitize=fuzzer,address" \
     fuzz-all
$ ./oss-fuzz/fuzz-date
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 2884061060
INFO: Loaded 1 modules   (55020 inline 8-bit counters): 55020 [0x562aff0d2840, 0x562aff0dff2c),
INFO: Loaded 1 PC tables (55020 PCs): 55020 [0x562aff0dff30,0x562aff1b6df0),
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
INFO: A corpus is not provided, starting from an empty corpus
[...]


This works because the fuzzing engine provides its own main() which
overrides ours, and so we never end up calling
dummy-cmd-main.o:cmd_main().

However, that build will fail if we remove the
--allow-multiple-definition flag:

$ make clean
$ make CC=clang FUZZ_CXX=clang++ \
     CFLAGS="-fsanitize=fuzzer-no-link,address" \
     LIB_FUZZING_ENGINE="-fsanitize=fuzzer,address" \
     fuzz-all
[...]
    LINK oss-fuzz/fuzz-commit-graph
/usr/bin/ld: common-main.o: in function `main':
common-main.c:(.text.main[main]+0x0): multiple definition of `main'; /usr/lib/llvm-16/lib/clang/16/lib/linux/libclang_rt.fuzzer-x86_64.a(FuzzerMain.cpp.o):(.text.main+0x0): first defined here
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [Makefile:3890: oss-fuzz/fuzz-commit-graph] Error 1


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?

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

[Jeff King]

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