Re: sparc nptl mystery resolved

[Roland McGrath <roland at hack dot frob dot com>]

> The difference is that for sparc 32-bit we put an explicit byte
> spinlock into the semaphore structures since pre-v9 32-bit we lack
> atomic operations other than "ldstub" (load and store byte atomic).

Thanks for that explanation.  It's interesting to know.  But I actually
didn't need to understand it at all to make the suggestion I'm making.

Purely going from reading the file, the diff for your sem_wait.c is just this:

	--- nptl/sem_wait.c
	+++ sysdeps/sparc/nptl/sem_wait.c
	@@ -25,14 +25,13 @@

	 #include <pthreadP.h>
	 #include <shlib-compat.h>
	-#include <atomic.h>


	 void
	 attribute_hidden
	 __sem_wait_cleanup (void *arg)
	 {
	-  struct new_sem *isem = (struct new_sem *) arg;
	+  struct sparc_new_sem *isem = (struct sparc_new_sem *) arg;

	   atomic_decrement (&isem->nwaiters);
	 }
	@@ -42,7 +41,7 @@ __sem_wait_cleanup (void *arg)
	    cleanups get properly run.  */
	 static int
	 __attribute__ ((noinline))
	-do_futex_wait (struct new_sem *isem)
	+do_futex_wait (struct sparc_new_sem *isem)
	 {
	   int err, oldtype = __pthread_enable_asynccancel ();

	@@ -55,7 +54,7 @@ do_futex_wait (struct new_sem *isem)
	 int
	 __new_sem_wait (sem_t *sem)
	 {
	-  struct new_sem *isem = (struct new_sem *) sem;
	+  struct sparc_new_sem *isem = (struct sparc_new_sem *) sem;
	   int err;

	   if (atomic_decrement_if_positive (&isem->value) > 0)
	@@ -96,19 +95,19 @@ int
	 attribute_compat_text_section
	 __old_sem_wait (sem_t *sem)
	 {
	-  int *futex = (int *) sem;
	+  struct sparc_old_sem *isem = (struct sparc_old_sem *) sem;
	   int err;

	   do
	     {
	-      if (atomic_decrement_if_positive (futex) > 0)
	+      if (atomic_decrement_if_positive (&isem->value) > 0)
		return 0;

	       /* Enable asynchronous cancellation.  Required by the standard.  */
	       int oldtype = __pthread_enable_asynccancel ();

	-      /* Always assume the semaphore is shared.  */
	-      err = lll_futex_wait (futex, 0, LLL_SHARED);
	+      err = lll_futex_wait (&isem->value, 0,
	+			    isem->private ^ FUTEX_PRIVATE_FLAG);

	       /* Disable asynchronous cancellation.  */
	       __pthread_disable_asynccancel (oldtype);

sem_timedwait.c differs even less.  (sem_post.c differs enough that trying
to share it is probably not worthwhile, and sem_init.c perhaps the same.)

So what you could do is:
1. Move struct {new,old}_sem and anything related out of internaltypes.h
   into a new header, say sem-internal.h.  The sysdeps/nptl/ file will get
   the current stuff just moved out.  (We could then make internaltypes.h
   #include <sem-internal.h> but I tend to think it's cleaner to just change
   sem_*.c to include what they need directly, since nothing else will need
   it.)  Then you can have a new sysdeps/sparc/nptl/sem-internal.h that
   defines your types but with the same {new,old}_sem names.
2. Clean up the existing shared nptl/sem_wait.c to use struct old_sem
   instead of int (actually not necessary because yours starts with the
   value field too, but bletch).
3. Add a macro to sem-internal.h for OLD_SEM_FUTEX_PRIVATE (old_sem) and
   and change nptl/sem_*wait.c to use it.
4. Remove sysdeps/sparc/nptl/sem_*wait.c and be happy.

Or something not exactly that, but along those lines.  I didn't try to grok
the sem_init and sem_post code enough to see if there are actually similar
opportunities there or not.

Of course, it's up to you what you think will make for the easiest
maintenance in the future.  But if I were you, I'd want to share the common
code more and duplicate it less.


Thanks,
Roland