From owner-freebsd-arch@FreeBSD.ORG Tue Feb 19 16:33:34 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 21783BD5; Tue, 19 Feb 2013 16:33:34 +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 ED26824C; Tue, 19 Feb 2013 16:33:33 +0000 (UTC) Received: from pakbsde14.localnet (unknown [38.105.238.108]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 5D22EB943; Tue, 19 Feb 2013 11:33:33 -0500 (EST) From: John Baldwin To: freebsd-arch@freebsd.org Subject: Re: Proposal: Unify printing the function name in panic messages() Date: Tue, 19 Feb 2013 11:33:08 -0500 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p25; KDE/4.5.5; amd64; ; ) References: <51141E33.4080103@gmx.de> <51194D73.2010804@FreeBSD.org> In-Reply-To: <51194D73.2010804@FreeBSD.org> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201302191133.08938.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Tue, 19 Feb 2013 11:33:33 -0500 (EST) Cc: Kirk McKusick , Christoph Mallon , Garance A Drosehn 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: Tue, 19 Feb 2013 16:33:34 -0000 On Monday, February 11, 2013 2:58:43 pm Garance A Drosehn wrote: > On 2/7/13 4:35 PM, Christoph Mallon wrote: > > > > currently the style for printing panic messages varies greatly. > > Here are some examples of the most common styles: > > panic("just a message"); > > panic("function_name: message"); > > panic("wrong_function_name: message"); > > panic("%s: message", __func__); > > Especially the third -- a wrong function name being shown -- is annoying > > and quite common. I propose a simple macro, which wraps panic() and > > ensures that the right name is shown always by using __func__. > > > Do you have suggestions to improve this proposal? > > > > Chrsopth > > While I'm replying to the first message in the thread, I've read > most of the messages up to this point. I do very little development > in the kernel, but as it happens I've spent the last three weeks > untangling a somewhat large C-based project which has similar > messages, and due to the nature of the project it's nearly impossible > to run debuggers on it. So I can't just pop into the debugger and > get some kind of stack trace. > > That project uses __func__ on some printf()s, and __FILE__ on others. > I found it quite useful there. When tracking down a few specific > bugs there were multiple places which could generate the error > message I was looking for, so I added __LINE__ to those printf()s. > This was helpful, particularly in one case where the entire message > was specified as a constant string in one place, but the same error > *could* be printed out by a different printf which built the message > via format specifiers. So scanning for the message would pick up > the first case as an obvious hit, and never notice the second case. > And, of course, it turned out that the message in my log file was > coming from that second case. > > So maybe it'd be good to have two panic options. One with the > __LINE__ number, and one without. Here's a (possibly) useful suggestion. Rather than having a macro that alters the format string, perhaps change panic() to be this: static void dopanic(const char *func, const char *file, int line, const char *fmt, ...); #define panic(...) do_panic(__func__, __FILE__, __LINE__, __VA_ARGS__) You could then use a runtime sysctl to decide if you want to log the extra metadata along with a panic. Keeping the "raw" format string works better for my testing stuff since I have a hook at the start of panic() and it can use the string without the extra metadata (and even possibly supply the metadata in the 'catch' phase so I can ignore panics based on that metadata. However, one consideration with this is how much bloat comes from having the various __func__ and __FILE__ strings (esp. the latter). You may want to have an option to strip them out (see LOCK_FILE/LOCK_LINE where we use NULL instead in production kernels when calling locking primitive routines to avoid the bloat of filenames). Also, if you want to use __FILE__ at all you will likely need something akin to 'fixup_filename()' from subr_witness.c. I think that one is tailored more to old style kernel builds and probably doesn't DTRT for the 'make buildkernel' case. It maybe that fixup_filename() should just be basename() at this point. -- John Baldwin