Re: [OE-core] [PATCH] perf: lift TARGET_CC_ARCH modification out of security_flags.inc

[Richard Purdie <richard.purdie@linuxfoundation.org>]

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