It looks like people started ignoring the compiler warnings (or even errors) when building the arm64-specific kselftests. The first three patches are printf() arguments adjustment. The last one adds ".arch_extension sme", otherwise they fail to build (with my toolchain versions at least).
Note for future kselftest contributors - try to keep them warning-free.
Catalin Marinas (4): kselftest/arm64: Fix printf() compiler warnings in the arm64 fp tests kselftest/arm64: Fix printf() warning in the arm64 MTE prctl() test kselftest/arm64: Fix printf() compiler warnings in the arm64 syscall-abi.c tests kselftest/arm64: Fix compilation of SME instructions in the arm64 fp tests
tools/testing/selftests/arm64/abi/syscall-abi.c | 8 ++++---- tools/testing/selftests/arm64/fp/sve-ptrace.c | 16 +++++++++------- tools/testing/selftests/arm64/fp/za-ptrace.c | 8 +++++--- tools/testing/selftests/arm64/fp/za-test.S | 1 + tools/testing/selftests/arm64/fp/zt-ptrace.c | 8 +++++--- tools/testing/selftests/arm64/fp/zt-test.S | 1 + tools/testing/selftests/arm64/mte/check_prctl.c | 2 +- 7 files changed, 26 insertions(+), 18 deletions(-)
Lots of incorrect length modifiers, missing arguments or conversion specifiers. Fix them.
Cc: Shuah Khan skhan@linuxfoundation.org Cc: Mark Brown broonie@kernel.org Signed-off-by: Catalin Marinas catalin.marinas@arm.com --- tools/testing/selftests/arm64/fp/sve-ptrace.c | 16 +++++++++------- tools/testing/selftests/arm64/fp/za-ptrace.c | 8 +++++--- tools/testing/selftests/arm64/fp/zt-ptrace.c | 8 +++++--- 3 files changed, 19 insertions(+), 13 deletions(-)
diff --git a/tools/testing/selftests/arm64/fp/sve-ptrace.c b/tools/testing/selftests/arm64/fp/sve-ptrace.c index 6d61992fe8a0..577b6e05e860 100644 --- a/tools/testing/selftests/arm64/fp/sve-ptrace.c +++ b/tools/testing/selftests/arm64/fp/sve-ptrace.c @@ -82,10 +82,12 @@ static void fill_buf(char *buf, size_t size) static int do_child(void) { if (ptrace(PTRACE_TRACEME, -1, NULL, NULL)) - ksft_exit_fail_msg("PTRACE_TRACEME", strerror(errno)); + ksft_exit_fail_msg("ptrace(PTRACE_TRACEME) failed: %s (%d)\n", + strerror(errno), errno);
if (raise(SIGSTOP)) - ksft_exit_fail_msg("raise(SIGSTOP)", strerror(errno)); + ksft_exit_fail_msg("raise(SIGSTOP) failed: %s (%d)\n", + strerror(errno), errno);
return EXIT_SUCCESS; } @@ -340,7 +342,7 @@ static void ptrace_set_sve_get_sve_data(pid_t child, data_size = SVE_PT_SVE_OFFSET + SVE_PT_SVE_SIZE(vq, SVE_PT_REGS_SVE); write_buf = malloc(data_size); if (!write_buf) { - ksft_test_result_fail("Error allocating %d byte buffer for %s VL %u\n", + ksft_test_result_fail("Error allocating %ld byte buffer for %s VL %u\n", data_size, type->name, vl); return; } @@ -441,7 +443,7 @@ static void ptrace_set_sve_get_fpsimd_data(pid_t child, data_size = SVE_PT_SVE_OFFSET + SVE_PT_SVE_SIZE(vq, SVE_PT_REGS_SVE); write_buf = malloc(data_size); if (!write_buf) { - ksft_test_result_fail("Error allocating %d byte buffer for %s VL %u\n", + ksft_test_result_fail("Error allocating %ld byte buffer for %s VL %u\n", data_size, type->name, vl); return; } @@ -545,7 +547,7 @@ static void ptrace_set_fpsimd_get_sve_data(pid_t child, read_sve = read_buf;
if (read_sve->vl != vl) { - ksft_test_result_fail("Child VL != expected VL %d\n", + ksft_test_result_fail("Child VL != expected VL: %u != %u\n", read_sve->vl, vl); goto out; } @@ -555,7 +557,7 @@ static void ptrace_set_fpsimd_get_sve_data(pid_t child, case SVE_PT_REGS_FPSIMD: expected_size = SVE_PT_FPSIMD_SIZE(vq, SVE_PT_REGS_FPSIMD); if (read_sve_size < expected_size) { - ksft_test_result_fail("Read %d bytes, expected %d\n", + ksft_test_result_fail("Read %ld bytes, expected %ld\n", read_sve_size, expected_size); goto out; } @@ -571,7 +573,7 @@ static void ptrace_set_fpsimd_get_sve_data(pid_t child, case SVE_PT_REGS_SVE: expected_size = SVE_PT_SVE_SIZE(vq, SVE_PT_REGS_SVE); if (read_sve_size < expected_size) { - ksft_test_result_fail("Read %d bytes, expected %d\n", + ksft_test_result_fail("Read %ld bytes, expected %ld\n", read_sve_size, expected_size); goto out; } diff --git a/tools/testing/selftests/arm64/fp/za-ptrace.c b/tools/testing/selftests/arm64/fp/za-ptrace.c index ac27d87396fc..08c777f87ea2 100644 --- a/tools/testing/selftests/arm64/fp/za-ptrace.c +++ b/tools/testing/selftests/arm64/fp/za-ptrace.c @@ -48,10 +48,12 @@ static void fill_buf(char *buf, size_t size) static int do_child(void) { if (ptrace(PTRACE_TRACEME, -1, NULL, NULL)) - ksft_exit_fail_msg("PTRACE_TRACEME", strerror(errno)); + ksft_exit_fail_msg("ptrace(PTRACE_TRACEME) failed: %s (%d)", + strerror(errno), errno);
if (raise(SIGSTOP)) - ksft_exit_fail_msg("raise(SIGSTOP)", strerror(errno)); + ksft_exit_fail_msg("raise(SIGSTOP) failed: %s (%d)\n", + strerror(errno), errno);
return EXIT_SUCCESS; } @@ -201,7 +203,7 @@ static void ptrace_set_get_data(pid_t child, unsigned int vl) data_size = ZA_PT_SIZE(vq); write_buf = malloc(data_size); if (!write_buf) { - ksft_test_result_fail("Error allocating %d byte buffer for VL %u\n", + ksft_test_result_fail("Error allocating %ld byte buffer for VL %u\n", data_size, vl); return; } diff --git a/tools/testing/selftests/arm64/fp/zt-ptrace.c b/tools/testing/selftests/arm64/fp/zt-ptrace.c index 996d9614a131..584b8d59b7ea 100644 --- a/tools/testing/selftests/arm64/fp/zt-ptrace.c +++ b/tools/testing/selftests/arm64/fp/zt-ptrace.c @@ -43,10 +43,12 @@ static void fill_buf(char *buf, size_t size) static int do_child(void) { if (ptrace(PTRACE_TRACEME, -1, NULL, NULL)) - ksft_exit_fail_msg("PTRACE_TRACEME", strerror(errno)); + ksft_exit_fail_msg("ptrace(PTRACE_TRACEME) failed: %s (%d)\n", + strerror(errno), errno);
if (raise(SIGSTOP)) - ksft_exit_fail_msg("raise(SIGSTOP)", strerror(errno)); + ksft_exit_fail_msg("raise(SIGSTOP) failed: %s (%d)\n", + strerror(errno), errno);
return EXIT_SUCCESS; } @@ -231,7 +233,7 @@ static void ptrace_enable_za_via_zt(pid_t child) /* Should have register data */ if (za_out->size < ZA_PT_SIZE(vq)) { ksft_print_msg("ZA data less than expected: %u < %u\n", - za_out->size, ZA_PT_SIZE(vq)); + za_out->size, (unsigned int)ZA_PT_SIZE(vq)); fail = true; vq = 0; }
On Fri, Nov 08, 2024 at 01:49:17PM +0000, Catalin Marinas wrote:
Lots of incorrect length modifiers, missing arguments or conversion specifiers. Fix them.
Reviewed-by: Mark Brown broonie@kernel.org
While prctl() returns an 'int', the PR_MTE_TCF_MASK is defined as unsigned long which results in the larger type following a bitwise 'and' operation. Cast the printf() argument to 'int'.
Cc: Shuah Khan skhan@linuxfoundation.org Cc: Mark Brown broonie@kernel.org Signed-off-by: Catalin Marinas catalin.marinas@arm.com --- tools/testing/selftests/arm64/mte/check_prctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/arm64/mte/check_prctl.c b/tools/testing/selftests/arm64/mte/check_prctl.c index f139a33a43ef..51e3f41a54d1 100644 --- a/tools/testing/selftests/arm64/mte/check_prctl.c +++ b/tools/testing/selftests/arm64/mte/check_prctl.c @@ -85,7 +85,7 @@ void set_mode_test(const char *name, int hwcap2, int mask) ksft_test_result_pass("%s\n", name); } else { ksft_print_msg("Got %x, expected %x\n", - (ret & PR_MTE_TCF_MASK), mask); + (int)(ret & PR_MTE_TCF_MASK), mask); ksft_test_result_fail("%s\n", name); } }
On Fri, Nov 08, 2024 at 01:49:18PM +0000, Catalin Marinas wrote:
While prctl() returns an 'int', the PR_MTE_TCF_MASK is defined as unsigned long which results in the larger type following a bitwise 'and' operation. Cast the printf() argument to 'int'.
} else { ksft_print_msg("Got %x, expected %x\n",
(ret & PR_MTE_TCF_MASK), mask);
(int)(ret & PR_MTE_TCF_MASK), mask);
Shouldn't we just use a %lx here? Casts tend to be suspicious...
On Fri, Nov 08, 2024 at 03:10:59PM +0000, Mark Brown wrote:
On Fri, Nov 08, 2024 at 01:49:18PM +0000, Catalin Marinas wrote:
While prctl() returns an 'int', the PR_MTE_TCF_MASK is defined as unsigned long which results in the larger type following a bitwise 'and' operation. Cast the printf() argument to 'int'.
} else { ksft_print_msg("Got %x, expected %x\n",
(ret & PR_MTE_TCF_MASK), mask);
(int)(ret & PR_MTE_TCF_MASK), mask);Shouldn't we just use a %lx here? Casts tend to be suspicious...
It's more like the ret is actually 32-bit and should stay like that when bits are masked out. But the bitwise op 'upgrades' it to a long (in hindsight, we should not have used UL for the TCF bits and mask).
On Fri, Nov 08, 2024 at 03:25:46PM +0000, Catalin Marinas wrote:
On Fri, Nov 08, 2024 at 03:10:59PM +0000, Mark Brown wrote:
(ret & PR_MTE_TCF_MASK), mask);
(int)(ret & PR_MTE_TCF_MASK), mask);
Shouldn't we just use a %lx here? Casts tend to be suspicious...
It's more like the ret is actually 32-bit and should stay like that when bits are masked out. But the bitwise op 'upgrades' it to a long (in hindsight, we should not have used UL for the TCF bits and mask).
Hrm, right. Possibly put the cast on PR_MTE_TCF_MASK rather than on the expression as a whole then?
On Fri, Nov 08, 2024 at 03:30:11PM +0000, Mark Brown wrote:
On Fri, Nov 08, 2024 at 03:25:46PM +0000, Catalin Marinas wrote:
On Fri, Nov 08, 2024 at 03:10:59PM +0000, Mark Brown wrote:
(ret & PR_MTE_TCF_MASK), mask);
(int)(ret & PR_MTE_TCF_MASK), mask);Shouldn't we just use a %lx here? Casts tend to be suspicious...
It's more like the ret is actually 32-bit and should stay like that when bits are masked out. But the bitwise op 'upgrades' it to a long (in hindsight, we should not have used UL for the TCF bits and mask).
Hrm, right. Possibly put the cast on PR_MTE_TCF_MASK rather than on the expression as a whole then?
Can do.
Fix the incorrect length modifiers in arm64/abi/syscall-abi.c.
Cc: Shuah Khan skhan@linuxfoundation.org Cc: Mark Brown broonie@kernel.org Signed-off-by: Catalin Marinas catalin.marinas@arm.com --- tools/testing/selftests/arm64/abi/syscall-abi.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/arm64/abi/syscall-abi.c b/tools/testing/selftests/arm64/abi/syscall-abi.c index d704511a0955..5ec9a18ec802 100644 --- a/tools/testing/selftests/arm64/abi/syscall-abi.c +++ b/tools/testing/selftests/arm64/abi/syscall-abi.c @@ -81,7 +81,7 @@ static int check_gpr(struct syscall_cfg *cfg, int sve_vl, int sme_vl, uint64_t s */ for (i = 9; i < ARRAY_SIZE(gpr_in); i++) { if (gpr_in[i] != gpr_out[i]) { - ksft_print_msg("%s SVE VL %d mismatch in GPR %d: %llx != %llx\n", + ksft_print_msg("%s SVE VL %d mismatch in GPR %d: %lx != %lx\n", cfg->name, sve_vl, i, gpr_in[i], gpr_out[i]); errors++; @@ -112,7 +112,7 @@ static int check_fpr(struct syscall_cfg *cfg, int sve_vl, int sme_vl, if (!sve_vl && !(svcr & SVCR_SM_MASK)) { for (i = 0; i < ARRAY_SIZE(fpr_in); i++) { if (fpr_in[i] != fpr_out[i]) { - ksft_print_msg("%s Q%d/%d mismatch %llx != %llx\n", + ksft_print_msg("%s Q%d/%d mismatch %lx != %lx\n", cfg->name, i / 2, i % 2, fpr_in[i], fpr_out[i]); @@ -294,13 +294,13 @@ static int check_svcr(struct syscall_cfg *cfg, int sve_vl, int sme_vl, int errors = 0;
if (svcr_out & SVCR_SM_MASK) { - ksft_print_msg("%s Still in SM, SVCR %llx\n", + ksft_print_msg("%s Still in SM, SVCR %lx\n", cfg->name, svcr_out); errors++; }
if ((svcr_in & SVCR_ZA_MASK) != (svcr_out & SVCR_ZA_MASK)) { - ksft_print_msg("%s PSTATE.ZA changed, SVCR %llx != %llx\n", + ksft_print_msg("%s PSTATE.ZA changed, SVCR %lx != %lx\n", cfg->name, svcr_in, svcr_out); errors++; }
On Fri, Nov 08, 2024 at 01:49:19PM +0000, Catalin Marinas wrote:
Fix the incorrect length modifiers in arm64/abi/syscall-abi.c.
Reviewed-by: Mark Brown broonie@kernel.org
SMSTOP/SMSTART require the SME arch extension for the assembler. Add '.arch_extension sme' to za-test.S and zt-test.S.
Cc: Shuah Khan skhan@linuxfoundation.org Cc: Mark Brown broonie@kernel.org Signed-off-by: Catalin Marinas catalin.marinas@arm.com --- tools/testing/selftests/arm64/fp/za-test.S | 1 + tools/testing/selftests/arm64/fp/zt-test.S | 1 + 2 files changed, 2 insertions(+)
diff --git a/tools/testing/selftests/arm64/fp/za-test.S b/tools/testing/selftests/arm64/fp/za-test.S index f902e6ef9077..9ebeab6ce5c0 100644 --- a/tools/testing/selftests/arm64/fp/za-test.S +++ b/tools/testing/selftests/arm64/fp/za-test.S @@ -16,6 +16,7 @@ #include "sme-inst.h"
.arch_extension sve +.arch_extension sme
#define MAXVL 2048 #define MAXVL_B (MAXVL / 8) diff --git a/tools/testing/selftests/arm64/fp/zt-test.S b/tools/testing/selftests/arm64/fp/zt-test.S index c96cb7c2ad4b..ad550463d31d 100644 --- a/tools/testing/selftests/arm64/fp/zt-test.S +++ b/tools/testing/selftests/arm64/fp/zt-test.S @@ -12,6 +12,7 @@ #include "sme-inst.h"
.arch_extension sve +.arch_extension sme
#define ZT_SZ 512 #define ZT_B (ZT_SZ / 8)
On Fri, Nov 08, 2024 at 01:49:20PM +0000, Catalin Marinas wrote:
SMSTOP/SMSTART require the SME arch extension for the assembler. Add '.arch_extension sme' to za-test.S and zt-test.S.
I've got an alternative, more idiomatic fix for this running through my testing - SME is still relatively recent so we can't rely on people's toolchains having it and we should use a .inst instead. The issue was that the toolchain I'm using locally appears to default to a full fat version of the architecture and happily assembled the SMSTART even though we don't have a .inst macro for it like we do with SMSTOP (which is why that didn't error). We only have SMSTART SM and SMSTART ZA.
On Fri, Nov 08, 2024 at 02:53:29PM +0000, Mark Brown wrote:
On Fri, Nov 08, 2024 at 01:49:20PM +0000, Catalin Marinas wrote:
SMSTOP/SMSTART require the SME arch extension for the assembler. Add '.arch_extension sme' to za-test.S and zt-test.S.
I've got an alternative, more idiomatic fix for this running through my testing - SME is still relatively recent so we can't rely on people's toolchains having it and we should use a .inst instead. The issue was that the toolchain I'm using locally appears to default to a full fat version of the architecture and happily assembled the SMSTART even though we don't have a .inst macro for it like we do with SMSTOP (which is why that didn't error). We only have SMSTART SM and SMSTART ZA.
It makes sense. Please post yours and I'll drop this one.
On Fri, Nov 08, 2024 at 01:49:16PM +0000, Catalin Marinas wrote:
It looks like people started ignoring the compiler warnings (or even errors) when building the arm64-specific kselftests. The first three patches are printf() arguments adjustment. The last one adds ".arch_extension sme", otherwise they fail to build (with my toolchain
Unfortunately there's a lot of toolchain variance with these warnings so a lot of people simply won't see them, you need a fairly fresh toolchain to see them, and IIRC GCC and clang warn about different sets of things too which doesn't help. If you look at the set of tests you're updating they're all fairly old so will predate the warnings having been added, I doubt anyone touched them recently enough to see the warnings.
On Fri, Nov 08, 2024 at 02:58:34PM +0000, Mark Brown wrote:
On Fri, Nov 08, 2024 at 01:49:16PM +0000, Catalin Marinas wrote:
It looks like people started ignoring the compiler warnings (or even errors) when building the arm64-specific kselftests. The first three patches are printf() arguments adjustment. The last one adds ".arch_extension sme", otherwise they fail to build (with my toolchain
Unfortunately there's a lot of toolchain variance with these warnings so a lot of people simply won't see them, you need a fairly fresh toolchain to see them, and IIRC GCC and clang warn about different sets of things too which doesn't help. If you look at the set of tests you're updating they're all fairly old so will predate the warnings having been added, I doubt anyone touched them recently enough to see the warnings.
But we should still fix them ;). I tried both gcc-12 and clang-14 that come with my Debian stable installation. If we see others, we'll fix them in time.
Thanks for the reviews.
On Fri, Nov 08, 2024 at 03:03:43PM +0000, Catalin Marinas wrote:
On Fri, Nov 08, 2024 at 02:58:34PM +0000, Mark Brown wrote:
On Fri, Nov 08, 2024 at 01:49:16PM +0000, Catalin Marinas wrote:
It looks like people started ignoring the compiler warnings (or even errors) when building the arm64-specific kselftests. The first three patches are printf() arguments adjustment. The last one adds ".arch_extension sme", otherwise they fail to build (with my toolchain
Unfortunately there's a lot of toolchain variance with these warnings so a lot of people simply won't see them, you need a fairly fresh toolchain
But we should still fix them ;). I tried both gcc-12 and clang-14 that come with my Debian stable installation. If we see others, we'll fix them in time.
Oh, we should definitely fix them - it's just I'm not sure people have been seeing them to be ignoring them as you mentioned above. AFAICT nobody goes on warning patrol in the selftests with any degree of frequency like happens in the main kernel, nor tunes the warning set, so they tend to build up in the older tests that aren't being updated as a matter of routine. You do sometimes get someone coming in and doing a round of cleanups but it's sporadic.
On Fri, 08 Nov 2024 13:49:16 +0000, Catalin Marinas wrote:
It looks like people started ignoring the compiler warnings (or even errors) when building the arm64-specific kselftests. The first three patches are printf() arguments adjustment. The last one adds ".arch_extension sme", otherwise they fail to build (with my toolchain versions at least).
Note for future kselftest contributors - try to keep them warning-free.
[...]
Applied to arm64 (for-next/kselftest), thanks!
[1/4] kselftest/arm64: Fix printf() compiler warnings in the arm64 fp tests https://git.kernel.org/arm64/c/b6bd50dd3b56 [2/4] kselftest/arm64: Fix printf() warning in the arm64 MTE prctl() test https://git.kernel.org/arm64/c/0cc6b94a445c [3/4] kselftest/arm64: Fix printf() compiler warnings in the arm64 syscall-abi.c tests https://git.kernel.org/arm64/c/694e2803fece
linux-kselftest-mirror@lists.linaro.org
-
Catalin Marinas -
Mark Brown