[RFC] Add pthread_cond_timedwaitonclock_np

[RFC] Add pthread_cond_timedwaitonclock_np

[Mike Crowe]

C++11's std::condition_variable::wait_until specifies the clock to be
used at the time of the wait and permits separate waits on the same
instance using different clocks.

Unfortunately pthread_cond_timedwait always uses the clock that was
specified (or defaulted) when pthread_cond_init was called. libstdc++'s
current implementation converts all waits to
std::chrono::system_clock (i.e. CLOCK_REALTIME) which can race against
the system clock changing.

Inventing a brand-new function pthread_cond_timedwaitonclock_np which
accepts both the clock and the time point as parameters is
straightforward and means that the C++11 standard behaviour can be
implemented in libstdc++ on Linux at least.

(This patch is only to the C implementation of pthread_cond_timedwait so
it is necessary to remove pthread_cond_timedwait.S and
pthread_cond_wait.S to use it on x86 and x86_64 at the moment. If the
approach is deemed acceptable I'll work on fixing those too along with
the documentation.)

Signed-off-by: Mike Crowe <mac@mcrowe.com>
---
 conform/data/pthread.h-data   |   3 +
 nptl/Makefile                 |   4 +-
 nptl/Versions                 |   1 +
 nptl/pthread_cond_timedwait.c |  26 ++++--
 nptl/tst-cond11-onclock.c     | 206 ++++++++++++++++++++++++++++++++++++++++++
 nptl/tst-cond5-onclock.c      | 113 +++++++++++++++++++++++
 sysdeps/nptl/pthread.h        |  13 +++
 7 files changed, 356 insertions(+), 10 deletions(-)
 create mode 100644 nptl/tst-cond11-onclock.c
 create mode 100644 nptl/tst-cond5-onclock.c

diff --git a/conform/data/pthread.h-data b/conform/data/pthread.h-data
index c1e32c8..d52f298 100644
--- a/conform/data/pthread.h-data
+++ b/conform/data/pthread.h-data
@@ -92,6 +92,9 @@ function int pthread_cond_destroy (pthread_cond_t*)
 function int pthread_cond_init (pthread_cond_t*, const pthread_condattr_t*)
 function int pthread_cond_signal (pthread_cond_t*)
 function int pthread_cond_timedwait (pthread_cond_t*, pthread_mutex_t*, const struct timespec*)
+#if !defined POSIX && !defined UNIX98 && !defined XOPEN2K
+function int pthread_cond_timedwaitonclock_np (pthread_cond_t*, pthread_mutex_t*, clockid_t, const struct timespec*)
+#endif
 function int pthread_cond_wait (pthread_cond_t*, pthread_mutex_t*)
 function int pthread_condattr_destroy (pthread_condattr_t*)
 #if !defined POSIX && !defined UNIX98 && !defined XOPEN2K
diff --git a/nptl/Makefile b/nptl/Makefile
index 4544aa2..ff06939 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -221,8 +221,8 @@ tests = tst-typesizes \
 	tst-mutexpi5a tst-mutexpi6 tst-mutexpi7 tst-mutexpi7a tst-mutexpi8 \
 	tst-mutexpi9 \
 	tst-spin1 tst-spin2 tst-spin3 tst-spin4 \
-	tst-cond1 tst-cond2 tst-cond3 tst-cond4 tst-cond5 tst-cond6 tst-cond7 \
-	tst-cond8 tst-cond9 tst-cond10 tst-cond11 tst-cond12 tst-cond13 \
+	tst-cond1 tst-cond2 tst-cond3 tst-cond4 tst-cond5 tst-cond5-onclock tst-cond6 tst-cond7 \
+	tst-cond8 tst-cond9 tst-cond10 tst-cond11 tst-cond11-onclock tst-cond12 tst-cond13 \
 	tst-cond14 tst-cond15 tst-cond16 tst-cond17 tst-cond18 tst-cond19 \
 	tst-cond20 tst-cond21 tst-cond22 tst-cond23 tst-cond24 tst-cond25 \
 	tst-cond-except \
diff --git a/nptl/Versions b/nptl/Versions
index 34e4b46..476301f 100644
--- a/nptl/Versions
+++ b/nptl/Versions
@@ -267,6 +267,7 @@ libpthread {
   }
 
   GLIBC_2.22 {
+    pthread_cond_timedwaitonclock_np;
   }
 
   GLIBC_PRIVATE {
diff --git a/nptl/pthread_cond_timedwait.c b/nptl/pthread_cond_timedwait.c
index 10b0a61..a4c0ee3 100644
--- a/nptl/pthread_cond_timedwait.c
+++ b/nptl/pthread_cond_timedwait.c
@@ -49,8 +49,9 @@ struct _condvar_cleanup_buffer
 };
 
 int
-__pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex,
-			  const struct timespec *abstime)
+pthread_cond_timedwaitonclock_np (pthread_cond_t *cond, pthread_mutex_t *mutex,
+				  clockid_t clockid,
+				  const struct timespec *abstime)
 {
   struct _pthread_cleanup_buffer buffer;
   struct _condvar_cleanup_buffer cbuffer;
@@ -120,8 +121,7 @@ __pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex,
 # ifdef __NR_clock_gettime
 	INTERNAL_SYSCALL_DECL (err);
 	(void) INTERNAL_VSYSCALL (clock_gettime, err, 2,
-				  (cond->__data.__nwaiters
-				   & ((1 << COND_NWAITERS_SHIFT) - 1)),
+				  clockid,
 				  &rt);
 	/* Convert the absolute timeout value to a relative timeout.  */
 	rt.tv_sec = abstime->tv_sec - rt.tv_sec;
@@ -175,8 +175,7 @@ __pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex,
 
       if (pi_flag)
 	{
-	  unsigned int clockbit = (cond->__data.__nwaiters & 1
-				   ? 0 : FUTEX_CLOCK_REALTIME);
+	  unsigned int clockbit = clockid ? 0 : FUTEX_CLOCK_REALTIME;
 	  err = lll_futex_timed_wait_requeue_pi (&cond->__data.__futex,
 						 futex_val, abstime, clockbit,
 						 &mutex->__data.__lock,
@@ -193,8 +192,7 @@ __pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex,
 	  err = lll_futex_timed_wait (&cond->__data.__futex,
 				      futex_val, &rt, pshared);
 #else
-	  unsigned int clockbit = (cond->__data.__nwaiters & 1
-				   ? 0 : FUTEX_CLOCK_REALTIME);
+	  unsigned int clockbit = clockid ? 0 : FUTEX_CLOCK_REALTIME;
 	  err = lll_futex_timed_wait_bitset (&cond->__data.__futex, futex_val,
 					     abstime, clockbit, pshared);
 #endif
@@ -264,5 +262,17 @@ __pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex,
   return err ?: result;
 }
 
+int
+__pthread_cond_timedwait (cond, mutex, abstime)
+     pthread_cond_t *cond;
+     pthread_mutex_t *mutex;
+     const struct timespec *abstime;
+{
+    return pthread_cond_timedwaitonclock_np (cond, mutex,
+					     (cond->__data.__nwaiters
+					      & ((1 << COND_NWAITERS_SHIFT) - 1)),
+					     abstime);
+}
+
 versioned_symbol (libpthread, __pthread_cond_timedwait, pthread_cond_timedwait,
 		  GLIBC_2_3_2);
diff --git a/nptl/tst-cond11-onclock.c b/nptl/tst-cond11-onclock.c
new file mode 100644
index 0000000..15b3730
--- /dev/null
+++ b/nptl/tst-cond11-onclock.c
@@ -0,0 +1,206 @@
+/* Copyright (C) 2003-2014 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+   Contributed by Ulrich Drepper <drepper@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
+   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/>.  */
+
+#include <errno.h>
+#include <pthread.h>
+#include <stdio.h>
+#include <time.h>
+#include <unistd.h>
+
+
+#if defined _POSIX_CLOCK_SELECTION && _POSIX_CLOCK_SELECTION >= 0
+static int
+run_test (clockid_t attr_clock, clockid_t wait_clock)
+{
+  pthread_condattr_t condattr;
+  pthread_cond_t cond;
+  pthread_mutexattr_t mutattr;
+  pthread_mutex_t mut;
+
+  printf ("attr_clock = %d, wait_clock = %d\n", (int) attr_clock, (int) wait_clock);
+
+  if (pthread_condattr_init (&condattr) != 0)
+    {
+      puts ("condattr_init failed");
+      return 1;
+    }
+
+  if (pthread_condattr_setclock (&condattr, attr_clock) != 0)
+    {
+      puts ("condattr_setclock failed");
+      return 1;
+    }
+
+  clockid_t cl2;
+  if (pthread_condattr_getclock (&condattr, &cl2) != 0)
+    {
+      puts ("condattr_getclock failed");
+      return 1;
+    }
+  if (attr_clock != cl2)
+    {
+      printf ("condattr_getclock returned wrong value: %d, expected %d\n",
+	      (int) cl2, (int) attr_clock);
+      return 1;
+    }
+
+  if (pthread_cond_init (&cond, &condattr) != 0)
+    {
+      puts ("cond_init failed");
+      return 1;
+    }
+
+  if (pthread_condattr_destroy (&condattr) != 0)
+    {
+      puts ("condattr_destroy failed");
+      return 1;
+    }
+
+  if (pthread_mutexattr_init (&mutattr) != 0)
+    {
+      puts ("mutexattr_init failed");
+      return 1;
+    }
+
+  if (pthread_mutexattr_settype (&mutattr, PTHREAD_MUTEX_ERRORCHECK) != 0)
+    {
+      puts ("mutexattr_settype failed");
+      return 1;
+    }
+
+  if (pthread_mutex_init (&mut, &mutattr) != 0)
+    {
+      puts ("mutex_init failed");
+      return 1;
+    }
+
+  if (pthread_mutexattr_destroy (&mutattr) != 0)
+    {
+      puts ("mutexattr_destroy failed");
+      return 1;
+    }
+
+  if (pthread_mutex_lock (&mut) != 0)
+    {
+      puts ("mutex_lock failed");
+      return 1;
+    }
+
+  if (pthread_mutex_lock (&mut) != EDEADLK)
+    {
+      puts ("2nd mutex_lock did not return EDEADLK");
+      return 1;
+    }
+
+  struct timespec ts;
+  if (clock_gettime (wait_clock, &ts) != 0)
+    {
+      puts ("clock_gettime failed");
+      return 1;
+    }
+
+  /* Wait one second.  */
+  ++ts.tv_sec;
+
+  int e = pthread_cond_timedwaitonclock_np (&cond, &mut, wait_clock, &ts);
+  if (e == 0)
+    {
+      puts ("cond_timedwait succeeded");
+      return 1;
+    }
+  else if (e != ETIMEDOUT)
+    {
+      puts ("cond_timedwait did not return ETIMEDOUT");
+      return 1;
+    }
+
+  struct timespec ts2;
+  if (clock_gettime (wait_clock, &ts2) != 0)
+    {
+      puts ("second clock_gettime failed");
+      return 1;
+    }
+
+  if (ts2.tv_sec < ts.tv_sec
+      || (ts2.tv_sec == ts.tv_sec && ts2.tv_nsec < ts.tv_nsec))
+    {
+      puts ("timeout too short");
+      return 1;
+    }
+
+  if (pthread_mutex_unlock (&mut) != 0)
+    {
+      puts ("mutex_unlock failed");
+      return 1;
+    }
+
+  if (pthread_mutex_destroy (&mut) != 0)
+    {
+      puts ("mutex_destroy failed");
+      return 1;
+    }
+
+  if (pthread_cond_destroy (&cond) != 0)
+    {
+      puts ("cond_destroy failed");
+      return 1;
+    }
+
+  return 0;
+}
+#endif
+
+
+static int
+do_test (void)
+{
+#if !defined _POSIX_CLOCK_SELECTION || _POSIX_CLOCK_SELECTION == -1
+
+  puts ("_POSIX_CLOCK_SELECTION not supported, test skipped");
+  return 0;
+
+#else
+
+  int res = run_test (CLOCK_REALTIME, CLOCK_REALTIME);
+
+# if defined _POSIX_MONOTONIC_CLOCK && _POSIX_MONOTONIC_CLOCK >= 0
+#  if _POSIX_MONOTONIC_CLOCK == 0
+  int e = sysconf (_SC_MONOTONIC_CLOCK);
+  if (e < 0)
+    puts ("CLOCK_MONOTONIC not supported");
+  else if (e == 0)
+    {
+      puts ("sysconf (_SC_MONOTONIC_CLOCK) must not return 0");
+      res = 1;
+    }
+  else
+#  endif
+    res |= run_test (CLOCK_REALTIME, CLOCK_MONOTONIC);
+    res |= run_test (CLOCK_MONOTONIC, CLOCK_MONOTONIC);
+    res |= run_test (CLOCK_MONOTONIC, CLOCK_REALTIME);
+# else
+  puts ("_POSIX_MONOTONIC_CLOCK not defined");
+# endif
+
+  return res;
+#endif
+}
+
+#define TIMEOUT 12
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
diff --git a/nptl/tst-cond5-onclock.c b/nptl/tst-cond5-onclock.c
new file mode 100644
index 0000000..ddb0ad1
--- /dev/null
+++ b/nptl/tst-cond5-onclock.c
@@ -0,0 +1,113 @@
+/* Copyright (C) 2002-2014 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+   Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.
+
+   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/>.  */
+
+#include <errno.h>
+#include <pthread.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <time.h>
+#include <sys/time.h>
+
+
+static pthread_mutex_t mut;
+static pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
+
+
+static int
+do_test_onclock(clockid_t clockid)
+{
+  pthread_mutexattr_t ma;
+  int err;
+  struct timespec ts;
+
+  if (pthread_mutexattr_init (&ma) != 0)
+    {
+      puts ("mutexattr_init failed");
+      exit (1);
+    }
+
+  if (pthread_mutexattr_settype (&ma, PTHREAD_MUTEX_ERRORCHECK) != 0)
+    {
+      puts ("mutexattr_settype failed");
+      exit (1);
+    }
+
+  if (pthread_mutex_init (&mut, &ma) != 0)
+    {
+      puts ("mutex_init failed");
+      exit (1);
+    }
+
+  /* Get the mutex.  */
+  if (pthread_mutex_lock (&mut) != 0)
+    {
+      puts ("mutex_lock failed");
+      exit (1);
+    }
+
+  /* Waiting for the condition will fail.  But we want the timeout here.  */
+  if (clock_gettime (clockid, &ts) != 0)
+    {
+      puts ("clock_gettime failed");
+      exit (1);
+    }
+
+  ts.tv_nsec += 500000000;
+  if (ts.tv_nsec >= 1000000000)
+    {
+      ts.tv_nsec -= 1000000000;
+      ++ts.tv_sec;
+    }
+  err = pthread_cond_timedwaitonclock_np (&cond, &mut, clockid, &ts);
+  if (err == 0)
+    {
+      /* This could in theory happen but here without any signal and
+	 additional waiter it should not.  */
+      puts ("cond_timedwait succeeded");
+      exit (1);
+    }
+  else if (err != ETIMEDOUT)
+    {
+      printf ("cond_timedwait returned with %s\n", strerror (err));
+      exit (1);
+    }
+
+  err = pthread_mutex_unlock (&mut);
+  if (err != 0)
+    {
+      printf ("mutex_unlock failed: %s\n", strerror (err));
+      exit (1);
+    }
+
+  return 0;
+}
+
+static int
+do_test (void)
+{
+  int rc;
+  rc = do_test_onclock(CLOCK_MONOTONIC);
+  if (rc == 0)
+    rc = do_test_onclock(CLOCK_REALTIME);
+
+  return rc;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
diff --git a/sysdeps/nptl/pthread.h b/sysdeps/nptl/pthread.h
index 89d0882..221924b 100644
--- a/sysdeps/nptl/pthread.h
+++ b/sysdeps/nptl/pthread.h
@@ -1002,6 +1002,19 @@ extern int pthread_cond_timedwait (pthread_cond_t *__restrict __cond,
 				   const struct timespec *__restrict __abstime)
      __nonnull ((1, 2, 3));
 
+/* Wait for condition variable COND to be signaled or broadcast until
+   ABSTIME measured by the specified clock. MUTEX is assumed to be
+   locked before. CLOCK is a clock specified. ABSTIME is an absolute
+   time specification against CLOCK's epoch.
+
+   This function is a cancellation point and therefore not marked with
+   __THROW. */
+extern int pthread_cond_timedwaitonclock_np (pthread_cond_t *__restrict __cond,
+					     pthread_mutex_t *__restrict __mutex,
+					     __clockid_t __clock_id,
+					     const struct timespec *__restrict __abstime)
+     __nonnull ((1, 2, 4));
+
 /* Functions for handling condition variable attributes.  */
 
 /* Initialize condition variable attribute ATTR.  */
-- 
2.1.4

Re: [RFC] Add pthread_cond_timedwaitonclock_np

[Szabolcs Nagy]

On 07/07/15 14:56, Mike Crowe wrote:
> C++11's std::condition_variable::wait_until specifies the clock to be
> used at the time of the wait and permits separate waits on the same
> instance using different clocks.
> 
> Unfortunately pthread_cond_timedwait always uses the clock that was
> specified (or defaulted) when pthread_cond_init was called. libstdc++'s
> current implementation converts all waits to
> std::chrono::system_clock (i.e. CLOCK_REALTIME) which can race against
> the system clock changing.
> 
> Inventing a brand-new function pthread_cond_timedwaitonclock_np which
> accepts both the clock and the time point as parameters is
> straightforward and means that the C++11 standard behaviour can be
> implemented in libstdc++ on Linux at least.
> 

do you have a reference to the rationale that introduced this api to C++?

this should be reported as an enhancement request to the austingroup
if there is real world need for it so all platforms implement it
consistently in the c runtime.

> (This patch is only to the C implementation of pthread_cond_timedwait so
> it is necessary to remove pthread_cond_timedwait.S and
> pthread_cond_wait.S to use it on x86 and x86_64 at the moment. If the
> approach is deemed acceptable I'll work on fixing those too along with
> the documentation.)
> 
> Signed-off-by: Mike Crowe <mac@mcrowe.com>
> ---
>  conform/data/pthread.h-data   |   3 +
>  nptl/Makefile                 |   4 +-
>  nptl/Versions                 |   1 +
>  nptl/pthread_cond_timedwait.c |  26 ++++--
>  nptl/tst-cond11-onclock.c     | 206 ++++++++++++++++++++++++++++++++++++++++++
>  nptl/tst-cond5-onclock.c      | 113 +++++++++++++++++++++++
>  sysdeps/nptl/pthread.h        |  13 +++
>  7 files changed, 356 insertions(+), 10 deletions(-)
>  create mode 100644 nptl/tst-cond11-onclock.c
>  create mode 100644 nptl/tst-cond5-onclock.c
> 
> diff --git a/conform/data/pthread.h-data b/conform/data/pthread.h-data
> index c1e32c8..d52f298 100644
> --- a/conform/data/pthread.h-data
> +++ b/conform/data/pthread.h-data
> @@ -92,6 +92,9 @@ function int pthread_cond_destroy (pthread_cond_t*)
>  function int pthread_cond_init (pthread_cond_t*, const pthread_condattr_t*)
>  function int pthread_cond_signal (pthread_cond_t*)
>  function int pthread_cond_timedwait (pthread_cond_t*, pthread_mutex_t*, const struct timespec*)
> +#if !defined POSIX && !defined UNIX98 && !defined XOPEN2K
> +function int pthread_cond_timedwaitonclock_np (pthread_cond_t*, pthread_mutex_t*, clockid_t, const struct timespec*)
> +#endif
>  function int pthread_cond_wait (pthread_cond_t*, pthread_mutex_t*)
>  function int pthread_condattr_destroy (pthread_condattr_t*)
>  #if !defined POSIX && !defined UNIX98 && !defined XOPEN2K
> diff --git a/nptl/Makefile b/nptl/Makefile
> index 4544aa2..ff06939 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -221,8 +221,8 @@ tests = tst-typesizes \
>         tst-mutexpi5a tst-mutexpi6 tst-mutexpi7 tst-mutexpi7a tst-mutexpi8 \
>         tst-mutexpi9 \
>         tst-spin1 tst-spin2 tst-spin3 tst-spin4 \
> -       tst-cond1 tst-cond2 tst-cond3 tst-cond4 tst-cond5 tst-cond6 tst-cond7 \
> -       tst-cond8 tst-cond9 tst-cond10 tst-cond11 tst-cond12 tst-cond13 \
> +       tst-cond1 tst-cond2 tst-cond3 tst-cond4 tst-cond5 tst-cond5-onclock tst-cond6 tst-cond7 \
> +       tst-cond8 tst-cond9 tst-cond10 tst-cond11 tst-cond11-onclock tst-cond12 tst-cond13 \
>         tst-cond14 tst-cond15 tst-cond16 tst-cond17 tst-cond18 tst-cond19 \
>         tst-cond20 tst-cond21 tst-cond22 tst-cond23 tst-cond24 tst-cond25 \
>         tst-cond-except \
> diff --git a/nptl/Versions b/nptl/Versions
> index 34e4b46..476301f 100644
> --- a/nptl/Versions
> +++ b/nptl/Versions
> @@ -267,6 +267,7 @@ libpthread {
>    }
> 
>    GLIBC_2.22 {
> +    pthread_cond_timedwaitonclock_np;
>    }
> 
>    GLIBC_PRIVATE {
> diff --git a/nptl/pthread_cond_timedwait.c b/nptl/pthread_cond_timedwait.c
> index 10b0a61..a4c0ee3 100644
> --- a/nptl/pthread_cond_timedwait.c
> +++ b/nptl/pthread_cond_timedwait.c
> @@ -49,8 +49,9 @@ struct _condvar_cleanup_buffer
>  };
> 
>  int
> -__pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex,
> -                         const struct timespec *abstime)
> +pthread_cond_timedwaitonclock_np (pthread_cond_t *cond, pthread_mutex_t *mutex,
> +                                 clockid_t clockid,
> +                                 const struct timespec *abstime)
>  {
>    struct _pthread_cleanup_buffer buffer;
>    struct _condvar_cleanup_buffer cbuffer;
> @@ -120,8 +121,7 @@ __pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex,
>  # ifdef __NR_clock_gettime
>         INTERNAL_SYSCALL_DECL (err);
>         (void) INTERNAL_VSYSCALL (clock_gettime, err, 2,
> -                                 (cond->__data.__nwaiters
> -                                  & ((1 << COND_NWAITERS_SHIFT) - 1)),
> +                                 clockid,
>                                   &rt);
>         /* Convert the absolute timeout value to a relative timeout.  */
>         rt.tv_sec = abstime->tv_sec - rt.tv_sec;
> @@ -175,8 +175,7 @@ __pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex,
> 
>        if (pi_flag)
>         {
> -         unsigned int clockbit = (cond->__data.__nwaiters & 1
> -                                  ? 0 : FUTEX_CLOCK_REALTIME);
> +         unsigned int clockbit = clockid ? 0 : FUTEX_CLOCK_REALTIME;
>           err = lll_futex_timed_wait_requeue_pi (&cond->__data.__futex,
>                                                  futex_val, abstime, clockbit,
>                                                  &mutex->__data.__lock,
> @@ -193,8 +192,7 @@ __pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex,
>           err = lll_futex_timed_wait (&cond->__data.__futex,
>                                       futex_val, &rt, pshared);
>  #else
> -         unsigned int clockbit = (cond->__data.__nwaiters & 1
> -                                  ? 0 : FUTEX_CLOCK_REALTIME);
> +         unsigned int clockbit = clockid ? 0 : FUTEX_CLOCK_REALTIME;
>           err = lll_futex_timed_wait_bitset (&cond->__data.__futex, futex_val,
>                                              abstime, clockbit, pshared);
>  #endif
> @@ -264,5 +262,17 @@ __pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex,
>    return err ?: result;
>  }
> 
> +int
> +__pthread_cond_timedwait (cond, mutex, abstime)
> +     pthread_cond_t *cond;
> +     pthread_mutex_t *mutex;
> +     const struct timespec *abstime;
> +{
> +    return pthread_cond_timedwaitonclock_np (cond, mutex,
> +                                            (cond->__data.__nwaiters
> +                                             & ((1 << COND_NWAITERS_SHIFT) - 1)),
> +                                            abstime);
> +}
> +
>  versioned_symbol (libpthread, __pthread_cond_timedwait, pthread_cond_timedwait,
>                   GLIBC_2_3_2);
> diff --git a/nptl/tst-cond11-onclock.c b/nptl/tst-cond11-onclock.c
> new file mode 100644
> index 0000000..15b3730
> --- /dev/null
> +++ b/nptl/tst-cond11-onclock.c
> @@ -0,0 +1,206 @@
> +/* Copyright (C) 2003-2014 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +   Contributed by Ulrich Drepper <drepper@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
> +   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/>.  */
> +
> +#include <errno.h>
> +#include <pthread.h>
> +#include <stdio.h>
> +#include <time.h>
> +#include <unistd.h>
> +
> +
> +#if defined _POSIX_CLOCK_SELECTION && _POSIX_CLOCK_SELECTION >= 0
> +static int
> +run_test (clockid_t attr_clock, clockid_t wait_clock)
> +{
> +  pthread_condattr_t condattr;
> +  pthread_cond_t cond;
> +  pthread_mutexattr_t mutattr;
> +  pthread_mutex_t mut;
> +
> +  printf ("attr_clock = %d, wait_clock = %d\n", (int) attr_clock, (int) wait_clock);
> +
> +  if (pthread_condattr_init (&condattr) != 0)
> +    {
> +      puts ("condattr_init failed");
> +      return 1;
> +    }
> +
> +  if (pthread_condattr_setclock (&condattr, attr_clock) != 0)
> +    {
> +      puts ("condattr_setclock failed");
> +      return 1;
> +    }
> +
> +  clockid_t cl2;
> +  if (pthread_condattr_getclock (&condattr, &cl2) != 0)
> +    {
> +      puts ("condattr_getclock failed");
> +      return 1;
> +    }
> +  if (attr_clock != cl2)
> +    {
> +      printf ("condattr_getclock returned wrong value: %d, expected %d\n",
> +             (int) cl2, (int) attr_clock);
> +      return 1;
> +    }
> +
> +  if (pthread_cond_init (&cond, &condattr) != 0)
> +    {
> +      puts ("cond_init failed");
> +      return 1;
> +    }
> +
> +  if (pthread_condattr_destroy (&condattr) != 0)
> +    {
> +      puts ("condattr_destroy failed");
> +      return 1;
> +    }
> +
> +  if (pthread_mutexattr_init (&mutattr) != 0)
> +    {
> +      puts ("mutexattr_init failed");
> +      return 1;
> +    }
> +
> +  if (pthread_mutexattr_settype (&mutattr, PTHREAD_MUTEX_ERRORCHECK) != 0)
> +    {
> +      puts ("mutexattr_settype failed");
> +      return 1;
> +    }
> +
> +  if (pthread_mutex_init (&mut, &mutattr) != 0)
> +    {
> +      puts ("mutex_init failed");
> +      return 1;
> +    }
> +
> +  if (pthread_mutexattr_destroy (&mutattr) != 0)
> +    {
> +      puts ("mutexattr_destroy failed");
> +      return 1;
> +    }
> +
> +  if (pthread_mutex_lock (&mut) != 0)
> +    {
> +      puts ("mutex_lock failed");
> +      return 1;
> +    }
> +
> +  if (pthread_mutex_lock (&mut) != EDEADLK)
> +    {
> +      puts ("2nd mutex_lock did not return EDEADLK");
> +      return 1;
> +    }
> +
> +  struct timespec ts;
> +  if (clock_gettime (wait_clock, &ts) != 0)
> +    {
> +      puts ("clock_gettime failed");
> +      return 1;
> +    }
> +
> +  /* Wait one second.  */
> +  ++ts.tv_sec;
> +
> +  int e = pthread_cond_timedwaitonclock_np (&cond, &mut, wait_clock, &ts);
> +  if (e == 0)
> +    {
> +      puts ("cond_timedwait succeeded");
> +      return 1;
> +    }
> +  else if (e != ETIMEDOUT)
> +    {
> +      puts ("cond_timedwait did not return ETIMEDOUT");
> +      return 1;
> +    }
> +
> +  struct timespec ts2;
> +  if (clock_gettime (wait_clock, &ts2) != 0)
> +    {
> +      puts ("second clock_gettime failed");
> +      return 1;
> +    }
> +
> +  if (ts2.tv_sec < ts.tv_sec
> +      || (ts2.tv_sec == ts.tv_sec && ts2.tv_nsec < ts.tv_nsec))
> +    {
> +      puts ("timeout too short");
> +      return 1;
> +    }
> +
> +  if (pthread_mutex_unlock (&mut) != 0)
> +    {
> +      puts ("mutex_unlock failed");
> +      return 1;
> +    }
> +
> +  if (pthread_mutex_destroy (&mut) != 0)
> +    {
> +      puts ("mutex_destroy failed");
> +      return 1;
> +    }
> +
> +  if (pthread_cond_destroy (&cond) != 0)
> +    {
> +      puts ("cond_destroy failed");
> +      return 1;
> +    }
> +
> +  return 0;
> +}
> +#endif
> +
> +
> +static int
> +do_test (void)
> +{
> +#if !defined _POSIX_CLOCK_SELECTION || _POSIX_CLOCK_SELECTION == -1
> +
> +  puts ("_POSIX_CLOCK_SELECTION not supported, test skipped");
> +  return 0;
> +
> +#else
> +
> +  int res = run_test (CLOCK_REALTIME, CLOCK_REALTIME);
> +
> +# if defined _POSIX_MONOTONIC_CLOCK && _POSIX_MONOTONIC_CLOCK >= 0
> +#  if _POSIX_MONOTONIC_CLOCK == 0
> +  int e = sysconf (_SC_MONOTONIC_CLOCK);
> +  if (e < 0)
> +    puts ("CLOCK_MONOTONIC not supported");
> +  else if (e == 0)
> +    {
> +      puts ("sysconf (_SC_MONOTONIC_CLOCK) must not return 0");
> +      res = 1;
> +    }
> +  else
> +#  endif
> +    res |= run_test (CLOCK_REALTIME, CLOCK_MONOTONIC);
> +    res |= run_test (CLOCK_MONOTONIC, CLOCK_MONOTONIC);
> +    res |= run_test (CLOCK_MONOTONIC, CLOCK_REALTIME);
> +# else
> +  puts ("_POSIX_MONOTONIC_CLOCK not defined");
> +# endif
> +
> +  return res;
> +#endif
> +}
> +
> +#define TIMEOUT 12
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> diff --git a/nptl/tst-cond5-onclock.c b/nptl/tst-cond5-onclock.c
> new file mode 100644
> index 0000000..ddb0ad1
> --- /dev/null
> +++ b/nptl/tst-cond5-onclock.c
> @@ -0,0 +1,113 @@
> +/* Copyright (C) 2002-2014 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +   Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.
> +
> +   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/>.  */
> +
> +#include <errno.h>
> +#include <pthread.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <time.h>
> +#include <sys/time.h>
> +
> +
> +static pthread_mutex_t mut;
> +static pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
> +
> +
> +static int
> +do_test_onclock(clockid_t clockid)
> +{
> +  pthread_mutexattr_t ma;
> +  int err;
> +  struct timespec ts;
> +
> +  if (pthread_mutexattr_init (&ma) != 0)
> +    {
> +      puts ("mutexattr_init failed");
> +      exit (1);
> +    }
> +
> +  if (pthread_mutexattr_settype (&ma, PTHREAD_MUTEX_ERRORCHECK) != 0)
> +    {
> +      puts ("mutexattr_settype failed");
> +      exit (1);
> +    }
> +
> +  if (pthread_mutex_init (&mut, &ma) != 0)
> +    {
> +      puts ("mutex_init failed");
> +      exit (1);
> +    }
> +
> +  /* Get the mutex.  */
> +  if (pthread_mutex_lock (&mut) != 0)
> +    {
> +      puts ("mutex_lock failed");
> +      exit (1);
> +    }
> +
> +  /* Waiting for the condition will fail.  But we want the timeout here.  */
> +  if (clock_gettime (clockid, &ts) != 0)
> +    {
> +      puts ("clock_gettime failed");
> +      exit (1);
> +    }
> +
> +  ts.tv_nsec += 500000000;
> +  if (ts.tv_nsec >= 1000000000)
> +    {
> +      ts.tv_nsec -= 1000000000;
> +      ++ts.tv_sec;
> +    }
> +  err = pthread_cond_timedwaitonclock_np (&cond, &mut, clockid, &ts);
> +  if (err == 0)
> +    {
> +      /* This could in theory happen but here without any signal and
> +        additional waiter it should not.  */
> +      puts ("cond_timedwait succeeded");
> +      exit (1);
> +    }
> +  else if (err != ETIMEDOUT)
> +    {
> +      printf ("cond_timedwait returned with %s\n", strerror (err));
> +      exit (1);
> +    }
> +
> +  err = pthread_mutex_unlock (&mut);
> +  if (err != 0)
> +    {
> +      printf ("mutex_unlock failed: %s\n", strerror (err));
> +      exit (1);
> +    }
> +
> +  return 0;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  int rc;
> +  rc = do_test_onclock(CLOCK_MONOTONIC);
> +  if (rc == 0)
> +    rc = do_test_onclock(CLOCK_REALTIME);
> +
> +  return rc;
> +}
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> diff --git a/sysdeps/nptl/pthread.h b/sysdeps/nptl/pthread.h
> index 89d0882..221924b 100644
> --- a/sysdeps/nptl/pthread.h
> +++ b/sysdeps/nptl/pthread.h
> @@ -1002,6 +1002,19 @@ extern int pthread_cond_timedwait (pthread_cond_t *__restrict __cond,
>                                    const struct timespec *__restrict __abstime)
>       __nonnull ((1, 2, 3));
> 
> +/* Wait for condition variable COND to be signaled or broadcast until
> +   ABSTIME measured by the specified clock. MUTEX is assumed to be
> +   locked before. CLOCK is a clock specified. ABSTIME is an absolute
> +   time specification against CLOCK's epoch.
> +
> +   This function is a cancellation point and therefore not marked with
> +   __THROW. */
> +extern int pthread_cond_timedwaitonclock_np (pthread_cond_t *__restrict __cond,
> +                                            pthread_mutex_t *__restrict __mutex,
> +                                            __clockid_t __clock_id,
> +                                            const struct timespec *__restrict __abstime)
> +     __nonnull ((1, 2, 4));
> +
>  /* Functions for handling condition variable attributes.  */
> 
>  /* Initialize condition variable attribute ATTR.  */
> --
> 2.1.4
> 
> 
> 

Re: [RFC] Add pthread_cond_timedwaitonclock_np

[Mike Crowe]

On Tuesday 07 July 2015 at 15:28:56 +0100, Szabolcs Nagy wrote:
> On 07/07/15 14:56, Mike Crowe wrote:
> > C++11's std::condition_variable::wait_until specifies the clock to be
> > used at the time of the wait and permits separate waits on the same
> > instance using different clocks.
> > 
> > Unfortunately pthread_cond_timedwait always uses the clock that was
> > specified (or defaulted) when pthread_cond_init was called. libstdc++'s
> > current implementation converts all waits to
> > std::chrono::system_clock (i.e. CLOCK_REALTIME) which can race against
> > the system clock changing.
> > 
> > Inventing a brand-new function pthread_cond_timedwaitonclock_np which
> > accepts both the clock and the time point as parameters is
> > straightforward and means that the C++11 standard behaviour can be
> > implemented in libstdc++ on Linux at least.
> > 
> 
> do you have a reference to the rationale that introduced this api to C++?

Not exactly. Here are some interesting links:

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2009/n2999.html

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4486.html (search
for "887")

https://www.redhat.com/archives/posix-c++-wg/2009-July/msg00002.html
(search for "887")

https://github.com/cplusplus/LWG/blob/master/xml/issue0887.xml

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=41861

It seems that even more work than I was previously aware of has been
invested in trying to solve this over the past six years or so. :(

Mike.

Re: [RFC] Add pthread_cond_timedwaitonclock_np

[Ondřej Bílka]

On Tue, Jul 07, 2015 at 02:56:29PM +0100, Mike Crowe wrote:
> C++11's std::condition_variable::wait_until specifies the clock to be
> used at the time of the wait and permits separate waits on the same
> instance using different clocks.
> 
> Unfortunately pthread_cond_timedwait always uses the clock that was
> specified (or defaulted) when pthread_cond_init was called. libstdc++'s
> current implementation converts all waits to
> std::chrono::system_clock (i.e. CLOCK_REALTIME) which can race against
> the system clock changing.
> 
> Inventing a brand-new function pthread_cond_timedwaitonclock_np which
> accepts both the clock and the time point as parameters is
> straightforward and means that the C++11 standard behaviour can be
> implemented in libstdc++ on Linux at least.
> 
> (This patch is only to the C implementation of pthread_cond_timedwait so
> it is necessary to remove pthread_cond_timedwait.S and
> pthread_cond_wait.S to use it on x86 and x86_64 at the moment. If the
> approach is deemed acceptable I'll work on fixing those too along with
> the documentation.)
>
Assembly will be deleted soon by Torvald's patch. I didn't make any
sense in first place.

Re: [RFC] Add pthread_cond_timedwaitonclock_np

[Joseph Myers]

On Tue, 7 Jul 2015, Mike Crowe wrote:

> diff --git a/conform/data/pthread.h-data b/conform/data/pthread.h-data
> index c1e32c8..d52f298 100644
> --- a/conform/data/pthread.h-data
> +++ b/conform/data/pthread.h-data
> @@ -92,6 +92,9 @@ function int pthread_cond_destroy (pthread_cond_t*)
>  function int pthread_cond_init (pthread_cond_t*, const pthread_condattr_t*)
>  function int pthread_cond_signal (pthread_cond_t*)
>  function int pthread_cond_timedwait (pthread_cond_t*, pthread_mutex_t*, const struct timespec*)
> +#if !defined POSIX && !defined UNIX98 && !defined XOPEN2K
> +function int pthread_cond_timedwaitonclock_np (pthread_cond_t*, pthread_mutex_t*, clockid_t, const struct timespec*)
> +#endif

No, this is wrong.  A function not in any released version of POSIX should 
not be added to the conform/ data.

> diff --git a/nptl/Versions b/nptl/Versions
> index 34e4b46..476301f 100644
> --- a/nptl/Versions
> +++ b/nptl/Versions
> @@ -267,6 +267,7 @@ libpthread {
>    }
>  
>    GLIBC_2.22 {
> +    pthread_cond_timedwaitonclock_np;

New symbol versions require all the ABI test baselines to be updated as 
well, in the same patch adding the new ABI.

> diff --git a/nptl/tst-cond11-onclock.c b/nptl/tst-cond11-onclock.c
> new file mode 100644
> index 0000000..15b3730
> --- /dev/null
> +++ b/nptl/tst-cond11-onclock.c
> @@ -0,0 +1,206 @@
> +/* Copyright (C) 2003-2014 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +   Contributed by Ulrich Drepper <drepper@redhat.com>, 2003.

No "Contributed by" lines in new files.  2015 in copyright dates.

> diff --git a/sysdeps/nptl/pthread.h b/sysdeps/nptl/pthread.h
> index 89d0882..221924b 100644
> --- a/sysdeps/nptl/pthread.h
> +++ b/sysdeps/nptl/pthread.h
> @@ -1002,6 +1002,19 @@ extern int pthread_cond_timedwait (pthread_cond_t *__restrict __cond,
>  				   const struct timespec *__restrict __abstime)
>       __nonnull ((1, 2, 3));
>  
> +/* Wait for condition variable COND to be signaled or broadcast until
> +   ABSTIME measured by the specified clock. MUTEX is assumed to be
> +   locked before. CLOCK is a clock specified. ABSTIME is an absolute
> +   time specification against CLOCK's epoch.
> +
> +   This function is a cancellation point and therefore not marked with
> +   __THROW. */
> +extern int pthread_cond_timedwaitonclock_np (pthread_cond_t *__restrict __cond,
> +					     pthread_mutex_t *__restrict __mutex,
> +					     __clockid_t __clock_id,
> +					     const struct timespec *__restrict __abstime)
> +     __nonnull ((1, 2, 4));

New non-POSIX functions should be conditional on __USE_GNU in headers.

New APIs also need documentation in the glibc manual.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [RFC] Add pthread_cond_timedwaitonclock_np

[Mike Crowe]

On Wednesday 22 July 2015 at 13:53:58 +0000, Joseph Myers wrote:
> On Tue, 7 Jul 2015, Mike Crowe wrote:
> 
> > diff --git a/conform/data/pthread.h-data b/conform/data/pthread.h-data
> > index c1e32c8..d52f298 100644
> > --- a/conform/data/pthread.h-data
> > +++ b/conform/data/pthread.h-data
> > @@ -92,6 +92,9 @@ function int pthread_cond_destroy (pthread_cond_t*)
> >  function int pthread_cond_init (pthread_cond_t*, const pthread_condattr_t*)
> >  function int pthread_cond_signal (pthread_cond_t*)
> >  function int pthread_cond_timedwait (pthread_cond_t*, pthread_mutex_t*, const struct timespec*)
> > +#if !defined POSIX && !defined UNIX98 && !defined XOPEN2K
> > +function int pthread_cond_timedwaitonclock_np (pthread_cond_t*, pthread_mutex_t*, clockid_t, const struct timespec*)
> > +#endif
> 
> No, this is wrong.  A function not in any released version of POSIX should 
> not be added to the conform/ data.

OK.

> > diff --git a/nptl/Versions b/nptl/Versions
> > index 34e4b46..476301f 100644
> > --- a/nptl/Versions
> > +++ b/nptl/Versions
> > @@ -267,6 +267,7 @@ libpthread {
> >    }
> >  
> >    GLIBC_2.22 {
> > +    pthread_cond_timedwaitonclock_np;
> 
> New symbol versions require all the ABI test baselines to be updated as 
> well, in the same patch adding the new ABI.

OK. I'll move to GLIBC_2.23 too since 2.22 is now frozen.

> > diff --git a/nptl/tst-cond11-onclock.c b/nptl/tst-cond11-onclock.c
> > new file mode 100644
> > index 0000000..15b3730
> > --- /dev/null
> > +++ b/nptl/tst-cond11-onclock.c
> > @@ -0,0 +1,206 @@
> > +/* Copyright (C) 2003-2014 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> > +   Contributed by Ulrich Drepper <drepper@redhat.com>, 2003.
> 
> No "Contributed by" lines in new files.  2015 in copyright dates.

I can fix that (although since it's basically a copy of tst-cond11.c I
think that I need to keep Ulrich Drepper's notice too.)

> > diff --git a/sysdeps/nptl/pthread.h b/sysdeps/nptl/pthread.h
> > index 89d0882..221924b 100644
> > --- a/sysdeps/nptl/pthread.h
> > +++ b/sysdeps/nptl/pthread.h
> > @@ -1002,6 +1002,19 @@ extern int pthread_cond_timedwait (pthread_cond_t *__restrict __cond,
> >  				   const struct timespec *__restrict __abstime)
> >       __nonnull ((1, 2, 3));
> >  
> > +/* Wait for condition variable COND to be signaled or broadcast until
> > +   ABSTIME measured by the specified clock. MUTEX is assumed to be
> > +   locked before. CLOCK is a clock specified. ABSTIME is an absolute
> > +   time specification against CLOCK's epoch.
> > +
> > +   This function is a cancellation point and therefore not marked with
> > +   __THROW. */
> > +extern int pthread_cond_timedwaitonclock_np (pthread_cond_t *__restrict __cond,
> > +					     pthread_mutex_t *__restrict __mutex,
> > +					     __clockid_t __clock_id,
> > +					     const struct timespec *__restrict __abstime)
> > +     __nonnull ((1, 2, 4));
> 
> New non-POSIX functions should be conditional on __USE_GNU in headers.

OK.

> New APIs also need documentation in the glibc manual.

I'm having some trouble with the safety section. pthread_cond_timedwait
isn't documented in the glibc manual since it is a POSIX function.

The pthread_cond_timedwait man page tells me that pthread_cond_timedwait is
MT-Safe but it's presumably neither AC-Safe nor AS-Safe. I'm not sure what to put in
the asunsafe and acunsafe reasons. Cut and paste gave me:

 @safety{@prelim{}@mtsafe{}@asunsafe{@asulock{}}@acunsafe{@asulock}}

is this correct?

(I also plan to submit changes to the pthread_cond_timedwait man page.)

Thanks for the review.

Mike.

Re: [RFC] Add pthread_cond_timedwaitonclock_np

[Carlos O'Donell]

On 07/27/2015 03:00 PM, Mike Crowe wrote:
>> New APIs also need documentation in the glibc manual.
> 
> I'm having some trouble with the safety section. pthread_cond_timedwait
> isn't documented in the glibc manual since it is a POSIX function.
> 
> The pthread_cond_timedwait man page tells me that pthread_cond_timedwait is
> MT-Safe but it's presumably neither AC-Safe nor AS-Safe. I'm not sure what to put in
> the asunsafe and acunsafe reasons. Cut and paste gave me:

Yes, the pthread_* functions are by definition MT-safe.

>  @safety{@prelim{}@mtsafe{}@asunsafe{@asulock{}}@acunsafe{@asulock}}
> 
> is this correct?

Almost.

@safety{@prelim{}@mtsafe{}@asunsafe{@asulock{}}@acunsafe{@aculock}}

Note: Use of "aculock" for acunsafe. Here it matters that the lock is lost
      not that we deadlock.

With suggested source level comments like this:

@c If exactly the same function with arguments is called from a signal
@c handler that interrupts between the mutex unlock and sleep then it
@c will unlock the mutex twice resulting in undefined behaviour. Keep
@c in mind that the unlock and sleep are only atomic with respect to other
@c threads (really a happens-after relationship for pthread_cond_broadcast
@c and pthread_cond_signal).
@c In the AC case we would cancel the thread and the mutex would remain
@c locked and we can't recover from that.
 
> (I also plan to submit changes to the pthread_cond_timedwait man page.)
> 
> Thanks for the review.

This does not constitute a full review, and I think a lot more people need
to review this before we have consensus over a new API like this.

Cheers,
Carlos.

Re: [RFC] Add pthread_cond_timedwaitonclock_np

[Joseph Myers]

On Mon, 27 Jul 2015, Mike Crowe wrote:

> > > diff --git a/nptl/tst-cond11-onclock.c b/nptl/tst-cond11-onclock.c
> > > new file mode 100644
> > > index 0000000..15b3730
> > > --- /dev/null
> > > +++ b/nptl/tst-cond11-onclock.c
> > > @@ -0,0 +1,206 @@
> > > +/* Copyright (C) 2003-2014 Free Software Foundation, Inc.
> > > +   This file is part of the GNU C Library.
> > > +   Contributed by Ulrich Drepper <drepper@redhat.com>, 2003.
> > 
> > No "Contributed by" lines in new files.  2015 in copyright dates.
> 
> I can fix that (although since it's basically a copy of tst-cond11.c I
> think that I need to keep Ulrich Drepper's notice too.)

No.  The first line of a new file, before the copyright notice, should be 
a comment describing the file's contents.  You can include "based on 
tst-cond11.c" in that comment, but the place for identifying contributors 
is the list in the manual, the NEWS file entries for major contributions, 
etc. - not comments in the sources.  (Or depending on how large the 
differences are, consider making them conditional in tst-cond11.c so that 
tst-cond11-onclock.c can just #define TEST_ONCLOCK then include 
tst-cond11.c, and avoid duplication.)

-- 
Joseph S. Myers
joseph@codesourcery.com