[PATCH] BZ#3123 fallouts patch

[Jakub Jelinek <jakub at redhat dot com>]

Hi!

Thanks for the fix, here are just some nits I found while trying to understand.
tst-cond22.c doesn't use anything from pthreadP.h what is not in the public
pthread.h, so I think it should use the public header instead.
But, more importantly, so far we were always holding the invariant that
cv->__data.__futex == (unsigned int) (cv->__data.__total_seq + cv->__data.__wakeup_seq)
but with today's changes that's no longer true and I think it is likely
to cause problems (don't have a testcase, but I fear e.g.
pthread_cond_broadcast could suddenly result in no change in __futex value
whatsoever and some threads might not notice something changed).
All other places that change the values of these 3 variables honor
this invariant (e.g. pthread_cond_signal increases both futex and
wakeup_seq, pthread_cond_wait increases both total_seq and futex
and pthread_cond_broadcast sets wakeup_seq to total_seq and afterwards sets
futex to (unsigned int) (2 * total_seq), i.e. the same as
(unsigned int) (total_seq + wakeup_seq).

2006-09-08  Jakub Jelinek  <jakub@redhat.com>

	* tst-cond22.c: Include pthread.h instead of pthreadP.h.
	Include stdlib.h.
	* sysdeps/pthread/pthread_cond_wait.c (__condvar_cleanup): Only
	increase FUTEX if increasing WAKEUP_SEQ.  Fix comment typo.
	* sysdeps/unix/sysv/linux/i386/i486/pthread_cond_wait.S: Likewise.
	* sysdeps/unix/sysv/linux/i386/i486/pthread_cond_timedwait.S: Likewise.
	* sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S: Likewise.

--- libc/nptl/tst-cond22.c	8 Sep 2006 10:40:24 -0000	1.1
+++ libc/nptl/tst-cond22.c	8 Sep 2006 12:59:45 -0000
@@ -1,5 +1,6 @@
-#include <pthreadP.h>
+#include <pthread.h>
 #include <stdio.h>
+#include <stdlib.h>
 
 
 static pthread_barrier_t b;
--- libc/nptl/sysdeps/pthread/pthread_cond_wait.c	8 Sep 2006 10:39:42 -0000	1.17
+++ libc/nptl/sysdeps/pthread/pthread_cond_wait.c	8 Sep 2006 12:59:45 -0000
@@ -51,13 +51,15 @@ __condvar_cleanup (void *arg)
     {
       /* This thread is not waiting anymore.  Adjust the sequence counters
 	 appropriately.  We do not increment WAKEUP_SEQ if this would
-	 bump it over the value of TOTAL_SEQ>  This can happen if a thread
+	 bump it over the value of TOTAL_SEQ.  This can happen if a thread
 	 was woken and then canceled.  */
       if (cbuffer->cond->__data.__wakeup_seq
 	  < cbuffer->cond->__data.__total_seq)
-	++cbuffer->cond->__data.__wakeup_seq;
+	{
+	  ++cbuffer->cond->__data.__wakeup_seq;
+	  ++cbuffer->cond->__data.__futex;
+	}
       ++cbuffer->cond->__data.__woken_seq;
-      ++cbuffer->cond->__data.__futex;
     }
 
   cbuffer->cond->__data.__nwaiters -= 1 << COND_CLOCK_BITS;
--- libc/nptl/sysdeps/unix/sysv/linux/i386/i486/pthread_cond_timedwait.S	8 Sep 2006 10:39:42 -0000	1.38
+++ libc/nptl/sysdeps/unix/sysv/linux/i386/i486/pthread_cond_timedwait.S	8 Sep 2006 12:59:45 -0000
@@ -406,7 +406,7 @@ __condvar_tw_cleanup:
 	cmpl	20(%esp), %eax
 	jne	3f
 
-	/* We increment the woken_seq counter only if it is lower than
+	/* We increment the wakeup_seq counter only if it is lower than
 	   total_seq.  If this is not the case the thread was woken and
 	   then canceled.  In this case we ignore the signal.  */
 	movl	total_seq(%ebx), %eax
@@ -419,10 +419,9 @@ __condvar_tw_cleanup:
 
 6:	addl	$1, wakeup_seq(%ebx)
 	adcl	$0, wakeup_seq+4(%ebx)
+	addl	$1, cond_futex(%ebx)
 
-7:	addl	$1, cond_futex(%ebx)
-
-	addl	$1, woken_seq(%ebx)
+7:	addl	$1, woken_seq(%ebx)
 	adcl	$0, woken_seq+4(%ebx)
 
 3:	subl	$(1 << clock_bits), cond_nwaiters(%ebx)
--- libc/nptl/sysdeps/unix/sysv/linux/i386/i486/pthread_cond_wait.S	8 Sep 2006 10:39:42 -0000	1.33
+++ libc/nptl/sysdeps/unix/sysv/linux/i386/i486/pthread_cond_wait.S	8 Sep 2006 12:59:45 -0000
@@ -297,7 +297,7 @@ __condvar_w_cleanup:
 	cmpl	12(%esp), %eax
 	jne	3f
 
-	/* We increment the woken_seq counter only if it is lower than
+	/* We increment the wakeup_seq counter only if it is lower than
 	   total_seq.  If this is not the case the thread was woken and
 	   then canceled.  In this case we ignore the signal.  */
 	movl	total_seq(%ebx), %eax
@@ -310,8 +310,9 @@ __condvar_w_cleanup:
 
 6:	addl	$1, wakeup_seq(%ebx)
 	adcl	$0, wakeup_seq+4(%ebx)
+	addl	$1, cond_futex(%ebx)
 
-7:	addl	$1, cond_futex(%ebx)
+7:
 	addl	$1, woken_seq(%ebx)
 	adcl	$0, woken_seq+4(%ebx)
 
--- libc/nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S	8 Sep 2006 10:39:41 -0000	1.23
+++ libc/nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S	8 Sep 2006 12:59:45 -0000
@@ -67,15 +67,15 @@ __condvar_cleanup:
 	cmpl	4(%r8), %edx
 	jne	3f
 
-	/* We increment the woken_seq counter only if it is lower than
+	/* We increment the wakeup_seq counter only if it is lower than
 	   total_seq.  If this is not the case the thread was woken and
 	   then canceled.  In this case we ignore the signal.  */
 	movq	total_seq(%rdi), %rax
 	cmpq	wakeup_seq(%rdi), %rax
 	jbe	6f
 	incq	wakeup_seq(%rdi)
-6:	incq	woken_seq(%rdi)
 	incl	cond_futex(%rdi)
+6:	incq	woken_seq(%rdi)
 
 3:	subl	$(1 << clock_bits), cond_nwaiters(%rdi)
 
--- libc/nptl/ChangeLog	8 Sep 2006 10:41:17 -0000	1.919
+++ libc/nptl/ChangeLog	8 Sep 2006 12:59:45 -0000
@@ -3,9 +3,9 @@
 	[BZ #3123]
 	* sysdeps/pthread/pthread_cond_wait.c (__condvar_cleanup): Don't
 	increment WAKEUP_SEQ if this would increase the value beyond TOTAL_SEQ.
-	* sysdeps/unix/sysv/linux/i386/i486/pthread_cond_wait.c: Likewise.
-	* sysdeps/unix/sysv/linux/i386/i486/pthread_cond_timedwait.c: Likewise.
-	* sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.c: Likewise.
+	* sysdeps/unix/sysv/linux/i386/i486/pthread_cond_wait.S: Likewise.
+	* sysdeps/unix/sysv/linux/i386/i486/pthread_cond_timedwait.S: Likewise.
+	* sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S: Likewise.
 	* Makefile (tests): Add tst-cond22.
 	* tst-cond22.c: New file.
 

	Jakub