Skip site navigation (1)Skip section navigation (2)
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>