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

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

quoted

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