Re: [PATCH] Fix print out of function which called WARN_ON()

[Linus Torvalds <torvalds@linux-foundation.org>]

[ Jakub - I added you to the participants list because it really looks 
  like this patch (without 'noinline') triggers a gcc bug.

  Or maybe gcc is truly smart enough to turn the "NULL arg pointer" case 
  into a "empty format" call, in which case we actually just solved the 
  original warning case in a _really_ subtle way. Somebody who understands 
  gcc varags should take a look.

  Regardless, even if the optimization happens to _work_, it seems bogus. 
  Even if "vprintk()" has been marked to have a printf-like format, that 
  doesn't mean that you can call it unconditionally by making the format 
  empty - it might have side effects.

  This is with 'gcc -v' reporting:

	gcc version 4.4.0 20090506 (Red Hat 4.4.0-4) (GCC) 

  and is the current fedora-11 compiler (as of a yum update yesterday or 
  the day before) on x86-64 ]

On Fri, 15 May 2009, Ian Campbell wrote:
>
> All WARN_ON()'s currently appear to come from warn_slowpath_null e.g.:
>  WARNING: at kernel/softirq.c:143 warn_slowpath_null+0x1c/0x20()
> 
> This is because since:
> 
>   commit 57adc4d2dbf968fdbe516359688094eef4d46581
>   Author: Andi Kleen <andi@firstfloor.org>
>   Date:   Wed May 6 16:02:53 2009 -0700
> 
>       Eliminate thousands of warnings with gcc 3.2 build
> 
> the caller of warn_slowpath_fmt really is warn_flowpath_null not the
> interesting caller next up the chain. Since __builtin_return_address(X) for X >
> 0 is not reliable, pass the real caller as an argument to warn_slowpath_fmt.

I'd actually much rather re-organize it so that there are the two exported 
functions (with the current interface) that use a common shared static 
function. Rather than passing in '0' in a macro.

IOW, something like this..

Btw, when doing this, it really looked to me like the "noinline" matters. 
I did it first to try to make the assembly clearer to read, but without it 
gcc-4.4 seems to actually generate the wrong code, and does an 
_unconditional_ call to vprintk().

Very odd. I've seen other bugs triggered by gcc-4.4 (alpine miscompiles), 
it all makes me very nervous about the compiler in current fedora-11.

Anybody want to double-check this?

		Linus

---
 kernel/panic.c |   35 ++++++++++++++++++++---------------
 1 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index 874ecf1..3c8048d 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -340,39 +340,44 @@ void oops_exit(void)
 }
 
 #ifdef WANT_WARN_ON_SLOWPATH
-void warn_slowpath_fmt(const char *file, int line, const char *fmt, ...)
-{
+struct slowpath_args {
+	const char *fmt;
 	va_list args;
-	char function[KSYM_SYMBOL_LEN];
-	unsigned long caller = (unsigned long)__builtin_return_address(0);
-	const char *board;
+};
 
-	sprint_symbol(function, caller);
+static void noinline warn_slowpath_common(const char *file, int line, void *caller, struct slowpath_args *args)
+{
+	const char *board;
 
 	printk(KERN_WARNING "------------[ cut here ]------------\n");
-	printk(KERN_WARNING "WARNING: at %s:%d %s()\n", file,
-		line, function);
+	printk(KERN_WARNING "WARNING: at %s:%d %pS()\n", file, line, caller);
 	board = dmi_get_system_info(DMI_PRODUCT_NAME);
 	if (board)
 		printk(KERN_WARNING "Hardware name: %s\n", board);
 
-	if (*fmt) {
-		va_start(args, fmt);
-		vprintk(fmt, args);
-		va_end(args);
-	}
+	if (args)
+		vprintk(args->fmt, args->args);
 
 	print_modules();
 	dump_stack();
 	print_oops_end_marker();
 	add_taint(TAINT_WARN);
 }
+
+void warn_slowpath_fmt(const char *file, int line, const char *fmt, ...)
+{
+	struct slowpath_args args;
+
+	args.fmt = fmt;
+	va_start(args.args, fmt);
+	warn_slowpath_common(file, line, __builtin_return_address(0), &args);
+	va_end(args.args);
+}
 EXPORT_SYMBOL(warn_slowpath_fmt);
 
 void warn_slowpath_null(const char *file, int line)
 {
-	static const char *empty = "";
-	warn_slowpath_fmt(file, line, empty);
+	warn_slowpath_fmt(file, line, __builtin_return_address(0), NULL);
 }
 EXPORT_SYMBOL(warn_slowpath_null);
 #endif