| Message ID | RFC-cover-0.3-00000000000-20220525T234908Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | trace2: log "signal" end events if we invoke BUG() | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > (depending on the TZ) that he'd encountered trace2 event streams > without an "exit" event, which as discussed can be seen from usage.c > is due to our invoking of abort() there. Josh, is this related to the "in rare cases" thing we discussed on the "run-command: don't spam trace2_child_exit()" thread? https://lore.kernel.org/git/4616d09ffa632bd2c9e308a713c4bdf2a1328c3c.1651179450.git.steadmon@google.com/
On 2022.05.25 18:20, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > > > (depending on the TZ) that he'd encountered trace2 event streams > > without an "exit" event, which as discussed can be seen from usage.c > > is due to our invoking of abort() there. > > Josh, is this related to the "in rare cases" thing we discussed on > the "run-command: don't spam trace2_child_exit()" thread? > > https://lore.kernel.org/git/4616d09ffa632bd2c9e308a713c4bdf2a1328c3c.1651179450.git.steadmon@google.com/ > No, this is a much more frequent issue than the repeated "child_exit" events from that thread. Any time Git exits without calling our exit() wrapper, we don't log any "exit" events (and presumably skip "atexit" as well).
Josh Steadmon noted in the Review Club tonight/today/yesterday (depending on the TZ) that he'd encountered trace2 event streams without an "exit" event, which as discussed can be seen from usage.c is due to our invoking of abort() there. This RFC series aims to rectify that, first we back out of test-tool specific behavior for BUG() in 1/3, and in 3/3 define abort() to be a wrapper macro like what we do for exit() already. I'm not sure about the direction of emitting such a "signal" event, should we just "fake it" and emit an "exit" event? Probably not, but I'm not confident in that. For one is the exit code (as in what a shell would get from $?) when we're killed with SIGABRT guaranteed to be portable? But hopefully this gets the ball rolling on a good fix for this by starting a discussion about what we should be doing here. One alternate way of dealing with this would be to first split up the "error" event so that we don't emit "error" for all of BUG(), die(), error() and warning(), and to instead split those up into "BUG", "die", "error", "warning". Once we did that we could pretty much declare that the current behavior before this series is a feature, and that anyone parsing trace2 output needs to deal with the stream potentially ending in a "BUG" event. Any such event parser will need to deal with at least: "start" -> ["exit" | "signal"] (with this series) So perhaps it's OK to simply say that consumers should expect? # Or "error" now, but then you can't distinguish "BUG()" from the # rest. "start" -> ["exit" | "BUG"] I think I like that less, but it's worth pointing out as an alternative. The implementation would certainly be smaller than this proposed RFC. Ævar Arnfjörð Bjarmason (3): test-tool: don't fake up BUG() exits as code 99 refs API: rename "abort" callback to avoid macro clash trace2: emit "signal" events after calling BUG() Documentation/technical/api-trace2.txt | 13 +++++++++---- git-compat-util.h | 9 +++++++++ refs/debug.c | 4 ++-- refs/files-backend.c | 4 ++-- refs/iterator.c | 8 ++++---- refs/packed-backend.c | 2 +- refs/ref-cache.c | 2 +- refs/refs-internal.h | 2 +- t/helper/test-tool.c | 1 - t/t0210-trace2-normal.sh | 5 ++--- t/t1406-submodule-ref-store.sh | 10 +++++----- t/test-lib-functions.sh | 13 +++++++++++++ trace2.c | 19 +++++++++++++++++++ trace2.h | 8 ++++++++ trace2/tr2_tgt.h | 3 +++ trace2/tr2_tgt_event.c | 11 +++++++++-- trace2/tr2_tgt_normal.c | 11 +++++++++-- trace2/tr2_tgt_perf.c | 11 +++++++++-- usage.c | 5 ----- 19 files changed, 106 insertions(+), 35 deletions(-)