Re: [PATCH 09/13] tools/nolibc: sys_poll: riscv: use __NR_ppoll_time64 for rv32

Zhangjin Wu
2023-05-28T08:25:49Z

Hi, Arnd, Thomas, Willy

> On Fri, May 26, 2023, at 09:15, Thomas Wei=C3=9Fschuh wrote:
> > On 2023-05-25 01:57:24+0800, Zhangjin Wu wrote:
> >>
> >> +/* needed by time64 syscalls */
> >> +struct timespec64 {
> >> + time64_t tv_sec; /* seconds */
> >> + long tv_nsec; /* nanoseconds */
> >> +};
> >
> > A question to you and Willy, as it's also done the same for other types:
> >
> > What is the advantage of custom definitions over using the one from the
> > kernel (maybe via a typedef).
> >
> > From linux/time_types.h:
> >
> > struct __kernel_timespec {
> > __kernel_time64_t tv_set;
> > long long tv_nsec;
> > };
>
> I agree the __kernel_* types are what we should be using when
> interacting with system calls directly, that is definitely what
> they are intended for.
>
> I would go further here and completely drop support for 32-bit
> time_t/off_t and derived types in nolibc. Unfortunately, the
> kernel's include/uapi/linux/time.h header still defines the
> old types, this is one of the last remnants the time32 syscalls
> definitions in the kernel headers, and this already conflicts
> with the glibc and musl definitions, so anything that includes
> this header is broken on real systems. I think it makes most
> sense for nolibc to just use the linux/time_types.h header
> instead and use something like
>
> #define timespec __kernel_timespec
> #define itimerspec __kernel_itimerspec
> typedef __kernel_time64_t time_t;
> /* timeval is only provided for users, not compatible with syscalls */
> struct timeval { __kernel_time64_t tv_sec; __s64 tv_nsec; };
>
> so we can drop all the fallbacks for old 32-bit targets. This
> also allows running with CONFIG_COMPAT_32BIT_TIME disabled.

Just a status update ...

I'm working on the pure time64 and 64bit off_t syscalls support, it almost
worked (tested on rv32/64, arm32/64), thanks very much for your suggestions.

It includes:

* Based on linux/types.h and
* Use 64bit off_t
* Use 64bit time_t
* the new std.h looks like this

typedef uint32_t __kernel_dev_t;

typedef __kernel_dev_t dev_t;
typedef __kernel_ulong_t ino_t;
typedef __kernel_mode_t mode_t;
typedef __kernel_pid_t pid_t;
typedef __kernel_uid32_t uid_t;
typedef __kernel_gid32_t gid_t;
typedef __kernel_loff_t off_t;
typedef __kernel_time64_t time_t;
typedef uint32_t nlink_t;
typedef uint64_t blksize_t;
typedef uint64_t blkcnt_t;

* Use __kernel_timespec as timespec
* Use 64bit time_t based struct timeval
* Disable gettimeofday syscall completely for 32bit platforms
* And disable the gettimeofday_bad1/2 test case too
* Remove the oldselect and newslect path completely
* The new types.h looks this:

/* always use time64 structs in user-space even on 32bit platforms */
#define timespec __kernel_timespec
#define itimerspec __kernel_itimerspec

/* timeval is only provided for users, not compatible with syscalls */
struct __timeval64 {
__kernel_time64_t tv_sec; /* seconds */
__s64 tv_usec; /* microseconds */
};
/* override the 32bit version of struct timeval in linux/time.h */
#define timeval __timeval64

/* itimerval is only provided for users, not compatible with syscalls */
struct __itimerval64 {
struct __timeval64 it_interval; /* timer interval */
struct __timeval64 it_value; /* current value */
};
/* override the 32bit version of struct itimerval in linux/time.h */
#define itimerval __itimerval64

* Use __NR_*time64 for all 32bit platforms
* Use __NR_pselect6/ppoll/clock_gettime only for 64bit platforms
* New sizeof tests added to verify off_t, time_t, timespec, itimerspec...

CASE_TEST(sizeof_time_t); EXPECT_EQ(1, 8, sizeof(time_t)); break;
CASE_TEST(sizeof_timespec); EXPECT_EQ(1, 16, sizeof(struct timespec)); break;
#ifdef NOLIBC
CASE_TEST(sizeof_itimerspec); EXPECT_EQ(1, 32, sizeof(struct itimerspec)); break;
#endif
CASE_TEST(sizeof_timeval); EXPECT_EQ(1, 16, sizeof(struct timeval)); break;
CASE_TEST(sizeof_itimerval); EXPECT_EQ(1, 32, sizeof(struct itimerval)); break;
CASE_TEST(sizeof_off_t); EXPECT_EQ(1, 8, sizeof(off_t)); break;

@Arnd, the above timeval/itimerval definitions are used to override the ones
from linux/time.h to avoid such error:

error: redefinition of ‘struct timeval’

nolibc/sysroot/riscv/include/types.h:225:8: error: redefinition of ‘struct timeval’
225 | struct timeval {
| ^~~~~~~
In file included from nolibc/sysroot/riscv/include/types.h:11,
from nolibc/sysroot/riscv/include/nolibc.h:98,
from nolibc/sysroot/riscv/include/errno.h:26,
from nolibc/sysroot/riscv/include/stdio.h:14,
from tools/testing/selftests/nolibc/nolibc-test.c:12:
nolibc/sysroot/riscv/include/linux/time.h:16:8: note: originally defined here
16 | struct timeval {

@Arnd, As you commented in another reply, is it time for us to update
include/uapi/linux/time.h together and let it provide time64 timeval/itimerval
instead of the old ones? perhaps some libc's are still using them.

Or perhaps we can add a switch like __ARCH_WANT_TIME32_SYSCALLS, add a
__ARCH_WANT_TIME32_STRUCTS and simply bind it with __ARCH_WANT_TIME32_SYSCALLS?

About the above ugly override code, What's your suggestion in v2? ;-)

Best regards,
Zhangjin

>
> Arnd