| Message ID | 19000.55666.866148.559620@cargo.ozlabs.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
* Paul Mackerras <paulus@samba.org> wrote: > +++ b/tools/perf/Makefile > @@ -157,9 +157,21 @@ uname_R := $(shell sh -c 'uname -r 2>/dev/null || echo not') > uname_P := $(shell sh -c 'uname -p 2>/dev/null || echo not') > uname_V := $(shell sh -c 'uname -v 2>/dev/null || echo not') > > +# If we're on a 64-bit kernel, use -m64 > +ifneq ($(patsubst %64,%,$(uname_M)),$(uname_M)) > + M64 := -m64 > +endif this is fine. > +# Don't use -Werror on ppc64; we get warnings due to using > +# %Lx formats on __u64, which is unsigned long. > +Werror := -Werror > +ifeq ($(uname_M),ppc64) > + Werror := > +endif hm, i dont really like this one - it just adds a special case on an arch. Why is __u64 unsigned long on powerpc and not unsigned long long? I thought the whole mess with u64 was fixed there recently and powerpc too now uses include/asm-generic/int-ll64.h ? ah, it does this: /* * This is here because we used to use l64 for 64bit powerpc * and we don't want to impact user mode with our change to ll64 * in the kernel. */ #if defined(__powerpc64__) && !defined(__KERNEL__) # include <asm-generic/int-l64.h> #else # include <asm-generic/int-ll64.h> #endif That's crappy really. Ingo
Ingo Molnar writes: > ah, it does this: > > /* > * This is here because we used to use l64 for 64bit powerpc > * and we don't want to impact user mode with our change to ll64 > * in the kernel. > */ > #if defined(__powerpc64__) && !defined(__KERNEL__) > # include <asm-generic/int-l64.h> > #else > # include <asm-generic/int-ll64.h> > #endif > > That's crappy really. We were concerned that changing the userland-visible type of __u64 from unsigned long to unsigned long long, etc., would be breaking the ABI, even if only in a small way - I thought it could possibly change C++ mangled function names, for instance, and it would cause fresh compile warnings on existing user code that prints __u64 with %lx, which has always been the correct thing to do on ppc64. A counter-argument would be, I guess, that __u64 et al. are purely for use in describing the kernel/user interface, so we have a little more latitude than with the type of e.g. u_int64_t. I dunno. I don't recall getting much of an answer from the glibc guys about what they thought of the idea of changing it. Anyway, of the 64-bit architectures, alpha, ia64, and mips64 also have __u64 as unsigned long in userspace, so this issue will still crop up even if we change it on powerpc. Paul.
* Paul Mackerras <paulus@samba.org> wrote: > Ingo Molnar writes: > > > ah, it does this: > > > > /* > > * This is here because we used to use l64 for 64bit powerpc > > * and we don't want to impact user mode with our change to ll64 > > * in the kernel. > > */ > > #if defined(__powerpc64__) && !defined(__KERNEL__) > > # include <asm-generic/int-l64.h> > > #else > > # include <asm-generic/int-ll64.h> > > #endif > > > > That's crappy really. > > We were concerned that changing the userland-visible type of __u64 > from unsigned long to unsigned long long, etc., would be breaking > the ABI, even if only in a small way - I thought it could possibly > change C++ mangled function names, for instance, and it would > cause fresh compile warnings on existing user code that prints > __u64 with %lx, which has always been the correct thing to do on > ppc64. > > A counter-argument would be, I guess, that __u64 et al. are purely > for use in describing the kernel/user interface, so we have a > little more latitude than with the type of e.g. u_int64_t. I > dunno. I don't recall getting much of an answer from the glibc > guys about what they thought of the idea of changing it. > > Anyway, of the 64-bit architectures, alpha, ia64, and mips64 also > have __u64 as unsigned long in userspace, so this issue will still > crop up even if we change it on powerpc. Having crap elsewhere is no reason to spread it further really. We need consistent types. Can we define __KERNEL__ perhaps to get to the real types? Ingo
* Paul Mackerras <paulus@samba.org> wrote: > This also removes the -Werror flag when building on a 64-bit powerpc > machine. The userspace definition of u64 is unsigned long rather > than unsigned long long, meaning that gcc warns every time a u64 > is printed with %Lx or %llx (though that does work properly). > In future we may use PRI64 etc. for printing 64-bit quantities, > which would eliminate these warnings. > +# Don't use -Werror on ppc64; we get warnings due to using > +# %Lx formats on __u64, which is unsigned long. > +Werror := -Werror > +ifeq ($(uname_M),ppc64) > + Werror := > +endif Note, i left out this bit from the commit - we need to find a better solution than to allow ugly warnings on PowerPC. Could we use the kernel's u64 type directly perhaps? That would allow us to change all __u64 to u64 in all of tools/perf/ which is a nice clean-up in any case. Ingo
Ingo Molnar writes: > Note, i left out this bit from the commit - we need to find a better > solution than to allow ugly warnings on PowerPC. > > Could we use the kernel's u64 type directly perhaps? That would > allow us to change all __u64 to u64 in all of tools/perf/ which is a > nice clean-up in any case. This is userspace, we can use "u64" however we like. :) I'll cook up a patch to add "typedef unsigned long long u64" and change __u64 to u64. Paul.
* Paul Mackerras <paulus@samba.org> wrote: > Ingo Molnar writes: > > > Note, i left out this bit from the commit - we need to find a > > better solution than to allow ugly warnings on PowerPC. > > > > Could we use the kernel's u64 type directly perhaps? That would > > allow us to change all __u64 to u64 in all of tools/perf/ which > > is a nice clean-up in any case. > > This is userspace, we can use "u64" however we like. :) I'll cook > up a patch to add "typedef unsigned long long u64" and change > __u64 to u64. Thanks, i've applied the patch. Note, it crossed with a few changes from Peter - i fixed up the conflicts - please double check whether it's still fine on PowerPC too. Ingo
diff --git a/tools/perf/Makefile b/tools/perf/Makefile index e8346f9..eddd61a 100644 --- a/tools/perf/Makefile +++ b/tools/perf/Makefile @@ -157,9 +157,21 @@ uname_R := $(shell sh -c 'uname -r 2>/dev/null || echo not') uname_P := $(shell sh -c 'uname -p 2>/dev/null || echo not') uname_V := $(shell sh -c 'uname -v 2>/dev/null || echo not') +# If we're on a 64-bit kernel, use -m64 +ifneq ($(patsubst %64,%,$(uname_M)),$(uname_M)) + M64 := -m64 +endif + +# Don't use -Werror on ppc64; we get warnings due to using +# %Lx formats on __u64, which is unsigned long. +Werror := -Werror +ifeq ($(uname_M),ppc64) + Werror := +endif + # CFLAGS and LDFLAGS are for the users to override from the command line. -CFLAGS = -ggdb3 -Wall -Werror -Wstrict-prototypes -Wmissing-declarations -Wmissing-prototypes -std=gnu99 -Wdeclaration-after-statement -O6 +CFLAGS = $(M64) -ggdb3 -Wall $(Werror) -Wstrict-prototypes -Wmissing-declarations -Wmissing-prototypes -std=gnu99 -Wdeclaration-after-statement -O6 LDFLAGS = -lpthread -lrt -lelf -lm ALL_CFLAGS = $(CFLAGS) ALL_LDFLAGS = $(LDFLAGS)
On 64-bit powerpc, perf needs to be built as a 64-bit executable. This arranges to add the -m64 flag to CFLAGS if we are running on a 64-bit machine, indicated by the result of uname -m ending in "64". This means that we'll use -m64 on x86_64 machines as well. This also removes the -Werror flag when building on a 64-bit powerpc machine. The userspace definition of u64 is unsigned long rather than unsigned long long, meaning that gcc warns every time a u64 is printed with %Lx or %llx (though that does work properly). In future we may use PRI64 etc. for printing 64-bit quantities, which would eliminate these warnings. Signed-off-by: Paul Mackerras <paulus@samba.org> --- tools/perf/Makefile | 14 +++++++++++++- 1 files changed, 13 insertions(+), 1 deletions(-)