[PATCH] more helpful WARN_ON and BUG_ON messages

[PATCH] more helpful WARN_ON and BUG_ON messages

[Eric Sandeen]

After a few bugs I encountered in FC6 in buffer.c, with output like:

Kernel BUG at fs/buffer.c: 2791

where buffer.c contains:

...
        BUG_ON(!buffer_locked(bh));
        BUG_ON(!buffer_mapped(bh));
        BUG_ON(!bh->b_end_io);
...

around line 2790, it's awfully tedious to go get the exact failing kernel tree
just to see -which- BUG_ON was encountered.

Printing out the failing condition as a string would make this more helpful IMHO.

This is mostly just compile-tested... comments?

Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Index: linux-2.6.18/include/asm-generic/bug.h
===================================================================
--- linux-2.6.18.orig/include/asm-generic/bug.h
+++ linux-2.6.18/include/asm-generic/bug.h
@@ -12,13 +12,19 @@
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (unlikely((condition)!=0)) BUG(); } while(0)
+#define BUG_ON(condition) do { \
+	if (unlikely((condition)!=0)) { \
+		printk("BUGging on (%s)\n", #condition); \
+		BUG(); \
+	} \
+} while(0)
 #endif
 
 #ifndef HAVE_ARCH_WARN_ON
 #define WARN_ON(condition) do { \
 	if (unlikely((condition)!=0)) { \
-		printk("BUG: warning at %s:%d/%s()\n", __FILE__, __LINE__, __FUNCTION__); \
+		printk("BUG: warning: (%s) at %s:%d/%s()\n", \
+			#condition, __FILE__, __LINE__, __FUNCTION__); \
 		dump_stack(); \
 	} \
 } while (0)

Re: [PATCH] (update) more helpful WARN_ON and BUG_ON messages

[Eric Sandeen]

Eric Sandeen wrote:

> After a few bugs I encountered in FC6 in buffer.c, with output like:
>
> Kernel BUG at fs/buffer.c: 2791
>
> where buffer.c contains:
>
> ...
>         BUG_ON(!buffer_locked(bh));
>         BUG_ON(!buffer_mapped(bh));
>         BUG_ON(!bh->b_end_io);
> ...
>
> around line 2790, it's awfully tedious to go get the exact failing kernel tree
> just to see -which- BUG_ON was encountered.
>
> Printing out the failing condition as a string would make this more helpful IMHO.
>
> This is mostly just compile-tested... comments?
Whoops, missed WARN_ON_ONCE... thanks Peter.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Index: linux-2.6.18/include/asm-generic/bug.h
===================================================================
--- linux-2.6.18.orig/include/asm-generic/bug.h
+++ linux-2.6.18/include/asm-generic/bug.h
@@ -12,13 +12,19 @@
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (unlikely((condition)!=0)) BUG(); } while(0)
+#define BUG_ON(condition) do { \
+	if (unlikely((condition)!=0)) { \
+		printk("BUGging on (%s)\n", #condition); \
+		BUG(); \
+	} \
+} while(0)
 #endif
 
 #ifndef HAVE_ARCH_WARN_ON
 #define WARN_ON(condition) do { \
 	if (unlikely((condition)!=0)) { \
-		printk("BUG: warning at %s:%d/%s()\n", __FILE__, __LINE__, __FUNCTION__); \
+		printk("BUG: warning: (%s) at %s:%d/%s()\n", \
+			#condition, __FILE__, __LINE__, __FUNCTION__); \
 		dump_stack(); \
 	} \
 } while (0)
@@ -45,7 +51,7 @@
 							\
 	if (unlikely((condition) && __warn_once)) {	\
 		__warn_once = 0;			\
-		WARN_ON(1);				\
+		WARN_ON(condition);			\
 		__ret = 1;				\
 	}						\
 	__ret;						\

Re: [PATCH] (update) more helpful WARN_ON and BUG_ON messages

[Peter Zijlstra]

On Fri, 2006-10-20 at 11:54 -0500, Eric Sandeen wrote:
> Eric Sandeen wrote:
> 
> > After a few bugs I encountered in FC6 in buffer.c, with output like:
> >
> > Kernel BUG at fs/buffer.c: 2791
> >
> > where buffer.c contains:
> >
> > ...
> >         BUG_ON(!buffer_locked(bh));
> >         BUG_ON(!buffer_mapped(bh));
> >         BUG_ON(!bh->b_end_io);
> > ...
> >
> > around line 2790, it's awfully tedious to go get the exact failing kernel tree
> > just to see -which- BUG_ON was encountered.
> >
> > Printing out the failing condition as a string would make this more helpful IMHO.
> >
> > This is mostly just compile-tested... comments?
> Whoops, missed WARN_ON_ONCE... thanks Peter.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>

I like this, as you pointed out, its not always obvious which condition
is the offending one, this makes it more clear.

Re: [PATCH] (update) more helpful WARN_ON and BUG_ON messages

[Eric Sandeen]

Eric Sandeen wrote:

>> Printing out the failing condition as a string would make this more helpful IMHO.
>>
>> This is mostly just compile-tested... comments?

hmm for reference the extra strings add about 16k to my bzImage on
x86_64... I suppose this could be put under CONFIG_DEBUG if that's too
distressing.

-Eric

Re: [PATCH] more helpful WARN_ON and BUG_ON messages

[Jeremy Fitzhardinge]

Eric Sandeen wrote:
> After a few bugs I encountered in FC6 in buffer.c, with output like:
>
> Kernel BUG at fs/buffer.c: 2791
>
> where buffer.c contains:
>
> ...
>         BUG_ON(!buffer_locked(bh));
>         BUG_ON(!buffer_mapped(bh));
>         BUG_ON(!bh->b_end_io);
> ...
>
> around line 2790, it's awfully tedious to go get the exact failing kernel tree
> just to see -which- BUG_ON was encountered.
>
> Printing out the failing condition as a string would make this more helpful IMHO.
>   

This seems like a generally useful idea - certainly more valuable than 
storing+printing the function name.

You might want to look at the BUG patches I wrote, which are currently 
in -mm.  I added general machinery to allow architectures to easily 
implement BUG() efficiently (ie, with a minimal amount of BUG-related 
icache pollution).  If you were to store the BUG_ON expression, it would 
be best to extend struct bug_entry and store it there - doing it in 
asm-generic BUG_ON() means you still end up with code to set up the 
printk in the mainline code path, and it also won't honour 
CONFIG_DEBUG_BUGVERBOSE being disabled.

    J

Re: [PATCH] more helpful WARN_ON and BUG_ON messages

[Eric Sandeen]

Jeremy Fitzhardinge wrote:

> This seems like a generally useful idea - certainly more valuable than 
> storing+printing the function name.
> 
> You might want to look at the BUG patches I wrote, which are currently 
> in -mm.  I added general machinery to allow architectures to easily 
> implement BUG() efficiently (ie, with a minimal amount of BUG-related 
> icache pollution).  If you were to store the BUG_ON expression, it would 
> be best to extend struct bug_entry and store it there - doing it in 
> asm-generic BUG_ON() means you still end up with code to set up the 
> printk in the mainline code path, and it also won't honour 
> CONFIG_DEBUG_BUGVERBOSE being disabled.

Thanks, I missed that change... I'll look into it.

-Eric

Re: [PATCH] more helpful WARN_ON and BUG_ON messages

[Adrian Bunk]

On Fri, Oct 20, 2006 at 11:23:54AM -0500, Eric Sandeen wrote:
> After a few bugs I encountered in FC6 in buffer.c, with output like:
> 
> Kernel BUG at fs/buffer.c: 2791
> 
> where buffer.c contains:
> 
> ...
>         BUG_ON(!buffer_locked(bh));
>         BUG_ON(!buffer_mapped(bh));
>         BUG_ON(!bh->b_end_io);
> ...
> 
> around line 2790, it's awfully tedious to go get the exact failing kernel tree
> just to see -which- BUG_ON was encountered.
> 
> Printing out the failing condition as a string would make this more helpful IMHO.
> 
> This is mostly just compile-tested... comments?
>...

Who really needs this considering it implies a size increase of the 
kernel image?

Using a kernel tree so unusual that you can't locate the source anymore 
sounds like an extremely rare and unintelligent situation, not something 
that must be handled.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] more helpful WARN_ON and BUG_ON messages

[Eric Sandeen]

Adrian Bunk wrote:

> Who really needs this considering it implies a size increase of the 
> kernel image?
> 
> Using a kernel tree so unusual that you can't locate the source anymore 
> sounds like an extremely rare and unintelligent situation, not something 
> that must be handled.

Most debugging code makes the kernel bigger, slower... and easier to
debug, no?

It's not a question of not being -able- to locate sources; it's a
question of being able to look at a bug report and triage it quickly
without digging around to find the kernel du jour that produced it.  *shrug*

-Eric

Re: [PATCH] more helpful WARN_ON and BUG_ON messages

[Adrian Bunk]

On Fri, Oct 20, 2006 at 04:56:37PM -0500, Eric Sandeen wrote:
> Adrian Bunk wrote:
> 
> > Who really needs this considering it implies a size increase of the 
> > kernel image?
> > 
> > Using a kernel tree so unusual that you can't locate the source anymore 
> > sounds like an extremely rare and unintelligent situation, not something 
> > that must be handled.
> 
> Most debugging code makes the kernel bigger, slower... and easier to
> debug, no?
> 
> It's not a question of not being -able- to locate sources; it's a
> question of being able to look at a bug report and triage it quickly
> without digging around to find the kernel du jour that produced it.  *shrug*

It's not that BUGs were that frequent.

And with your suggestion "I suppose this could be put under CONFIG_DEBUG", 
it would anyway be turned off by nearly everyone.

> -Eric

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] more helpful WARN_ON and BUG_ON messages

[Dave Jones]

On Sat, Oct 21, 2006 at 12:07:17AM +0200, Adrian Bunk wrote:

 > > Most debugging code makes the kernel bigger, slower... and easier to
 > > debug, no?
 > > 
 > > It's not a question of not being -able- to locate sources; it's a
 > > question of being able to look at a bug report and triage it quickly
 > > without digging around to find the kernel du jour that produced it.  *shrug*
 > 
 > It's not that BUGs were that frequent.

You're not trying hard enough ;) 

 > And with your suggestion "I suppose this could be put under CONFIG_DEBUG", 
 > it would anyway be turned off by nearly everyone.

For better bug reports, 16K is peanuts. We had exactly the same straw-man
come up when kksymoops was first proposed. And now, most people don't run
without it.

I've seen numerous cases where reporters have hand transcribed BUG() reports,
and got the line numbers wrong because they misremembered a 4 digit number.
Words are inherently easier to remember, and even if typoed, usually there's
enough context to figure out what the problem was without another round-trip
to the bug reporter.

If this were optional, I don't see how anyone can argue against it,
and that should be a trivial improvement to Eric's existing patch.

	Dave

-- 
http://www.codemonkey.org.uk