Proposal: Unify printing the function name in panic messages()

Christoph Mallon christoph.mallon at gmx.de
Mon Feb 11 16:39:39 UTC 2013


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.

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

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

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

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

	Christoph


More information about the freebsd-arch mailing list