linux-tools/perf: Respect TARGET_CFLAGS
diff mbox series

Message ID 20190418093902.41835-1-abrodkin@synopsys.com
State Rejected
Headers show
Series
  • linux-tools/perf: Respect TARGET_CFLAGS
Related show

Commit Message

Alexey Brodkin April 18, 2019, 9:39 a.m. UTC
perf uses its own build-system and doesn't respect externally set flags.
Let's force it to use target flags as perf utility is nothing but
an ordinary user-space application.

This among other things is required to build perf for ARC700 for which
we pass "-matomic" via TARGET_ABI, see [1] for more details.

[1] https://patchwork.ozlabs.org/patch/1087471/

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
---
 package/linux-tools/linux-tool-perf.mk.in | 1 +
 1 file changed, 1 insertion(+)

Comments

Thomas Petazzoni April 18, 2019, 9:43 a.m. UTC | #1
Hello,

On Thu, 18 Apr 2019 12:39:02 +0300
Alexey Brodkin <Alexey.Brodkin@synopsys.com> wrote:

> perf uses its own build-system and doesn't respect externally set flags.
> Let's force it to use target flags as perf utility is nothing but
> an ordinary user-space application.
> 
> This among other things is required to build perf for ARC700 for which
> we pass "-matomic" via TARGET_ABI, see [1] for more details.
> 
> [1] https://patchwork.ozlabs.org/patch/1087471/
> 
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> ---
>  package/linux-tools/linux-tool-perf.mk.in | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/package/linux-tools/linux-tool-perf.mk.in b/package/linux-tools/linux-tool-perf.mk.in
> index 80e00c3c56..e7565773c4 100644
> --- a/package/linux-tools/linux-tool-perf.mk.in
> +++ b/package/linux-tools/linux-tool-perf.mk.in
> @@ -19,6 +19,7 @@ PERF_MAKE_FLAGS = \
>  	JOBS=$(PARALLEL_JOBS) \
>  	ARCH=$(PERF_ARCH) \
>  	DESTDIR=$(TARGET_DIR) \
> +	EXTRA_CFLAGS="$(TARGET_CFLAGS)" \

The change is OK, but I believe that if -matomic is really a
core/important CFLAGS on ARC, then we should ideally pass it through
the toolchain wrapper, so that we are sure it is always passed.

Arnout, what do you think?

Best regards,

Thomas
Arnout Vandecappelle April 21, 2019, 7:36 a.m. UTC | #2
On 18/04/2019 11:43, Thomas Petazzoni wrote:
> Hello,
> 
> On Thu, 18 Apr 2019 12:39:02 +0300
> Alexey Brodkin <Alexey.Brodkin@synopsys.com> wrote:
> 
>> perf uses its own build-system and doesn't respect externally set flags.
>> Let's force it to use target flags as perf utility is nothing but
>> an ordinary user-space application.
>>
>> This among other things is required to build perf for ARC700 for which
>> we pass "-matomic" via TARGET_ABI, see [1] for more details.
>>
>> [1] https://patchwork.ozlabs.org/patch/1087471/
>>
>> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
>> ---
>>  package/linux-tools/linux-tool-perf.mk.in | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/package/linux-tools/linux-tool-perf.mk.in b/package/linux-tools/linux-tool-perf.mk.in
>> index 80e00c3c56..e7565773c4 100644
>> --- a/package/linux-tools/linux-tool-perf.mk.in
>> +++ b/package/linux-tools/linux-tool-perf.mk.in
>> @@ -19,6 +19,7 @@ PERF_MAKE_FLAGS = \
>>  	JOBS=$(PARALLEL_JOBS) \
>>  	ARCH=$(PERF_ARCH) \
>>  	DESTDIR=$(TARGET_DIR) \
>> +	EXTRA_CFLAGS="$(TARGET_CFLAGS)" \
> 
> The change is OK, but I believe that if -matomic is really a
> core/important CFLAGS on ARC, then we should ideally pass it through
> the toolchain wrapper, so that we are sure it is always passed.
> 
> Arnout, what do you think?

 +1.

 Thomas, last year we made a beginning of deciding what should go through the
wrapper and what through CFLAGS, but I'm not sure if we ever got to a conclusion?

 Regards,
 Arnout
Alexey Brodkin April 26, 2019, 6:30 a.m. UTC | #3
Hi Arnout, Thomas,

> -----Original Message-----
> From: Arnout Vandecappelle <arnout@mind.be>
> Sent: Sunday, April 21, 2019 10:36 AM
> To: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; Alexey Brodkin <abrodkin@synopsys.com>
> Cc: buildroot@busybox.net; ARC Buildroot mailing list <arc-buildroot@synopsys.com>
> Subject: Re: [Buildroot] [PATCH] linux-tools/perf: Respect TARGET_CFLAGS
> 
> 
> 
> On 18/04/2019 11:43, Thomas Petazzoni wrote:
> > Hello,
> >
> > On Thu, 18 Apr 2019 12:39:02 +0300
> > Alexey Brodkin <Alexey.Brodkin@synopsys.com> wrote:
> >
> >> perf uses its own build-system and doesn't respect externally set flags.
> >> Let's force it to use target flags as perf utility is nothing but
> >> an ordinary user-space application.
> >>
> >> This among other things is required to build perf for ARC700 for which
> >> we pass "-matomic" via TARGET_ABI, see [1] for more details.
> >>
> >> [1] https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__patchwork.ozlabs.org_patch_1087471_&d=DwICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=lqdeeSSEes0GFDDl656eViXO7b
> reS55ytWkhpk5R81I&m=eB4KQfsAGH4782y3faOnCUr3nb3xgbJBuMv3P9P6cxY&s=g7hFDq2PCWQQNFtfEGkCnU8PyqSm-NS-
> FzrkT4QMA8c&e=
> >>
> >> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> >> ---
> >>  package/linux-tools/linux-tool-perf.mk.in | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/package/linux-tools/linux-tool-perf.mk.in b/package/linux-tools/linux-tool-perf.mk.in
> >> index 80e00c3c56..e7565773c4 100644
> >> --- a/package/linux-tools/linux-tool-perf.mk.in
> >> +++ b/package/linux-tools/linux-tool-perf.mk.in
> >> @@ -19,6 +19,7 @@ PERF_MAKE_FLAGS = \
> >>  	JOBS=$(PARALLEL_JOBS) \
> >>  	ARCH=$(PERF_ARCH) \
> >>  	DESTDIR=$(TARGET_DIR) \
> >> +	EXTRA_CFLAGS="$(TARGET_CFLAGS)" \
> >
> > The change is OK, but I believe that if -matomic is really a
> > core/important CFLAGS on ARC, then we should ideally pass it through
> > the toolchain wrapper, so that we are sure it is always passed.
> >
> > Arnout, what do you think?
> 
>  +1.
> 
>  Thomas, last year we made a beginning of deciding what should go through the
> wrapper and what through CFLAGS, but I'm not sure if we ever got to a conclusion?

So are we going to wait for some infrastructure changes or
shall we first fix the problem at hand with broken perf build to ARC700?

-Alexey
Arnout Vandecappelle April 27, 2019, 5:39 p.m. UTC | #4
On 26/04/2019 08:30, Alexey Brodkin wrote:
> Hi Arnout, Thomas,
> 
>> -----Original Message-----
>> From: Arnout Vandecappelle <arnout@mind.be>
>> Sent: Sunday, April 21, 2019 10:36 AM
>> To: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; Alexey Brodkin <abrodkin@synopsys.com>
>> Cc: buildroot@busybox.net; ARC Buildroot mailing list <arc-buildroot@synopsys.com>
>> Subject: Re: [Buildroot] [PATCH] linux-tools/perf: Respect TARGET_CFLAGS
>>
>>
>>
>> On 18/04/2019 11:43, Thomas Petazzoni wrote:
>>> Hello,
>>>
>>> On Thu, 18 Apr 2019 12:39:02 +0300
>>> Alexey Brodkin <Alexey.Brodkin@synopsys.com> wrote:
>>>
>>>> perf uses its own build-system and doesn't respect externally set flags.
>>>> Let's force it to use target flags as perf utility is nothing but
>>>> an ordinary user-space application.
>>>>
>>>> This among other things is required to build perf for ARC700 for which
>>>> we pass "-matomic" via TARGET_ABI, see [1] for more details.
>>>>
>>>> [1] https://urldefense.proofpoint.com/v2/url?u=https-
>> 3A__patchwork.ozlabs.org_patch_1087471_&d=DwICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=lqdeeSSEes0GFDDl656eViXO7b
>> reS55ytWkhpk5R81I&m=eB4KQfsAGH4782y3faOnCUr3nb3xgbJBuMv3P9P6cxY&s=g7hFDq2PCWQQNFtfEGkCnU8PyqSm-NS-
>> FzrkT4QMA8c&e=
>>>>
>>>> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
>>>> ---
>>>>  package/linux-tools/linux-tool-perf.mk.in | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/package/linux-tools/linux-tool-perf.mk.in b/package/linux-tools/linux-tool-perf.mk.in
>>>> index 80e00c3c56..e7565773c4 100644
>>>> --- a/package/linux-tools/linux-tool-perf.mk.in
>>>> +++ b/package/linux-tools/linux-tool-perf.mk.in
>>>> @@ -19,6 +19,7 @@ PERF_MAKE_FLAGS = \
>>>>  	JOBS=$(PARALLEL_JOBS) \
>>>>  	ARCH=$(PERF_ARCH) \
>>>>  	DESTDIR=$(TARGET_DIR) \
>>>> +	EXTRA_CFLAGS="$(TARGET_CFLAGS)" \
>>>
>>> The change is OK, but I believe that if -matomic is really a
>>> core/important CFLAGS on ARC, then we should ideally pass it through
>>> the toolchain wrapper, so that we are sure it is always passed.
>>>
>>> Arnout, what do you think?
>>
>>  +1.
>>
>>  Thomas, last year we made a beginning of deciding what should go through the
>> wrapper and what through CFLAGS, but I'm not sure if we ever got to a conclusion?
> 
> So are we going to wait for some infrastructure changes or
> shall we first fix the problem at hand with broken perf build to ARC700?

 It's not really infrastructure changes. Rather, it is a better way to fix the
issue.

 You just need to move the -matomic from TARGET_ABI to TOOLCHAIN_WRAPPER_OPTS.

 The only tricky part is where that should be done. I think arch/arch.mk.arc
would be the right place. But that indeed requires an infrastructural change,
because the TOOLCHAIN_WRAPPER_OPTS = ... in toolchain-wrapper.mk has to change
into a +=.

 Regards,
 Arnout


 Regards,
 Arnout
Alexey Brodkin May 8, 2019, 1:19 p.m. UTC | #5
Hi Yann, all,

> -----Original Message-----
> From: Yann E. MORIN <yann.morin.1998@gmail.com> On Behalf Of Yann E. MORIN
> Sent: Saturday, April 27, 2019 8:56 PM
> To: Arnout Vandecappelle <arnout@mind.be>
> Cc: Alexey Brodkin <abrodkin@synopsys.com>; Thomas Petazzoni <thomas.petazzoni@bootlin.com>;
> buildroot@busybox.net; ARC Buildroot mailing list <arc-buildroot@synopsys.com>
> Subject: Re: [Buildroot] [PATCH] linux-tools/perf: Respect TARGET_CFLAGS
> 
> Arnout, Alexey, All,
> 
> On 2019-04-27 19:39 +0200, Arnout Vandecappelle spake thusly:
> > On 26/04/2019 08:30, Alexey Brodkin wrote:
> [--SNIP--]
> > >>> The change is OK, but I believe that if -matomic is really a
> > >>> core/important CFLAGS on ARC, then we should ideally pass it through
> > >>> the toolchain wrapper, so that we are sure it is always passed.
> > >>> Arnout, what do you think?
> > >>  +1.
> > >>  Thomas, last year we made a beginning of deciding what should go through the
> > >> wrapper and what through CFLAGS, but I'm not sure if we ever got to a conclusion?
> > > So are we going to wait for some infrastructure changes or
> > > shall we first fix the problem at hand with broken perf build to ARC700?
> >
> >  It's not really infrastructure changes. Rather, it is a better way to fix the
> > issue.
> >
> >  You just need to move the -matomic from TARGET_ABI to TOOLCHAIN_WRAPPER_OPTS.
> >
> >  The only tricky part is where that should be done. I think arch/arch.mk.arc
> > would be the right place. But that indeed requires an infrastructural change,
> > because the TOOLCHAIN_WRAPPER_OPTS = ... in toolchain-wrapper.mk has to change
> > into a +=.
> 
> I'm not too fond of this, but what about the following:
> 
> diff --git a/arch/arch.mk.arc b/arch/arch.mk.arc
> new file mode 100644
> index 0000000000..83503841b6
> --- /dev/null
> +++ b/arch/arch.mk.arc
> @@ -0,0 +1,3 @@
> +ifeq ($(BR2_arcle),y)
> +ARCH_TOOLCHAIN_WRAPPER_OPTS = -matomic
> +endif
> diff --git a/toolchain/toolchain-wrapper.mk
> b/toolchain/toolchain-wrapper.mk
> index ca66fa7ba4..8f872506ec 100644
> --- a/toolchain/toolchain-wrapper.mk
> +++ b/toolchain/toolchain-wrapper.mk
> @@ -17,6 +17,7 @@ TOOLCHAIN_WRAPPER_ARGS =
> $($(PKG)_TOOLCHAIN_WRAPPER_ARGS)
>  TOOLCHAIN_WRAPPER_ARGS += -DBR_SYSROOT='"$(STAGING_SUBDIR)"'
> 
>  TOOLCHAIN_WRAPPER_OPTS = \
> +   $(ARCH_TOOLCHAIN_WRAPPER_OPTS) \
>     $(call qstrip,$(BR2_SSP_OPTION)) \
>     $(call qstrip,$(BR2_TARGET_OPTIMIZATION))

That looks good to me with only one change: prefer generic "BR2_arc"
instead of little-endian only "BR2_arcle".

Care to send-out a patch?

-Alexey
Yann E. MORIN May 8, 2019, 2:31 p.m. UTC | #6
Alexey, All,

On 2019-05-08 13:19 +0000, Alexey Brodkin spake thusly:
> From: Yann E. MORIN <yann.morin.1998@gmail.com> On Behalf Of Yann E. MORIN
[--SNIP--]
> > I'm not too fond of this, but what about the following:
> > 
> > diff --git a/arch/arch.mk.arc b/arch/arch.mk.arc
> > new file mode 100644
> > index 0000000000..83503841b6
> > --- /dev/null
> > +++ b/arch/arch.mk.arc
> > @@ -0,0 +1,3 @@
> > +ifeq ($(BR2_arcle),y)
> > +ARCH_TOOLCHAIN_WRAPPER_OPTS = -matomic
> > +endif
> > diff --git a/toolchain/toolchain-wrapper.mk
> > b/toolchain/toolchain-wrapper.mk
> > index ca66fa7ba4..8f872506ec 100644
> > --- a/toolchain/toolchain-wrapper.mk
> > +++ b/toolchain/toolchain-wrapper.mk
> > @@ -17,6 +17,7 @@ TOOLCHAIN_WRAPPER_ARGS =
> > $($(PKG)_TOOLCHAIN_WRAPPER_ARGS)
> >  TOOLCHAIN_WRAPPER_ARGS += -DBR_SYSROOT='"$(STAGING_SUBDIR)"'
> > 
> >  TOOLCHAIN_WRAPPER_OPTS = \
> > +   $(ARCH_TOOLCHAIN_WRAPPER_OPTS) \
> >     $(call qstrip,$(BR2_SSP_OPTION)) \
> >     $(call qstrip,$(BR2_TARGET_OPTIMIZATION))
> 
> That looks good to me with only one change: prefer generic "BR2_arc"
> instead of little-endian only "BR2_arcle".

Yes, of course, it was just a quicky to demo what I was thinking...

> Care to send-out a patch?

Aw... But *you* are the arc guy! ;-)

OK, will do.

Regards,
Yann E. MORIN.
Alexey Brodkin May 8, 2019, 2:54 p.m. UTC | #7
Hi Yann,

> -----Original Message-----
> From: Yann E. MORIN <yann.morin.1998@free.fr>
> Sent: Wednesday, May 8, 2019 5:31 PM
> To: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; buildroot@busybox.net; ARC Buildroot mailing list
> <arc-buildroot@synopsys.com>; Arnout Vandecappelle <arnout@mind.be>
> Subject: Re: [arc-buildroot] [Buildroot] [PATCH] linux-tools/perf: Respect TARGET_CFLAGS
> 
> Alexey, All,
> 
> On 2019-05-08 13:19 +0000, Alexey Brodkin spake thusly:
> > From: Yann E. MORIN <yann.morin.1998@gmail.com> On Behalf Of Yann E. MORIN
> [--SNIP--]
> > > I'm not too fond of this, but what about the following:
> > >
> > > diff --git a/arch/arch.mk.arc b/arch/arch.mk.arc
> > > new file mode 100644
> > > index 0000000000..83503841b6
> > > --- /dev/null
> > > +++ b/arch/arch.mk.arc
> > > @@ -0,0 +1,3 @@
> > > +ifeq ($(BR2_arcle),y)
> > > +ARCH_TOOLCHAIN_WRAPPER_OPTS = -matomic
> > > +endif
> > > diff --git a/toolchain/toolchain-wrapper.mk
> > > b/toolchain/toolchain-wrapper.mk
> > > index ca66fa7ba4..8f872506ec 100644
> > > --- a/toolchain/toolchain-wrapper.mk
> > > +++ b/toolchain/toolchain-wrapper.mk
> > > @@ -17,6 +17,7 @@ TOOLCHAIN_WRAPPER_ARGS =
> > > $($(PKG)_TOOLCHAIN_WRAPPER_ARGS)
> > >  TOOLCHAIN_WRAPPER_ARGS += -DBR_SYSROOT='"$(STAGING_SUBDIR)"'
> > >
> > >  TOOLCHAIN_WRAPPER_OPTS = \
> > > +   $(ARCH_TOOLCHAIN_WRAPPER_OPTS) \
> > >     $(call qstrip,$(BR2_SSP_OPTION)) \
> > >     $(call qstrip,$(BR2_TARGET_OPTIMIZATION))
> >
> > That looks good to me with only one change: prefer generic "BR2_arc"
> > instead of little-endian only "BR2_arcle".
> 
> Yes, of course, it was just a quicky to demo what I was thinking...
> 
> > Care to send-out a patch?
> 
> Aw... But *you* are the arc guy! ;-)
> 
> OK, will do.

I'm all for sending it myself but it will be like I'm stealing
your nice idea - nothing more that that.

Want me to do that instead?

-Alexey
Yann E. MORIN May 9, 2019, 6:12 p.m. UTC | #8
Alexey, All,

On 2019-05-08 14:54 +0000, Alexey Brodkin spake thusly:
[--SNIP--]
> > > Care to send-out a patch?
> > Aw... But *you* are the arc guy! ;-)
> > OK, will do.
> 
> I'm all for sending it myself but it will be like I'm stealing
> your nice idea - nothing more that that.
> Want me to do that instead?

I've done the patches localy already, I just need a bit of time to test
then and send the series.

I shall not promise anything, but maybe in the evening/night.

BTW, are all ARC cores impacted, or only the 7x0d ?

Regards,
Yann E. MORIN.
Yann E. MORIN July 18, 2019, 8:58 p.m. UTC | #9
Alexey, All,

On 2019-04-18 12:39 +0300, Alexey Brodkin spake thusly:
> perf uses its own build-system and doesn't respect externally set flags.
> Let's force it to use target flags as perf utility is nothing but
> an ordinary user-space application.
> 
> This among other things is required to build perf for ARC700 for which
> we pass "-matomic" via TARGET_ABI, see [1] for more details.

As was discussed in the thread, the proper option is now passed in via
the wrapper and arch-specific CFLAGS:
    https://git.buildroot.org/buildroot/commit/?id=4281288d2d568d5ce748a0e66e0872f54fbe956c
    https://git.buildroot.org/buildroot/commit/?id=c568b4f37fa6d7f51e6d14d33d7eb75dfe26d7bf

Thanks!

Regards,
Yann E. MORIN.

> [1] https://patchwork.ozlabs.org/patch/1087471/
> 
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> ---
>  package/linux-tools/linux-tool-perf.mk.in | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/package/linux-tools/linux-tool-perf.mk.in b/package/linux-tools/linux-tool-perf.mk.in
> index 80e00c3c56..e7565773c4 100644
> --- a/package/linux-tools/linux-tool-perf.mk.in
> +++ b/package/linux-tools/linux-tool-perf.mk.in
> @@ -19,6 +19,7 @@ PERF_MAKE_FLAGS = \
>  	JOBS=$(PARALLEL_JOBS) \
>  	ARCH=$(PERF_ARCH) \
>  	DESTDIR=$(TARGET_DIR) \
> +	EXTRA_CFLAGS="$(TARGET_CFLAGS)" \
>  	prefix=/usr \
>  	WERROR=0 \
>  	NO_LIBAUDIT=1 \
> -- 
> 2.16.2
> 
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

Patch
diff mbox series

diff --git a/package/linux-tools/linux-tool-perf.mk.in b/package/linux-tools/linux-tool-perf.mk.in
index 80e00c3c56..e7565773c4 100644
--- a/package/linux-tools/linux-tool-perf.mk.in
+++ b/package/linux-tools/linux-tool-perf.mk.in
@@ -19,6 +19,7 @@  PERF_MAKE_FLAGS = \
 	JOBS=$(PARALLEL_JOBS) \
 	ARCH=$(PERF_ARCH) \
 	DESTDIR=$(TARGET_DIR) \
+	EXTRA_CFLAGS="$(TARGET_CFLAGS)" \
 	prefix=/usr \
 	WERROR=0 \
 	NO_LIBAUDIT=1 \