Re: [PATCH] usage: trace2 BUG() invocations

From: Jeff King <peff@peff.net>
Date: Feb 5, 2021, 13:44 UTC
Message-ID: <YB1LsacCd4au0P5z@coredump.intra.peff.net>
In-Reply-To: <1051d473-5d1b-1155-8d9e-93eb2cc349f0@gmail.com>

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

Show 14 quoted lines

> > 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.

Show 5 quoted lines

> 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