[v2,09/18] RISC-V: The ABI implementation for 32-bit

  From: Zong Li <zongbox@gmail.com>

This patch adds the ABI implementation about 32 bit version. It contains
the Linux-specific and RISC-V architecture code, I've collected here.
---
 sysdeps/riscv/bits/wordsize.h                 |  4 +-
 sysdeps/riscv/nptl/bits/pthreadtypes-arch.h   | 10 +++-
 sysdeps/riscv/nptl/bits/struct_rwlock.h       | 27 +++++++++-
 sysdeps/riscv/sfp-machine.h                   | 27 +++++++++-
 sysdeps/riscv/sys/asm.h                       |  5 +-
 .../unix/sysv/linux/riscv/jmp_buf-macros.h    | 53 +++++++++++++++++++
 6 files changed, 121 insertions(+), 5 deletions(-)
 create mode 100644 sysdeps/unix/sysv/linux/riscv/jmp_buf-macros.h
  

On Wed, 3 Jun 2020, Alistair Francis via Libc-alpha wrote:

> This patch adds the ABI implementation about 32 bit version. It contains
> the Linux-specific and RISC-V architecture code, I've collected here.

 I think this might be a little better worded, and perhaps discuss briefly 
at least some choices made.

> diff --git a/sysdeps/riscv/bits/wordsize.h b/sysdeps/riscv/bits/wordsize.h
> index faccc71828..ee430d9036 100644
> --- a/sysdeps/riscv/bits/wordsize.h
> +++ b/sysdeps/riscv/bits/wordsize.h
> @@ -25,5 +25,7 @@
>  #if __riscv_xlen == 64
>  # define __WORDSIZE_TIME64_COMPAT32 1
>  #else
> -# error "rv32i-based targets are not supported"
> +# define __WORDSIZE_TIME64_COMPAT32 0

 Hmm, this will be problematic on RV64 hardware running mixed RV64/RV32 
userland.  This is because files like /var/log/lastlog or /var/log/wtmp 
might then be processed by both RV64 and RV32 executables, meaning that 
the interpretation will be wrong for executables using the other format.

 We made the wrong choice with RV64, anticipating that `__time_t' will be 
32-bit on RV32 and will have to live with that, by following whatever the 
solution is for ports that hold 32-bit time records in the affected 
structures.  For now I think that means we have to keep the RV32 time 
records 32-bit, i.e. set __WORDSIZE_TIME64_COMPAT32 to 1, consistently 
with RV64.

 I note there is an extensive discussion on the way to move forward here: 
<https://sourceware.org/glibc/wiki/Y2038ProofnessDesign#utmp_types_and_APIs> 
We might as well try to implement it right away, so as to avoid being 
limited to 32-bit time records here.

 At least these records are not supposed to refer to the future, so 
nothing will be broken right now, however if someone for whatever reason 
wants to keep a single login record for their system past 2038, then 
they'll have an issue (a conversion tool would be straightforward though).

 NB some existing ports do have __WORDSIZE_TIME64_COMPAT32 set and cleared 
for their 64-bit and 32-bit ABIs respectively, as per the note in our 
top-level bits/wordsize.h, however this reflects the state as before we 
introduced the possibility for `__time_t' to be a 64-bit type with 
`__WORDSIZE == 32' ABIs.  Given the turn of events I think the note ought 
to be updated accordingly; I gather it was missed with commit 07fe93cd9850 
("generic/typesizes.h: Add support for 32-bit arches with 64-bit types").

> +# define __WORDSIZE32_SIZE_ULONG    0
> +# define __WORDSIZE32_PTRDIFF_LONG  0

 Ack; this matches GCC's <stddef.h>.

> diff --git a/sysdeps/riscv/nptl/bits/pthreadtypes-arch.h b/sysdeps/riscv/nptl/bits/pthreadtypes-arch.h
> index c3c72d6c10..363034c38a 100644
> --- a/sysdeps/riscv/nptl/bits/pthreadtypes-arch.h
> +++ b/sysdeps/riscv/nptl/bits/pthreadtypes-arch.h
> @@ -32,7 +32,15 @@
>  # define __SIZEOF_PTHREAD_BARRIER_T 		32
>  # define __SIZEOF_PTHREAD_BARRIERATTR_T 	 4
>  #else
> -# error "rv32i-based systems are not supported"
> +# define __SIZEOF_PTHREAD_ATTR_T 		32
> +# define __SIZEOF_PTHREAD_MUTEX_T 		32
> +# define __SIZEOF_PTHREAD_MUTEXATTR_T 		 4
> +# define __SIZEOF_PTHREAD_COND_T 		48
> +# define __SIZEOF_PTHREAD_CONDATTR_T 		 4
> +# define __SIZEOF_PTHREAD_RWLOCK_T 		48
> +# define __SIZEOF_PTHREAD_RWLOCKATTR_T 		 8
> +# define __SIZEOF_PTHREAD_BARRIER_T 		20
> +# define __SIZEOF_PTHREAD_BARRIERATTR_T 	 4

 The values are correct, however some of these macros have the same 
expansion regardless of the ABI.  For clarity please place them outside 
the conditional, as other ports do.

> diff --git a/sysdeps/riscv/nptl/bits/struct_rwlock.h b/sysdeps/riscv/nptl/bits/struct_rwlock.h
> index acfaa75e1b..b478da0132 100644
> --- a/sysdeps/riscv/nptl/bits/struct_rwlock.h
> +++ b/sysdeps/riscv/nptl/bits/struct_rwlock.h
> @@ -32,14 +32,39 @@ struct __pthread_rwlock_arch_t
>    unsigned int __writers_futex;
>    unsigned int __pad3;
>    unsigned int __pad4;
> +#if __riscv_xlen == 64

 Same concern about the use of `__riscv_xlen' vs `__WORDSIZE' as before.

>    int __cur_writer;
>    int __shared;
>    unsigned long int __pad1;
>    unsigned long int __pad2;
>    unsigned int __flags;
> +#else
> +# if __BYTE_ORDER == __BIG_ENDIAN
> +  unsigned char __pad1;
> +  unsigned char __pad2;
> +  unsigned char __shared;
> +  unsigned char __flags;
> +# else
> +  unsigned char __flags;
> +  unsigned char __shared;
> +  unsigned char __pad1;
> +  unsigned char __pad2;
> +# endif
> +  int __cur_writer;
> +#endif
>  };

 I note with this change the RV32 structure will use the generic layout as 
per sysdeps/nptl/bits/struct_rwlock.h, however regrettably RV64 does not.  
Would it make sense to instead have the layout the same between RV64 and 
RV32, perhaps by redefining `__pad1' and `__pad2' in terms of `unsigned 
long long' (which would have alignment implications though) or otherwise?

 Is there any benefit from having `__flags' and `__shared' (and the 
reserve) grouped within a single 32-bit word?  I gather there is, given 
the lengths gone to to match the bit lanes across the word regardless of 
the endianness.  But what is it?

> -#define __PTHREAD_RWLOCK_INITIALIZER(__flags) \
> +#if __riscv_xlen == 64

 Again, `__riscv_xlen' vs `__WORDSIZE'.

> +# define __PTHREAD_RWLOCK_INITIALIZER(__flags) \
>    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, __flags
> +#else
> +# if __BYTE_ORDER == __BIG_ENDIAN

 I would suggest using #elif so as to reduce the number of conditionals 
and eliminate nesting.

> diff --git a/sysdeps/riscv/sfp-machine.h b/sysdeps/riscv/sfp-machine.h
> index 08a84fd701..aef8c61a67 100644
> --- a/sysdeps/riscv/sfp-machine.h
> +++ b/sysdeps/riscv/sfp-machine.h
> @@ -22,7 +22,32 @@
>  
>  #if __riscv_xlen == 32

 NB I think it's OK to keep this `__riscv_xlen' reference as soft-fp is 
largely an independent subsystem and shared between projects (at least 
libgcc and the Linux kernel).

> -# error "rv32i-based targets are not supported"
> +# define _FP_W_TYPE_SIZE		32
> +# define _FP_W_TYPE		unsigned long
> +# define _FP_WS_TYPE		signed long
> +# define _FP_I_TYPE		long

 Please align the RHS of these to the same column.

> +# define _FP_DIV_MEAT_S(R, X, Y)	_FP_DIV_MEAT_1_udiv_norm (S, R, X, Y)
> +# define _FP_DIV_MEAT_D(R, X, Y)	_FP_DIV_MEAT_2_udiv (D, R, X, Y)
> +# define _FP_DIV_MEAT_Q(R, X, Y)	_FP_DIV_MEAT_4_udiv (Q, R, X, Y)
> +
> +# define _FP_NANFRAC_S		_FP_QNANBIT_S
> +# define _FP_NANFRAC_D		_FP_QNANBIT_D, 0
> +# define _FP_NANFRAC_Q		_FP_QNANBIT_Q, 0, 0, 0

 Likewise.  There seems to be an established practice for this header 
across architectures to have no space between macro arguments or before 
the opening parenthesis.  This might help with the alignment.

 This looks otherwise OK to me (and virtually the same as libgcc's copy, 
except for the extra widening operations defined accordingly for FMA).

> diff --git a/sysdeps/unix/sysv/linux/riscv/jmp_buf-macros.h b/sysdeps/unix/sysv/linux/riscv/jmp_buf-macros.h
> new file mode 100644
> index 0000000000..7e48f24345
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/riscv/jmp_buf-macros.h

 Shouldn't it go into sysdeps/unix/sysv/linux/riscv/rv32/jmp_buf-macros.h?

> @@ -0,0 +1,53 @@
> +/* jump buffer constants for RISC-V

 Please make it a proper sentence.

> +   Copyright (C) 2017-2020 Free Software Foundation, Inc.

 This is a new file so just 2020 should do.

> +/* Produced by this program:
> +
> +   #include <stdio.h>
> +   #include <unistd.h>
> +   #include <setjmp.h>
> +   #include <stddef.h>
> +
> +   int main (int argc, char **argv)
> +   {
> +       printf ("#define JMP_BUF_SIZE %d\n", sizeof (jmp_buf));
> +       printf ("#define JMP_BUF_ALIGN %d\n", __alignof__ (jmp_buf));
> +       printf ("#define SIGJMP_BUF_SIZE %d\n", sizeof (sigjmp_buf));
> +       printf ("#define SIGJMP_BUF_ALIGN %d\n", __alignof__ (sigjmp_buf));
> +       printf ("#define MASK_WAS_SAVED_OFFSET %d\n", offsetof (struct __jmp_buf_tag, __mask_was_saved));
> +       printf ("#define SAVED_MASK_OFFSET %d\n", offsetof (struct __jmp_buf_tag, __saved_mask));
> +   } */

 Please reformat according to the GNU coding style.

 This file is otherwise OK.

  Maciej
  

On Thu, Jul 9, 2020 at 4:33 PM Maciej W. Rozycki via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> On Wed, 3 Jun 2020, Alistair Francis via Libc-alpha wrote:
>
> > This patch adds the ABI implementation about 32 bit version. It contains
> > the Linux-specific and RISC-V architecture code, I've collected here.
>
>  I think this might be a little better worded, and perhaps discuss briefly
> at least some choices made.
>
> > diff --git a/sysdeps/riscv/bits/wordsize.h b/sysdeps/riscv/bits/wordsize.h
> > index faccc71828..ee430d9036 100644
> > --- a/sysdeps/riscv/bits/wordsize.h
> > +++ b/sysdeps/riscv/bits/wordsize.h
> > @@ -25,5 +25,7 @@
> >  #if __riscv_xlen == 64
> >  # define __WORDSIZE_TIME64_COMPAT32 1
> >  #else
> > -# error "rv32i-based targets are not supported"
> > +# define __WORDSIZE_TIME64_COMPAT32 0
>
>  Hmm, this will be problematic on RV64 hardware running mixed RV64/RV32
> userland.  This is because files like /var/log/lastlog or /var/log/wtmp
> might then be processed by both RV64 and RV32 executables, meaning that
> the interpretation will be wrong for executables using the other format.
>
>  We made the wrong choice with RV64, anticipating that `__time_t' will be
> 32-bit on RV32 and will have to live with that, by following whatever the
> solution is for ports that hold 32-bit time records in the affected
> structures.  For now I think that means we have to keep the RV32 time
> records 32-bit, i.e. set __WORDSIZE_TIME64_COMPAT32 to 1, consistently
> with RV64.

Aw :(

I have changed this so __WORDSIZE_TIME64_COMPAT32 is 1 for both RV64 and RV32.

>
>  I note there is an extensive discussion on the way to move forward here:
> <https://sourceware.org/glibc/wiki/Y2038ProofnessDesign#utmp_types_and_APIs>
> We might as well try to implement it right away, so as to avoid being
> limited to 32-bit time records here.

Is there an advantage of doing it now or can we put this off for the
next release?

>
>  At least these records are not supposed to refer to the future, so
> nothing will be broken right now, however if someone for whatever reason
> wants to keep a single login record for their system past 2038, then
> they'll have an issue (a conversion tool would be straightforward though).
>
>  NB some existing ports do have __WORDSIZE_TIME64_COMPAT32 set and cleared
> for their 64-bit and 32-bit ABIs respectively, as per the note in our
> top-level bits/wordsize.h, however this reflects the state as before we
> introduced the possibility for `__time_t' to be a 64-bit type with
> `__WORDSIZE == 32' ABIs.  Given the turn of events I think the note ought
> to be updated accordingly; I gather it was missed with commit 07fe93cd9850
> ("generic/typesizes.h: Add support for 32-bit arches with 64-bit types").
>
> > +# define __WORDSIZE32_SIZE_ULONG    0
> > +# define __WORDSIZE32_PTRDIFF_LONG  0
>
>  Ack; this matches GCC's <stddef.h>.
>
> > diff --git a/sysdeps/riscv/nptl/bits/pthreadtypes-arch.h b/sysdeps/riscv/nptl/bits/pthreadtypes-arch.h
> > index c3c72d6c10..363034c38a 100644
> > --- a/sysdeps/riscv/nptl/bits/pthreadtypes-arch.h
> > +++ b/sysdeps/riscv/nptl/bits/pthreadtypes-arch.h
> > @@ -32,7 +32,15 @@
> >  # define __SIZEOF_PTHREAD_BARRIER_T          32
> >  # define __SIZEOF_PTHREAD_BARRIERATTR_T       4
> >  #else
> > -# error "rv32i-based systems are not supported"
> > +# define __SIZEOF_PTHREAD_ATTR_T             32
> > +# define __SIZEOF_PTHREAD_MUTEX_T            32
> > +# define __SIZEOF_PTHREAD_MUTEXATTR_T                 4
> > +# define __SIZEOF_PTHREAD_COND_T             48
> > +# define __SIZEOF_PTHREAD_CONDATTR_T                  4
> > +# define __SIZEOF_PTHREAD_RWLOCK_T           48
> > +# define __SIZEOF_PTHREAD_RWLOCKATTR_T                8
> > +# define __SIZEOF_PTHREAD_BARRIER_T          20
> > +# define __SIZEOF_PTHREAD_BARRIERATTR_T       4
>
>  The values are correct, however some of these macros have the same
> expansion regardless of the ABI.  For clarity please place them outside
> the conditional, as other ports do.

Fixed.

>
> > diff --git a/sysdeps/riscv/nptl/bits/struct_rwlock.h b/sysdeps/riscv/nptl/bits/struct_rwlock.h
> > index acfaa75e1b..b478da0132 100644
> > --- a/sysdeps/riscv/nptl/bits/struct_rwlock.h
> > +++ b/sysdeps/riscv/nptl/bits/struct_rwlock.h
> > @@ -32,14 +32,39 @@ struct __pthread_rwlock_arch_t
> >    unsigned int __writers_futex;
> >    unsigned int __pad3;
> >    unsigned int __pad4;
> > +#if __riscv_xlen == 64
>
>  Same concern about the use of `__riscv_xlen' vs `__WORDSIZE' as before.

Fixed.

>
> >    int __cur_writer;
> >    int __shared;
> >    unsigned long int __pad1;
> >    unsigned long int __pad2;
> >    unsigned int __flags;
> > +#else
> > +# if __BYTE_ORDER == __BIG_ENDIAN
> > +  unsigned char __pad1;
> > +  unsigned char __pad2;
> > +  unsigned char __shared;
> > +  unsigned char __flags;
> > +# else
> > +  unsigned char __flags;
> > +  unsigned char __shared;
> > +  unsigned char __pad1;
> > +  unsigned char __pad2;
> > +# endif
> > +  int __cur_writer;
> > +#endif
> >  };
>
>  I note with this change the RV32 structure will use the generic layout as
> per sysdeps/nptl/bits/struct_rwlock.h, however regrettably RV64 does not.
> Would it make sense to instead have the layout the same between RV64 and
> RV32, perhaps by redefining `__pad1' and `__pad2' in terms of `unsigned
> long long' (which would have alignment implications though) or otherwise?

I'm not sure which one is better. On one hand it seems better to be
more generic and therefore RV32 should use the generic interface. On
the other hand the more similar they are the better. I'm still leaning
towards we should be generic where possible.

>
>  Is there any benefit from having `__flags' and `__shared' (and the
> reserve) grouped within a single 32-bit word?  I gather there is, given
> the lengths gone to to match the bit lanes across the word regardless of
> the endianness.  But what is it?

I have no idea.

>
> > -#define __PTHREAD_RWLOCK_INITIALIZER(__flags) \
> > +#if __riscv_xlen == 64
>
>  Again, `__riscv_xlen' vs `__WORDSIZE'.

Fixed.

>
> > +# define __PTHREAD_RWLOCK_INITIALIZER(__flags) \
> >    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, __flags
> > +#else
> > +# if __BYTE_ORDER == __BIG_ENDIAN
>
>  I would suggest using #elif so as to reduce the number of conditionals
> and eliminate nesting.

Fixed.

>
> > diff --git a/sysdeps/riscv/sfp-machine.h b/sysdeps/riscv/sfp-machine.h
> > index 08a84fd701..aef8c61a67 100644
> > --- a/sysdeps/riscv/sfp-machine.h
> > +++ b/sysdeps/riscv/sfp-machine.h
> > @@ -22,7 +22,32 @@
> >
> >  #if __riscv_xlen == 32
>
>  NB I think it's OK to keep this `__riscv_xlen' reference as soft-fp is
> largely an independent subsystem and shared between projects (at least
> libgcc and the Linux kernel).

No changed here then

>
> > -# error "rv32i-based targets are not supported"
> > +# define _FP_W_TYPE_SIZE             32
> > +# define _FP_W_TYPE          unsigned long
> > +# define _FP_WS_TYPE         signed long
> > +# define _FP_I_TYPE          long
>
>  Please align the RHS of these to the same column.

Fixed.

>
> > +# define _FP_DIV_MEAT_S(R, X, Y)     _FP_DIV_MEAT_1_udiv_norm (S, R, X, Y)
> > +# define _FP_DIV_MEAT_D(R, X, Y)     _FP_DIV_MEAT_2_udiv (D, R, X, Y)
> > +# define _FP_DIV_MEAT_Q(R, X, Y)     _FP_DIV_MEAT_4_udiv (Q, R, X, Y)
> > +
> > +# define _FP_NANFRAC_S               _FP_QNANBIT_S
> > +# define _FP_NANFRAC_D               _FP_QNANBIT_D, 0
> > +# define _FP_NANFRAC_Q               _FP_QNANBIT_Q, 0, 0, 0
>
>  Likewise.  There seems to be an established practice for this header
> across architectures to have no space between macro arguments or before
> the opening parenthesis.  This might help with the alignment.

I still think it makes sense to follow the glibc style though, even if
other archs don't.

Let me know if it should be a different way and I'll update it.

>
>  This looks otherwise OK to me (and virtually the same as libgcc's copy,
> except for the extra widening operations defined accordingly for FMA).

Great!

>
> > diff --git a/sysdeps/unix/sysv/linux/riscv/jmp_buf-macros.h b/sysdeps/unix/sysv/linux/riscv/jmp_buf-macros.h
> > new file mode 100644
> > index 0000000000..7e48f24345
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/riscv/jmp_buf-macros.h
>
>  Shouldn't it go into sysdeps/unix/sysv/linux/riscv/rv32/jmp_buf-macros.h?
>
> > @@ -0,0 +1,53 @@
> > +/* jump buffer constants for RISC-V
>
>  Please make it a proper sentence.

Fixed

>
> > +   Copyright (C) 2017-2020 Free Software Foundation, Inc.
>
>  This is a new file so just 2020 should do.

Fixed

>
> > +/* Produced by this program:
> > +
> > +   #include <stdio.h>
> > +   #include <unistd.h>
> > +   #include <setjmp.h>
> > +   #include <stddef.h>
> > +
> > +   int main (int argc, char **argv)
> > +   {
> > +       printf ("#define JMP_BUF_SIZE %d\n", sizeof (jmp_buf));
> > +       printf ("#define JMP_BUF_ALIGN %d\n", __alignof__ (jmp_buf));
> > +       printf ("#define SIGJMP_BUF_SIZE %d\n", sizeof (sigjmp_buf));
> > +       printf ("#define SIGJMP_BUF_ALIGN %d\n", __alignof__ (sigjmp_buf));
> > +       printf ("#define MASK_WAS_SAVED_OFFSET %d\n", offsetof (struct __jmp_buf_tag, __mask_was_saved));
> > +       printf ("#define SAVED_MASK_OFFSET %d\n", offsetof (struct __jmp_buf_tag, __saved_mask));
> > +   } */
>
>  Please reformat according to the GNU coding style.

Fixed

Alistair

>
>  This file is otherwise OK.
>
>   Maciej
  

On Fri, 10 Jul 2020, Alistair Francis wrote:

> >  I note there is an extensive discussion on the way to move forward here:
> > <https://sourceware.org/glibc/wiki/Y2038ProofnessDesign#utmp_types_and_APIs>
> > We might as well try to implement it right away, so as to avoid being
> > limited to 32-bit time records here.
> 
> Is there an advantage of doing it now or can we put this off for the
> next release?

 The change is major enough it'll have to wait for the next development 
cycle anyway.  It shouldn't matter that much for RV32 glibc deployments 
though, given the amount of suitable hardware available.

> >  NB some existing ports do have __WORDSIZE_TIME64_COMPAT32 set and cleared
> > for their 64-bit and 32-bit ABIs respectively, as per the note in our
> > top-level bits/wordsize.h, however this reflects the state as before we
> > introduced the possibility for `__time_t' to be a 64-bit type with
> > `__WORDSIZE == 32' ABIs.  Given the turn of events I think the note ought
> > to be updated accordingly; I gather it was missed with commit 07fe93cd9850
> > ("generic/typesizes.h: Add support for 32-bit arches with 64-bit types").

 Will you be able to look into it?

 The context here is before Y2038 changes __WORDSIZE_TIME64_COMPAT32 would 
only be clear for 64-bit ABIs with those 64-bit systems that do not have 
any 32-bit ABI (compatibility mode) to support, such as the DEC Alpha.  
And it would always be clear for 32-bit ABIs, so as to use the proper 
`__time_t' type without changing the width of actual data held there in 
the structure.

 I'm not sure what the story is behind the S/390 port though; perhaps it 
does not support ABI coexistence in a single run-time environment.

> > >    int __cur_writer;
> > >    int __shared;
> > >    unsigned long int __pad1;
> > >    unsigned long int __pad2;
> > >    unsigned int __flags;
> > > +#else
> > > +# if __BYTE_ORDER == __BIG_ENDIAN
> > > +  unsigned char __pad1;
> > > +  unsigned char __pad2;
> > > +  unsigned char __shared;
> > > +  unsigned char __flags;
> > > +# else
> > > +  unsigned char __flags;
> > > +  unsigned char __shared;
> > > +  unsigned char __pad1;
> > > +  unsigned char __pad2;
> > > +# endif
> > > +  int __cur_writer;
> > > +#endif
> > >  };
> >
> >  I note with this change the RV32 structure will use the generic layout as
> > per sysdeps/nptl/bits/struct_rwlock.h, however regrettably RV64 does not.
> > Would it make sense to instead have the layout the same between RV64 and
> > RV32, perhaps by redefining `__pad1' and `__pad2' in terms of `unsigned
> > long long' (which would have alignment implications though) or otherwise?
> 
> I'm not sure which one is better. On one hand it seems better to be
> more generic and therefore RV32 should use the generic interface. On
> the other hand the more similar they are the better. I'm still leaning
> towards we should be generic where possible.

 It would be good to get a second opinion here.

> >  Is there any benefit from having `__flags' and `__shared' (and the
> > reserve) grouped within a single 32-bit word?  I gather there is, given
> > the lengths gone to to match the bit lanes across the word regardless of
> > the endianness.  But what is it?
> 
> I have no idea.

 Especially given this.

> > > +# define _FP_DIV_MEAT_S(R, X, Y)     _FP_DIV_MEAT_1_udiv_norm (S, R, X, Y)
> > > +# define _FP_DIV_MEAT_D(R, X, Y)     _FP_DIV_MEAT_2_udiv (D, R, X, Y)
> > > +# define _FP_DIV_MEAT_Q(R, X, Y)     _FP_DIV_MEAT_4_udiv (Q, R, X, Y)
> > > +
> > > +# define _FP_NANFRAC_S               _FP_QNANBIT_S
> > > +# define _FP_NANFRAC_D               _FP_QNANBIT_D, 0
> > > +# define _FP_NANFRAC_Q               _FP_QNANBIT_Q, 0, 0, 0
> >
> >  Likewise.  There seems to be an established practice for this header
> > across architectures to have no space between macro arguments or before
> > the opening parenthesis.  This might help with the alignment.
> 
> I still think it makes sense to follow the glibc style though, even if
> other archs don't.
> 
> Let me know if it should be a different way and I'll update it.

 There is the issue of the discrepancy compared to the libgcc version, and 
while `diff -l' and `patch -l' solve that for manual processing, more 
sophisticated tools may not cope and require manual intervention.

 Again, I would suggest getting a second opinion.

  Maciej
  

On Fri, Jul 10, 2020 at 6:24 PM Maciej W. Rozycki <macro@wdc.com> wrote:
>
> On Fri, 10 Jul 2020, Alistair Francis wrote:
>
> > >  I note there is an extensive discussion on the way to move forward here:
> > > <https://sourceware.org/glibc/wiki/Y2038ProofnessDesign#utmp_types_and_APIs>
> > > We might as well try to implement it right away, so as to avoid being
> > > limited to 32-bit time records here.
> >
> > Is there an advantage of doing it now or can we put this off for the
> > next release?
>
>  The change is major enough it'll have to wait for the next development
> cycle anyway.  It shouldn't matter that much for RV32 glibc deployments
> though, given the amount of suitable hardware available.
>
> > >  NB some existing ports do have __WORDSIZE_TIME64_COMPAT32 set and cleared
> > > for their 64-bit and 32-bit ABIs respectively, as per the note in our
> > > top-level bits/wordsize.h, however this reflects the state as before we
> > > introduced the possibility for `__time_t' to be a 64-bit type with
> > > `__WORDSIZE == 32' ABIs.  Given the turn of events I think the note ought
> > > to be updated accordingly; I gather it was missed with commit 07fe93cd9850
> > > ("generic/typesizes.h: Add support for 32-bit arches with 64-bit types").
>
>  Will you be able to look into it?
>
>  The context here is before Y2038 changes __WORDSIZE_TIME64_COMPAT32 would
> only be clear for 64-bit ABIs with those 64-bit systems that do not have
> any 32-bit ABI (compatibility mode) to support, such as the DEC Alpha.
> And it would always be clear for 32-bit ABIs, so as to use the proper
> `__time_t' type without changing the width of actual data held there in
> the structure.
>
>  I'm not sure what the story is behind the S/390 port though; perhaps it
> does not support ABI coexistence in a single run-time environment.

I was just about to start this but Adhemerval has very kindly sent a
patch series already. I'm going to leave this as is (defining it as 1
for RV32 and RV64).


>
> > > >    int __cur_writer;
> > > >    int __shared;
> > > >    unsigned long int __pad1;
> > > >    unsigned long int __pad2;
> > > >    unsigned int __flags;
> > > > +#else
> > > > +# if __BYTE_ORDER == __BIG_ENDIAN
> > > > +  unsigned char __pad1;
> > > > +  unsigned char __pad2;
> > > > +  unsigned char __shared;
> > > > +  unsigned char __flags;
> > > > +# else
> > > > +  unsigned char __flags;
> > > > +  unsigned char __shared;
> > > > +  unsigned char __pad1;
> > > > +  unsigned char __pad2;
> > > > +# endif
> > > > +  int __cur_writer;
> > > > +#endif
> > > >  };
> > >
> > >  I note with this change the RV32 structure will use the generic layout as
> > > per sysdeps/nptl/bits/struct_rwlock.h, however regrettably RV64 does not.
> > > Would it make sense to instead have the layout the same between RV64 and
> > > RV32, perhaps by redefining `__pad1' and `__pad2' in terms of `unsigned
> > > long long' (which would have alignment implications though) or otherwise?
> >
> > I'm not sure which one is better. On one hand it seems better to be
> > more generic and therefore RV32 should use the generic interface. On
> > the other hand the more similar they are the better. I'm still leaning
> > towards we should be generic where possible.
>
>  It would be good to get a second opinion here.

The RV64 __pthread_rwlock_arch_t is exactly the same as the AArch64
version. My guess is that it was just copied to be the same.

The generic version also has this comment

/* Generic struct for both POSIX read-write lock.  New ports are expected
   to use the default layout, however archictetures can redefine it to add
   arch-specific extensions (such as lock-elision).  The struct have a size
   of 32 bytes on both LP32 and LP64 architectures.  */

As we aren't adding any arch-specific extensions I think we should
keep the generic one. It's unfortunate we didn't do that for RV64, but
too late now.

The only downside I see in using the generic version is what happens
if the value of _shared or flags overflows a char. There are currently
only 4 flags though, so I think it's ok.

>
> > >  Is there any benefit from having `__flags' and `__shared' (and the
> > > reserve) grouped within a single 32-bit word?  I gather there is, given
> > > the lengths gone to to match the bit lanes across the word regardless of
> > > the endianness.  But what is it?
> >
> > I have no idea.
>
>  Especially given this.
>
> > > > +# define _FP_DIV_MEAT_S(R, X, Y)     _FP_DIV_MEAT_1_udiv_norm (S, R, X, Y)
> > > > +# define _FP_DIV_MEAT_D(R, X, Y)     _FP_DIV_MEAT_2_udiv (D, R, X, Y)
> > > > +# define _FP_DIV_MEAT_Q(R, X, Y)     _FP_DIV_MEAT_4_udiv (Q, R, X, Y)
> > > > +
> > > > +# define _FP_NANFRAC_S               _FP_QNANBIT_S
> > > > +# define _FP_NANFRAC_D               _FP_QNANBIT_D, 0
> > > > +# define _FP_NANFRAC_Q               _FP_QNANBIT_Q, 0, 0, 0
> > >
> > >  Likewise.  There seems to be an established practice for this header
> > > across architectures to have no space between macro arguments or before
> > > the opening parenthesis.  This might help with the alignment.
> >
> > I still think it makes sense to follow the glibc style though, even if
> > other archs don't.
> >
> > Let me know if it should be a different way and I'll update it.
>
>  There is the issue of the discrepancy compared to the libgcc version, and
> while `diff -l' and `patch -l' solve that for manual processing, more
> sophisticated tools may not cope and require manual intervention.
>
>  Again, I would suggest getting a second opinion.

The current option matches the rest of the file, so I think it makes
sense to leave as is.

If this is a problem we can just change the spacing for the entire
file in a future patch.

Alistair

>
>   Maciej
  

On 10/08/2020 18:29, Alistair Francis via Libc-alpha wrote:
> On Fri, Jul 10, 2020 at 6:24 PM Maciej W. Rozycki <macro@wdc.com> wrote:
>>
>> On Fri, 10 Jul 2020, Alistair Francis wrote:
>>
>>>>  I note there is an extensive discussion on the way to move forward here:
>>>> <https://sourceware.org/glibc/wiki/Y2038ProofnessDesign#utmp_types_and_APIs>
>>>> We might as well try to implement it right away, so as to avoid being
>>>> limited to 32-bit time records here.
>>>
>>> Is there an advantage of doing it now or can we put this off for the
>>> next release?
>>
>>  The change is major enough it'll have to wait for the next development
>> cycle anyway.  It shouldn't matter that much for RV32 glibc deployments
>> though, given the amount of suitable hardware available.
>>
>>>>  NB some existing ports do have __WORDSIZE_TIME64_COMPAT32 set and cleared
>>>> for their 64-bit and 32-bit ABIs respectively, as per the note in our
>>>> top-level bits/wordsize.h, however this reflects the state as before we
>>>> introduced the possibility for `__time_t' to be a 64-bit type with
>>>> `__WORDSIZE == 32' ABIs.  Given the turn of events I think the note ought
>>>> to be updated accordingly; I gather it was missed with commit 07fe93cd9850
>>>> ("generic/typesizes.h: Add support for 32-bit arches with 64-bit types").
>>
>>  Will you be able to look into it?
>>
>>  The context here is before Y2038 changes __WORDSIZE_TIME64_COMPAT32 would
>> only be clear for 64-bit ABIs with those 64-bit systems that do not have
>> any 32-bit ABI (compatibility mode) to support, such as the DEC Alpha.
>> And it would always be clear for 32-bit ABIs, so as to use the proper
>> `__time_t' type without changing the width of actual data held there in
>> the structure.
>>
>>  I'm not sure what the story is behind the S/390 port though; perhaps it
>> does not support ABI coexistence in a single run-time environment.
> 
> I was just about to start this but Adhemerval has very kindly sent a
> patch series already. I'm going to leave this as is (defining it as 1
> for RV32 and RV64).

Don't forget to help on review ;).

> 
> 
>>
>>>>>    int __cur_writer;
>>>>>    int __shared;
>>>>>    unsigned long int __pad1;
>>>>>    unsigned long int __pad2;
>>>>>    unsigned int __flags;
>>>>> +#else
>>>>> +# if __BYTE_ORDER == __BIG_ENDIAN
>>>>> +  unsigned char __pad1;
>>>>> +  unsigned char __pad2;
>>>>> +  unsigned char __shared;
>>>>> +  unsigned char __flags;
>>>>> +# else
>>>>> +  unsigned char __flags;
>>>>> +  unsigned char __shared;
>>>>> +  unsigned char __pad1;
>>>>> +  unsigned char __pad2;
>>>>> +# endif
>>>>> +  int __cur_writer;
>>>>> +#endif
>>>>>  };
>>>>
>>>>  I note with this change the RV32 structure will use the generic layout as
>>>> per sysdeps/nptl/bits/struct_rwlock.h, however regrettably RV64 does not.
>>>> Would it make sense to instead have the layout the same between RV64 and
>>>> RV32, perhaps by redefining `__pad1' and `__pad2' in terms of `unsigned
>>>> long long' (which would have alignment implications though) or otherwise?
>>>
>>> I'm not sure which one is better. On one hand it seems better to be
>>> more generic and therefore RV32 should use the generic interface. On
>>> the other hand the more similar they are the better. I'm still leaning
>>> towards we should be generic where possible.
>>
>>  It would be good to get a second opinion here.
> 
> The RV64 __pthread_rwlock_arch_t is exactly the same as the AArch64
> version. My guess is that it was just copied to be the same.
> 
> The generic version also has this comment
> 
> /* Generic struct for both POSIX read-write lock.  New ports are expected
>    to use the default layout, however archictetures can redefine it to add
>    arch-specific extensions (such as lock-elision).  The struct have a size
>    of 32 bytes on both LP32 and LP64 architectures.  */
> 
> As we aren't adding any arch-specific extensions I think we should
> keep the generic one. It's unfortunate we didn't do that for RV64, but
> too late now.
> 
> The only downside I see in using the generic version is what happens
> if the value of _shared or flags overflows a char. There are currently
> only 4 flags though, so I think it's ok.

I come up with the generic interface by using the already one in place for
mostly 64-bit architecture and the internal layout is mainly to avoid duplicate
the internal type on multiple architectures (so we can use the generic 
implementation).

The only usage I can think of where same structure for RV64 and RV32 may
pose a role is to use a shared rwlock between a 32-bit and a 64-bit process
(pthread_rwlockattr_setpshared with PTHREAD_PROCESS_SHARED).  But afaik this
is not supported on other architectures.

> 
>>
>>>>  Is there any benefit from having `__flags' and `__shared' (and the
>>>> reserve) grouped within a single 32-bit word?  I gather there is, given
>>>> the lengths gone to to match the bit lanes across the word regardless of
>>>> the endianness.  But what is it?
>>>
>>> I have no idea.
>>
>>  Especially given this.

The internal member that are most concurrently accessed by the common operations
(pthread_rwlock_{clock,timed}{wr,rd}lock) are the __readers, __writers,
__wrphase_futex, __writers_futex, and __cur_writer.  The __shared and __flag
are either accessed with non-atomic operation or at initialization,  so I don't
think there is much different in packing or not in a 32-bit word.  My 
understanding is architectures usually does that to avoid possible internal
padding, since the usual types for both member are uint8_t.

>>
>>>>> +# define _FP_DIV_MEAT_S(R, X, Y)     _FP_DIV_MEAT_1_udiv_norm (S, R, X, Y)
>>>>> +# define _FP_DIV_MEAT_D(R, X, Y)     _FP_DIV_MEAT_2_udiv (D, R, X, Y)
>>>>> +# define _FP_DIV_MEAT_Q(R, X, Y)     _FP_DIV_MEAT_4_udiv (Q, R, X, Y)
>>>>> +
>>>>> +# define _FP_NANFRAC_S               _FP_QNANBIT_S
>>>>> +# define _FP_NANFRAC_D               _FP_QNANBIT_D, 0
>>>>> +# define _FP_NANFRAC_Q               _FP_QNANBIT_Q, 0, 0, 0
>>>>
>>>>  Likewise.  There seems to be an established practice for this header
>>>> across architectures to have no space between macro arguments or before
>>>> the opening parenthesis.  This might help with the alignment.
>>>
>>> I still think it makes sense to follow the glibc style though, even if
>>> other archs don't.
>>>
>>> Let me know if it should be a different way and I'll update it.
>>
>>  There is the issue of the discrepancy compared to the libgcc version, and
>> while `diff -l' and `patch -l' solve that for manual processing, more
>> sophisticated tools may not cope and require manual intervention.
>>
>>  Again, I would suggest getting a second opinion.
> 
> The current option matches the rest of the file, so I think it makes
> sense to leave as is.
> 
> If this is a problem we can just change the spacing for the entire
> file in a future patch.
> 
> Alistair
> 
>>
>>   Maciej
  

On Thu, Aug 27, 2020 at 12:43 PM Adhemerval Zanella via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
>
>
> On 10/08/2020 18:29, Alistair Francis via Libc-alpha wrote:
> > On Fri, Jul 10, 2020 at 6:24 PM Maciej W. Rozycki <macro@wdc.com> wrote:
> >>
> >> On Fri, 10 Jul 2020, Alistair Francis wrote:
> >>
> >>>>  I note there is an extensive discussion on the way to move forward here:
> >>>> <https://sourceware.org/glibc/wiki/Y2038ProofnessDesign#utmp_types_and_APIs>
> >>>> We might as well try to implement it right away, so as to avoid being
> >>>> limited to 32-bit time records here.
> >>>
> >>> Is there an advantage of doing it now or can we put this off for the
> >>> next release?
> >>
> >>  The change is major enough it'll have to wait for the next development
> >> cycle anyway.  It shouldn't matter that much for RV32 glibc deployments
> >> though, given the amount of suitable hardware available.
> >>
> >>>>  NB some existing ports do have __WORDSIZE_TIME64_COMPAT32 set and cleared
> >>>> for their 64-bit and 32-bit ABIs respectively, as per the note in our
> >>>> top-level bits/wordsize.h, however this reflects the state as before we
> >>>> introduced the possibility for `__time_t' to be a 64-bit type with
> >>>> `__WORDSIZE == 32' ABIs.  Given the turn of events I think the note ought
> >>>> to be updated accordingly; I gather it was missed with commit 07fe93cd9850
> >>>> ("generic/typesizes.h: Add support for 32-bit arches with 64-bit types").
> >>
> >>  Will you be able to look into it?
> >>
> >>  The context here is before Y2038 changes __WORDSIZE_TIME64_COMPAT32 would
> >> only be clear for 64-bit ABIs with those 64-bit systems that do not have
> >> any 32-bit ABI (compatibility mode) to support, such as the DEC Alpha.
> >> And it would always be clear for 32-bit ABIs, so as to use the proper
> >> `__time_t' type without changing the width of actual data held there in
> >> the structure.
> >>
> >>  I'm not sure what the story is behind the S/390 port though; perhaps it
> >> does not support ABI coexistence in a single run-time environment.
> >
> > I was just about to start this but Adhemerval has very kindly sent a
> > patch series already. I'm going to leave this as is (defining it as 1
> > for RV32 and RV64).
>
> Don't forget to help on review ;).

I'm trying :)

>
> >
> >
> >>
> >>>>>    int __cur_writer;
> >>>>>    int __shared;
> >>>>>    unsigned long int __pad1;
> >>>>>    unsigned long int __pad2;
> >>>>>    unsigned int __flags;
> >>>>> +#else
> >>>>> +# if __BYTE_ORDER == __BIG_ENDIAN
> >>>>> +  unsigned char __pad1;
> >>>>> +  unsigned char __pad2;
> >>>>> +  unsigned char __shared;
> >>>>> +  unsigned char __flags;
> >>>>> +# else
> >>>>> +  unsigned char __flags;
> >>>>> +  unsigned char __shared;
> >>>>> +  unsigned char __pad1;
> >>>>> +  unsigned char __pad2;
> >>>>> +# endif
> >>>>> +  int __cur_writer;
> >>>>> +#endif
> >>>>>  };
> >>>>
> >>>>  I note with this change the RV32 structure will use the generic layout as
> >>>> per sysdeps/nptl/bits/struct_rwlock.h, however regrettably RV64 does not.
> >>>> Would it make sense to instead have the layout the same between RV64 and
> >>>> RV32, perhaps by redefining `__pad1' and `__pad2' in terms of `unsigned
> >>>> long long' (which would have alignment implications though) or otherwise?
> >>>
> >>> I'm not sure which one is better. On one hand it seems better to be
> >>> more generic and therefore RV32 should use the generic interface. On
> >>> the other hand the more similar they are the better. I'm still leaning
> >>> towards we should be generic where possible.
> >>
> >>  It would be good to get a second opinion here.
> >
> > The RV64 __pthread_rwlock_arch_t is exactly the same as the AArch64
> > version. My guess is that it was just copied to be the same.
> >
> > The generic version also has this comment
> >
> > /* Generic struct for both POSIX read-write lock.  New ports are expected
> >    to use the default layout, however archictetures can redefine it to add
> >    arch-specific extensions (such as lock-elision).  The struct have a size
> >    of 32 bytes on both LP32 and LP64 architectures.  */
> >
> > As we aren't adding any arch-specific extensions I think we should
> > keep the generic one. It's unfortunate we didn't do that for RV64, but
> > too late now.
> >
> > The only downside I see in using the generic version is what happens
> > if the value of _shared or flags overflows a char. There are currently
> > only 4 flags though, so I think it's ok.
>
> I come up with the generic interface by using the already one in place for
> mostly 64-bit architecture and the internal layout is mainly to avoid duplicate
> the internal type on multiple architectures (so we can use the generic
> implementation).
>
> The only usage I can think of where same structure for RV64 and RV32 may
> pose a role is to use a shared rwlock between a 32-bit and a 64-bit process
> (pthread_rwlockattr_setpshared with PTHREAD_PROCESS_SHARED).  But afaik this
> is not supported on other architectures.

Thanks for describing this. I'm going to stick with the current
generic implementation for RV32 then.

>
> >
> >>
> >>>>  Is there any benefit from having `__flags' and `__shared' (and the
> >>>> reserve) grouped within a single 32-bit word?  I gather there is, given
> >>>> the lengths gone to to match the bit lanes across the word regardless of
> >>>> the endianness.  But what is it?
> >>>
> >>> I have no idea.
> >>
> >>  Especially given this.
>
> The internal member that are most concurrently accessed by the common operations
> (pthread_rwlock_{clock,timed}{wr,rd}lock) are the __readers, __writers,
> __wrphase_futex, __writers_futex, and __cur_writer.  The __shared and __flag
> are either accessed with non-atomic operation or at initialization,  so I don't
> think there is much different in packing or not in a 32-bit word.  My
> understanding is architectures usually does that to avoid possible internal
> padding, since the usual types for both member are uint8_t.

Thanks for the info here as well.

Alistair

>
> >>
> >>>>> +# define _FP_DIV_MEAT_S(R, X, Y)     _FP_DIV_MEAT_1_udiv_norm (S, R, X, Y)
> >>>>> +# define _FP_DIV_MEAT_D(R, X, Y)     _FP_DIV_MEAT_2_udiv (D, R, X, Y)
> >>>>> +# define _FP_DIV_MEAT_Q(R, X, Y)     _FP_DIV_MEAT_4_udiv (Q, R, X, Y)
> >>>>> +
> >>>>> +# define _FP_NANFRAC_S               _FP_QNANBIT_S
> >>>>> +# define _FP_NANFRAC_D               _FP_QNANBIT_D, 0
> >>>>> +# define _FP_NANFRAC_Q               _FP_QNANBIT_Q, 0, 0, 0
> >>>>
> >>>>  Likewise.  There seems to be an established practice for this header
> >>>> across architectures to have no space between macro arguments or before
> >>>> the opening parenthesis.  This might help with the alignment.
> >>>
> >>> I still think it makes sense to follow the glibc style though, even if
> >>> other archs don't.
> >>>
> >>> Let me know if it should be a different way and I'll update it.
> >>
> >>  There is the issue of the discrepancy compared to the libgcc version, and
> >> while `diff -l' and `patch -l' solve that for manual processing, more
> >> sophisticated tools may not cope and require manual intervention.
> >>
> >>  Again, I would suggest getting a second opinion.
> >
> > The current option matches the rest of the file, so I think it makes
> > sense to leave as is.
> >
> > If this is a problem we can just change the spacing for the entire
> > file in a future patch.
> >
> > Alistair
> >
> >>
> >>   Maciej