[PATCH v2 02/18] RISC-V: Define __NR_* as __NR_*_time64/64 for 32-bit

[Adhemerval Zanella]

On 07/07/2020 21:09, Maciej W. Rozycki via Libc-alpha wrote:
> Alistair,
> 
>  I think the change heading is too cryptic and does not express the intent 
> of the change well enough.  How about:
> 
> RISC-V: Use 64-bit-time syscall numbers with the 32-bit port
> 
> and then maybe explain in a little more details in the change description.
> 
> On Wed, 3 Jun 2020, Alistair Francis via Libc-alpha wrote:
> 
>> diff --git a/sysdeps/unix/sysv/linux/riscv/sysdep.h b/sysdeps/unix/sysv/linux/riscv/sysdep.h
>> index 83e4adf6a2..aa61e8b04d 100644
>> --- a/sysdeps/unix/sysv/linux/riscv/sysdep.h
>> +++ b/sysdeps/unix/sysv/linux/riscv/sysdep.h
>> @@ -116,6 +116,67 @@
>>  
>>  #include <sysdeps/unix/sysdep.h>
> 
>  This file is weird, as it includes <sysdeps/unix/sysdep.h> twice, first 
> time indirectly via <sysdeps/unix/sysv/linux/generic/sysdep.h> at the top, 
> and then second time here.  So I think this second inclusion can be 
> removed (along with the preceding inclusion of <errno.h>, as it does not 
> appear to change anything), and the following conditional moved to the 
> top, just after the inclusion of <tls.h>.  Oddly <sysdeps/unix/sysdep.h> 
> has not been protected against multiple inclusion, but its contents do not 
> trigger compilation warnings if processed more than once.

This multiple inclusion is done internally in some places so the file
that wants to override a definition would first include the generic
definitions and then #undef/#define the new one (kernel-features.h
still does it).

I think this is kind of confusing, but this is how sysdep.h is currently
organized.  We have been changing bit per bit, but there is a lot of
place where inclusing is done without guards.

> 
>  The two #ifdef/#ifndef __ASSEMBLER__ conditionals will then become 
> adjacent and can be merged into a single #ifdef/#else one.
> 
>  This clean-up would probably better be made as a separate preceding 
> change.
> 
>> +#if __riscv_xlen == 32
> 
>  I think using __WORDSIZE here would be more consistent with the rest of 
> our code (we do use `__riscv_xlen' in a couple of places, but I think they 
> ought to be cleaned up).
> 
>> +/* Define the __NR_futex as __NR_futex64 as RV32 doesn't have a
>> + * __NR_futex syscall.
>> + */
>> +# ifndef __NR_futex
>> +#  define __NR_futex __NR_futex_time64
>> +# endif
> 
>  The comment does not match the code.
> 
>  I think it makes no sense to comment on individual entries as they all 
> repeat the same pattern and the same purpose, so an introductory comment 
> covering them all at the beginning of the conditional would be better 
> instead.  I suppose you can then reuse it for the change description too.
> 
>> +# ifndef __NR_clock_adjtime
>> +#  define __NR_clock_adjtime __NR_clock_adjtime64
>> +# endif
>> +#endif /* __riscv_xlen == 32 */
> 
>  Since you have multiple inner conditionals separated by an empty line 
> each I think it will make sense to have an empty line as well between the 
> final one and the closing of the outer conditional.  Likewise at the 
> beginning.

I think the correct way to how ARC port is doing which is:

 1. Add any required syscall suppression for 32-bit off_t/time_t on 
    fixup-asm-unistd.h so arch-syscall.h will have only the required 
    definitions.  This might be the case for riscv32 since its kernel
    ABI only supports 64-bit off_t/time_t.

 2. Regenerate arch-syscall.h if it were the case.

 3. On sysdep.h redefine only the syscall where the generic implementation
    still does not have actual 64-bit time_t support:

   /* "workarounds" for generic code needing to handle 64-bit time_t.  */

   /* Fix sysdeps/unix/sysv/linux/clock_getcpuclockid.c.  */
   #define __NR_clock_getres	__NR_clock_getres_time64
   /* Fix sysdeps/nptl/lowlevellock-futex.h.  */
   #define __NR_futex		__NR_futex_time64
   [...]

   (with proper guards it should require the #ifndef/#define)

 4. Add a comment it is a workaround to handle 64-bit time_t 
    and on each #define comment for which implementation it intends to.