linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] selftests: Remove explicit headers for clang
@ 2021-10-04 23:04 Andrew Delgadilo
  2021-10-05 16:59 ` Nick Desaulniers
  0 siblings, 1 reply; 4+ messages in thread
From: Andrew Delgadilo @ 2021-10-04 23:04 UTC (permalink / raw)
  To: Shuah Khan, linux-kselftest, linux-kernel
  Cc: Nathan Chancellor, Nick Desaulniers, gthelen, Andrew Delgadillo, stable

From: Andrew Delgadillo <adelg@google.com>

GCC allows paths to header files to be passed on the command line while
using -o, but clang does not:

$ make -C tools/testing/selftests TARGETS=futex

$ make -C tools/testing/selftests TARGETS=futex LLVM=1
clang -Wall   -g -O2 -Wall -D_GNU_SOURCE -pthread -I../include \
-I../../ -I../../../../../usr/include/ -I/kselftest/usr/include \
futex_wait_timeout.c ../include/futextest.h ../include/atomic.h \
../include/logging.h -lpthread -lrt -o \
tools/testing/selftests/futex/functional/futex_wait_timeout
clang: error: cannot specify -o when generating multiple output files

To fix this, remove explicit paths to headers from the commandline in
lib.mk. We must explicitly remove them for x86 and binderfs as they are
not filtered out by the change to lib.mk, but the compiler search paths
for includes are already setup correctly, so the compiler finds the
correct headers.

Tested: selftests build with LLVM=1 now.
Cc: stable@vger.kernel.org
Signed-off-by: Andrew Delgadillo <adelg@google.com>
---
 tools/testing/selftests/filesystems/binderfs/Makefile | 2 +-
 tools/testing/selftests/lib.mk                        | 2 +-
 tools/testing/selftests/x86/Makefile                  | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/filesystems/binderfs/Makefile b/tools/testing/selftests/filesystems/binderfs/Makefile
index 8af25ae96049..58e41bd98200 100644
--- a/tools/testing/selftests/filesystems/binderfs/Makefile
+++ b/tools/testing/selftests/filesystems/binderfs/Makefile
@@ -3,6 +3,6 @@
 CFLAGS += -I../../../../../usr/include/ -pthread
 TEST_GEN_PROGS := binderfs_test
 
-binderfs_test: binderfs_test.c ../../kselftest.h ../../kselftest_harness.h
+binderfs_test: binderfs_test.c
 
 include ../../lib.mk
diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
index fa2ac0e56b43..fb152e20c86a 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -142,7 +142,7 @@ endif
 ifeq ($(OVERRIDE_TARGETS),)
 LOCAL_HDRS := $(selfdir)/kselftest_harness.h $(selfdir)/kselftest.h
 $(OUTPUT)/%:%.c $(LOCAL_HDRS)
-	$(LINK.c) $(filter-out $(LOCAL_HDRS),$^) $(LDLIBS) -o $@
+	$(LINK.c) $(filter-out %.h,$^) $(LDLIBS) -o $@
 
 $(OUTPUT)/%.o:%.S
 	$(COMPILE.S) $^ -o $@
diff --git a/tools/testing/selftests/x86/Makefile b/tools/testing/selftests/x86/Makefile
index b4142cd1c5c2..68967006b3e9 100644
--- a/tools/testing/selftests/x86/Makefile
+++ b/tools/testing/selftests/x86/Makefile
@@ -72,10 +72,10 @@ all_64: $(BINARIES_64)
 EXTRA_CLEAN := $(BINARIES_32) $(BINARIES_64)
 
 $(BINARIES_32): $(OUTPUT)/%_32: %.c helpers.h
-	$(CC) -m32 -o $@ $(CFLAGS) $(EXTRA_CFLAGS) $^ -lrt -ldl -lm
+	$(CC) -m32 -o $@ $(CFLAGS) $(EXTRA_CFLAGS) $(filter-out %.h,$^) -lrt -ldl -lm
 
 $(BINARIES_64): $(OUTPUT)/%_64: %.c helpers.h
-	$(CC) -m64 -o $@ $(CFLAGS) $(EXTRA_CFLAGS) $^ -lrt -ldl
+	$(CC) -m64 -o $@ $(CFLAGS) $(EXTRA_CFLAGS) $(filter-out %.h,$^) -lrt -ldl
 
 # x86_64 users should be encouraged to install 32-bit libraries
 ifeq ($(CAN_BUILD_I386)$(CAN_BUILD_X86_64),01)
-- 
2.33.0.800.g4c38ced690-goog


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] selftests: Remove explicit headers for clang
  2021-10-04 23:04 [PATCH] selftests: Remove explicit headers for clang Andrew Delgadilo
@ 2021-10-05 16:59 ` Nick Desaulniers
  2021-10-05 18:43   ` Andrew Delgadillo
  0 siblings, 1 reply; 4+ messages in thread
From: Nick Desaulniers @ 2021-10-05 16:59 UTC (permalink / raw)
  To: Andrew Delgadilo
  Cc: Shuah Khan, linux-kselftest, linux-kernel, Nathan Chancellor,
	gthelen, stable

On Mon, Oct 4, 2021 at 4:04 PM Andrew Delgadilo <adelg@google.com> wrote:
>
> From: Andrew Delgadillo <adelg@google.com>
>
> GCC allows paths to header files to be passed on the command line while
> using -o, but clang does not:

Ah, it's because `-I` *insn't* being used more so than `-o` being present.

>
> $ make -C tools/testing/selftests TARGETS=futex
>
> $ make -C tools/testing/selftests TARGETS=futex LLVM=1
> clang -Wall   -g -O2 -Wall -D_GNU_SOURCE -pthread -I../include \
> -I../../ -I../../../../../usr/include/ -I/kselftest/usr/include \
> futex_wait_timeout.c ../include/futextest.h ../include/atomic.h \
> ../include/logging.h -lpthread -lrt -o \
> tools/testing/selftests/futex/functional/futex_wait_timeout
> clang: error: cannot specify -o when generating multiple output files

Why aren't `-I` flags being passed? Rather than:

$ clang ... ../include/futextest.h ../include/atomic.h ../include/logging.h ...

shouldn't this be:

$ clang ... -I ../include/futextest.h -I ../include/atomic.h -I
../include/logging.h

>
> To fix this, remove explicit paths to headers from the commandline in
> lib.mk. We must explicitly remove them for x86 and binderfs as they are
> not filtered out by the change to lib.mk, but the compiler search paths
> for includes are already setup correctly, so the compiler finds the
> correct headers.
>
> Tested: selftests build with LLVM=1 now.

With this patch applied
$ make -C tools/testing/selftests TARGETS=futex LLVM=1
WFM but
$ make -C tools/testing/selftests LLVM=1
fails, horribly. Are you always expected to pass TARGETS when building
the selftests?

> Cc: stable@vger.kernel.org
> Signed-off-by: Andrew Delgadillo <adelg@google.com>
> ---
>  tools/testing/selftests/filesystems/binderfs/Makefile | 2 +-
>  tools/testing/selftests/lib.mk                        | 2 +-
>  tools/testing/selftests/x86/Makefile                  | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/filesystems/binderfs/Makefile b/tools/testing/selftests/filesystems/binderfs/Makefile
> index 8af25ae96049..58e41bd98200 100644
> --- a/tools/testing/selftests/filesystems/binderfs/Makefile
> +++ b/tools/testing/selftests/filesystems/binderfs/Makefile
> @@ -3,6 +3,6 @@
>  CFLAGS += -I../../../../../usr/include/ -pthread
>  TEST_GEN_PROGS := binderfs_test
>
> -binderfs_test: binderfs_test.c ../../kselftest.h ../../kselftest_harness.h
> +binderfs_test: binderfs_test.c
>
>  include ../../lib.mk
> diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
> index fa2ac0e56b43..fb152e20c86a 100644
> --- a/tools/testing/selftests/lib.mk
> +++ b/tools/testing/selftests/lib.mk
> @@ -142,7 +142,7 @@ endif
>  ifeq ($(OVERRIDE_TARGETS),)
>  LOCAL_HDRS := $(selfdir)/kselftest_harness.h $(selfdir)/kselftest.h
>  $(OUTPUT)/%:%.c $(LOCAL_HDRS)
> -       $(LINK.c) $(filter-out $(LOCAL_HDRS),$^) $(LDLIBS) -o $@
> +       $(LINK.c) $(filter-out %.h,$^) $(LDLIBS) -o $@

What? Aren't kselftest.h and kselftest_harness.h already part of
LOCAL_HDRS?  Perhaps that filter-out is broken, or LOCAL_HDRS.  Yeah,
adding some debugging:

diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
index fe7ee2b0f29c..827f766d6057 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -142,6 +142,7 @@ endif
 # OVERRIDE_TARGETS = 1.
 ifeq ($(OVERRIDE_TARGETS),)
 LOCAL_HDRS := $(selfdir)/kselftest_harness.h $(selfdir)/kselftest.h
+$(info $$LOCAL_HDRS is [${LOCAL_HDRS}])
 $(OUTPUT)/%:%.c $(LOCAL_HDRS)
        $(LINK.c) $(filter-out $(LOCAL_HDRS),$^) $(LDLIBS) -o $@

prints:

$LOCAL_HDRS is [/android0/kernel-all/tools/testing/selftests/kselftest_harness.h
/android0/kernel-all/tools/testing/selftests/kselftest.h]

so of course filter-out isn't going to match `../include/futextest.h
../include/atomic.h ../include/logging.h`.

>
>  $(OUTPUT)/%.o:%.S
>         $(COMPILE.S) $^ -o $@
> diff --git a/tools/testing/selftests/x86/Makefile b/tools/testing/selftests/x86/Makefile
> index b4142cd1c5c2..68967006b3e9 100644
> --- a/tools/testing/selftests/x86/Makefile
> +++ b/tools/testing/selftests/x86/Makefile
> @@ -72,10 +72,10 @@ all_64: $(BINARIES_64)
>  EXTRA_CLEAN := $(BINARIES_32) $(BINARIES_64)
>
>  $(BINARIES_32): $(OUTPUT)/%_32: %.c helpers.h
> -       $(CC) -m32 -o $@ $(CFLAGS) $(EXTRA_CFLAGS) $^ -lrt -ldl -lm
> +       $(CC) -m32 -o $@ $(CFLAGS) $(EXTRA_CFLAGS) $(filter-out %.h,$^) -lrt -ldl -lm
>
>  $(BINARIES_64): $(OUTPUT)/%_64: %.c helpers.h
> -       $(CC) -m64 -o $@ $(CFLAGS) $(EXTRA_CFLAGS) $^ -lrt -ldl
> +       $(CC) -m64 -o $@ $(CFLAGS) $(EXTRA_CFLAGS) $(filter-out %.h,$^) -lrt -ldl
>
>  # x86_64 users should be encouraged to install 32-bit libraries
>  ifeq ($(CAN_BUILD_I386)$(CAN_BUILD_X86_64),01)
> --
> 2.33.0.800.g4c38ced690-goog
>


-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] selftests: Remove explicit headers for clang
  2021-10-05 16:59 ` Nick Desaulniers
@ 2021-10-05 18:43   ` Andrew Delgadillo
  2021-10-05 18:50     ` Nick Desaulniers
  0 siblings, 1 reply; 4+ messages in thread
From: Andrew Delgadillo @ 2021-10-05 18:43 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Shuah Khan, open list:KERNEL SELFTEST FRAMEWORK, LKML,
	Nathan Chancellor, Greg Thelen, stable

On Tue, Oct 5, 2021 at 9:59 AM Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> On Mon, Oct 4, 2021 at 4:04 PM Andrew Delgadilo <adelg@google.com> wrote:
> >
> > From: Andrew Delgadillo <adelg@google.com>
> >
> > GCC allows paths to header files to be passed on the command line while
> > using -o, but clang does not:
>
> Ah, it's because `-I` *insn't* being used more so than `-o` being present.
> >
> > $ make -C tools/testing/selftests TARGETS=futex
> >
> > $ make -C tools/testing/selftests TARGETS=futex LLVM=1
> > clang -Wall   -g -O2 -Wall -D_GNU_SOURCE -pthread -I../include \
> > -I../../ -I../../../../../usr/include/ -I/kselftest/usr/include \
> > futex_wait_timeout.c ../include/futextest.h ../include/atomic.h \
> > ../include/logging.h -lpthread -lrt -o \
> > tools/testing/selftests/futex/functional/futex_wait_timeout
> > clang: error: cannot specify -o when generating multiple output files
>
> Why aren't `-I` flags being passed? Rather than:
>
> $ clang ... ../include/futextest.h ../include/atomic.h ../include/logging.h ...
>
> shouldn't this be:
>
> $ clang ... -I ../include/futextest.h -I ../include/atomic.h -I
> ../include/logging.h

Okay, I see, so the fix here wouldn't be to remove the headers from
the commandline, we should just prepend them with `-l`.

> >
> > To fix this, remove explicit paths to headers from the commandline in
> > lib.mk. We must explicitly remove them for x86 and binderfs as they are
> > not filtered out by the change to lib.mk, but the compiler search paths
> > for includes are already setup correctly, so the compiler finds the
> > correct headers.
> >
> > Tested: selftests build with LLVM=1 now.
>
> With this patch applied
> $ make -C tools/testing/selftests TARGETS=futex LLVM=1
> WFM but
> $ make -C tools/testing/selftests LLVM=1
> fails, horribly. Are you always expected to pass TARGETS when building
> the selftests?

I specifically passed TARGETS=futex because I want to point out a
specific example where this is in an issue as there are other errors I
see when I build all of selftests with LLVM=1. But to answer your
question, no, I do not think you are expected to always pass TARGETS.

When I run (without this patch)

$ make -C tools/testing/selftests LLVM=1
 I get a bunch of errors as well ranging from:
- clang: error: cannot specify -o when generating multiple output
files <-- the specific one I'm trying to fix
- clang: warning: argument unused during compilation: '-pie'
[-Wunused-command-line-argument]
- include/x86_64/processor.h:344:25: warning: variable 'xmm7' is
uninitialized when used here [-Wuninitialized]

                return (unsigned long)xmm7;
- fuse_mnt.c:17:10: fatal error: 'fuse.h' file not found

#include <fuse.h>

         ^~~~~~~~

However with the patch applied, I no longer see any "clang: error:
cannot specify -o when generating multiple output files", meaning that
I fixed one class of errors when building with LLVM=1.

Do you see a clean build currently when building selftests with
LLVM=1? I'm not arguing that this patch fixes *all* the errors seen,
but it at least fixes one class of them. Although, it seems I went
about fixing it in the wrong manner. I can respin this to prepend -l
before header includes to get rid of the "clang: error: cannot specify
-o when generating multiple output files" errors.




> > Cc: stable@vger.kernel.org
> > Signed-off-by: Andrew Delgadillo <adelg@google.com>
> > ---
> >  tools/testing/selftests/filesystems/binderfs/Makefile | 2 +-
> >  tools/testing/selftests/lib.mk                        | 2 +-
> >  tools/testing/selftests/x86/Makefile                  | 4 ++--
> >  3 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/testing/selftests/filesystems/binderfs/Makefile b/tools/testing/selftests/filesystems/binderfs/Makefile
> > index 8af25ae96049..58e41bd98200 100644
> > --- a/tools/testing/selftests/filesystems/binderfs/Makefile
> > +++ b/tools/testing/selftests/filesystems/binderfs/Makefile
> > @@ -3,6 +3,6 @@
> >  CFLAGS += -I../../../../../usr/include/ -pthread
> >  TEST_GEN_PROGS := binderfs_test
> >
> > -binderfs_test: binderfs_test.c ../../kselftest.h ../../kselftest_harness.h
> > +binderfs_test: binderfs_test.c
> >
> >  include ../../lib.mk
> > diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
> > index fa2ac0e56b43..fb152e20c86a 100644
> > --- a/tools/testing/selftests/lib.mk
> > +++ b/tools/testing/selftests/lib.mk
> > @@ -142,7 +142,7 @@ endif
> >  ifeq ($(OVERRIDE_TARGETS),)
> >  LOCAL_HDRS := $(selfdir)/kselftest_harness.h $(selfdir)/kselftest.h
> >  $(OUTPUT)/%:%.c $(LOCAL_HDRS)
> > -       $(LINK.c) $(filter-out $(LOCAL_HDRS),$^) $(LDLIBS) -o $@
> > +       $(LINK.c) $(filter-out %.h,$^) $(LDLIBS) -o $@
>
> What? Aren't kselftest.h and kselftest_harness.h already part of
> LOCAL_HDRS?  Perhaps that filter-out is broken, or LOCAL_HDRS.  Yeah,
> adding some debugging:
>
> diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
> index fe7ee2b0f29c..827f766d6057 100644
> --- a/tools/testing/selftests/lib.mk
> +++ b/tools/testing/selftests/lib.mk
> @@ -142,6 +142,7 @@ endif
>  # OVERRIDE_TARGETS = 1.
>  ifeq ($(OVERRIDE_TARGETS),)
>  LOCAL_HDRS := $(selfdir)/kselftest_harness.h $(selfdir)/kselftest.h
> +$(info $$LOCAL_HDRS is [${LOCAL_HDRS}])
>  $(OUTPUT)/%:%.c $(LOCAL_HDRS)
>         $(LINK.c) $(filter-out $(LOCAL_HDRS),$^) $(LDLIBS) -o $@
>
> prints:
>
> $LOCAL_HDRS is [/android0/kernel-all/tools/testing/selftests/kselftest_harness.h
> /android0/kernel-all/tools/testing/selftests/kselftest.h]
>
> so of course filter-out isn't going to match `../include/futextest.h
> ../include/atomic.h ../include/logging.h`.

Like you mentioned above, it seems a better way to about this would be
to prepend -I before the includes. I'll go ahead and send a new patch
to do that.
> >
> >  $(OUTPUT)/%.o:%.S
> >         $(COMPILE.S) $^ -o $@
> > diff --git a/tools/testing/selftests/x86/Makefile b/tools/testing/selftests/x86/Makefile
> > index b4142cd1c5c2..68967006b3e9 100644
> > --- a/tools/testing/selftests/x86/Makefile
> > +++ b/tools/testing/selftests/x86/Makefile
> > @@ -72,10 +72,10 @@ all_64: $(BINARIES_64)
> >  EXTRA_CLEAN := $(BINARIES_32) $(BINARIES_64)
> >
> >  $(BINARIES_32): $(OUTPUT)/%_32: %.c helpers.h
> > -       $(CC) -m32 -o $@ $(CFLAGS) $(EXTRA_CFLAGS) $^ -lrt -ldl -lm
> > +       $(CC) -m32 -o $@ $(CFLAGS) $(EXTRA_CFLAGS) $(filter-out %.h,$^) -lrt -ldl -lm
> >
> >  $(BINARIES_64): $(OUTPUT)/%_64: %.c helpers.h
> > -       $(CC) -m64 -o $@ $(CFLAGS) $(EXTRA_CFLAGS) $^ -lrt -ldl
> > +       $(CC) -m64 -o $@ $(CFLAGS) $(EXTRA_CFLAGS) $(filter-out %.h,$^) -lrt -ldl
> >
> >  # x86_64 users should be encouraged to install 32-bit libraries
> >  ifeq ($(CAN_BUILD_I386)$(CAN_BUILD_X86_64),01)
> > --
> > 2.33.0.800.g4c38ced690-goog
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] selftests: Remove explicit headers for clang
  2021-10-05 18:43   ` Andrew Delgadillo
@ 2021-10-05 18:50     ` Nick Desaulniers
  0 siblings, 0 replies; 4+ messages in thread
From: Nick Desaulniers @ 2021-10-05 18:50 UTC (permalink / raw)
  To: Andrew Delgadillo
  Cc: Shuah Khan, open list:KERNEL SELFTEST FRAMEWORK, LKML,
	Nathan Chancellor, Greg Thelen, stable

On Tue, Oct 5, 2021 at 11:43 AM Andrew Delgadillo <adelg@google.com> wrote:
>
> On Tue, Oct 5, 2021 at 9:59 AM Nick Desaulniers <ndesaulniers@google.com> wrote:
> >
> > On Mon, Oct 4, 2021 at 4:04 PM Andrew Delgadilo <adelg@google.com> wrote:
> > >
> > > From: Andrew Delgadillo <adelg@google.com>
> > >
> > > GCC allows paths to header files to be passed on the command line while
> > > using -o, but clang does not:
> >
> > Ah, it's because `-I` *insn't* being used more so than `-o` being present.
> > >
> > > $ make -C tools/testing/selftests TARGETS=futex
> > >
> > > $ make -C tools/testing/selftests TARGETS=futex LLVM=1
> > > clang -Wall   -g -O2 -Wall -D_GNU_SOURCE -pthread -I../include \
> > > -I../../ -I../../../../../usr/include/ -I/kselftest/usr/include \
> > > futex_wait_timeout.c ../include/futextest.h ../include/atomic.h \
> > > ../include/logging.h -lpthread -lrt -o \
> > > tools/testing/selftests/futex/functional/futex_wait_timeout
> > > clang: error: cannot specify -o when generating multiple output files
> >
> > Why aren't `-I` flags being passed? Rather than:
> >
> > $ clang ... ../include/futextest.h ../include/atomic.h ../include/logging.h ...
> >
> > shouldn't this be:
> >
> > $ clang ... -I ../include/futextest.h -I ../include/atomic.h -I
> > ../include/logging.h
>
> Okay, I see, so the fix here wouldn't be to remove the headers from
> the commandline, we should just prepend them with `-l`.

Yes; I suspect they're added for a reason. If not for the futex tests
then perhaps others, so removing them outright may allow the futex
selftests to build, but I worry stripping them out might cause more
issues down the line for building other selftests with clang, or
regress the build with GCC support even.  But maybe not?

>
> > >
> > > To fix this, remove explicit paths to headers from the commandline in
> > > lib.mk. We must explicitly remove them for x86 and binderfs as they are
> > > not filtered out by the change to lib.mk, but the compiler search paths
> > > for includes are already setup correctly, so the compiler finds the
> > > correct headers.
> > >
> > > Tested: selftests build with LLVM=1 now.
> >
> > With this patch applied
> > $ make -C tools/testing/selftests TARGETS=futex LLVM=1
> > WFM but
> > $ make -C tools/testing/selftests LLVM=1
> > fails, horribly. Are you always expected to pass TARGETS when building
> > the selftests?
>
> I specifically passed TARGETS=futex because I want to point out a
> specific example where this is in an issue as there are other errors I
> see when I build all of selftests with LLVM=1. But to answer your
> question, no, I do not think you are expected to always pass TARGETS.
>
> When I run (without this patch)
>
> $ make -C tools/testing/selftests LLVM=1
>  I get a bunch of errors as well ranging from:
> - clang: error: cannot specify -o when generating multiple output
> files <-- the specific one I'm trying to fix
> - clang: warning: argument unused during compilation: '-pie'
> [-Wunused-command-line-argument]
> - include/x86_64/processor.h:344:25: warning: variable 'xmm7' is
> uninitialized when used here [-Wuninitialized]
>
>                 return (unsigned long)xmm7;
> - fuse_mnt.c:17:10: fatal error: 'fuse.h' file not found
>
> #include <fuse.h>
>
>          ^~~~~~~~
>
> However with the patch applied, I no longer see any "clang: error:
> cannot specify -o when generating multiple output files", meaning that
> I fixed one class of errors when building with LLVM=1.
>
> Do you see a clean build currently when building selftests with
> LLVM=1?

No.

> I'm not arguing that this patch fixes *all* the errors seen,

That was my interpretation of your `Tested` tag. Perhaps it can be reworded?

> but it at least fixes one class of them. Although, it seems I went
> about fixing it in the wrong manner. I can respin this to prepend -l
> before header includes to get rid of the "clang: error: cannot specify
> -o when generating multiple output files" errors.
>
>
>
>
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Andrew Delgadillo <adelg@google.com>
> > > ---
> > >  tools/testing/selftests/filesystems/binderfs/Makefile | 2 +-
> > >  tools/testing/selftests/lib.mk                        | 2 +-
> > >  tools/testing/selftests/x86/Makefile                  | 4 ++--
> > >  3 files changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/filesystems/binderfs/Makefile b/tools/testing/selftests/filesystems/binderfs/Makefile
> > > index 8af25ae96049..58e41bd98200 100644
> > > --- a/tools/testing/selftests/filesystems/binderfs/Makefile
> > > +++ b/tools/testing/selftests/filesystems/binderfs/Makefile
> > > @@ -3,6 +3,6 @@
> > >  CFLAGS += -I../../../../../usr/include/ -pthread
> > >  TEST_GEN_PROGS := binderfs_test
> > >
> > > -binderfs_test: binderfs_test.c ../../kselftest.h ../../kselftest_harness.h
> > > +binderfs_test: binderfs_test.c
> > >
> > >  include ../../lib.mk
> > > diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
> > > index fa2ac0e56b43..fb152e20c86a 100644
> > > --- a/tools/testing/selftests/lib.mk
> > > +++ b/tools/testing/selftests/lib.mk
> > > @@ -142,7 +142,7 @@ endif
> > >  ifeq ($(OVERRIDE_TARGETS),)
> > >  LOCAL_HDRS := $(selfdir)/kselftest_harness.h $(selfdir)/kselftest.h
> > >  $(OUTPUT)/%:%.c $(LOCAL_HDRS)
> > > -       $(LINK.c) $(filter-out $(LOCAL_HDRS),$^) $(LDLIBS) -o $@
> > > +       $(LINK.c) $(filter-out %.h,$^) $(LDLIBS) -o $@
> >
> > What? Aren't kselftest.h and kselftest_harness.h already part of
> > LOCAL_HDRS?  Perhaps that filter-out is broken, or LOCAL_HDRS.  Yeah,
> > adding some debugging:
> >
> > diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
> > index fe7ee2b0f29c..827f766d6057 100644
> > --- a/tools/testing/selftests/lib.mk
> > +++ b/tools/testing/selftests/lib.mk
> > @@ -142,6 +142,7 @@ endif
> >  # OVERRIDE_TARGETS = 1.
> >  ifeq ($(OVERRIDE_TARGETS),)
> >  LOCAL_HDRS := $(selfdir)/kselftest_harness.h $(selfdir)/kselftest.h
> > +$(info $$LOCAL_HDRS is [${LOCAL_HDRS}])
> >  $(OUTPUT)/%:%.c $(LOCAL_HDRS)
> >         $(LINK.c) $(filter-out $(LOCAL_HDRS),$^) $(LDLIBS) -o $@
> >
> > prints:
> >
> > $LOCAL_HDRS is [/android0/kernel-all/tools/testing/selftests/kselftest_harness.h
> > /android0/kernel-all/tools/testing/selftests/kselftest.h]
> >
> > so of course filter-out isn't going to match `../include/futextest.h
> > ../include/atomic.h ../include/logging.h`.
>
> Like you mentioned above, it seems a better way to about this would be
> to prepend -I before the includes. I'll go ahead and send a new patch
> to do that.

SGTM.  Thanks for pursuing this! ++beers_owed;

> > >
> > >  $(OUTPUT)/%.o:%.S
> > >         $(COMPILE.S) $^ -o $@
> > > diff --git a/tools/testing/selftests/x86/Makefile b/tools/testing/selftests/x86/Makefile
> > > index b4142cd1c5c2..68967006b3e9 100644
> > > --- a/tools/testing/selftests/x86/Makefile
> > > +++ b/tools/testing/selftests/x86/Makefile
> > > @@ -72,10 +72,10 @@ all_64: $(BINARIES_64)
> > >  EXTRA_CLEAN := $(BINARIES_32) $(BINARIES_64)
> > >
> > >  $(BINARIES_32): $(OUTPUT)/%_32: %.c helpers.h
> > > -       $(CC) -m32 -o $@ $(CFLAGS) $(EXTRA_CFLAGS) $^ -lrt -ldl -lm
> > > +       $(CC) -m32 -o $@ $(CFLAGS) $(EXTRA_CFLAGS) $(filter-out %.h,$^) -lrt -ldl -lm
> > >
> > >  $(BINARIES_64): $(OUTPUT)/%_64: %.c helpers.h
> > > -       $(CC) -m64 -o $@ $(CFLAGS) $(EXTRA_CFLAGS) $^ -lrt -ldl
> > > +       $(CC) -m64 -o $@ $(CFLAGS) $(EXTRA_CFLAGS) $(filter-out %.h,$^) -lrt -ldl
> > >
> > >  # x86_64 users should be encouraged to install 32-bit libraries
> > >  ifeq ($(CAN_BUILD_I386)$(CAN_BUILD_X86_64),01)
> > > --
> > > 2.33.0.800.g4c38ced690-goog
> > >
> >
> >
> > --
> > Thanks,
> > ~Nick Desaulniers



-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-10-05 18:50 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-04 23:04 [PATCH] selftests: Remove explicit headers for clang Andrew Delgadilo
2021-10-05 16:59 ` Nick Desaulniers
2021-10-05 18:43   ` Andrew Delgadillo
2021-10-05 18:50     ` Nick Desaulniers

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).