diff mbox

Clean pthread types namespaces on x86 and x86_64

Message ID CAD82F-p35X2_3PrOo-uKeHUNbsZq=a-dck0xRu9ssOZviAebgA@mail.gmail.com
State New
Headers show

Commit Message

Juan Manuel Torres Palma June 23, 2015, 5:50 p.m. UTC 
This patch creates the file bits/pthread_ct.h which contains types
pthread_mutex_t and pthread_cond_t, that were previously in
pthreadtypes.h. The actual structure declaration
is in pthread_st.h, that stands for specific types, so now when
creating mtx_t and cnd_t structs, namespaces won't be corrupted.

It's my first ever patch so I wanted it to be a small one. A few more
are coming in the next couple days. I'm not sure if the patch format
is ok, if it isn't let me know and how to fix it. I attach the
changelog too because gmail doesn't allow me to use tabs here.

Cheers.

Tested for x86_64.

2015-06-23  Juan Manuel Torres Palma  <jmtorrespalma@gmail.com>

    * posix/Makefile (headers): Add files bits/pthread_ct.h and
    bits/pthread_st.h to this variable.
    * bits/pthread_ct: New file.
    (pthread_mutex_t): Move struct definition.
    (pthread_cond_t): Likewise.
    * sysdeps/x86/bits/pthread_st.h: New file.
    (__SIZEOF_PTHREAD_MUTEX_T): Definition.
    (__SIZEOF_PTHREAD_COND_T): Likewise.
    (__pthread_list_t) [__x86_64__]: Likewise.
    (__pthread_slist_t) [!__x86_64__]: Likewise.
    (__pthread_mutex_s): Likewise.
    (__PTHREAD_MUTEX_T_CONTENT): Likewise.
    (__PTHREAD_COND_T_CONTENT): Likewise.
    (__PTHREAD_MUTEX_HAVE_PREV)[__x86_64__]: Likewise
    (__elision)[!__x86_64__]: Likewise
    (__spins)[!__x86_64__]: Likewise
    (__PTHREAD_SPINS): Likewise
    * sysdeps/x86/bits/pthread_st.h: Include <bits/pthread_ct.h>.
    (__SIZEOF_PTHREAD_MUTEX_T): Remove.
    (__SIZEOF_PTHREAD_COND_T): Likewise.
    (__pthread_list_t): Likewise.
    (__pthread_slist_t): Likewise.
    (__pthread_mutex_s): Likewise.
    (__PTHREAD_MUTEX_T_CONTENT): Likewise.
    (__PTHREAD_COND_T_CONTENT): Likewise.
    (__PTHREAD_MUTEX_HAVE_PREV): Likewise
    (__elision): Likewise
    (__spins): Likewise
    (__PTHREAD_SPINS): Likewise

Comments

Joseph Myers June 23, 2015, 8:20 p.m. UTC | #1 
On Tue, 23 Jun 2015, Juan Manuel Torres Palma wrote:

> This patch creates the file bits/pthread_ct.h which contains types
> pthread_mutex_t and pthread_cond_t, that were previously in
> pthreadtypes.h. The actual structure declaration
> is in pthread_st.h, that stands for specific types, so now when
> creating mtx_t and cnd_t structs, namespaces won't be corrupted.

I don't think the short cryptic file names are good.  I'd prefer something 
like bits/thread-shared-types.h for the content of types that are shared 
between pthreads and C11 threads, and bits/pthreadtypes-common.h for the 
definitions of pthread_cond_t and pthread_mutex_t in terms of that type 
content.

Mechanical patches such as this one are expected to update all 
architectures to keep the sources bisectable.  That includes creating 
dummy versions of the new headers in the toplevel bits/, like the existing 
bits/pthreadtypes.h, as well as versions of bits/pthread_st.h (which I 
think should be bits/thread-shared-types.h) for each architecture that 
currently has its own bits/pthreadtypes.h,

> +typedef union
> +{
> +	__PTHREAD_MUTEX_T_CONTENT

The indentation looks odd here (and in several other places) - it should 
be two spaces, not a tab (with initial multiples of 8 spaces replaced by 
tabs).
Juan Manuel Torres Palma June 23, 2015, 9:43 p.m. UTC | #2 
> I don't think the short cryptic file names are good.  I'd prefer something
> like bits/thread-shared-types.h for the content of types that are shared
> between pthreads and C11 threads, and bits/pthreadtypes-common.h for the
> definitions of pthread_cond_t and pthread_mutex_t in terms of that type
> content.

I agree with you, but some other people prefer short names. I will
change file names to more specific ones.

> Mechanical patches such as this one are expected to update all
> architectures to keep the sources bisectable.  That includes creating
> dummy versions of the new headers in the toplevel bits/, like the existing
> bits/pthreadtypes.h, as well as versions of bits/pthread_st.h (which I
> think should be bits/thread-shared-types.h) for each architecture that
> currently has its own bits/pthreadtypes.h,

I planned to update every architecture with a different patch better
than a single huge one, so it could be tracked down if any issue
happens since I can only test for x86_64 and ARM. Or do you mean
creating bits/thread-shared-types.h so other architectures can still
build? I didn't realize of this last possibility until now...

Cheers.

2015-06-23 22:20 GMT+02:00 Joseph Myers <joseph@codesourcery.com>:
> On Tue, 23 Jun 2015, Juan Manuel Torres Palma wrote:
>
>> This patch creates the file bits/pthread_ct.h which contains types
>> pthread_mutex_t and pthread_cond_t, that were previously in
>> pthreadtypes.h. The actual structure declaration
>> is in pthread_st.h, that stands for specific types, so now when
>> creating mtx_t and cnd_t structs, namespaces won't be corrupted.
>
> I don't think the short cryptic file names are good.  I'd prefer something
> like bits/thread-shared-types.h for the content of types that are shared
> between pthreads and C11 threads, and bits/pthreadtypes-common.h for the
> definitions of pthread_cond_t and pthread_mutex_t in terms of that type
> content.
>
> Mechanical patches such as this one are expected to update all
> architectures to keep the sources bisectable.  That includes creating
> dummy versions of the new headers in the toplevel bits/, like the existing
> bits/pthreadtypes.h, as well as versions of bits/pthread_st.h (which I
> think should be bits/thread-shared-types.h) for each architecture that
> currently has its own bits/pthreadtypes.h,
>
>> +typedef union
>> +{
>> +     __PTHREAD_MUTEX_T_CONTENT
>
> The indentation looks odd here (and in several other places) - it should
> be two spaces, not a tab (with initial multiples of 8 spaces replaced by
> tabs).
>
> --
> Joseph S. Myers
> joseph@codesourcery.com
Joseph Myers June 23, 2015, 9:46 p.m. UTC | #3 
On Tue, 23 Jun 2015, Juan Manuel Torres Palma wrote:

> > Mechanical patches such as this one are expected to update all
> > architectures to keep the sources bisectable.  That includes creating
> > dummy versions of the new headers in the toplevel bits/, like the existing
> > bits/pthreadtypes.h, as well as versions of bits/pthread_st.h (which I
> > think should be bits/thread-shared-types.h) for each architecture that
> > currently has its own bits/pthreadtypes.h,
> 
> I planned to update every architecture with a different patch better
> than a single huge one, so it could be tracked down if any issue
> happens since I can only test for x86_64 and ARM. Or do you mean
> creating bits/thread-shared-types.h so other architectures can still
> build? I didn't realize of this last possibility until now...

It's best to keep all architectures in sync at every commit boundary as 
far as possible, which means a single patch should make the same change 
for all architectures.
diff mbox

Patch

From 140d97e40a56cb3b3e70c21fc0526dc484c59407 Mon Sep 17 00:00:00 2001
From: Juan Manuel Torres Palma <jmtorrespalma@gmail.com>
Date: Tue, 23 Jun 2015 17:54:25 +0200
Subject: [PATCH] Clean pthread types namespaces on x86 and x86_64

This patch creates the file bits/pthread_ct.h which contains types
pthread_mutex_t and pthread_cond_t, that were previously in
pthreadtypes.h. The actual structure declaration
is in pthread_st.h, that stands for specific types, so now when
creating mtx_t and cnd_t structs, namespaces won't be corrupted.
---
 bits/pthread_ct.h               |  43 +++++++++++++++
 posix/Makefile                  |   3 +-
 sysdeps/x86/bits/pthread_st.h   | 116 ++++++++++++++++++++++++++++++++++++++++
 sysdeps/x86/bits/pthreadtypes.h |  76 +-------------------------
 4 files changed, 162 insertions(+), 76 deletions(-)
 create mode 100644 bits/pthread_ct.h
 create mode 100644 sysdeps/x86/bits/pthread_st.h

diff --git a/bits/pthread_ct.h b/bits/pthread_ct.h
new file mode 100644
index 0000000..4f7cfef
--- /dev/null
+++ b/bits/pthread_ct.h
@@ -0,0 +1,43 @@ 
+/* Copyright (C) 2002-2015 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/>.  */
+
+
+/* Declaration of common pthread types for all architectures.
+   Macros *_CONTENT are architecture dependent and defined in
+   bits/pthread_st.h */
+
+#ifndef _BITS_THREAD_CT_H
+# define _BITS_THREAD_CT_H	1
+
+# include <bits/pthread_st.h>
+
+/* Common definition of pthread_mutex_t. */
+
+typedef union
+{
+	__PTHREAD_MUTEX_T_CONTENT
+} pthread_mutex_t;
+
+
+/* Common definition of pthread_cond_t. */
+
+typedef union
+{
+	__PTHREAD_COND_T_CONTENT
+} pthread_cond_t;
+
+#endif /* _BITS_THREAD_CT_H */
diff --git a/posix/Makefile b/posix/Makefile
index 15e8818..eabe3ff 100644
--- a/posix/Makefile
+++ b/posix/Makefile
@@ -29,7 +29,8 @@  headers	:= sys/utsname.h sys/times.h sys/wait.h sys/types.h unistd.h	      \
 	   bits/local_lim.h tar.h bits/utsname.h bits/confname.h	      \
 	   bits/waitflags.h bits/waitstatus.h sys/unistd.h sched.h	      \
 	   bits/sched.h re_comp.h wait.h bits/environments.h cpio.h	      \
-	   sys/sysmacros.h spawn.h bits/unistd.h
+	   sys/sysmacros.h spawn.h bits/unistd.h bits/pthread_st.h        \
+	   bits/pthread_ct.h
 
 routines :=								      \
 	uname								      \
diff --git a/sysdeps/x86/bits/pthread_st.h b/sysdeps/x86/bits/pthread_st.h
new file mode 100644
index 0000000..e64d194
--- /dev/null
+++ b/sysdeps/x86/bits/pthread_st.h
@@ -0,0 +1,116 @@ 
+/* Copyright (C) 2002-2015 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/>.  */
+
+/* This header contains macros definition required for 
+   the declaration of pthread_mutex_t and pthread_cond_t, 
+   both needed by C11 types mtx_t and cnd_t. */
+
+#ifndef _BITS_PTHREAD_ST_H
+#define _BITS_PTHREAD_ST_H	1
+
+#include <bits/wordsize.h>
+
+#ifdef __x86_64__
+# if __WORDSIZE == 64
+#  define __SIZEOF_PTHREAD_MUTEX_T 40
+#  define __SIZEOF_PTHREAD_COND_T 48
+# else
+#  define __SIZEOF_PTHREAD_MUTEX_T 32
+#  define __SIZEOF_PTHREAD_COND_T 48
+# endif
+#else
+# define __SIZEOF_PTHREAD_MUTEX_T 24
+# define __SIZEOF_PTHREAD_COND_T 48
+#endif
+
+#ifdef __x86_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
+
+
+/* Data structure for mutex handling. */
+
+  struct __pthread_mutex_s
+  {
+    int __lock;
+    unsigned int __count;
+    int __owner;
+#ifdef __x86_64__
+    unsigned int __nusers;
+#endif
+    /* KIND must stay at this position in the structure to maintain
+       binary compatibility.  */
+    int __kind;
+#ifdef __x86_64__
+    short __spins;
+    short __elision;
+    __pthread_list_t __list;
+# define __PTHREAD_MUTEX_HAVE_PREV	1
+/* Mutex __spins initializer used by PTHREAD_MUTEX_INITIALIZER.  */
+# define __PTHREAD_SPINS             0, 0
+#else
+    unsigned int __nusers;
+    __extension__ union
+    {
+      struct
+      {
+	    short __espins;
+	    short __elision;
+# define __spins __elision_data.__espins
+# define __elision __elision_data.__elision
+# define __PTHREAD_SPINS         { 0, 0 }
+      } __elision_data;
+      __pthread_slist_t __list;
+    };
+#endif
+  };
+
+
+# define __PTHREAD_MUTEX_T_CONTENT            \
+  struct __pthread_mutex_s __data;            \
+  char __size[__SIZEOF_PTHREAD_MUTEX_T];      \
+  long int __align;
+
+
+/* Data structure for conditional variable handling */
+
+#define __PTHREAD_COND_T_CONTENT                          \
+  struct                                                  \
+  {                                                       \
+    int __lock;                                           \
+    unsigned int __futex;                                 \
+    __extension__ unsigned long long int __total_seq;     \
+    __extension__ unsigned long long int __wakeup_seq;    \
+    __extension__ unsigned long long int __woken_seq;     \
+    void *__mutex;                                        \
+    unsigned int __nwaiters;                              \
+    unsigned int __broadcast_seq;                         \
+  } __data;                                               \
+  char __size[__SIZEOF_PTHREAD_COND_T];                   \
+  __extension__ long long int __align;
+
+
+#endif	/* bits/pthread_st.h */
diff --git a/sysdeps/x86/bits/pthreadtypes.h b/sysdeps/x86/bits/pthreadtypes.h
index 4460615..ccb73b0 100644
--- a/sysdeps/x86/bits/pthreadtypes.h
+++ b/sysdeps/x86/bits/pthreadtypes.h
@@ -19,13 +19,12 @@ 
 #define _BITS_PTHREADTYPES_H	1
 
 #include <bits/wordsize.h>
+#include <bits/pthread_ct.h>
 
 #ifdef __x86_64__
 # if __WORDSIZE == 64
 #  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
@@ -33,9 +32,7 @@ 
 #  define __SIZEOF_PTHREAD_BARRIERATTR_T 4
 # else
 #  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 44
 #  define __SIZEOF_PTHREAD_RWLOCKATTR_T 8
@@ -44,9 +41,7 @@ 
 # endif
 #else
 # define __SIZEOF_PTHREAD_ATTR_T 36
-# define __SIZEOF_PTHREAD_MUTEX_T 24
 # define __SIZEOF_PTHREAD_MUTEXATTR_T 4
-# define __SIZEOF_PTHREAD_COND_T 48
 # define __SIZEOF_PTHREAD_CONDATTR_T 4
 # define __SIZEOF_PTHREAD_RWLOCK_T 32
 # define __SIZEOF_PTHREAD_RWLOCKATTR_T 8
@@ -71,61 +66,8 @@  typedef union pthread_attr_t pthread_attr_t;
 #endif
 
 
-#ifdef __x86_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
-
-
 /* Data structures for mutex handling.  The structure of the attribute
    type is not exposed on purpose.  */
-typedef union
-{
-  struct __pthread_mutex_s
-  {
-    int __lock;
-    unsigned int __count;
-    int __owner;
-#ifdef __x86_64__
-    unsigned int __nusers;
-#endif
-    /* KIND must stay at this position in the structure to maintain
-       binary compatibility.  */
-    int __kind;
-#ifdef __x86_64__
-    short __spins;
-    short __elision;
-    __pthread_list_t __list;
-# define __PTHREAD_MUTEX_HAVE_PREV	1
-/* Mutex __spins initializer used by PTHREAD_MUTEX_INITIALIZER.  */
-# define __PTHREAD_SPINS             0, 0
-#else
-    unsigned int __nusers;
-    __extension__ union
-    {
-      struct
-      {
-	short __espins;
-	short __elision;
-# define __spins __elision_data.__espins
-# define __elision __elision_data.__elision
-# define __PTHREAD_SPINS         { 0, 0 }
-      } __elision_data;
-      __pthread_slist_t __list;
-    };
-#endif
-  } __data;
-  char __size[__SIZEOF_PTHREAD_MUTEX_T];
-  long int __align;
-} pthread_mutex_t;
 
 typedef union
 {
@@ -136,22 +78,6 @@  typedef union
 
 /* Data structure for conditional variable handling.  The structure of
    the attribute type is not exposed on purpose.  */
-typedef union
-{
-  struct
-  {
-    int __lock;
-    unsigned int __futex;
-    __extension__ unsigned long long int __total_seq;
-    __extension__ unsigned long long int __wakeup_seq;
-    __extension__ unsigned long long int __woken_seq;
-    void *__mutex;
-    unsigned int __nwaiters;
-    unsigned int __broadcast_seq;
-  } __data;
-  char __size[__SIZEOF_PTHREAD_COND_T];
-  __extension__ long long int __align;
-} pthread_cond_t;
 
 typedef union
 {
-- 
2.1.0