[v2,07/18] RISC-V: nptl: update default pthread-offsets.h

Update the RISC-V pthread-offsets.h values to support RV32.
---
 sysdeps/riscv/nptl/pthread-offsets.h | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

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

> diff --git a/sysdeps/riscv/nptl/pthread-offsets.h b/sysdeps/riscv/nptl/pthread-offsets.h
> index 31f0587bec..a85c752a1f 100644
> --- a/sysdeps/riscv/nptl/pthread-offsets.h
> +++ b/sysdeps/riscv/nptl/pthread-offsets.h
> @@ -1,3 +1,12 @@
> -#define __PTHREAD_MUTEX_KIND_OFFSET		16
> +#if __WORDSIZE == 64
> +# define __PTHREAD_MUTEX_KIND_OFFSET           16
> +#else
> +# define __PTHREAD_MUTEX_KIND_OFFSET           12
> +#endif

 OK, we have:

typedef union
{
  struct __pthread_mutex_s __data;
  char __size[__SIZEOF_PTHREAD_MUTEX_T];
  long int __align;
} pthread_mutex_t;

and then:

struct __pthread_mutex_s
{
  int __lock __LOCK_ALIGNMENT;
  unsigned int __count;
  int __owner;
#if __WORDSIZE == 64
  unsigned int __nusers;
#endif
  int __kind;
#if __WORDSIZE != 64
  unsigned int __nusers;
#endif

which means the `__nusers' and `__kind' members are swapped between 64-bit 
and 32-bit hosts.  Which I find kind of weird (what for?), but the offset 
of `__kind' changes accordingly and the values are correct.

> -#define __PTHREAD_RWLOCK_FLAGS_OFFSET		48
> +
> +#if __WORDSIZE == 64
> +# define __PTHREAD_RWLOCK_FLAGS_OFFSET		48
> +#else
> +# define __PTHREAD_RWLOCK_FLAGS_OFFSET          24
> +#endif

 Likewise (as from 09/18):

typedef union
{
  struct __pthread_rwlock_arch_t __data;
  char __size[__SIZEOF_PTHREAD_RWLOCK_T];
  long int __align;
} pthread_rwlock_t;

and:

struct __pthread_rwlock_arch_t
{
  unsigned int __readers;
  unsigned int __writers;
  unsigned int __wrphase_futex;
  unsigned int __writers_futex;
  unsigned int __pad3;
  unsigned int __pad4;
#if __riscv_xlen == 64
  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
};

so here we have 48 for RV64, 24 for RV32/LE, and 27 for RV/BE, meaning 
that your change is wrong.  Please fix that.

 Also I think this change makes no sense at this point or indeed on its 
own as there's been no RV32 support added to <bits/struct_rwlock.h> as at 
this commit yet, meaning that the offsets would become inconsistent.  
Please fold it into 09/18.

  Maciej

On 08/07/2020 21:14, Maciej W. Rozycki via Libc-alpha wrote:
> On Wed, 3 Jun 2020, Alistair Francis via Libc-alpha wrote:
> 
>> diff --git a/sysdeps/riscv/nptl/pthread-offsets.h b/sysdeps/riscv/nptl/pthread-offsets.h
>> index 31f0587bec..a85c752a1f 100644
>> --- a/sysdeps/riscv/nptl/pthread-offsets.h
>> +++ b/sysdeps/riscv/nptl/pthread-offsets.h
>> @@ -1,3 +1,12 @@
>> -#define __PTHREAD_MUTEX_KIND_OFFSET		16
>> +#if __WORDSIZE == 64
>> +# define __PTHREAD_MUTEX_KIND_OFFSET           16
>> +#else
>> +# define __PTHREAD_MUTEX_KIND_OFFSET           12
>> +#endif
> 
>  OK, we have:
> 
> typedef union
> {
>   struct __pthread_mutex_s __data;
>   char __size[__SIZEOF_PTHREAD_MUTEX_T];
>   long int __align;
> } pthread_mutex_t;
> 
> and then:
> 
> struct __pthread_mutex_s
> {
>   int __lock __LOCK_ALIGNMENT;
>   unsigned int __count;
>   int __owner;
> #if __WORDSIZE == 64
>   unsigned int __nusers;
> #endif
>   int __kind;
> #if __WORDSIZE != 64
>   unsigned int __nusers;
> #endif
> 
> which means the `__nusers' and `__kind' members are swapped between 64-bit 
> and 32-bit hosts.  Which I find kind of weird (what for?), but the offset 
> of `__kind' changes accordingly and the values are correct.

The default __pthread_mutex_s was modeled from how other architectures
has done so we use the common ground as default (and avoid having to
create even more arch specific struct_mutex.h).  I don't exactly why
each port has used this specific layout though.

> 
>> -#define __PTHREAD_RWLOCK_FLAGS_OFFSET		48
>> +
>> +#if __WORDSIZE == 64
>> +# define __PTHREAD_RWLOCK_FLAGS_OFFSET		48
>> +#else
>> +# define __PTHREAD_RWLOCK_FLAGS_OFFSET          24
>> +#endif
> 
>  Likewise (as from 09/18):
> 
> typedef union
> {
>   struct __pthread_rwlock_arch_t __data;
>   char __size[__SIZEOF_PTHREAD_RWLOCK_T];
>   long int __align;
> } pthread_rwlock_t;
> 
> and:
> 
> struct __pthread_rwlock_arch_t
> {
>   unsigned int __readers;
>   unsigned int __writers;
>   unsigned int __wrphase_futex;
>   unsigned int __writers_futex;
>   unsigned int __pad3;
>   unsigned int __pad4;
> #if __riscv_xlen == 64
>   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
> };
> 
> so here we have 48 for RV64, 24 for RV32/LE, and 27 for RV/BE, meaning 
> that your change is wrong.  Please fix that.

This raises a flags because the wrong __PTHREAD_RWLOCK_FLAGS_OFFSET should 
generate a build failure at pthread_rwlock_init.c.

> 
>  Also I think this change makes no sense at this point or indeed on its 
> own as there's been no RV32 support added to <bits/struct_rwlock.h> as at 
> this commit yet, meaning that the offsets would become inconsistent.  
> Please fold it into 09/18.
> 
>   Maciej
>

[Added cc's back.]

On Thu, 9 Jul 2020, Adhemerval Zanella via Libc-alpha wrote:

> > so here we have 48 for RV64, 24 for RV32/LE, and 27 for RV/BE, meaning 
> > that your change is wrong.  Please fix that.
> 
> This raises a flags because the wrong __PTHREAD_RWLOCK_FLAGS_OFFSET should 
> generate a build failure at pthread_rwlock_init.c.

 Possibly no one has ever built this code BE; I haven't.

 Now that I looked into it again I think GCC currently has no option for 
the big endianness with the RISC-V target, so maybe we can leave it as it 
is under your observation that a build failure is expected to happen here 
if someone tries it once/if we have such support in the compiler.

  Maciej

On Wed, Jul 8, 2020 at 5:14 PM Maciej W. Rozycki via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> On Wed, 3 Jun 2020, Alistair Francis via Libc-alpha wrote:
>
> > diff --git a/sysdeps/riscv/nptl/pthread-offsets.h b/sysdeps/riscv/nptl/pthread-offsets.h
> > index 31f0587bec..a85c752a1f 100644
> > --- a/sysdeps/riscv/nptl/pthread-offsets.h
> > +++ b/sysdeps/riscv/nptl/pthread-offsets.h
> > @@ -1,3 +1,12 @@
> > -#define __PTHREAD_MUTEX_KIND_OFFSET          16
> > +#if __WORDSIZE == 64
> > +# define __PTHREAD_MUTEX_KIND_OFFSET           16
> > +#else
> > +# define __PTHREAD_MUTEX_KIND_OFFSET           12
> > +#endif
>
>  OK, we have:
>
> typedef union
> {
>   struct __pthread_mutex_s __data;
>   char __size[__SIZEOF_PTHREAD_MUTEX_T];
>   long int __align;
> } pthread_mutex_t;
>
> and then:
>
> struct __pthread_mutex_s
> {
>   int __lock __LOCK_ALIGNMENT;
>   unsigned int __count;
>   int __owner;
> #if __WORDSIZE == 64
>   unsigned int __nusers;
> #endif
>   int __kind;
> #if __WORDSIZE != 64
>   unsigned int __nusers;
> #endif
>
> which means the `__nusers' and `__kind' members are swapped between 64-bit
> and 32-bit hosts.  Which I find kind of weird (what for?), but the offset
> of `__kind' changes accordingly and the values are correct.

Great

>
> > -#define __PTHREAD_RWLOCK_FLAGS_OFFSET                48
> > +
> > +#if __WORDSIZE == 64
> > +# define __PTHREAD_RWLOCK_FLAGS_OFFSET               48
> > +#else
> > +# define __PTHREAD_RWLOCK_FLAGS_OFFSET          24
> > +#endif
>
>  Likewise (as from 09/18):
>
> typedef union
> {
>   struct __pthread_rwlock_arch_t __data;
>   char __size[__SIZEOF_PTHREAD_RWLOCK_T];
>   long int __align;
> } pthread_rwlock_t;
>
> and:
>
> struct __pthread_rwlock_arch_t
> {
>   unsigned int __readers;
>   unsigned int __writers;
>   unsigned int __wrphase_futex;
>   unsigned int __writers_futex;
>   unsigned int __pad3;
>   unsigned int __pad4;
> #if __riscv_xlen == 64
>   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
> };
>
> so here we have 48 for RV64, 24 for RV32/LE, and 27 for RV/BE, meaning
> that your change is wrong.  Please fix that.

I have fixed the BE offset, although I don't think we actually support BE.

>
>  Also I think this change makes no sense at this point or indeed on its
> own as there's been no RV32 support added to <bits/struct_rwlock.h> as at
> this commit yet, meaning that the offsets would become inconsistent.
> Please fold it into 09/18.

Done.

Alistair

>
>   Maciej