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

Message ID 25ef7709e4468a35cc42607cfc0f233d4cec3c25.1591201405.git.alistair.francis@wdc.com
State Superseded
Headers show
Series
  • glibc port for 32-bit RISC-V (RV32)
Related show

Commit Message

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

-- 
2.26.2

Comments

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

Patch

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
 
-#define __PTHREAD_RWLOCK_FLAGS_OFFSET		48
+
+#if __WORDSIZE == 64
+# define __PTHREAD_RWLOCK_FLAGS_OFFSET		48
+#else
+# define __PTHREAD_RWLOCK_FLAGS_OFFSET          24
+#endif