Date: Mon, 11 Feb 2013 11:05:57 -0500 From: John Baldwin <jhb@freebsd.org> To: freebsd-arch@freebsd.org Cc: Kirk McKusick <mckusick@mckusick.com>, Christoph Mallon <christoph.mallon@gmx.de>, Andriy Gapon <avg@freebsd.org> Subject: Re: Proposal: Unify printing the function name in panic messages() Message-ID: <201302111105.58186.jhb@freebsd.org> In-Reply-To: <5118C4E4.6020706@gmx.de> References: <51141E33.4080103@gmx.de> <511622A2.2090601@FreeBSD.org> <5118C4E4.6020706@gmx.de>
next in thread | previous in thread | raw e-mail | index | archive | help
On Monday, February 11, 2013 5:16:04 am Christoph Mallon wrote:
> On 09.02.2013 11:19, Andriy Gapon wrote:
> > on 09/02/2013 11:28 Christoph Mallon said the following:
> >> I do not understand, what the problem is.
> >> There are bugs and cumbersome code.
> >> This simple changes solves it.
> >
> > You conveniently omitted some questions of mine.
> > I'll reproduce them:
> > Well, have you experienced any problems with debugging due to those
> > (absent/misleading) function names? Or do you see many reports about such problems?
>
> I have dropped them, because they are not relevant.
Eh, showing that there is an actual problem being solved rather than making a
change gratuitously is highly relevant. You are the one arguing for a change
so you have to make the case for why it is a good one. I for one would _really_
not like it if you are magically changing the format string for panics. I am
in the process of writing in-kernel regression tests that rely on being able to
"catch" panics via a modified try-catch model where the catches do simple
string compares on the panic message. Adding a silent ("not visible in the
code") prefix to each panic message makes this process more error prone.
A sample test to show what I mean:
/* Tests for spin mutexes. */
static int
spin_recurse(void)
{
struct mtx m;
bzero(&m, sizeof(m));
mtx_init(&m, "test spin", NULL, MTX_SPIN | MTX_RECURSE);
mtx_lock_spin(&m);
mtx_lock_spin(&m);
mtx_unlock_spin(&m);
mtx_unlock_spin(&m);
mtx_destroy(&m);
mtx_init(&m, "test spin", NULL, MTX_SPIN);
mtx_lock_spin(&m);
PANIC_TRY {
mtx_lock_spin(&m);
} PANIC_CATCH {
IGNORE_PANIC_STARTS_WITH(
"mtx_lock_spin: recursed on non-recursive mutex");
} PANIC_END;
mtx_unlock_spin(&m);
mtx_destroy(&m);
return (TEST_OK);
}
UNIT_TEST("spin lock recurse", spin_recurse);
Also, this is a case where your script will claim that the KASSERT() is
"wrong" but in actual fact it is correct. The programmer is using
mtx_lock_spin() and expects to see that in a panic message not
__mtx_lock_spin_flags(). Only someone working with the implementation of
spin locks should have to know about __mtx_lock_spin_flags(). For someone
working on a device driver or other code, using "mtx_lock_spin" is more
correct and far more useful. IOW, your changes will break just about
every panic that is part of a macro where the macro is the public API and
__func__ is not relevant.
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201302111105.58186.jhb>