Re: [Compile Regression in 2.4.25-pre8][PATCH 37/42]

Re: [Compile Regression in 2.4.25-pre8][PATCH 37/42]

[Jon Foster]

Hi,

Kronos wrote:
> Comments on the patch?
> 
> diff -Nru -X dontdiff linux-2.4-vanilla/include/asm-i386/page.h linux-2.4/include/asm-i386/page.h
> --- linux-2.4-vanilla/include/asm-i386/page.h	Tue Nov 11 18:05:52 2003
> +++ linux-2.4/include/asm-i386/page.h	Wed Feb  4 14:43:00 2004
> @@ -95,14 +95,28 @@
>   * undefined" opcode for parsing in the trap handler.
>   */
>  
> +#ifdef __bug
> +static inline void __dummy_noreturn(void) __bug;
> +static inline void __dummy_noreturn(void) {
> +	while(1) {}
> +}

My first thought was "this obviously makes the kernel bigger".  GCC will
actually compile this loop - it's only a single jump instruction, possibly
with a nop for branch target alignment, but it's duplicated for every
call to BUG().

On the other hand, marking BUG() as noreturn means that GCC won't have
to generate the code following the BUG().  Even if that code is just
a jump, it's a similar size to the code that this patch adds.  So it's
not as obvious as I first thought, and does need measuring.

Tested with Linux 2.4.22-gentoo-r5 & my normal kernel config, by
measuring total uncompressed size of vmlinux:

Without patch:   3,475,213 bytes
With patch:      3,475,149 bytes
This patch saves:       64 bytes

OK, that saving is lost in the noise, but it seems that this patch
isn't going to change the kernel size much (if at all).  And it is
good to let the compiler know about BUG(), so it doesn't emit
spurious warnings and can catch unused code.  So I like this patch.

Obviously, the most elegent (and space-saving) solution would be
if GCC allowed you to mark a block of inline assembly as noreturn.
Any GCC folks out there able to help?

Kind regards,

Jon


> +#else
> +#define __dummy_noreturn() do {} while(0)
> +#endif
> +
> #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__))
> +#define BUG() do { \
> + __asm__ __volatile__( "ud2\n" \
> + "\t.word %c0\n" \
> + "\t.long %c1\n" \
> + : : "i" (__LINE__), "i" (__FILE__)); \
> + __dummy_noreturn(); \
> + } while(0)
> #else
> -#define BUG() __asm__ __volatile__("ud2\n")
> +#define BUG() do { \
> + __asm__ __volatile__("ud2\n"); \
> + __dummy_noreturn(); \
> + } while(0)
> #endif
> 
> #define PAGE_BUG(page) do { \
> diff -Nru -X dontdiff linux-2.4-vanilla/include/linux/compiler.h linux-2.4/include/linux/compiler.h
> --- linux-2.4-vanilla/include/linux/compiler.h Tue Sep 18 23:12:45 2001
> +++ linux-2.4/include/linux/compiler.h Tue Feb 3 18:29:56 2004
> @@ -13,4 +13,11 @@
> #define likely(x) __builtin_expect((x),1)
> #define unlikely(x) __builtin_expect((x),0)
> 
> +#if __GNUC__ >= 3
> +/* __noreturn__ is implemented since gcc 2.5.
> + * __always_inline__ is not present in 2.9x
> + */
> +#define __bug __attribute__((__noreturn__, __always_inline__))
> +#endif
> +
> #endif /* __LINUX_COMPILER_H */

Re: [Compile Regression in 2.4.25-pre8][PATCH 37/42]

[Kronos]

Il Tue, Feb 03, 2004 at 11:14:21PM +0000, Philippe Elie ha scritto: 
> > diff -Nru -X dontdiff linux-2.4-vanilla/include/asm-i386/page.h linux-2.4/include/asm-i386/page.h
> > --- linux-2.4-vanilla/include/asm-i386/page.h	Tue Nov 11 18:05:52 2003
> > +++ linux-2.4/include/asm-i386/page.h	Tue Feb  3 07:26:04 2004
> > @@ -10,6 +10,7 @@
> >  #ifndef __ASSEMBLY__
> >  
> >  #include <linux/config.h>
> > +#include <linux/compiler.h>
> >  
> >  #ifdef CONFIG_X86_USE_3DNOW
> >  
> > @@ -94,6 +95,26 @@
> >   * The offending file and line are encoded after the "officially
> >   * undefined" opcode for parsing in the trap handler.
> >   */
> > +#ifdef __bug
> > +#if 1	/* Set to zero for a slightly smaller kernel */
> > +__bug void __bugfn(void) {
> > +	while(1) {
> > +		 __asm__ __volatile__(	"ud2\n"
> > +					"\t.word %c0\n"
> > +					"\t.long %c1\n"
> > +					: : "i" (__LINE__), "i" (__FILE__));
> > + 	}
> > +}
> 
> You must pass __LINE__ and __FILE__ as parameter to this function.

Good point. It's not that easy though ;)
BUG() places file name and line number after the invalid opcode (ud2) and
the kernel trap handler decodes these information when printing a BUG().
In order to do that you need immediate values to pass to assembler.

I think that the following patch will work. This is a BUG() from a dummy module
which calls BUG() in the init function:

kernel BUG at /tmp/mod3.c:22!
invalid operand: 0000 [#3]
CPU:    0
EIP:    0060:[<e09ed003>]    Tainted: PF
EFLAGS: 00010202
EIP is at modinit+0x3/0x20 [mod3]
eax: e09ed000   ebx: cdf9a000   ecx: c036de78   edx: 00000000
esi: e09ed180   edi: c036de78   ebp: cdf9bfa4   esp: cdf9bfa4
ds: 007b   es: 007b   ss: 0068
Process insmod (pid: 1960, threadinfo=cdf9a000 task=dd7546b0)
Stack: cdf9bfbc c01345d8 0804b018 080486dd 4009199b c036de5c cdf9a000 c0109067
       0804b018 0000055c 0804b008 080486dd 4009199b bffffb28 00000080 0000007b
       0000007b 00000080 400fb4ce 00000073 00000246 bffffae0 0000007b
Call Trace:
 [<c01345d8>] sys_init_module+0x118/0x240
 [<c0109067>] syscall_call+0x7/0xb

Code: 0f 0b 16 00 25 d0 9e e0 90 8d 74 26 00 eb fe 8d b4 26 00 00
 Segmentation fault
		 
It seems ok to me (kernel is tainted because I forgot MODULE_LICENSE...).
Comments on the patch?

diff -Nru -X dontdiff linux-2.4-vanilla/include/asm-i386/page.h linux-2.4/include/asm-i386/page.h
--- linux-2.4-vanilla/include/asm-i386/page.h	Tue Nov 11 18:05:52 2003
+++ linux-2.4/include/asm-i386/page.h	Wed Feb  4 14:43:00 2004
@@ -95,14 +95,28 @@
  * undefined" opcode for parsing in the trap handler.
  */
 
+#ifdef __bug
+static inline void __dummy_noreturn(void) __bug;
+static inline void __dummy_noreturn(void) {
+	while(1) {}
+}
+#else
+#define __dummy_noreturn() do {} while(0)
+#endif
+
 #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__))
+#define BUG() do {						\
+		__asm__ __volatile__(	"ud2\n"			\
+			"\t.word %c0\n"				\
+			"\t.long %c1\n"				\
+			 : : "i" (__LINE__), "i" (__FILE__));	\
+		__dummy_noreturn();				\
+	} while(0)
 #else
-#define BUG() __asm__ __volatile__("ud2\n")
+#define BUG() do {						\
+		__asm__ __volatile__("ud2\n");			\
+		__dummy_noreturn();				\
+	} while(0)
 #endif
 
 #define PAGE_BUG(page) do { \
diff -Nru -X dontdiff linux-2.4-vanilla/include/linux/compiler.h linux-2.4/include/linux/compiler.h
--- linux-2.4-vanilla/include/linux/compiler.h	Tue Sep 18 23:12:45 2001
+++ linux-2.4/include/linux/compiler.h	Tue Feb  3 18:29:56 2004
@@ -13,4 +13,11 @@
 #define likely(x)	__builtin_expect((x),1)
 #define unlikely(x)	__builtin_expect((x),0)
 
+#if __GNUC__ >= 3
+/* __noreturn__ is implemented since gcc 2.5. 
+ * __always_inline__ is not present in 2.9x
+ */
+#define __bug __attribute__((__noreturn__, __always_inline__))
+#endif
+
 #endif /* __LINUX_COMPILER_H */


Luca
-- 
Reply-To: kronos@kronoz.cjb.net
Home: http://kronoz.cjb.net
"L'ottimista pensa che questo sia il migliore dei mondi possibili. 
 Il pessimista sa che e` vero" -- J. Robert Oppenheimer

Re: [Compile Regression in 2.4.25-pre8][PATCH 37/42]

[Philippe Elie]

On Tue, 03 Feb 2004 at 22:07 +0000, Kronos wrote:

> Ok, I cooked up this patch. BUG() is a function marked as noreturn that is
> always inlined. Unfortunately, gcc prior to 3.x does not support
> __always_inline__ attribute, so I had to revert to the old macro with older
> compiler.
> 
> The patch fixes warnings with newer compiler, but not with older ones.
> Note that the while(1) is needed, otherwise gcc will say that the
> function marked as noreturn does actually return.
> 
> Comments?
> 
> diff -Nru -X dontdiff linux-2.4-vanilla/include/asm-i386/page.h linux-2.4/include/asm-i386/page.h
> --- linux-2.4-vanilla/include/asm-i386/page.h	Tue Nov 11 18:05:52 2003
> +++ linux-2.4/include/asm-i386/page.h	Tue Feb  3 07:26:04 2004
> @@ -10,6 +10,7 @@
>  #ifndef __ASSEMBLY__
>  
>  #include <linux/config.h>
> +#include <linux/compiler.h>
>  
>  #ifdef CONFIG_X86_USE_3DNOW
>  
> @@ -94,6 +95,26 @@
>   * The offending file and line are encoded after the "officially
>   * undefined" opcode for parsing in the trap handler.
>   */
> +#ifdef __bug
> +#if 1	/* Set to zero for a slightly smaller kernel */
> +__bug void __bugfn(void) {
> +	while(1) {
> +		 __asm__ __volatile__(	"ud2\n"
> +					"\t.word %c0\n"
> +					"\t.long %c1\n"
> +					: : "i" (__LINE__), "i" (__FILE__));
> + 	}
> +}

You must pass __LINE__ and __FILE__ as parameter to this function.

regards,
Philippe Elie

Re: [Compile Regression in 2.4.25-pre8][PATCH 37/42]

[Kronos]

Il Mon, Feb 02, 2004 at 10:08:48PM +0100, Geert Uytterhoeven ha scritto: 
> On Mon, 2 Feb 2004, Kronos wrote:
> > siimage.c:65: warning: control reaches end of non-void function
> >
> > The last statement before the end is BUG(), but I added a return to
> > silence the warning.
> >
> > diff -Nru -X dontdiff linux-2.4-vanilla/drivers/ide/pci/siimage.c linux-2.4/drivers/ide/pci/siimage.c
> > --- linux-2.4-vanilla/drivers/ide/pci/siimage.c	Tue Nov 11 17:51:38 2003
> > +++ linux-2.4/drivers/ide/pci/siimage.c	Sat Jan 31 19:07:56 2004
> > @@ -62,6 +62,9 @@
> >  			return 0;
> >  	}
> >  	BUG();
> > +
> > +	/* gcc will complain */
> > +	return 0;
> >  }
> 
> What about adding `attribute ((noreturn))' to the declaration of BUG() instead?

Ok, I cooked up this patch. BUG() is a function marked as noreturn that is
always inlined. Unfortunately, gcc prior to 3.x does not support
__always_inline__ attribute, so I had to revert to the old macro with older
compiler.

The patch fixes warnings with newer compiler, but not with older ones.
Note that the while(1) is needed, otherwise gcc will say that the
function marked as noreturn does actually return.

Comments?

diff -Nru -X dontdiff linux-2.4-vanilla/include/asm-i386/page.h linux-2.4/include/asm-i386/page.h
--- linux-2.4-vanilla/include/asm-i386/page.h	Tue Nov 11 18:05:52 2003
+++ linux-2.4/include/asm-i386/page.h	Tue Feb  3 07:26:04 2004
@@ -10,6 +10,7 @@
 #ifndef __ASSEMBLY__
 
 #include <linux/config.h>
+#include <linux/compiler.h>
 
 #ifdef CONFIG_X86_USE_3DNOW
 
@@ -94,6 +95,26 @@
  * The offending file and line are encoded after the "officially
  * undefined" opcode for parsing in the trap handler.
  */
+#ifdef __bug
+#if 1	/* Set to zero for a slightly smaller kernel */
+__bug void __bugfn(void) {
+	while(1) {
+		 __asm__ __volatile__(	"ud2\n"
+					"\t.word %c0\n"
+					"\t.long %c1\n"
+					: : "i" (__LINE__), "i" (__FILE__));
+ 	}
+}
+#else
+__bug void __bugfn(void) {
+	while(1) {
+		__asm__ __volatile__("ud2\n");
+	}
+}
+#endif
+#define BUG() __bugfn()
+
+#else /* __bug */
 
 #if 1	/* Set to zero for a slightly smaller kernel */
 #define BUG()				\
@@ -104,6 +125,8 @@
 #else
 #define BUG() __asm__ __volatile__("ud2\n")
 #endif
+
+#endif /* __bug */
 
 #define PAGE_BUG(page) do { \
 	BUG(); \
diff -Nru -X dontdiff linux-2.4-vanilla/include/linux/compiler.h linux-2.4/include/linux/compiler.h
--- linux-2.4-vanilla/include/linux/compiler.h	Tue Sep 18 23:12:45 2001
+++ linux-2.4/include/linux/compiler.h	Tue Feb  3 07:25:57 2004
@@ -13,4 +13,11 @@
 #define likely(x)	__builtin_expect((x),1)
 #define unlikely(x)	__builtin_expect((x),0)
 
+#if __GNUC__ >= 3
+/* __noreturn__ is implemented since gcc 2.5. 
+ * __always_inline__ is not present in 2.9x
+ */
+#define __bug __attribute__((__noreturn__, __always_inline__))
+#endif
+
 #endif /* __LINUX_COMPILER_H */


Luca
PS: I wrote the patch early this morning, on train. It may be a huge pile of
crap ;)
-- 
Reply-To: kronos@kronoz.cjb.net
Home: http://kronoz.cjb.net
The trouble with computers is that they do what you tell them, 
not what you want.
D. Cohen

Re: [Compile Regression in 2.4.25-pre8][PATCH 37/42]

[Kronos]

Il Mon, Feb 02, 2004 at 10:08:48PM +0100, Geert Uytterhoeven ha scritto: 
> On Mon, 2 Feb 2004, Kronos wrote:
> > siimage.c:65: warning: control reaches end of non-void function
> >
> > The last statement before the end is BUG(), but I added a return to
> > silence the warning.
> >
> > diff -Nru -X dontdiff linux-2.4-vanilla/drivers/ide/pci/siimage.c linux-2.4/drivers/ide/pci/siimage.c
> > --- linux-2.4-vanilla/drivers/ide/pci/siimage.c	Tue Nov 11 17:51:38 2003
> > +++ linux-2.4/drivers/ide/pci/siimage.c	Sat Jan 31 19:07:56 2004
> > @@ -62,6 +62,9 @@
> >  			return 0;
> >  	}
> >  	BUG();
> > +
> > +	/* gcc will complain */
> > +	return 0;
> >  }
> 
> What about adding `attribute ((noreturn))' to the declaration of BUG() instead?

BUG() is a macro, it won't work.

Luca
-- 
Reply-To: kronos@kronoz.cjb.net
Home: http://kronoz.cjb.net
#include <stdio.h> 
int main(void) {printf("\t\t\b\b\b\b\b\b");
printf("\t\t\b\b\b\b\b\b");return 0;}

Re: [Compile Regression in 2.4.25-pre8][PATCH 37/42]

[Geert Uytterhoeven]

On Mon, 2 Feb 2004, Kronos wrote:
> siimage.c:65: warning: control reaches end of non-void function
>
> The last statement before the end is BUG(), but I added a return to
> silence the warning.
>
> diff -Nru -X dontdiff linux-2.4-vanilla/drivers/ide/pci/siimage.c linux-2.4/drivers/ide/pci/siimage.c
> --- linux-2.4-vanilla/drivers/ide/pci/siimage.c	Tue Nov 11 17:51:38 2003
> +++ linux-2.4/drivers/ide/pci/siimage.c	Sat Jan 31 19:07:56 2004
> @@ -62,6 +62,9 @@
>  			return 0;
>  	}
>  	BUG();
> +
> +	/* gcc will complain */
> +	return 0;
>  }

What about adding `attribute ((noreturn))' to the declaration of BUG() instead?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[Compile Regression in 2.4.25-pre8][PATCH 37/42]

[Kronos]

siimage.c:65: warning: control reaches end of non-void function

The last statement before the end is BUG(), but I added a return to
silence the warning.

diff -Nru -X dontdiff linux-2.4-vanilla/drivers/ide/pci/siimage.c linux-2.4/drivers/ide/pci/siimage.c
--- linux-2.4-vanilla/drivers/ide/pci/siimage.c	Tue Nov 11 17:51:38 2003
+++ linux-2.4/drivers/ide/pci/siimage.c	Sat Jan 31 19:07:56 2004
@@ -62,6 +62,9 @@
 			return 0;
 	}
 	BUG();
+
+	/* gcc will complain */
+	return 0;
 }
  
 /**

-- 
Reply-To: kronos@kronoz.cjb.net
Home: http://kronoz.cjb.net
You and me baby ain't nothin' but mammals
So let's do it like they do on the Discovery Channel