git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Derrick Stolee <stolee@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	Jonathan Tan <jonathantanmy@google.com>,
	git@vger.kernel.org, Jeff Hostetler <jeffhost@microsoft.com>
Subject: Re: [PATCH] usage: trace2 BUG() invocations
Date: Fri, 5 Feb 2021 08:44:17 -0500	[thread overview]
Message-ID: <YB1LsacCd4au0P5z@coredump.intra.peff.net> (raw)
In-Reply-To: <1051d473-5d1b-1155-8d9e-93eb2cc349f0@gmail.com>

On Fri, Feb 05, 2021 at 07:51:09AM -0500, Derrick Stolee wrote:

> > We've had a similar problem on the die() side in the past, and solved it
> > with a recursion flag. But note it gets a bit non-trivial in the face of
> > threads. There's some discussion in 1ece66bc9e (run-command: use
> > thread-aware die_is_recursing routine, 2013-04-16).
> > 
> > That commit talks about a case where "die()" in a thread takes down the
> > thread but not the whole process. That wouldn't be true here (we'd
> > expect BUG() to take everything down). So a single counter might be OK
> > in practice, though I suspect we could trigger the problem racily
> > Likewise this is probably a lurking problem when other threaded code
> > calls die(), but we just don't do that often enough for anybody to have
> > noticed.
> 
> Would a simple "BUG() has been called" static suffice?

Yeah, perhaps. That's what we started with for die(), and what we
replaced in the commit I mentioned when it became a problem with
threads. But we might be able to get away with it here.

> so the trace2 call would want to be as close to the abort as
> possible to avoid a silent failure. So, in the patch...
> [...]
> We would want this vreportf() to be before the call to
> trace2_cmd_error_va(), right?

Yeah, that is definitely preferable. The die-is-recursing logic is bad
for that, and it's annoying. I think it dies with "woah, I'm recursing"
without printing anything, because the recursion happens in the handler
that does the printing. And I suspect nobody bothered to improve it
because the whole point is that this recursing case shouldn't come up.
But if it's easy to make it do the right thing for the BUG() case (and I
think it is), we might as well.

-Peff

  reply	other threads:[~2021-02-05 13:48 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-05  5:49 [PATCH] usage: trace2 BUG() invocations Jonathan Tan
2021-02-05  6:17 ` Junio C Hamano
2021-02-05  9:01   ` Jeff King
2021-02-05 12:51     ` Derrick Stolee
2021-02-05 13:44       ` Jeff King [this message]
2021-02-05 16:34       ` Jeff Hostetler
2021-02-05 20:09 ` [PATCH v2] " Jonathan Tan
2021-02-09 12:03   ` Jeff King
2021-02-09 19:34     ` Jonathan Tan
2021-02-09 21:18       ` Junio C Hamano
2021-02-09 22:15       ` Junio C Hamano
2021-03-27 17:56   ` Ævar Arnfjörð Bjarmason

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YB1LsacCd4au0P5z@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jeffhost@microsoft.com \
    --cc=jonathantanmy@google.com \
    --cc=stolee@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).