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

---
 sysdeps/unix/sysv/linux/riscv/sysdep.h | 61 ++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

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.

 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.

  Maciej

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.

On Tue, Jul 7, 2020 at 5:09 PM Maciej W. Rozycki via Libc-alpha
<libc-alpha@sourceware.org> 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

Fixed.

>
> and then maybe explain in a little more details in the change description.

Done.

>
> 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.
>
>  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).

I have split out these changes into a seperate patch.

>
> > +/* 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 have changed this based on what Adhemerval said (the same as the ARC port).

Alistair

>
>   Maciej

On Wed, Jul 8, 2020 at 10:08 AM Adhemerval Zanella via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
>
>
> 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'm not sure if this is required. I split out the clean up into a
seperate patch and I don't see any regressions.

>
> 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.

Thanks! I have updated this to be similar to the ARC port. I also
paraphrased the points above in the commit message (I hope that is
ok).

Alistair

[Added cc's back.]

On Wed, 8 Jul 2020, Adhemerval Zanella 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.

 Fair enough, however the RISC-V port does not appear to make use of it as 
the removal of the second inclusion does not change anything (I actually 
double-checked).

  Maciej