Re: [PATCH v4 3/4] selftests/x86: don't clobber CFLAGS

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4864 bytes --]

On Tue, 3 Sep 2024, Reinette Chatre wrote:
> On 9/3/24 7:45 AM, Ilpo Järvinen wrote:
> > The x86 selftests makefile clobbers CFLAGS preventing lib.mk from
> > making the necessary adjustments into CFLAGS. This would lead to a
> > build failure after upcoming change which wants to add -DHAVE_CPUID=
> > into CFLAGS.
> > 
> > Reorder CFLAGS initialization in x86 selftest. Place the constant part
> > of CFLAGS initialization before inclusion of lib.mk but leave adding
> > KHDR_INCLUDES into CFLAGS into the existing position because
> > KHDR_INCLUDES might be generated inside lib.mk.
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> > v4:
> > - New patch
> > 
> >   tools/testing/selftests/x86/Makefile | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/x86/Makefile
> > b/tools/testing/selftests/x86/Makefile
> > index 5c8757a25998..88a6576de92f 100644
> > --- a/tools/testing/selftests/x86/Makefile
> > +++ b/tools/testing/selftests/x86/Makefile
> > @@ -1,4 +1,6 @@
> >   # SPDX-License-Identifier: GPL-2.0
> > +CFLAGS := -O2 -g -std=gnu99 -pthread -Wall
> > +
> >   all:
> >     include ../lib.mk
> > @@ -35,7 +37,7 @@ BINARIES_64 := $(TARGETS_C_64BIT_ALL:%=%_64)
> >   BINARIES_32 := $(patsubst %,$(OUTPUT)/%,$(BINARIES_32))
> >   BINARIES_64 := $(patsubst %,$(OUTPUT)/%,$(BINARIES_64))
> >   -CFLAGS := -O2 -g -std=gnu99 -pthread -Wall $(KHDR_INCLUDES)
> > +CFLAGS += $(KHDR_INCLUDES)
> >     # call32_from_64 in thunks.S uses absolute addresses.
> >   ifeq ($(CAN_BUILD_WITH_NOPIE),1)
> 
> These changes are becoming less obvious to me. The first two
> red flags are:
> - Most other top level Makefiles assign KHDR_INCLUDES to CFLAGS
>   *before* including lib.mk

Most toplevel makefiles assigned CFLAGS before including lib.mk so x86 
selftest was an exception overwriting CFLAGS after including lib.mk.
That looks like a real problem/bug and you don't seem to contest lib.mk 
having the right to alter CFLAGS. So I'm still convinced this patch is
necessary independent of the cpuid/resctrl selftest issue.

I believe most of those Makefile are _buggy_ wrt. KHDR_INCLUDES but I 
don't care where $(KHDR_INCLUDES) goes, it's irrelevant for the problem
I'm trying to solve which is the CFLAGS clobber. ...I just didn't want to
add yet another problem by moving KHDR_INCLUDES before including lib.mk. 
I'm fine with leaving that can of worms for somebody else to sort out :-).

> - What current Makefiles do matches the guidance from
>   Documentation/dev-tools/kselftest.rst as per example (verbatim copy):
>     CFLAGS = $(KHDR_INCLUDES)
>     TEST_GEN_PROGS := close_range_test
>     include ../lib.mk

I'm not objecting moving the entire CFLAGS line before including lib.mk
in the x86 selftests makefile if that is what you'd want me to do?

> Looking closer it is not clear to me why lib.mk sets KHDR_INCLUDES.

So are you suggesting the commit a52540522c95 ("selftests/landlock: Fix 
out-of-tree builds") added it for no reason? The commit message indicates
it was added on purpose but I don't fully understand what "to other test 
collections when building in their directory" means.

> As I understand the usage is intended to be:
>   make TARGETS="target" -C tools/testing/selftests
> This means that it is tools/testing/selftests/Makefile that will
> run first and it ensures that KHDR_INCLUDES is set and supports
> the usage documented in Documentation/dev-tools/kselftest.rst
> 
> One usage that a change like in this patch could support is
> for users to be able to run "make" from within the test
> directory self ... but considering the current KHDR_INCLUDES
> custom this does not seem to be supported? (but we cannot
> know which tests/users rely on this behavior)

Perhaps "when building in their directory" is exactly that?

I don't doubt the makefiles are very full of problems.

> Looking further I also noticed that
> tools/testing/selftests/Makefile even sets ARCH (but does
> not export it). When considering the next patch it almost looks
> like what is missing is for tools/testing/selftests/Makefile to
> "export ARCH" ... but that potentially impacts the Makefiles that
> do manipulation of ARCH.

The ARCH handling in various Makefiles is another mess.

> I initially started to look at this because of the
> resctrl impact, but I clearly am not familiar enough
> with the kselftest build files to understand the
> impacts nor provide guidance. I do hope the kselftest
> folks can help here.

Luckily this seems to have caught Shuah attention now so hopefully we've 
soon some reasonable resolution to ARCH dependent building and code 
fragments so each selftest doesn't need to come up their own way of 
handling it. :-)

-- 
 i.