| Message ID | 20130119013052.GA24693@us.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Fri, Jan 18, 2013 at 05:30:52PM -0800, Sukadev Bhattiprolu wrote: > From 4d266e5040c33103f5d226b0d16b89f8ef79e3ad Mon Sep 17 00:00:00 2001 > From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> > Date: Fri, 18 Jan 2013 11:14:28 -0800 > Subject: [PATCH] perf: Fix compile warnings in tests/attr.c > > Replace '%llu' in printf()s with 'PRIu64' in 'tools/perf/tests/attr.c' > to fix compile warnings (which become errors due to -Werror). i386 and x86_64 compiles fine for me with gcc versions 4.6.3-2 and 4.7.2-2 with your patch for x86_64 I'm getting following warnings/errors: CC tests/attr.o tests/attr.c: In function ‘store_event’: tests/attr.c:69:4: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 6 has type ‘__u64’ [-Werror=format] tests/attr.c:69:4: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 6 has type ‘__u64’ [-Werror=format] tests/attr.c:78:7: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘__u64’ [-Werror=format] tests/attr.c:94:2: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘__u64’ [-Werror=format] tests/attr.c:94:2: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘__u64’ [-Werror=format] tests/attr.c:95:2: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘__u64’ [-Werror=format] tests/attr.c:95:2: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘__u64’ [-Werror=format] tests/attr.c:96:2: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘__u64’ [-Werror=format] tests/attr.c:96:2: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘__u64’ [-Werror=format] tests/attr.c:97:2: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘__u64’ [-Werror=format] tests/attr.c:97:2: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘__u64’ [-Werror=format] tests/attr.c:122:2: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘__u64’ [-Werror=format] tests/attr.c:122:2: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘__u64’ [-Werror=format] tests/attr.c:123:2: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘__u64’ [-Werror=format] tests/attr.c:123:2: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘__u64’ [-Werror=format] tests/attr.c:124:2: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘__u64’ [-Werror=format] tests/attr.c:124:2: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘__u64’ [-Werror=format] tests/attr.c:125:2: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘__u64’ [-Werror=format] tests/attr.c:125:2: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘__u64’ [-Werror=format] cc1: all warnings being treated as errors make: *** [tests/attr.o] Error 1 i386 compiles fine jirka
Jiri Olsa [jolsa@redhat.com] wrote: | On Fri, Jan 18, 2013 at 05:30:52PM -0800, Sukadev Bhattiprolu wrote: | > From 4d266e5040c33103f5d226b0d16b89f8ef79e3ad Mon Sep 17 00:00:00 2001 | > From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> | > Date: Fri, 18 Jan 2013 11:14:28 -0800 | > Subject: [PATCH] perf: Fix compile warnings in tests/attr.c | > | > Replace '%llu' in printf()s with 'PRIu64' in 'tools/perf/tests/attr.c' | > to fix compile warnings (which become errors due to -Werror). | | i386 and x86_64 compiles fine for me with gcc versions 4.6.3-2 and 4.7.2-2 But is broken on Power for 64bit :-( I am trying to fix that and thought that use of format specifiers like 'PRIu64' was the way to go. | | with your patch for x86_64 I'm getting following warnings/errors: | | CC tests/attr.o | tests/attr.c: In function ‘store_event’: | tests/attr.c:69:4: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 6 has type ‘__u64’ [-Werror=format] Here is what I see on an x86_64 box, RHEL6.2 box: $ rpm -qf /usr/include/linux/types.h kernel-headers-2.6.32-220.4.2.el6.x86_64 $ cat foo.c #include <linux/types.h> $ cc -Werror -Wall foo.c In file included from /usr/include/asm-generic/types.h:7, from /usr/include/asm/types.h:6, from /usr/include/linux/types.h:4, from foo5.c:1: /usr/include/asm-generic/int-ll64.h:31:2: error: #error __u64 defined as unsigned long long where the #error is my debug message. <snip> | make: *** [tests/attr.o] Error 1 | | i386 compiles fine __u64 is 'unsigned long long' on x86 and PRIu64 is 'llu' which is fine. __u64 is 'unsigned long' on Power and PRIu64 is 'lu' which is again fine. But __u64 is 'unsigned long long' on x86_64, but PRIu64 is '%lu' bc __WORDSIZE is 64. On x86_64, shouldn't __u64, be defined as 'unsigned long' rather than 'unsigned long long' - ie include 'int-l64.h' rather than 'int-ll64.h' ? BTW, does 'perf' with my patch compile, (with warnings) for you on x86_64 with 'WERROR=0 make' ? Sukadev
On Mon, Jan 21, 2013 at 01:38:23PM -0800, Sukadev Bhattiprolu wrote: > Jiri Olsa [jolsa@redhat.com] wrote: > | On Fri, Jan 18, 2013 at 05:30:52PM -0800, Sukadev Bhattiprolu wrote: SNIP > __u64 is 'unsigned long long' on x86 and PRIu64 is 'llu' which is fine. > > __u64 is 'unsigned long' on Power and PRIu64 is 'lu' which is again fine. > > But __u64 is 'unsigned long long' on x86_64, but PRIu64 is '%lu' bc __WORDSIZE > is 64. > > On x86_64, shouldn't __u64, be defined as 'unsigned long' rather than > 'unsigned long long' - ie include 'int-l64.h' rather than 'int-ll64.h' ? hum, not sure ;-) will try to find some time to look on that > > BTW, does 'perf' with my patch compile, (with warnings) for you on x86_64 > with 'WERROR=0 make' ? this one passes with warnings jirka
On Mon, 2013-01-21 at 13:38 -0800, Sukadev Bhattiprolu wrote: > Jiri Olsa [jolsa@redhat.com] wrote: > | On Fri, Jan 18, 2013 at 05:30:52PM -0800, Sukadev Bhattiprolu wrote: > | > From 4d266e5040c33103f5d226b0d16b89f8ef79e3ad Mon Sep 17 00:00:00 2001 > | > From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> > | > Date: Fri, 18 Jan 2013 11:14:28 -0800 > | > Subject: [PATCH] perf: Fix compile warnings in tests/attr.c > | > > | > Replace '%llu' in printf()s with 'PRIu64' in 'tools/perf/tests/attr.c' > | > to fix compile warnings (which become errors due to -Werror). > | > | i386 and x86_64 compiles fine for me with gcc versions 4.6.3-2 and 4.7.2-2 > > But is broken on Power for 64bit :-( I am trying to fix that and thought > that use of format specifiers like 'PRIu64' was the way to go. > > | > | with your patch for x86_64 I'm getting following warnings/errors: > > | > | CC tests/attr.o > | tests/attr.c: In function ‘store_event’: > | tests/attr.c:69:4: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 6 has type ‘__u64’ [-Werror=format] > > Here is what I see on an x86_64 box, RHEL6.2 box: > > $ rpm -qf /usr/include/linux/types.h > kernel-headers-2.6.32-220.4.2.el6.x86_64 > > $ cat foo.c > #include <linux/types.h> > > $ cc -Werror -Wall foo.c > In file included from /usr/include/asm-generic/types.h:7, > from /usr/include/asm/types.h:6, > from /usr/include/linux/types.h:4, > from foo5.c:1: > /usr/include/asm-generic/int-ll64.h:31:2: error: #error __u64 defined as unsigned long long > > where the #error is my debug message. > > <snip> > > | make: *** [tests/attr.o] Error 1 > | > | i386 compiles fine > > __u64 is 'unsigned long long' on x86 and PRIu64 is 'llu' which is fine. > > __u64 is 'unsigned long' on Power and PRIu64 is 'lu' which is again fine. > > But __u64 is 'unsigned long long' on x86_64, but PRIu64 is '%lu' bc __WORDSIZE > is 64. This is a bit of a mess, but let me see if I can help explain it. The root of the problem is that you're mixing up the kernel type __u64, with the userspace format specifier PRIu64. PRIu64 is the format specifier for printing a uint64_t, it _may_ also be the right specifier for a __u64, but there's no guarantee of that - as you have discovered. Inside the kernel both x86 and powerpc use unsigned long long always, in 32-bit and 64-bit code. That means in the kernel we can always use %llu. On x86 that definition is also exported to userspace, so on x86 __u64 is always unsigned long long. As you noticed this potentially differs from uint64_t, which can be confusing. However it means in x86 userspace code you can always print a __u64 with %llu. On powerpc we default to using definitions that match userspace, so __u64 changes depending on your wordsize, and so you must use PRIu64 etc. to print them. There is however support in recent powerpc kernels to switch to using unsigned long long even on 64-bit. See commit 2c9c6ce. You need to define __SANE_USERSPACE_TYPES__ before including types.h. Then you can always use %llu to print __u64. cheers
Michael Ellerman [michael@ellerman.id.au] wrote: | > | make: *** [tests/attr.o] Error 1 | > | | > | i386 compiles fine | > | > __u64 is 'unsigned long long' on x86 and PRIu64 is 'llu' which is fine. | > | > __u64 is 'unsigned long' on Power and PRIu64 is 'lu' which is again fine. | > | > But __u64 is 'unsigned long long' on x86_64, but PRIu64 is '%lu' bc __WORDSIZE | > is 64. | | | This is a bit of a mess, but let me see if I can help explain it. Yes it is :-) thanks for explaining it. | | The root of the problem is that you're mixing up the kernel type __u64, | with the userspace format specifier PRIu64. struct perf_event_attr is shared with user space and is using __u64. Should it use uint64_t instead ? | | PRIu64 is the format specifier for printing a uint64_t, it _may_ also be | the right specifier for a __u64, but there's no guarantee of that - as | you have discovered. | | Inside the kernel both x86 and powerpc use unsigned long long always, in | 32-bit and 64-bit code. That means in the kernel we can always use %llu. | | On x86 that definition is also exported to userspace, so on x86 __u64 is | always unsigned long long. As you noticed this potentially differs from | uint64_t, which can be confusing. However it means in x86 userspace code | you can always print a __u64 with %llu. | | On powerpc we default to using definitions that match userspace, so | __u64 changes depending on your wordsize, and so you must use PRIu64 | etc. to print them. Well, using __u64 and PRIu64 seems breaks x86-64... | | There is however support in recent powerpc kernels to switch to using | unsigned long long even on 64-bit. See commit 2c9c6ce. | | You need to define __SANE_USERSPACE_TYPES__ before including types.h. | Then you can always use %llu to print __u64. but __SANE_USERSPACE_TYPES__ with __u64 and %llu seems to work on x86, x86-64, powerpc. Will modify my patch to add __SANE_USERSPACE_TYPES__ but leave the %llu as is. Sukadev
On Wed, 2013-01-23 at 10:57 -0800, Sukadev Bhattiprolu wrote: > Michael Ellerman [michael@ellerman.id.au] wrote: > | > | make: *** [tests/attr.o] Error 1 > | > | > | > | i386 compiles fine > | > > | > __u64 is 'unsigned long long' on x86 and PRIu64 is 'llu' which is fine. > | > > | > __u64 is 'unsigned long' on Power and PRIu64 is 'lu' which is again fine. > | > > | > But __u64 is 'unsigned long long' on x86_64, but PRIu64 is '%lu' bc __WORDSIZE > | > is 64. > | > | > | This is a bit of a mess, but let me see if I can help explain it. > > Yes it is :-) thanks for explaining it. > > | > | The root of the problem is that you're mixing up the kernel type __u64, > | with the userspace format specifier PRIu64. > > struct perf_event_attr is shared with user space and is using __u64. Should > it use uint64_t instead ? Absolutely not. That's the whole reason we have __u64, it's so we can define it in the kernel but share it with userspace, and not have it conflict with any userspace types. > | PRIu64 is the format specifier for printing a uint64_t, it _may_ also be > | the right specifier for a __u64, but there's no guarantee of that - as > | you have discovered. > | > | Inside the kernel both x86 and powerpc use unsigned long long always, in > | 32-bit and 64-bit code. That means in the kernel we can always use %llu. > | > | On x86 that definition is also exported to userspace, so on x86 __u64 is > | always unsigned long long. As you noticed this potentially differs from > | uint64_t, which can be confusing. However it means in x86 userspace code > | you can always print a __u64 with %llu. > | > | On powerpc we default to using definitions that match userspace, so > | __u64 changes depending on your wordsize, and so you must use PRIu64 > | etc. to print them. > > Well, using __u64 and PRIu64 seems breaks x86-64... Yes, see the previous paragraph. > | There is however support in recent powerpc kernels to switch to using > | unsigned long long even on 64-bit. See commit 2c9c6ce. > | > | You need to define __SANE_USERSPACE_TYPES__ before including types.h. > | Then you can always use %llu to print __u64. > > but __SANE_USERSPACE_TYPES__ with __u64 and %llu seems to work on x86, > x86-64, powerpc. > Will modify my patch to add __SANE_USERSPACE_TYPES__ but leave the %llu > as is. Right. It will only work with newer kernel headers, but that's basically unsolvable. cheers
diff --git a/tools/perf/tests/attr.c b/tools/perf/tests/attr.c index f61dd3f..5cbb2b3 100644 --- a/tools/perf/tests/attr.c +++ b/tools/perf/tests/attr.c @@ -65,7 +65,7 @@ static int store_event(struct perf_event_attr *attr, pid_t pid, int cpu, FILE *file; char path[PATH_MAX]; - snprintf(path, PATH_MAX, "%s/event-%d-%llu-%d", dir, + snprintf(path, PATH_MAX, "%s/event-%d-%" PRIu64 "-%d", dir, attr->type, attr->config, fd); file = fopen(path, "w+"); @@ -74,7 +74,7 @@ static int store_event(struct perf_event_attr *attr, pid_t pid, int cpu, return -1; } - if (fprintf(file, "[event-%d-%llu-%d]\n", + if (fprintf(file, "[event-%d-%" PRIu64 "-%d]\n", attr->type, attr->config, fd) < 0) { perror("test attr - failed to write event file"); fclose(file); @@ -91,10 +91,10 @@ static int store_event(struct perf_event_attr *attr, pid_t pid, int cpu, /* struct perf_event_attr */ WRITE_ASS(type, PRIu32); WRITE_ASS(size, PRIu32); - WRITE_ASS(config, "llu"); - WRITE_ASS(sample_period, "llu"); - WRITE_ASS(sample_type, "llu"); - WRITE_ASS(read_format, "llu"); + WRITE_ASS(config, PRIu64); + WRITE_ASS(sample_period, PRIu64); + WRITE_ASS(sample_type, PRIu64); + WRITE_ASS(read_format, PRIu64); WRITE_ASS(disabled, "d"); WRITE_ASS(inherit, "d"); WRITE_ASS(pinned, "d"); @@ -119,10 +119,10 @@ static int store_event(struct perf_event_attr *attr, pid_t pid, int cpu, WRITE_ASS(exclude_callchain_user, "d"); WRITE_ASS(wakeup_events, PRIu32); WRITE_ASS(bp_type, PRIu32); - WRITE_ASS(config1, "llu"); - WRITE_ASS(config2, "llu"); - WRITE_ASS(branch_sample_type, "llu"); - WRITE_ASS(sample_regs_user, "llu"); + WRITE_ASS(config1, PRIu64); + WRITE_ASS(config2, PRIu64); + WRITE_ASS(branch_sample_type, PRIu64); + WRITE_ASS(sample_regs_user, PRIu64); WRITE_ASS(sample_stack_user, PRIu32); fclose(file);