[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <alpine.LFD.2.01.0905151018370.3343@localhost.localdomain>
Date: Fri, 15 May 2009 10:52:44 -0700 (PDT)
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Ian Campbell <ian.campbell@...rix.com>,
Jakub Jelinek <jakub@...hat.com>
cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Jesper Nilsson <jesper.nilsson@...s.com>,
Johannes Weiner <hannes@...xchg.org>,
Arjan van de Ven <arjan@...ux.intel.com>,
Andi Kleen <ak@...ux.intel.com>,
Hugh Dickins <hugh@...itas.com>,
Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] Fix print out of function which called WARN_ON()
[ 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@...stfloor.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists