Date: Tue, 19 Feb 2013 13:20:55 -0500 From: Garance A Drosehn <gad@FreeBSD.org> To: John Baldwin <jhb@FreeBSD.org> Cc: Kirk McKusick <mckusick@mckusick.com>, Christoph Mallon <christoph.mallon@gmx.de>, freebsd-arch@FreeBSD.org Subject: Re: Proposal: Unify printing the function name in panic messages() Message-ID: <5123C287.4040201@FreeBSD.org> In-Reply-To: <201302191133.08938.jhb@freebsd.org> References: <51141E33.4080103@gmx.de> <51194D73.2010804@FreeBSD.org> <201302191133.08938.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 2/19/13 11:33 AM, John Baldwin wrote:
> On Monday, February 11, 2013 Garance A Drosehn wrote:
>>
>> 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.
I've tried this, and there's one minor annoyance with it. Let's say
the programmer does:
panic(badaddr: invalid size (%s)", int_size);
The compiler will warn that parameter #5 does not match the format-code,
not parameter #2. In this example it's easy to see the problem, but it
gets more confusing if the call does have a lot of parameters. But in
glancing through the panic() messages in the base source, almost all
of them are so short that this wouldn't be much of an issue.
Yet another alternate tactic would be to have:
static void prepanic1(const char *func);
static void prepanic2(const char *file, int line);
static void prepanic3(const char *file, const char *func, int line);
And then on a per-source-file basis, one could select one of:
#define panic(...) { prepanic1(__func__) ; panic(__VA_ARGS__) }
#define panic(...) { prepanic2(__FILE__, __LINE__) ; \
panic(__VA_ARGS__) }
#define panic(...) { prepanic3(__FILE__, __func__, __LINE__) ; \
panic(__VA_ARGS__) }
One could even just #define panic1(), panic2(), and panic3(), and
then let the programmer decide on a per-call basis how much detail
any given panic message will need. Either way, someone who wanted
to recompile could trim unwanted bloat with very little effort.
Apologies if I seem obsessed about these alternatives. It's just that
I have happened to be working on something similar to this over the
last two weeks. (for a project unreleated to FreeBSD).
> 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.
Hmm. In the places where I've used this, __FILE__ has produced just
the basename of the file. Maybe I'm always compiling things where
'cc' was given just the basename, but I'm pretty sure some of my
things are compiled as "subdir/somefile.c". Quite possible that
I've just been lucky on this!
--
Garance Alistair Drosehn = gad@gilead.netel.rpi.edu
Senior Systems Programmer or gad@freebsd.org
Rensselaer Polytechnic Institute or drosih@rpi.edu
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5123C287.4040201>