linux-kernel - Re: [PATCH] selftests: Remove explicit headers for clang

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 5 Oct 2021 09:59:34 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Andrew Delgadilo <adelg@...gle.com>
Cc:     Shuah Khan <shuah@...nel.org>, linux-kselftest@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Nathan Chancellor <nathan@...nel.org>, gthelen@...gle.com,
        stable@...r.kernel.org
Subject: Re: [PATCH] selftests: Remove explicit headers for clang

On Mon, Oct 4, 2021 at 4:04 PM Andrew Delgadilo <adelg@...gle.com> wrote:
>
> From: Andrew Delgadillo <adelg@...gle.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@...r.kernel.org
> Signed-off-by: Andrew Delgadillo <adelg@...gle.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