(heck, let's cc lkml - avoid having to go through all this again)
On Fri, 4 Jul 2008 14:42:53 -0600 Matthew Wilcox <[email protected]> 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 <[email protected]> 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?
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 <[email protected]>
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."
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
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}?
On Sat, Jul 5, 2008 at 1:33 PM, Jan Engelhardt <[email protected]> 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 <[email protected]>
Date: Sat, 5 Jul 2008 14:01:00 +0200
Subject: [PATCH] printf: add %p{} extension
Signed-off-by: Vegard Nossum <[email protected]>
---
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
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;
On Sat, Jul 5, 2008 at 3:24 PM, Jan Engelhardt <[email protected]> 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
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.
On Sat, 5 Jul 2008, Vegard Nossum wrote:
> On Sat, Jul 5, 2008 at 1:33 PM, Jan Engelhardt <[email protected]> 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
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?
On Sat, Jul 5, 2008 at 7:56 PM, Linus Torvalds
<[email protected]> wrote:
> On Sat, 5 Jul 2008, Vegard Nossum wrote:
>> On Sat, Jul 5, 2008 at 1:33 PM, Jan Engelhardt <[email protected]> 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
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
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."
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 <[email protected]> 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?
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 <[email protected]> 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/
On Fri, 4 Jul 2008 20:03:51 -0600 Matthew Wilcox <[email protected]> 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 <[email protected]>
>
> 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.
On Tue, 2008-07-22 at 03:05 -0700, Andrew Morton wrote:
> On Fri, 4 Jul 2008 20:03:51 -0600 Matthew Wilcox <[email protected]> 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 <[email protected]>
> >
> > 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
On Tue, 22 Jul 2008 20:36:35 +1000 Michael Ellerman <[email protected]> wrote:
> On Tue, 2008-07-22 at 03:05 -0700, Andrew Morton wrote:
> > On Fri, 4 Jul 2008 20:03:51 -0600 Matthew Wilcox <[email protected]> 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 <[email protected]>
> > >
> > > 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.
>
> 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.
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.