From owner-freebsd-arch@FreeBSD.ORG Mon Feb 11 16:39:39 2013 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 2EFB7806 for ; Mon, 11 Feb 2013 16:39:39 +0000 (UTC) (envelope-from christoph.mallon@gmx.de) Received: from mout.gmx.net (mout.gmx.net [212.227.17.21]) by mx1.freebsd.org (Postfix) with ESMTP id C8BD2DB7 for ; Mon, 11 Feb 2013 16:39:38 +0000 (UTC) Received: from mailout-de.gmx.net ([10.1.76.1]) by mrigmx.server.lan (mrigmx002) with ESMTP (Nemesis) id 0MXkSn-1USOYt0G0t-00WmKw for ; Mon, 11 Feb 2013 17:39:32 +0100 Received: (qmail invoked by alias); 11 Feb 2013 16:39:31 -0000 Received: from p5B1326F3.dip.t-dialin.net (EHLO rotluchs.lokal) [91.19.38.243] by mail.gmx.net (mp001) with SMTP; 11 Feb 2013 17:39:31 +0100 X-Authenticated: #1673122 X-Provags-ID: V01U2FsdGVkX1+VRTROvhuV9XQxsrIqt2WXzKyx+4BwQca3OhgcCp vSW7iCDZjClOTE Message-ID: <51191EC1.9000303@gmx.de> Date: Mon, 11 Feb 2013 17:39:29 +0100 From: Christoph Mallon User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:17.0) Gecko/20130129 Thunderbird/17.0.2 MIME-Version: 1.0 To: John Baldwin Subject: Re: Proposal: Unify printing the function name in panic messages() References: <51141E33.4080103@gmx.de> <511622A2.2090601@FreeBSD.org> <5118C4E4.6020706@gmx.de> <201302111105.58186.jhb@freebsd.org> In-Reply-To: <201302111105.58186.jhb@freebsd.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Y-GMX-Trusted: 0 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 16:39:39 -0000 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