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 <[email protected]>
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)
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 <[email protected]>
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; \
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
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 <[email protected]>
Acked-by: Peter Zijlstra <[email protected]>
I like this, as you pointed out, its not always obvious which condition
is the offending one, this makes it more clear.
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
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
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
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
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
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