Hi,
Just a bunch of build and warnings fixes that show up when building with clang. Some of these depend on each other, so I'm sending them as a series.
Changes since the first version:
1) Rebased onto Linux 6.10-rc1
Enjoy!
thanks, John Hubbard
John Hubbard (6): selftests/x86: build test_FISTTP.c with clang selftests/x86: build fsgsbase_restore.c with clang selftests/x86: build sysret_rip.c with clang selftests/x86: avoid -no-pie warnings from clang during compilation selftests/x86: remove (or use) unused variables and functions selftests/x86: fix printk warnings reported by clang
tools/testing/selftests/x86/Makefile | 10 +++++++ tools/testing/selftests/x86/amx.c | 16 ----------- .../testing/selftests/x86/clang_helpers_32.S | 11 ++++++++ .../testing/selftests/x86/clang_helpers_64.S | 28 +++++++++++++++++++ tools/testing/selftests/x86/fsgsbase.c | 6 ---- .../testing/selftests/x86/fsgsbase_restore.c | 11 ++++---- tools/testing/selftests/x86/sigreturn.c | 2 +- .../testing/selftests/x86/syscall_arg_fault.c | 1 - tools/testing/selftests/x86/sysret_rip.c | 20 ++++--------- tools/testing/selftests/x86/test_FISTTP.c | 8 +++--- tools/testing/selftests/x86/test_vsyscall.c | 15 ++++------ tools/testing/selftests/x86/vdso_restorer.c | 2 ++ 12 files changed, 72 insertions(+), 58 deletions(-) create mode 100644 tools/testing/selftests/x86/clang_helpers_32.S create mode 100644 tools/testing/selftests/x86/clang_helpers_64.S
base-commit: 2bfcfd584ff5ccc8bb7acde19b42570414bf880b prerequisite-patch-id: 39d606b9b165077aa1a3a3b0a3b396dba0c20070
First of all, in order to build with clang at all, one must first apply Valentin Obst's build fix for LLVM [1]. Once that is done, then when building with clang, via:
make LLVM=1 -C tools/testing/selftests
...the build fails because clang inline asm requires size suffixes for instructions, including the fisttp instruction. In this case, the 16-bit suffix 's' is missing.
Fix by adding an 's' (short) suffix in each place that it is missing.
[1] https://lore.kernel.org/all/20240329-selftests-libmk-llvm-rfc-v1-1-2f9ed7d1c...
Signed-off-by: John Hubbard jhubbard@nvidia.com --- tools/testing/selftests/x86/test_FISTTP.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/x86/test_FISTTP.c b/tools/testing/selftests/x86/test_FISTTP.c index 09789c0ce3e9..b9ae9d8cebcb 100644 --- a/tools/testing/selftests/x86/test_FISTTP.c +++ b/tools/testing/selftests/x86/test_FISTTP.c @@ -25,7 +25,7 @@ int test(void) feclearexcept(FE_DIVBYZERO|FE_INEXACT|FE_INVALID|FE_OVERFLOW|FE_UNDERFLOW); asm volatile ("\n" " fld1""\n" - " fisttp res16""\n" + " fisttps res16""\n" " fld1""\n" " fisttpl res32""\n" " fld1""\n" @@ -45,7 +45,7 @@ int test(void) feclearexcept(FE_DIVBYZERO|FE_INEXACT|FE_INVALID|FE_OVERFLOW|FE_UNDERFLOW); asm volatile ("\n" " fldpi""\n" - " fisttp res16""\n" + " fisttps res16""\n" " fldpi""\n" " fisttpl res32""\n" " fldpi""\n" @@ -66,7 +66,7 @@ int test(void) asm volatile ("\n" " fldpi""\n" " fchs""\n" - " fisttp res16""\n" + " fisttps res16""\n" " fldpi""\n" " fchs""\n" " fisttpl res32""\n" @@ -88,7 +88,7 @@ int test(void) feclearexcept(FE_DIVBYZERO|FE_INEXACT|FE_INVALID|FE_OVERFLOW|FE_UNDERFLOW); asm volatile ("\n" " fldln2""\n" - " fisttp res16""\n" + " fisttps res16""\n" " fldln2""\n" " fisttpl res32""\n" " fldln2""\n"
First of all, in order to build with clang at all, one must first apply Valentin Obst's build fix for LLVM [1]. Once that is done, then when building with clang, via:
make LLVM=1 -C tools/testing/selftests
Fix this by moving the inline asm to "pure" assembly, in two new files: clang_helpers_32.S, clang_helpers_64.S.
As a bonus, the pure asm avoids the need for ifdefs, and is now very simple and easy on the eyes.
[1] https://lore.kernel.org/all/20240329-selftests-libmk-llvm-rfc-v1-1-2f9ed7d1c...
Signed-off-by: John Hubbard jhubbard@nvidia.com --- tools/testing/selftests/x86/Makefile | 2 ++ tools/testing/selftests/x86/clang_helpers_32.S | 11 +++++++++++ tools/testing/selftests/x86/clang_helpers_64.S | 12 ++++++++++++ tools/testing/selftests/x86/fsgsbase_restore.c | 11 +++++------ 4 files changed, 30 insertions(+), 6 deletions(-) create mode 100644 tools/testing/selftests/x86/clang_helpers_32.S create mode 100644 tools/testing/selftests/x86/clang_helpers_64.S
diff --git a/tools/testing/selftests/x86/Makefile b/tools/testing/selftests/x86/Makefile index c1269466e0f8..99bc2ef84f5a 100644 --- a/tools/testing/selftests/x86/Makefile +++ b/tools/testing/selftests/x86/Makefile @@ -113,6 +113,8 @@ endef $(eval $(call extra-files,sysret_ss_attrs_64,thunks.S)) $(eval $(call extra-files,ptrace_syscall_32,raw_syscall_helper_32.S)) $(eval $(call extra-files,test_syscall_vdso_32,thunks_32.S)) +$(eval $(call extra-files,fsgsbase_restore_64,clang_helpers_64.S)) +$(eval $(call extra-files,fsgsbase_restore_32,clang_helpers_32.S))
# check_initial_reg_state is special: it needs a custom entry, and it # needs to be static so that its interpreter doesn't destroy its initial diff --git a/tools/testing/selftests/x86/clang_helpers_32.S b/tools/testing/selftests/x86/clang_helpers_32.S new file mode 100644 index 000000000000..dc16271bac70 --- /dev/null +++ b/tools/testing/selftests/x86/clang_helpers_32.S @@ -0,0 +1,11 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * 32-bit assembly helpers for asm operations that lack support in both gcc and + * clang. For example, clang asm does not support segment prefixes. + */ +.global dereference_seg_base +dereference_seg_base: + mov %fs:(0), %eax + ret + +.section .note.GNU-stack,"",%progbits diff --git a/tools/testing/selftests/x86/clang_helpers_64.S b/tools/testing/selftests/x86/clang_helpers_64.S new file mode 100644 index 000000000000..0d81c71cad97 --- /dev/null +++ b/tools/testing/selftests/x86/clang_helpers_64.S @@ -0,0 +1,12 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * 64-bit assembly helpers for asm operations that lack support in both gcc and + * clang. For example, clang asm does not support segment prefixes. + */ +.global dereference_seg_base + +dereference_seg_base: + mov %gs:(0), %rax + ret + +.section .note.GNU-stack,"",%progbits diff --git a/tools/testing/selftests/x86/fsgsbase_restore.c b/tools/testing/selftests/x86/fsgsbase_restore.c index 6fffadc51579..224058c1e4b2 100644 --- a/tools/testing/selftests/x86/fsgsbase_restore.c +++ b/tools/testing/selftests/x86/fsgsbase_restore.c @@ -39,12 +39,11 @@ # define SEG "%fs" #endif
-static unsigned int dereference_seg_base(void) -{ - int ret; - asm volatile ("mov %" SEG ":(0), %0" : "=rm" (ret)); - return ret; -} +/* + * Defined in clang_helpers_[32|64].S, because unlike gcc, clang inline asm does + * not support segmentation prefixes. + */ +unsigned int dereference_seg_base(void);
static void init_seg(void) {
First of all, in order to build with clang at all, one must first apply Valentin Obst's build fix for LLVM [1]. Once that is done, then when building with clang, via:
make LLVM=1 -C tools/testing/selftests
...the build fails because clang's inline asm doesn't support all of the features that are used in the asm() snippet in sysret_rip.c.
Fix this by moving the asm code into the clang_helpers_64.S file, where it can be built with the assembler's full set of features.
[1] https://lore.kernel.org/all/20240329-selftests-libmk-llvm-rfc-v1-1-2f9ed7d1c...
Signed-off-by: John Hubbard jhubbard@nvidia.com --- tools/testing/selftests/x86/Makefile | 1 + .../testing/selftests/x86/clang_helpers_64.S | 16 +++++++++++++++ tools/testing/selftests/x86/sysret_rip.c | 20 ++++++------------- 3 files changed, 23 insertions(+), 14 deletions(-)
diff --git a/tools/testing/selftests/x86/Makefile b/tools/testing/selftests/x86/Makefile index 99bc2ef84f5a..d0bb32bd5538 100644 --- a/tools/testing/selftests/x86/Makefile +++ b/tools/testing/selftests/x86/Makefile @@ -115,6 +115,7 @@ $(eval $(call extra-files,ptrace_syscall_32,raw_syscall_helper_32.S)) $(eval $(call extra-files,test_syscall_vdso_32,thunks_32.S)) $(eval $(call extra-files,fsgsbase_restore_64,clang_helpers_64.S)) $(eval $(call extra-files,fsgsbase_restore_32,clang_helpers_32.S)) +$(eval $(call extra-files,sysret_rip_64,clang_helpers_64.S))
# check_initial_reg_state is special: it needs a custom entry, and it # needs to be static so that its interpreter doesn't destroy its initial diff --git a/tools/testing/selftests/x86/clang_helpers_64.S b/tools/testing/selftests/x86/clang_helpers_64.S index 0d81c71cad97..185a69dbf39c 100644 --- a/tools/testing/selftests/x86/clang_helpers_64.S +++ b/tools/testing/selftests/x86/clang_helpers_64.S @@ -9,4 +9,20 @@ dereference_seg_base: mov %gs:(0), %rax ret
+.global test_page +.global test_syscall_insn + +.pushsection ".text", "ax" +.balign 4096 +test_page: .globl test_page + .fill 4094,1,0xcc + +test_syscall_insn: + syscall + +.ifne . - test_page - 4096 + .error "test page is not one page long" +.endif +.popsection + .section .note.GNU-stack,"",%progbits diff --git a/tools/testing/selftests/x86/sysret_rip.c b/tools/testing/selftests/x86/sysret_rip.c index 84d74be1d902..b30de9aaa6d4 100644 --- a/tools/testing/selftests/x86/sysret_rip.c +++ b/tools/testing/selftests/x86/sysret_rip.c @@ -22,21 +22,13 @@ #include <sys/mman.h> #include <assert.h>
- -asm ( - ".pushsection ".text", "ax"\n\t" - ".balign 4096\n\t" - "test_page: .globl test_page\n\t" - ".fill 4094,1,0xcc\n\t" - "test_syscall_insn:\n\t" - "syscall\n\t" - ".ifne . - test_page - 4096\n\t" - ".error "test page is not one page long"\n\t" - ".endif\n\t" - ".popsection" - ); - +/* + * These items are in clang_helpers_64.S, in order to avoid clang inline asm + * limitations: + */ +void test_syscall_ins(void); extern const char test_page[]; + static void const *current_test_page_addr = test_page;
static void sethandler(int sig, void (*handler)(int, siginfo_t *, void *),
First of all, in order to build with clang at all, one must first apply Valentin Obst's build fix for LLVM [1]. Once that is done, then when building with clang, via:
make LLVM=1 -C tools/testing/selftests
...clang warns that -no-pie is "unused during compilation".
This occurs because clang only wants to see -no-pie during linking. Here, we don't have a separate linking stage, so a compiler warning is unavoidable without (wastefully) restructuring the Makefile.
Avoid the warning by simply disabling that warning, for clang builds.
[1] https://lore.kernel.org/all/20240329-selftests-libmk-llvm-rfc-v1-1-2f9ed7d1c...
Signed-off-by: John Hubbard jhubbard@nvidia.com --- tools/testing/selftests/x86/Makefile | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/tools/testing/selftests/x86/Makefile b/tools/testing/selftests/x86/Makefile index d0bb32bd5538..5c8757a25998 100644 --- a/tools/testing/selftests/x86/Makefile +++ b/tools/testing/selftests/x86/Makefile @@ -40,6 +40,13 @@ CFLAGS := -O2 -g -std=gnu99 -pthread -Wall $(KHDR_INCLUDES) # call32_from_64 in thunks.S uses absolute addresses. ifeq ($(CAN_BUILD_WITH_NOPIE),1) CFLAGS += -no-pie + +ifneq ($(LLVM),) +# clang only wants to see -no-pie during linking. Here, we don't have a separate +# linking stage, so a compiler warning is unavoidable without (wastefully) +# restructuring the Makefile. Avoid this by simply disabling that warning. +CFLAGS += -Wno-unused-command-line-argument +endif endif
define gen-target-rule-32
First of all, in order to build with clang at all, one must first apply Valentin Obst's build fix for LLVM [1]. Once that is done, then when building with clang, via:
make LLVM=1 -C tools/testing/selftests
...quite a few functions are variables are generating "unused" warnings. Fix the warnings by deleting the unused items.
One item, the "nerrs" variable in vsdo_restorer.c's main(), is unused but probably wants to be returned from main(), as a non-zero result. That result is also unused right now, so another option would be to delete it entirely, but this way, main() also gets fixed. It was missing a return value.
[1] https://lore.kernel.org/all/20240329-selftests-libmk-llvm-rfc-v1-1-2f9ed7d1c...
Signed-off-by: John Hubbard jhubbard@nvidia.com --- tools/testing/selftests/x86/amx.c | 16 ---------------- tools/testing/selftests/x86/fsgsbase.c | 6 ------ tools/testing/selftests/x86/syscall_arg_fault.c | 1 - tools/testing/selftests/x86/test_vsyscall.c | 5 ----- tools/testing/selftests/x86/vdso_restorer.c | 2 ++ 5 files changed, 2 insertions(+), 28 deletions(-)
diff --git a/tools/testing/selftests/x86/amx.c b/tools/testing/selftests/x86/amx.c index 95aad6d8849b..1fdf35a4d7f6 100644 --- a/tools/testing/selftests/x86/amx.c +++ b/tools/testing/selftests/x86/amx.c @@ -39,16 +39,6 @@ struct xsave_buffer { }; };
-static inline uint64_t xgetbv(uint32_t index) -{ - uint32_t eax, edx; - - asm volatile("xgetbv;" - : "=a" (eax), "=d" (edx) - : "c" (index)); - return eax + ((uint64_t)edx << 32); -} - static inline void xsave(struct xsave_buffer *xbuf, uint64_t rfbm) { uint32_t rfbm_lo = rfbm; @@ -164,12 +154,6 @@ static inline void clear_xstate_header(struct xsave_buffer *buffer) memset(&buffer->header, 0, sizeof(buffer->header)); }
-static inline uint64_t get_xstatebv(struct xsave_buffer *buffer) -{ - /* XSTATE_BV is at the beginning of the header: */ - return *(uint64_t *)&buffer->header; -} - static inline void set_xstatebv(struct xsave_buffer *buffer, uint64_t bv) { /* XSTATE_BV is at the beginning of the header: */ diff --git a/tools/testing/selftests/x86/fsgsbase.c b/tools/testing/selftests/x86/fsgsbase.c index 8c780cce941d..50cf32de6313 100644 --- a/tools/testing/selftests/x86/fsgsbase.c +++ b/tools/testing/selftests/x86/fsgsbase.c @@ -109,11 +109,6 @@ static inline void wrgsbase(unsigned long gsbase) asm volatile("wrgsbase %0" :: "r" (gsbase) : "memory"); }
-static inline void wrfsbase(unsigned long fsbase) -{ - asm volatile("wrfsbase %0" :: "r" (fsbase) : "memory"); -} - enum which_base { FS, GS };
static unsigned long read_base(enum which_base which) @@ -212,7 +207,6 @@ static void mov_0_gs(unsigned long initial_base, bool schedule) }
static volatile unsigned long remote_base; -static volatile bool remote_hard_zero; static volatile unsigned int ftx;
/* diff --git a/tools/testing/selftests/x86/syscall_arg_fault.c b/tools/testing/selftests/x86/syscall_arg_fault.c index 461fa41a4d02..48ab065a76f9 100644 --- a/tools/testing/selftests/x86/syscall_arg_fault.c +++ b/tools/testing/selftests/x86/syscall_arg_fault.c @@ -29,7 +29,6 @@ static void sethandler(int sig, void (*handler)(int, siginfo_t *, void *), err(1, "sigaction"); }
-static volatile sig_atomic_t sig_traps; static sigjmp_buf jmpbuf;
static volatile sig_atomic_t n_errs; diff --git a/tools/testing/selftests/x86/test_vsyscall.c b/tools/testing/selftests/x86/test_vsyscall.c index d4c8e8d79d38..1c9895cfc660 100644 --- a/tools/testing/selftests/x86/test_vsyscall.c +++ b/tools/testing/selftests/x86/test_vsyscall.c @@ -97,11 +97,6 @@ static inline long sys_gtod(struct timeval *tv, struct timezone *tz) return syscall(SYS_gettimeofday, tv, tz); }
-static inline int sys_clock_gettime(clockid_t id, struct timespec *ts) -{ - return syscall(SYS_clock_gettime, id, ts); -} - static inline long sys_time(time_t *t) { return syscall(SYS_time, t); diff --git a/tools/testing/selftests/x86/vdso_restorer.c b/tools/testing/selftests/x86/vdso_restorer.c index fe99f2434155..ac8d8e1e9805 100644 --- a/tools/testing/selftests/x86/vdso_restorer.c +++ b/tools/testing/selftests/x86/vdso_restorer.c @@ -92,4 +92,6 @@ int main() printf("[FAIL]\t!SA_SIGINFO handler was not called\n"); nerrs++; } + + return nerrs; }
These warnings are all of the form, "the format specified a short (signed or unsigned) int, but the value is a full length int".
Signed-off-by: John Hubbard jhubbard@nvidia.com --- tools/testing/selftests/x86/sigreturn.c | 2 +- tools/testing/selftests/x86/test_vsyscall.c | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/x86/sigreturn.c b/tools/testing/selftests/x86/sigreturn.c index 5d7961a5f7f6..0b75b29f794b 100644 --- a/tools/testing/selftests/x86/sigreturn.c +++ b/tools/testing/selftests/x86/sigreturn.c @@ -487,7 +487,7 @@ static void sigtrap(int sig, siginfo_t *info, void *ctx_void) greg_t asm_ss = ctx->uc_mcontext.gregs[REG_CX]; if (asm_ss != sig_ss && sig == SIGTRAP) { /* Sanity check failure. */ - printf("[FAIL]\tSIGTRAP: ss = %hx, frame ss = %hx, ax = %llx\n", + printf("[FAIL]\tSIGTRAP: ss = %hx, frame ss = %x, ax = %llx\n", ss, *ssptr(ctx), (unsigned long long)asm_ss); nerrs++; } diff --git a/tools/testing/selftests/x86/test_vsyscall.c b/tools/testing/selftests/x86/test_vsyscall.c index 1c9895cfc660..6de11b4df458 100644 --- a/tools/testing/selftests/x86/test_vsyscall.c +++ b/tools/testing/selftests/x86/test_vsyscall.c @@ -247,7 +247,7 @@ static void test_getcpu(int cpu)
if (ret_sys == 0) { if (cpu_sys != cpu) - ksft_print_msg("syscall reported CPU %hu but should be %d\n", + ksft_print_msg("syscall reported CPU %u but should be %d\n", cpu_sys, cpu);
have_node = true; @@ -265,10 +265,10 @@ static void test_getcpu(int cpu)
if (cpu_vdso != cpu || node_vdso != node) { if (cpu_vdso != cpu) - ksft_print_msg("vDSO reported CPU %hu but should be %d\n", + ksft_print_msg("vDSO reported CPU %u but should be %d\n", cpu_vdso, cpu); if (node_vdso != node) - ksft_print_msg("vDSO reported node %hu but should be %hu\n", + ksft_print_msg("vDSO reported node %u but should be %u\n", node_vdso, node); ksft_test_result_fail("Wrong values\n"); } else { @@ -290,10 +290,10 @@ static void test_getcpu(int cpu)
if (cpu_vsys != cpu || node_vsys != node) { if (cpu_vsys != cpu) - ksft_print_msg("vsyscall reported CPU %hu but should be %d\n", + ksft_print_msg("vsyscall reported CPU %u but should be %d\n", cpu_vsys, cpu); if (node_vsys != node) - ksft_print_msg("vsyscall reported node %hu but should be %hu\n", + ksft_print_msg("vsyscall reported node %u but should be %u\n", node_vsys, node); ksft_test_result_fail("Wrong values\n"); } else {
On 5/30/24 7:58 AM, Shuah Khan wrote:
Hi Shuah,
Yes. I'm having some difficulty getting anyone from x86 to respond to these. Let me add a few more x86 maintainers to this thread in hopes of hearing something.
thanks,
On 5/27/24 14:00, John Hubbard wrote:
John, could you and Muhammad have a chat and perhaps settle on a series series that gets acks from both of you?
https://lore.kernel.org/all/20240501122918.3831734-1-usama.anjum@collabora.c...
I had Muhammad's in my queue and didn't realize we had two overlapping series' bouncing around until now.
On 5/30/24 12:46 PM, Dave Hansen wrote:
Aha OK. Muhummad, after looking through this, I see that our test_FISTTP.c fix is identical, and that's about it. My series goes a bit deeper IMHO and completely fixes all the errors; the tradeoff is that it is more intrusive. Which I think is appropriate.
Would you be OK with my posting v3 that uses your patch for test_FISTTP.c [1], and the rest of my patches for the rest?
[1] https://lore.kernel.org/all/20240501122918.3831734-7-usama.anjum@collabora.c...
thanks,
linux-kselftest-mirror@lists.linaro.org
-
Dave Hansen -
John Hubbard -
Muhammad Usama Anjum -
Shuah Khan