On 2022.06.07 23:12, Ævar Arnfjörð Bjarmason wrote: > > On Tue, Jun 07 2022, Josh Steadmon wrote: > > > 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@xxxxxxxxx> > >> --- > >> 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? Ah yeah, that would probably work. The main concern is that we want these to run via OSS-Fuzz[1], but I believe I can send a change to our build script there to add that flag. Thanks for the suggestion! [1]: https://google.github.io/oss-fuzz/
