Willy, Thomas,
On Sun, May 21, 2023 at 02:30:22AM +0800, Zhangjin Wu wrote:
...
I think what Thomas meant is that he wants to be sure the call doesn't end up as read(-1, &tmp, 1). Here you could have -EBADF or -EFAULT, it depends. Anyway other solutions can be found if necessary. Another approach could be to switch back to __NR_fstat and condition it to its definition.
Maybe we can find a new syscall to test with?
Maybe it would be worth considering pselect() or equivalent which involve many arguments. I don't know if rv32 has fstatat() or lstat() for example, that could be used as alternatives ?
Unfortuantely, none of them is available in rv32, we have the same tricks you used in another reply:
$ echo "#include <asm/unistd.h>" | \ riscv64-linux-gnu-gcc -march=rv32im -mabi=ilp32 -Wl,-melf32lriscv_ilp32 -xc - -E -dM | \ grep -E "pselect|fstat|lstat" #define __NR_pselect6_time64 413 #define __NR3264_fstatfs 44 #define __NR_fstatfs64 __NR3264_fstatfsThen probably fstatfs should work equally for this test.
I tested a change like this (try __NR_fstatfs64, __NR_fstatfs, and __NR_fstat one by one):
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 063f9959ac44..0a60e8ca5756 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -500,6 +500,16 @@ static int test_fork(void) } }
+#ifdef __NR_fstatfs64 +#define EXPECT_SYSER_SYSCALL_ARGS() EXPECT_SYSER(1, syscall(__NR_fstatfs64, 0, 0, NULL), -1, EINVAL) +#elif defined(__NR_fstatfs) +#define EXPECT_SYSER_SYSCALL_ARGS() EXPECT_SYSER(1, syscall(__NR_fstatfs, 0, NULL), -1, EFAULT) +#elif defined(__NR_fstat) +#define EXPECT_SYSER_SYSCALL_ARGS() EXPECT_SYSER(1, syscall(__NR_fstat, 0, NULL), -1, EFAULT) +#else +#error None of __NR_fstatfs64, __NR_fstatfs and __NR_fstat defined, cannot implement syscall_args test +#endif + /* Run syscall tests between IDs <min> and <max>. * Return 0 on success, non-zero on failure. */ @@ -596,7 +606,7 @@ int run_syscall(int min, int max) CASE_TEST(write_badf); EXPECT_SYSER(1, write(-1, &tmp, 1), -1, EBADF); break; CASE_TEST(write_zero); EXPECT_SYSZR(1, write(1, &tmp, 0)); break; CASE_TEST(syscall_noargs); EXPECT_SYSEQ(1, syscall(__NR_getpid), getpid()); break; - CASE_TEST(syscall_args); EXPECT_SYSER(1, syscall(__NR_fstat, 0, NULL), -1, EFAULT); break; + CASE_TEST(syscall_args); EXPECT_SYSER_SYSCALL_ARGS(); break; case __LINE__: return ret; /* must be last */ /* note: do not set any defaults so as to permit holes above */
And another change like this (try __NR_statx and then __NR_fstat):
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 063f9959ac44..d740ba405cb2 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -500,6 +500,17 @@ static int test_fork(void) } }
+static int test_syscall_args(void) +{ +#ifdef __NR_statx + return syscall(__NR_statx, 0, NULL, 0, 0, NULL); +#elif defined(__NR_fstat) + return syscall(__NR_fstat, 0, NULL); +#else +#error Neither __NR_statx nor __NR_fstat defined, cannot implement syscall_args test +#endif +} + /* Run syscall tests between IDs <min> and <max>. * Return 0 on success, non-zero on failure. */ @@ -596,7 +607,7 @@ int run_syscall(int min, int max) CASE_TEST(write_badf); EXPECT_SYSER(1, write(-1, &tmp, 1), -1, EBADF); break; CASE_TEST(write_zero); EXPECT_SYSZR(1, write(1, &tmp, 0)); break; CASE_TEST(syscall_noargs); EXPECT_SYSEQ(1, syscall(__NR_getpid), getpid()); break; - CASE_TEST(syscall_args); EXPECT_SYSER(1, syscall(__NR_fstat, 0, NULL), -1, EFAULT); break; + CASE_TEST(syscall_args); EXPECT_SYSER(1, test_syscall_args(), -1, EFAULT); break; case __LINE__: return ret; /* must be last */ /* note: do not set any defaults so as to permit holes above */
Which one is acceptable?
Or, use the rv32 test result as a crude reference:
(... trimmed to keep only the failed ones ...)
15 chmod_net = -1 ENOENT [FAIL] 16 chmod_self = -1 ENOENT != (-1 EPERM) [FAIL] 17 chown_self = -1 ENOENT != (-1 EPERM) [FAIL] 20 chroot_exe = -1 ENOENT != (-1 ENOTDIR) [FAIL] 30 fork = 1 ENOSYS [FAIL] 33 gettimeofday_null = -1 ENOSYS [FAIL] 35 gettimeofday_bad1 = -1 ENOSYS != (-1 EFAULT) [FAIL] 36 gettimeofday_bad2 = -1 ENOSYS != (-1 EFAULT) [FAIL] 37 gettimeofday_bad2 = -1 ENOSYS != (-1 EFAULT) [FAIL] 45 link_cross = -1 ENOENT != (-1 EXDEV) [FAIL] 51 poll_null = -1 ENOSYS [FAIL] 52 poll_stdout = -1 ENOSYS [FAIL] 53 poll_fault = -1 ENOSYS != (-1 EFAULT) [FAIL] 56 select_null = -1 ENOSYS [FAIL] 57 select_stdout = -1 ENOSYS [FAIL] 58 select_fault = -1 ENOSYS != (-1 EFAULT) [FAIL] 64 wait_child = -1 ENOSYS != (-1 ECHILD) [FAIL] 65 waitpid_min = -1 ENOSYS != (-1 ESRCH) [FAIL] 66 waitpid_child = -1 ENOSYS != (-1 ECHILD) [FAIL] Errors during this test: 19So that's a lot of failures and we should start to blindly degrade other tests just for the sake of fixing these ones here, it should be done more carefully.
Removed the config options related failures (will use defconfig to re-check them, I did use a tiny config for faster compile):
30 fork = 1 ENOSYS [FAIL] 33 gettimeofday_null = -1 ENOSYS [FAIL] 35 gettimeofday_bad1 = -1 ENOSYS != (-1 EFAULT) [FAIL] 36 gettimeofday_bad2 = -1 ENOSYS != (-1 EFAULT) [FAIL] 37 gettimeofday_bad2 = -1 ENOSYS != (-1 EFAULT) [FAIL] 51 poll_null = -1 ENOSYS [FAIL] 52 poll_stdout = -1 ENOSYS [FAIL] 53 poll_fault = -1 ENOSYS != (-1 EFAULT) [FAIL] 56 select_null = -1 ENOSYS [FAIL] 57 select_stdout = -1 ENOSYS [FAIL] 58 select_fault = -1 ENOSYS != (-1 EFAULT) [FAIL] 64 wait_child = -1 ENOSYS != (-1 ECHILD) [FAIL] 65 waitpid_min = -1 ENOSYS != (-1 ESRCH) [FAIL] 66 waitpid_child = -1 ENOSYS != (-1 ECHILD) [FAIL]
The left failed syscalls may be waitpid, wait4, gettimeofday, poll and select, it is not too many.
My preference for the short term would be the following:
- make sure we fix build issues for all platforms, including rv32
- make sure Thomas' work on syscall() and STKP works fine where it should, as it used to till now on other platforms
=> this should be added to the 6.5 queue, and for this I don't want to make this series regress as it should be queued quickly so that test code used by other developers working on 6.5 is reasonably stabilized.
Hope one of the above changes meets this goal, if no, perhaps we can simply revert my '__NR_read' patch and left it together with the other rv32 failures to next merge window.
- evaluate what needs to be done regarding time32, this implies working in the lower abstraction layers to depend on __ARCH_WANT_TIME32_SYSCALLS and use the new syscalls instead.
=> I don't know how much work it requires; if it's trivial this could possibly be for 6.5, otherwise it will have to be postponed.
My suggestion is directly fix up the failures one by one in current stage, because nolibc currently only uses part of the time32 syscalls, it may be not that complex to fix up them. Have read the code of musl and glibc today, both of them have good time64 syscalls support, I plan to fix up the above failures in several days, hope so but no promise ;-)
And another question: for the new changes, If a platform support both of the 32bit and 64bit syscalls, do we need to put the 64bit syscalls at first?
For example, the __NR_llseek we just added, do we need to check __NR_llseek before check __NR_lseek? this may support 64bit better but also may generate bigger size of code, currently, my patch checks __NR_lseek at first and then __NR_llseek to get smaller size of code, but as I read from the musl and glibc code, both of them put the 64bit syscalls path before others.
Thanks, Zhangjin
Thanks, Willy