| Message ID | patch-v3-1.6-9c4f8d840e9-20220602T122106Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 19d75948ef7abe9066efa4462108827d70565ae9 |
| Headers | show |
| Series | usage API: add and use a bug() + BUG_if_bug() | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Subject: Re: [PATCH v3 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c
.o, not .c?
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' [...] 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?
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@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?
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@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? 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/
diff --git a/common-main.c b/common-main.c index 29fb7452f8a..b6124dd2c2c 100644 --- a/common-main.c +++ b/common-main.c @@ -55,10 +55,22 @@ int main(int argc, const char **argv) result = cmd_main(argc, argv); + /* Not exit(3), but a wrapper calling our common_exit() */ + exit(result); +} + +/* We wrap exit() to call common_exit() in git-compat-util.h */ +int common_exit(const char *file, int line, int code) +{ /* - * We define exit() to call trace2_cmd_exit_fl() in - * git-compat-util.h. Whether we reach this or exit() - * elsewhere we'll always run our trace2 exit handler. + * For non-POSIX systems: Take the lowest 8 bits of the "code" + * to e.g. turn -1 into 255. On a POSIX system this is + * redundant, see exit(3) and wait(2), but as it doesn't harm + * anything there we don't need to guard this with an "ifdef". */ - exit(result); + code &= 0xff; + + trace2_cmd_exit_fl(file, line, code); + + return code; } diff --git a/git-compat-util.h b/git-compat-util.h index 96293b6c43a..1227ff80ca7 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -1451,8 +1451,8 @@ int cmd_main(int, const char **); * Intercept all calls to exit() and route them to trace2 to * optionally emit a message before calling the real exit(). */ -int trace2_cmd_exit_fl(const char *file, int line, int code); -#define exit(code) exit(trace2_cmd_exit_fl(__FILE__, __LINE__, (code))) +int common_exit(const char *file, int line, int code); +#define exit(code) exit(common_exit(__FILE__, __LINE__, (code))) /* * You can mark a stack variable with UNLEAK(var) to avoid it being diff --git a/trace2.c b/trace2.c index e01cf77f1a8..0c0a11e07d5 100644 --- a/trace2.c +++ b/trace2.c @@ -202,17 +202,15 @@ void trace2_cmd_start_fl(const char *file, int line, const char **argv) argv); } -int trace2_cmd_exit_fl(const char *file, int line, int code) +void trace2_cmd_exit_fl(const char *file, int line, int code) { struct tr2_tgt *tgt_j; int j; uint64_t us_now; uint64_t us_elapsed_absolute; - code &= 0xff; - if (!trace2_enabled) - return code; + return; trace_git_fsync_stats(); trace2_collect_process_info(TRACE2_PROCESS_INFO_EXIT); @@ -226,8 +224,6 @@ int trace2_cmd_exit_fl(const char *file, int line, int code) if (tgt_j->pfn_exit_fl) tgt_j->pfn_exit_fl(file, line, us_elapsed_absolute, code); - - return code; } void trace2_cmd_error_va_fl(const char *file, int line, const char *fmt, diff --git a/trace2.h b/trace2.h index 1b109f57d0a..88d906ea830 100644 --- a/trace2.h +++ b/trace2.h @@ -101,14 +101,8 @@ void trace2_cmd_start_fl(const char *file, int line, const char **argv); /* * Emit an 'exit' event. - * - * Write the exit-code that will be passed to exit() or returned - * from main(). - * - * Use this prior to actually calling exit(). - * See "#define exit()" in git-compat-util.h */ -int trace2_cmd_exit_fl(const char *file, int line, int code); +void trace2_cmd_exit_fl(const char *file, int line, int code); #define trace2_cmd_exit(code) (trace2_cmd_exit_fl(__FILE__, __LINE__, (code)))
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(-)