[PATCH] BUG preserve registers

[PATCH] BUG preserve registers

[Hugh Dickins]

It's frustrating that when Verbose BUG() reporting is configured,
info gets lost: fix for i386 below.  This is your area, Andrew:
please confirm to Marcelo if you'd like him to apply this.

Example: in hpa's recent prune_dcache crash, %eax showed the length of
the kernel BUG printk, when we'd have liked to see the invalid d_count:
off-by-one or obviously corrupted?

Hugh

--- 2.4.18-pre9/arch/i386/kernel/entry.S	Thu Feb  7 14:38:06 2002
+++ linux/arch/i386/kernel/entry.S	Fri Feb  8 21:47:39 2002
@@ -132,6 +132,30 @@
 	movl $-8192, reg; \
 	andl %esp, reg
 
+#ifdef CONFIG_DEBUG_BUGVERBOSE
+BUG_format:
+	.asciz "kernel BUG at %s:%d!\n"
+ENTRY(do_BUG)
+	pushfl			# Save flags and registers changed in C
+	pushl %eax
+	pushl %ecx
+	pushl %edx
+	pushl $1
+	call SYMBOL_NAME(bust_spinlocks)
+	movl 28(%esp),%eax
+	movl 24(%esp),%ecx
+	pushl %eax
+	pushl %ecx
+	pushl $BUG_format
+	call SYMBOL_NAME(printk)
+	addl $16,%esp
+	popl %edx		# Restore registers and flags for display
+	popl %ecx
+	popl %eax
+	popfl
+	ret
+#endif /* CONFIG_DEBUG_BUGVERBOSE */
+
 ENTRY(lcall7)
 	pushfl			# We get a different stack layout with call gates,
 	pushl %eax		# which has to be cleaned up later..
--- 2.4.18-pre9/arch/i386/mm/fault.c	Thu Feb  7 14:38:07 2002
+++ linux/arch/i386/mm/fault.c	Fri Feb  8 19:06:45 2002
@@ -125,12 +125,6 @@
 	}
 }
 
-void do_BUG(const char *file, int line)
-{
-	bust_spinlocks(1);
-	printk("kernel BUG at %s:%d!\n", file, line);
-}
-
 asmlinkage void do_invalid_op(struct pt_regs *, unsigned long);
 extern unsigned long idt;
 

Re: [PATCH] BUG preserve registers

[Andrew Morton]

Hugh Dickins wrote:
> 
> It's frustrating that when Verbose BUG() reporting is configured,
> info gets lost: fix for i386 below.  This is your area, Andrew:

Well you know I'm a sucker for debug code.

> please confirm to Marcelo if you'd like him to apply this.

I'd like it.
 
> Example: in hpa's recent prune_dcache crash, %eax showed the length of
> the kernel BUG printk, when we'd have liked to see the invalid d_count:
> off-by-one or obviously corrupted?

Absolutely.
 
> Hugh
> 
> --- 2.4.18-pre9/arch/i386/kernel/entry.S        Thu Feb  7 14:38:06 2002
> +++ linux/arch/i386/kernel/entry.S      Fri Feb  8 21:47:39 2002

The implementation looks fine to me.  You've checked that it
builds OK with CONFIG_DEBUG_BUGVERBOSE=n?

It's a shame that gcc never seems to have gained full support for
attribute __interrupt__, which, if they did it right, would preserve 
all registers.   gcc-for-PPC seems to support it.

-

Re: [PATCH] BUG preserve registers

[Hugh Dickins]

On Sat, 9 Feb 2002, Andrew Morton wrote:
> 
> The implementation looks fine to me.  You've checked that it
> builds OK with CONFIG_DEBUG_BUGVERBOSE=n?

I have checked now, and yes, that builds fine: thanks.

Hugh

Re: [PATCH] BUG preserve registers

[Linus Torvalds]

On Sat, 9 Feb 2002, Hugh Dickins wrote:
>
> It's frustrating that when Verbose BUG() reporting is configured,
> info gets lost: fix for i386 below.  This is your area, Andrew:
> please confirm to Marcelo if you'd like him to apply this.
>
> Example: in hpa's recent prune_dcache crash, %eax showed the length of
> the kernel BUG printk, when we'd have liked to see the invalid d_count:
> off-by-one or obviously corrupted?

Don't do it this way.

Instead, put the string printout in the _trap_ handler, and make the
format of BUG() be something like this:

	#ifdef CONFIG_DEBUG_BUGVERBOSE
	#define BUGSTR "\"" __FILE__ "\""
	#else
	#define BUGSTR ""
	#endif

	#define BUG() \
		asm("ud2\n\t.word __LINE__\n\t.asciiz " BUGSTR)

and then have the trap handler print out the pretty-printed version.

The advantage of this is:
 - much smaller footprint even when messages are enabled.
 - no register corruption
 - easily extendible (ie we can put a "bug code" etc, and have the trap
   handler do different things depending on the code)

Ok?

		Linus

Re: [PATCH] BUG preserve registers

[Andrew Morton]

Linus Torvalds wrote:
> 
> On Sat, 9 Feb 2002, Hugh Dickins wrote:
> >
> > It's frustrating that when Verbose BUG() reporting is configured,
> > info gets lost: fix for i386 below.  This is your area, Andrew:
> > please confirm to Marcelo if you'd like him to apply this.
> >
> > Example: in hpa's recent prune_dcache crash, %eax showed the length of
> > the kernel BUG printk, when we'd have liked to see the invalid d_count:
> > off-by-one or obviously corrupted?
> 
> Don't do it this way.
> 
> Instead, put the string printout in the _trap_ handler, and make the
> format of BUG() be something like this:
> 
>         #ifdef CONFIG_DEBUG_BUGVERBOSE
>         #define BUGSTR "\"" __FILE__ "\""
>         #else
>         #define BUGSTR ""
>         #endif
> 
>         #define BUG() \
>                 asm("ud2\n\t.word __LINE__\n\t.asciiz " BUGSTR)
> 

Is better, except the filename gets expanded multipe times into
the object file.  How about:

#define BUG()                   \
        asm(    "ud2\n"         \
                "\t.word %0\n"  \
                "\t.long %1\n"  \
                 : : "i" (__LINE__), "i" (__FILE__))

void erp(void)
{
        if (c())
                BUG();
        if (d())
                BUG();
}

-

Re: [PATCH] BUG preserve registers

[Linus Torvalds]

On Sat, 9 Feb 2002, Andrew Morton wrote:
> >
>
> Is better, except the filename gets expanded multipe times into
> the object file.  How about:
>
> #define BUG()                   \
>         asm(    "ud2\n"         \
>                 "\t.word %0\n"  \
>                 "\t.long %1\n"  \
>                  : : "i" (__LINE__), "i" (__FILE__))

Even better.

That way you can actually totally remove the "verbose bug" config option,
because even the verbose BUG's aren't actually using up any noticeable
amounts of space.

This is all assuming that gcc doesn't create the string for inline
functions that aren't used, which it probably cannot, so maybe this
doesn't work out.

			Linus

Re: [PATCH] BUG preserve registers

[H. Peter Anvin]

Linus Torvalds wrote:

> 
> On Sat, 9 Feb 2002, Andrew Morton wrote:
> 
>>Is better, except the filename gets expanded multipe times into
>>the object file.  How about:
>>
>>#define BUG()                   \
>>        asm(    "ud2\n"         \
>>                "\t.word %0\n"  \
>>                "\t.long %1\n"  \
>>                 : : "i" (__LINE__), "i" (__FILE__))
>>
> 
> Even better.
> 
> That way you can actually totally remove the "verbose bug" config option,
> because even the verbose BUG's aren't actually using up any noticeable
> amounts of space.
> 
> This is all assuming that gcc doesn't create the string for inline
> functions that aren't used, which it probably cannot, so maybe this
> doesn't work out.
> 


Since gcc wouldn't even *see* a macro it didn't use, I find it hard to 
imagine it would create anything.

However, you really want to do "asm volatile" rather than "asm"...

	-hpa

Re: [PATCH] BUG preserve registers

[Linus Torvalds]

On Sat, 9 Feb 2002, H. Peter Anvin wrote:
> >
> > This is all assuming that gcc doesn't create the string for inline
> > functions that aren't used, which it probably cannot, so maybe this
> > doesn't work out.
>
> Since gcc wouldn't even *see* a macro it didn't use, I find it hard to
> imagine it would create anything.

Oh, but I was talking about the case of the macro being used in an "static
inline" in a header file, and that inline is not actually _used_
anywhere..

> However, you really want to do "asm volatile" rather than "asm"...

Without any inputs and outputs, asms default to volatile, but yes, I
agree. Better make it explicit.

		Linus

Re: [PATCH] BUG preserve registers

[H. Peter Anvin]

Linus Torvalds wrote:

> 
> Oh, but I was talking about the case of the macro being used in an "static
> inline" in a header file, and that inline is not actually _used_
> anywhere..
> 


Ah... but it was talked about in reference to a macro.

Also, since AFAIK BUG() never returns, you may want to have it end with 
for(;;); to let gcc know that any code downstream of a BUG() is dead.

	-hpa

Re: [PATCH] BUG preserve registers

[Andrew Morton]

Linus Torvalds wrote:
> 
> On Sat, 9 Feb 2002, Andrew Morton wrote:
> > >
> >
> > Is better, except the filename gets expanded multipe times into
> > the object file.  How about:
> >
> > #define BUG()                   \
> >         asm(    "ud2\n"         \
> >                 "\t.word %0\n"  \
> >                 "\t.long %1\n"  \
> >                  : : "i" (__LINE__), "i" (__FILE__))
> 
> Even better.
> 
> That way you can actually totally remove the "verbose bug" config option,
> because even the verbose BUG's aren't actually using up any noticeable
> amounts of space.
> 
> This is all assuming that gcc doesn't create the string for inline
> functions that aren't used, which it probably cannot, so maybe this
> doesn't work out.
> 

gcc generally get it wrong - unreferenced strings still appear
in the object code from multiple usage patterns.  I think this
was fixed about six months ago.

But yes, the verbose BUG overhead is now six bytes per BUG, plus
a few bytes per file for the filename.  And it's my opinion that
the non-verbose BUG option is undesirable - it's making the
developers' job harder.  Seems that with this change, the reasons
for CONFIG_DEBUG_BUGVERBOSE are no longer with us, and it can
disappear.

-

Re: [PATCH] BUG preserve registers

[Linus Torvalds]

On Sat, 9 Feb 2002, Andrew Morton wrote:
>
> gcc generally get it wrong - unreferenced strings still appear
> in the object code from multiple usage patterns.  I think this
> was fixed about six months ago.

Ok. If it's already fixed in recent gcc's, and since even a unfixed gcc
should at most cause just one extra string per file, this sounds
acceptable.

> But yes, the verbose BUG overhead is now six bytes per BUG, plus
> a few bytes per file for the filename.  And it's my opinion that
> the non-verbose BUG option is undesirable - it's making the
> developers' job harder.  Seems that with this change, the reasons
> for CONFIG_DEBUG_BUGVERBOSE are no longer with us, and it can
> disappear.

Yes. Make it so.

		Linus

Re: [PATCH] BUG preserve registers

[Andrew Morton]

Linus Torvalds wrote:
> 
> Yes. Make it so.

This works for me, from in-kernel as well as in-module.  It'd
be good if someone more familiar with x86 could check it over.

The ILL_ILLOPN information is not conveniently available within
die(), but I think the opcode parsing here is sufficient?

(There are no kernel source files >65535 lines.  I checked :))


--- linux-2.4.18-pre9/include/asm-i386/page.h	Thu Feb  7 13:04:22 2002
+++ linux-akpm/include/asm-i386/page.h	Sat Feb  9 13:42:24 2002
@@ -91,17 +91,15 @@ typedef struct { unsigned long pgprot; }
 /*
  * Tell the user there is some problem. Beep too, so we can
  * see^H^H^Hhear bugs in early bootup as well!
+ * The offending file and line are encoded after the "officially
+ * undefined" opcode for parsing in the trap handler.
  */
 
-#ifdef CONFIG_DEBUG_BUGVERBOSE
-extern void do_BUG(const char *file, int line);
-#define BUG() do {					\
-	do_BUG(__FILE__, __LINE__);			\
-	__asm__ __volatile__("ud2");			\
-} while (0)
-#else
-#define BUG() __asm__ __volatile__(".byte 0x0f,0x0b")
-#endif
+#define BUG()				\
+ __asm__ __volatile__(	"ud2\n"		\
+			"\t.word %c0\n"	\
+			"\t.long %c1\n"	\
+			 : : "i" (__LINE__), "i" (__FILE__))
 
 #define PAGE_BUG(page) do { \
 	BUG(); \
--- linux-2.4.18-pre9/arch/i386/kernel/traps.c	Sun Sep 30 12:26:08 2001
+++ linux-akpm/arch/i386/kernel/traps.c	Sat Feb  9 13:53:47 2002
@@ -237,6 +237,40 @@ bad:
 	printk("\n");
 }	
 
+static void handle_BUG(struct pt_regs *regs)
+{
+	unsigned short ud2;
+	unsigned short line;
+	char *file;
+	char c;
+	unsigned long eip;
+
+	if (regs->xcs & 3)
+		goto no_bug;		/* Not in kernel */
+
+	eip = regs->eip;
+
+	if (eip < PAGE_OFFSET)
+		goto no_bug;
+	if (__get_user(ud2, (unsigned short *)eip))
+		goto no_bug;
+	if (ud2 != 0x0b0f)
+		goto no_bug;
+	if (__get_user(line, (unsigned short *)(eip + 2)))
+		goto no_bug;
+	if (__get_user(file, (char **)(eip + 4)))
+		goto no_bug;
+	if ((unsigned long)file < PAGE_OFFSET)
+		goto no_bug;
+	if (__get_user(c, file))
+		file = "<bad filename>";
+
+	printk("Kernel BUG at %s:%d\n", file, line);
+
+no_bug:
+	return;
+}
+
 spinlock_t die_lock = SPIN_LOCK_UNLOCKED;
 
 void die(const char * str, struct pt_regs * regs, long err)
@@ -244,6 +278,7 @@ void die(const char * str, struct pt_reg
 	console_verbose();
 	spin_lock_irq(&die_lock);
 	bust_spinlocks(1);
+	handle_BUG(regs);
 	printk("%s: %04lx\n", str, err & 0xffff);
 	show_registers(regs);
 	bust_spinlocks(0);
--- linux-2.4.18-pre9/arch/i386/kernel/i386_ksyms.c	Thu Feb  7 13:04:11 2002
+++ linux-akpm/arch/i386/kernel/i386_ksyms.c	Sat Feb  9 13:42:24 2002
@@ -168,9 +168,5 @@ EXPORT_SYMBOL_NOVERS(memset);
 EXPORT_SYMBOL(atomic_dec_and_lock);
 #endif
 
-#ifdef CONFIG_DEBUG_BUGVERBOSE
-EXPORT_SYMBOL(do_BUG);
-#endif
-
 extern int is_sony_vaio_laptop;
 EXPORT_SYMBOL(is_sony_vaio_laptop);
--- linux-2.4.18-pre9/arch/i386/config.in	Thu Feb  7 13:04:11 2002
+++ linux-akpm/arch/i386/config.in	Sat Feb  9 13:42:24 2002
@@ -421,7 +421,6 @@ if [ "$CONFIG_DEBUG_KERNEL" != "n" ]; th
    bool '  Memory mapped I/O debugging' CONFIG_DEBUG_IOVIRT
    bool '  Magic SysRq key' CONFIG_MAGIC_SYSRQ
    bool '  Spinlock debugging' CONFIG_DEBUG_SPINLOCK
-   bool '  Verbose BUG() reporting (adds 70K)' CONFIG_DEBUG_BUGVERBOSE
 fi
 
 endmenu
--- linux-2.4.18-pre9/arch/i386/mm/fault.c	Thu Feb  7 13:04:11 2002
+++ linux-akpm/arch/i386/mm/fault.c	Sat Feb  9 13:42:24 2002
@@ -125,12 +125,6 @@ void bust_spinlocks(int yes)
 	}
 }
 
-void do_BUG(const char *file, int line)
-{
-	bust_spinlocks(1);
-	printk("kernel BUG at %s:%d!\n", file, line);
-}
-
 asmlinkage void do_invalid_op(struct pt_regs *, unsigned long);
 extern unsigned long idt;

Re: [PATCH] BUG preserve registers

[Alan Cox]

> This works for me, from in-kernel as well as in-module.  It'd
> be good if someone more familiar with x86 could check it over.

This looks a really bad reversion. The CONFIG_DEBUG_BUGVERBOSE ifdef saves
over 70K of memory on my standard kernel build. Putting the file name pointers
back in everywhere is going to put a fair amount of that back except on 
very new gcc's that maybe will do string merging in this case. Have you
verified string merging occurs in gcc 2.95 for __FILE__ string constants
passed into asm blocks ? 

I also don't understand what the problem you are trying to solve is. If you
want to debug the kernel you build with debug verbose. If not you don't.
With the symbol table you can still easily trace down BUG events.

Alan

Re: [PATCH] BUG preserve registers

[Andrew Morton]

Alan Cox wrote:
> 
> > This works for me, from in-kernel as well as in-module.  It'd
> > be good if someone more familiar with x86 could check it over.
> 
> This looks a really bad reversion. The CONFIG_DEBUG_BUGVERBOSE ifdef saves
> over 70K of memory on my standard kernel build.

About the time the 70k claim was made, I moved the printk out-of-line,
so things got not so bad.  However, with my (large) kernel build, on
egcs-1.1.2:

non-verbose BUG:
        2589971  293436  373404 3256811  31b1eb vmlinux
verbose BUG:
        2709055  293436  373404 3375895  338317 vmlinux
Patched:
        2694537  293436  373404 3361377  334a61 vmlinux

Which is 100k, which is preposterous.

This is due to BUG() calls in inline functions in headers.  The biggest
culprit is dget(), in dcache.h.  This causes the full path of the header file
to be expanded into each and every compilation unit which includes
dcache.h.  In my build, it's 25 kilobytes of:

...
/net/akpm-1/usr/src/linux-akpm/include/linux/dcache.h
/net/akpm-1/usr/src/linux-akpm/include/linux/dcache.h                           /net/akpm-1/usr/src/linux-akpm/include/linux/dcache.h
...

I'm showing thirteen header files, for a total of 83k.  I'll do something
about this...

> Putting the file name pointers
> back in everywhere is going to put a fair amount of that back except on
> very new gcc's that maybe will do string merging in this case. Have you
> verified string merging occurs in gcc 2.95 for __FILE__ string constants
> passed into asm blocks ?

Yes, the compiler gets it right.
 
> I also don't understand what the problem you are trying to solve is. If you
> want to debug the kernel you build with debug verbose. If not you don't.
> With the symbol table you can still easily trace down BUG events.
> 

Alan, the thing I really don't like about the config option is
when people come onto this list reporting that they got a "Kernel
BUG" and we simply don't know where it happened.  It's especially
hard when we have a bunch of BUGs near together, such as the
rather popular ones in page_alloc.c

Fixing the above problem should reduce the overhead from 100k down
to 20k.  I'll also change the implementation to

#if 1   /* Set to zero for a slightly smaller kernel */
#define BUG()                           \
 __asm__ __volatile__(  "ud2\n"         \
                        "\t.word %c0\n" \
                        "\t.long %c1\n" \
                         : : "i" (__LINE__), "i" (__FILE__))
#else
#define BUG() __asm__ __volatile__("ud2\n")
#endif

And we can add this to the list of kernel unpiggying tricks
which Lars Brinkhoff developed, which I guess should really
be in the Documentation/ directory somewhere.

But I dislike the config option, because of the information
loss to us.

-

Re: [PATCH] BUG preserve registers

[H. Peter Anvin]

Andrew Morton wrote:

> Alan Cox wrote:
> 
>>>This works for me, from in-kernel as well as in-module.  It'd
>>>be good if someone more familiar with x86 could check it over.
>>>
>>This looks a really bad reversion. The CONFIG_DEBUG_BUGVERBOSE ifdef saves
>>over 70K of memory on my standard kernel build.
>>
> 
> About the time the 70k claim was made, I moved the printk out-of-line,
> so things got not so bad.  However, with my (large) kernel build, on
> egcs-1.1.2:
> 
> non-verbose BUG:
>         2589971  293436  373404 3256811  31b1eb vmlinux
> verbose BUG:
>         2709055  293436  373404 3375895  338317 vmlinux
> Patched:
>         2694537  293436  373404 3361377  334a61 vmlinux
> 
> Which is 100k, which is preposterous.
> 


Use "size" to determine the actual size, or strip the binary.

	-hpa

Re: [PATCH] BUG preserve registers

[Andrew Morton]

"H. Peter Anvin" wrote:
> 
> > non-verbose BUG:
> >         2589971  293436  373404 3256811  31b1eb vmlinux
> > verbose BUG:
> >         2709055  293436  373404 3375895  338317 vmlinux
> > Patched:
> >         2694537  293436  373404 3361377  334a61 vmlinux
> >
> > Which is 100k, which is preposterous.
> >
> 
> Use "size" to determine the actual size, or strip the binary.
> 

That is the output from /usr/bin/size

-

Re: [PATCH] BUG preserve registers

[Linus Torvalds]

On Sat, 9 Feb 2002, Andrew Morton wrote:
>
> This is due to BUG() calls in inline functions in headers.  The biggest
> culprit is dget(), in dcache.h.  This causes the full path of the header file
> to be expanded into each and every compilation unit which includes
> dcache.h.

Hmm. Which brings up another issue: can somebody come up with an idea of
how to make the thing not use the whole pathname, but only the basename
relative to the top-of-tree?

I doubt it is possible, but maybe there is some clever way to avoid it..

> I'm showing thirteen header files, for a total of 83k.  I'll do something
> about this...

Ok, so even your gcc obviously is _not_ intelligent enough to throw away
strings from inline functions that aren't used. Oh well.

Note that the correct thing to do may be to un-inline a lot of things.
We've done that before, simply because it often improves performance.

What ends up happening is that some function starts out really small, and
then later on people add logic and debug code to it, and suddenly it's not
really appropriate to inline any more.

		Linus

Re: [PATCH] BUG preserve registers

[Andrew Morton]

Linus Torvalds wrote:
> 
> On Sat, 9 Feb 2002, Andrew Morton wrote:
> >
> > This is due to BUG() calls in inline functions in headers.  The biggest
> > culprit is dget(), in dcache.h.  This causes the full path of the header file
> > to be expanded into each and every compilation unit which includes
> > dcache.h.
> 
> Hmm. Which brings up another issue: can somebody come up with an idea of
> how to make the thing not use the whole pathname, but only the basename
> relative to the top-of-tree?

This is the cue for Keith to pop up and say "fixed in kbuild 2.5".

The preprocessor is simply pasting together its -I argument and the
string from the #include statement.  There doesn't seem to be a way
of getting it to just emit "include/linux/dcache.h" or "drivers/char/serial.c".

__BASE_FILE__ seems to have been supported for a sufficiently long time.  It
will just give us "dcache.h".  I think that's OK.  We don't get full path
info for the .c files either, and I'm not aware of that causing problems - if
there's a BUG() at inode.c:1234 we seem to be able to work out which inode.c
to look at.

__BASE_FILE__ is already used a bit in spinlock.h, so I think I'll
go with that.  But I'll also take a look at all the inlined BUGs.


> 
> Note that the correct thing to do may be to un-inline a lot of things.
> We've done that before, simply because it often improves performance.
> 
> What ends up happening is that some function starts out really small, and
> then later on people add logic and debug code to it, and suddenly it's not
> really appropriate to inline any more.

I noticed :)


-

Re: [PATCH] BUG preserve registers

[Linus Torvalds]

On Sat, 9 Feb 2002, Andrew Morton wrote:
>
> This is the cue for Keith to pop up and say "fixed in kbuild 2.5".

Nope.

> __BASE_FILE__ seems to have been supported for a sufficiently long time.

__BASE_FILE__ is not useful.

Remember: when we have a BUG in a header file, we need to get the HEADER
file, not the base file.

__BASE_FILE__ only works for .c files.

And .c files aren't the problem anyway (ie if we didn't have BUG()
statements in header files, we wouldn't have problems anyway).

		Linus

Re: [PATCH] BUG preserve registers

[Andrew Morton]

Linus Torvalds wrote:
> 
> On Sat, 9 Feb 2002, Andrew Morton wrote:
> >
> > This is the cue for Keith to pop up and say "fixed in kbuild 2.5".
> 
> Nope.
> 
> > __BASE_FILE__ seems to have been supported for a sufficiently long time.
> 
> __BASE_FILE__ is not useful.
> 
> Remember: when we have a BUG in a header file, we need to get the HEADER
> file, not the base file.
> 
> __BASE_FILE__ only works for .c files.
> 

Oh so that's what __BASE_FILE__ does :(

I looked at cccp.c - there doesn't seem to be a way.  Best
I can come up with is, in each directory, to do

	ln -s $(TOPDIR)/include/linux i
and
	gcc -Ii ...

which seems like a good way to get one's linux kernel license taken
away.  Better off feeding the asm through sed(1).


No magic bullets.   I think it's sufficient to go and individually
address the BUG-in-inline instances.  But that's a separate patch.

Macrelo, here's the "Don't trash registers in BUG()" patch.

I tried hpa's `for ( ; ; );' suggestion - the kernel got bigger.
And I couldn't find a way of declaring BUG() to be noreturn, so
I think I'm done with this now.

BTW: it has been noted that I'm not doing 2.5 patches.  All the stuff
I'm working on at present tends to be small, nasty, needed for 2.4 and
not critical for 2.5 at this time.  I shall forward port anything which
falls through Dave's cracks, but not for a while yet.



--- linux-2.4.18-pre9/include/asm-i386/page.h	Thu Feb  7 13:04:22 2002
+++ linux-akpm/include/asm-i386/page.h	Sat Feb  9 22:01:01 2002
@@ -91,16 +91,18 @@ typedef struct { unsigned long pgprot; }
 /*
  * Tell the user there is some problem. Beep too, so we can
  * see^H^H^Hhear bugs in early bootup as well!
+ * The offending file and line are encoded after the "officially
+ * undefined" opcode for parsing in the trap handler.
  */
 
-#ifdef CONFIG_DEBUG_BUGVERBOSE
-extern void do_BUG(const char *file, int line);
-#define BUG() do {					\
-	do_BUG(__FILE__, __LINE__);			\
-	__asm__ __volatile__("ud2");			\
-} while (0)
+#if 1	/* Set to zero for a slightly smaller kernel */
+#define BUG()				\
+ __asm__ __volatile__(	"ud2\n"		\
+			"\t.word %c0\n"	\
+			"\t.long %c1\n"	\
+			 : : "i" (__LINE__), "i" (__FILE__))
 #else
-#define BUG() __asm__ __volatile__(".byte 0x0f,0x0b")
+#define BUG() __asm__ __volatile__("ud2\n")
 #endif
 
 #define PAGE_BUG(page) do { \
--- linux-2.4.18-pre9/arch/i386/kernel/traps.c	Sun Sep 30 12:26:08 2001
+++ linux-akpm/arch/i386/kernel/traps.c	Sat Feb  9 21:30:06 2002
@@ -237,6 +237,42 @@ bad:
 	printk("\n");
 }	
 
+static void handle_BUG(struct pt_regs *regs)
+{
+	unsigned short ud2;
+	unsigned short line;
+	char *file;
+	char c;
+	unsigned long eip;
+
+	if (regs->xcs & 3)
+		goto no_bug;		/* Not in kernel */
+
+	eip = regs->eip;
+
+	if (eip < PAGE_OFFSET)
+		goto no_bug;
+	if (__get_user(ud2, (unsigned short *)eip))
+		goto no_bug;
+	if (ud2 != 0x0b0f)
+		goto no_bug;
+	if (__get_user(line, (unsigned short *)(eip + 2)))
+		goto bug;
+	if (__get_user(file, (char **)(eip + 4)))
+		goto bug;
+	if ((unsigned long)file < PAGE_OFFSET || __get_user(c, file))
+		file = "<bad filename>";
+
+	printk("Kernel BUG at %s:%d\n", file, line);
+
+no_bug:
+	return;
+
+	/* Here we know it was a BUG but file-n-line is unavailable */
+bug:
+	printk("Kernel BUG\n");
+}
+
 spinlock_t die_lock = SPIN_LOCK_UNLOCKED;
 
 void die(const char * str, struct pt_regs * regs, long err)
@@ -244,6 +280,7 @@ void die(const char * str, struct pt_reg
 	console_verbose();
 	spin_lock_irq(&die_lock);
 	bust_spinlocks(1);
+	handle_BUG(regs);
 	printk("%s: %04lx\n", str, err & 0xffff);
 	show_registers(regs);
 	bust_spinlocks(0);
--- linux-2.4.18-pre9/arch/i386/kernel/i386_ksyms.c	Thu Feb  7 13:04:11 2002
+++ linux-akpm/arch/i386/kernel/i386_ksyms.c	Sat Feb  9 19:49:38 2002
@@ -168,9 +168,5 @@ EXPORT_SYMBOL_NOVERS(memset);
 EXPORT_SYMBOL(atomic_dec_and_lock);
 #endif
 
-#ifdef CONFIG_DEBUG_BUGVERBOSE
-EXPORT_SYMBOL(do_BUG);
-#endif
-
 extern int is_sony_vaio_laptop;
 EXPORT_SYMBOL(is_sony_vaio_laptop);
--- linux-2.4.18-pre9/arch/i386/config.in	Thu Feb  7 13:04:11 2002
+++ linux-akpm/arch/i386/config.in	Sat Feb  9 19:49:38 2002
@@ -421,7 +421,6 @@ if [ "$CONFIG_DEBUG_KERNEL" != "n" ]; th
    bool '  Memory mapped I/O debugging' CONFIG_DEBUG_IOVIRT
    bool '  Magic SysRq key' CONFIG_MAGIC_SYSRQ
    bool '  Spinlock debugging' CONFIG_DEBUG_SPINLOCK
-   bool '  Verbose BUG() reporting (adds 70K)' CONFIG_DEBUG_BUGVERBOSE
 fi
 
 endmenu
--- linux-2.4.18-pre9/arch/i386/mm/fault.c	Thu Feb  7 13:04:11 2002
+++ linux-akpm/arch/i386/mm/fault.c	Sat Feb  9 19:49:38 2002
@@ -125,12 +125,6 @@ void bust_spinlocks(int yes)
 	}
 }
 
-void do_BUG(const char *file, int line)
-{
-	bust_spinlocks(1);
-	printk("kernel BUG at %s:%d!\n", file, line);
-}
-
 asmlinkage void do_invalid_op(struct pt_regs *, unsigned long);
 extern unsigned long idt;

Re: [PATCH] BUG preserve registers

[Andrew Morton]

Andrew Morton wrote:
> 
> I think I'm done with this now.

Famous last words.  There's a slightly updated version at
http://www.zip.com.au/~akpm/linux/2.4/2.4.18-pre9/

CONFIG_DEBUG_BUGVEROSE has been retained - it's now used to enable
BUG() in a couple of bloaty places.  But BUG() now always
reports file-and-line.

Also, here's the kill-BUG-in-headers patch.  With this, the
kernel image is in fact a few kbytes smaller than stock
2.4.18-pre9 with CONFIG_DEBUG_BUGVERBOSE=n, and we get full
file-and-line and the registers aren't trashed, so everyone's
happy.

It's interesting to run

	strings -n16 vmlinux | sort

and look at the output.  There are still quite a lot of printk's
in inline functions which could be tidied up.  NFS blows about
3k and reiserfs with extra debug checking enabled is pretty gross.

I guess I'll propose these patches for 2.4.19-pre.  There isn't
anything really critical here.   Total saving in my build is
120 kbytes, or 5% of the kernel size.




--- linux-2.4.18-pre9/include/asm-i386/mmu_context.h	Tue Oct 23 21:59:06 2001
+++ linux-akpm/include/asm-i386/mmu_context.h	Sun Feb 10 00:42:25 2002
@@ -48,7 +48,7 @@ static inline void switch_mm(struct mm_s
 	else {
 		cpu_tlbstate[cpu].state = TLBSTATE_OK;
 		if(cpu_tlbstate[cpu].active_mm != next)
-			BUG();
+			VERBOSE_BUG();
 		if(!test_and_set_bit(cpu, &next->cpu_vm_mask)) {
 			/* We were in lazy tlb mode and leave_mm disabled 
 			 * tlb flush IPI delivery. We must flush our tlb.
--- linux-2.4.18-pre9/include/linux/kernel.h	Thu Feb  7 13:04:22 2002
+++ linux-akpm/include/linux/kernel.h	Sun Feb 10 00:07:18 2002
@@ -181,4 +181,7 @@ struct sysinfo {
 	char _f[20-2*sizeof(long)-sizeof(int)];	/* Padding: libc5 uses this.. */
 };
 
+extern void bug_in_interrupt(void);
+extern void out_of_line_bug(void);
+
 #endif
--- linux-2.4.18-pre9/include/asm-i386/pci.h	Thu Feb  7 13:04:22 2002
+++ linux-akpm/include/asm-i386/pci.h	Sun Feb 10 00:42:35 2002
@@ -73,7 +73,7 @@ static inline dma_addr_t pci_map_single(
 					size_t size, int direction)
 {
 	if (direction == PCI_DMA_NONE)
-		BUG();
+		VERBOSE_BUG();
 	flush_write_buffers();
 	return virt_to_bus(ptr);
 }
@@ -89,7 +89,7 @@ static inline void pci_unmap_single(stru
 				    size_t size, int direction)
 {
 	if (direction == PCI_DMA_NONE)
-		BUG();
+		VERBOSE_BUG();
 	/* Nothing to do */
 }
 
@@ -101,7 +101,7 @@ static inline dma_addr_t pci_map_page(st
 				      unsigned long offset, size_t size, int direction)
 {
 	if (direction == PCI_DMA_NONE)
-		BUG();
+		VERBOSE_BUG();
 
 	return (page - mem_map) * PAGE_SIZE + offset;
 }
@@ -110,7 +110,7 @@ static inline void pci_unmap_page(struct
 				  size_t size, int direction)
 {
 	if (direction == PCI_DMA_NONE)
-		BUG();
+		VERBOSE_BUG();
 	/* Nothing to do */
 }
 
@@ -143,16 +143,16 @@ static inline int pci_map_sg(struct pci_
 	int i;
 
 	if (direction == PCI_DMA_NONE)
-		BUG();
+		VERBOSE_BUG();
  
  	/*
  	 * temporary 2.4 hack
  	 */
  	for (i = 0; i < nents; i++ ) {
  		if (sg[i].address && sg[i].page)
- 			BUG();
+ 			VERBOSE_BUG();
  		else if (!sg[i].address && !sg[i].page)
- 			BUG();
+ 			VERBOSE_BUG();
  
  		if (sg[i].address)
  			sg[i].dma_address = virt_to_bus(sg[i].address);
@@ -172,7 +172,7 @@ static inline void pci_unmap_sg(struct p
 				int nents, int direction)
 {
 	if (direction == PCI_DMA_NONE)
-		BUG();
+		VERBOSE_BUG();
 	/* Nothing to do */
 }
 
@@ -190,7 +190,7 @@ static inline void pci_dma_sync_single(s
 				       size_t size, int direction)
 {
 	if (direction == PCI_DMA_NONE)
-		BUG();
+		VERBOSE_BUG();
 	flush_write_buffers();
 }
 
@@ -205,7 +205,7 @@ static inline void pci_dma_sync_sg(struc
 				   int nelems, int direction)
 {
 	if (direction == PCI_DMA_NONE)
-		BUG();
+		VERBOSE_BUG();
 	flush_write_buffers();
 }
 
--- linux-2.4.18-pre9/include/asm-i386/smplock.h	Tue Oct 23 21:59:10 2001
+++ linux-akpm/include/asm-i386/smplock.h	Sun Feb 10 00:42:26 2002
@@ -59,7 +59,7 @@ static __inline__ void lock_kernel(void)
 static __inline__ void unlock_kernel(void)
 {
 	if (current->lock_depth < 0)
-		BUG();
+		VERBOSE_BUG();
 #if 1
 	if (--current->lock_depth < 0)
 		spin_unlock(&kernel_flag);
--- linux-2.4.18-pre9/include/linux/dcache.h	Tue Oct 23 21:59:05 2001
+++ linux-akpm/include/linux/dcache.h	Sun Feb 10 00:07:18 2002
@@ -242,11 +242,8 @@ extern char * __d_path(struct dentry *, 
  
 static __inline__ struct dentry * dget(struct dentry *dentry)
 {
-	if (dentry) {
-		if (!atomic_read(&dentry->d_count))
-			BUG();
+	if (dentry)
 		atomic_inc(&dentry->d_count);
-	}
 	return dentry;
 }
 
--- linux-2.4.18-pre9/include/linux/file.h	Wed Aug 23 11:22:26 2000
+++ linux-akpm/include/linux/file.h	Sun Feb 10 00:07:18 2002
@@ -71,30 +71,7 @@ static inline void put_unused_fd(unsigne
 	write_unlock(&files->file_lock);
 }
 
-/*
- * Install a file pointer in the fd array.  
- *
- * The VFS is full of places where we drop the files lock between
- * setting the open_fds bitmap and installing the file in the file
- * array.  At any such point, we are vulnerable to a dup2() race
- * installing a file in the array before us.  We need to detect this and
- * fput() the struct file we are about to overwrite in this case.
- *
- * It should never happen - if we allow dup2() do it, _really_ bad things
- * will follow.
- */
-
-static inline void fd_install(unsigned int fd, struct file * file)
-{
-	struct files_struct *files = current->files;
-	
-	write_lock(&files->file_lock);
-	if (files->fd[fd])
-		BUG();
-	files->fd[fd] = file;
-	write_unlock(&files->file_lock);
-}
-
+void fd_install(unsigned int fd, struct file * file);
 void put_files_struct(struct files_struct *fs);
 
 #endif /* __LINUX_FILE_H */
--- linux-2.4.18-pre9/include/linux/nfs_fs.h	Fri Dec 21 11:19:23 2001
+++ linux-akpm/include/linux/nfs_fs.h	Sun Feb 10 00:42:44 2002
@@ -168,7 +168,7 @@ nfs_file_cred(struct file *file)
 	struct rpc_cred *cred = (struct rpc_cred *)(file->private_data);
 #ifdef RPC_DEBUG
 	if (cred && cred->cr_magic != RPCAUTH_CRED_MAGIC)
-		BUG();
+		out_of_line_bug();
 #endif
 	return cred;
 }
--- linux-2.4.18-pre9/include/linux/quotaops.h	Tue Oct 23 21:59:31 2001
+++ linux-akpm/include/linux/quotaops.h	Sun Feb 10 00:42:52 2002
@@ -40,8 +40,6 @@ extern int  dquot_transfer(struct inode 
 
 static __inline__ void DQUOT_INIT(struct inode *inode)
 {
-	if (!inode->i_sb)
-		BUG();
 	lock_kernel();
 	if (sb_any_quota_enabled(inode->i_sb) && !IS_NOQUOTA(inode))
 		inode->i_sb->dq_op->initialize(inode, -1);
@@ -51,11 +49,8 @@ static __inline__ void DQUOT_INIT(struct
 static __inline__ void DQUOT_DROP(struct inode *inode)
 {
 	lock_kernel();
-	if (IS_QUOTAINIT(inode)) {
-		if (!inode->i_sb)
-			BUG();
+	if (IS_QUOTAINIT(inode))
 		inode->i_sb->dq_op->drop(inode);	/* Ops must be set when there's any quota... */
-	}
 	unlock_kernel();
 }
 
--- linux-2.4.18-pre9/include/linux/sched.h	Fri Dec 21 11:19:23 2001
+++ linux-akpm/include/linux/sched.h	Sun Feb 10 00:42:25 2002
@@ -888,7 +888,6 @@ static inline int task_on_runqueue(struc
 
 static inline void unhash_process(struct task_struct *p)
 {
-	if (task_on_runqueue(p)) BUG();
 	write_lock_irq(&tasklist_lock);
 	nr_threads--;
 	unhash_pid(p);
--- linux-2.4.18-pre9/include/linux/highmem.h	Thu Feb  7 13:04:22 2002
+++ linux-akpm/include/linux/highmem.h	Sun Feb 10 00:42:28 2002
@@ -63,8 +63,6 @@ static inline void memclear_highpage_flu
 {
 	char *kaddr;
 
-	if (offset + size > PAGE_SIZE)
-		BUG();
 	kaddr = kmap(page);
 	memset(kaddr + offset, 0, size);
 	flush_dcache_page(page);
--- linux-2.4.18-pre9/include/linux/skbuff.h	Mon Nov  5 21:01:12 2001
+++ linux-akpm/include/linux/skbuff.h	Sun Feb 10 00:42:29 2002
@@ -756,9 +756,9 @@ static inline int skb_headlen(const stru
 	return skb->len - skb->data_len;
 }
 
-#define SKB_PAGE_ASSERT(skb) do { if (skb_shinfo(skb)->nr_frags) BUG(); } while (0)
-#define SKB_FRAG_ASSERT(skb) do { if (skb_shinfo(skb)->frag_list) BUG(); } while (0)
-#define SKB_LINEAR_ASSERT(skb) do { if (skb_is_nonlinear(skb)) BUG(); } while (0)
+#define SKB_PAGE_ASSERT(skb) do { if (skb_shinfo(skb)->nr_frags) out_of_line_bug(); } while (0)
+#define SKB_FRAG_ASSERT(skb) do { if (skb_shinfo(skb)->frag_list) out_of_line_bug(); } while (0)
+#define SKB_LINEAR_ASSERT(skb) do { if (skb_is_nonlinear(skb)) out_of_line_bug(); } while (0)
 
 /*
  *	Add data to an sk_buff
@@ -825,8 +825,6 @@ static inline unsigned char *skb_push(st
 static inline char *__skb_pull(struct sk_buff *skb, unsigned int len)
 {
 	skb->len-=len;
-	if (skb->len < skb->data_len)
-		BUG();
 	return 	skb->data+=len;
 }
 
@@ -1094,7 +1092,7 @@ static inline void *kmap_skb_frag(const 
 {
 #ifdef CONFIG_HIGHMEM
 	if (in_irq())
-		BUG();
+		out_of_line_bug();
 
 	local_bh_disable();
 #endif
--- linux-2.4.18-pre9/include/asm-i386/highmem.h	Tue Oct 23 21:59:06 2001
+++ linux-akpm/include/asm-i386/highmem.h	Sun Feb 10 00:42:25 2002
@@ -62,7 +62,7 @@ extern void FASTCALL(kunmap_high(struct 
 static inline void *kmap(struct page *page)
 {
 	if (in_interrupt())
-		BUG();
+		bug_in_interrupt();
 	if (page < highmem_start_page)
 		return page_address(page);
 	return kmap_high(page);
@@ -71,7 +71,7 @@ static inline void *kmap(struct page *pa
 static inline void kunmap(struct page *page)
 {
 	if (in_interrupt())
-		BUG();
+		bug_in_interrupt();
 	if (page < highmem_start_page)
 		return;
 	kunmap_high(page);
@@ -95,7 +95,7 @@ static inline void *kmap_atomic(struct p
 	vaddr = __fix_to_virt(FIX_KMAP_BEGIN + idx);
 #if HIGHMEM_DEBUG
 	if (!pte_none(*(kmap_pte-idx)))
-		BUG();
+		out_of_line_bug();
 #endif
 	set_pte(kmap_pte-idx, mk_pte(page, kmap_prot));
 	__flush_tlb_one(vaddr);
@@ -113,7 +113,7 @@ static inline void kunmap_atomic(void *k
 		return;
 
 	if (vaddr != __fix_to_virt(FIX_KMAP_BEGIN+idx))
-		BUG();
+		out_of_line_bug();
 
 	/*
 	 * force other mappings to Oops if they'll try to access
--- linux-2.4.18-pre9/include/net/tcp.h	Tue Oct 23 21:59:58 2001
+++ linux-akpm/include/net/tcp.h	Sun Feb 10 00:42:59 2002
@@ -1329,8 +1329,6 @@ static __inline__ int tcp_prequeue(struc
 		if (tp->ucopy.memory > sk->rcvbuf) {
 			struct sk_buff *skb1;
 
-			if (sk->lock.users) BUG();
-
 			while ((skb1 = __skb_dequeue(&tp->ucopy.prequeue)) != NULL) {
 				sk->backlog_rcv(sk, skb1);
 				NET_INC_STATS_BH(TCPPrequeueDropped);
--- linux-2.4.18-pre9/fs/open.c	Fri Oct 12 13:48:42 2001
+++ linux-akpm/fs/open.c	Sun Feb 10 00:48:33 2002
@@ -71,6 +71,30 @@ out:
 	return error;
 }
 
+/*
+ * Install a file pointer in the fd array.  
+ *
+ * The VFS is full of places where we drop the files lock between
+ * setting the open_fds bitmap and installing the file in the file
+ * array.  At any such point, we are vulnerable to a dup2() race
+ * installing a file in the array before us.  We need to detect this and
+ * fput() the struct file we are about to overwrite in this case.
+ *
+ * It should never happen - if we allow dup2() do it, _really_ bad things
+ * will follow.
+ */
+
+void fd_install(unsigned int fd, struct file * file)
+{
+	struct files_struct *files = current->files;
+	
+	write_lock(&files->file_lock);
+	if (files->fd[fd])
+		BUG();
+	files->fd[fd] = file;
+	write_unlock(&files->file_lock);
+}
+
 int do_truncate(struct dentry *dentry, loff_t length)
 {
 	struct inode *inode = dentry->d_inode;
--- linux-2.4.18-pre9/kernel/ksyms.c	Thu Feb  7 13:04:22 2002
+++ linux-akpm/kernel/ksyms.c	Sun Feb 10 00:07:18 2002
@@ -164,6 +164,7 @@ EXPORT_SYMBOL(mark_buffer_dirty);
 EXPORT_SYMBOL(set_buffer_async_io); /* for reiserfs_writepage */
 EXPORT_SYMBOL(__mark_buffer_dirty);
 EXPORT_SYMBOL(__mark_inode_dirty);
+EXPORT_SYMBOL(fd_install);
 EXPORT_SYMBOL(get_empty_filp);
 EXPORT_SYMBOL(init_private_file);
 EXPORT_SYMBOL(filp_open);
@@ -452,6 +453,8 @@ EXPORT_SYMBOL(nr_running);
 
 /* misc */
 EXPORT_SYMBOL(panic);
+EXPORT_SYMBOL(out_of_line_bug);
+EXPORT_SYMBOL(bug_in_interrupt);
 EXPORT_SYMBOL(sprintf);
 EXPORT_SYMBOL(snprintf);
 EXPORT_SYMBOL(sscanf);
--- linux-2.4.18-pre9/kernel/panic.c	Sun Sep 30 12:26:08 2001
+++ linux-akpm/kernel/panic.c	Sun Feb 10 00:07:18 2002
@@ -120,3 +120,25 @@ const char *print_tainted()
 }
 
 int tainted = 0;
+
+/*
+ * A BUG() call in an inline function in a header should be avoided,
+ * because it can seriously bloat the kernel.  So here we have
+ * helper functions.
+ * We lose the BUG()-time file-and-line info this way, but it's
+ * usually not very useful from an inline anyway.  The backtrace
+ * tells us what we want to know.
+ */
+
+void out_of_line_bug(void)
+{
+	BUG();
+}
+
+void bug_in_interrupt(void)
+{
+	if (in_interrupt()) {
+		printk("BUG: in_interrupt()\n");
+		BUG();
+	}
+}




-

Re: [PATCH] BUG preserve registers

[Andrew Morton]

Denis Vlasenko wrote:
> 
> On 10 February 2002 07:05, Andrew Morton wrote:
> > Andrew Morton wrote:
> > > I think I'm done with this now.
> >
> > Famous last words.  There's a slightly updated version at
> > http://www.zip.com.au/~akpm/linux/2.4/2.4.18-pre9/
> >
> > CONFIG_DEBUG_BUGVEROSE has been retained - it's now used to enable
> > BUG() in a couple of bloaty places.  But BUG() now always
> > reports file-and-line.
> >
> > Also, here's the kill-BUG-in-headers patch.  With this, the
> > kernel image is in fact a few kbytes smaller than stock
> > 2.4.18-pre9 with CONFIG_DEBUG_BUGVERBOSE=n, and we get full
> > file-and-line and the registers aren't trashed, so everyone's
> > happy.
> 
> I downloaded BUG.patch and inline-BUG.patch and compiled 2.4.18-pre9
> bzImage with and without these patches:
> 
> 2.4.18-pre9:         bzImage 996963  vmlinux 2440880
> 2.4.18-pre9+patches: bzImage 997531  vmlinux 2441182
> 
> .config is the same, you may find it below.
> "# CONFIG_DEBUG_BUGVERBOSE is not set"
> Andrew, are these numbers expected?

Pretty much.  In adding the patch, you've added six more bytes
to each BUG() call site and numerous filenames.  For me, the
kernel came out a little smaller, but for you, it grew a bit.  I
wonder why.  From a quick test it seems that gcc-3.0.3 and
recent binutils do fold common strings between compilation
units.  But I need to double-check that.  Maybe it's due to
that somehow.

Of course, for the extra 300 bytes of kernel you've gained
file-and-line info in all the BUG calls.

-

Re: [PATCH] BUG preserve registers

[Hugh Dickins]

On Sun, 10 Feb 2002, Andrew Morton wrote:
> > 
> > I think I'm done with this now.
> 
> Famous last words.  There's a slightly updated version at
> http://www.zip.com.au/~akpm/linux/2.4/2.4.18-pre9/

Thanks for taking this on, Andrew.  A lot of nice work, but...

1. I'd be sorry to see it go in as is: you haven't noticed how this
   looks to a disassembler: not pretty!  I don't see an alternative
   to wasting some more bytes: putting "push"es in front of line and
   file? or is there one instruction (some "mov"?) to encompass both
   line and file together?

2. Was change from "kernel BUG at %s:%d!\n" to "Kernel BUG at %s:%d\n"
   intentional?  I'd have thought leave it as was.

3. This probably falls right outside your brief, and I do not want
   to stoke a flamewar about .config in kernel, but let me mention:
   the oops info would be significantly more helpful if it showed
   the version of compiler used, and whether CONFIG_SMP e.g. so we
   can quickly see which of the page_cache_releases in shrink_cache
   gave rise to some error, without trying out assorted compilers
   and configs - BUGVERBOSE was a nuisance for that too, now remedied.
   (Personally, I'd also like to know whether NOHIGHMEM, HIGHMEM4G or
   HIGHMEM64G, but that may reflect my own interests too much, and
   open floodgates to endless such requests.)  Of course such info can
   be supplied by reporters, at the time or later, but nice if it were
   there by default like "Tainted".  Ignore me unless you feel the same.

4. In your patch removing BUGs from inlines, I suggest you delete
   bug_in_interrupt: it's only used in asm/highmem.h (hmm, you check
   in_interrupt once before it, and again inside it), and those
   checks are much better moved into mm/highmem.c kmap_high and
   kunmap_high themselves - I already gave Andrea a patch with that
   change, and that part he approved.  And since you're attacking
   skbuff.h, I think you can then remove interrupt.h from highmem.h,
   but include it in skbuff.h: if I remember rightly, skbuff.h is the
   only file which assumes interrupt.h has already been included by
   highmem.h.  But again, ignore all these remarks unless they chime
   with your own desires - I shouldn't be playing backseat janitor
   with your time.

(Aside to Marcelo: if you're hesitant to put Andrew's changes into
2.4.18, please consider my original asm do_BUG version: it would be
a pity if 2.4.18 went out still destroying registers when BUGVERBOSE,
just because a better implementation is on the way.)

Hugh

Re: [PATCH] BUG preserve registers

[Andrew Morton]

Hugh Dickins wrote:
> 
> On Sun, 10 Feb 2002, Andrew Morton wrote:
> > >
> > > I think I'm done with this now.
> >
> > Famous last words.  There's a slightly updated version at
> > http://www.zip.com.au/~akpm/linux/2.4/2.4.18-pre9/
> 
> Thanks for taking this on, Andrew.  A lot of nice work, but...
> 
> 1. I'd be sorry to see it go in as is: you haven't noticed how this
>    looks to a disassembler: not pretty!  I don't see an alternative
>    to wasting some more bytes: putting "push"es in front of line and
>    file? or is there one instruction (some "mov"?) to encompass both
>    line and file together?

This will also confound ksymoops when it parses the opcode
dump.  But we know the file-and-line, so that doesn't seem
very important.

If the unpretty disassembler output is a problem, we could go
back to your do_BUG code if CONFIG_DEBUG_BUGVERBOSE=y?

> 2. Was change from "kernel BUG at %s:%d!\n" to "Kernel BUG at %s:%d\n"
>    intentional?  I'd have thought leave it as was.

oops.  Actually I was thinking of licensing the message as advertising
space to Pepsi...   Will fix.
 
> 3. This probably falls right outside your brief, and I do not want
>    to stoke a flamewar about .config in kernel, but let me mention:
>    the oops info would be significantly more helpful if it showed
>    the version of compiler used, and whether CONFIG_SMP e.g. so we
>    can quickly see which of the page_cache_releases in shrink_cache
>    gave rise to some error, without trying out assorted compilers
>    and configs - BUGVERBOSE was a nuisance for that too, now remedied.
>    (Personally, I'd also like to know whether NOHIGHMEM, HIGHMEM4G or
>    HIGHMEM64G, but that may reflect my own interests too much, and
>    open floodgates to endless such requests.)  Of course such info can
>    be supplied by reporters, at the time or later, but nice if it were
>    there by default like "Tainted".  Ignore me unless you feel the same.

mm.  It's hard to know where to stop with this.  My general feeling
is that we just need to pester the reporter for the relevant info.
The one thing I *would* like to see in the oops output is the
text "Please read Documentation/what-to-do-now.txt".  And that
file contains realy simple instructions on what the reporter
should do next.

> 4. In your patch removing BUGs from inlines, I suggest you delete
>    bug_in_interrupt: it's only used in asm/highmem.h (hmm, you check
>    in_interrupt once before it, and again inside it), and those
>    checks are much better moved into mm/highmem.c kmap_high and
>    kunmap_high themselves - I already gave Andrea a patch with that
>    change, and that part he approved.  And since you're attacking
>    skbuff.h, I think you can then remove interrupt.h from highmem.h,
>    but include it in skbuff.h: if I remember rightly, skbuff.h is the
>    only file which assumes interrupt.h has already been included by
>    highmem.h.  But again, ignore all these remarks unless they chime
>    with your own desires - I shouldn't be playing backseat janitor
>    with your time.

OK, I'll scratch my head over this.

> (Aside to Marcelo: if you're hesitant to put Andrew's changes into
> 2.4.18, please consider my original asm do_BUG version: it would be
> a pity if 2.4.18 went out still destroying registers when BUGVERBOSE,
> just because a better implementation is on the way.)
> 

I agree.  The asm do_BUG code can be retained for the "don't confuse the
disassembler" feature (do we really want that?), and I think we'll hold
off on the other stuff until 2.4.19-pre.  I suggest you resend the
original to Marcelo.

-

Re: [PATCH] BUG preserve registers

[Hugh Dickins]

On Mon, 11 Feb 2002, Andrew Morton wrote:
> Hugh Dickins wrote:
> > 
> > 1. I'd be sorry to see it go in as is: you haven't noticed how this
> >    looks to a disassembler: not pretty!  I don't see an alternative
> >    to wasting some more bytes: putting "push"es in front of line and
> >    file? or is there one instruction (some "mov"?) to encompass both
> >    line and file together?
> 
> This will also confound ksymoops when it parses the opcode
> dump.  But we know the file-and-line, so that doesn't seem
> very important.

Well, it's not very important if it's immediately obvious why the
BUG() on that source line should fire.  But in general, BUG()s are
catching problems that are not immediately obvious.

Very often we want to peer at the registers, work out exactly what's
happening.  That is precisely why you're making these changes.  Maybe
your in-head disassembler is better than objdump, but I need objdump
or ksymoops or kdb to make sense of what's there.

Of course, the nonsense comes _after_ the BUG(): but once you have
several BUG()s in succession, it can be hard even to find subsequent
"ud2a"s - in __free_pages_ok, I see "08 0f  or %cl,(%edi)
0b 4c 00 7c  or 0x7c(%eax,%eax,1),%ecx" and suspect that
there's really a "0f 0b  ud2a" hidden in there.

The BUG() info is there to help debugging, it should not be obscuring
the code.  I guess it would be easy to (mis)teach ksymoops and kdb
that "ud2a" is actually a seven-byte instruction, but not objdump.

> If the unpretty disassembler output is a problem, we could go
> back to your do_BUG code if CONFIG_DEBUG_BUGVERBOSE=y?

I don't care for that, I really like that the option should go (or be
sidelined to mean something else, which only affects a few places,
as you have it), given your triumph in cutting down its overhead.

Hugh

Re: [PATCH] BUG preserve registers

[H. Peter Anvin]

Linus Torvalds wrote:

> 
> __BASE_FILE__ is not useful.
> 
> Remember: when we have a BUG in a header file, we need to get the HEADER
> file, not the base file.
> 
> __BASE_FILE__ only works for .c files.
> 
> And .c files aren't the problem anyway (ie if we didn't have BUG()
> statements in header files, we wouldn't have problems anyway).
> 


What we'd really like is something like:

/* foo.h */
#ifndef _FOO_H
#define _FOO_H
#define _FILE_REF __fileref_foo_h

[...]

#undef _FILE_REF
#endif /* _FOO_H */

... and have an automatically generated library (.a file) with all these 
strings that could be pulled in as necessary -- but only one instance of 
each.  However, doing this without compiler support seems ugly in the 
extreme.

Of course, if the linker would fold strings for us then it would be a 
helluva lot easier...

	-hpa

Re: [PATCH] BUG preserve registers

[arjan]

In article <3C661D89.8070202@zytor.com> you wrote:

> Of course, if the linker would fold strings for us then it would be a 
> helluva lot easier...

Recent combinations of gcc/binutils do this just fine.
The only question is: do we care for older toolchains (gcc <= 2.95) not
doing the optimal thing for 2.5.....

Re: [PATCH] BUG preserve registers

[Jamie Lokier]

Andrew Morton wrote:
> The preprocessor is simply pasting together its -I argument and the
> string from the #include statement.  There doesn't seem to be a way
> of getting it to just emit "include/linux/dcache.h" or "drivers/char/serial.c".

Doesn't that work if you do this?

   (cd $(TOPDIR);
    gcc -Idrivers/char -I- -Iinclude \
        -c -o drivers/char/serial.o drivers/char/serial.c)

-- Jamie

Re: [PATCH] BUG preserve registers

[Eric W. Biederman]

Linus Torvalds <torvalds@transmeta.com> writes:

> On Sat, 9 Feb 2002, Andrew Morton wrote:
> >
> > This is due to BUG() calls in inline functions in headers.  The biggest
> > culprit is dget(), in dcache.h.  This causes the full path of the header file
> > to be expanded into each and every compilation unit which includes
> > dcache.h.
> 
> Hmm. Which brings up another issue: can somebody come up with an idea of
> how to make the thing not use the whole pathname, but only the basename
> relative to the top-of-tree?
> 
> I doubt it is possible, but maybe there is some clever way to avoid it..
> 
> > I'm showing thirteen header files, for a total of 83k.  I'll do something
> > about this...
> 
> Ok, so even your gcc obviously is _not_ intelligent enough to throw away
> strings from inline functions that aren't used. Oh well.

One possibility is to do something like:

ud2
.word __LINE__
.long 1f
.section __FILE__
.linkonce discard
1: .asciz __FILE__
.previous

Which will put each filename string in it's own section and let the
linker merge the duplicates.  I don't know how to wrap all of this up
nicely in a inline asm statement but perhaps someone else can work
out the remaining details.  Ideally I would put a prefix on the name
of all of the sections for easy processing, but string concatenation
doesn't work in asm :(

Eric

Re: [PATCH] BUG preserve registers

[Andrew Morton]

"Eric W. Biederman" wrote:
> 
> One possibility is to do something like:
> 
> ud2
> .word __LINE__
> .long 1f
> .section __FILE__
> .linkonce discard
> 1: .asciz __FILE__
> .previous
> 
> Which will put each filename string in it's own section and let the
> linker merge the duplicates.

That would work.  When it didn't I r'ed tfm:

	http://www.gnu.org/manual/gas-2.9.1/html_node/as_102.html

	"This directive is only supported by a few object file formats;
	 as of this writing, the only object file format which supports
	 it is the Portable Executable format used on Windows NT."


-

Re: [PATCH] BUG preserve registers

[Eric W. Biederman]

Andrew Morton <akpm@zip.com.au> writes:

> "Eric W. Biederman" wrote:
> > 
> > One possibility is to do something like:
> > 
> > ud2
> > .word __LINE__
> > .long 1f
> > .section __FILE__
> > .linkonce discard
> > 1: .asciz __FILE__
> > .previous
> > 
> > Which will put each filename string in it's own section and let the
> > linker merge the duplicates.
> 
> That would work.  When it didn't I r'ed tfm:
> 
> 	http://www.gnu.org/manual/gas-2.9.1/html_node/as_102.html
> 
> 	"This directive is only supported by a few object file formats;
> 	 as of this writing, the only object file format which supports
> 	 it is the Portable Executable format used on Windows NT."

It is supported for ELF and most other targets, but unfortunately not
with the same syntax :(  I just looked and you have to do it a
slightly different way.  You must prefix section name with  ".gnu.linkonce"

binutils/bfd/elf.c:416
  /* As a GNU extension, if the name begins with .gnu.linkonce, we
     only link a single copy of the section.  This is used to support
     g++.  g++ will emit each template expansion in its own section.
     The symbols will be defined as weak, so that multiple definitions
     are permitted.  The GNU linker extension is to actually discard
     all but one of the sections.  */

This has been supported since 1997 so it should be safe to use.
Though it would be nice if someone would actually document it...

Eric

Re: [PATCH] BUG preserve registers

[Stevie O]

At 10:50 PM 2/9/2002 -0800, Andrew Morton wrote:
> > .linkonce discard
> > 1: .asciz __FILE__
> > .previous
> >
> > Which will put each filename string in it's own section and let the
> > linker merge the duplicates.
>
>That would work.  When it didn't I r'ed tfm:
>
>         http://www.gnu.org/manual/gas-2.9.1/html_node/as_102.html
>
>         "This directive is only supported by a few object file formats;
>         as of this writing, the only object file format which supports
>         it is the Portable Executable format used on Windows NT."


So, all we need to do is port Linux to run under NT? ;)


--
Stevie-O

Real programmers use COPY CON PROGRAM.EXE

Re: [PATCH] BUG preserve registers

[Olaf Dietsche]

Linus Torvalds <torvalds@transmeta.com> writes:

> On Sat, 9 Feb 2002, Andrew Morton wrote:
>> I'm showing thirteen header files, for a total of 83k.  I'll do something
>> about this...
>
> Ok, so even your gcc obviously is _not_ intelligent enough to throw away
> strings from inline functions that aren't used. Oh well.

Try using gcc 3.x. Or is this a not an option with linux right now?

Regards, Olaf.

Re: [PATCH] BUG preserve registers

[Linus Torvalds]

On Sun, 10 Feb 2002, Alan Cox wrote:
>
> This looks a really bad reversion. The CONFIG_DEBUG_BUGVERBOSE ifdef saves
> over 70K of memory on my standard kernel build.

It saves 70k because the old code was total _crap_, and couldn't merge
strings within the same file etc.

With the new code I bet it's less than a kB with new gccs, and probably on
the order of a few kB with old ones.

		Linus

Re: [PATCH] BUG preserve registers

[Linus Torvalds]

On Sun, 10 Feb 2002, Alan Cox wrote:
>
> I also don't understand what the problem you are trying to solve is. If you
> want to debug the kernel you build with debug verbose. If not you don't.

No.

With the old stupid code, if you wanted to debug the kernel, you had to
make sure that the VERBOSE flag was _off_.

Because if it wasn't off, the printk's that triggered would totally
destroy the register state at the point of the BUG, which often hides real
information (the register state is what tends to contain the exact flags
that were tested for the bug).

> With the symbol table you can still easily trace down BUG events.

.. and with VERBOSE it end sup often being much simpler.

Also note that if you want to save space, what you should do is disable
BUG() altogether. In a real embedded device where memory is that tight,
the output generally won't make it anywhere anyway.

That will speed up the kernel a bit too (don't test for conditions that
cannot happen).

So don't be a proponent for stupidity. Which the old CONFIG_DEBUG_VERBOSE
was.

		Linus

Re: [PATCH] BUG preserve registers

[Brian Gerst]

Linus Torvalds wrote:
> 
> On Sat, 9 Feb 2002, Andrew Morton wrote:
> > >
> >
> > Is better, except the filename gets expanded multipe times into
> > the object file.  How about:
> >
> > #define BUG()                   \
> >         asm(    "ud2\n"         \
> >                 "\t.word %0\n"  \
> >                 "\t.long %1\n"  \
> >                  : : "i" (__LINE__), "i" (__FILE__))
> 
> Even better.
> 
> That way you can actually totally remove the "verbose bug" config option,
> because even the verbose BUG's aren't actually using up any noticeable
> amounts of space.
> 
> This is all assuming that gcc doesn't create the string for inline
> functions that aren't used, which it probably cannot, so maybe this
> doesn't work out.
> 
>                         Linus

Note that if you're going to do this you may want to use another vector
(ie. int $0x82) instead of the undefined opcode.  That way you know for
certain that there will be valid debugging info following.

-- 

						Brian Gerst