"eliminate warn_on_slowpath()" change causes many gcc-3.2.3 warnings

"eliminate warn_on_slowpath()" change causes many gcc-3.2.3 warnings

[Mikael Pettersson]

Commit ec5679e513305f1411753e5f5489935bd638af23 "eliminate warn_on_slowpath()"
changed __WARN() to call warn_slowpath() with a NULL value for the format
string parameter. Now every WARN_ON() triggers a warning from gcc-3.2.3:

In file included from include/linux/kmod.h:22,
                 from include/linux/module.h:13,
                 from include/linux/crypto.h:21,
                 from arch/x86/kernel/asm-offsets_32.c:7,
                 from arch/x86/kernel/asm-offsets.c:2:
include/linux/gfp.h: In function `allocflags_to_migratetype':
include/linux/gfp.h:104: warning: null format string

Compiling kernel 2.6.29-rc2 with a minimalistic .config for my 486
generates 1767 'null format string' warnings.

warn_slowpath() is declared with attribute((format(printf, 3, 4))),
and gcc-3.2.3 warns if the format string parameter is NULL. gcc-3.3
and newer do not warn in that case.

Since gcc-3.2 is still a supported compiler for the kernel, this is
a regression.

(Just FYI. I don't consider this serious enough to warrant reverting
that cleanup patch or declaring gcc-3.2.3 too old. I can always update
my 486 to gcc-3.3.6 if the warnings start to bug me too much.)

/Mikael

Re: "eliminate warn_on_slowpath()" change causes many gcc-3.2.3 warnings

[Ingo Molnar]

* Mikael Pettersson <mikpe@it.uu.se> wrote:

> Commit ec5679e513305f1411753e5f5489935bd638af23 "eliminate warn_on_slowpath()"
> changed __WARN() to call warn_slowpath() with a NULL value for the format
> string parameter. Now every WARN_ON() triggers a warning from gcc-3.2.3:
> 
> In file included from include/linux/kmod.h:22,
>                  from include/linux/module.h:13,
>                  from include/linux/crypto.h:21,
>                  from arch/x86/kernel/asm-offsets_32.c:7,
>                  from arch/x86/kernel/asm-offsets.c:2:
> include/linux/gfp.h: In function `allocflags_to_migratetype':
> include/linux/gfp.h:104: warning: null format string
> 
> Compiling kernel 2.6.29-rc2 with a minimalistic .config for my 486
> generates 1767 'null format string' warnings.
> 
> warn_slowpath() is declared with attribute((format(printf, 3, 4))),
> and gcc-3.2.3 warns if the format string parameter is NULL. gcc-3.3
> and newer do not warn in that case.
> 
> Since gcc-3.2 is still a supported compiler for the kernel, this is
> a regression.
> 
> (Just FYI. I don't consider this serious enough to warrant reverting 
> that cleanup patch or declaring gcc-3.2.3 too old. I can always update 
> my 486 to gcc-3.3.6 if the warnings start to bug me too much.)

hm, that's unfortunate. GCC seems totally on crack for not accepting a 
NULL format string.

	Ingo

Re: "eliminate warn_on_slowpath()" change causes many gcc-3.2.3 warnings

[H. Peter Anvin]

Ingo Molnar wrote:
> 
> hm, that's unfortunate. GCC seems totally on crack for not accepting a 
> NULL format string.
> 

Why should it?  I don't think a NULL pointer as the format to a
printf-style function is well defined.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: "eliminate warn_on_slowpath()" change causes many gcc-3.2.3 warnings

[Kyle McMartin]

On Sat, Jan 17, 2009 at 12:01:22PM -0800, H. Peter Anvin wrote:
> Ingo Molnar wrote:
> > 
> > hm, that's unfortunate. GCC seems totally on crack for not accepting a 
> > NULL format string.
> > 
> 
> Why should it?  I don't think a NULL pointer as the format to a
> printf-style function is well defined.
> 

How about something utterly evil? (Since you can't pass a zero-length
string to a printf attributed function either...)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 37b82cb..6c9f612 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -58,11 +58,12 @@ struct bug_entry {
  */
 #ifndef __WARN
 #ifndef __ASSEMBLY__
+extern const char * const warn_slowpath_nofmt;
 extern void warn_slowpath(const char *file, const int line,
 		const char *fmt, ...) __attribute__((format(printf, 3, 4)));
 #define WANT_WARN_ON_SLOWPATH
 #endif
-#define __WARN()		warn_slowpath(__FILE__, __LINE__, NULL)
+#define __WARN()		warn_slowpath(__FILE__, __LINE__, warn_slowpath_nofmt)
 #define __WARN_printf(arg...)	warn_slowpath(__FILE__, __LINE__, arg)
 #else
 #define __WARN_printf(arg...)	do { printk(arg); __WARN(); } while (0)
diff --git a/kernel/panic.c b/kernel/panic.c
index 2a2ff36..af749af 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -324,6 +324,8 @@ void oops_exit(void)
 }
 
 #ifdef WANT_WARN_ON_SLOWPATH
+
+const char * const warn_slowpath_nofmt = "";
 void warn_slowpath(const char *file, int line, const char *fmt, ...)
 {
 	va_list args;
@@ -340,7 +342,7 @@ void warn_slowpath(const char *file, int line, const char *fmt, ...)
 	if (board)
 		printk(KERN_WARNING "Hardware name: %s\n", board);
 
-	if (fmt) {
+	if (fmt != warn_slowpath_nofmt) {
 		va_start(args, fmt);
 		vprintk(fmt, args);
 		va_end(args);

Re: "eliminate warn_on_slowpath()" change causes many gcc-3.2.3 warnings

[Kyle McMartin]

On Sat, Jan 17, 2009 at 03:44:21PM -0500, Kyle McMartin wrote:
> How about something utterly evil? (Since you can't pass a zero-length
> string to a printf attributed function either...)
> 

Or more seriously... There really isn't a particularly nice way to solve
this, since you can't seem attach attributes to usage of the function, just
definitions. :/

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 37b82cb..02f4e9b 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -59,11 +59,32 @@ struct bug_entry {
 #ifndef __WARN
 #ifndef __ASSEMBLY__
 extern void warn_slowpath(const char *file, const int line,
-		const char *fmt, ...) __attribute__((format(printf, 3, 4)));
+		const char *fmt, va_list args); 
+
+static __attribute__((format(printf, 3, 4))) inline void warn_slowpath_chk(const char *file, const int line,
+		const char *fmt, ...)
+{
+	va_list args;
+
+	va_start(args, fmt);
+	warn_slowpath(file, line, fmt, args);
+	va_end(args);
+}
+
+static inline void warn_slowpath_nochk(const char *file, const int line,
+		const char *fmt, ...)
+{
+	va_list args;
+
+	va_start(args, fmt);
+	warn_slowpath(file, line, fmt, args);
+	va_end(args);
+}
+
 #define WANT_WARN_ON_SLOWPATH
 #endif
-#define __WARN()		warn_slowpath(__FILE__, __LINE__, NULL)
-#define __WARN_printf(arg...)	warn_slowpath(__FILE__, __LINE__, arg)
+#define __WARN()		warn_slowpath_nochk(__FILE__, __LINE__, NULL)
+#define __WARN_printf(arg...)	warn_slowpath_chk(__FILE__, __LINE__, arg)
 #else
 #define __WARN_printf(arg...)	do { printk(arg); __WARN(); } while (0)
 #endif
diff --git a/kernel/panic.c b/kernel/panic.c
index 2a2ff36..201a50b 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -324,9 +324,8 @@ void oops_exit(void)
 }
 
 #ifdef WANT_WARN_ON_SLOWPATH
-void warn_slowpath(const char *file, int line, const char *fmt, ...)
+void warn_slowpath(const char *file, int line, const char *fmt, va_list args)
 {
-	va_list args;
 	char function[KSYM_SYMBOL_LEN];
 	unsigned long caller = (unsigned long)__builtin_return_address(0);
 	const char *board;
@@ -340,11 +339,8 @@ void warn_slowpath(const char *file, int line, const char *fmt, ...)
 	if (board)
 		printk(KERN_WARNING "Hardware name: %s\n", board);
 
-	if (fmt) {
-		va_start(args, fmt);
+	if (fmt)
 		vprintk(fmt, args);
-		va_end(args);
-	}
 
 	print_modules();
 	dump_stack();

Re: "eliminate warn_on_slowpath()" change causes many gcc-3.2.3 warnings

[H. Peter Anvin]

Kyle McMartin wrote:
> 
> How about something utterly evil? (Since you can't pass a zero-length
> string to a printf attributed function either...)
> 

?!

*That* should definitely be permitted... anything else is an utter bug.

On the other hand, having a global empty string in the kernel isn't a
bad thing.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: "eliminate warn_on_slowpath()" change causes many gcc-3.2.3 warnings

[Kyle McMartin]

On Sat, Jan 17, 2009 at 01:07:58PM -0800, H. Peter Anvin wrote:
> Kyle McMartin wrote:
> > 
> > How about something utterly evil? (Since you can't pass a zero-length
> > string to a printf attributed function either...)
> > 
> 
> ?!
> 
> *That* should definitely be permitted... anything else is an utter bug.
> 

Sadly, 
kyle@ihatethathostname ~ $ cat foo.c
#include <stdio.h>
int main(void) {
	printf("");
	return 0;
}
kyle@ihatethathostname ~ $ gcc -O2 -Wall -o foo foo.c
foo.c: In function ‘main’:
foo.c:3: warning: zero-length printf format string

gcc version 4.3.2 20081105 (Red Hat 4.3.2-7) (GCC) 

Re: "eliminate warn_on_slowpath()" change causes many gcc-3.2.3 warnings

[Linus Torvalds]

On Sat, 17 Jan 2009, Kyle McMartin wrote:
> 
> How about something utterly evil? (Since you can't pass a zero-length
> string to a printf attributed function either...)

Don't do this. That just forces a load off a complex pointer instead, with 
no upsides. At least if it was

	extern const char warn_slowpath_nofmt[];

it would only load the pointer itself, which is still a fairly  expensive 
op, but at least doesn't require the extra memory load.

But you'd be better off jusst making it something like

	#define NO_FMT ((const char *)(-1))

instead, which is really much more obvious, and doesn't need any of that 
"get a pointer" overhead.

		Linus

Re: "eliminate warn_on_slowpath()" change causes many gcc-3.2.3 warnings

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> Don't do this. That just forces a load off a complex pointer instead, with 
> no upsides. At least if it was
> 
> 	extern const char warn_slowpath_nofmt[];
> 
> it would only load the pointer itself, which is still a fairly  expensive 
> op, but at least doesn't require the extra memory load.
> 
> But you'd be better off jusst making it something like
> 
> 	#define NO_FMT ((const char *)(-1))
> 
> instead, which is really much more obvious, and doesn't need any of that 
> "get a pointer" overhead.
> 

At least on x86, the two ops should be the same cost?

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: "eliminate warn_on_slowpath()" change causes many gcc-3.2.3 warnings

[Linus Torvalds]

On Sat, 17 Jan 2009, H. Peter Anvin wrote:
> 
> At least on x86, the two ops should be the same cost?

Not with the code Kyle had, which forces a memory load.

But yes, with a constant address, it at least comes close. But with a 
small explicit constant value, the compiler can often do even better. For 
example, you can generate a 64-bit -1 in many ways, while a 64-bit random 
address is much more work to generate.

Of course, I don't know how much gcc takes advantage of this. Maybe it 
always just generates a silly "movq" rather than being smarter about it 
(eg "orl $-1,reg" can do it in four bytes, I think, because you can use a 
single-byte constant).

Of course, zero is even easier to generate, so NULL is the best constant 
of all, but generally small integers are more amenable to optimization 
than generic addresses. They're also generally easier to test for.

		Linus

Re: "eliminate warn_on_slowpath()" change causes many gcc-3.2.3 warnings

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> On Sat, 17 Jan 2009, H. Peter Anvin wrote:
>> At least on x86, the two ops should be the same cost?
> 
> Not with the code Kyle had, which forces a memory load.
> 
> But yes, with a constant address, it at least comes close. But with a 
> small explicit constant value, the compiler can often do even better. For 
> example, you can generate a 64-bit -1 in many ways, while a 64-bit random 
> address is much more work to generate.
> 
> Of course, I don't know how much gcc takes advantage of this. Maybe it 
> always just generates a silly "movq" rather than being smarter about it 
> (eg "orl $-1,reg" can do it in four bytes, I think, because you can use a 
> single-byte constant).
> 
> Of course, zero is even easier to generate, so NULL is the best constant 
> of all, but generally small integers are more amenable to optimization 
> than generic addresses. They're also generally easier to test for.
> 

When compiling with -O2 -mcmodel=kernel on gcc 4.3.2, you end up with
the same 7-byte sequence:

   4:   48 c7 c7 00 00 00 00    mov    $0x0,%rdi
                        7: R_X86_64_32S bluttan
  10:   48 c7 c7 ff ff ff ff    mov    $0xffffffffffffffff,%rdi

With -Os -mcmodel=kernel, it's a bit better:


   4:   48 c7 c7 00 00 00 00    mov    $0x0,%rdi
                        7: R_X86_64_32S bluttan
  10:   48 83 cf ff             or     $0xffffffffffffffff,%rdi

I would have expected it to have used leaq in the first case, but it's
the same length (7 bytes) and probably has higher latencies.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.