* Re: the printk problem [not found] ` <alpine.LFD.1.10.0807041147450.2815@woody.linux-foundation.org> @ 2008-07-04 20:02 ` Linus Torvalds 2008-07-04 20:27 ` Andrew Morton ` (2 more replies) 0 siblings, 3 replies; 40+ messages in thread From: Linus Torvalds @ 2008-07-04 20:02 UTC (permalink / raw) To: Andrew Morton; +Cc: linuxppc-dev, linux-ia64, David S. Miller, Peter Anvin 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; ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-04 20:02 ` the printk problem Linus Torvalds @ 2008-07-04 20:27 ` Andrew Morton 2008-07-04 20:41 ` Linus Torvalds 2008-07-04 20:42 ` Matthew Wilcox 2008-07-04 20:36 ` Matthew Wilcox 2008-07-04 23:00 ` Benjamin Herrenschmidt 2 siblings, 2 replies; 40+ messages in thread From: Andrew Morton @ 2008-07-04 20:27 UTC (permalink / raw) To: Linus Torvalds; +Cc: linuxppc-dev, linux-ia64, David S. Miller, Peter Anvin 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. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-04 20:27 ` Andrew Morton @ 2008-07-04 20:41 ` Linus Torvalds 2008-07-04 20:42 ` Matthew Wilcox 1 sibling, 0 replies; 40+ messages in thread From: Linus Torvalds @ 2008-07-04 20:41 UTC (permalink / raw) To: Andrew Morton; +Cc: linuxppc-dev, linux-ia64, David S. Miller, Peter Anvin 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-04 20:27 ` Andrew Morton 2008-07-04 20:41 ` Linus Torvalds @ 2008-07-04 20:42 ` Matthew Wilcox 2008-07-04 22:01 ` Andrew Morton 2008-07-04 22:58 ` Benjamin Herrenschmidt 1 sibling, 2 replies; 40+ messages in thread From: Matthew Wilcox @ 2008-07-04 20:42 UTC (permalink / raw) To: Andrew Morton Cc: linuxppc-dev, linux-ia64, Linus Torvalds, David S. Miller, Peter Anvin 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." ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-04 20:42 ` Matthew Wilcox @ 2008-07-04 22:01 ` Andrew Morton 2008-07-05 2:03 ` Matthew Wilcox ` (2 more replies) 2008-07-04 22:58 ` Benjamin Herrenschmidt 1 sibling, 3 replies; 40+ messages in thread From: Andrew Morton @ 2008-07-04 22:01 UTC (permalink / raw) To: Matthew Wilcox Cc: linux-ia64, linux-kernel, linuxppc-dev, Peter Anvin, Linus Torvalds, David S. Miller (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? ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-04 22:01 ` Andrew Morton @ 2008-07-05 2:03 ` Matthew Wilcox 2008-07-22 10:05 ` [PATCH] Make u64 long long on all architectures (was: the printk problem) Andrew Morton 2008-07-05 10:20 ` the printk problem Denys Vlasenko 2008-07-05 11:33 ` Jan Engelhardt 2 siblings, 1 reply; 40+ messages in thread From: Matthew Wilcox @ 2008-07-05 2:03 UTC (permalink / raw) To: Andrew Morton Cc: linux-ia64, linux-kernel, linuxppc-dev, Peter Anvin, Linus Torvalds, David S. Miller 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." ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH] Make u64 long long on all architectures (was: the printk problem) 2008-07-05 2:03 ` Matthew Wilcox @ 2008-07-22 10:05 ` Andrew Morton 2008-07-22 10:36 ` Michael Ellerman 2008-07-22 11:35 ` Benjamin Herrenschmidt 0 siblings, 2 replies; 40+ messages in thread From: Andrew Morton @ 2008-07-22 10:05 UTC (permalink / raw) To: Matthew Wilcox Cc: linux-arch, linux-ia64, linux-kernel, linuxppc-dev, Peter Anvin, Linus Torvalds, David S. Miller 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. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] Make u64 long long on all architectures (was: the printk problem) 2008-07-22 10:05 ` [PATCH] Make u64 long long on all architectures (was: the printk problem) Andrew Morton @ 2008-07-22 10:36 ` Michael Ellerman 2008-07-22 10:53 ` Andrew Morton 2008-07-22 11:36 ` Benjamin Herrenschmidt 2008-07-22 11:35 ` Benjamin Herrenschmidt 1 sibling, 2 replies; 40+ messages in thread From: Michael Ellerman @ 2008-07-22 10:36 UTC (permalink / raw) To: Andrew Morton Cc: linux-arch, linux-ia64, Matthew Wilcox, linux-kernel, linuxppc-dev, Peter Anvin, Linus Torvalds, David S. Miller [-- 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 --] ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] Make u64 long long on all architectures (was: the printk problem) 2008-07-22 10:36 ` Michael Ellerman @ 2008-07-22 10:53 ` Andrew Morton 2008-07-22 11:36 ` Benjamin Herrenschmidt 1 sibling, 0 replies; 40+ messages in thread From: Andrew Morton @ 2008-07-22 10:53 UTC (permalink / raw) To: michael Cc: linux-arch, linux-ia64, Matthew Wilcox, linux-kernel, linuxppc-dev, Peter Anvin, Linus Torvalds, David S. Miller 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. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] Make u64 long long on all architectures (was: the printk problem) 2008-07-22 10:36 ` Michael Ellerman 2008-07-22 10:53 ` Andrew Morton @ 2008-07-22 11:36 ` Benjamin Herrenschmidt 1 sibling, 0 replies; 40+ messages in thread From: Benjamin Herrenschmidt @ 2008-07-22 11:36 UTC (permalink / raw) To: michael Cc: linux-arch, linux-ia64, Matthew Wilcox, linux-kernel, linuxppc-dev, Peter Anvin, Andrew Morton, Linus Torvalds, David S. Miller 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. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] Make u64 long long on all architectures (was: the printk problem) 2008-07-22 10:05 ` [PATCH] Make u64 long long on all architectures (was: the printk problem) Andrew Morton 2008-07-22 10:36 ` Michael Ellerman @ 2008-07-22 11:35 ` Benjamin Herrenschmidt 1 sibling, 0 replies; 40+ messages in thread From: Benjamin Herrenschmidt @ 2008-07-22 11:35 UTC (permalink / raw) To: Andrew Morton Cc: linux-arch, linux-ia64, Matthew Wilcox, linux-kernel, linuxppc-dev, Peter Anvin, Linus Torvalds, David S. Miller > > 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. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-04 22:01 ` Andrew Morton 2008-07-05 2:03 ` Matthew Wilcox @ 2008-07-05 10:20 ` Denys Vlasenko 2008-07-05 11:33 ` Jan Engelhardt 2 siblings, 0 replies; 40+ messages in thread From: Denys Vlasenko @ 2008-07-05 10:20 UTC (permalink / raw) To: Andrew Morton Cc: linux-ia64, Matthew Wilcox, linux-kernel, linuxppc-dev, Peter Anvin, Linus Torvalds, David S. Miller 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-04 22:01 ` Andrew Morton 2008-07-05 2:03 ` Matthew Wilcox 2008-07-05 10:20 ` the printk problem Denys Vlasenko @ 2008-07-05 11:33 ` Jan Engelhardt 2008-07-05 12:52 ` Vegard Nossum 2 siblings, 1 reply; 40+ messages in thread From: Jan Engelhardt @ 2008-07-05 11:33 UTC (permalink / raw) To: Andrew Morton Cc: linux-ia64, Matthew Wilcox, linux-kernel, linuxppc-dev, Peter Anvin, Linus Torvalds, David S. Miller 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}? ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-05 11:33 ` Jan Engelhardt @ 2008-07-05 12:52 ` Vegard Nossum 2008-07-05 13:24 ` Jan Engelhardt 2008-07-05 17:56 ` Linus Torvalds 0 siblings, 2 replies; 40+ messages in thread From: Vegard Nossum @ 2008-07-05 12:52 UTC (permalink / raw) To: Jan Engelhardt Cc: linux-ia64, Matthew Wilcox, linux-kernel, linuxppc-dev, Peter Anvin, Andrew Morton, Linus Torvalds, David S. Miller 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 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-05 12:52 ` Vegard Nossum @ 2008-07-05 13:24 ` Jan Engelhardt 2008-07-05 13:50 ` Vegard Nossum 2008-07-05 17:56 ` Linus Torvalds 1 sibling, 1 reply; 40+ messages in thread From: Jan Engelhardt @ 2008-07-05 13:24 UTC (permalink / raw) To: Vegard Nossum Cc: linux-ia64, Matthew Wilcox, linux-kernel, linuxppc-dev, Peter Anvin, Andrew Morton, Linus Torvalds, David S. Miller 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; ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-05 13:24 ` Jan Engelhardt @ 2008-07-05 13:50 ` Vegard Nossum 2008-07-05 14:07 ` Jan Engelhardt 0 siblings, 1 reply; 40+ messages in thread From: Vegard Nossum @ 2008-07-05 13:50 UTC (permalink / raw) To: Jan Engelhardt Cc: linux-ia64, Matthew Wilcox, linux-kernel, linuxppc-dev, Peter Anvin, Andrew Morton, Linus Torvalds, David S. Miller 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-05 13:50 ` Vegard Nossum @ 2008-07-05 14:07 ` Jan Engelhardt 0 siblings, 0 replies; 40+ messages in thread From: Jan Engelhardt @ 2008-07-05 14:07 UTC (permalink / raw) To: Vegard Nossum Cc: linux-ia64, Matthew Wilcox, linux-kernel, linuxppc-dev, Peter Anvin, Andrew Morton, Linus Torvalds, David S. Miller 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. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-05 12:52 ` Vegard Nossum 2008-07-05 13:24 ` Jan Engelhardt @ 2008-07-05 17:56 ` Linus Torvalds 2008-07-05 18:40 ` Jan Engelhardt 2008-07-05 18:41 ` Vegard Nossum 1 sibling, 2 replies; 40+ messages in thread From: Linus Torvalds @ 2008-07-05 17:56 UTC (permalink / raw) To: Vegard Nossum Cc: linux-ia64, Matthew Wilcox, linux-kernel, Jan Engelhardt, linuxppc-dev, Peter Anvin, Andrew Morton, David S. Miller 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-05 17:56 ` Linus Torvalds @ 2008-07-05 18:40 ` Jan Engelhardt 2008-07-05 18:44 ` Linus Torvalds 2008-07-05 18:41 ` Vegard Nossum 1 sibling, 1 reply; 40+ messages in thread From: Jan Engelhardt @ 2008-07-05 18:40 UTC (permalink / raw) To: Linus Torvalds Cc: linux-ia64, Matthew Wilcox, Vegard Nossum, linux-kernel, linuxppc-dev, Peter Anvin, Andrew Morton, David S. Miller 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? ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-05 18:40 ` Jan Engelhardt @ 2008-07-05 18:44 ` Linus Torvalds 0 siblings, 0 replies; 40+ messages in thread From: Linus Torvalds @ 2008-07-05 18:44 UTC (permalink / raw) To: Jan Engelhardt Cc: linux-ia64, Matthew Wilcox, Vegard Nossum, linux-kernel, linuxppc-dev, Peter Anvin, Andrew Morton, David S. Miller 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-05 17:56 ` Linus Torvalds 2008-07-05 18:40 ` Jan Engelhardt @ 2008-07-05 18:41 ` Vegard Nossum 2008-07-05 18:52 ` Matthew Wilcox 1 sibling, 1 reply; 40+ messages in thread From: Vegard Nossum @ 2008-07-05 18:41 UTC (permalink / raw) To: Linus Torvalds Cc: linux-ia64, Matthew Wilcox, linux-kernel, Jan Engelhardt, linuxppc-dev, Peter Anvin, Andrew Morton, David S. Miller 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-05 18:41 ` Vegard Nossum @ 2008-07-05 18:52 ` Matthew Wilcox 2008-07-06 0:02 ` Pekka Enberg 0 siblings, 1 reply; 40+ messages in thread From: Matthew Wilcox @ 2008-07-05 18:52 UTC (permalink / raw) To: Vegard Nossum Cc: linux-ia64, linux-kernel, Jan Engelhardt, linuxppc-dev, Peter Anvin, Andrew Morton, Linus Torvalds, David S. Miller 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." ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-05 18:52 ` Matthew Wilcox @ 2008-07-06 0:02 ` Pekka Enberg 2008-07-06 5:17 ` Randy Dunlap 0 siblings, 1 reply; 40+ messages in thread From: Pekka Enberg @ 2008-07-06 0:02 UTC (permalink / raw) To: Matthew Wilcox Cc: linux-ia64, Vegard Nossum, linux-kernel, Jan Engelhardt, linuxppc-dev, Peter Anvin, Andrew Morton, Linus Torvalds, David S. Miller 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? ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-06 0:02 ` Pekka Enberg @ 2008-07-06 5:17 ` Randy Dunlap 0 siblings, 0 replies; 40+ messages in thread From: Randy Dunlap @ 2008-07-06 5:17 UTC (permalink / raw) To: Pekka Enberg Cc: linux-ia64, Matthew Wilcox, Vegard Nossum, linux-kernel, Jan Engelhardt, linuxppc-dev, Peter Anvin, Andrew Morton, Linus Torvalds, David S. Miller 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/ ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-04 20:42 ` Matthew Wilcox 2008-07-04 22:01 ` Andrew Morton @ 2008-07-04 22:58 ` Benjamin Herrenschmidt 1 sibling, 0 replies; 40+ messages in thread From: Benjamin Herrenschmidt @ 2008-07-04 22:58 UTC (permalink / raw) To: Matthew Wilcox Cc: linux-ia64, linuxppc-dev, Peter Anvin, Andrew Morton, Linus Torvalds, David S. Miller > 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. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-04 20:02 ` the printk problem Linus Torvalds 2008-07-04 20:27 ` Andrew Morton @ 2008-07-04 20:36 ` Matthew Wilcox 2008-07-08 1:44 ` Kyle McMartin 2008-07-04 23:00 ` Benjamin Herrenschmidt 2 siblings, 1 reply; 40+ messages in thread From: Matthew Wilcox @ 2008-07-04 20:36 UTC (permalink / raw) To: Linus Torvalds Cc: linux-ia64, linuxppc-dev, Peter Anvin, Andrew Morton, David S. Miller, parisc-linux 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." ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-04 20:36 ` Matthew Wilcox @ 2008-07-08 1:44 ` Kyle McMartin 0 siblings, 0 replies; 40+ messages in thread From: Kyle McMartin @ 2008-07-08 1:44 UTC (permalink / raw) To: Matthew Wilcox Cc: linux-ia64, linuxppc-dev, Peter Anvin, Andrew Morton, Linus Torvalds, David S. Miller, parisc-linux 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. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-04 20:02 ` the printk problem Linus Torvalds 2008-07-04 20:27 ` Andrew Morton 2008-07-04 20:36 ` Matthew Wilcox @ 2008-07-04 23:00 ` Benjamin Herrenschmidt 2008-07-04 23:25 ` Linus Torvalds 2 siblings, 1 reply; 40+ messages in thread From: Benjamin Herrenschmidt @ 2008-07-04 23:00 UTC (permalink / raw) To: Linus Torvalds Cc: linuxppc-dev, Andrew Morton, linux-ia64, David S. Miller, Peter Anvin 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. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-04 23:00 ` Benjamin Herrenschmidt @ 2008-07-04 23:25 ` Linus Torvalds 2008-07-05 22:32 ` Linus Torvalds 2008-07-07 1:14 ` Benjamin Herrenschmidt 0 siblings, 2 replies; 40+ messages in thread From: Linus Torvalds @ 2008-07-04 23:25 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: linux-ia64, linuxppc-dev, Peter Anvin, Andrew Morton, David S. Miller, parisc-linux 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; ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-04 23:25 ` Linus Torvalds @ 2008-07-05 22:32 ` Linus Torvalds 2008-07-05 22:57 ` Arjan van de Ven 2008-07-06 5:27 ` Ingo Molnar 2008-07-07 1:14 ` Benjamin Herrenschmidt 1 sibling, 2 replies; 40+ messages in thread From: Linus Torvalds @ 2008-07-05 22:32 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: linux-ia64, linuxppc-dev, Peter Anvin, Andrew Morton, Arjan van de Ven, David S. Miller, parisc-linux 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, ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-05 22:32 ` Linus Torvalds @ 2008-07-05 22:57 ` Arjan van de Ven 2008-07-06 5:27 ` Ingo Molnar 1 sibling, 0 replies; 40+ messages in thread From: Arjan van de Ven @ 2008-07-05 22:57 UTC (permalink / raw) To: Linus Torvalds Cc: linux-ia64, linuxppc-dev, Peter Anvin, Andrew Morton, David S. Miller, parisc-linux 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> ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-05 22:32 ` Linus Torvalds 2008-07-05 22:57 ` Arjan van de Ven @ 2008-07-06 5:27 ` Ingo Molnar 2008-07-06 5:37 ` Linus Torvalds 1 sibling, 1 reply; 40+ messages in thread From: Ingo Molnar @ 2008-07-06 5:27 UTC (permalink / raw) To: Linus Torvalds Cc: linux-ia64, linuxppc-dev, Peter Anvin, Andrew Morton, Arjan van de Ven, David S. Miller, parisc-linux * 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, ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-06 5:27 ` Ingo Molnar @ 2008-07-06 5:37 ` Linus Torvalds 2008-07-06 5:53 ` Ingo Molnar 0 siblings, 1 reply; 40+ messages in thread From: Linus Torvalds @ 2008-07-06 5:37 UTC (permalink / raw) To: Ingo Molnar Cc: linux-ia64, Linux Kernel Mailing List, linuxppc-dev, Peter Anvin, Andrew Morton, Arjan van de Ven, David S. Miller, parisc-linux 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; ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-06 5:37 ` Linus Torvalds @ 2008-07-06 5:53 ` Ingo Molnar 2008-07-06 6:13 ` Ingo Molnar 0 siblings, 1 reply; 40+ messages in thread From: Ingo Molnar @ 2008-07-06 5:53 UTC (permalink / raw) To: Linus Torvalds Cc: linux-ia64, Linux Kernel Mailing List, linuxppc-dev, Peter Anvin, Andrew Morton, Arjan van de Ven, David S. Miller, parisc-linux * 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; ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-06 5:53 ` Ingo Molnar @ 2008-07-06 6:13 ` Ingo Molnar 0 siblings, 0 replies; 40+ messages in thread From: Ingo Molnar @ 2008-07-06 6:13 UTC (permalink / raw) To: Linus Torvalds Cc: linux-ia64, Linux Kernel Mailing List, linuxppc-dev, Peter Anvin, Andrew Morton, Arjan van de Ven, David S. Miller, parisc-linux * 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-04 23:25 ` Linus Torvalds 2008-07-05 22:32 ` Linus Torvalds @ 2008-07-07 1:14 ` Benjamin Herrenschmidt 2008-07-07 3:26 ` Stephen Rothwell 1 sibling, 1 reply; 40+ messages in thread From: Benjamin Herrenschmidt @ 2008-07-07 1:14 UTC (permalink / raw) To: Linus Torvalds Cc: linux-ia64, linuxppc-dev, Peter Anvin, Andrew Morton, David S. Miller, parisc-linux 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. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-07 1:14 ` Benjamin Herrenschmidt @ 2008-07-07 3:26 ` Stephen Rothwell 2008-07-07 3:28 ` Michael Ellerman 2008-07-07 3:43 ` Benjamin Herrenschmidt 0 siblings, 2 replies; 40+ messages in thread From: Stephen Rothwell @ 2008-07-07 3:26 UTC (permalink / raw) To: linuxppc-dev [-- 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 --] ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-07 3:26 ` Stephen Rothwell @ 2008-07-07 3:28 ` Michael Ellerman 2008-07-07 4:59 ` Stephen Rothwell 2008-07-07 3:43 ` Benjamin Herrenschmidt 1 sibling, 1 reply; 40+ messages in thread From: Michael Ellerman @ 2008-07-07 3:28 UTC (permalink / raw) To: Stephen Rothwell; +Cc: linuxppc-dev [-- 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 --] ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-07 3:28 ` Michael Ellerman @ 2008-07-07 4:59 ` Stephen Rothwell 0 siblings, 0 replies; 40+ messages in thread From: Stephen Rothwell @ 2008-07-07 4:59 UTC (permalink / raw) To: michael; +Cc: linuxppc-dev [-- 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 --] ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: the printk problem 2008-07-07 3:26 ` Stephen Rothwell 2008-07-07 3:28 ` Michael Ellerman @ 2008-07-07 3:43 ` Benjamin Herrenschmidt 1 sibling, 0 replies; 40+ messages in thread From: Benjamin Herrenschmidt @ 2008-07-07 3:43 UTC (permalink / raw) To: Stephen Rothwell; +Cc: linuxppc-dev 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. ^ permalink raw reply [flat|nested] 40+ messages in thread
end of thread, other threads:[~2008-07-22 11:55 UTC | newest] Thread overview: 40+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- [not found] <20080625131101.GA6205@digi.com> [not found] ` <20080704104634.GA31634@digi.com> [not found] ` <20080704111540.ddffd241.akpm@linux-foundation.org> [not found] ` <alpine.LFD.1.10.0807041147450.2815@woody.linux-foundation.org> 2008-07-04 20:02 ` the printk problem Linus Torvalds 2008-07-04 20:27 ` Andrew Morton 2008-07-04 20:41 ` Linus Torvalds 2008-07-04 20:42 ` Matthew Wilcox 2008-07-04 22:01 ` Andrew Morton 2008-07-05 2:03 ` Matthew Wilcox 2008-07-22 10:05 ` [PATCH] Make u64 long long on all architectures (was: the printk problem) Andrew Morton 2008-07-22 10:36 ` Michael Ellerman 2008-07-22 10:53 ` Andrew Morton 2008-07-22 11:36 ` Benjamin Herrenschmidt 2008-07-22 11:35 ` Benjamin Herrenschmidt 2008-07-05 10:20 ` the printk problem Denys Vlasenko 2008-07-05 11:33 ` Jan Engelhardt 2008-07-05 12:52 ` Vegard Nossum 2008-07-05 13:24 ` Jan Engelhardt 2008-07-05 13:50 ` Vegard Nossum 2008-07-05 14:07 ` Jan Engelhardt 2008-07-05 17:56 ` Linus Torvalds 2008-07-05 18:40 ` Jan Engelhardt 2008-07-05 18:44 ` Linus Torvalds 2008-07-05 18:41 ` Vegard Nossum 2008-07-05 18:52 ` Matthew Wilcox 2008-07-06 0:02 ` Pekka Enberg 2008-07-06 5:17 ` Randy Dunlap 2008-07-04 22:58 ` Benjamin Herrenschmidt 2008-07-04 20:36 ` Matthew Wilcox 2008-07-08 1:44 ` Kyle McMartin 2008-07-04 23:00 ` Benjamin Herrenschmidt 2008-07-04 23:25 ` Linus Torvalds 2008-07-05 22:32 ` Linus Torvalds 2008-07-05 22:57 ` Arjan van de Ven 2008-07-06 5:27 ` Ingo Molnar 2008-07-06 5:37 ` Linus Torvalds 2008-07-06 5:53 ` Ingo Molnar 2008-07-06 6:13 ` Ingo Molnar 2008-07-07 1:14 ` Benjamin Herrenschmidt 2008-07-07 3:26 ` Stephen Rothwell 2008-07-07 3:28 ` Michael Ellerman 2008-07-07 4:59 ` Stephen Rothwell 2008-07-07 3:43 ` Benjamin Herrenschmidt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).