[PATCH] perf: Fix compile warnings in tests/attr.c

[PATCH] perf: Fix compile warnings in tests/attr.c

[Sukadev Bhattiprolu]

>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).

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
---
 tools/perf/tests/attr.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

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);
-- 
1.7.1

Re: [PATCH] perf: Fix compile warnings in tests/attr.c

[Jiri Olsa]

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

Re: [PATCH] perf: Fix compile warnings in tests/attr.c

[Sukadev Bhattiprolu]

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
| >=20
| > Replace '%llu' in printf()s with 'PRIu64' in 'tools/perf/tests/attr.c'
| > to fix compile warnings (which become errors due to -Werror).
|=20
| 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.

|=20
| with your patch for x86_64 I'm getting following warnings/errors:

|=20
|     CC tests/attr.o
| tests/attr.c: In function =E2=80=98store_event=E2=80=99:
| tests/attr.c:69:4: error: format =E2=80=98%lu=E2=80=99 expects argument o=
f type =E2=80=98long unsigned int=E2=80=99, but argument 6 has type =E2=80=
=98__u64=E2=80=99 [-Werror=3Dformat]

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 u=
nsigned long long

where the #error is my debug message.

<snip>

| make: *** [tests/attr.o] Error 1
|=20
| 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 __WORDS=
IZE
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=3D0 make' ?

Sukadev

Re: [PATCH] perf: Fix compile warnings in tests/attr.c

[Jiri Olsa]

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

Re: [PATCH] perf: Fix compile warnings in tests/attr.c

[Michael Ellerman]

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

Re: [PATCH] perf: Fix compile warnings in tests/attr.c

[Sukadev Bhattiprolu]

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

Re: [PATCH] perf: Fix compile warnings in tests/attr.c

[Michael Ellerman]

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