Re: [PATCH v3 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 7, 2022, 21:12 UTC
Message-ID: <220607.8635ggupws.gmgdl@evledraar.gmail.com>
In-Reply-To: <Yp+wjCPhqieTku3X@google.com>

On Tue, Jun 07 2022, Josh Steadmon wrote:

Show 48 quoted lines

> On 2022.06.02 14:25, Ævar Arnfjörð Bjarmason wrote:
>> Change the exit() wrapper added in ee4512ed481 (trace2: create new
>> combined trace facility, 2019-02-22) so that we'll split up the trace2
>> logging concerns from wanting to wrap the "exit()" function itself for
>> other purposes.
>> 
>> This makes more sense structurally, as we won't seem to conflate
>> non-trace2 behavior with the trace2 code. I'd previously added an
>> explanation for this in 368b5843158 (common-main.c: call exit(), don't
>> return, 2021-12-07), that comment is being adjusted here.
>> 
>> Now the only thing we'll do if we're not using trace2 is to truncate
>> the "code" argument to the lowest 8 bits.
>> 
>> We only need to do that truncation on non-POSIX systems, but in
>> ee4512ed481 that "if defined(__MINGW32__)" code added in
>> 47e3de0e796 (MinGW: truncate exit()'s argument to lowest 8 bits,
>> 2009-07-05) was made to run everywhere. It might be good for clarify
>> to narrow that down by an "ifdef" again, but I'm not certain that in
>> the interim we haven't had some other non-POSIX systems rely the
>> behavior. On a POSIX system taking the lowest 8 bits is implicit, see
>> exit(3)[1] and wait(2)[2]. Let's leave a comment about that instead.
>> 
>> 1. https://man7.org/linux/man-pages/man3/exit.3.html
>> 2. https://man7.org/linux/man-pages/man2/wait.2.html
>> 
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  common-main.c     | 20 ++++++++++++++++----
>>  git-compat-util.h |  4 ++--
>>  trace2.c          |  8 ++------
>>  trace2.h          |  8 +-------
>>  4 files changed, 21 insertions(+), 19 deletions(-)
>
> Just realized that this unexpectedly breaks the fuzzer builds:
>
> $ git checkout ab/bug-if-bug
> $ make CC=clang CXX=clang++ \
>    CFLAGS="-fsanitize=fuzzer-no-link,address" \
>    LIB_FUZZING_ENGINE="-fsanitize=fuzzer" \
>    fuzz-all
>
> [...]
>     LINK fuzz-pack-headers
>     LINK fuzz-pack-idx
> /usr/bin/ld: archive.o: in function `parse_archive_args':
> archive.c:(.text.parse_archive_args[parse_archive_args]+0x2cb8): undefined reference to `common_exit'
> [...]

Hrm, that's a pain, sorry about that.

> The issue is that we don't link the fuzzers against common-main.o
> because the fuzzing engine provides its own main(). Would it be too much
> of a pain to move this out to a new common-exit.c file?

I could do that, I actually did that in a WIP copy, but figured it was too much boilerplate to pass the list.

But why it is that we can't just link to common-main.o? Yeah the linker error, but we already depend on modern clang here, so can't we just use -Wl,--allow-multiple-definition?

This works for me with my local clang & GNU ld:

    make CC=clang CXX=clang++ \
	CFLAGS="-fsanitize=fuzzer-no-link,address" \
        FUZZ_CXXFLAGS="-fsanitize=fuzzer-no-link,address -Wl,--allow-multiple-definition" \
	LIB_FUZZING_ENGINE="common-main.o -fsanitize=fuzzer" fuzz-all

It seems to me that the $(FUZZ_PROGRAMS) rule is mostly trying to work around that by size, i.e. re-declaring most of $(OBJECTS).

But maybe it's a no-go for some reason, maybe due to OSX ld?