Re: the printk problem

Re: the printk problem

[Linus Torvalds]

On Fri, 4 Jul 2008, Linus Torvalds wrote:
> 
> so I think we could easily just say that we extend %p in various ways:
> 
>  - %pS - print pointer as a symbol
> 
> and leave tons of room for future extensions for different kinds of 
> pointers. 

So here's a totally untested example patch of this, which could probably 
easily be extended to to other things.

I actually made it '%pF' and '%pS' for a Function descriptor pointer and 
normal Symbolic pointer respectively, because of the stupid things ia64 
and PPC64 do with the pointer indirection through function descriptors. 

That function descriptor indirection is totally untested, and I did it 
with a

	pagefault_disable();
	__get_user(..)
	pagefault_enable();

thing because I thought it would be nice if printk() was always safe, and 
passing bogus function pointers to '%pF' should try to work, but quite 
frankly, I didn't even check that that part compiles, much less works.

ia64/ppc lists cc'd, just in case somebody wants to test this.

NOTE! There are no current actual users of this, but the _idea_ is that we 
should be able to just do

	printk("%pF\n", desc->handle_irq);

instead of using

	print_symbol("%s\n", (unsigned long)desc->handle_irq);

The latter is from kernel/irq/internals.h, and actually looks incorrect - 
shouldn't it use print_fn_descriptor_symbol(), since it's a C level 
function pointer? We should use "print_symbol()" for return pointers we 
find on the stack and data pointers, but not for stuff that actually has a 
C type that is a function pointer?

Somebody who cares about the insane function descriptors should take a 
deeper look.

NOTE AGAIN! UNTESTED! I could easily have screwed up printk() _entirely_, 
since I had to factor out the string handling into a function of its own.

		Linus
---
 lib/vsprintf.c |  102 ++++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 74 insertions(+), 28 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 6021757..148b656 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -22,6 +22,7 @@
 #include <linux/string.h>
 #include <linux/ctype.h>
 #include <linux/kernel.h>
+#include <linux/kallsyms.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/div64.h>
@@ -482,6 +483,72 @@ static char *number(char *buf, char *end, unsigned long long num, int base, int
 	return buf;
 }
 
+static char *string(char *buf, char *end, char *s, int field_width, int precision, int flags)
+{
+	int len, i;
+
+	if ((unsigned long)s < PAGE_SIZE)
+		s = "<NULL>";
+
+	len = strnlen(s, precision);
+
+	if (!(flags & LEFT)) {
+		while (len < field_width--) {
+			if (buf < end)
+				*buf = ' ';
+			++buf;
+		}
+	}
+	for (i = 0; i < len; ++i) {
+		if (buf < end)
+			*buf = *s;
+		++buf; ++s;
+	}
+	while (len < field_width--) {
+		if (buf < end)
+			*buf = ' ';
+		++buf;
+	}
+	return buf;
+}
+
+/*
+ * Show a '%p' thing.  A kernel extension is that the '%p' is followed
+ * by an extra set of alphanumeric characters that are extended format
+ * specifiers.  Right now we just handle 'F' (for symbolic Function
+ * pointers) and 'S' (for Symbolic data pointers), but this can easily
+ * be extended in the future (network address types etc).
+ *
+ * The difference between 'S' and 'F' is that on ia64 and ppc64 function
+ * pointers are really function descriptors, which contain a pointer the
+ * real address. 
+ */
+static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int base, int size, int precision, int type)
+{
+	char sym[KSYM_SYMBOL_LEN];
+	switch (*fmt) {
+	case 'F':	/* Function pointer */
+#if defined(CONFIG_IA64) || defined(CONFIG_PPC64)
+		{ void *p;
+		pagefault_disable();
+		if (!__get_user((void **)ptr, &p))
+			ptr = p;
+		pagefault_enable();
+		}
+#endif
+	/* Fallthrough */
+	case 'S':	/* Other pointer */
+#if CONFIG_KALLSYMS
+		sprint_symbol(sym, (unsigned long) ptr);
+		return string(buf, end, sym, size, precision, type);
+#else
+		type |= SPECIAL;
+		break;
+#endif
+	}
+	return number(buf, end, (unsigned long long) ptr, base, size, precision, type);
+}
+
 /**
  * vsnprintf - Format a string and place it in a buffer
  * @buf: The buffer to place the result into
@@ -502,11 +569,9 @@ static char *number(char *buf, char *end, unsigned long long num, int base, int
  */
 int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 {
-	int len;
 	unsigned long long num;
-	int i, base;
+	int base;
 	char *str, *end, c;
-	const char *s;
 
 	int flags;		/* flags to number() */
 
@@ -622,29 +687,7 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 				continue;
 
 			case 's':
-				s = va_arg(args, char *);
-				if ((unsigned long)s < PAGE_SIZE)
-					s = "<NULL>";
-
-				len = strnlen(s, precision);
-
-				if (!(flags & LEFT)) {
-					while (len < field_width--) {
-						if (str < end)
-							*str = ' ';
-						++str;
-					}
-				}
-				for (i = 0; i < len; ++i) {
-					if (str < end)
-						*str = *s;
-					++str; ++s;
-				}
-				while (len < field_width--) {
-					if (str < end)
-						*str = ' ';
-					++str;
-				}
+				str = string(str, end, va_arg(args, char *), field_width, precision, flags);
 				continue;
 
 			case 'p':
@@ -653,9 +696,12 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 					field_width = 2*sizeof(void *);
 					flags |= ZEROPAD;
 				}
-				str = number(str, end,
-						(unsigned long) va_arg(args, void *),
+				str = pointer(fmt+1, str, end,
+						va_arg(args, void *),
 						16, field_width, precision, flags);
+				/* Skip all alphanumeric pointer suffixes */
+				while (isalnum(fmt[1]))
+					fmt++;
 				continue;
 
 

Re: the printk problem

[Andrew Morton]

On Fri, 4 Jul 2008 13:02:05 -0700 (PDT) Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 
> 
> On Fri, 4 Jul 2008, Linus Torvalds wrote:
> > 
> > so I think we could easily just say that we extend %p in various ways:
> > 
> >  - %pS - print pointer as a symbol
> > 
> > and leave tons of room for future extensions for different kinds of 
> > pointers. 
> 
> So here's a totally untested example patch of this, which could probably 
> easily be extended to to other things.
> 
> I actually made it '%pF' and '%pS' for a Function descriptor pointer and 
> normal Symbolic pointer respectively, because of the stupid things ia64 
> and PPC64 do with the pointer indirection through function descriptors. 
> 
> That function descriptor indirection is totally untested, and I did it 
> with a
> 
> 	pagefault_disable();
> 	__get_user(..)
> 	pagefault_enable();
> 
> thing because I thought it would be nice if printk() was always safe, and 
> passing bogus function pointers to '%pF' should try to work, but quite 
> frankly, I didn't even check that that part compiles, much less works.

probe_kernel_address() should be usable here.

> ia64/ppc lists cc'd, just in case somebody wants to test this.
> 
> NOTE! There are no current actual users of this, but the _idea_ is that we 
> should be able to just do
> 
> 	printk("%pF\n", desc->handle_irq);
> 
> instead of using
> 
> 	print_symbol("%s\n", (unsigned long)desc->handle_irq);
> 
> The latter is from kernel/irq/internals.h, and actually looks incorrect - 
> shouldn't it use print_fn_descriptor_symbol(), since it's a C level 
> function pointer? We should use "print_symbol()" for return pointers we 
> find on the stack and data pointers, but not for stuff that actually has a 
> C type that is a function pointer?
> 
> Somebody who cares about the insane function descriptors should take a 
> deeper look.
> 
> NOTE AGAIN! UNTESTED! I could easily have screwed up printk() _entirely_, 
> since I had to factor out the string handling into a function of its own.
> 
> 		Linus
> ---
>  lib/vsprintf.c |  102 ++++++++++++++++++++++++++++++++++++++++---------------
>  1 files changed, 74 insertions(+), 28 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 6021757..148b656 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -22,6 +22,7 @@
>  #include <linux/string.h>
>  #include <linux/ctype.h>
>  #include <linux/kernel.h>
> +#include <linux/kallsyms.h>
>  
>  #include <asm/page.h>		/* for PAGE_SIZE */
>  #include <asm/div64.h>
> @@ -482,6 +483,72 @@ static char *number(char *buf, char *end, unsigned long long num, int base, int
>  	return buf;
>  }
>  
> +static char *string(char *buf, char *end, char *s, int field_width, int precision, int flags)
> +{
> +	int len, i;
> +
> +	if ((unsigned long)s < PAGE_SIZE)
> +		s = "<NULL>";

hm, is that needed for other reasons than "it will fault"?

otherwise we could walk the whole string with probe_kernel_address()
before we do anything with it.

That's slightly racy against vunmap and CONFIG_DEBUG_PAGEALLOC and
stuff.  Fixable by *never* dereferencing that pointer except via
probe_kernel_address().

> +	len = strnlen(s, precision);
> +
> +	if (!(flags & LEFT)) {
> +		while (len < field_width--) {
> +			if (buf < end)
> +				*buf = ' ';
> +			++buf;
> +		}
> +	}
> +	for (i = 0; i < len; ++i) {
> +		if (buf < end)
> +			*buf = *s;
> +		++buf; ++s;
> +	}
> +	while (len < field_width--) {
> +		if (buf < end)
> +			*buf = ' ';
> +		++buf;
> +	}
> +	return buf;
> +}
> +
> +/*
> + * Show a '%p' thing.  A kernel extension is that the '%p' is followed
> + * by an extra set of alphanumeric characters that are extended format
> + * specifiers.  Right now we just handle 'F' (for symbolic Function
> + * pointers) and 'S' (for Symbolic data pointers), but this can easily
> + * be extended in the future (network address types etc).
> + *
> + * The difference between 'S' and 'F' is that on ia64 and ppc64 function
> + * pointers are really function descriptors, which contain a pointer the
> + * real address. 
> + */
> +static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int base, int size, int precision, int type)
> +{
> +	char sym[KSYM_SYMBOL_LEN];
> +	switch (*fmt) {
> +	case 'F':	/* Function pointer */
> +#if defined(CONFIG_IA64) || defined(CONFIG_PPC64)
> +		{ void *p;
> +		pagefault_disable();
> +		if (!__get_user((void **)ptr, &p))
> +			ptr = p;
> +		pagefault_enable();
> +		}
> +#endif
> +	/* Fallthrough */
> +	case 'S':	/* Other pointer */
> +#if CONFIG_KALLSYMS
> +		sprint_symbol(sym, (unsigned long) ptr);
> +		return string(buf, end, sym, size, precision, type);
> +#else
> +		type |= SPECIAL;
> +		break;
> +#endif
> +	}
> +	return number(buf, end, (unsigned long long) ptr, base, size, precision, type);
> +}

If this takes off we might want a register-your-printk-handler
interface.  Maybe.

We also jump through hoops to print things like sector_t and
resource_size_t.  They always need to be cast to `unsiged long long',
which generates additional stack space and text in some setups.

And then there's the perennial "need to cast u64 to unsigned long long
to print it".  If we were to do

	printk("%SL", (void *)some_u64);

then that's still bloody ugly, but it'll save a little text-n-stack.

Re: the printk problem

[Linus Torvalds]

On Fri, 4 Jul 2008, Andrew Morton wrote:
> 
> probe_kernel_address() should be usable here.

Right you are.

> > +static char *string(char *buf, char *end, char *s, int field_width, int precision, int flags)
> > +{
> > +	int len, i;
> > +
> > +	if ((unsigned long)s < PAGE_SIZE)
> > +		s = "<NULL>";
> 
> hm, is that needed for other reasons than "it will fault"?

It's similar to what glibc does - showing a NULL ptr gracefully. They are 
pretty common, and it's very rude to SIGSEGV or oops just because you 
wanted to print a string out for debugging.

The code is also just copied from the old code - just moved it to a 
function of its own.

> otherwise we could walk the whole string with probe_kernel_address()
> before we do anything with it.

That would be pretty slow, wed' be better off then unrolling it (ie doing 
all the ugly setup around the whole string).

> If this takes off we might want a register-your-printk-handler
> interface.  Maybe.

Yeah. 

> We also jump through hoops to print things like sector_t and
> resource_size_t.  They always need to be cast to `unsiged long long',
> which generates additional stack space and text in some setups.

Indeed. Though doing it with a pointer is often not a _whole_ lot cleaner, 
but yes, it's often nicer to add a '&' than adding a cast.

> And then there's the perennial "need to cast u64 to unsigned long long
> to print it".  If we were to do
> 
> 	printk("%SL", (void *)some_u64);
> 
> then that's still bloody ugly, but it'll save a little text-n-stack.

No can do. (void *) isn't big enough to hold a u64. So it would have to be 
something like this:

	printk("%p64i", &some_u64);

instead. Avoiding the cast, and often being more efficient calling 
convention on 32-bit.

But it can often generate worse code too - now we're taking an address of 
a variable, and that will disable many optimizations because now i's not a 
purely local variable that the compiler knows all uses for any more.

So I'm not entirely conviced it's a good idea to do this for just "long 
long" cases. Dunno.

		Linus

Re: the printk problem

[Matthew Wilcox]

On Fri, Jul 04, 2008 at 01:27:16PM -0700, Andrew Morton wrote:
> On Fri, 4 Jul 2008 13:02:05 -0700 (PDT) Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > > so I think we could easily just say that we extend %p in various ways:
> > > 
> > >  - %pS - print pointer as a symbol
> > > 
> > > and leave tons of room for future extensions for different kinds of 
> > > pointers. 
> 
> If this takes off we might want a register-your-printk-handler
> interface.  Maybe.
> 
> We also jump through hoops to print things like sector_t and
> resource_size_t.  They always need to be cast to `unsiged long long',
> which generates additional stack space and text in some setups.

The thing is that GCC checks types.  So it's fine to add "print this
pointer specially", but you can't in general add new printf arguments
without also hacking GCC.  Unless you use -Wno-format, and require
sparse to check special kernel types.

> And then there's the perennial "need to cast u64 to unsigned long long
> to print it".  If we were to do
> 
> 	printk("%SL", (void *)some_u64);
> 
> then that's still bloody ugly, but it'll save a little text-n-stack.

u64 is easy to fix, and I don't know why we haven't.  Just make it
unsigned long long on all architectures.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: the printk problem

[Andrew Morton]

(heck, let's cc lkml - avoid having to go through all this again)

On Fri, 4 Jul 2008 14:42:53 -0600 Matthew Wilcox <matthew@wil.cx> wrote:

> On Fri, Jul 04, 2008 at 01:27:16PM -0700, Andrew Morton wrote:
> > On Fri, 4 Jul 2008 13:02:05 -0700 (PDT) Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > > > so I think we could easily just say that we extend %p in various ways:
> > > > 
> > > >  - %pS - print pointer as a symbol
> > > > 
> > > > and leave tons of room for future extensions for different kinds of 
> > > > pointers. 
> > 
> > If this takes off we might want a register-your-printk-handler
> > interface.  Maybe.
> > 
> > We also jump through hoops to print things like sector_t and
> > resource_size_t.  They always need to be cast to `unsiged long long',
> > which generates additional stack space and text in some setups.
> 
> The thing is that GCC checks types.  So it's fine to add "print this
> pointer specially", but you can't in general add new printf arguments
> without also hacking GCC.  Unless you use -Wno-format, and require
> sparse to check special kernel types.

It would be excellent if gcc had an extension system so that you could
add new printf control chars and maybe even tell gcc how to check them.
But of course, if that were to happen, we couldn't use it for 4-5 years.

What I had initially proposed was to abuse %S, which takes a wchar_t*. 
gcc accepts `unsigned long *' for %S.

Then, we put the kernel-specific control char after the S, so we can
print an inode (rofl) with

	struct inode *inode;

	printk("here is an inode: %Si\n", (unsigned long *)inode);

Downsides are:

- there's a cast, so you could accidentally do

	printk("here is an inode: %Si\n", (unsigned long *)dentry);

- there's a cast, and they're ugly

- gcc cannot of course check that the arg matches the control string

Unfortunately (and this seems weird), gcc printf checking will not
accept a void* for %S: it _has_ to be wchar_t*, and the checker won't
permit void* substitution for that.

Anyway, Linus took all that and said "let's abuse %p instead".  It
_will_ accept any pointer so we can instead do:

	printk("here is an inode: %pi\n", inode);

which is nicer.


I think the main customers of this are print_symbol():

	printk("I am about to call %ps\n", fn);
	(*fn)();

and NIPQUAD and its ipv6 version.

We don't know how much interest there would be in churning NIPQUAD from
the net guys.  Interestingly, there's also %C (wint_t) which is a
32-bit quantity.  So we could just go and say "%C prints an ipv4
address" and be done with it.  But there's no way of doing that for
ipv6 addresses so things would become asymmetrical there.

Another customer is net mac addresses.  There are surely others.  One
which should have been in printf 30 years ago was %b: binary.


> > And then there's the perennial "need to cast u64 to unsigned long long
> > to print it".  If we were to do
> > 
> > 	printk("%SL", (void *)some_u64);
> > 
> > then that's still bloody ugly, but it'll save a little text-n-stack.
> 
> u64 is easy to fix, and I don't know why we haven't.  Just make it
> unsigned long long on all architectures.

Yeah.  Why don't we do that?

Re: the printk problem

[Matthew Wilcox]

On Fri, Jul 04, 2008 at 03:01:00PM -0700, Andrew Morton wrote:
> (heck, let's cc lkml - avoid having to go through all this again)
> 
> It would be excellent if gcc had an extension system so that you could
> add new printf control chars and maybe even tell gcc how to check them.
> But of course, if that were to happen, we couldn't use it for 4-5 years.

I believe NetBSD added that as an extension many years ago.  Dunno if
they still have it.

> What I had initially proposed was to abuse %S, which takes a wchar_t*. 
> gcc accepts `unsigned long *' for %S.
> 
[...]
> - there's a cast, so you could accidentally do
> 
> 	printk("here is an inode: %Si\n", (unsigned long *)dentry);
> 
> - there's a cast, and they're ugly
> 
> - gcc cannot of course check that the arg matches the control string
> 
> Unfortunately (and this seems weird), gcc printf checking will not
> accept a void* for %S: it _has_ to be wchar_t*, and the checker won't
> permit void* substitution for that.

Presumably that's the compiler getting rid of the first and third
downside ;-)

> Anyway, Linus took all that and said "let's abuse %p instead".  It
> _will_ accept any pointer so we can instead do:
> 
> 	printk("here is an inode: %pi\n", inode);
> 
> which is nicer.

Yes.  It's possible to confuse it, of course.

	printk("Function %pSucks\n", sys_open);

but I really doubt we have such a usage in the kernel today.

> > u64 is easy to fix, and I don't know why we haven't.  Just make it
> > unsigned long long on all architectures.
> 
> Yeah.  Why don't we do that?

Patch ...

[PATCH] Make u64 long long on all architectures

It is currently awkward to print a u64 type.  Some architectures use
unsigned long while others use unsigned long long.  Since unsigned long
long is 64-bit for all existing Linux architectures, change those that
use long to use long long.  Note that this applies only within the
kernel.  If u64 is being used in a C++ method definition, the symbol
mangling would change.

Signed-off-by: Matthew Wilcox <willy@linux.intel.com>

diff --git a/include/asm-generic/int-l64.h b/include/asm-generic/int-l64.h
index 2af9b75..32f07bd 100644
--- a/include/asm-generic/int-l64.h
+++ b/include/asm-generic/int-l64.h
@@ -23,8 +23,13 @@ typedef unsigned short __u16;
 typedef __signed__ int __s32;
 typedef unsigned int __u32;
 
+#ifdef __KERNEL__
+typedef __signed__ long long __s64;
+typedef unsigned long long __u64;
+#else
 typedef __signed__ long __s64;
 typedef unsigned long __u64;
+#endif
 
 #endif /* __ASSEMBLY__ */
 

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] Make u64 long long on all architectures (was: the printk problem)

[Andrew Morton]

On Fri, 4 Jul 2008 20:03:51 -0600 Matthew Wilcox <matthew@wil.cx> wrote:

> [PATCH] Make u64 long long on all architectures
> 
> It is currently awkward to print a u64 type.  Some architectures use
> unsigned long while others use unsigned long long.  Since unsigned long
> long is 64-bit for all existing Linux architectures, change those that
> use long to use long long.  Note that this applies only within the
> kernel.  If u64 is being used in a C++ method definition, the symbol
> mangling would change.
> 
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> 
> diff --git a/include/asm-generic/int-l64.h b/include/asm-generic/int-l64.h
> index 2af9b75..32f07bd 100644
> --- a/include/asm-generic/int-l64.h
> +++ b/include/asm-generic/int-l64.h
> @@ -23,8 +23,13 @@ typedef unsigned short __u16;
>  typedef __signed__ int __s32;
>  typedef unsigned int __u32;
>  
> +#ifdef __KERNEL__
> +typedef __signed__ long long __s64;
> +typedef unsigned long long __u64;
> +#else
>  typedef __signed__ long __s64;
>  typedef unsigned long __u64;
> +#endif
>  
>  #endif /* __ASSEMBLY__ */

This is (IMO) a desirable change and will prevent a heck of a lot of
goofing around, and will permit a lot of prior goofing around to
be removed.

But I bet there are lots of instalces of printk("%l", some_u64) down in
arch code where the type of u64 _is_ known which will now spew warnings.

Oh well.

Re: [PATCH] Make u64 long long on all architectures (was: the printk problem)

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2086 bytes --]

On Tue, 2008-07-22 at 03:05 -0700, Andrew Morton wrote:
> On Fri, 4 Jul 2008 20:03:51 -0600 Matthew Wilcox <matthew@wil.cx> wrote:
> 
> > [PATCH] Make u64 long long on all architectures
> > 
> > It is currently awkward to print a u64 type.  Some architectures use
> > unsigned long while others use unsigned long long.  Since unsigned long
> > long is 64-bit for all existing Linux architectures, change those that
> > use long to use long long.  Note that this applies only within the
> > kernel.  If u64 is being used in a C++ method definition, the symbol
> > mangling would change.
> > 
> > Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> > 
> > diff --git a/include/asm-generic/int-l64.h b/include/asm-generic/int-l64.h
> > index 2af9b75..32f07bd 100644
> > --- a/include/asm-generic/int-l64.h
> > +++ b/include/asm-generic/int-l64.h
> > @@ -23,8 +23,13 @@ typedef unsigned short __u16;
> >  typedef __signed__ int __s32;
> >  typedef unsigned int __u32;
> >  
> > +#ifdef __KERNEL__
> > +typedef __signed__ long long __s64;
> > +typedef unsigned long long __u64;
> > +#else
> >  typedef __signed__ long __s64;
> >  typedef unsigned long __u64;
> > +#endif
> >  
> >  #endif /* __ASSEMBLY__ */
> 
> This is (IMO) a desirable change and will prevent a heck of a lot of
> goofing around, and will permit a lot of prior goofing around to
> be removed.
> 
> But I bet there are lots of instalces of printk("%l", some_u64) down in
> arch code where the type of u64 _is_ known which will now spew warnings.
> 
> Oh well.

As a rough estimate:

concordia powerpc(master) $ find arch/powerpc/ ! -name '*32.*' | xargs grep "%l" | grep -v "%ll" | wc -l
635


Someone's gonna get a lot of git points for fixing all those. Might keep
the speeling fix crowd busy for a while.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Make u64 long long on all architectures (was: the printk problem)

[Andrew Morton]

On Tue, 22 Jul 2008 20:36:35 +1000 Michael Ellerman <michael@ellerman.id.au> wrote:

> On Tue, 2008-07-22 at 03:05 -0700, Andrew Morton wrote:
> > On Fri, 4 Jul 2008 20:03:51 -0600 Matthew Wilcox <matthew@wil.cx> wrote:
> > 
> > > [PATCH] Make u64 long long on all architectures
> > > 
> > > It is currently awkward to print a u64 type.  Some architectures use
> > > unsigned long while others use unsigned long long.  Since unsigned long
> > > long is 64-bit for all existing Linux architectures, change those that
> > > use long to use long long.  Note that this applies only within the
> > > kernel.  If u64 is being used in a C++ method definition, the symbol
> > > mangling would change.
> > > 
> > > Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> > > 
> > > diff --git a/include/asm-generic/int-l64.h b/include/asm-generic/int-l64.h
> > > index 2af9b75..32f07bd 100644
> > > --- a/include/asm-generic/int-l64.h
> > > +++ b/include/asm-generic/int-l64.h
> > > @@ -23,8 +23,13 @@ typedef unsigned short __u16;
> > >  typedef __signed__ int __s32;
> > >  typedef unsigned int __u32;
> > >  
> > > +#ifdef __KERNEL__
> > > +typedef __signed__ long long __s64;
> > > +typedef unsigned long long __u64;
> > > +#else
> > >  typedef __signed__ long __s64;
> > >  typedef unsigned long __u64;
> > > +#endif
> > >  
> > >  #endif /* __ASSEMBLY__ */
> > 
> > This is (IMO) a desirable change and will prevent a heck of a lot of
> > goofing around, and will permit a lot of prior goofing around to
> > be removed.
> > 
> > But I bet there are lots of instalces of printk("%l", some_u64) down in
> > arch code where the type of u64 _is_ known which will now spew warnings.
> > 
> > Oh well.
> 
> As a rough estimate:
> 
> concordia powerpc(master) $ find arch/powerpc/ ! -name '*32.*' | xargs grep "%l" | grep -v "%ll" | wc -l
> 635

lolz.  If yesterdays-linux-next on todays-mainline wasn't such a
hilarious trainwreck I'd test your grepping.  I guess that could be
done on mainline too.

> Someone's gonna get a lot of git points for fixing all those. Might keep
> the speeling fix crowd busy for a while.

I'm not sure I have the energy for this.

But we really should do it.

Re: [PATCH] Make u64 long long on all architectures (was: the printk problem)

[Benjamin Herrenschmidt]

On Tue, 2008-07-22 at 20:36 +1000, Michael Ellerman wrote:
> concordia powerpc(master) $ find arch/powerpc/ ! -name '*32.*' | xargs
> grep "%l" | grep -v "%ll" | wc -l
> 635
> 
> 
> Someone's gonna get a lot of git points for fixing all those. Might
> keep
> the speeling fix crowd busy for a 

But a bunch of those might actually be real longs, not u64's ...

Easier to do the change and build something like ppc6xx_defconfig to get
a better approximation.

Ben.

Re: [PATCH] Make u64 long long on all architectures (was: the printk problem)

[Benjamin Herrenschmidt]

> 
> This is (IMO) a desirable change and will prevent a heck of a lot of
> goofing around, and will permit a lot of prior goofing around to
> be removed.
> 
> But I bet there are lots of instalces of printk("%l", some_u64) down in
> arch code where the type of u64 _is_ known which will now spew warnings.

Well, I'm about to call a big "warning fixing day" on the powerpc list,
I saw a few today when building a couple of configs and that hurts my
eyes so we may as well fold that in :-)

Cheers,
Ben.
 

Re: the printk problem

[Denys Vlasenko]

On Saturday 05 July 2008 00:01, Andrew Morton wrote:
> > > We also jump through hoops to print things like sector_t and
> > > resource_size_t.  They always need to be cast to `unsiged long long',
> > > which generates additional stack space and text in some setups.
> > 
> > The thing is that GCC checks types.  So it's fine to add "print this
> > pointer specially", but you can't in general add new printf arguments
> > without also hacking GCC.  Unless you use -Wno-format, and require
> > sparse to check special kernel types.
> 
> It would be excellent if gcc had an extension system so that you could
> add new printf control chars and maybe even tell gcc how to check them.
> But of course, if that were to happen, we couldn't use it for 4-5 years.
> 
> What I had initially proposed was to abuse %S, which takes a wchar_t*. 
> gcc accepts `unsigned long *' for %S.
> 
> Then, we put the kernel-specific control char after the S, so we can
> print an inode (rofl) with
> 
> 	struct inode *inode;
> 
> 	printk("here is an inode: %Si\n", (unsigned long *)inode);
> 
> Downsides are:
> 
> - there's a cast, so you could accidentally do
> 
> 	printk("here is an inode: %Si\n", (unsigned long *)dentry);
> 
> - there's a cast, and they're ugly
> 
> - gcc cannot of course check that the arg matches the control string
> 
> Unfortunately (and this seems weird), gcc printf checking will not
> accept a void* for %S: it _has_ to be wchar_t*, and the checker won't
> permit void* substitution for that.

Repeating myself here...
We can add an alternative alias to printk:

 asmlinkage int printk(const char * fmt, ...)
         __attribute__ ((format (printf, 1, 2))) __cold;
+asmlinkage int custom_printk(const char * fmt, ...) __cold asm ("printk");

custom_printk() is actually just printk(), that is,
we won't need additional function, we need to teach
*printk* about MAC addresses, NIPQUADs etc;

and then use printk() if you use only standard %fmt (and have it
checked by gcc), or use custom_printk() if you have non-standard
%fmt in the format string.

The only downside that in second case, you lose gcc checking.
No big deal.
--
vda

Re: the printk problem

[Jan Engelhardt]

On Saturday 2008-07-05 00:01, Andrew Morton wrote:
>
>We don't know how much interest there would be in churning NIPQUAD from
>the net guys.  Interestingly, there's also %C (wint_t) which is a
>32-bit quantity.  So we could just go and say "%C prints an ipv4
>address" and be done with it.  But there's no way of doing that for
>ipv6 addresses so things would become asymmetrical there.

	struct in6_addr src;
	printk("Source address: %p{ipv6}\n", &src);

How about %p{feature}?

Re: the printk problem

[Vegard Nossum]

On Sat, Jul 5, 2008 at 1:33 PM, Jan Engelhardt <jengelh@medozas.de> wrote:
>
> On Saturday 2008-07-05 00:01, Andrew Morton wrote:
>>
>>We don't know how much interest there would be in churning NIPQUAD from
>>the net guys.  Interestingly, there's also %C (wint_t) which is a
>>32-bit quantity.  So we could just go and say "%C prints an ipv4
>>address" and be done with it.  But there's no way of doing that for
>>ipv6 addresses so things would become asymmetrical there.
>
>        struct in6_addr src;
>        printk("Source address: %p{ipv6}\n", &src);
>
> How about %p{feature}?

Something like this?

(It's hard on the stack, yes, I know. We should fix kallsyms.)

Vegard


From: Vegard Nossum <vegard.nossum@gmail.com>
Date: Sat, 5 Jul 2008 14:01:00 +0200
Subject: [PATCH] printf: add %p{} extension

Signed-off-by: Vegard Nossum <vegard.nossum@gmail.com>
---
 lib/vsprintf.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 6021757..011cf3f 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -17,6 +17,7 @@
  */
 
 #include <stdarg.h>
+#include <linux/kallsyms.h>
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/string.h>
@@ -366,6 +367,30 @@ static noinline char* put_dec(char *buf, unsigned long long num)
 #define SMALL	32		/* Must be 32 == 0x20 */
 #define SPECIAL	64		/* 0x */
 
+static char *custom_format(char *buf, char *end,
+	const char *fmt, unsigned int fmtlen, void *arg)
+{
+	if (!strncmp(fmt, "sym", fmtlen)) {
+		char name[KSYM_SYMBOL_LEN];
+		int len;
+		int i;
+
+		len = sprint_symbol(name, (unsigned long) arg);
+		if (len < 0)
+			return buf;
+
+		i = 0;
+		while (i < len) {
+			if (buf < end)
+				*buf = name[i];
+			++buf;
+			++i;
+		}
+	}
+
+	return buf;
+}
+
 static char *number(char *buf, char *end, unsigned long long num, int base, int size, int precision, int type)
 {
 	/* we are called with base 8, 10 or 16, only, thus don't need "G..."  */
@@ -648,6 +673,25 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 				continue;
 
 			case 'p':
+				if (fmt[1] == '{') {
+					const char *cfmt;
+
+					/* Skip the '%{' */
+					++fmt;
+					++fmt;
+
+					cfmt = fmt;
+
+					/* Skip everything up to the '}' */
+					while (*fmt && *fmt != '}')
+						++fmt;
+
+					str = custom_format(str, end,
+						cfmt, fmt - cfmt,
+						va_arg(args, void *));
+					continue;
+				}
+
 				flags |= SMALL;
 				if (field_width == -1) {
 					field_width = 2*sizeof(void *);
-- 
1.5.4.1

Re: the printk problem

[Jan Engelhardt]

On Saturday 2008-07-05 14:52, Vegard Nossum wrote:
>> On Saturday 2008-07-05 00:01, Andrew Morton wrote:
>>>
>>>We don't know how much interest there would be in churning NIPQUAD from
>>>the net guys.  Interestingly, there's also %C (wint_t) which is a
>>>32-bit quantity.  So we could just go and say "%C prints an ipv4
>>>address" and be done with it.  But there's no way of doing that for
>>>ipv6 addresses so things would become asymmetrical there.
>>
>>        struct in6_addr src;
>>        printk("Source address: %p{ipv6}\n", &src);
>>
>> How about %p{feature}?
>
>Something like this?
>
>+static char *custom_format(char *buf, char *end,
>+	const char *fmt, unsigned int fmtlen, void *arg)

I'd use const void *arg here, so nobody gets the idea
that you could modify the argument while printing :)

>+{
>+	if (!strncmp(fmt, "sym", fmtlen)) {
>+		char name[KSYM_SYMBOL_LEN];
>+		int len;
>+		int i;
>+
>+		len = sprint_symbol(name, (unsigned long) arg);
>+		if (len < 0)
>+			return buf;
>+
>+		i = 0;
>+		while (i < len) {
>+			if (buf < end)
>+				*buf = name[i];
>+			++buf;
>+			++i;
>+		}
>+	}

And I assume it's then as simple as

	} else if (strncmp(fmt, "nip6", fmtlen) == 0) {
		snprintf(buf, end - buf (+1?), NIP6_FMT in expanded form,
		         NIP6 in expanded form(arg));
	}

Hm, that's going to get a big function when everyone adds their
fmttypes.

>+
>+	return buf;
>+}
>+
> static char *number(char *buf, char *end, unsigned long long num, int base, int size, int precision, int type)
> {
> 	/* we are called with base 8, 10 or 16, only, thus don't need "G..."  */
>@@ -648,6 +673,25 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
> 				continue;
> 
> 			case 'p':
>+				if (fmt[1] == '{') {
>+					const char *cfmt;
>+
>+					/* Skip the '%{' */
>+					++fmt;
>+					++fmt;
>+

Not this?

	/* Skip the '%p{' */
	fmt += 3;

Re: the printk problem

[Vegard Nossum]

On Sat, Jul 5, 2008 at 3:24 PM, Jan Engelhardt <jengelh@medozas.de> wrote:
>
> On Saturday 2008-07-05 14:52, Vegard Nossum wrote:
>>> On Saturday 2008-07-05 00:01, Andrew Morton wrote:
>>>>
>>>>We don't know how much interest there would be in churning NIPQUAD from
>>>>the net guys.  Interestingly, there's also %C (wint_t) which is a
>>>>32-bit quantity.  So we could just go and say "%C prints an ipv4
>>>>address" and be done with it.  But there's no way of doing that for
>>>>ipv6 addresses so things would become asymmetrical there.
>>>
>>>        struct in6_addr src;
>>>        printk("Source address: %p{ipv6}\n", &src);
>>>
>>> How about %p{feature}?
>>
>>Something like this?
>>
>>+static char *custom_format(char *buf, char *end,
>>+      const char *fmt, unsigned int fmtlen, void *arg)
>
> I'd use const void *arg here, so nobody gets the idea
> that you could modify the argument while printing :)
>

Oops, of course. Thanks.

>>+{
>>+      if (!strncmp(fmt, "sym", fmtlen)) {
...
>>+      }
>
> And I assume it's then as simple as
>
>        } else if (strncmp(fmt, "nip6", fmtlen) == 0) {
>                snprintf(buf, end - buf (+1?), NIP6_FMT in expanded form,
>                         NIP6 in expanded form(arg));
>        }
>
> Hm, that's going to get a big function when everyone adds their
> fmttypes.
>

Yes. Luckily, the entry point is then fixed in a single place and it's
easy to convert it into something more dynamic :-)

A static array wouldn't help much because it still concentrates all
the names. But we could at least do a binary search on that and get
some speed improvements if it grows large.

I think the most elegant solution would be a macro similar to the
initcall macros, that adds the custom extensions to a table which is
defined by a special linker section. This allows complete
decentralization, but I don't think it's possible to do binary search
on the names anymore.

Dynamic registering/unregistering could be done too. Maybe this is a
good thing for modules...

Thoughts?

>>+
>>+      return buf;
>>+}
>>+
>> static char *number(char *buf, char *end, unsigned long long num, int base, int size, int precision, int type)
>> {
>>       /* we are called with base 8, 10 or 16, only, thus don't need "G..."  */
>>@@ -648,6 +673,25 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
>>                               continue;
>>
>>                       case 'p':
>>+                              if (fmt[1] == '{') {
>>+                                      const char *cfmt;
>>+
>>+                                      /* Skip the '%{' */
>>+                                      ++fmt;
>>+                                      ++fmt;
>>+
>
> Not this?
>
>        /* Skip the '%p{' */
>        fmt += 3;
>

Oops, the comment is wrong. It should say: "Skip the p{". But fmt += 3
is wrong. Since fmt[0] is at this point 'p', and fmt[1] is '{'. The %
and flags, etc. have already been skipped by the common code. So it
should be fmt += 2 :-)

Thanks!


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: the printk problem

[Jan Engelhardt]

On Saturday 2008-07-05 15:50, Vegard Nossum wrote:
>
>I think the most elegant solution would be a macro similar to the
>initcall macros, that adds the custom extensions to a table which is
>defined by a special linker section. This allows complete
>decentralization, but I don't think it's possible to do binary search
>on the names anymore.

With an rbtree, you can still do binary search.

Re: the printk problem

[Linus Torvalds]

On Sat, 5 Jul 2008, Vegard Nossum wrote:

> On Sat, Jul 5, 2008 at 1:33 PM, Jan Engelhardt <jengelh@medozas.de> wrote:
> >
> > On Saturday 2008-07-05 00:01, Andrew Morton wrote:
> >>
> >>We don't know how much interest there would be in churning NIPQUAD from
> >>the net guys.  Interestingly, there's also %C (wint_t) which is a
> >>32-bit quantity.  So we could just go and say "%C prints an ipv4
> >>address" and be done with it.  But there's no way of doing that for
> >>ipv6 addresses so things would become asymmetrical there.
> >
> >        struct in6_addr src;
> >        printk("Source address: %p{ipv6}\n", &src);
> >
> > How about %p{feature}?

No.

I _expressly_ chose '%p[alphanumeric]*' because it's basically 
totally insane to have that in a *real* printk() string: the end result 
would be totally unreadable.

In contrast, '%p[specialchar]' is not unreadable, and in fact we have lots 
of those already in the kernel. In fact, there are 40 occurrences of '%p{' 
in the kernel, just grep for it (especially the AFS code seems to be very 
happy to use that kind of printout in its debug statements).

So it makes perfect sense to have a _real_ printk string that says

	"BUG: Dentry %p{i=%lx,n=%s}"

where we have that '%p{...' sequence: the end result is easily parseable. 
In contrast, anybody who uses '%pS' or something like that and expects a 
pointer name to be immediately followed by teh letter 'S' is simply 
insane, because the end result is an unreadable mess.

> (It's hard on the stack, yes, I know. We should fix kallsyms.)

Not just that, but it's broken when KALLSYMS is disabled. Look at what 
sprint_symbol() becomes.

The patch I already sent out is about a million times better, because it 
avoids all these issues, and knows about subtle issues like the difference 
between a direct pointer and a pointer to a function descriptor. 

		Linus

Re: the printk problem

[Jan Engelhardt]

On Saturday 2008-07-05 19:56, Linus Torvalds wrote:
>> >
>> > How about %p{feature}?
>
>No.
>
>I _expressly_ chose '%p[alphanumeric]*' because it's basically 
>totally insane to have that in a *real* printk() string: the end result 
>would be totally unreadable.

So, and what do you do when you run out of alphanumeric characters?

Re: the printk problem

[Linus Torvalds]

On Sat, 5 Jul 2008, Jan Engelhardt wrote:
> 
> So, and what do you do when you run out of alphanumeric characters?

Did you actually look at my patch?

It's not a single alnum character. It's an arbitrary sequence of alnum 
characters. IOW, my patch allows

	%p6N

or something like that for showing a ipv6 "NIP" format string etc. Or you 
could spell them out even more, although I consider it unlikely that you 
really want to see too many of these, since gcc won't actually be able to 
type-check them (so they will always remain _secondary_ formats).

			Linus

Re: the printk problem

[Vegard Nossum]

On Sat, Jul 5, 2008 at 7:56 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sat, 5 Jul 2008, Vegard Nossum wrote:
>> On Sat, Jul 5, 2008 at 1:33 PM, Jan Engelhardt <jengelh@medozas.de> wrote:
>> > How about %p{feature}?
>
> No.
>
> I _expressly_ chose '%p[alphanumeric]*' because it's basically
> totally insane to have that in a *real* printk() string: the end result
> would be totally unreadable.
>
> In contrast, '%p[specialchar]' is not unreadable, and in fact we have lots
> of those already in the kernel. In fact, there are 40 occurrences of '%p{'
> in the kernel, just grep for it (especially the AFS code seems to be very
> happy to use that kind of printout in its debug statements).
>
> So it makes perfect sense to have a _real_ printk string that says
>
>        "BUG: Dentry %p{i=%lx,n=%s}"
>
> where we have that '%p{...' sequence: the end result is easily parseable.
> In contrast, anybody who uses '%pS' or something like that and expects a
> pointer name to be immediately followed by teh letter 'S' is simply
> insane, because the end result is an unreadable mess.

That's really a truly hideous hack :-)

Single letters are bad because it hurts readability and limits the
usefulness of the extension.</MHO>

If it's just the {} that is the problem, use something else. I'm sure
we can find something that isn't used already.

>
>> (It's hard on the stack, yes, I know. We should fix kallsyms.)
>
> Not just that, but it's broken when KALLSYMS is disabled. Look at what
> sprint_symbol() becomes.

Oops. Not hard to mend, though.

> The patch I already sent out is about a million times better, because it
> avoids all these issues, and knows about subtle issues like the difference
> between a direct pointer and a pointer to a function descriptor.

Right, right. I found it now:

http://ozlabs.org/pipermail/linuxppc-dev/2008-July/059257.html

Argh... Some pointer to original thread would be nice when adding new Ccs.


Vegard

PS: Your version has exactly the same stack problem. Will send a patch
for kallsyms later.

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: the printk problem

[Matthew Wilcox]

On Sat, Jul 05, 2008 at 08:41:39PM +0200, Vegard Nossum wrote:
> Single letters are bad because it hurts readability and limits the
> usefulness of the extension.</MHO>

I think you need a little warning noise that goes off in your head that
means "I might be overdesigning this".  Linus' code is elegant and
solves a problem nicely.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: the printk problem

[Pekka Enberg]

On Sat, Jul 05, 2008 at 08:41:39PM +0200, Vegard Nossum wrote:
>> Single letters are bad because it hurts readability and limits the
>> usefulness of the extension.</MHO>

On Sat, Jul 5, 2008 at 9:52 PM, Matthew Wilcox <matthew@wil.cx> wrote:
> I think you need a little warning noise that goes off in your head that
> means "I might be overdesigning this".  Linus' code is elegant and
> solves a problem nicely.

Am I the only one who missed Linus' patch? Did it make it to the list?

Re: the printk problem

[Randy Dunlap]

On Sun, 6 Jul 2008 03:02:59 +0300 Pekka Enberg wrote:

> On Sat, Jul 05, 2008 at 08:41:39PM +0200, Vegard Nossum wrote:
> >> Single letters are bad because it hurts readability and limits the
> >> usefulness of the extension.</MHO>
> 
> On Sat, Jul 5, 2008 at 9:52 PM, Matthew Wilcox <matthew@wil.cx> wrote:
> > I think you need a little warning noise that goes off in your head that
> > means "I might be overdesigning this".  Linus' code is elegant and
> > solves a problem nicely.
> 
> Am I the only one who missed Linus' patch? Did it make it to the list?

No, you are not the only one.  It was on linuxppc-dev for some reason.
http://ozlabs.org/pipermail/linuxppc-dev/2008-July/059257.html

---
~Randy
Linux Plumbers Conference, 17-19 September 2008, Portland, Oregon USA
http://linuxplumbersconf.org/

Re: the printk problem

[Benjamin Herrenschmidt]

> u64 is easy to fix, and I don't know why we haven't.  Just make it
> unsigned long long on all architectures.

Yup. Also, one of the major user of that is to print a struct resource,
which everybody does differently, so we may even look at doing a %pR
that does the nice start..end [attr]..

Ben.

Re: the printk problem

[Matthew Wilcox]

On Fri, Jul 04, 2008 at 01:02:05PM -0700, Linus Torvalds wrote:
> On Fri, 4 Jul 2008, Linus Torvalds wrote:
> > 
> > so I think we could easily just say that we extend %p in various ways:
> > 
> >  - %pS - print pointer as a symbol
> > 
> > and leave tons of room for future extensions for different kinds of 
> > pointers. 
> 
> So here's a totally untested example patch of this, which could probably 
> easily be extended to to other things.
> 
> I actually made it '%pF' and '%pS' for a Function descriptor pointer and 
> normal Symbolic pointer respectively, because of the stupid things ia64 
> and PPC64 do with the pointer indirection through function descriptors. 

It's also true for parisc, fwiw.  Added a cc to them.

> That function descriptor indirection is totally untested, and I did it 
> with a
> 
> 	pagefault_disable();
> 	__get_user(..)
> 	pagefault_enable();
> 
> thing because I thought it would be nice if printk() was always safe, and 
> passing bogus function pointers to '%pF' should try to work, but quite 
> frankly, I didn't even check that that part compiles, much less works.
> 
> ia64/ppc lists cc'd, just in case somebody wants to test this.
> 
> NOTE! There are no current actual users of this, but the _idea_ is that we 
> should be able to just do
> 
> 	printk("%pF\n", desc->handle_irq);
> 
> instead of using
> 
> 	print_symbol("%s\n", (unsigned long)desc->handle_irq);
> 
> The latter is from kernel/irq/internals.h, and actually looks incorrect - 
> shouldn't it use print_fn_descriptor_symbol(), since it's a C level 
> function pointer? We should use "print_symbol()" for return pointers we 
> find on the stack and data pointers, but not for stuff that actually has a 
> C type that is a function pointer?
> 
> Somebody who cares about the insane function descriptors should take a 
> deeper look.
> 
> NOTE AGAIN! UNTESTED! I could easily have screwed up printk() _entirely_, 
> since I had to factor out the string handling into a function of its own.
> 
> 		Linus
> ---
>  lib/vsprintf.c |  102 ++++++++++++++++++++++++++++++++++++++++---------------
>  1 files changed, 74 insertions(+), 28 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 6021757..148b656 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -22,6 +22,7 @@
>  #include <linux/string.h>
>  #include <linux/ctype.h>
>  #include <linux/kernel.h>
> +#include <linux/kallsyms.h>
>  
>  #include <asm/page.h>		/* for PAGE_SIZE */
>  #include <asm/div64.h>
> @@ -482,6 +483,72 @@ static char *number(char *buf, char *end, unsigned long long num, int base, int
>  	return buf;
>  }
>  
> +static char *string(char *buf, char *end, char *s, int field_width, int precision, int flags)
> +{
> +	int len, i;
> +
> +	if ((unsigned long)s < PAGE_SIZE)
> +		s = "<NULL>";
> +
> +	len = strnlen(s, precision);
> +
> +	if (!(flags & LEFT)) {
> +		while (len < field_width--) {
> +			if (buf < end)
> +				*buf = ' ';
> +			++buf;
> +		}
> +	}
> +	for (i = 0; i < len; ++i) {
> +		if (buf < end)
> +			*buf = *s;
> +		++buf; ++s;
> +	}
> +	while (len < field_width--) {
> +		if (buf < end)
> +			*buf = ' ';
> +		++buf;
> +	}
> +	return buf;
> +}
> +
> +/*
> + * Show a '%p' thing.  A kernel extension is that the '%p' is followed
> + * by an extra set of alphanumeric characters that are extended format
> + * specifiers.  Right now we just handle 'F' (for symbolic Function
> + * pointers) and 'S' (for Symbolic data pointers), but this can easily
> + * be extended in the future (network address types etc).
> + *
> + * The difference between 'S' and 'F' is that on ia64 and ppc64 function
> + * pointers are really function descriptors, which contain a pointer the
> + * real address. 
> + */
> +static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int base, int size, int precision, int type)
> +{
> +	char sym[KSYM_SYMBOL_LEN];
> +	switch (*fmt) {
> +	case 'F':	/* Function pointer */
> +#if defined(CONFIG_IA64) || defined(CONFIG_PPC64)
> +		{ void *p;
> +		pagefault_disable();
> +		if (!__get_user((void **)ptr, &p))
> +			ptr = p;
> +		pagefault_enable();
> +		}
> +#endif
> +	/* Fallthrough */
> +	case 'S':	/* Other pointer */
> +#if CONFIG_KALLSYMS
> +		sprint_symbol(sym, (unsigned long) ptr);
> +		return string(buf, end, sym, size, precision, type);
> +#else
> +		type |= SPECIAL;
> +		break;
> +#endif
> +	}
> +	return number(buf, end, (unsigned long long) ptr, base, size, precision, type);
> +}
> +
>  /**
>   * vsnprintf - Format a string and place it in a buffer
>   * @buf: The buffer to place the result into
> @@ -502,11 +569,9 @@ static char *number(char *buf, char *end, unsigned long long num, int base, int
>   */
>  int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
>  {
> -	int len;
>  	unsigned long long num;
> -	int i, base;
> +	int base;
>  	char *str, *end, c;
> -	const char *s;
>  
>  	int flags;		/* flags to number() */
>  
> @@ -622,29 +687,7 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
>  				continue;
>  
>  			case 's':
> -				s = va_arg(args, char *);
> -				if ((unsigned long)s < PAGE_SIZE)
> -					s = "<NULL>";
> -
> -				len = strnlen(s, precision);
> -
> -				if (!(flags & LEFT)) {
> -					while (len < field_width--) {
> -						if (str < end)
> -							*str = ' ';
> -						++str;
> -					}
> -				}
> -				for (i = 0; i < len; ++i) {
> -					if (str < end)
> -						*str = *s;
> -					++str; ++s;
> -				}
> -				while (len < field_width--) {
> -					if (str < end)
> -						*str = ' ';
> -					++str;
> -				}
> +				str = string(str, end, va_arg(args, char *), field_width, precision, flags);
>  				continue;
>  
>  			case 'p':
> @@ -653,9 +696,12 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
>  					field_width = 2*sizeof(void *);
>  					flags |= ZEROPAD;
>  				}
> -				str = number(str, end,
> -						(unsigned long) va_arg(args, void *),
> +				str = pointer(fmt+1, str, end,
> +						va_arg(args, void *),
>  						16, field_width, precision, flags);
> +				/* Skip all alphanumeric pointer suffixes */
> +				while (isalnum(fmt[1]))
> +					fmt++;
>  				continue;
>  
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: the printk problem

[Kyle McMartin]

On Fri, Jul 04, 2008 at 02:36:21PM -0600, Matthew Wilcox wrote:
> It's also true for parisc, fwiw.  Added a cc to them.
> 

I posted a patch months ago for kallsyms on parisc, but it looks like
nobody ever responded or cared. Nice.

Re: the printk problem

[Benjamin Herrenschmidt]

On Fri, 2008-07-04 at 13:02 -0700, Linus Torvalds wrote:
> 
> That function descriptor indirection is totally untested, and I did it 
> with a
> 
>         pagefault_disable();
>         __get_user(..)
>         pagefault_enable();
> 
> thing because I thought it would be nice if printk() was always safe, and 
> passing bogus function pointers to '%pF' should try to work, but quite 
> frankly, I didn't even check that that part compiles, much less works.
> 
> ia64/ppc lists cc'd, just in case somebody wants to test this.

I'll give it a try using probe_kernel_address() instead on monday.

Cheers,
Ben.

Re: the printk problem

[Linus Torvalds]

On Sat, 5 Jul 2008, Benjamin Herrenschmidt wrote:
> 
> I'll give it a try using probe_kernel_address() instead on monday.

Here's the updated patch which uses probe_kernel_address() instead (and 
moves the whole #ifdef mess out of the code that wants it and into a 
helper function - and maybe we should then put that helper into 
kallsyms.h, but that's a different issue).

Still all happily untested, of course. And still with no actual users 
converted.

			Linus

---
 lib/vsprintf.c |  108 +++++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 80 insertions(+), 28 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 6021757..1fbb1c0 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -22,6 +22,8 @@
 #include <linux/string.h>
 #include <linux/ctype.h>
 #include <linux/kernel.h>
+#include <linux/kallsyms.h>
+#include <linux/uaccess.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/div64.h>
@@ -482,6 +484,77 @@ static char *number(char *buf, char *end, unsigned long long num, int base, int
 	return buf;
 }
 
+static char *string(char *buf, char *end, char *s, int field_width, int precision, int flags)
+{
+	int len, i;
+
+	if ((unsigned long)s < PAGE_SIZE)
+		s = "<NULL>";
+
+	len = strnlen(s, precision);
+
+	if (!(flags & LEFT)) {
+		while (len < field_width--) {
+			if (buf < end)
+				*buf = ' ';
+			++buf;
+		}
+	}
+	for (i = 0; i < len; ++i) {
+		if (buf < end)
+			*buf = *s;
+		++buf; ++s;
+	}
+	while (len < field_width--) {
+		if (buf < end)
+			*buf = ' ';
+		++buf;
+	}
+	return buf;
+}
+
+static inline void *dereference_function_descriptor(void *ptr)
+{
+#if defined(CONFIG_IA64) || defined(CONFIG_PPC64)
+	void *p;
+	if (!probe_kernel_address(ptr, p))
+		ptr = p;
+#endif
+	return ptr;
+}
+
+
+/*
+ * Show a '%p' thing.  A kernel extension is that the '%p' is followed
+ * by an extra set of alphanumeric characters that are extended format
+ * specifiers.  Right now we just handle 'F' (for symbolic Function
+ * pointers) and 'S' (for Symbolic data pointers), but this can easily
+ * be extended in the future (network address types etc).
+ *
+ * The difference between 'S' and 'F' is that on ia64 and ppc64 function
+ * pointers are really function descriptors, which contain a pointer the
+ * real address. 
+ */
+static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int base, int size, int precision, int type)
+{
+	switch (*fmt) {
+	case 'F':
+		ptr = dereference_function_descriptor(ptr);
+		/* Fallthrough */
+	case 'S': {	/* Other (direct) pointer */
+#if CONFIG_KALLSYMS
+		char sym[KSYM_SYMBOL_LEN];
+		sprint_symbol(sym, (unsigned long) ptr);
+		return string(buf, end, sym, size, precision, type);
+#else
+		type |= SPECIAL;
+		break;
+#endif
+	}
+	}
+	return number(buf, end, (unsigned long long) ptr, base, size, precision, type);
+}
+
 /**
  * vsnprintf - Format a string and place it in a buffer
  * @buf: The buffer to place the result into
@@ -502,11 +575,9 @@ static char *number(char *buf, char *end, unsigned long long num, int base, int
  */
 int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 {
-	int len;
 	unsigned long long num;
-	int i, base;
+	int base;
 	char *str, *end, c;
-	const char *s;
 
 	int flags;		/* flags to number() */
 
@@ -622,29 +693,7 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 				continue;
 
 			case 's':
-				s = va_arg(args, char *);
-				if ((unsigned long)s < PAGE_SIZE)
-					s = "<NULL>";
-
-				len = strnlen(s, precision);
-
-				if (!(flags & LEFT)) {
-					while (len < field_width--) {
-						if (str < end)
-							*str = ' ';
-						++str;
-					}
-				}
-				for (i = 0; i < len; ++i) {
-					if (str < end)
-						*str = *s;
-					++str; ++s;
-				}
-				while (len < field_width--) {
-					if (str < end)
-						*str = ' ';
-					++str;
-				}
+				str = string(str, end, va_arg(args, char *), field_width, precision, flags);
 				continue;
 
 			case 'p':
@@ -653,9 +702,12 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 					field_width = 2*sizeof(void *);
 					flags |= ZEROPAD;
 				}
-				str = number(str, end,
-						(unsigned long) va_arg(args, void *),
+				str = pointer(fmt+1, str, end,
+						va_arg(args, void *),
 						16, field_width, precision, flags);
+				/* Skip all alphanumeric pointer suffixes */
+				while (isalnum(fmt[1]))
+					fmt++;
 				continue;
 
 

Re: the printk problem

[Linus Torvalds]

On Fri, 4 Jul 2008, Linus Torvalds wrote:
> 
> Still all happily untested, of course. And still with no actual users 
> converted.

Ok, it's tested, and here's an example usage conversion.

The diffstat pretty much says it all. It _does_ change the format of the 
stack trace entry a bit, but I don't think it's for the worse (unless it 
breaks things like the oops tracker - Arjan?)

It changes the symbol-in-module format from

	:ext3:add_dirent_to_buf+0x6c/0x26c

to

	add_dirent_to_buf+0x6c/0x26c [ext3]

but quite frankly, the latter was the standard format anyway (it's what 
"sprint_symbol()" gives you), and traps_64.c was the odd man out.

In fact, traps_32.c already uses the standard print_symbol() format, so it 
really was an issue of the 64-bit code being odd (and I assume that this 
also means that it cannot break the oops tracker, since it already had to 
be able to handle both formats).

I also removed the KALLSYMS dependency, so if KALLSYMS isn't enabled it 
will now give the same hex format twice, but I doubt we really care (such 
stack traces are unreadable whether it shows up once or twice, and the 
simplicity is worth it).

If people do just a few more conversions like this, then the 52 added 
lines in lib/vsnprintf.c are more than made up for by removed lines 
elsewhere (and more readable source code).

		Linus

---
 arch/x86/kernel/traps_64.c |   25 +------------------------
 1 files changed, 1 insertions(+), 24 deletions(-)

diff --git a/arch/x86/kernel/traps_64.c b/arch/x86/kernel/traps_64.c
index adff76e..f1a95d1 100644
--- a/arch/x86/kernel/traps_64.c
+++ b/arch/x86/kernel/traps_64.c
@@ -104,30 +104,7 @@ int kstack_depth_to_print = 12;
 
 void printk_address(unsigned long address, int reliable)
 {
-#ifdef CONFIG_KALLSYMS
-	unsigned long offset = 0, symsize;
-	const char *symname;
-	char *modname;
-	char *delim = ":";
-	char namebuf[KSYM_NAME_LEN];
-	char reliab[4] = "";
-
-	symname = kallsyms_lookup(address, &symsize, &offset,
-					&modname, namebuf);
-	if (!symname) {
-		printk(" [<%016lx>]\n", address);
-		return;
-	}
-	if (!reliable)
-		strcpy(reliab, "? ");
-
-	if (!modname)
-		modname = delim = "";
-	printk(" [<%016lx>] %s%s%s%s%s+0x%lx/0x%lx\n",
-		address, reliab, delim, modname, delim, symname, offset, symsize);
-#else
-	printk(" [<%016lx>]\n", address);
-#endif
+	printk(" [<%016lx>] %s%pS\n", address, reliable ? "": "? ", (void *) address);
 }
 
 static unsigned long *in_exception_stack(unsigned cpu, unsigned long stack,

Re: the printk problem

[Arjan van de Ven]

Linus Torvalds wrote:
> 
> On Fri, 4 Jul 2008, Linus Torvalds wrote:
>> Still all happily untested, of course. And still with no actual users 
>> converted.
> 
> Ok, it's tested, and here's an example usage conversion.
> 
> The diffstat pretty much says it all. It _does_ change the format of the 
> stack trace entry a bit, but I don't think it's for the worse (unless it 
> breaks things like the oops tracker - Arjan?)
> 
> It changes the symbol-in-module format from
> 
> 	:ext3:add_dirent_to_buf+0x6c/0x26c
> 
> to
> 
> 	add_dirent_to_buf+0x6c/0x26c [ext3]
> 
> but quite frankly, the latter was the standard format anyway (it's what 
> "sprint_symbol()" gives you), and traps_64.c was the odd man out.
> 

This won't break anything for me actually; I already deal with either case.

$ cat oopsparse.pl | wc -l
1252

the kernel is so inconsistent with oops formats (over time/across architectures)
that once you deal with what there is today... you pretty much deal with everything.

I also like the improvement; I wished something like this existed several times already
in the last few months so for sure

Acked-by: Arjan van de Ven <arjan@linux.intel.com>

Re: the printk problem

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> > Still all happily untested, of course. And still with no actual 
> > users converted.
> 
> Ok, it's tested, and here's an example usage conversion.
> 
> The diffstat pretty much says it all. It _does_ change the format of 
> the stack trace entry a bit, but I don't think it's for the worse 
> (unless it breaks things like the oops tracker - Arjan?)
> 
> It changes the symbol-in-module format from
> 
> 	:ext3:add_dirent_to_buf+0x6c/0x26c
> 
> to
> 
> 	add_dirent_to_buf+0x6c/0x26c [ext3]
> 
> but quite frankly, the latter was the standard format anyway (it's 
> what "sprint_symbol()" gives you), and traps_64.c was the odd man out.
> 
> In fact, traps_32.c already uses the standard print_symbol() format, 
> so it really was an issue of the 64-bit code being odd (and I assume 
> that this also means that it cannot break the oops tracker, since it 
> already had to be able to handle both formats).
> 
> I also removed the KALLSYMS dependency, so if KALLSYMS isn't enabled 
> it will now give the same hex format twice, but I doubt we really care 
> (such stack traces are unreadable whether it shows up once or twice, 
> and the simplicity is worth it).
> 
> If people do just a few more conversions like this, then the 52 added 
> lines in lib/vsnprintf.c are more than made up for by removed lines 
> elsewhere (and more readable source code).

applied (with the commit message below) to tip/x86/debug for v2.6.27 
merging, thanks Linus. Can i add your SOB too?

	Ingo

-------------------->
commit 4afd2534d6d4a77f4b7497c92f1ff7528d8f4eaa
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Sat Jul 5 15:32:41 2008 -0700

    x86, 64-bit: standardize printk_address()
    
    Changes the symbol-in-module format from
    
    	:ext3:add_dirent_to_buf+0x6c/0x26c
    
    to
    
    	add_dirent_to_buf+0x6c/0x26c [ext3]
    
    the latter was the standard format anyway (it's what "sprint_symbol()"
    gives you), and traps_64.c was the odd man out.
    
    In fact, traps_32.c already uses the standard print_symbol() format, so
    it really was an issue of the 64-bit code being odd (and I assume that
    this also means that it cannot break the oops tracker, since it already
    had to be able to handle both formats).
    
    I also removed the KALLSYMS dependency, so if KALLSYMS isn't enabled it
    will now give the same hex format twice, but I doubt we really care
    (such stack traces are unreadable whether it shows up once or twice, and
    the simplicity is worth it).
    
    If people do just a few more conversions like this, then the 52 added
    lines in lib/vsnprintf.c are more than made up for by removed lines
    elsewhere (and more readable source code).
    
    Signed-off-by: Ingo Molnar <mingo@elte.hu>

diff --git a/arch/x86/kernel/traps_64.c b/arch/x86/kernel/traps_64.c
index adff76e..f1a95d1 100644
--- a/arch/x86/kernel/traps_64.c
+++ b/arch/x86/kernel/traps_64.c
@@ -104,30 +104,7 @@ int kstack_depth_to_print = 12;
 
 void printk_address(unsigned long address, int reliable)
 {
-#ifdef CONFIG_KALLSYMS
-	unsigned long offset = 0, symsize;
-	const char *symname;
-	char *modname;
-	char *delim = ":";
-	char namebuf[KSYM_NAME_LEN];
-	char reliab[4] = "";
-
-	symname = kallsyms_lookup(address, &symsize, &offset,
-					&modname, namebuf);
-	if (!symname) {
-		printk(" [<%016lx>]\n", address);
-		return;
-	}
-	if (!reliable)
-		strcpy(reliab, "? ");
-
-	if (!modname)
-		modname = delim = "";
-	printk(" [<%016lx>] %s%s%s%s%s+0x%lx/0x%lx\n",
-		address, reliab, delim, modname, delim, symname, offset, symsize);
-#else
-	printk(" [<%016lx>]\n", address);
-#endif
+	printk(" [<%016lx>] %s%pS\n", address, reliable ? "": "? ", (void *) address);
 }
 
 static unsigned long *in_exception_stack(unsigned cpu, unsigned long stack,

Re: the printk problem

[Linus Torvalds]

On Sun, 6 Jul 2008, Ingo Molnar wrote:
> 
> applied (with the commit message below) to tip/x86/debug for v2.6.27 
> merging, thanks Linus. Can i add your SOB too?

Sure, add my S-O-B. But I hope/assuem that you also added my earlier patch 
that added the support for '%pS' too? I'm not entirely sure that should go 
in an x86-specific branch, since it has nothing x86-specific in it.

Also, I've cleaned up the lib/vsnprintf.c patch to fix up some minor 
details. For example, it should not default to a field width of 
"2*sizeof(unsigned long)" - it should do that only if it ends up printing 
the pointer as a hex number.

So this is my current version.. (the "precision" thing was moved into the 
'pointer()' function, and to after the special case handling).

		Linus
---
 lib/vsprintf.c |  118 ++++++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 85 insertions(+), 33 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 6021757..f60c7c0 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -22,6 +22,8 @@
 #include <linux/string.h>
 #include <linux/ctype.h>
 #include <linux/kernel.h>
+#include <linux/kallsyms.h>
+#include <linux/uaccess.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/div64.h>
@@ -482,6 +484,82 @@ static char *number(char *buf, char *end, unsigned long long num, int base, int
 	return buf;
 }
 
+static char *string(char *buf, char *end, char *s, int field_width, int precision, int flags)
+{
+	int len, i;
+
+	if ((unsigned long)s < PAGE_SIZE)
+		s = "<NULL>";
+
+	len = strnlen(s, precision);
+
+	if (!(flags & LEFT)) {
+		while (len < field_width--) {
+			if (buf < end)
+				*buf = ' ';
+			++buf;
+		}
+	}
+	for (i = 0; i < len; ++i) {
+		if (buf < end)
+			*buf = *s;
+		++buf; ++s;
+	}
+	while (len < field_width--) {
+		if (buf < end)
+			*buf = ' ';
+		++buf;
+	}
+	return buf;
+}
+
+static inline void *dereference_function_descriptor(void *ptr)
+{
+#if defined(CONFIG_IA64) || defined(CONFIG_PPC64)
+	void *p;
+	if (!probe_kernel_address(ptr, p))
+		ptr = p;
+#endif
+	return ptr;
+}
+
+
+/*
+ * Show a '%p' thing.  A kernel extension is that the '%p' is followed
+ * by an extra set of alphanumeric characters that are extended format
+ * specifiers.  Right now we just handle 'F' (for symbolic Function
+ * pointers) and 'S' (for Symbolic data pointers), but this can easily
+ * be extended in the future (network address types etc).
+ *
+ * The difference between 'S' and 'F' is that on ia64 and ppc64 function
+ * pointers are really function descriptors, which contain a pointer the
+ * real address. 
+ */
+static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int base, int size, int precision, int type)
+{
+	switch (*fmt) {
+	case 'F':
+		ptr = dereference_function_descriptor(ptr);
+		/* Fallthrough */
+	case 'S': {	/* Other (direct) pointer */
+#if CONFIG_KALLSYMS
+		char sym[KSYM_SYMBOL_LEN];
+		sprint_symbol(sym, (unsigned long) ptr);
+		return string(buf, end, sym, size, precision, type);
+#else
+		type |= SPECIAL;
+		break;
+#endif
+	}
+	}
+	type |= SMALL;
+	if (precision == -1) {
+		precision = 2*sizeof(void *);
+		type |= ZEROPAD;
+	}
+	return number(buf, end, (unsigned long long) ptr, base, size, precision, type);
+}
+
 /**
  * vsnprintf - Format a string and place it in a buffer
  * @buf: The buffer to place the result into
@@ -502,11 +580,9 @@ static char *number(char *buf, char *end, unsigned long long num, int base, int
  */
 int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 {
-	int len;
 	unsigned long long num;
-	int i, base;
+	int base;
 	char *str, *end, c;
-	const char *s;
 
 	int flags;		/* flags to number() */
 
@@ -622,40 +698,16 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 				continue;
 
 			case 's':
-				s = va_arg(args, char *);
-				if ((unsigned long)s < PAGE_SIZE)
-					s = "<NULL>";
-
-				len = strnlen(s, precision);
-
-				if (!(flags & LEFT)) {
-					while (len < field_width--) {
-						if (str < end)
-							*str = ' ';
-						++str;
-					}
-				}
-				for (i = 0; i < len; ++i) {
-					if (str < end)
-						*str = *s;
-					++str; ++s;
-				}
-				while (len < field_width--) {
-					if (str < end)
-						*str = ' ';
-					++str;
-				}
+				str = string(str, end, va_arg(args, char *), field_width, precision, flags);
 				continue;
 
 			case 'p':
-				flags |= SMALL;
-				if (field_width == -1) {
-					field_width = 2*sizeof(void *);
-					flags |= ZEROPAD;
-				}
-				str = number(str, end,
-						(unsigned long) va_arg(args, void *),
+				str = pointer(fmt+1, str, end,
+						va_arg(args, void *),
 						16, field_width, precision, flags);
+				/* Skip all alphanumeric pointer suffixes */
+				while (isalnum(fmt[1]))
+					fmt++;
 				continue;
 
 

Re: the printk problem

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sun, 6 Jul 2008, Ingo Molnar wrote:
> > 
> > applied (with the commit message below) to tip/x86/debug for v2.6.27 
> > merging, thanks Linus. Can i add your SOB too?
> 
> Sure, add my S-O-B. But I hope/assuem that you also added my earlier 
> patch that added the support for '%pS' too? I'm not entirely sure that 
> should go in an x86-specific branch, since it has nothing x86-specific 
> in it.

yeah, agreed, combined it's not an x86 topic anymore.

[ There's some lkml trouble so i've missed the earlier patch. I'm not 
  sure the email problem is on my side, see how incomplete the 
  discussion is on lkml.org as well:

     http://lkml.org/lkml/2008/6/25/170   ]

Anyway, i have have added this second patch of yours to tip/core/printk 
and moved your first patch over to that topic (which relies on it).

That topic has a few other (smaller) printk enhancements queued for 
v2.6.27 already:

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git core/printk

[find other stats below]

so it fits in naturally.

	Ingo

------------------>
Ingo Molnar (1):
      printk: export console_drivers

Jan Kiszka (1):
      printk: don't prefer unsuited consoles on registration

Jiri Slaby (1):
      x86, generic: mark early_printk as asmlinkage

Linus Torvalds (2):
      printk: add support for '%pS'
      x86, 64-bit: standardize printk_address()

Nick Andrew (2):
      printk: refactor processing of line severity tokens
      printk: remember the message level for multi-line output

Tejun Heo (1):
      printk: clean up recursion check related static variables

Thomas Gleixner (2):
      namespacecheck: fix kernel printk.c
      namespacecheck: more kernel/printk.c fixes

 arch/x86/kernel/early_printk.c |    2 +-
 arch/x86/kernel/traps_64.c     |   25 +--------
 include/linux/kernel.h         |    8 +---
 kernel/printk.c                |  107 +++++++++++++++---------------------
 lib/vsprintf.c                 |  118 +++++++++++++++++++++++++++++-----------
 5 files changed, 133 insertions(+), 127 deletions(-)

diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
index 643fd86..ff9e735 100644
--- a/arch/x86/kernel/early_printk.c
+++ b/arch/x86/kernel/early_printk.c
@@ -196,7 +196,7 @@ static struct console simnow_console = {
 static struct console *early_console = &early_vga_console;
 static int early_console_initialized;
 
-void early_printk(const char *fmt, ...)
+asmlinkage void early_printk(const char *fmt, ...)
 {
 	char buf[512];
 	int n;
diff --git a/arch/x86/kernel/traps_64.c b/arch/x86/kernel/traps_64.c
index adff76e..f1a95d1 100644
--- a/arch/x86/kernel/traps_64.c
+++ b/arch/x86/kernel/traps_64.c
@@ -104,30 +104,7 @@ int kstack_depth_to_print = 12;
 
 void printk_address(unsigned long address, int reliable)
 {
-#ifdef CONFIG_KALLSYMS
-	unsigned long offset = 0, symsize;
-	const char *symname;
-	char *modname;
-	char *delim = ":";
-	char namebuf[KSYM_NAME_LEN];
-	char reliab[4] = "";
-
-	symname = kallsyms_lookup(address, &symsize, &offset,
-					&modname, namebuf);
-	if (!symname) {
-		printk(" [<%016lx>]\n", address);
-		return;
-	}
-	if (!reliable)
-		strcpy(reliab, "? ");
-
-	if (!modname)
-		modname = delim = "";
-	printk(" [<%016lx>] %s%s%s%s%s+0x%lx/0x%lx\n",
-		address, reliab, delim, modname, delim, symname, offset, symsize);
-#else
-	printk(" [<%016lx>]\n", address);
-#endif
+	printk(" [<%016lx>] %s%pS\n", address, reliable ? "": "? ", (void *) address);
 }
 
 static unsigned long *in_exception_stack(unsigned cpu, unsigned long stack,
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 792bf0a..4cb8d3d 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -184,9 +184,6 @@ asmlinkage int vprintk(const char *fmt, va_list args)
 	__attribute__ ((format (printf, 1, 0)));
 asmlinkage int printk(const char * fmt, ...)
 	__attribute__ ((format (printf, 1, 2))) __cold;
-extern int log_buf_get_len(void);
-extern int log_buf_read(int idx);
-extern int log_buf_copy(char *dest, int idx, int len);
 
 extern int printk_ratelimit_jiffies;
 extern int printk_ratelimit_burst;
@@ -202,9 +199,6 @@ static inline int vprintk(const char *s, va_list args) { return 0; }
 static inline int printk(const char *s, ...)
 	__attribute__ ((format (printf, 1, 2)));
 static inline int __cold printk(const char *s, ...) { return 0; }
-static inline int log_buf_get_len(void) { return 0; }
-static inline int log_buf_read(int idx) { return 0; }
-static inline int log_buf_copy(char *dest, int idx, int len) { return 0; }
 static inline int printk_ratelimit(void) { return 0; }
 static inline int __printk_ratelimit(int ratelimit_jiffies, \
 				     int ratelimit_burst) { return 0; }
@@ -213,7 +207,7 @@ static inline bool printk_timed_ratelimit(unsigned long *caller_jiffies, \
 		{ return false; }
 #endif
 
-extern void __attribute__((format(printf, 1, 2)))
+extern void asmlinkage __attribute__((format(printf, 1, 2)))
 	early_printk(const char *fmt, ...);
 
 unsigned long int_sqrt(unsigned long);
diff --git a/kernel/printk.c b/kernel/printk.c
index 8fb01c3..de1a4f4 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -38,7 +38,7 @@
 /*
  * Architectures can override it:
  */
-void __attribute__((weak)) early_printk(const char *fmt, ...)
+void asmlinkage __attribute__((weak)) early_printk(const char *fmt, ...)
 {
 }
 
@@ -75,6 +75,8 @@ EXPORT_SYMBOL(oops_in_progress);
 static DECLARE_MUTEX(console_sem);
 static DECLARE_MUTEX(secondary_console_sem);
 struct console *console_drivers;
+EXPORT_SYMBOL_GPL(console_drivers);
+
 /*
  * This is used for debugging the mess that is the VT code by
  * keeping track if we have the console semaphore held. It's
@@ -231,7 +233,7 @@ static inline void boot_delay_msec(void)
 /*
  * Return the number of unread characters in the log buffer.
  */
-int log_buf_get_len(void)
+static int log_buf_get_len(void)
 {
 	return logged_chars;
 }
@@ -268,19 +270,6 @@ int log_buf_copy(char *dest, int idx, int len)
 }
 
 /*
- * Extract a single character from the log buffer.
- */
-int log_buf_read(int idx)
-{
-	char ret;
-
-	if (log_buf_copy(&ret, idx, 1) == 1)
-		return ret;
-	else
-		return -1;
-}
-
-/*
  * Commands to do_syslog:
  *
  * 	0 -- Close the log.  Currently a NOP.
@@ -665,18 +654,17 @@ static int acquire_console_semaphore_for_printk(unsigned int cpu)
 	spin_unlock(&logbuf_lock);
 	return retval;
 }
-
-const char printk_recursion_bug_msg [] =
-			KERN_CRIT "BUG: recent printk recursion!\n";
-static int printk_recursion_bug;
+static const char recursion_bug_msg [] =
+		KERN_CRIT "BUG: recent printk recursion!\n";
+static int recursion_bug;
+	static int new_text_line = 1;
+static char printk_buf[1024];
 
 asmlinkage int vprintk(const char *fmt, va_list args)
 {
-	static int log_level_unknown = 1;
-	static char printk_buf[1024];
-
-	unsigned long flags;
 	int printed_len = 0;
+	int current_log_level = default_message_loglevel;
+	unsigned long flags;
 	int this_cpu;
 	char *p;
 
@@ -699,7 +687,7 @@ asmlinkage int vprintk(const char *fmt, va_list args)
 		 * it can be printed at the next appropriate moment:
 		 */
 		if (!oops_in_progress) {
-			printk_recursion_bug = 1;
+			recursion_bug = 1;
 			goto out_restore_irqs;
 		}
 		zap_locks();
@@ -709,70 +697,62 @@ asmlinkage int vprintk(const char *fmt, va_list args)
 	spin_lock(&logbuf_lock);
 	printk_cpu = this_cpu;
 
-	if (printk_recursion_bug) {
-		printk_recursion_bug = 0;
-		strcpy(printk_buf, printk_recursion_bug_msg);
-		printed_len = sizeof(printk_recursion_bug_msg);
+	if (recursion_bug) {
+		recursion_bug = 0;
+		strcpy(printk_buf, recursion_bug_msg);
+		printed_len = sizeof(recursion_bug_msg);
 	}
 	/* Emit the output into the temporary buffer */
 	printed_len += vscnprintf(printk_buf + printed_len,
 				  sizeof(printk_buf) - printed_len, fmt, args);
 
+
 	/*
 	 * Copy the output into log_buf.  If the caller didn't provide
 	 * appropriate log level tags, we insert them here
 	 */
 	for (p = printk_buf; *p; p++) {
-		if (log_level_unknown) {
-                        /* log_level_unknown signals the start of a new line */
+		if (new_text_line) {
+			/* If a token, set current_log_level and skip over */
+			if (p[0] == '<' && p[1] >= '0' && p[1] <= '7' &&
+			    p[2] == '>') {
+				current_log_level = p[1] - '0';
+				p += 3;
+				printed_len -= 3;
+			}
+
+			/* Always output the token */
+			emit_log_char('<');
+			emit_log_char(current_log_level + '0');
+			emit_log_char('>');
+			printed_len += 3;
+			new_text_line = 0;
+
 			if (printk_time) {
-				int loglev_char;
+				/* Follow the token with the time */
 				char tbuf[50], *tp;
 				unsigned tlen;
 				unsigned long long t;
 				unsigned long nanosec_rem;
 
-				/*
-				 * force the log level token to be
-				 * before the time output.
-				 */
-				if (p[0] == '<' && p[1] >='0' &&
-				   p[1] <= '7' && p[2] == '>') {
-					loglev_char = p[1];
-					p += 3;
-					printed_len -= 3;
-				} else {
-					loglev_char = default_message_loglevel
-						+ '0';
-				}
 				t = cpu_clock(printk_cpu);
 				nanosec_rem = do_div(t, 1000000000);
-				tlen = sprintf(tbuf,
-						"<%c>[%5lu.%06lu] ",
-						loglev_char,
-						(unsigned long)t,
-						nanosec_rem/1000);
+				tlen = sprintf(tbuf, "[%5lu.%06lu] ",
+						(unsigned long) t,
+						nanosec_rem / 1000);
 
 				for (tp = tbuf; tp < tbuf + tlen; tp++)
 					emit_log_char(*tp);
 				printed_len += tlen;
-			} else {
-				if (p[0] != '<' || p[1] < '0' ||
-				   p[1] > '7' || p[2] != '>') {
-					emit_log_char('<');
-					emit_log_char(default_message_loglevel
-						+ '0');
-					emit_log_char('>');
-					printed_len += 3;
-				}
 			}
-			log_level_unknown = 0;
+
 			if (!*p)
 				break;
 		}
+
 		emit_log_char(*p);
 		if (*p == '\n')
-			log_level_unknown = 1;
+			new_text_line = 1;
 	}
 
 	/*
@@ -1172,8 +1152,11 @@ void register_console(struct console *console)
 			console->index = 0;
 		if (console->setup == NULL ||
 		    console->setup(console, NULL) == 0) {
-			console->flags |= CON_ENABLED | CON_CONSDEV;
-			preferred_console = 0;
+			console->flags |= CON_ENABLED;
+			if (console->device) {
+				console->flags |= CON_CONSDEV;
+				preferred_console = 0;
+			}
 		}
 	}
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 6021757..f60c7c0 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -22,6 +22,8 @@
 #include <linux/string.h>
 #include <linux/ctype.h>
 #include <linux/kernel.h>
+#include <linux/kallsyms.h>
+#include <linux/uaccess.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/div64.h>
@@ -482,6 +484,82 @@ static char *number(char *buf, char *end, unsigned long long num, int base, int
 	return buf;
 }
 
+static char *string(char *buf, char *end, char *s, int field_width, int precision, int flags)
+{
+	int len, i;
+
+	if ((unsigned long)s < PAGE_SIZE)
+		s = "<NULL>";
+
+	len = strnlen(s, precision);
+
+	if (!(flags & LEFT)) {
+		while (len < field_width--) {
+			if (buf < end)
+				*buf = ' ';
+			++buf;
+		}
+	}
+	for (i = 0; i < len; ++i) {
+		if (buf < end)
+			*buf = *s;
+		++buf; ++s;
+	}
+	while (len < field_width--) {
+		if (buf < end)
+			*buf = ' ';
+		++buf;
+	}
+	return buf;
+}
+
+static inline void *dereference_function_descriptor(void *ptr)
+{
+#if defined(CONFIG_IA64) || defined(CONFIG_PPC64)
+	void *p;
+	if (!probe_kernel_address(ptr, p))
+		ptr = p;
+#endif
+	return ptr;
+}
+
+
+/*
+ * Show a '%p' thing.  A kernel extension is that the '%p' is followed
+ * by an extra set of alphanumeric characters that are extended format
+ * specifiers.  Right now we just handle 'F' (for symbolic Function
+ * pointers) and 'S' (for Symbolic data pointers), but this can easily
+ * be extended in the future (network address types etc).
+ *
+ * The difference between 'S' and 'F' is that on ia64 and ppc64 function
+ * pointers are really function descriptors, which contain a pointer the
+ * real address. 
+ */
+static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int base, int size, int precision, int type)
+{
+	switch (*fmt) {
+	case 'F':
+		ptr = dereference_function_descriptor(ptr);
+		/* Fallthrough */
+	case 'S': {	/* Other (direct) pointer */
+#if CONFIG_KALLSYMS
+		char sym[KSYM_SYMBOL_LEN];
+		sprint_symbol(sym, (unsigned long) ptr);
+		return string(buf, end, sym, size, precision, type);
+#else
+		type |= SPECIAL;
+		break;
+#endif
+	}
+	}
+	type |= SMALL;
+	if (precision == -1) {
+		precision = 2*sizeof(void *);
+		type |= ZEROPAD;
+	}
+	return number(buf, end, (unsigned long long) ptr, base, size, precision, type);
+}
+
 /**
  * vsnprintf - Format a string and place it in a buffer
  * @buf: The buffer to place the result into
@@ -502,11 +580,9 @@ static char *number(char *buf, char *end, unsigned long long num, int base, int
  */
 int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 {
-	int len;
 	unsigned long long num;
-	int i, base;
+	int base;
 	char *str, *end, c;
-	const char *s;
 
 	int flags;		/* flags to number() */
 
@@ -622,40 +698,16 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 				continue;
 
 			case 's':
-				s = va_arg(args, char *);
-				if ((unsigned long)s < PAGE_SIZE)
-					s = "<NULL>";
-
-				len = strnlen(s, precision);
-
-				if (!(flags & LEFT)) {
-					while (len < field_width--) {
-						if (str < end)
-							*str = ' ';
-						++str;
-					}
-				}
-				for (i = 0; i < len; ++i) {
-					if (str < end)
-						*str = *s;
-					++str; ++s;
-				}
-				while (len < field_width--) {
-					if (str < end)
-						*str = ' ';
-					++str;
-				}
+				str = string(str, end, va_arg(args, char *), field_width, precision, flags);
 				continue;
 
 			case 'p':
-				flags |= SMALL;
-				if (field_width == -1) {
-					field_width = 2*sizeof(void *);
-					flags |= ZEROPAD;
-				}
-				str = number(str, end,
-						(unsigned long) va_arg(args, void *),
+				str = pointer(fmt+1, str, end,
+						va_arg(args, void *),
 						16, field_width, precision, flags);
+				/* Skip all alphanumeric pointer suffixes */
+				while (isalnum(fmt[1]))
+					fmt++;
 				continue;
 
 

Re: the printk problem

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> yeah, agreed, combined it's not an x86 topic anymore.
> 
> [ There's some lkml trouble so i've missed the earlier patch. I'm not 
>   sure the email problem is on my side, see how incomplete the 
>   discussion is on lkml.org as well:
> 
>      http://lkml.org/lkml/2008/6/25/170   ]
> 
> Anyway, i have have added this second patch of yours to 
> tip/core/printk and moved your first patch over to that topic (which 
> relies on it).

ah, i found the reason - it started out on linux-ia64 originally then 
moved over to lkml - so only half of the discussion was visible there. 
And linux-ia64 is one of the few vger lists i'm not subscribed to 
apparently. (there's no vger-please-give-me-all-emails list - making the 
following of Linux development even harder)

	Ingo

Re: the printk problem

[Benjamin Herrenschmidt]

On Fri, 2008-07-04 at 16:25 -0700, Linus Torvalds wrote:
> 
> On Sat, 5 Jul 2008, Benjamin Herrenschmidt wrote:
> > 
> > I'll give it a try using probe_kernel_address() instead on monday.
> 
> Here's the updated patch which uses probe_kernel_address() instead (and 
> moves the whole #ifdef mess out of the code that wants it and into a 
> helper function - and maybe we should then put that helper into 
> kallsyms.h, but that's a different issue).
> 
> Still all happily untested, of course. And still with no actual users 
> converted.

Did a few tests and it seems to work. I'll stick a patch converting
powerpc to use %pS for oops display in -next.

Thanks !

Cheers,
Ben.

Re: the printk problem

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1036 bytes --]

On Mon, 07 Jul 2008 11:14:36 +1000 Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
> On Fri, 2008-07-04 at 16:25 -0700, Linus Torvalds wrote:
> > 
> > On Sat, 5 Jul 2008, Benjamin Herrenschmidt wrote:
> > > 
> > > I'll give it a try using probe_kernel_address() instead on monday.
> > 
> > Here's the updated patch which uses probe_kernel_address() instead (and 
> > moves the whole #ifdef mess out of the code that wants it and into a 
> > helper function - and maybe we should then put that helper into 
> > kallsyms.h, but that's a different issue).
> > 
> > Still all happily untested, of course. And still with no actual users 
> > converted.
> 
> Did a few tests and it seems to work. I'll stick a patch converting
> powerpc to use %pS for oops display in -next.

After you post it to linuxppc-dev and get review comments, of course ...

And after the patch to make %pS work goes in ...
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: the printk problem

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1379 bytes --]

On Mon, 2008-07-07 at 13:26 +1000, Stephen Rothwell wrote:
> On Mon, 07 Jul 2008 11:14:36 +1000 Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> >
> > On Fri, 2008-07-04 at 16:25 -0700, Linus Torvalds wrote:
> > > 
> > > On Sat, 5 Jul 2008, Benjamin Herrenschmidt wrote:
> > > > 
> > > > I'll give it a try using probe_kernel_address() instead on monday.
> > > 
> > > Here's the updated patch which uses probe_kernel_address() instead (and 
> > > moves the whole #ifdef mess out of the code that wants it and into a 
> > > helper function - and maybe we should then put that helper into 
> > > kallsyms.h, but that's a different issue).
> > > 
> > > Still all happily untested, of course. And still with no actual users 
> > > converted.
> > 
> > Did a few tests and it seems to work. I'll stick a patch converting
> > powerpc to use %pS for oops display in -next.
> 
> After you post it to linuxppc-dev and get review comments, of course ...
> 
> And after the patch to make %pS work goes in ...

Wasn't that already merged via the trivial scheduler fixes tree or
something? ;)

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: the printk problem

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 296 bytes --]

On Mon, 07 Jul 2008 13:28:18 +1000 Michael Ellerman <michael@ellerman.id.au> wrote:
>
> Wasn't that already merged via the trivial scheduler fixes tree or
> something? ;)

Not yet.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: the printk problem

[Benjamin Herrenschmidt]

On Mon, 2008-07-07 at 13:26 +1000, Stephen Rothwell wrote:
> > Did a few tests and it seems to work. I'll stick a patch converting
> > powerpc to use %pS for oops display in -next.
> 
> After you post it to linuxppc-dev and get review comments, of
> course ...

I though I did that already, looks like I didn't. Let me see what went
wrong..

> And after the patch to make %pS work goes in ...

Yup.

Ben.