Hi, Willy
Hi Zhangjin,
On Thu, May 25, 2023 at 01:33:14AM +0800, Zhangjin Wu wrote:
Hi, Willy
Thanks very mush for your kindly review, discuss and suggestion, now we get full rv32 support ;-)
In the first series [1], we have fixed up the compile errors about _start and __NR_llseek for rv32, but left compile errors about tons of time32 syscalls (removed after kernel commit d4c08b9776b3 ("riscv: Use latest system call ABI")) and the missing fstat in nolibc-test.c [2], now we have fixed up all of them.
(...)
I have read the comments that others made on the series and overall agree. I've seen that you intend to prepare a v2. I think we must first decide how to better deal with emulated syscalls as I said in an earlier message. Probably that we should just add a specific test case for EFAULT in nolibc-test since it's the only one (I think) that risks to trigger crashes with emulated syscalls. We could also imagine dealing with the signal ourselves but I'm not that keen on going to implement signal() & longjmp() for now :-/
Yes, user-space signal() may be the right direction, we just need to let user-space not crash the kernel, what about this 'solution' for current stage (consider the pure time64 support too):
#if defined(NOLIBC) && defined(__NR_gettimeofday) && __SIZEOF_LONG__ == 8 CASE_TEST(gettimeofday_bad1); EXPECT_SYSER(1, gettimeofday((void *)1, NULL), -1, EFAULT); break; CASE_TEST(gettimeofday_bad2); EXPECT_SYSER(1, gettimeofday(NULL, (void *)1), -1, EFAULT); break; #endif
This idea is from your commit 1da02f51088 ("selftests/nolibc: support glibc as well") for glibc, but the difference is of course glibc not crashes the kernel.
Btw, since the gettimeofday_null case may be optimized by compiler and not trigger such errors:
// rv32 nolibc-test.c:(.text.run_syscall+0x8c0): undefined reference to `__divdi3'
// arm32 nolibc-test.c:(.text.run_syscall+0x820): undefined reference to `__aeabi_ldivmod'
The above errors have been hidden after the disabling of the gettimeofday_bad1 test case, so, still need to solve it before sending v2.
The method used by musl may work, but the high bits may be lost (from long long to int)?
tv->tv_usec = (int)ts.tv_nsec / 1000;
Perhaps we really need to add the missing __divdi3 and __aeabi_ldivmod and the ones for the other architectures, or get one from lib/math/div64.c.
Will add such new test cases to detect the above issues:
CASE_TEST(gettimeofday_tv); EXPECT_SYSZR(1, gettimeofday(&tv, NULL)); break; CASE_TEST(gettimeofday_tz); EXPECT_SYSZR(1, gettimeofday(NULL, &tz)); break; CASE_TEST(gettimeofday_tv_tz);EXPECT_SYSZR(1, gettimeofday(&tv, &tz)); break;
May still require to add 'used' attribute to 'struct timeval tv' and 'struct timeval tz' to let compiler not optimize them away.
For the waitid syscall based waitpid INT_MIN test case, I have prepared such code:
#define IF_TEST(name) \ if (strcmp(test, #name) == 0)
const int _errorno(const char *test) { #ifdef __NR_wait4 IF_TEST(waitpid_min); return ESRCH; #else /* __NR_waitid */ IF_TEST(waitpid_min); return EINVAL; #endif return 0; }
#define errorno(test) _errorno(#test)
CASE_TEST(waitpid_min); EXPECT_SYSER(1, waitpid(INT_MIN, &tmp, WNOHANG), -1, errorno(waitpid_min)); break;
Instead of simply disabling this case, the above code allows to return different values for different syscalls.
is a standalone errorno_waitpid_min() better? I just want to let future test cases share some code, but it may be slower ;-)
Regardless, in order to clean the things up and relieve you from the non-rv32 stuff, I've just reverted the two patches that your series reverts (1 & 2), and added the EOVERFLOW one (3). I'm pushing this to branch 20230528-nolibc-rv32+stkp5.
Thanks very much and I have seen another two have been pushed too, will rebase everything on this new branch.
Based on the other suggestions from you and Thomas, I plan to send some generic and independent changes at first, and then the left hard parts, It may simplify the whole progress.
Best regards, Zhangjin
Regards, Willy