[PATCH] Fix BUG macro

[PATCH] Fix BUG macro

[Rusty Russell]

Replaces filename with object name.  Sure, it's not as canonical, but
it means that ccache works across different directories (at the
moment, ccache gets almost no caceh hits when you compile in a
different dir).

Thanks to Stephen Rothwell for the inspiration,
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

diff -urN -I \$.*\$ --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5.15/include/asm-i386/page.h working-2.5.15-rcu/include/asm-i386/page.h
--- linux-2.5.15/include/asm-i386/page.h	Wed May 15 19:53:25 2002
+++ working-2.5.15-rcu/include/asm-i386/page.h	Thu May 16 17:34:47 2002
@@ -96,11 +96,16 @@
  */
 
 #if 1	/* Set to zero for a slightly smaller kernel */
+#define __STRINGIZE2(x) #x
+#define __STRINGIZE(x) __STRINGIZE2(x)
 #define BUG()				\
  __asm__ __volatile__(	"ud2\n"		\
 			"\t.word %c0\n"	\
 			"\t.long %c1\n"	\
-			 : : "i" (__LINE__), "i" (__FILE__))
+			"\t.long %c2\n"	\
+			 : : "i" (__LINE__), \
+			"i" (__STRINGIZE(KBUILD_BASENAME)), \
+			"i" (__FUNCTION__))
 #else
 #define BUG() __asm__ __volatile__("ud2\n")
 #endif
diff -urN -I \$.*\$ --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5.15/arch/i386/kernel/traps.c working-2.5.15-rcu/arch/i386/kernel/traps.c
--- linux-2.5.15/arch/i386/kernel/traps.c	Tue Apr 23 11:39:32 2002
+++ working-2.5.15-rcu/arch/i386/kernel/traps.c	Thu May 16 17:43:28 2002
@@ -245,7 +245,7 @@
 {
 	unsigned short ud2;
 	unsigned short line;
-	char *file;
+	char *object, *func;
 	char c;
 	unsigned long eip;
 
@@ -262,11 +262,14 @@
 		goto no_bug;
 	if (__get_user(line, (unsigned short *)(eip + 2)))
 		goto bug;
-	if (__get_user(file, (char **)(eip + 4)) ||
-		(unsigned long)file < PAGE_OFFSET || __get_user(c, file))
-		file = "<bad filename>";
+	if (__get_user(object, (char **)(eip + 4)) ||
+		(unsigned long)object < PAGE_OFFSET || __get_user(c, object))
+		object = "<bad objectname>";
+	if (__get_user(func, (char **)(eip + 8)) ||
+		(unsigned long)func < PAGE_OFFSET || __get_user(c, func))
+		func = "<bad funcname>";
 
-	printk("kernel BUG at %s:%d!\n", file, line);
+	printk("kernel BUG at %s:%s:%d!\n", object, func, line);
 
 no_bug:
 	return;

Re: [PATCH] Fix BUG macro

[Hugh Dickins]

On Thu, 16 May 2002, Rusty Russell wrote:

> Replaces filename with object name.  Sure, it's not as canonical, but
> it means that ccache works across different directories (at the
> moment, ccache gets almost no caceh hits when you compile in a
> different dir).

__STRINGIZE(KBUILD_BASENAME) sounds good, except in inline
function from header file; perhaps that's why you're adding
__FUNCTION__, which will waste a lot of space.  Suggest you
should test __INCLUDE_LEVEL__: use __STRINGIZE(KBUILD_BASENAME)
at __INCLUDE_LEVEL__ 0, __FUNCTION__ at included levels?

[ empty space in which I try to keep quiet about ud2 disassembly ]

Hugh

Re: [PATCH] Fix BUG macro

[Ghozlane Toumi]

Hi

On Thursday 16 May 2002 04:08, Rusty Russell wrote:

>--- linux-2.5.15/include/asm-i386/page.h        Wed May 15 19:53:25 2002
>+++ working-2.5.15-rcu/include/asm-i386/page.h  Thu May 16 17:34:47 2002
>@@ -96,11 +96,16 @@
>  */
> 
> #if 1  /* Set to zero for a slightly smaller kernel */
>+#define __STRINGIZE2(x) #x
>+#define __STRINGIZE(x) __STRINGIZE2(x)
> #define BUG()                          \
>  __asm__ __volatile__( "ud2\n"         \
>                        "\t.word %c0\n" \
>                        "\t.long %c1\n" \
>-                        : : "i" (__LINE__), "i" (__FILE__))
>+                       "\t.long %c2\n" \
>+                        : : "i" (__LINE__), \
>+                       "i" (__STRINGIZE(KBUILD_BASENAME)), \
>+                       "i" (__FUNCTION__))
> #else
> #define BUG() __asm__ __volatile__("ud2\n")
> #endif

Minor nit : any reason why you don't use  __stringify from 
include/linux/stringify.h ?

Ghoz

Re: [PATCH] Fix BUG macro

[Rusty Russell]

In message <Pine.LNX.4.21.0205161154410.1036-100000@localhost.localdomain> you 
write:
> On Thu, 16 May 2002, Rusty Russell wrote:
> 
> > Replaces filename with object name.  Sure, it's not as canonical, but
> > it means that ccache works across different directories (at the
> > moment, ccache gets almost no caceh hits when you compile in a
> > different dir).
> 
> __STRINGIZE(KBUILD_BASENAME) sounds good, except in inline
> function from header file; perhaps that's why you're adding
> __FUNCTION__, which will waste a lot of space.

Um, show me where sizeof(KBUILD_BASENAME) + sizeof(__FUNCTION__) >
sizeof(__FILENAME__).

>  Suggest you
> should test __INCLUDE_LEVEL__: use __STRINGIZE(KBUILD_BASENAME)
> at __INCLUDE_LEVEL__ 0, __FUNCTION__ at included levels?

That'd be cute.  Of course best would be using KBUILD_BASENAME (which
means BUG() in a header works correclty) and make BUG() take a string
arg.

Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: [PATCH] Fix BUG macro

[Rusty Russell]

In message <20020516162841.PYWL19243.tomts15-srv.bellnexxia.net@there> you writ
e:
> Minor nit : any reason why you don't use  __stringify from 
> include/linux/stringify.h ?

Ignorance.  Mea culpa.  Linus, please apply.

diff -urN -I \$.*\$ --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.15/arch/i386/kernel/traps.c working-2.5.15-bug/arch/i386/kernel/traps.c
--- linux-2.5.15/arch/i386/kernel/traps.c	Tue Apr 23 11:39:32 2002
+++ working-2.5.15-bug/arch/i386/kernel/traps.c	Fri May 17 12:23:06 2002
@@ -245,7 +245,7 @@
 {
 	unsigned short ud2;
 	unsigned short line;
-	char *file;
+	char *object, *func;
 	char c;
 	unsigned long eip;
 
@@ -262,11 +262,14 @@
 		goto no_bug;
 	if (__get_user(line, (unsigned short *)(eip + 2)))
 		goto bug;
-	if (__get_user(file, (char **)(eip + 4)) ||
-		(unsigned long)file < PAGE_OFFSET || __get_user(c, file))
-		file = "<bad filename>";
+	if (__get_user(object, (char **)(eip + 4)) ||
+		(unsigned long)object < PAGE_OFFSET || __get_user(c, object))
+		object = "<bad objectname>";
+	if (__get_user(func, (char **)(eip + 8)) ||
+		(unsigned long)func < PAGE_OFFSET || __get_user(c, func))
+		func = "<bad funcname>";
 
-	printk("kernel BUG at %s:%d!\n", file, line);
+	printk("kernel BUG at %s:%s:%d!\n", object, func, line);
 
 no_bug:
 	return;
diff -urN -I \$.*\$ --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.15/include/asm-i386/page.h working-2.5.15-bug/include/asm-i386/page.h
--- linux-2.5.15/include/asm-i386/page.h	Mon May  6 16:00:10 2002
+++ working-2.5.15-bug/include/asm-i386/page.h	Fri May 17 12:23:59 2002
@@ -10,6 +10,7 @@
 #ifndef __ASSEMBLY__
 
 #include <linux/config.h>
+#include <linux/stringify.h>
 
 #ifdef CONFIG_X86_USE_3DNOW
 
@@ -100,7 +101,10 @@
  __asm__ __volatile__(	"ud2\n"		\
 			"\t.word %c0\n"	\
 			"\t.long %c1\n"	\
-			 : : "i" (__LINE__), "i" (__FILE__))
+			"\t.long %c2\n"	\
+			 : : "i" (__LINE__), \
+			"i" (__stringify(KBUILD_BASENAME)), \
+			"i" (__FUNCTION__))
 #else
 #define BUG() __asm__ __volatile__("ud2\n")
 #endif
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: [PATCH] Fix BUG macro

[Andrew Morton]

Rusty Russell wrote:
> 
> ...
>   __asm__ __volatile__( "ud2\n"         \
>                         "\t.word %c0\n" \
>                         "\t.long %c1\n" \
> -                        : : "i" (__LINE__), "i" (__FILE__))
> +                       "\t.long %c2\n" \
> +                        : : "i" (__LINE__), \
> +                       "i" (__stringify(KBUILD_BASENAME)), \
> +                       "i" (__FUNCTION__))

I'd share Hugh's concern on this one.  Adding the name of the
containing function to every BUG() expansion will increase
the size of .rodata.

Do you have before-and-after /usr/bin/size output?

-

Re: [PATCH] Fix BUG macro

[Rusty Russell]

In message <3CE46DF6.62EF67E0@zip.com.au> you write:
> I'd share Hugh's concern on this one.  Adding the name of the
> containing function to every BUG() expansion will increase
> the size of .rodata.

Andrew: you used to be such a bright young man. 8)

> Do you have before-and-after /usr/bin/size output?

I even put the kernel in /usr/src/, not my home directory, to help you
out here.

before: 
	   text    data     bss     dec     hex filename
	1192605  355848  353780 1902233  1d0699 vmlinux

after: 
	   text    data     bss     dec     hex filename
	1168396  355848  353780 1878024  1ca808 vmlinux

Understand?
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: [PATCH] Fix BUG macro

[Andrew Morton]

Rusty Russell wrote:
> Understand?

No.

Are you using a compiler which performs inter-compilation unit
string sharing?

Some explanation of how this works, and of why I should not fill
your ear with toothpaste would be appreciated here.

-

Re: [PATCH] Fix BUG macro

[Rusty Russell]

In message <3CE48C08.B0E59851@zip.com.au> you write:
> Some explanation of how this works, and of why I should not fill
> your ear with toothpaste would be appreciated here.

When an include file is found using "-I dir", __FILE__ in that include
file is "dir/filename":

gcc -D__KERNEL__ -I/usr/src/working-2.5.15-bug/include -Wall -Wstrict-prototypes -Wno-trigraphs -O2 -fomit-frame-pointer -fno-strict-aliasing -fno-common -pipe -mpreferred-stack-boundary=2 -march=i686   -DKBUILD_BASENAME=delay  -c -o delay.o delay.c

Hope that helps,
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: [PATCH] Fix BUG macro

[Andrew Morton]

Rusty Russell wrote:
> 
> In message <3CE48C08.B0E59851@zip.com.au> you write:
> > Some explanation of how this works, and of why I should not fill
> > your ear with toothpaste would be appreciated here.
> 
> When an include file is found using "-I dir", __FILE__ in that include
> file is "dir/filename":
> 
> gcc -D__KERNEL__ -I/usr/src/working-2.5.15-bug/include -Wall -Wstrict-prototypes -Wno-trigraphs -O2 -fomit-frame-pointer -fno-strict-aliasing -fno-common -pipe -mpreferred-stack-boundary=2 -march=i686   -DKBUILD_BASENAME=delay  -c -o delay.o delay.c
> 
> Hope that helps,

It would help if you told us whether you're using a toolchain which
combines strings across .o files.

Presumably, you're not.  So the space savings which you're seeing
are due to lessening the bloat which is caused by the inline functions
in headers which expand BUG().   Which is what out_of_line_bug() does too.
Assuming the toolchain fixes that for us in 2.5, you've gone and added
zillions of function names to the kernel image.

-

Re: [PATCH] Fix BUG macro

[Hugh Dickins]

On Fri, 17 May 2002, Rusty Russell wrote:
> 
> Um, show me where sizeof(KBUILD_BASENAME) + sizeof(__FUNCTION__) >
> sizeof(__FILENAME__).

If you're talking about kbuild2.5 where all the __FILENAME__s
become absolute (for good reason) instead of the leafnames they
usually were (header files excepted): not many instances, but
that's not the point.  The point is that all the instances of
__FILENAME__ within one compilation unit (across compilation
units? rumours that that will become so) get combined into a
single string, so little overhead to many BUG()s in one file.
But you are now creating lots of __FUNCTION__ strings which
cannot be combined to the nearly same extent (though, yeah,
all those "__free_pages_ok"s will get combined into one).

Your BUG() may have made space savings relative to kbuild2.5,
I don't know, but that's because kbuild2.5 has inadvertently
added a bloat there, and I thought we were looking for ways
to recover from that (I'd earlier proposed KBUILD_BASENAME,
but didn't understand stringification), to get back to
Andrew's lean mean clean implementation (before which we
needed CONFIG_DEBUG_BUGVERBOSE=n to cut out the overhead).

Hugh

Re: [PATCH] Fix BUG macro

[Rusty Russell]

In message <3CE4A6CD.75039761@zip.com.au> you write:
> It would help if you told us whether you're using a toolchain which
> combines strings across .o files.

No, am not.

> Presumably, you're not.  So the space savings which you're seeing
> are due to lessening the bloat which is caused by the inline functions
> in headers which expand BUG().   Which is what out_of_line_bug() does too.

... which hasn't been accepted by Linus...

> Assuming the toolchain fixes that for us in 2.5, you've gone and added
> zillions of function names to the kernel image.

I thought you said the toolchain would merge them?

I don't care about the bloat: I care about the compile time exploding
because every file is different in different trees, due to the
filename strings.

It'd be very nice to have a solution to this, and I'll keep sending
patches to Linus until he applies them or says "no do it this way".

Cheers!
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: [PATCH] Fix BUG macro

[Rusty Russell]

In message <Pine.LNX.4.21.0205170839240.1369-100000@localhost.localdomain> you 
write:
> On Fri, 17 May 2002, Rusty Russell wrote:
> > 
> > Um, show me where sizeof(KBUILD_BASENAME) + sizeof(__FUNCTION__) >
> > sizeof(__FILENAME__).
> 
> If you're talking about kbuild2.5

No.  It's the include files, which makes up the majority of strings.
See reply to Andrew Morton.

Summary: faster cached compiles no bloat,
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: [PATCH] Fix BUG macro

[Hugh Dickins]

On Fri, 17 May 2002, Rusty Russell wrote:
> In message <Pine.LNX.4.21.0205170839240.1369-100000@localhost.localdomain> you 
> write:
> > On Fri, 17 May 2002, Rusty Russell wrote:
> > > 
> > > Um, show me where sizeof(KBUILD_BASENAME) + sizeof(__FUNCTION__) >
> > > sizeof(__FILENAME__).
> > 
> > If you're talking about kbuild2.5
> 
> No.  It's the include files, which makes up the majority of strings.

If they do make up the majority of strings, that's partly because
you don't have Andrew's out_of_line_bug work in your tree, partly
because your linker isn't combining strings (mine neither, does any?),
partly because (I suspect) you're overlooking the vast number of BUG
__FILE__ strings which are just leafnames, to each of which you're
now proposing to add one or more __FUNCTION__ strings.

And note that __LINE__ number is of __FILE__, not of KBUILD_BASENAME,
nor of __FUNCTION__; so __INCLUDE_LEVEL__ may be necessary to make
sensible messages (if you insist on proceeding without out_of_line_bug:
which Andrew didn't push to Linus, in belief that a string-combining
linker is on the way).

> See reply to Andrew Morton.

I did see your exchanges with Andrew, but you don't seem to be
understanding one another, so I'm trying to be more explicit.

__HUGH__

Re: [PATCH] Fix BUG macro

[Andreas Schwab]

Hugh Dickins <hugh@veritas.com> writes:

|> On Fri, 17 May 2002, Rusty Russell wrote:
|> > In message <Pine.LNX.4.21.0205170839240.1369-100000@localhost.localdomain> you 
|> > write:
|> > > On Fri, 17 May 2002, Rusty Russell wrote:
|> > > > 
|> > > > Um, show me where sizeof(KBUILD_BASENAME) + sizeof(__FUNCTION__) >
|> > > > sizeof(__FILENAME__).
|> > > 
|> > > If you're talking about kbuild2.5
|> > 
|> > No.  It's the include files, which makes up the majority of strings.
|> 
|> If they do make up the majority of strings, that's partly because
|> you don't have Andrew's out_of_line_bug work in your tree, partly
|> because your linker isn't combining strings (mine neither, does any?),

Recent gcc and ld do.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE GmbH, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Fix BUG macro

[Rusty Russell]

In message <Pine.LNX.4.21.0205171220480.986-100000@localhost.localdomain> you w
rite:
> If they do make up the majority of strings, that's partly because
> you don't have Andrew's out_of_line_bug work in your tree, partly
> because your linker isn't combining strings (mine neither, does any?),
> partly because (I suspect) you're overlooking the vast number of BUG
> __FILE__ strings which are just leafnames, to each of which you're
> now proposing to add one or more __FUNCTION__ strings.

I don't care about the size of the kernel, I care about the fact that
the compile is 5x slower than it needs to be because the contents of
every pre-processed file depends on where the kernel tree happens to
be (see http://ccache.samba.org)

And I compile a lot of Linus kernels,
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: [PATCH] Fix BUG macro

[Hugh Dickins]

On Fri, 17 May 2002, Rusty Russell wrote:
> 
> I don't care about the size of the kernel, I care about the fact that
> the compile is 5x slower than it needs to be because the contents of
> every pre-processed file depends on where the kernel tree happens to
> be (see http://ccache.samba.org)

All power to you, speeding up your (and others') builds.  All power
to you, eliminating the absolute pathnames (which I abhor for making
comparisons harder).  But some do care about the size of the kernel
too: your additional __FUNCTION__ hurts them and doesn't help you.

Hugh

Re: [PATCH] Fix BUG macro

[Kai Germaschewski]

On Fri, 17 May 2002, Rusty Russell wrote:

> I don't care about the bloat: I care about the compile time exploding
> because every file is different in different trees, due to the
> filename strings.
> 
> It'd be very nice to have a solution to this, and I'll keep sending
> patches to Linus until he applies them or says "no do it this way".

Well, a way to work around this would be to replace

	-I$(TOPDIR)/include

with

	-I../../include

on the command line, I suppose, with the right amount of "../". A bit 
hackish, but it should do.

--Kai

Re: [PATCH] Fix BUG macro

[Andrew Morton]

Kai Germaschewski wrote:
> 
> On Fri, 17 May 2002, Rusty Russell wrote:
> 
> > I don't care about the bloat: I care about the compile time exploding
> > because every file is different in different trees, due to the
> > filename strings.
> >
> > It'd be very nice to have a solution to this, and I'll keep sending
> > patches to Linus until he applies them or says "no do it this way".
> 
> Well, a way to work around this would be to replace
> 
>         -I$(TOPDIR)/include
> 
> with
> 
>         -I../../include
> 
> on the command line, I suppose, with the right amount of "../". A bit
> hackish, but it should do.

Almost..  The final solution to all problems is to merge
kbuild-2.5 and then to teach it to use relative pathnames
when performing a build within the source tree.  Presumably
that's not hard, but I'm surely about to learn why it's
not feasible.

-

Re: [PATCH] Fix BUG macro

[Stevie O]

At 11:41 AM 5/17/2002 -0700, Andrew Morton wrote:

>> Well, a way to work around this would be to replace
>> 
>>         -I$(TOPDIR)/include
>> 
>> with
>> 
>>         -I../../include
>> 
>> on the command line, I suppose, with the right amount of "../". A bit
>> hackish, but it should do.
>
>Almost..  The final solution to all problems is to merge
>kbuild-2.5 and then to teach it to use relative pathnames
>when performing a build within the source tree.  Presumably
>that's not hard, but I'm surely about to learn why it's
>not feasible.

What if you just let $TOPDIR = '../..' ?


--
Stevie-O

Real programmers use COPY CON PROGRAM.EXE

Re: [PATCH] Fix BUG macro

[Kai Germaschewski]

On Fri, 17 May 2002, Stevie O wrote:

> What if you just let $TOPDIR = '../..' ?

It's not quite as easy (TOPDIR changes with the subdir, CFLAGS are
actually passed to sub-makes, ...), but I got a crude version working 
here. (There's actually a second way to do this more nicely, too).

Anyway, I have pending cleanups to Rules.make, which will make that change
easier too, so I'll just do it step by step.

--Kai

Re: [PATCH] Fix BUG macro

[Linus Torvalds]

On Fri, 17 May 2002, Andrew Morton wrote:
>
> Almost..  The final solution to all problems is to merge
> kbuild-2.5 and then to teach it to use relative pathnames
> when performing a build within the source tree.  Presumably
> that's not hard, but I'm surely about to learn why it's
> not feasible.

I'm hoping we can get there in small steps, rather than a big traumatic
merge. I'd love to just try to merge it piecemeal.

Especially as I don't find the existign system so broken.

		Linus