From owner-freebsd-arch@FreeBSD.ORG Mon Feb 11 21:12:09 2013 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id D243A8F2; Mon, 11 Feb 2013 21:12:09 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigknife-pt.tunnel.tserv9.chi1.ipv6.he.net [IPv6:2001:470:1f10:75::2]) by mx1.freebsd.org (Postfix) with ESMTP id 7C259E63; Mon, 11 Feb 2013 21:12:09 +0000 (UTC) Received: from pakbsde14.localnet (unknown [38.105.238.108]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 79576B9A6; Mon, 11 Feb 2013 16:12:08 -0500 (EST) From: John Baldwin To: Christoph Mallon Subject: Re: Proposal: Unify printing the function name in panic messages() Date: Mon, 11 Feb 2013 12:03:19 -0500 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p22; KDE/4.5.5; amd64; ; ) References: <51141E33.4080103@gmx.de> <201302111105.58186.jhb@freebsd.org> <51191EC1.9000303@gmx.de> In-Reply-To: <51191EC1.9000303@gmx.de> MIME-Version: 1.0 Message-Id: <201302111203.19422.jhb@freebsd.org> Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Mon, 11 Feb 2013 16:12:08 -0500 (EST) Cc: Kirk McKusick , Andriy Gapon , freebsd-arch@freebsd.org X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 11 Feb 2013 21:12:09 -0000 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