Re: [PATCH 2/2] Move shared pthread definitions to common headers

[Torvald Riegel <triegel at redhat dot com>]

On Mon, 2017-04-03 at 12:08 -0300, Adhemerval Zanella wrote:
> This patch removes all the replicated pthread definition accross the
> architectures and consolidates it on shared headers.  The new
> organization is as follow:
> 
>   * Architecture specific definition (such as pthread types sizes) are
>     place in the new pthreadtypes-arch.h header in arch specific path.
> 
>   * All shared structure definition are moved to a common NPTL header
>     at sysdeps/nptl/bits/pthreadtypes.h (with now includes the arch
>     specific one for internal definitions).
> 
>   * Also, for C11 future thread support, both mutex and conditional
>     definition are placed in a common header at
>     sysdeps/nptl/bits/thread-shared-types.h.
> 
> It is also a refactor patch without expected functional changes.
> Checked with a build for all major ABI (aarch64-linux-gnu, alpha-linux-gnu,
> arm-linux-gnueabi, i386-linux-gnu, ia64-linux-gnu,
> m68k-linux-gnu, microblaze-linux-gnu, mips{64}-linux-gnu, nios2-linux-gnu,
> powerpc{64le}-linux-gnu, s390{x}-linux-gnu, sparc{64}-linux-gnu,
> tile{pro,gx}-linux-gnu, and x86_64-linux-gnu).

OK with the changes below.

Patch 1/2 is OK.

> ---
> diff --git a/sysdeps/ia64/nptl/bits/pthreadtypes.h b/sysdeps/nptl/bits/pthreadtypes.h
> similarity index 53%
> rename from sysdeps/ia64/nptl/bits/pthreadtypes.h
> rename to sysdeps/nptl/bits/pthreadtypes.h
> index c67ee86..6348cf5 100644
> --- a/sysdeps/ia64/nptl/bits/pthreadtypes.h
> +++ b/sysdeps/nptl/bits/pthreadtypes.h
> @@ -1,6 +1,6 @@
> -/* Copyright (C) 2003-2017 Free Software Foundation, Inc.
> +/* Declaration of common pthread types for all architectures.
> +   Copyright (C) 2017 Free Software Foundation, Inc.
>     This file is part of the GNU C Library.
> -   Contributed by Jakub Jelinek <jakub@redhat.com>, 2003.
>  
>     The GNU C Library is free software; you can redistribute it and/or
>     modify it under the terms of the GNU Lesser General Public
> @@ -16,69 +16,21 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> -#ifndef _BITS_PTHREADTYPES_H
> -#define _BITS_PTHREADTYPES_H	1
> -
> -#define __SIZEOF_PTHREAD_ATTR_T 56
> -#define __SIZEOF_PTHREAD_MUTEX_T 40
> -#define __SIZEOF_PTHREAD_MUTEXATTR_T 4
> -#define __SIZEOF_PTHREAD_COND_T 48
> -#define __SIZEOF_PTHREAD_CONDATTR_T 4
> -#define __SIZEOF_PTHREAD_RWLOCK_T 56
> -#define __SIZEOF_PTHREAD_RWLOCKATTR_T 8
> -#define __SIZEOF_PTHREAD_BARRIER_T 32
> -#define __SIZEOF_PTHREAD_BARRIERATTR_T 4
> +#ifndef _BITS_PTHREADTYPES_COMMON_H
> +# define _BITS_PTHREADTYPES_COMMON_H	1
>  
> +/* For internal mutex and conditional definitions.  */

s/conditional/condition variable/

> @@ -131,33 +53,43 @@ typedef unsigned int pthread_key_t;
>  typedef int pthread_once_t;
>  
> 
> +union pthread_attr_t
> +{
> +  char __size[__SIZEOF_PTHREAD_ATTR_T];
> +  long int __align;
> +};
> +#ifndef __have_pthread_attr_t
> +typedef union pthread_attr_t pthread_attr_t;
> +# define __have_pthread_attr_t1
> +#endif
> +
> +
> +typedef union
> +{
> +  struct __pthread_mutex_s __data;
> +  char __size[__SIZEOF_PTHREAD_MUTEX_T];
> +  long int __align;
> +} pthread_mutex_t;
> +
> +
> +typedef union
> +{
> +  struct __pthread_cond_s __data;
> +  char __size[__SIZEOF_PTHREAD_COND_T];
> +  __extension__ long long int __align;
> +} pthread_cond_t;
> +
> +
>  #if defined __USE_UNIX98 || defined __USE_XOPEN2K
>  /* Data structure for read-write lock variable handling.  The

s/read-write/reader--writer/

> -   structure of the attribute type is not exposed on purpose.  */
> +   structure of the attribute type is deliberately not exposed.  */
>  typedef union
>  {
> -  struct
> -  {
> -    unsigned int __readers;
> -    unsigned int __writers;
> -    unsigned int __wrphase_futex;
> -    unsigned int __writers_futex;
> -    unsigned int __pad3;
> -    unsigned int __pad4;
> -    int __cur_writer;
> -    int __shared;
> -    unsigned long int __pad1;
> -    unsigned long int __pad2;
> -    /* FLAGS must stay at this position in the structure to maintain
> -       binary compatibility.  */
> -    unsigned int __flags;
> -  } __data;
> +  struct __pthread_rwlock_arch_t __data;
>    char __size[__SIZEOF_PTHREAD_RWLOCK_T];
>    long int __align;
>  } pthread_rwlock_t;
>  
> -#define __PTHREAD_RWLOCK_ELISION_EXTRA 0
> -
>  typedef union
>  {
>    char __size[__SIZEOF_PTHREAD_RWLOCKATTR_T];
> @@ -186,5 +118,4 @@ typedef union
>  } pthread_barrierattr_t;
>  #endif
>  
> -
> -#endif	/* bits/pthreadtypes.h */
> +#endif
> diff --git a/sysdeps/nptl/bits/thread-shared-types.h b/sysdeps/nptl/bits/thread-shared-types.h
> new file mode 100644
> index 0000000..82085f1
> --- /dev/null
> +++ b/sysdeps/nptl/bits/thread-shared-types.h
> @@ -0,0 +1,121 @@
> +/* Common thread definition for both POSIX and C11.

s/thread definition/threading primitives definitions/

> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef _THREAD_SHARED_TYPES_H
> +#define _THREAD_SHARED_TYPES_H 1
> +
> +/* Arch-specific definitions.  */
> +#include <bits/pthreadtypes-arch.h>
> +
> +/* Common definition of pthread_mutex_t. */
> +
> +#if __WORDSIZE == 64
> +typedef struct __pthread_internal_list
> +{
> +  struct __pthread_internal_list *__prev;
> +  struct __pthread_internal_list *__next;
> +} __pthread_list_t;
> +#else
> +typedef struct __pthread_internal_slist
> +{
> +  struct __pthread_internal_slist *__next;
> +} __pthread_slist_t;
> +#endif
> +
> +/* Lock elision support.  */
> +#if __PTHREAD_MUTEX_LOCK_ELISION
> +# if __WORDSIZE == 64
> +#  define __PTHREAD_SPINS_DATA	\
> +  short __spins;		\
> +  short __elision;
> +#  define __PTHREAD_SPINS             0, 0
> +# else
> +#  define __PTHREAD_SPINS_DATA	\
> +  struct			\
> +  {				\
> +    short __espins;		\
> +    short __eelision;		\
> +  } __elision_data
> +#  define __PTHREAD_SPINS         { 0, 0 }
> +#  define __spins __elision_data.__espins
> +#  define __elision __elision_data.__eelision

These two macros seem rather risky to me.  Is there another way to make
this work?  For which archs would struct vs. fields make a difference in
terms of alignment etc?

> +# endif
> +#else
> +# define __PTHREAD_SPINS_DATA int __spins
> +/* Mutex __spins initializer used by PTHREAD_MUTEX_INITIALIZER.  */
> +# define __PTHREAD_SPINS 0
> +#endif

Likewise, and you don't undefined __spins and __elision

> +
> +struct __pthread_mutex_s
> +{ 
> +  int __lock __PTHREAD_LOCK_ALIGNMENT;
> +  unsigned int __count;
> +  int __owner; 
> +#if __WORDSIZE == 64
> +  unsigned int __nusers;
> +#endif
> +  /* KIND must stay at this position in the structure to maintain
> +     binary compatibility with static initializers.  */
> +  int __kind;
> +  __PTHREAD_COMPAT_PADDING_MID;
> +#if __WORDSIZE == 64
> +  __PTHREAD_SPINS_DATA;
> +  __pthread_list_t __list;
> +# define __PTHREAD_MUTEX_HAVE_PREV      1
> +#else
> +  unsigned int __nusers;
> +  __extension__ union
> +  {
> +    __PTHREAD_SPINS_DATA;
> +    __pthread_slist_t __list;
> +  }
> +#endif
> +  __PTHREAD_COMPAT_PADDING_END;
> +};
> +
> +
> +/* Common definition of pthread_cond_t. */
> +
> +struct __pthread_cond_s
> +{ 
> +  __extension__ union
> +  {
> +    __extension__ unsigned long long int __wseq;
> +    struct
> +    {
> +      unsigned int __low;
> +      unsigned int __high;
> +    } __wseq32;
> +  };
> +  __extension__ union
> +  {
> +    __extension__ unsigned long long int __g1_start;
> +    struct
> +    {
> +      unsigned int __low;
> +      unsigned int __high;
> +    } __g1_start32;
> +  };
> +  unsigned int __g_refs[2] __PTHREAD_LOCK_ALIGNMENT;

If __PTHREAD_LOCK_ALIGNMENT is not just for locks, but more generally
captures alignment requirements for futex words, I think it should be
named differently, and there should be a comment somewhere explaining
it's use.  This is probably also the case for the other macros that this
file uses (though perhaps the __SIZEOF_PTHREAD_* macros are
self-explainatory).  A stub pthreadtypes-arch.h may be a way to do it,
though I'm not sure this works as well in this case as it works in the
case of futex-internal.h.

> +  unsigned int __g_size[2];
> +  unsigned int __g1_orig_size;
> +  unsigned int __wrefs;
> +  unsigned int __g_signals[2];
> +};
> +
> +#endif /* _THREAD_SHARED_TYPES_H  */