a more efficient BUG() macro

I was poking around in a vmlinux the other day and was surprised at the amount of repetitive crap text that was in there. For example, try:

strings vmlinux|grep $PWD|wc -c

which gets some 70KB in my case - depends on strlen($PWD) obviously. The culprit is BUG() in a static inline that is in a header file. In this case cpp expands __FILE__ to the full path of the header file in question. (IIRC there is a __BASEFILE__ that would be a better choice than __FILE__)

There is also some 5 to 10k worth of "kernel BUG at %s:%d!\n" scattered through a typical vmlinux. Note that neither of these show up in [b]zImage size since they compress to something like 99%, but they do cost memory once the kernel is booted.

Anyway this small patch makes sure there is only one "kernel BUG..." string, and dumps __FILE__ in favour of an address value since System.map data is needed to make full use of the BUG() dump anyways. The memory stats of two otherwise identical kernels:

Memory: 22004k/24576k available (1163k kernel code, 2184k reserved,

				 309k data, 64k init, 0k highmem)
Memory: 22076k/24576k available (1163k kernel code, 2112k reserved,
				 238k data, 64k init, 0k highmem)

so the lightweight BUG() nets a worthwhile (IMHO) saving of 72KB for this particular kernel. (Which is even more than the __init goodies recover.) Patch is against 2.4.2pre3 and is fully contained within i386 files. I can supply a 2.2.x version of the patch if there is demand for it.

Paul.

diff -ur linux~/arch/i386/kernel/i386_ksyms.c linux/arch/i386/kernel/i386_ksyms.c
@@ -78,6 +78,7 @@
 EXPORT_SYMBOL_NOVERS(__down_write_failed);
 EXPORT_SYMBOL_NOVERS(__down_read_failed);
 EXPORT_SYMBOL_NOVERS(__rwsem_wake);
+EXPORT_SYMBOL_NOVERS(bugstring);
 /* Networking helper routines. */
 EXPORT_SYMBOL(csum_partial_copy_generic);
 /* Delay loops */

diff -ur linux~/arch/i386/kernel/setup.c linux/arch/i386/kernel/setup.c
@@ -105,6 +105,8 @@
 char ignore_irq13;		/* set if exception 16 works */
 struct cpuinfo_x86 boot_cpu_data = { 0, 0, 0, 0, -1, 1, 0, 0, -1 };
 
+const char *bugstring="<1>kernel BUG at 0x%p\n";
+
 unsigned long mmu_cr4_features;
 
 /*

diff -ur linux~/include/asm-i386/page.h linux/include/asm-i386/page.h
@@ -86,10 +86,13 @@
  * Tell the user there is some problem. Beep too, so we can
  * see^H^H^Hhear bugs in early bootup as well!
  */
-#define BUG() do { \
-	printk("kernel BUG at %s:%d!\n", __FILE__, __LINE__); \
+extern const char *bugstring;	/* ...is in i386/kernel/setup.c - Paul G. */
+#define BUG() ({ \
+	__label__ bugaddr;			\
+bugaddr:					\
+	printk(bugstring, &&bugaddr);		\
 	__asm__ __volatile__(".byte 0x0f,0x0b"); \
-} while (0)
+})
 
 #define PAGE_BUG(page) do { \
 	BUG(); \

_________________________________________________________
Do You Yahoo!?
Get your free @yahoo.com address at http://mail.yahoo.com

Shouldn't the linker drop duplicate strings?

info gcc mentions that gcc merges duplicate strings, but it seems that it doesn't work (gcc-2.96-69, binutils-2.10.0.18-1).

--
Manfred

Yes, but that wasn't his patch. He split off the constant text in the bugline to a separate string and made sure /that/ string was used only once.

The old way contained the 'kernel BUG at' string a zillion times, now it only contains 'bla.c: 412' and variants thereof a lot of times. (And most of them are NO duplicates, or you would have had 2 BUGs at the same line...

Met vriendelijke groet,
Pauline Middelink

GPG Key fingerprint = 2D5B 87A7 DDA6 0378 5DEA  BD3B 9A50 B416 E2D0 C3C2
For more details look at my website http://www.polyware.nl/~middelink

Well done, dammit! I've been sitting on that observation for a couple of weeks, now you've beaten me to the patch.

gcc 2.97 (snapshot) does a much better job here, eliminating the strings from the many objects in which those inline functions are not used. But that still leaves quite a lot of full pathnames of build tree header files in the resultant vmlinux. And it'll be quite some while before gcc 3.0 becomes the choice for building the kernel.

Not that I've found.

Indeed.

Well, in many cases, no System.map data is needed to decipher a BUG():
a particular problem can quickly become familiar just by its file:line; which a reliance on System.map might sometimes obscure.

I was leaving BUG()'s printk format unchanged (though shared); but inline functions in header files using INLINE_BUG() instead, its format "kernel BUG inlined from header:%d!\n" i.e. no __FILE__, but peculiarly __LINE__ even so to help identify familiar bugs quickly.

That never quite satisfied me. I think the best would be to combine approaches: keep BUG() as it was (though sharing format), substitute INLINE_BUG() in inline functions, but use your macro (with 0x%p address) for it instead of mine. What do others think? Keith, does the address format need adjusting to suit ksymoops?

These changes need not be kept to i386,
but like you I hadn't yet gone further.

I'd very much like to see some such changes go into Linus' tree. I'm fortunate that I can afford the waste of memory: what bothers me about those strings is, I'm interested in whether objects built with different CONFIG options are equivalent or not, and those strings make that hard to establish (I'm thinking particularly of CONFIG_NOHIGHMEM v. CONFIG_HIGHMEM4G v. CONFIG_HIGHMEM64G, which pull in different header files with inlined BUG()s).

Paul, I apologize if I seem to be trying to steal your thunder:
just a subject close to my heart right now!

Hugh

I would prefer to see

extern const char *kernel_bug;
printk(kernel_bug, __C_FILE__, __C_LINE__)

init/main.c contains
const char *kernel_bug = "kernel BUG at %s:%d!\n";
kernel_bug is also exported.

__C_FILE__ and __C_LINE__ refer to the .c or .s file that included the header, so you get the exact location of the failing code instead of the name and line number of a common header which is used all over the place. __C_FILE__ would be replaced with the minimum string required to uniquely identify the file, e.g. isdn_audio.c (unique at one level) or romfs/inode.c (unique at two levels).

Standard gcc will not do this. But as part of my makefile rewrite I am reading the pre-processed output as it is generated by cpp and before it is read by cc1, to extract data for module symbol versions. It is trivial for my code to look for __C_FILE__ and __C_LINE__ (cpp will have left them alone) and replace them with the relevant string and number.

Would people prefer the C/ASM filename in BUG() messages instead of the include header? If so I will add it to my todo list for the makefile rewrite. Of course you can still use __FILE__ and __LINE_ if you really want to report the error against the include file.

I think include file name makes more sense, otherwise you'll have a hard time to find the actual BUG check. If someone wants more they can decode the oops.

-Andi

Are you sure about that ?
Try this:
a.h:
#define hello printf("%d at %s\n",__LINE__,__FILE__)

a.c:
#include <stdio.h>
#include "a.h"

int main()

{
    hello;
    hello;
    return 0;
}

werewolf:~/ko> gcc a.c -o a
werewolf:~/ko> a
6 at a.c
7 at a.c

(line changes, and name is .c)
because __FILE__ and __LINE__ are expanded with respect to the current SOURCE file (see info cpp), not header file.

As I said before, my choose would be the __func__ + __LINE__ predefined macros. I would prefer to see 'bug in my_buggy_device_init(), line 42'. And you can even drop the __LINE__ part.

J.A. Magallon                                                      $> cd pub
mailto:[email protected]                                          $> more beer

Linux werewolf 2.4.1-ac17 #1 SMP Sat Feb 17 01:47:56 CET 2001 i686

But ....

a.h
static inline void hello(void) { printf("%d at %s\n",__LINE__,__FILE__); }

a.c
#include <stdio.h>
#include "a.h"

int main()

{
    hello();
    hello();
    return 0;
}

# ./a
1 at a.h
1 at a.h

Most uses of BUG() in headers use inline functions instead of #define. 48 occurrences of BUG() in include/{linux,asm-i386}, only 2 are in #define.

Function names are not unique, especially when you get into modules.

__BASE_FILE__ does this. It expands to the thing which you typed on the `gcc' command line.

bix:/home/morton> cat a.h
static inline void hello(void)

{
        printf("%d at %s\n",__LINE__,__BASE_FILE__);
}

bix:/home/morton> cat a.c
#include <stdio.h>
#include "a.h"

int main()

{
        hello();
        hello();
        return 0;
}

bix:/home/morton> gcc -O a.c
bix:/home/morton> ./a.out
3 at a.c
3 at a.c

But __LINE__ is wrong. Forget what I said about __C_FILE__ and __C_LINE__, __C_LINE__ would not work for inline functions. Looks like the best option is a combination of __BASE_FILE__ and function name.

a.h
#define BUG() \
printf("kernel BUG in func %s, file %s\n",__FUNCTION__,__BASE_FILE__);

static inline void hello(void)
{
BUG();
}

a.c
#include <stdio.h>
#include <a.h>

int main()

{
    hello();
    hello();
    return 0;
}

# gcc -I`pwd` `pwd`/a.c -o a
# ./a
kernel BUG in func hello, file /home/kaos/a.c
kernel BUG in func hello, file /home/kaos/a.c

..

I also thought that, but look at the line numbers...wrong and repeated.

J.A. Magallon                                                      $> cd pub
mailto:[email protected]                                          $> more beer

Linux werewolf 2.4.1-ac17 #1 SMP Sat Feb 17 01:47:56 CET 2001 i686

Sure. There's no __BASE_LINE__.

I don't think providing file-n-line in BUG() provides much utility. Just remove it. Replace it with "please feed the following into ksymoops".

If you try
strings vmlinux|grep /usr

you get a bunch of strings like:
..
/usr/src/linux/include/asm/io.h
..

One other couple of Kb. The problem is not that, but the string comes from:
/usr/src/linux/include/asm/io.h:
..(line 110)
/*
* Temporary debugging check to catch old code using
* unmapped ISA addresses. Will be removed in 2.4.
*/
#if 1
extern void *__io_virt_debug(unsigned long x, const char *file, int line); extern unsigned long __io_phys_debug(unsigned long x, const char *file, int li
ne);
#define __io_virt(x) __io_virt_debug((unsigned long)(x), __FILE__, __LINE__)
//#define __io_phys(x) __io_phys_debug((unsigned long)(x), __FILE__, __LINE__) #else
#define __io_virt(x) ((void *)(x))
//#define __io_phys(x) __pa(x)
#endif
..

As you see, it was not removed in 2.4...

J.A. Magallon                                                      $> cd pub
mailto:[email protected]                                          $> more beer

Linux werewolf 2.4.1-ac17 #1 SMP Sat Feb 17 01:47:56 CET 2001 i686

Loking at it (arch/i386/lib/iodebug.c):
void * __io_virt_debug(unsigned long x, const char *file, int line)

{
    if (x < PAGE_OFFSET) {
        printk("io mapaddr 0x%05lx not valid at %s:%d!\n", x, file, line);
        return __va(x);
    }
    return (void *)x;
}

is changed (if you turn off the #if 1), from 1_function_call+1_if+cache pollution with the function code to nothing (just a cast). This will make some difference in performance, won't it ?

J.A. Magallon                                                      $> cd pub
mailto:[email protected]                                          $> more beer

Linux werewolf 2.4.1-ac17 #1 SMP Sat Feb 17 01:47:56 CET 2001 i686

Speaking of io_virt_debug... This patch that makes all your MMIO devices run faster, simply by eliminating io_virt_debug :)

(sorry for the CONFIG_VMDUMP pollution in config.in... I'm too slack to rediff nicely)

Jeff

Index: linux_2_4/arch/i386/config.in

diff -u linux_2_4/arch/i386/config.in:1.1.1.20 linux_2_4/arch/i386/config.in:1.1.1.20.2.2
@@ -365,5 +365,7 @@
 comment 'Kernel hacking'
 
 #bool 'Debug kmalloc/kfree' CONFIG_DEBUG_MALLOC
+bool 'I/O read/write debugging' CONFIG_DEBUG_IOVIRT
 bool 'Magic SysRq key' CONFIG_MAGIC_SYSRQ
+bool 'Support kernel crash dump capabilities' CONFIG_VMDUMP
 endmenu

Index: linux_2_4/arch/i386/kernel/i386_ksyms.c

diff -u linux_2_4/arch/i386/kernel/i386_ksyms.c:1.1.1.14 linux_2_4/arch/i386/kernel/i386_ksyms.c:1.1.1.14.2.1
@@ -59,7 +60,9 @@
 EXPORT_SYMBOL(dump_extended_fpu);
 EXPORT_SYMBOL(__ioremap);
 EXPORT_SYMBOL(iounmap);
+#ifdef CONFIG_DEBUG_IOVIRT
 EXPORT_SYMBOL(__io_virt_debug);
+#endif
 EXPORT_SYMBOL(enable_irq);
 EXPORT_SYMBOL(disable_irq);
 EXPORT_SYMBOL(disable_irq_nosync);

Index: linux_2_4/arch/i386/lib/Makefile

diff -u linux_2_4/arch/i386/lib/Makefile:1.1.1.11 linux_2_4/arch/i386/lib/Makefile:1.1.1.11.2.1
@@ -8,10 +8,11 @@
 L_TARGET = lib.a
 
 obj-y = checksum.o old-checksum.o delay.o \
-	usercopy.o getuser.o putuser.o iodebug.o \
+	usercopy.o getuser.o putuser.o \
 	memcpy.o
 
 obj-$(CONFIG_X86_USE_3DNOW) += mmx.o
 obj-$(CONFIG_HAVE_DEC_LOCK) += dec_and_lock.o
+obj-$(CONFIG_DEBUG_IOVIRT)  += iodebug.o
 
 include $(TOPDIR)/Rules.make

Index: linux_2_4/include/asm-i386/io.h

diff -u linux_2_4/include/asm-i386/io.h:1.1.1.1 linux_2_4/include/asm-i386/io.h:1.1.1.1.118.1
@@ -1,6 +1,8 @@
 #ifndef _ASM_IO_H
 #define _ASM_IO_H
 
+#include <linux/config.h>
+
 /*
  * This file contains the definitions for the x86 IO instructions
  * inb/inw/inl/outb/outw/outl and the "string versions" of the same

@@ -111,7 +113,7 @@
  * Temporary debugging check to catch old code using
  * unmapped ISA addresses. Will be removed in 2.4.
  */
-#if 1
+#if CONFIG_DEBUG_IOVIRT
   extern void *__io_virt_debug(unsigned long x, const char *file, int line);
   extern unsigned long __io_phys_debug(unsigned long x, const char *file, int line);
   #define __io_virt(x) __io_virt_debug((unsigned long)(x), __FILE__, __LINE__)

Or better __FUNCTION__. Or even better __func__ that is gcc and ANSI99 C compatible.

Time to make a patch...

J.A. Magallon                                                      $> cd pub
mailto:[email protected]                                          $> more beer

Linux werewolf 2.4.1-ac17 #1 SMP Sat Feb 17 01:47:56 CET 2001 i686