Re: [PATCH] selftests: Remove explicit headers for clang

[Nick Desaulniers <ndesaulniers@google.com>]

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