Re: [PATCH 00/13] tools/nolibc: riscv: Add full rv32 support

[Zhangjin Wu <falcon@tinylab.org>]

Hi, Arnd

> On Wed, May 24, 2023, at 19:59, Zhangjin Wu wrote:
> > diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
> > index c0335a84f880..00c7197dcd50 100644
> > --- a/tools/include/nolibc/sys.h
> > +++ b/tools/include/nolibc/sys.h
> > @@ -1041,8 +1041,13 @@ int sys_select(int nfds, fd_set *rfds, fd_set 
> > *wfds, fd_set *efds, struct timeva
> >  		struct timeval *t;
> >  	} arg = { .n = nfds, .r = rfds, .w = wfds, .e = efds, .t = timeout };
> >  	return my_syscall1(__NR_select, &arg);
> > -#elif defined(__ARCH_WANT_SYS_PSELECT6) && defined(__NR_pselect6)
> > +#elif defined(__ARCH_WANT_SYS_PSELECT6) && (defined(__NR_pselect6) || 
> > defined(__NR_pselect6_time64))
> > +#ifdef __NR_pselect6
> >  	struct timespec t;
> > +#else
> > +	struct timespec64 t;
> > +#define __NR_pselect6 __NR_pselect6_time64
> > +#endif
> 
> Overriding the __NR_pselect6 constant seems wrong here, this can
> easily lead to more bugs, as pselect6 and pselect6_time64 are
> not compatible because of the different arguments, so anything
> using the constant after including sys.h will be broken.
>

Yes, thanks, Thomas also pointed out this issue in another reply of this
message thread, it has been fixed locally with his suggestion, it looks
like this:

    #ifdef __NR_pselect6
    	struct timespec t;
    	const long nr_pselect = __NR_pselect6;
    #else
    	struct timespec64 t;
    	const long nr_pselect = __NR_pselect6_time64;
    #endif
    
    	if (timeout) {
    		t.tv_sec  = timeout->tv_sec;
    		t.tv_nsec = timeout->tv_usec * 1000;
    	}
    	return my_syscall6(nr_pselect, nfds, rfds, wfds, efds, timeout ? &t : NULL, NULL);

I have applied this method for ppoll_time64 and clock_gettime64 too,
this method can save several duplicated lines for us, I have prepared v2
patches locally for them. 

> I would just use __kernel_timespec64 unconditionally and then
> use __NR_pselect6_time64 on all 32-bit architectures here,
> but use __NR_pselect6 on 32-bit architectures.
>

The 2nd 32-bit you mean is 64-bit?

This is related to the timespec64/time64_t definitions as you commented
in another reply. I will learn how to use the one from
<linux/time_types.h>, it may require to clean up the existing files in
tools/include/nolibc/ at first.

> I think we can also kill off the oldselect and newselect 
> cases, using pselect6/pselect6_time64 unconditionally here,
> and no longer care about building against pre-5.1 kernel
> headers, at least for the copy of nolibc that ships with the
> kernel.

As Willy commented in another reply, users may want to copy the recent
one and use them with an old kernel, even if want to drop them, a
standalone patch may be preferable.

Thanks very much,
Zhangjin

> 
>      Arnd