hppa: Use generic pthread conditional variable support

Message ID 3278DDA9-61C9-44E2-9130-F625953652FA@bell.net
State New
Headers show

Commit Message

John David Anglin March 12, 2017, 2:58 p.m.
The attach change switches the hppa target to use the generic pthread conditional variable support.
We lose compatibility with linuxthreads but I don't think that is relevant anymore for the existing distributions.

The change fixes bug nptl/21016.

Okay?

Dave
--
John David Anglin	dave.anglin@bell.net
2017-03-12  John David Anglin  <danglin@gcc.gnu.org>

	[BZ #21016]
	* sysdeps/hppa/nptl/bits/pthreadtypes.h: Use generic data structure
	for conditional variables.
	* sysdeps/unix/sysv/linux/hppa/internaltypes.h (cond_compat_clear):
	Remove.
	(cond_compat_check_and_clear): Likewise.
	* sysdeps/unix/sysv/linux/hppa/pthread.h (PTHREAD_COND_INITIALIZER):
	Revise.
	* sysdeps/unix/sysv/linux/hppa/pthread_cond_broadcast.c: Delete file.
	* sysdeps/unix/sysv/linux/hppa/pthread_cond_destroy.c: Likewise.
	* sysdeps/unix/sysv/linux/hppa/pthread_cond_signal.c: Likewise.
	* sysdeps/unix/sysv/linux/hppa/pthread_cond_wait.c: Likewise.

Comments

Florian Weimer March 12, 2017, 5:59 p.m. | #1
On 03/12/2017 03:58 PM, John David Anglin wrote:
> -    int __lock __attribute__ ((__aligned__(16)));

If this reduces the type alignment from 16 to 4, that's not acceptable 
because it changes application struct offsets when a condition variable 
is embedded into a struct.

Thanks,
Florian
John David Anglin March 12, 2017, 6:41 p.m. | #2
On 2017-03-12, at 1:59 PM, Florian Weimer wrote:

> On 03/12/2017 03:58 PM, John David Anglin wrote:
>> -    int __lock __attribute__ ((__aligned__(16)));
> 
> If this reduces the type alignment from 16 to 4, that's not acceptable because it changes application struct offsets when a condition variable is embedded into a struct.


I don't think so:

+    } __attribute__ ((__aligned__(16)));

Dave
--
John David Anglin	dave.anglin@bell.net
Torvald Riegel March 12, 2017, 7:41 p.m. | #3
On Sun, 2017-03-12 at 10:58 -0400, John David Anglin wrote:
> The attach change switches the hppa target to use the generic pthread conditional variable support.
> We lose compatibility with linuxthreads but I don't think that is relevant anymore for the existing distributions.
> 
> The change fixes bug nptl/21016.
> 
> Okay?

Carlos was thinking about re-arranging the struct layout and reserving
the LSB of __g1_orig_size for the bit that would be set in a condvar
initialized by linuxthreads:
https://sourceware.org/ml/libc-alpha/2017-03/msg00058.html
John David Anglin March 12, 2017, 8 p.m. | #4
On 2017-03-12, at 3:41 PM, Torvald Riegel wrote:

> Carlos was thinking about re-arranging the struct layout and reserving
> the LSB of __g1_orig_size for the bit that would be set in a condvar
> initialized by linuxthreads:
> https://sourceware.org/ml/libc-alpha/2017-03/msg00058.html

It's up to Carlos.  Personally, I'm not sure it's worth maintaining the linuxthreads compatibility.
Removing it gets rid of a lot of hppa code.

Dave
--
John David Anglin	dave.anglin@bell.net
John David Anglin March 12, 2017, 8:07 p.m. | #5
On 2017-03-12, at 4:00 PM, John David Anglin wrote:

> On 2017-03-12, at 3:41 PM, Torvald Riegel wrote:
> 
>> Carlos was thinking about re-arranging the struct layout and reserving
>> the LSB of __g1_orig_size for the bit that would be set in a condvar
>> initialized by linuxthreads:
>> https://sourceware.org/ml/libc-alpha/2017-03/msg00058.html
> 
> It's up to Carlos.  Personally, I'm not sure it's worth maintaining the linuxthreads compatibility.
> Removing it gets rid of a lot of hppa code.


Oh, I know Carlos had said he was going to work on this bug.  I sent my patch because Richard asked
if there was any progress on this bug.  He's been working on supporting parisc in qemu.

Dave
--
John David Anglin	dave.anglin@bell.net
Mike Frysinger March 15, 2017, 7:22 a.m. | #6
On 12 Mar 2017 16:00, John David Anglin wrote:
> On 2017-03-12, at 3:41 PM, Torvald Riegel wrote:
> > Carlos was thinking about re-arranging the struct layout and reserving
> > the LSB of __g1_orig_size for the bit that would be set in a condvar
> > initialized by linuxthreads:
> > https://sourceware.org/ml/libc-alpha/2017-03/msg00058.html
> 
> It's up to Carlos.  Personally, I'm not sure it's worth maintaining the linuxthreads compatibility.
> Removing it gets rid of a lot of hppa code.

i agree -- linuxthreads should die w/hppa.  if we can keep compat with
nptl but drop linuxthreads, let's do it.  we've already broken the ABI
once after the linuxthreads->nptl change.
-mike
Aaro Koskinen March 31, 2017, 9:21 p.m. | #7
Hi,

On Sun, Mar 12, 2017 at 10:58:46AM -0400, John David Anglin wrote:
> The attach change switches the hppa target to use the generic pthread
> conditional variable support. We lose compatibility with linuxthreads
> but I don't think that is relevant anymore for the existing distributions.
> 
> The change fixes bug nptl/21016.
> 
> Okay?

[...]

> 2017-03-12  John David Anglin  <danglin@gcc.gnu.org>
> 
> 	[BZ #21016]
> 	* sysdeps/hppa/nptl/bits/pthreadtypes.h: Use generic data structure
> 	for conditional variables.
> 	* sysdeps/unix/sysv/linux/hppa/internaltypes.h (cond_compat_clear):
> 	Remove.
> 	(cond_compat_check_and_clear): Likewise.
> 	* sysdeps/unix/sysv/linux/hppa/pthread.h (PTHREAD_COND_INITIALIZER):
> 	Revise.
> 	* sysdeps/unix/sysv/linux/hppa/pthread_cond_broadcast.c: Delete file.
> 	* sysdeps/unix/sysv/linux/hppa/pthread_cond_destroy.c: Likewise.
> 	* sysdeps/unix/sysv/linux/hppa/pthread_cond_signal.c: Likewise.
> 	* sysdeps/unix/sysv/linux/hppa/pthread_cond_wait.c: Likewise.

Should you delete sysdeps/unix/sysv/linux/hppa/pthread_cond_init.c
as well? That code is calling cond_compat_clear() which is removed by
your changes, and the linking fails.

A.
John David Anglin March 31, 2017, 10:34 p.m. | #8
On 2017-03-31, at 5:21 PM, Aaro Koskinen wrote:

> Should you delete sysdeps/unix/sysv/linux/hppa/pthread_cond_init.c
> as well? That code is calling cond_compat_clear() which is removed by
> your changes, and the linking fails.

You are correct.  I had inadvertently removed it from index in my local tree.  I'll prepare a new patch.

Dave
--
John David Anglin	dave.anglin@bell.net

Patch

diff --git a/sysdeps/hppa/nptl/bits/pthreadtypes.h b/sysdeps/hppa/nptl/bits/pthreadtypes.h
index e37111a2f3..4f742bfaa6 100644
--- a/sysdeps/hppa/nptl/bits/pthreadtypes.h
+++ b/sysdeps/hppa/nptl/bits/pthreadtypes.h
@@ -103,39 +103,32 @@  typedef union
   long int __align;
 } pthread_mutexattr_t;
 
-
-/* Data structure for conditional variable handling.  The structure of
-   the attribute type is not exposed on purpose. However, this structure
-   is exposed via PTHREAD_COND_INITIALIZER, and because of this, the
-   Linuxthreads version sets the first four ints to one. In the NPTL
-   version we must check, in every function using pthread_cond_t,
-   for the static Linuxthreads initializer and clear the appropriate
-   words. */
+/* Data structure for conditional variable handling. */
 typedef union
 {
   struct
   {
-    /* In the old Linuxthreads pthread_cond_t, this is the
-       start of the 4-word lock structure, the next four words
-       are set all to 1 by the Linuxthreads
-       PTHREAD_COND_INITIALIZER.  */
-    int __lock __attribute__ ((__aligned__(16)));
-    /* Tracks the initialization of this structure:
-       0  initialized with NPTL PTHREAD_COND_INITIALIZER.
-       1  initialized with Linuxthreads PTHREAD_COND_INITIALIZER.
-       2  initialization in progress.  */
-    int __initializer;
-    unsigned int __futex;
-    void *__mutex;
-    /* In the old Linuxthreads this would have been the start
-       of the pthread_fastlock status word.  */
-    __extension__ unsigned long long int __total_seq;
-    __extension__ unsigned long long int __wakeup_seq;
-    __extension__ unsigned long long int __woken_seq;
-    unsigned int __nwaiters;
-    unsigned int __broadcast_seq;
-    /* The NPTL pthread_cond_t is exactly the same size as
-       the Linuxthreads version, there are no words to spare.  */
+    __extension__ union
+    {
+      __extension__ unsigned long long int __wseq;
+      struct {
+	unsigned int __low;
+	unsigned int __high;
+      } __wseq32;
+    } __attribute__ ((__aligned__(16)));
+    __extension__ union
+    {
+      __extension__ unsigned long long int __g1_start;
+      struct {
+	unsigned int __low;
+        unsigned int __high;
+      } __g1_start32;
+    };
+    unsigned int __g_refs[2];
+    unsigned int __g_size[2];
+    unsigned int __g1_orig_size;
+    unsigned int __wrefs;
+    unsigned int __g_signals[2];
   } __data;
   char __size[__SIZEOF_PTHREAD_COND_T];
   __extension__ long long int __align;
diff --git a/sysdeps/unix/sysv/linux/hppa/internaltypes.h b/sysdeps/unix/sysv/linux/hppa/internaltypes.h
deleted file mode 100644
index d6496579da..0000000000
--- a/sysdeps/unix/sysv/linux/hppa/internaltypes.h
+++ /dev/null
@@ -1,84 +0,0 @@ 
-#include_next <internaltypes.h>
-#ifndef _INTERNAL_TYPES_H_HPPA_
-#define _INTERNAL_TYPES_H_HPPA_ 1
-#include <atomic.h>
-
-/* In GLIBC 2.10 HPPA switched from Linuxthreads to NPTL, and in order
-to maintain ABI compatibility with pthread_cond_t, some care had to be
-taken.
-
-The NPTL pthread_cond_t grew in size. When HPPA switched to NPTL, we
-dropped the use of ldcw, and switched to the kernel helper routine for
-compare-and-swap.  This allowed HPPA to use the 4-word 16-byte aligned
-lock words, and alignment words to store the additional pthread_cond_t
-data. Once organized properly the new NPTL pthread_cond_t was 1 word
-smaller than the Linuxthreads version.
-
-However, we were faced with the case that users may have initialized the
-pthread_cond_t with PTHREAD_COND_INITIALIZER. In this case, the first
-four words were set to one, and must be cleared before any NPTL code
-used these words.
-
-We didn't want to use LDCW, because it continues to be a source of bugs
-when applications memset pthread_cond_t to all zeroes by accident. This
-works on all other architectures where lock words are unlocked at zero.
-Remember that because of the semantics of LDCW, a locked word is set to
-zero, and an unlocked word is set to 1.
-
-Instead we used atomic_compare_and_exchange_val_acq, but we couldn't use
-this on any of the pthread_cond_t words, otherwise it might interfere
-with the current operation of the structure. To solve this problem we
-used the left over word.
-
-If the stucture was initialized by a legacy Linuxthread
-PTHREAD_COND_INITIALIZER it contained a 1, and this indicates that the
-structure requires zeroing for NPTL. The first thread to come upon a
-pthread_cond_t with a 1 in the __initializer field, will
-compare-and-swap the value, placing a 2 there which will cause all other
-threads using the same pthread_cond_t to wait for the completion of the
-initialization. Lastly, we use a store (with memory barrier) to change
-__initializer from 2 to 0. Note that the store is strongly ordered, but
-we use the PA 1.1 compatible form which is ",ma" with zero offset.
-
-In the future, when the application is recompiled with NPTL
-PTHREAD_COND_INITIALIZER it will be a quick compare-and-swap, which
-fails because __initializer is zero, and the structure will be used as
-is correctly.  */
-
-#define cond_compat_clear(var) \
-({									\
-  int tmp = 0;								\
-  var->__data.__wseq = 0;						\
-  var->__data.__signals_sent = 0;					\
-  var->__data.__confirmed = 0;						\
-  var->__data.__generation = 0;						\
-  var->__data.__mutex = NULL;						\
-  var->__data.__quiescence_waiters = 0;					\
-  var->__data.__clockid = 0;						\
-  /* Clear __initializer last, to indicate initialization is done.  */	\
-  /* This synchronizes-with the acquire load below.  */			\
-  atomic_store_release (&var->__data.__initializer, 0);			\
-})
-
-#define cond_compat_check_and_clear(var) \
-({								\
-  int v;							\
-  int *value = &var->__data.__initializer;			\
-  /* This synchronizes-with the release store above.  */	\
-  while ((v = atomic_load_acquire (value)) != 0)		\
-    {								\
-      if (v == 1						\
-	  /* Relaxed MO is fine; it only matters who's first.  */        \
-	  && atomic_compare_exchange_acquire_weak_relaxed (value, 1, 2)) \
-	{							\
-	  /* We're first; initialize structure.  */		\
-	  cond_compat_clear (var);				\
-	  break;						\
-	}							\
-      else							\
-	/* Yield before we re-check initialization status.  */	\
-	sched_yield ();						\
-    }								\
-})
-
-#endif
diff --git a/sysdeps/unix/sysv/linux/hppa/pthread.h b/sysdeps/unix/sysv/linux/hppa/pthread.h
index ac617201d2..d3b960edd4 100644
--- a/sysdeps/unix/sysv/linux/hppa/pthread.h
+++ b/sysdeps/unix/sysv/linux/hppa/pthread.h
@@ -185,7 +185,7 @@  enum
 
 
 /* Conditional variable handling.  */
-#define PTHREAD_COND_INITIALIZER { { 0, 0, 0, 0, 0, (void *) 0, 0, 0 } }
+#define PTHREAD_COND_INITIALIZER { { {0}, {0}, {0, 0}, {0, 0}, 0, 0, {0, 0} } }
 
 
 /* Cleanup buffers */
@@ -1165,12 +1165,6 @@  __END_DECLS
 #ifndef _PTHREAD_H_HPPA_
 #define _PTHREAD_H_HPPA_ 1
 
-/* The pthread_cond_t initializer is compatible only with NPTL. We do not
-   want to be forwards compatible, we eventually want to drop the code
-   that has to clear the old LT initializer.  */
-#undef PTHREAD_COND_INITIALIZER
-#define PTHREAD_COND_INITIALIZER { { 0, 0, 0, (void *) 0, 0, 0, 0, 0, 0 } }
-
 /* The pthread_mutex_t and pthread_rwlock_t initializers are compatible
    only with NPTL. NPTL assumes pthread_rwlock_t is all zero.  */
 #undef PTHREAD_MUTEX_INITIALIZER
diff --git a/sysdeps/unix/sysv/linux/hppa/pthread_cond_broadcast.c b/sysdeps/unix/sysv/linux/hppa/pthread_cond_broadcast.c
deleted file mode 100644
index a6f9f5d433..0000000000
--- a/sysdeps/unix/sysv/linux/hppa/pthread_cond_broadcast.c
+++ /dev/null
@@ -1,40 +0,0 @@ 
-/* Copyright (C) 2009-2017 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-   Contributed by Carlos O'Donell <carlos@codesourcery.com>, 2009.
-
-   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 INCLUDED_SELF
-# define INCLUDED_SELF
-# include <pthread_cond_broadcast.c>
-#else
-# include <pthread.h>
-# include <pthreadP.h>
-# include <internaltypes.h>
-# include <shlib-compat.h>
-int
-__pthread_cond_broadcast (pthread_cond_t *cond)
-{
-  cond_compat_check_and_clear (cond);
-  return __pthread_cond_broadcast_internal (cond);
-}
-versioned_symbol (libpthread, __pthread_cond_broadcast, pthread_cond_broadcast,
-                  GLIBC_2_3_2);
-# undef versioned_symbol
-# define versioned_symbol(lib, local, symbol, version)
-# undef __pthread_cond_broadcast
-# define __pthread_cond_broadcast __pthread_cond_broadcast_internal
-# include_next <pthread_cond_broadcast.c>
-#endif
diff --git a/sysdeps/unix/sysv/linux/hppa/pthread_cond_destroy.c b/sysdeps/unix/sysv/linux/hppa/pthread_cond_destroy.c
deleted file mode 100644
index 49af087bb4..0000000000
--- a/sysdeps/unix/sysv/linux/hppa/pthread_cond_destroy.c
+++ /dev/null
@@ -1,40 +0,0 @@ 
-/* Copyright (C) 2009-2017 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-   Contributed by Carlos O'Donell <carlos@codesourcery.com>, 2009.
-
-   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 INCLUDED_SELF
-# define INCLUDED_SELF
-# include <pthread_cond_destroy.c>
-#else
-# include <pthread.h>
-# include <pthreadP.h>
-# include <internaltypes.h>
-# include <shlib-compat.h>
-int
-__pthread_cond_destroy (pthread_cond_t *cond)
-{
-  cond_compat_check_and_clear (cond);
-  return __pthread_cond_destroy_internal (cond);
-}
-versioned_symbol (libpthread, __pthread_cond_destroy, pthread_cond_destroy,
-                  GLIBC_2_3_2);
-# undef versioned_symbol
-# define versioned_symbol(lib, local, symbol, version)
-# undef __pthread_cond_destroy
-# define __pthread_cond_destroy __pthread_cond_destroy_internal
-# include_next <pthread_cond_destroy.c>
-#endif
diff --git a/sysdeps/unix/sysv/linux/hppa/pthread_cond_signal.c b/sysdeps/unix/sysv/linux/hppa/pthread_cond_signal.c
deleted file mode 100644
index 2bf32af933..0000000000
--- a/sysdeps/unix/sysv/linux/hppa/pthread_cond_signal.c
+++ /dev/null
@@ -1,40 +0,0 @@ 
-/* Copyright (C) 2009-2017 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-   Contributed by Carlos O'Donell <carlos@codesourcery.com>, 2009.
-
-   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 INCLUDED_SELF
-# define INCLUDED_SELF
-# include <pthread_cond_signal.c>
-#else
-# include <pthread.h>
-# include <pthreadP.h>
-# include <internaltypes.h>
-# include <shlib-compat.h>
-int
-__pthread_cond_signal (pthread_cond_t *cond)
-{
-  cond_compat_check_and_clear (cond);
-  return __pthread_cond_signal_internal (cond);
-}
-versioned_symbol (libpthread, __pthread_cond_signal, pthread_cond_signal,
-                  GLIBC_2_3_2);
-# undef versioned_symbol
-# define versioned_symbol(lib, local, symbol, version)
-# undef __pthread_cond_signal
-# define __pthread_cond_signal __pthread_cond_signal_internal
-# include_next <pthread_cond_signal.c>
-#endif
diff --git a/sysdeps/unix/sysv/linux/hppa/pthread_cond_wait.c b/sysdeps/unix/sysv/linux/hppa/pthread_cond_wait.c
deleted file mode 100644
index 1cc2fc15d4..0000000000
--- a/sysdeps/unix/sysv/linux/hppa/pthread_cond_wait.c
+++ /dev/null
@@ -1,53 +0,0 @@ 
-/* Copyright (C) 2009-2017 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-   Contributed by Carlos O'Donell <carlos@codesourcery.com>, 2009.
-
-   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 INCLUDED_SELF
-# define INCLUDED_SELF
-# include <pthread_cond_wait.c>
-#else
-# include <pthread.h>
-# include <pthreadP.h>
-# include <internaltypes.h>
-# include <shlib-compat.h>
-int
-__pthread_cond_wait (pthread_cond_t *cond, pthread_mutex_t *mutex)
-{
-  cond_compat_check_and_clear (cond);
-  return __pthread_cond_wait_internal (cond, mutex);
-}
-versioned_symbol (libpthread, __pthread_cond_wait, pthread_cond_wait,
-                  GLIBC_2_3_2);
-int
-__pthread_cond_timedwait (cond, mutex, abstime)
-     pthread_cond_t *cond;
-     pthread_mutex_t *mutex;
-     const struct timespec *abstime;
-{
-  cond_compat_check_and_clear (cond);
-  return __pthread_cond_timedwait_internal (cond, mutex, abstime);
-}
-versioned_symbol (libpthread, __pthread_cond_timedwait, pthread_cond_timedwait,
-                  GLIBC_2_3_2);
-# undef versioned_symbol
-# define versioned_symbol(lib, local, symbol, version)
-# undef __pthread_cond_wait
-# define __pthread_cond_wait __pthread_cond_wait_internal
-# undef __pthread_cond_timedwait
-# define __pthread_cond_timedwait __pthread_cond_timedwait_internal
-# include_next <pthread_cond_wait.c>
-#endif