Patchwork [5/6] Fix sysprof for powerpc64

login
register
mail settings
Submitter Matthew McClintock
Date Oct. 4, 2011, 10:03 p.m.
Message ID <1317765787-19127-5-git-send-email-msm@freescale.com>
Download mbox | patch
Permalink /patch/12759/
State New, archived
Headers show

Comments

Matthew McClintock - Oct. 4, 2011, 10:03 p.m.
sysprof will not build properly without this defined

Signed-off-by: Matthew McClintock <msm@freescale.com>
---
No comments on original patch sent

 meta/recipes-kernel/sysprof/sysprof_git.bb |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
Khem Raj - Oct. 5, 2011, 12:05 a.m.
On 10/4/2011 3:03 PM, Matthew McClintock wrote:
> sysprof will not build properly without this defined
>
> Signed-off-by: Matthew McClintock<msm@freescale.com>
> ---
> No comments on original patch sent
>
>   meta/recipes-kernel/sysprof/sysprof_git.bb |    2 ++
>   1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/meta/recipes-kernel/sysprof/sysprof_git.bb b/meta/recipes-kernel/sysprof/sysprof_git.bb
> index 10bde04..271b5d8 100644
> --- a/meta/recipes-kernel/sysprof/sysprof_git.bb
> +++ b/meta/recipes-kernel/sysprof/sysprof_git.bb
> @@ -16,6 +16,8 @@ SRC_URI_append_arm  = " file://rmb-arm.patch"
>   SRC_URI_append_mips = " file://rmb-mips.patch"
>   SRC_URI_append_powerpc = " file://ppc-macro-fix.patch"
>
> +export CFLAGS_append_powerpc64 = " -D__ppc64__"
> +

this is a gcc built-in define I wonder why you need to add it explicitly

>   S = "${WORKDIR}/git"
>
>   inherit autotools
McClintock Matthew-B29882 - Oct. 5, 2011, 6:44 a.m.
On Tue, Oct 4, 2011 at 7:05 PM, Khem Raj <raj.khem@gmail.com> wrote:
>> diff --git a/meta/recipes-kernel/sysprof/sysprof_git.bb
>> b/meta/recipes-kernel/sysprof/sysprof_git.bb
>> index 10bde04..271b5d8 100644
>> --- a/meta/recipes-kernel/sysprof/sysprof_git.bb
>> +++ b/meta/recipes-kernel/sysprof/sysprof_git.bb
>> @@ -16,6 +16,8 @@ SRC_URI_append_arm  = " file://rmb-arm.patch"
>>  SRC_URI_append_mips = " file://rmb-mips.patch"
>>  SRC_URI_append_powerpc = " file://ppc-macro-fix.patch"
>>
>> +export CFLAGS_append_powerpc64 = " -D__ppc64__"
>> +
>
> this is a gcc built-in define I wonder why you need to add it explicitly

The file in question is compiled like:

powerpc64-fsl-linux-gcc     -m64 -mcpu=e5500
--sysroot=/local/home/mattsm/git/poky/build_p5020ds-64b_release/tmp/sysroots/p5020ds-64b
-DHAVE_CONFIG_H -I.
-I/local/home/mattsm/git/poky/build_p5020ds-64b_release/tmp/sysroots/p5020ds-64b/usr/include/glib-2.0
-I/local/home/mattsm/git/poky/build_p5020ds-64b_release/tmp/sysroots/p5020ds-64b/usr/lib64/glib-2.0/include
    -O2 -pipe -g -feliminate-unused-debug-types -Wall -MT
sysprof_cli-collector.o -MD -MP -MF .deps/sysprof_cli-collector.Tpo -c
-o sysprof_cli-collector.o `test -f 'collector.c' || echo
'./'`collector.c

Without the -D__ppc64__ it gives the following:

collector.c: In function ‘sysprof_perf_counter_open’:
collector.c:193:21: error: ‘__NR_perf_counter_open’ undeclared (first
use in this function)
collector.c:193:21: note: each undeclared identifier is reported only
once for each function it appears in
collector.c: In function ‘process_event’:
collector.c:654:11: warning: variable ‘name’ set but not used
[-Wunused-but-set-variable]
collector.c: In function ‘sysprof_perf_counter_open’:
collector.c:194:1: warning: control reaches end of non-void function
[-Wreturn-type]

Looking at collector.c it's very simple:

#ifndef __NR_perf_counter_open
#if defined(__i386__)
#define __NR_perf_counter_open 336
#elif defined(__x86_64__)
#define __NR_perf_counter_open 298
[...snip...]
#elif defined(__hppa__)
#define __NR_perf_counter_open 318
#elif defined(__ppc__) || defined(__ppc64__)
#define __NR_perf_counter_open 319
#elif defined(__s390__)
[...snip...]
#endif
#endif

Am I missing something obvious?

-M
Richard Purdie - Oct. 5, 2011, 11:35 a.m.
On Tue, 2011-10-04 at 17:05 -0700, Khem Raj wrote:
> On 10/4/2011 3:03 PM, Matthew McClintock wrote:
> > sysprof will not build properly without this defined
> >
> > Signed-off-by: Matthew McClintock<msm@freescale.com>
> > ---
> > No comments on original patch sent
> >
> >   meta/recipes-kernel/sysprof/sysprof_git.bb |    2 ++
> >   1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/meta/recipes-kernel/sysprof/sysprof_git.bb b/meta/recipes-kernel/sysprof/sysprof_git.bb
> > index 10bde04..271b5d8 100644
> > --- a/meta/recipes-kernel/sysprof/sysprof_git.bb
> > +++ b/meta/recipes-kernel/sysprof/sysprof_git.bb
> > @@ -16,6 +16,8 @@ SRC_URI_append_arm  = " file://rmb-arm.patch"
> >   SRC_URI_append_mips = " file://rmb-mips.patch"
> >   SRC_URI_append_powerpc = " file://ppc-macro-fix.patch"
> >
> > +export CFLAGS_append_powerpc64 = " -D__ppc64__"
> > +
> 
> this is a gcc built-in define I wonder why you need to add it explicitly

Are you sure? Is the built-in define not __powerpc64__ ?

Cheers,

Richard
Khem Raj - Oct. 5, 2011, 4:46 p.m.
On Wed, Oct 5, 2011 at 4:35 AM, Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
> On Tue, 2011-10-04 at 17:05 -0700, Khem Raj wrote:
>> On 10/4/2011 3:03 PM, Matthew McClintock wrote:
>> > sysprof will not build properly without this defined
>> >
>> > Signed-off-by: Matthew McClintock<msm@freescale.com>
>> > ---
>> > No comments on original patch sent
>> >
>> >   meta/recipes-kernel/sysprof/sysprof_git.bb |    2 ++
>> >   1 files changed, 2 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/meta/recipes-kernel/sysprof/sysprof_git.bb b/meta/recipes-kernel/sysprof/sysprof_git.bb
>> > index 10bde04..271b5d8 100644
>> > --- a/meta/recipes-kernel/sysprof/sysprof_git.bb
>> > +++ b/meta/recipes-kernel/sysprof/sysprof_git.bb
>> > @@ -16,6 +16,8 @@ SRC_URI_append_arm  = " file://rmb-arm.patch"
>> >   SRC_URI_append_mips = " file://rmb-mips.patch"
>> >   SRC_URI_append_powerpc = " file://ppc-macro-fix.patch"
>> >
>> > +export CFLAGS_append_powerpc64 = " -D__ppc64__"
>> > +
>>
>> this is a gcc built-in define I wonder why you need to add it explicitly
>
> Are you sure? Is the built-in define not __powerpc64__ ?

yes that one too is a builtin define. I don't have the OE/ppc64 build
around to explore more

>
> Cheers,
>
> Richard
>
>
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
>
McClintock Matthew-B29882 - Oct. 5, 2011, 4:48 p.m.
On Wed, Oct 5, 2011 at 6:35 AM, Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
>> this is a gcc built-in define I wonder why you need to add it explicitly
>
> Are you sure? Is the built-in define not __powerpc64__ ?

Further investigation:

[mattsm@right build_p5020ds-64b_release (testing $)]$
./tmp/sysroots/x86_64-linux/usr/bin/ppc64e5500-fsl-linux/powerpc64-fsl-linux-gcc
-c -E -dM empty.c | grep __p
#define __powerpc64__ 1
#define __powerpc__ 1
[mattsm@right build_p5020ds-64b_release (testing $)]$

-M
Khem Raj - Oct. 5, 2011, 5:06 p.m.
On Wed, Oct 5, 2011 at 9:48 AM, McClintock Matthew-B29882
<B29882@freescale.com> wrote:
> On Wed, Oct 5, 2011 at 6:35 AM, Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
>>> this is a gcc built-in define I wonder why you need to add it explicitly
>>
>> Are you sure? Is the built-in define not __powerpc64__ ?
>
> Further investigation:
>
> [mattsm@right build_p5020ds-64b_release (testing $)]$
> ./tmp/sysroots/x86_64-linux/usr/bin/ppc64e5500-fsl-linux/powerpc64-fsl-linux-gcc
> -c -E -dM empty.c | grep __p
> #define __powerpc64__ 1
> #define __powerpc__ 1
> [mattsm@right build_p5020ds-64b_release (testing $)]$

yes I was trying it with a differently configured gcc. For linux gcc
defines __powerpc64__
and for darwin it has __ppc64__ as builtin define.

I think adding check for __powerpc64__ and __powerpc__
would be nice and can be submitted to sysprof upstream IMO

>
> -M
>
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
>
Saul Wold - Oct. 5, 2011, 5:10 p.m.
On 10/05/2011 10:06 AM, Khem Raj wrote:
> On Wed, Oct 5, 2011 at 9:48 AM, McClintock Matthew-B29882
> <B29882@freescale.com>  wrote:
>> On Wed, Oct 5, 2011 at 6:35 AM, Richard Purdie
>> <richard.purdie@linuxfoundation.org>  wrote:
>>>> this is a gcc built-in define I wonder why you need to add it explicitly
>>>
>>> Are you sure? Is the built-in define not __powerpc64__ ?
>>
>> Further investigation:
>>
>> [mattsm@right build_p5020ds-64b_release (testing $)]$
>> ./tmp/sysroots/x86_64-linux/usr/bin/ppc64e5500-fsl-linux/powerpc64-fsl-linux-gcc
>> -c -E -dM empty.c | grep __p
>> #define __powerpc64__ 1
>> #define __powerpc__ 1
>> [mattsm@right build_p5020ds-64b_release (testing $)]$
>
> yes I was trying it with a differently configured gcc. For linux gcc
> defines __powerpc64__
> and for darwin it has __ppc64__ as builtin define.
>
> I think adding check for __powerpc64__ and __powerpc__
> would be nice and can be submitted to sysprof upstream IMO
>
I think this would be a good fix to have in the upstream sysprof and it 
would remove the CFLAGS_append from the recipe.

Sau!

>>
>> -M
>>
>> _______________________________________________
>> Openembedded-core mailing list
>> Openembedded-core@lists.openembedded.org
>> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
>>
>
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
>
Phil Blundell - Oct. 5, 2011, 6:30 p.m.
On Wed, 2011-10-05 at 12:35 +0100, Richard Purdie wrote:
> On Tue, 2011-10-04 at 17:05 -0700, Khem Raj wrote:
> > On 10/4/2011 3:03 PM, Matthew McClintock wrote:
> > > sysprof will not build properly without this defined
> > >
> > > Signed-off-by: Matthew McClintock<msm@freescale.com>
> > > ---
> > > No comments on original patch sent
> > >
> > >   meta/recipes-kernel/sysprof/sysprof_git.bb |    2 ++
> > >   1 files changed, 2 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/meta/recipes-kernel/sysprof/sysprof_git.bb b/meta/recipes-kernel/sysprof/sysprof_git.bb
> > > index 10bde04..271b5d8 100644
> > > --- a/meta/recipes-kernel/sysprof/sysprof_git.bb
> > > +++ b/meta/recipes-kernel/sysprof/sysprof_git.bb
> > > @@ -16,6 +16,8 @@ SRC_URI_append_arm  = " file://rmb-arm.patch"
> > >   SRC_URI_append_mips = " file://rmb-mips.patch"
> > >   SRC_URI_append_powerpc = " file://ppc-macro-fix.patch"
> > >
> > > +export CFLAGS_append_powerpc64 = " -D__ppc64__"
> > > +
> > 
> > this is a gcc built-in define I wonder why you need to add it explicitly
> 
> Are you sure? Is the built-in define not __powerpc64__ ?

Yes, __powerpc64__ is the generic one.  I think __ppc64__ is only a
predefine on Darwin.

p.
McClintock Matthew-B29882 - Oct. 5, 2011, 6:41 p.m.
On Wed, Oct 5, 2011 at 1:30 PM, Phil Blundell <philb@gnu.org> wrote:
> On Wed, 2011-10-05 at 12:35 +0100, Richard Purdie wrote:
>> On Tue, 2011-10-04 at 17:05 -0700, Khem Raj wrote:
>> > On 10/4/2011 3:03 PM, Matthew McClintock wrote:
>> > > sysprof will not build properly without this defined
>> > >
>> > > Signed-off-by: Matthew McClintock<msm@freescale.com>
>> > > ---
>> > > No comments on original patch sent
>> > >
>> > >   meta/recipes-kernel/sysprof/sysprof_git.bb |    2 ++
>> > >   1 files changed, 2 insertions(+), 0 deletions(-)
>> > >
>> > > diff --git a/meta/recipes-kernel/sysprof/sysprof_git.bb b/meta/recipes-kernel/sysprof/sysprof_git.bb
>> > > index 10bde04..271b5d8 100644
>> > > --- a/meta/recipes-kernel/sysprof/sysprof_git.bb
>> > > +++ b/meta/recipes-kernel/sysprof/sysprof_git.bb
>> > > @@ -16,6 +16,8 @@ SRC_URI_append_arm  = " file://rmb-arm.patch"
>> > >   SRC_URI_append_mips = " file://rmb-mips.patch"
>> > >   SRC_URI_append_powerpc = " file://ppc-macro-fix.patch"
>> > >
>> > > +export CFLAGS_append_powerpc64 = " -D__ppc64__"
>> > > +
>> >
>> > this is a gcc built-in define I wonder why you need to add it explicitly
>>
>> Are you sure? Is the built-in define not __powerpc64__ ?
>
> Yes, __powerpc64__ is the generic one.  I think __ppc64__ is only a
> predefine on Darwin.

This seems to be recently fixed upstream. I will just add a patch for
this, it's not even in 1.1.8 which we just updated too in oe-core.

-M
Khem Raj - Oct. 5, 2011, 7:13 p.m.
On Wed, Oct 5, 2011 at 11:30 AM, Phil Blundell <philb@gnu.org> wrote:
> On Wed, 2011-10-05 at 12:35 +0100, Richard Purdie wrote:
>> On Tue, 2011-10-04 at 17:05 -0700, Khem Raj wrote:
>> > On 10/4/2011 3:03 PM, Matthew McClintock wrote:
>> > > sysprof will not build properly without this defined
>> > >
>> > > Signed-off-by: Matthew McClintock<msm@freescale.com>
>> > > ---
>> > > No comments on original patch sent
>> > >
>> > >   meta/recipes-kernel/sysprof/sysprof_git.bb |    2 ++
>> > >   1 files changed, 2 insertions(+), 0 deletions(-)
>> > >
>> > > diff --git a/meta/recipes-kernel/sysprof/sysprof_git.bb b/meta/recipes-kernel/sysprof/sysprof_git.bb
>> > > index 10bde04..271b5d8 100644
>> > > --- a/meta/recipes-kernel/sysprof/sysprof_git.bb
>> > > +++ b/meta/recipes-kernel/sysprof/sysprof_git.bb
>> > > @@ -16,6 +16,8 @@ SRC_URI_append_arm  = " file://rmb-arm.patch"
>> > >   SRC_URI_append_mips = " file://rmb-mips.patch"
>> > >   SRC_URI_append_powerpc = " file://ppc-macro-fix.patch"
>> > >
>> > > +export CFLAGS_append_powerpc64 = " -D__ppc64__"
>> > > +
>> >
>> > this is a gcc built-in define I wonder why you need to add it explicitly
>>
>> Are you sure? Is the built-in define not __powerpc64__ ?
>
> Yes, __powerpc64__ is the generic one.  I think __ppc64__ is only a
> predefine on Darwin.

__powerpc64__ is not generic. Its Linux specific.

>
> p.
>
>
>
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
>

Patch

diff --git a/meta/recipes-kernel/sysprof/sysprof_git.bb b/meta/recipes-kernel/sysprof/sysprof_git.bb
index 10bde04..271b5d8 100644
--- a/meta/recipes-kernel/sysprof/sysprof_git.bb
+++ b/meta/recipes-kernel/sysprof/sysprof_git.bb
@@ -16,6 +16,8 @@  SRC_URI_append_arm  = " file://rmb-arm.patch"
 SRC_URI_append_mips = " file://rmb-mips.patch"
 SRC_URI_append_powerpc = " file://ppc-macro-fix.patch"
 
+export CFLAGS_append_powerpc64 = " -D__ppc64__"
+
 S = "${WORKDIR}/git"
 
 inherit autotools