On Fri, Oct 20, 2023 at 10:24 AM Richard Purdie
<richard.pur...@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.villem...@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
--
- Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end
- "Use the force Harry" - Gandalf, Star Trek II
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#189544):
https://lists.openembedded.org/g/openembedded-core/message/189544
Mute This Topic: https://lists.openembedded.org/mt/102058904/21656
Group Owner: openembedded-core+ow...@lists.openembedded.org
Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
