svn commit: branches/uClibc-nptl: libc/sysdeps/linux/common libpthread/lin etc...

[Denys Vlasenko]

On Thursday 26 February 2009 05:27:50 pm Will Newton wrote:
> On Thu, Feb 26, 2009 at 4:06 PM, Denys Vlasenko
> <vda.linux at googlemail.com> wrote:
> 
> >> Also,
> >> what mechanism does linuxthreads_new use to make sure it's using the
> >> functions in libc, rather than something overridden by the application?
> >
> > Have no idea. There is no evidence linuxthreads_new works for
> > any architecture. And no, I'm not going to check 12 architectures
> > just because you want to know it for sure. That had to be done by
> > the submitter of that code, at the time of submission.
> > Obviously, [s]he did not even test it on the most easily available
> > arch, x86.
> 
> The new linuxthreads does work and is at this moment shipping in
> products. I posted a patch some time ago to update it to the latest
> version of linuxthreads upstream (and fix a serious bug in the
> process)

Which part of the patch fixes the bug? What the bug is?

> but it does not seem to have been applied. I've attached it 
> again if anyone's interested.

Applying it as is without understanding what I am fixing
would be a bit dangerous. Need your comments.

--- metag-uClibc2/libpthread/linuxthreads/descr.h       5 Feb 2008 14:49:33 -0000       1.2
+++ metag-uClibc2/libpthread/linuxthreads/descr.h       23 Apr 2008 13:27:00 -0000
@@ -123,9 +123,9 @@
       union dtv *dtvp;
       pthread_descr self;      /* Pointer to this structure */
       int multiple_threads;
-# ifdef NEED_DL_SYSINFO
       uintptr_t sysinfo;
-# endif
+      uintptr_t stack_guard;
+      uintptr_t pointer_guard;
...
+  /* Copy the stack guard canary.  */
+#ifdef THREAD_COPY_STACK_GUARD
+  THREAD_COPY_STACK_GUARD (new_thread);
+#endif
+
+  /* Copy the pointer guard value.  */
+#ifdef THREAD_COPY_POINTER_GUARD
+  THREAD_COPY_POINTER_GUARD (new_thread);
+#endif

these members appear to be only written to.
Why do you need write-only members?
THREAD_COPY_STACK_GUARD and THREAD_COPY_POINTER_GUARD
are not defined anywhere.


   size_t p_alloca_cutoff;      /* Maximum size which should be allocated
                                   using alloca() instead of malloc().  */
   /* New elements must be added at the end.  */
+
+  /* This member must be last.  */
+  char end_padding[];
+
+#define PTHREAD_STRUCT_END_PADDING \
+  (sizeof (struct _pthread_descr_struct)                             \
+   - offsetof (struct _pthread_descr_struct, end_padding))
 } __attribute__ ((aligned(32))); /* We need to align the structure so that
                                    doubles are aligned properly.  This is 8
                                    bytes on MIPS and 16 bytes on MIPS64.

PTHREAD_STRUCT_END_PADDING is not used anywhere.


          pid = __clone2(__pthread_manager_event,
                         (void **) __pthread_manager_thread_bos,
                         THREAD_MANAGER_STACK_SIZE,
-                        CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND,
+                        CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND | CLONE_SYSVSEM,
                         mgr);

What does it fix? (Imagine that know almost nothing about threads).


+    /* Make sure we come back here after suspend(), in case we entered
+       from a signal handler.  */
+    THREAD_SETMEM(self, p_signal_jmp, NULL);
+

what does this fix?


-     If no threads have been created yet, clear it just in the
-     current thread.  */
+     If no threads have been created yet, or if we are exiting, clear
+     it just in the current thread.  */

   struct pthread_key_delete_helper_args args;
   args.idx1st = key / PTHREAD_KEY_2NDLEVEL_SIZE;
   args.idx2nd = key % PTHREAD_KEY_2NDLEVEL_SIZE;
-  if (__pthread_manager_request != -1)
+  if (__pthread_manager_request != -1
+      && !(__builtin_expect (__pthread_exit_requested, 0)))

what does this fix? (I'd also remove __builtin_expect, it uglifies code
much more than it helps wrt performance).


   __pthread_lock(THREAD_GETMEM(self, p_lock), self);
   for (i = 0; i < PTHREAD_KEY_1STLEVEL_SIZE; i++) {
     if (THREAD_GETMEM_NC(self, p_specific[i]) != NULL) {
-      free(THREAD_GETMEM_NC(self, p_specific[i]));
+      void *p = THREAD_GETMEM_NC(self, p_specific[i]);
       THREAD_SETMEM_NC(self, p_specific[i], NULL);
+      free(p);
     }
   }

what does this fix?


 #if defined HAS_COMPARE_AND_SWAP
        wait_node_dequeue(pp_head, pp_max_prio, p_max_prio);
 #endif
+
+      /* Release the spinlock before restarting.  */
+#if defined TEST_FOR_COMPARE_AND_SWAP
+      if (!__pthread_has_cas)
+#endif
+#if !defined HAS_COMPARE_AND_SWAP || defined TEST_FOR_COMPARE_AND_SWAP
+       {
+         __pthread_release(&lock->__spinlock);
+       }
+#endif
+
       restart(p_max_prio->thr);
-      break;
+
+      return;
     }
   }

what does this fix?

--
vda