perf: lift TARGET_CC_ARCH modification out of security_flags.inc

From: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Building perf without security_flags.inc being included in one's
distro results in the buildpaths warning

WARNING: perf-1.0-r9 do_package_qa: QA Issue: File /usr/bin/trace in
package perf contains reference to TMPDIR

because the ${DEBUG_PREFIX_MAP} does not get used. Most recipes get
that from CFLAGS, but the perf recipe explicitly unsets that.

Now ${SELECTED_OPTIMIZATION} of course contains more than just
${DEBUG_FLAGS}/${DEBUG_PREFIX_MAP}. For most TUs, perf's build system
adds its own optimization flags (-O6 for odd reasons), so for those
including the -O2 or -Og doesn't change anything. But looking at the
.o.cmd files show that there are some TUs which currently get built
without any -O flag. So for those adding the distro's
SELECTED_OPTIMIZATION seem to be the right thing to do.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 meta/conf/distro/include/security_flags.inc | 1 -
 meta/recipes-kernel/perf/perf.bb            | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

On Thu, Oct 19, 2023 at 8:32 AM Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> From: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>
> Building perf without security_flags.inc being included in one's
> distro results in the buildpaths warning
>
> WARNING: perf-1.0-r9 do_package_qa: QA Issue: File /usr/bin/trace in
> package perf contains reference to TMPDIR
>
> because the ${DEBUG_PREFIX_MAP} does not get used. Most recipes get
> that from CFLAGS, but the perf recipe explicitly unsets that.
>
> Now ${SELECTED_OPTIMIZATION} of course contains more than just
> ${DEBUG_FLAGS}/${DEBUG_PREFIX_MAP}. For most TUs, perf's build system
> adds its own optimization flags (-O6 for odd reasons), so for those
> including the -O2 or -Og doesn't change anything. But looking at the
> .o.cmd files show that there are some TUs which currently get built
> without any -O flag. So for those adding the distro's
> SELECTED_OPTIMIZATION seem to be the right thing to do.

The change is fine with me.

>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  meta/conf/distro/include/security_flags.inc | 1 -
>  meta/recipes-kernel/perf/perf.bb            | 1 +
>  2 files changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/meta/conf/distro/include/security_flags.inc b/meta/conf/distro/include/security_flags.inc
> index 2972f05b4e..d97a6edb0f 100644
> --- a/meta/conf/distro/include/security_flags.inc
> +++ b/meta/conf/distro/include/security_flags.inc
> @@ -69,4 +69,3 @@ SECURITY_LDFLAGS:pn-xserver-xorg = "${SECURITY_X_LDFLAGS}"
>  TARGET_CC_ARCH:append:pn-binutils = " ${SELECTED_OPTIMIZATION}"
>  TARGET_CC_ARCH:append:pn-gcc = " ${SELECTED_OPTIMIZATION}"
>  TARGET_CC_ARCH:append:pn-gdb = " ${SELECTED_OPTIMIZATION}"
> -TARGET_CC_ARCH:append:pn-perf = " ${SELECTED_OPTIMIZATION}"
> diff --git a/meta/recipes-kernel/perf/perf.bb b/meta/recipes-kernel/perf/perf.bb
> index 675acfaf26..a6110dedc9 100644
> --- a/meta/recipes-kernel/perf/perf.bb
> +++ b/meta/recipes-kernel/perf/perf.bb
> @@ -72,6 +72,7 @@ SPDX_S = "${S}/tools/perf"
>  # symbols and this is preferred than requiring patches to every old
>  # supported kernel.
>  LDFLAGS="-ldl -lutil"
> +TARGET_CC_ARCH += "${SELECTED_OPTIMIZATION}"

I'd even go so far as to add a comment above the relocated line with a
hint / reminder that this adds the debug prefix map.

Bruce

>
>  EXTRA_OEMAKE = '\
>      V=1 \
> --
> 2.40.1.1.g1c60b9335d
>

On Thu, 2023-10-19 at 14:32 +0200, Rasmus Villemoes via
lists.openembedded.org wrote:
> From: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> 
> Building perf without security_flags.inc being included in one's
> distro results in the buildpaths warning
> 
> WARNING: perf-1.0-r9 do_package_qa: QA Issue: File /usr/bin/trace in
> package perf contains reference to TMPDIR
> 
> because the ${DEBUG_PREFIX_MAP} does not get used. Most recipes get
> that from CFLAGS, but the perf recipe explicitly unsets that.
> 
> Now ${SELECTED_OPTIMIZATION} of course contains more than just
> ${DEBUG_FLAGS}/${DEBUG_PREFIX_MAP}. For most TUs, perf's build system
> adds its own optimization flags (-O6 for odd reasons), so for those
> including the -O2 or -Og doesn't change anything. But looking at the
> .o.cmd files show that there are some TUs which currently get built
> without any -O flag. So for those adding the distro's
> SELECTED_OPTIMIZATION seem to be the right thing to do.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  meta/conf/distro/include/security_flags.inc | 1 -
>  meta/recipes-kernel/perf/perf.bb            | 1 +
>  2 files changed, 1 insertion(+), 1 deletion(-)

The fact this works suggests perf is ignoring TARGET_CFLAGS. Is there
anything in the perf build system where we should be passing in cflags
we really want used?

It feels like the fix is still a little fragile and we should find out
how to get it to use our flags. The security_flags hack really needs to
not be needed.

Cheers,

Richard

On 19/10/2023 14.48, Richard Purdie wrote:
> On Thu, 2023-10-19 at 14:32 +0200, Rasmus Villemoes via
> lists.openembedded.org wrote:
>> From: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>
>> Building perf without security_flags.inc being included in one's
>> distro results in the buildpaths warning
>>
>> WARNING: perf-1.0-r9 do_package_qa: QA Issue: File /usr/bin/trace in
>> package perf contains reference to TMPDIR
>>
>> because the ${DEBUG_PREFIX_MAP} does not get used. Most recipes get
>> that from CFLAGS, but the perf recipe explicitly unsets that.
>>
>> Now ${SELECTED_OPTIMIZATION} of course contains more than just
>> ${DEBUG_FLAGS}/${DEBUG_PREFIX_MAP}. For most TUs, perf's build system
>> adds its own optimization flags (-O6 for odd reasons), so for those
>> including the -O2 or -Og doesn't change anything. But looking at the
>> .o.cmd files show that there are some TUs which currently get built
>> without any -O flag. So for those adding the distro's
>> SELECTED_OPTIMIZATION seem to be the right thing to do.
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> ---
>>  meta/conf/distro/include/security_flags.inc | 1 -
>>  meta/recipes-kernel/perf/perf.bb            | 1 +
>>  2 files changed, 1 insertion(+), 1 deletion(-)
> 
> The fact this works suggests perf is ignoring TARGET_CFLAGS. Is there
> anything in the perf build system where we should be passing in cflags
> we really want used?

Well, IIUC, the normal way TARGET_CFLAGS would take effect is via the
'export CFLAGS = "${TARGET_CFLAGS}'. But in the perf recipe, both
do_compile and do_install start with

        # Linux kernel build system is expected to do the right thing
        unset CFLAGS

And perf's build system does indeed add lots of flags of its own, at
least for most TUs, but nowhere is any -f(macro/debug/file)-prefix being
set.

So I do think that TARGET_CC_ARCH is the best place for flags that we
really want used. At least as an initial step, because this is known to
work when using the security_flags.inc file. Maybe there's some cleaner
way where we don't unset CFLAGS, but that could be a giant can of worms.

And yes, a comment should be added. Is something like

# Perf's build system adds its own optimization flags for most TUs,
# overriding the flags included here. But for some, perf does not add
# any -O option, so ensure the distro's chosen optimization gets used
# for those. Since ${SELECTED_OPTIMIZATION} always includes
# ${DEBUG_FLAGS} which in turn includes ${DEBUG_PREFIX_MAP}, this also
# ensures perf is built with appropriate -f*-prefix-map options,
# avoiding the 'buildpaths' QA warning.

too verbose?

Rasmus

On Fri, 2023-10-20 at 10:10 +0200, Rasmus Villemoes wrote:
> On 19/10/2023 14.48, Richard Purdie wrote:
> > On Thu, 2023-10-19 at 14:32 +0200, Rasmus Villemoes via
> > lists.openembedded.org wrote:
> > > From: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > > 
> > > Building perf without security_flags.inc being included in one's
> > > distro results in the buildpaths warning
> > > 
> > > WARNING: perf-1.0-r9 do_package_qa: QA Issue: File /usr/bin/trace in
> > > package perf contains reference to TMPDIR
> > > 
> > > because the ${DEBUG_PREFIX_MAP} does not get used. Most recipes get
> > > that from CFLAGS, but the perf recipe explicitly unsets that.
> > > 
> > > Now ${SELECTED_OPTIMIZATION} of course contains more than just
> > > ${DEBUG_FLAGS}/${DEBUG_PREFIX_MAP}. For most TUs, perf's build system
> > > adds its own optimization flags (-O6 for odd reasons), so for those
> > > including the -O2 or -Og doesn't change anything. But looking at the
> > > .o.cmd files show that there are some TUs which currently get built
> > > without any -O flag. So for those adding the distro's
> > > SELECTED_OPTIMIZATION seem to be the right thing to do.
> > > 
> > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > > ---
> > >  meta/conf/distro/include/security_flags.inc | 1 -
> > >  meta/recipes-kernel/perf/perf.bb            | 1 +
> > >  2 files changed, 1 insertion(+), 1 deletion(-)
> > 
> > The fact this works suggests perf is ignoring TARGET_CFLAGS. Is there
> > anything in the perf build system where we should be passing in cflags
> > we really want used?
> 
> Well, IIUC, the normal way TARGET_CFLAGS would take effect is via the
> 'export CFLAGS = "${TARGET_CFLAGS}'. But in the perf recipe, both
> do_compile and do_install start with
> 
>         # Linux kernel build system is expected to do the right thing
>         unset CFLAGS
> 
> And perf's build system does indeed add lots of flags of its own, at
> least for most TUs, but nowhere is any -f(macro/debug/file)-prefix being
> set.
> 
> So I do think that TARGET_CC_ARCH is the best place for flags that we
> really want used. At least as an initial step, because this is known to
> work when using the security_flags.inc file. Maybe there's some cleaner
> way where we don't unset CFLAGS, but that could be a giant can of worms.
> 
> And yes, a comment should be added. Is something like
> 
> # Perf's build system adds its own optimization flags for most TUs,
> # overriding the flags included here. But for some, perf does not add
> # any -O option, so ensure the distro's chosen optimization gets used
> # for those. Since ${SELECTED_OPTIMIZATION} always includes
> # ${DEBUG_FLAGS} which in turn includes ${DEBUG_PREFIX_MAP}, this also
> # ensures perf is built with appropriate -f*-prefix-map options,
> # avoiding the 'buildpaths' QA warning.
> 
> too verbose?

If it were me I'd probably write:

# The perf makefile has "unset CFLAGS" as "Linux kernel build system is
# expected to do the right thing". It doesn't and we need our prefix 
# map options and security flags amongst other things so force our 
# cflags in.

Cheers,

Richard

On 20/10/2023 11.38, Richard Purdie wrote:
> On Fri, 2023-10-20 at 10:10 +0200, Rasmus Villemoes wrote:
>> On 19/10/2023 14.48, Richard Purdie wrote:

>>> The fact this works suggests perf is ignoring TARGET_CFLAGS. Is there
>>> anything in the perf build system where we should be passing in cflags
>>> we really want used?
>>
>> Well, IIUC, the normal way TARGET_CFLAGS would take effect is via the
>> 'export CFLAGS = "${TARGET_CFLAGS}'. But in the perf recipe, both
>> do_compile and do_install start with
>>
>>         # Linux kernel build system is expected to do the right thing
>>         unset CFLAGS
>>
>> And perf's build system does indeed add lots of flags of its own, at
>> least for most TUs, but nowhere is any -f(macro/debug/file)-prefix being
>> set.
>>
>> So I do think that TARGET_CC_ARCH is the best place for flags that we
>> really want used. At least as an initial step, because this is known to
>> work when using the security_flags.inc file. Maybe there's some cleaner
>> way where we don't unset CFLAGS, but that could be a giant can of worms.
>>
>> And yes, a comment should be added. Is something like
>>
>> # Perf's build system adds its own optimization flags for most TUs,
>> # overriding the flags included here. But for some, perf does not add
>> # any -O option, so ensure the distro's chosen optimization gets used
>> # for those. Since ${SELECTED_OPTIMIZATION} always includes
>> # ${DEBUG_FLAGS} which in turn includes ${DEBUG_PREFIX_MAP}, this also
>> # ensures perf is built with appropriate -f*-prefix-map options,
>> # avoiding the 'buildpaths' QA warning.
>>
>> too verbose?
> 
> If it were me I'd probably write:
> 
> # The perf makefile has "unset CFLAGS" as "Linux kernel build system is
> # expected to do the right thing". 

It's not the perf makefile, it's right there in our recipe itself. I
don't think upstream perf is to blame.

> It doesn't and we need our prefix 
> # map options 

Yes.

> and security flags amongst other things so force our 
> # cflags in.

This has nothing to do with the security flags. That file still contains

TARGET_CC_ARCH:append:class-target = " ${SECURITY_CFLAGS}"

and TARGET_CC_ARCH is included in the CC value we pass to perf's make,
with or without this patch. So if the distro includes
security_flags.inc, perf gets built with those flags as expected.

But when that file is _not_ included, we do miss the prefix-map flags,
which should be passed regardless of any security distro settings. I
think the line I'm moving out of security_flags.inc have never really
had any business being there in the first place, regardless of whether
adding ${SELECTED_OPTIMIZATION} to TARGET_CC_ARCH is the most
appropriate method to get those prefix-map options passed. But since
that's how the poky distro has achieved that until now, it seems to be
the least invasive and least risky fix for distros that don't use
security_flags.inc.

Rasmus

On Fri, 2023-10-20 at 12:03 +0200, Rasmus Villemoes wrote:
> On 20/10/2023 11.38, Richard Purdie wrote:
> > On Fri, 2023-10-20 at 10:10 +0200, Rasmus Villemoes wrote:
> > > On 19/10/2023 14.48, Richard Purdie wrote:
> 
> > > > The fact this works suggests perf is ignoring TARGET_CFLAGS. Is there
> > > > anything in the perf build system where we should be passing in cflags
> > > > we really want used?
> > > 
> > > Well, IIUC, the normal way TARGET_CFLAGS would take effect is via the
> > > 'export CFLAGS = "${TARGET_CFLAGS}'. But in the perf recipe, both
> > > do_compile and do_install start with
> > > 
> > >         # Linux kernel build system is expected to do the right thing
> > >         unset CFLAGS
> > > 
> > > And perf's build system does indeed add lots of flags of its own, at
> > > least for most TUs, but nowhere is any -f(macro/debug/file)-prefix being
> > > set.
> > > 
> > > So I do think that TARGET_CC_ARCH is the best place for flags that we
> > > really want used. At least as an initial step, because this is known to
> > > work when using the security_flags.inc file. Maybe there's some cleaner
> > > way where we don't unset CFLAGS, but that could be a giant can of worms.
> > > 
> > > And yes, a comment should be added. Is something like
> > > 
> > > # Perf's build system adds its own optimization flags for most TUs,
> > > # overriding the flags included here. But for some, perf does not add
> > > # any -O option, so ensure the distro's chosen optimization gets used
> > > # for those. Since ${SELECTED_OPTIMIZATION} always includes
> > > # ${DEBUG_FLAGS} which in turn includes ${DEBUG_PREFIX_MAP}, this also
> > > # ensures perf is built with appropriate -f*-prefix-map options,
> > > # avoiding the 'buildpaths' QA warning.
> > > 
> > > too verbose?
> > 
> > If it were me I'd probably write:
> > 
> > # The perf makefile has "unset CFLAGS" as "Linux kernel build system is
> > # expected to do the right thing". 
> 
> It's not the perf makefile, it's right there in our recipe itself. I
> don't think upstream perf is to blame.

Sorry, I misunderstood. I thought this was the perf environment that
was doing this so in that case your comment does make much more sense
than mine.

Out of interest did you see what happens if you allow our CFLAGS in and
remove that unset? I also wondered if EXTRA_CFLAGS in EXTRA_OEMAKE
might be an option?

I'm a bit worried that the current situation all feels a little
fragile.

> > and security flags amongst other things so force our 
> > # cflags in.
> 
> This has nothing to do with the security flags. That file still contains
> 
> TARGET_CC_ARCH:append:class-target = " ${SECURITY_CFLAGS}"
> 
> and TARGET_CC_ARCH is included in the CC value we pass to perf's make,
> with or without this patch. So if the distro includes
> security_flags.inc, perf gets built with those flags as expected.
> 
> But when that file is _not_ included, we do miss the prefix-map flags,
> which should be passed regardless of any security distro settings. I
> think the line I'm moving out of security_flags.inc have never really
> had any business being there in the first place, regardless of whether
> adding ${SELECTED_OPTIMIZATION} to TARGET_CC_ARCH is the most
> appropriate method to get those prefix-map options passed.

I agree. It was added as a "lets get security flags roughly working and
find out where the issues are" and has unfortunately stuck around and
not been properly fixed.

>  But since
> that's how the poky distro has achieved that until now, it seems to be
> the least invasive and least risky fix for distros that don't use
> security_flags.inc.

Agreed, however I am interested to see if we can remove some of the
complexity instead as that would be better overall I think if we could
do it.

Cheers,

Richard

On 20/10/2023 12.13, Richard Purdie wrote:
> On Fri, 2023-10-20 at 12:03 +0200, Rasmus Villemoes wrote:
>> On 20/10/2023 11.38, Richard Purdie wrote:
>>> On Fri, 2023-10-20 at 10:10 +0200, Rasmus Villemoes wrote:
>>>> On 19/10/2023 14.48, Richard Purdie wrote:
>>
>>>>> The fact this works suggests perf is ignoring TARGET_CFLAGS. Is there
>>>>> anything in the perf build system where we should be passing in cflags
>>>>> we really want used?
>>>>
>>>> Well, IIUC, the normal way TARGET_CFLAGS would take effect is via the
>>>> 'export CFLAGS = "${TARGET_CFLAGS}'. But in the perf recipe, both
>>>> do_compile and do_install start with
>>>>
>>>>         # Linux kernel build system is expected to do the right thing
>>>>         unset CFLAGS
>>>>
>>>> And perf's build system does indeed add lots of flags of its own, at
>>>> least for most TUs, but nowhere is any -f(macro/debug/file)-prefix being
>>>> set.
>>>>
>>>> So I do think that TARGET_CC_ARCH is the best place for flags that we
>>>> really want used. At least as an initial step, because this is known to
>>>> work when using the security_flags.inc file. Maybe there's some cleaner
>>>> way where we don't unset CFLAGS, but that could be a giant can of worms.
>>>>
>>>> And yes, a comment should be added. Is something like
>>>>
>>>> # Perf's build system adds its own optimization flags for most TUs,
>>>> # overriding the flags included here. But for some, perf does not add
>>>> # any -O option, so ensure the distro's chosen optimization gets used
>>>> # for those. Since ${SELECTED_OPTIMIZATION} always includes
>>>> # ${DEBUG_FLAGS} which in turn includes ${DEBUG_PREFIX_MAP}, this also
>>>> # ensures perf is built with appropriate -f*-prefix-map options,
>>>> # avoiding the 'buildpaths' QA warning.
>>>>
>>>> too verbose?
>>>
>>> If it were me I'd probably write:
>>>
>>> # The perf makefile has "unset CFLAGS" as "Linux kernel build system is
>>> # expected to do the right thing". 
>>
>> It's not the perf makefile, it's right there in our recipe itself. I
>> don't think upstream perf is to blame.
> 
> Sorry, I misunderstood. I thought this was the perf environment that
> was doing this so in that case your comment does make much more sense
> than mine.

OK. Bruce, should I resend?

> Out of interest did you see what happens if you allow our CFLAGS in and
> remove that unset? 

I don't think I've tried that, but I've tried a few other things that I
thought would be cleaner but which then exploded, so in the end I
decided that just lifting exactly what the poky distro does should be
safest and have the highest likelyhood of being accepted.

> I also wondered if EXTRA_CFLAGS in EXTRA_OEMAKE
> might be an option?

Based on quick reading of perf build documentation, yes, EXTRA_CFLAGS
seems like a reasonable approach. What I'm a little worried about is the
TUs where perf apparently does not by itself add all its cflags (-O6 and
whatnot), so I'm not sure if they would be built with what is passed via
EXTRA_CFLAGS.

I will experiment sometime next week if I remember and nothing else
preempts that plan.

> I'm a bit worried that the current situation all feels a little
> fragile.

Agree. But the QA checks do help quite a lot here.

Rasmus

On Fri, Oct 20, 2023 at 7:19 AM Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 20/10/2023 12.13, Richard Purdie wrote:
> > On Fri, 2023-10-20 at 12:03 +0200, Rasmus Villemoes wrote:
> >> On 20/10/2023 11.38, Richard Purdie wrote:
> >>> On Fri, 2023-10-20 at 10:10 +0200, Rasmus Villemoes wrote:
> >>>> On 19/10/2023 14.48, Richard Purdie wrote:
> >>
> >>>>> The fact this works suggests perf is ignoring TARGET_CFLAGS. Is there
> >>>>> anything in the perf build system where we should be passing in cflags
> >>>>> we really want used?
> >>>>
> >>>> Well, IIUC, the normal way TARGET_CFLAGS would take effect is via the
> >>>> 'export CFLAGS = "${TARGET_CFLAGS}'. But in the perf recipe, both
> >>>> do_compile and do_install start with
> >>>>
> >>>>         # Linux kernel build system is expected to do the right thing
> >>>>         unset CFLAGS
> >>>>
> >>>> And perf's build system does indeed add lots of flags of its own, at
> >>>> least for most TUs, but nowhere is any -f(macro/debug/file)-prefix being
> >>>> set.
> >>>>
> >>>> So I do think that TARGET_CC_ARCH is the best place for flags that we
> >>>> really want used. At least as an initial step, because this is known to
> >>>> work when using the security_flags.inc file. Maybe there's some cleaner
> >>>> way where we don't unset CFLAGS, but that could be a giant can of worms.
> >>>>
> >>>> And yes, a comment should be added. Is something like
> >>>>
> >>>> # Perf's build system adds its own optimization flags for most TUs,
> >>>> # overriding the flags included here. But for some, perf does not add
> >>>> # any -O option, so ensure the distro's chosen optimization gets used
> >>>> # for those. Since ${SELECTED_OPTIMIZATION} always includes
> >>>> # ${DEBUG_FLAGS} which in turn includes ${DEBUG_PREFIX_MAP}, this also
> >>>> # ensures perf is built with appropriate -f*-prefix-map options,
> >>>> # avoiding the 'buildpaths' QA warning.
> >>>>
> >>>> too verbose?
> >>>
> >>> If it were me I'd probably write:
> >>>
> >>> # The perf makefile has "unset CFLAGS" as "Linux kernel build system is
> >>> # expected to do the right thing".
> >>
> >> It's not the perf makefile, it's right there in our recipe itself. I
> >> don't think upstream perf is to blame.
> >
> > Sorry, I misunderstood. I thought this was the perf environment that
> > was doing this so in that case your comment does make much more sense
> > than mine.
>
> OK. Bruce, should I resend?

Yes, let's go for a resend with the comment added. If we can avoid even some of
the pain wondering about this again .. our future selves will thank us :)

>
> > Out of interest did you see what happens if you allow our CFLAGS in and
> > remove that unset?
>
> I don't think I've tried that, but I've tried a few other things that I
> thought would be cleaner but which then exploded, so in the end I
> decided that just lifting exactly what the poky distro does should be
> safest and have the highest likelyhood of being accepted.

This is not directed at Rasmus, but a general comment ..

For pretty much anything kernel, and perf has always strongly been in
that category, we've always unset everything and allowed the Makefiles
(and the nested mess of tests, includes and trickery) to set their own
flags. It has saved subtle runtime issues and this is one area that the
developers tend to know what they are doing.

When there's a chance to append, they've give us a variable to do so,
that's why we've always used EXTRA_CFLAGS, and that likely is the
right place to use here as well. But even then, we shouldn't pass ALL of
our flags in there, just what we think needs to be in addition to the
perf generated set of CFLAGS.

Bruce

>
> > I also wondered if EXTRA_CFLAGS in EXTRA_OEMAKE
> > might be an option?
>
> Based on quick reading of perf build documentation, yes, EXTRA_CFLAGS
> seems like a reasonable approach. What I'm a little worried about is the
> TUs where perf apparently does not by itself add all its cflags (-O6 and
> whatnot), so I'm not sure if they would be built with what is passed via
> EXTRA_CFLAGS.
>
> I will experiment sometime next week if I remember and nothing else
> preempts that plan.
>
> > I'm a bit worried that the current situation all feels a little
> > fragile.
>
> Agree. But the QA checks do help quite a lot here.
>
> Rasmus
>

On Fri, 2023-10-20 at 08:52 -0400, Bruce Ashfield wrote:
> On Fri, Oct 20, 2023 at 7:19 AM Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
> > 
> > On 20/10/2023 12.13, Richard Purdie wrote:
> > > On Fri, 2023-10-20 at 12:03 +0200, Rasmus Villemoes wrote:
> > > > On 20/10/2023 11.38, Richard Purdie wrote:
> > > > > On Fri, 2023-10-20 at 10:10 +0200, Rasmus Villemoes wrote:
> > > > > > On 19/10/2023 14.48, Richard Purdie wrote:
> > > > 
> > > > > > > The fact this works suggests perf is ignoring TARGET_CFLAGS. Is there
> > > > > > > anything in the perf build system where we should be passing in cflags
> > > > > > > we really want used?
> > > > > > 
> > > > > > Well, IIUC, the normal way TARGET_CFLAGS would take effect is via the
> > > > > > 'export CFLAGS = "${TARGET_CFLAGS}'. But in the perf recipe, both
> > > > > > do_compile and do_install start with
> > > > > > 
> > > > > >         # Linux kernel build system is expected to do the right thing
> > > > > >         unset CFLAGS
> > > > > > 
> > > > > > And perf's build system does indeed add lots of flags of its own, at
> > > > > > least for most TUs, but nowhere is any -f(macro/debug/file)-prefix being
> > > > > > set.
> > > > > > 
> > > > > > So I do think that TARGET_CC_ARCH is the best place for flags that we
> > > > > > really want used. At least as an initial step, because this is known to
> > > > > > work when using the security_flags.inc file. Maybe there's some cleaner
> > > > > > way where we don't unset CFLAGS, but that could be a giant can of worms.
> > > > > > 
> > > > > > And yes, a comment should be added. Is something like
> > > > > > 
> > > > > > # Perf's build system adds its own optimization flags for most TUs,
> > > > > > # overriding the flags included here. But for some, perf does not add
> > > > > > # any -O option, so ensure the distro's chosen optimization gets used
> > > > > > # for those. Since ${SELECTED_OPTIMIZATION} always includes
> > > > > > # ${DEBUG_FLAGS} which in turn includes ${DEBUG_PREFIX_MAP}, this also
> > > > > > # ensures perf is built with appropriate -f*-prefix-map options,
> > > > > > # avoiding the 'buildpaths' QA warning.
> > > > > > 
> > > > > > too verbose?
> > > > > 
> > > > > If it were me I'd probably write:
> > > > > 
> > > > > # The perf makefile has "unset CFLAGS" as "Linux kernel build system is
> > > > > # expected to do the right thing".
> > > > 
> > > > It's not the perf makefile, it's right there in our recipe itself. I
> > > > don't think upstream perf is to blame.
> > > 
> > > Sorry, I misunderstood. I thought this was the perf environment that
> > > was doing this so in that case your comment does make much more sense
> > > than mine.
> > 
> > OK. Bruce, should I resend?
> 
> Yes, let's go for a resend with the comment added. If we can avoid even some of
> the pain wondering about this again .. our future selves will thank us :)
> 
> > 
> > > Out of interest did you see what happens if you allow our CFLAGS in and
> > > remove that unset?
> > 
> > I don't think I've tried that, but I've tried a few other things that I
> > thought would be cleaner but which then exploded, so in the end I
> > decided that just lifting exactly what the poky distro does should be
> > safest and have the highest likelyhood of being accepted.
> 
> This is not directed at Rasmus, but a general comment ..
> 
> For pretty much anything kernel, and perf has always strongly been in
> that category, we've always unset everything and allowed the Makefiles
> (and the nested mess of tests, includes and trickery) to set their own
> flags. It has saved subtle runtime issues and this is one area that the
> developers tend to know what they are doing.

For the kernel itself, yes, they do. For userspace binaries, we do in
general have a good idea of how binaries should be being built and
we're a bit ahead of the curve on the flags/features that should be
configured.

> When there's a chance to append, they've give us a variable to do so,
> that's why we've always used EXTRA_CFLAGS, and that likely is the
> right place to use here as well. But even then, we shouldn't pass ALL of
> our flags in there, just what we think needs to be in addition to the
> perf generated set of CFLAGS.

I'm not entirely convinced of that. Which CFLAGS do we have which would
be harmful to userspace binaries?

I'm basically saying we should at least investigate this as I'm not
sure which things we have in CFLAGS would be problematic to userspace
binaries in general. It may not work and that is fine but I don't think
perf userspace should be that sensitive to the things we usually have
set.

Cheers,

Richard

On Fri, Oct 20, 2023 at 10:24 AM Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
>
> On Fri, 2023-10-20 at 08:52 -0400, Bruce Ashfield wrote:
> > On Fri, Oct 20, 2023 at 7:19 AM Rasmus Villemoes
> > <rasmus.villemoes@prevas.dk> wrote:
> > >
> > > On 20/10/2023 12.13, Richard Purdie wrote:
> > > > On Fri, 2023-10-20 at 12:03 +0200, Rasmus Villemoes wrote:
> > > > > On 20/10/2023 11.38, Richard Purdie wrote:
> > > > > > On Fri, 2023-10-20 at 10:10 +0200, Rasmus Villemoes wrote:
> > > > > > > On 19/10/2023 14.48, Richard Purdie wrote:
> > > > >
> > > > > > > > The fact this works suggests perf is ignoring TARGET_CFLAGS. Is there
> > > > > > > > anything in the perf build system where we should be passing in cflags
> > > > > > > > we really want used?
> > > > > > >
> > > > > > > Well, IIUC, the normal way TARGET_CFLAGS would take effect is via the
> > > > > > > 'export CFLAGS = "${TARGET_CFLAGS}'. But in the perf recipe, both
> > > > > > > do_compile and do_install start with
> > > > > > >
> > > > > > >         # Linux kernel build system is expected to do the right thing
> > > > > > >         unset CFLAGS
> > > > > > >
> > > > > > > And perf's build system does indeed add lots of flags of its own, at
> > > > > > > least for most TUs, but nowhere is any -f(macro/debug/file)-prefix being
> > > > > > > set.
> > > > > > >
> > > > > > > So I do think that TARGET_CC_ARCH is the best place for flags that we
> > > > > > > really want used. At least as an initial step, because this is known to
> > > > > > > work when using the security_flags.inc file. Maybe there's some cleaner
> > > > > > > way where we don't unset CFLAGS, but that could be a giant can of worms.
> > > > > > >
> > > > > > > And yes, a comment should be added. Is something like
> > > > > > >
> > > > > > > # Perf's build system adds its own optimization flags for most TUs,
> > > > > > > # overriding the flags included here. But for some, perf does not add
> > > > > > > # any -O option, so ensure the distro's chosen optimization gets used
> > > > > > > # for those. Since ${SELECTED_OPTIMIZATION} always includes
> > > > > > > # ${DEBUG_FLAGS} which in turn includes ${DEBUG_PREFIX_MAP}, this also
> > > > > > > # ensures perf is built with appropriate -f*-prefix-map options,
> > > > > > > # avoiding the 'buildpaths' QA warning.
> > > > > > >
> > > > > > > too verbose?
> > > > > >
> > > > > > If it were me I'd probably write:
> > > > > >
> > > > > > # The perf makefile has "unset CFLAGS" as "Linux kernel build system is
> > > > > > # expected to do the right thing".
> > > > >
> > > > > It's not the perf makefile, it's right there in our recipe itself. I
> > > > > don't think upstream perf is to blame.
> > > >
> > > > Sorry, I misunderstood. I thought this was the perf environment that
> > > > was doing this so in that case your comment does make much more sense
> > > > than mine.
> > >
> > > OK. Bruce, should I resend?
> >
> > Yes, let's go for a resend with the comment added. If we can avoid even some of
> > the pain wondering about this again .. our future selves will thank us :)
> >
> > >
> > > > Out of interest did you see what happens if you allow our CFLAGS in and
> > > > remove that unset?
> > >
> > > I don't think I've tried that, but I've tried a few other things that I
> > > thought would be cleaner but which then exploded, so in the end I
> > > decided that just lifting exactly what the poky distro does should be
> > > safest and have the highest likelyhood of being accepted.
> >
> > This is not directed at Rasmus, but a general comment ..
> >
> > For pretty much anything kernel, and perf has always strongly been in
> > that category, we've always unset everything and allowed the Makefiles
> > (and the nested mess of tests, includes and trickery) to set their own
> > flags. It has saved subtle runtime issues and this is one area that the
> > developers tend to know what they are doing.
>
> For the kernel itself, yes, they do. For userspace binaries, we do in
> general have a good idea of how binaries should be being built and
> we're a bit ahead of the curve on the flags/features that should be
> configured.

And I'm saying that these userspace packages in the kernel are
almost as opinionated as the kernel. I generally trust their options
and don't want to control all aspects of the flags. I certainly don't
want to support them as we climb through the new kernel versions,
so keeping the defaults is a much better option.

>
> > When there's a chance to append, they've give us a variable to do so,
> > that's why we've always used EXTRA_CFLAGS, and that likely is the
> > right place to use here as well. But even then, we shouldn't pass ALL of
> > our flags in there, just what we think needs to be in addition to the
> > perf generated set of CFLAGS.
>
> I'm not entirely convinced of that. Which CFLAGS do we have which would
> be harmful to userspace binaries?
>

Optimization has always been an issue, as have been the include paths
(which chain into how we sync the kernel headers and perf similar /
duplicate headers), even some of the security options have caused
troubles as perf really isn't something you can use in a "secure" way.

Which is why we've always added to the flags, versus controlling
them completely.

> I'm basically saying we should at least investigate this as I'm not
> sure which things we have in CFLAGS would be problematic to userspace
> binaries in general. It may not work and that is fine but I don't think
> perf userspace should be that sensitive to the things we usually have
> set.

I don't see a lot of value in looking at it too much, as we are just
trading one set of issues for another. But I of course can't stop
anyone from looking and suggesting changes, I'll just be asking them
for help when I end up crashing into issues during new kernel
introductions :)

Bruce

>
> Cheers,
>
> Richard