Date: Mon, 11 Feb 2013 12:03:19 -0500 From: John Baldwin <jhb@freebsd.org> To: Christoph Mallon <christoph.mallon@gmx.de> Cc: Kirk McKusick <mckusick@mckusick.com>, Andriy Gapon <avg@freebsd.org>, freebsd-arch@freebsd.org Subject: Re: Proposal: Unify printing the function name in panic messages() Message-ID: <201302111203.19422.jhb@freebsd.org> In-Reply-To: <51191EC1.9000303@gmx.de> References: <51141E33.4080103@gmx.de> <201302111105.58186.jhb@freebsd.org> <51191EC1.9000303@gmx.de>
next in thread | previous in thread | raw e-mail | index | archive | help
On Monday, February 11, 2013 11:39:29 am Christoph Mallon wrote:
> On 11.02.2013 17:05, John Baldwin wrote:
> > 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_
>
> I explained the reasons already multiple times.
> For example directly below the line the only line of my text, which you cited.
I have yet to see a single specific instance where _you_ have been unable to
debug a panic due to the function name not being present or not matching. It
is all hypothetical so far.
> > 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.
>
> This was already handled:
> Just do not set NAMED_PANIC.
Then it cannot be set by default (regression tests should work out of the box),
so it won't make any difference for users reporting bugs, so why have it?
> > 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
>
> KASSERT? My script? I never did a script for KASSERT.
Then it's a broken script as that is a large number of panic invocations. :)
If you haven't even looked at the KASSERT()'s in the tree then you haven't
really looked at the range of panic messages used.
> > 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
>
> For these extremely rare cases, you can still directly call panic().
Not if you redefine 'panic()' to always include the prefix as your most
recent e-mails suggest you will do based on feedback from Alfred.
> The vast majority simply wants the name of the current function.
> Also, for the mutex stuff a lot of pointless indirection seems to be going on:
> E.g. the macro _mtx_unlock_flags and several of its brothers only have a single user each.
They are not pointless. It is a matter of providing a stable ABI for
modules while still inlining in the kernel (in the non-debug case).
> > every panic that is part of a macro where the macro is the public API and
> > __func__ is not relevant.
>
> Macros using panic already use __func__ (or worse: __FUNCTION__) or do not print a name currently.
Not if they use an explicit function name that you deem to be a "wrong" function
name. That is the case you will break. That is, if your script were fixed to
handle KASSERT, would it not "fix" the above panic to use the internal function
name thus making it less useful for developers as I asserted? Can't you see that
this is a flaw in your reasoning where you assume the raw function name is always
the correct prefix, but in fact it may not be?
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201302111203.19422.jhb>