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

[Will Newton]

On Tue, Mar 10, 2009 at 12:23 PM, Denys Vlasenko
<vda.linux at googlemail.com> wrote:
> 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?

The bug is this one:

http://sourceware.org/ml/libc-ports/2005-10/msg00025.html

>> 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.

They are defined in the TLS code from glibc. They do not have any
particular effect if they are not defined but they reduce the diff
from upstream LinuxThreads.

>   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.

As above.

>          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).

Ok. This allows pthreads to share Sys V semaphores in order to get
appropriate SEM_UNDO semantics.

> +    /* 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?

http://sources.redhat.com/ml/libc-ports/2006-05/msg00000.html

> -     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).

It's trade off with reducing the diff from upstream LinuxThreads. It's
easy to break code cleaning it up and e.g. losing a !.

>   __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?

It looks like a possible race of some kind. I am not aware of the details.

>  #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?

The spinlock unlock issue. That was the main issue that I needed to
fix with this patch set, most of the rest is just synching up with
LinuxThreads HEAD so we can more easily compare and merge in the
future.