A patch for linuxthreads

A patch for linuxthreads

[H . J . Lu]

Hi,

While working on the ia64 port for linuxthreads, I found
this patch was necessary. Any comments?

Thanks.


-- 
H.J. Lu (hjl@gnu.org)
---
Thu Dec 16 15:59:58 1999  H.J. Lu  <hjl@gnu.org>

	* manager.c (pthread_allocate_stack): Correct the calculation
	of "new_thread_bottom".

Index: manager.c
===================================================================
RCS file: /work/cvs/gnu/glibc-2.1/linuxthreads/manager.c,v
retrieving revision 1.1.1.22
diff -u -p -r1.1.1.22 manager.c
--- manager.c	1999/11/27 18:44:03	1.1.1.22
+++ manager.c	1999/12/17 00:10:18
@@ -291,7 +291,7 @@ static int pthread_allocate_stack(const 
     {
       /* Allocate space for stack and thread descriptor at default address */
       new_thread = default_new_thread;
-      new_thread_bottom = (char *) new_thread - STACK_SIZE;
+      new_thread_bottom = (char *) (new_thread + 1) - STACK_SIZE;
       if (mmap((caddr_t)((char *)(new_thread + 1) - INITIAL_STACK_SIZE),
                INITIAL_STACK_SIZE, PROT_READ | PROT_WRITE | PROT_EXEC,
                MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED | MAP_GROWSDOWN,

Re: A patch for linuxthreads

[H . J . Lu]

On Thu, Dec 16, 1999 at 04:33:18PM -0800, Geoff Keating wrote:
> > Date: Thu, 16 Dec 1999 16:12:43 -0800
> > From: "H . J . Lu" <hjl@valinux.com>
> > 
> > Hi,
> > 
> > While working on the ia64 port for linuxthreads, I found
> > this patch was necessary. Any comments?
> 
> Yes.
> 
> > -- 
> > H.J. Lu (hjl@gnu.org)
> > ---
> > Thu Dec 16 15:59:58 1999  H.J. Lu  <hjl@gnu.org>
> > 
> > 	* manager.c (pthread_allocate_stack): Correct the calculation
> > 	of "new_thread_bottom".
> > 
> > Index: manager.c
> > ===================================================================
> > RCS file: /work/cvs/gnu/glibc-2.1/linuxthreads/manager.c,v
> > retrieving revision 1.1.1.22
> > diff -u -p -r1.1.1.22 manager.c
> > --- manager.c	1999/11/27 18:44:03	1.1.1.22
> > +++ manager.c	1999/12/17 00:10:18
> > @@ -291,7 +291,7 @@ static int pthread_allocate_stack(const 
> >      {
> >        /* Allocate space for stack and thread descriptor at default address */
> >        new_thread = default_new_thread;
> > -      new_thread_bottom = (char *) new_thread - STACK_SIZE;
> > +      new_thread_bottom = (char *) (new_thread + 1) - STACK_SIZE;
> >        if (mmap((caddr_t)((char *)(new_thread + 1) - INITIAL_STACK_SIZE),
> >                 INITIAL_STACK_SIZE, PROT_READ | PROT_WRITE | PROT_EXEC,
> >                 MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED | MAP_GROWSDOWN,
> > 
> 
> 1. When you submit a patch, it is helpful if you explain what bug you
>    are fixing or what new feature you are adding.  This saves us from
>    having to deduce it by inspection of the code.  This is even more
>    important when you are implementing on hardware that is not readily
>    available.

I am not sure how much I am allowed to say. Let me just say we
need to access new_thread_bottom. If I understand the code
correctly, the stack top is (char *)(new_thread + 1) since we do

        if (mmap((caddr_t)((char *)(new_thread + 1) - INITIAL_STACK_SIZE),
                 INITIAL_STACK_SIZE, PROT_READ | PROT_WRITE | PROT_EXEC,
                 MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED | MAP_GROWSDOWN,

That means we mmap the top INITIAL_STACK_SIZE bytes from the stack
top and grow the stack downwards. If it is true, the stack bottom should
be 

(char *)(new_thread + 1) - STACK_SIZE

We didn't see any bug reports since not many archs need to access
the stack bottom. In fact, if you compute it,

(char *) new_thread - STACK_SIZE

usually is not page aligned.

> 
> 2. If you need to change this, don't you need to change the similar
>    calculation in the previous {} group a few lines above?

It is because the user should provide the correct stack top and
stack size.


-- 
H.J. Lu (hjl@gnu.org)

Re: A patch for linuxthreads

[Ulrich Drepper]

"H . J . Lu" <hjl@valinux.com> writes:

> page_roundup is faster than roundup. Since `ps' is returned from
> __getpagesize (), it should be ok.

I've applied the patch.  Thanks,

-- 
---------------.                          ,-.   1325 Chesapeake Terrace
Ulrich Drepper  \    ,-------------------'   \  Sunnyvale, CA 94089 USA
Red Hat          `--' drepper at redhat.com   `------------------------

A patch for linuxthreads

[H . J . Lu]

page_roundup is faster than roundup. Since `ps' is returned from
__getpagesize (), it should be ok.


-- 
H.J. Lu (hjl@gnu.org)
---
2000-09-04  H.J. Lu  <hjl@gnu.org>

	* attr.c (__pthread_attr_setguardsize): Use page_roundup
	instead of roundup to round up to the page size.

Index: attr.c
===================================================================
RCS file: /work/cvs/gnu/glibc/linuxthreads/attr.c,v
retrieving revision 1.1.1.2
diff -u -p -r1.1.1.2 attr.c
--- attr.c	2000/08/16 00:55:46	1.1.1.2
+++ attr.c	2000/08/17 16:18:05
@@ -147,7 +147,7 @@ int __pthread_attr_setguardsize(pthread_
   size_t ps = __getpagesize ();
 
   /* First round up the guard size.  */
-  guardsize = roundup (guardsize, ps);
+  guardsize = page_roundup (guardsize, ps);
 
   /* The guard size must not be larger than the stack itself */
   if (guardsize >= attr->__stacksize) return EINVAL;

Re: A patch for linuxthreads

[Ulrich Drepper]

hjl@varesearch.com (H.J. Lu) writes:

> This patch is almost a month old.

It's in now.  Thanks,

-- 
---------------.      drepper at gnu.org  ,-.   1325 Chesapeake Terrace
Ulrich Drepper  \    ,-------------------'   \  Sunnyvale, CA 94089 USA
Cygnus Solutions `--' drepper at cygnus.com   `------------------------

A patch for linuxthreads

[H.J. Lu]

Ulrich,

This patch is almost a month old.

1. If th->p_terminated is 1, th->p_specific is already deleted. If
you delete them again, you may corrupt the memory.
2. __libc_internal_tsd_get and __libc_internal_tsd_set are referenced
in malloc/malloc.c and defined in linuxthreads/no-tsd.c. We want to
override them with -lpthread. To do that, we have to make them global
in libc.so.

Thanks.

-- 
H.J. Lu (hjl@gnu.org)
---
Tue Mar 16 19:30:50 1999  H.J. Lu  <hjl@gnu.org>

	* specific.c (pthread_key_delete): Check th->p_terminated to
	if the thread is running.

	* Versions (__libc_internal_tsd_get, __libc_internal_tsd_set):
	Added to GLIBC_2.0 for libc.so.

Index: specific.c
===================================================================
RCS file: /local/work/cvs/gnu/glibc/linuxthreads/specific.c,v
retrieving revision 1.1.1.5
diff -u -p -r1.1.1.5 specific.c
--- specific.c	1998/12/01 16:22:30	1.1.1.5
+++ specific.c	1999/03/26 01:08:41
@@ -80,7 +80,7 @@ int pthread_key_delete(pthread_key_t key
   idx2nd = key % PTHREAD_KEY_2NDLEVEL_SIZE;
   th = self;
   do {
-    if (th->p_specific[idx1st] != NULL)
+    if (!th->p_terminated && th->p_specific[idx1st] != NULL)
       th->p_specific[idx1st][idx2nd] = NULL;
     th = th->p_nextlive;
   } while (th != self);
Index: Versions
===================================================================
RCS file: /local/work/cvs/gnu/glibc/linuxthreads/Versions,v
retrieving revision 1.1.1.3
diff -u -p -r1.1.1.3 Versions
--- Versions	1999/02/16 23:21:28	1.1.1.3
+++ Versions	1999/03/26 01:08:41
@@ -13,6 +13,9 @@ libc {
     pthread_mutexattr_getkind_np; pthread_mutexattr_setkind_np;
     pthread_self; pthread_setcancelstate; pthread_setcanceltype;
     pthread_setschedparam;
+
+    # Internal libc interface to libpthread
+    __libc_internal_tsd_get; __libc_internal_tsd_set;
   }
   GLIBC_2.1 {
     pthread_attr_init;

Re: A patch for linuxthreads

[H.J. Lu]

> 
> Forgive my possible ignorance, but if on an future SMP system, you can
> have two threads running on different CPUs at the same time in a
> shared memory system, and there is no `other' locking, the above seems
> like it needs a lock around it.
> 

Thanks for your concern. The function, pthread_key_delete, is protected
by

	pthread_mutex_lock(&pthread_keys_mutex);

	....

	pthread_mutex_unlock(&pthread_keys_mutex);

Have you tried glibc 2.1? It is great.


-- 
H.J. Lu (hjl@gnu.org)

Re: A patch for linuxthreads

[Mike Stump]

> From: hjl@lucon.org (H.J. Lu)
> To: libc-hacker@cygnus.com (GNU C Library)
> Date: Tue, 16 Mar 1999 20:28:23 -0800 (PST)

> The problem is we should not access those key data stuctures after
> a thread is terminared since they have been freed.

> The patch for Versions fixes another bug. 

> Index: specific.c
> ===================================================================
> RCS file: /work/cvs/gnu/glibc/linuxthreads/specific.c,v
> retrieving revision 1.1.1.5
> diff -u -p -r1.1.1.5 specific.c
> --- specific.c	1998/12/01 16:22:30	1.1.1.5
> +++ specific.c	1999/03/17 03:30:10
> @@ -80,7 +80,7 @@ int pthread_key_delete(pthread_key_t key
>    idx2nd = key % PTHREAD_KEY_2NDLEVEL_SIZE;
>    th = self;
>    do {
> -    if (th->p_specific[idx1st] != NULL)
> +    if (!th->p_terminated && th->p_specific[idx1st] != NULL)
>        th->p_specific[idx1st][idx2nd] = NULL;
>      th = th->p_nextlive;
>    } while (th != self);

Forgive my possible ignorance, but if on an future SMP system, you can
have two threads running on different CPUs at the same time in a
shared memory system, and there is no `other' locking, the above seems
like it needs a lock around it.

Re: A patch for linuxthreads

[H.J. Lu]

> 
>    From: hjl@lucon.org (H.J. Lu)
>    Date: Tue, 16 Mar 1999 20:28:23 -0800 (PST)
> 
>    The patch for Versions fixes another bug. 
> 
> Which one?  HJ, please explain the problem.  The __libc_internal
> functions are supposed to be internal, so without an explanation why
> we need to export them I'd say the patch is worng!
> 

If you take a look at malloc and linuxthreads, you will find that both
libc and linuxthreads define them and we are expecting "-lpthread"
will replace the ones in libc with the ones in linuxthreads. From
linuxthreads/Versions:

    # Internal libc interface to libpthread
    __libc_internal_tsd_get; __libc_internal_tsd_set;

what it means libc has to export them so that libpthread can override
them. I hope it is clear now.


-- 
H.J. Lu (hjl@gnu.org)

RE: A patch for linuxthreads

[Andrey Slepuhin]

On 17-Mar-99 H.J. Lu wrote:
> Hi,
> 
> This Linuxthreads patch should fix
> 
> http://egcs.cygnus.com/ml/egcs-bugs/1999-03/msg00316.html
> 
> The problem is we should not access those key data stuctures after
> a thread is terminared since they have been freed.

Does this mean that your previous message
( http://egcs.cygnus.com/ml/egcs-bugs/1999-03/msg00461.html )
was wrong or egcs still has a separate bug in thread-safe EH?
If so, do you have a testcase?

Regards,
Andrey.

Re: A patch for linuxthreads

[Mark Kettenis]

   From: hjl@lucon.org (H.J. Lu)
   Date: Tue, 16 Mar 1999 20:28:23 -0800 (PST)

   The patch for Versions fixes another bug. 

Which one?  HJ, please explain the problem.  The __libc_internal
functions are supposed to be internal, so without an explanation why
we need to export them I'd say the patch is worng!

Mark

A patch for linuxthreads

[H.J. Lu]

Hi,

This Linuxthreads patch should fix

http://egcs.cygnus.com/ml/egcs-bugs/1999-03/msg00316.html

The problem is we should not access those key data stuctures after
a thread is terminared since they have been freed.

The patch for Versions fixes another bug. 


-- 
H.J. Lu (hjl@gnu.org)
--
Tue Mar 16 19:30:50 1999  H.J. Lu  <hjl@gnu.org>

	* specific.c (pthread_key_delete): Check th->p_terminated to
	if the thread is running.

	* Versions (__libc_internal_tsd_get, __libc_internal_tsd_set):
	Added to GLIBC_2.0 for libc.so.

Index: Versions
===================================================================
RCS file: /work/cvs/gnu/glibc/linuxthreads/Versions,v
retrieving revision 1.1.1.3
diff -u -p -r1.1.1.3 Versions
--- Versions	1999/02/16 23:21:28	1.1.1.3
+++ Versions	1999/03/17 04:21:23
@@ -13,6 +13,9 @@ libc {
     pthread_mutexattr_getkind_np; pthread_mutexattr_setkind_np;
     pthread_self; pthread_setcancelstate; pthread_setcanceltype;
     pthread_setschedparam;
+
+    # Internal libc interface to libpthread
+    __libc_internal_tsd_get; __libc_internal_tsd_set;
   }
   GLIBC_2.1 {
     pthread_attr_init;
Index: specific.c
===================================================================
RCS file: /work/cvs/gnu/glibc/linuxthreads/specific.c,v
retrieving revision 1.1.1.5
diff -u -p -r1.1.1.5 specific.c
--- specific.c	1998/12/01 16:22:30	1.1.1.5
+++ specific.c	1999/03/17 03:30:10
@@ -80,7 +80,7 @@ int pthread_key_delete(pthread_key_t key
   idx2nd = key % PTHREAD_KEY_2NDLEVEL_SIZE;
   th = self;
   do {
-    if (th->p_specific[idx1st] != NULL)
+    if (!th->p_terminated && th->p_specific[idx1st] != NULL)
       th->p_specific[idx1st][idx2nd] = NULL;
     th = th->p_nextlive;
   } while (th != self);

Re: A patch for linuxthreads

[Ulrich Drepper]

hjl@lucon.org (H.J. Lu) writes:

> Wed Dec  2 08:40:43 1998  H.J. Lu  <hjl@gnu.org>
> 
> 	* pthread.c (__pthread_sig_restart): Initiliaze to -1 if
> 	SIGRTMIN is defined.
> 	(__pthread_sig_cancel): Likewise.

Let's use zero for the initializers (i.e., no initializer at all).
This moves the vars into .bss and since signal 0 cannot be used is
equally well to detect uninitialized variables.

-- 
---------------.      drepper at gnu.org  ,-.   1325 Chesapeake Terrace
Ulrich Drepper  \    ,-------------------'   \  Sunnyvale, CA 94089 USA
Cygnus Solutions `--' drepper at cygnus.com   `------------------------

A patch for linuxthreads

[H.J. Lu]

Please ignore my last patch for linuxthreads. This one should be used.

Thanks.


-- 
H.J. Lu (hjl@gnu.org)
---
Wed Dec  2 08:40:43 1998  H.J. Lu  <hjl@gnu.org>

	* pthread.c (__pthread_sig_restart): Initiliaze to -1 if
	SIGRTMIN is defined.
	(__pthread_sig_cancel): Likewise.

Index: pthread.c
===================================================================
RCS file: /home/work/cvs/gnu/glibc/linuxthreads/pthread.c,v
retrieving revision 1.1.1.8
diff -u -p -r1.1.1.8 pthread.c
--- pthread.c	1998/10/31 16:47:04	1.1.1.8
+++ pthread.c	1998/12/02 18:33:36
@@ -147,8 +147,13 @@ const int __pthread_offsetof_pid = offse
                                             p_pid);
 
 /* Signal numbers used for the communication.  */
+#ifdef SIGRTMIN
+int __pthread_sig_restart = -1;
+int __pthread_sig_cancel = -1;
+#else
 int __pthread_sig_restart = DEFAULT_SIG_RESTART;
 int __pthread_sig_cancel = DEFAULT_SIG_CANCEL;
+#endif
 
 /* These variables are used by the setup code.  */
 extern int _errno;

Re: A patch for linuxthreads

[Ulrich Drepper]

hjl@lucon.org (H.J. Lu) writes:

> They include <bits/pthreadtypes.h> or <bits/sigthread.h>. But I didn't
> see any changes.

I haven't comitted the changes.  I head to run all the tests first.
This will be finished in a minue and then I'll start comitting
everything.

-- 
---------------.      drepper at gnu.org  ,-.   1325 Chesapeake Terrace
Ulrich Drepper  \    ,-------------------'   \  Sunnyvale, CA 94089 USA
Cygnus Solutions `--' drepper at cygnus.com   `------------------------

Re: A patch for linuxthreads

[H.J. Lu]

> 
> hjl@lucon.org (H.J. Lu) writes:
> 
> > Sat Oct 24 18:15:53 1998  H.J. Lu  <hjl@gnu.org>
> > 
> > 	* linuxthreads/sysdeps/pthread/bits/pthreadtypes.h: Protect
> > 	with __USE_REENTRANT, __USE_MISC, __USE_GNU and _PTHREAD_H.
> > 	* linuxthreads/sysdeps/unix/sysv/linux/bits/sigthread.h:
> > 	Likewise.
> 
> This patch is not ok.  The changes must be made wherever these files
> get included.  I think I did it correctly now.  Thanks,
> 

I don't think so. The problem is in

sysdeps/unix/sysv/linux/alpha/bits/types.h
sysdeps/unix/sysv/linux/mips/bits/types.h
sysdeps/unix/sysv/linux/sparc/sparc64/bits/types.h
sysdeps/unix/sysv/linux/bits/types.h
signal/signal.h

They include <bits/pthreadtypes.h> or <bits/sigthread.h>. But I didn't
see any changes.

-- 
H.J. Lu (hjl@gnu.org)

Re: A patch for linuxthreads

[Ulrich Drepper]

hjl@lucon.org (H.J. Lu) writes:

> Sat Oct 24 18:15:53 1998  H.J. Lu  <hjl@gnu.org>
> 
> 	* linuxthreads/sysdeps/pthread/bits/pthreadtypes.h: Protect
> 	with __USE_REENTRANT, __USE_MISC, __USE_GNU and _PTHREAD_H.
> 	* linuxthreads/sysdeps/unix/sysv/linux/bits/sigthread.h:
> 	Likewise.

This patch is not ok.  The changes must be made wherever these files
get included.  I think I did it correctly now.  Thanks,

-- 
---------------.      drepper at gnu.org  ,-.   1325 Chesapeake Terrace
Ulrich Drepper  \    ,-------------------'   \  Sunnyvale, CA 94089 USA
Cygnus Solutions `--' drepper at cygnus.com   `------------------------

A patch for linuxthreads

[H.J. Lu]

We should not include them indiscriminately.


-- 
H.J. Lu (hjl@gnu.org)
---
Sat Oct 24 18:15:53 1998  H.J. Lu  <hjl@gnu.org>

	* linuxthreads/sysdeps/pthread/bits/pthreadtypes.h: Protect
	with __USE_REENTRANT, __USE_MISC, __USE_GNU and _PTHREAD_H.
	* linuxthreads/sysdeps/unix/sysv/linux/bits/sigthread.h:
	Likewise.

Index: linuxthreads/sysdeps/pthread/bits/pthreadtypes.h
===================================================================
RCS file: /home/work/cvs/gnu/glibc/linuxthreads/sysdeps/pthread/bits/pthreadtypes.h,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 pthreadtypes.h
--- pthreadtypes.h	1998/07/01 02:47:57	1.1.1.1
+++ pthreadtypes.h	1998/10/22 14:22:52
@@ -19,6 +19,9 @@
 #ifndef _BITS_PTHREADTYPES_H
 #define _BITS_PTHREADTYPES_H	1
 
+#if defined __USE_REENTRANT || defined __USE_MISC || \
+    defined __USE_GNU || defined _PTHREAD_H
+
 #define __need_schedparam
 #include <bits/sched.h>
 
@@ -115,5 +118,7 @@
 
 /* Thread identifiers */
 typedef unsigned long int pthread_t;
+
+#endif
 
 #endif	/* bits/pthreadtypes.h */
Index: linuxthreads/sysdeps/unix/sysv/linux/bits/sigthread.h
===================================================================
RCS file: /home/work/cvs/gnu/glibc/linuxthreads/sysdeps/unix/sysv/linux/bits/sigthread.h,v
retrieving revision 1.1.1.3
diff -u -r1.1.1.3 sigthread.h
--- sigthread.h	1998/09/14 16:08:53	1.1.1.3
+++ sigthread.h	1998/10/22 14:23:06
@@ -24,6 +24,9 @@
 # error "Never include this file directly.  Use <pthread.h> instead"
 #endif
 
+#if defined __USE_REENTRANT || defined __USE_MISC || \
+    defined __USE_GNU || defined _PTHREAD_H
+
 /* Functions for handling signals. */
 
 /* Modify the signal mask for the calling thread.  The arguments have
@@ -33,5 +36,7 @@
 
 /* Send signal SIGNO to the given thread. */
 extern int pthread_kill __P ((pthread_t __thread, int __signo));
+
+#endif
 
 #endif	/* bits/sigthread.h */

Re: A patch for linuxthreads

[Ulrich Drepper]

hjl@lucon.org (H.J. Lu) writes:

> Hi,
> 
> Here is a patch for linuxthreads.

Thanks, it's in.

-- 
---------------.      drepper at gnu.org  ,-.   1325 Chesapeake Terrace
Ulrich Drepper  \    ,-------------------'   \  Sunnyvale, CA 94089 USA
Cygnus Solutions `--' drepper at cygnus.com   `------------------------

A patch for linuxthreads

[H.J. Lu]

Hi,

Here is a patch for linuxthreads.


-- 
H.J. Lu (hjl@gnu.org)
---
Sat Aug  8 11:18:52 1998  H.J. Lu  (hjl@gnu.org)

	* signals.c (sigaction): Handle NULL argument.

Index: signals.c
===================================================================
RCS file: /home/work/cvs/gnu/glibc/linuxthreads/signals.c,v
retrieving revision 1.1.1.3
diff -u -p -r1.1.1.3 signals.c
--- signals.c	1998/07/30 18:36:36	1.1.1.3
+++ signals.c	1998/08/08 18:10:18
@@ -93,16 +93,24 @@ int sigaction(int sig, const struct siga
               struct sigaction * oact)
 {
   struct sigaction newact;
+  struct sigaction *newactp;
 
   if (sig == __pthread_sig_restart || sig == __pthread_sig_cancel)
     return EINVAL;
-  newact = *act;
-  if (act->sa_handler != SIG_IGN && act->sa_handler != SIG_DFL)
-    newact.sa_handler = pthread_sighandler;
-  if (__sigaction(sig, &newact, oact) == -1)
+  if (act)
+    {
+      newact = *act;
+      if (act->sa_handler != SIG_IGN && act->sa_handler != SIG_DFL)
+	newact.sa_handler = pthread_sighandler;
+      newactp = &newact;
+    }
+  else
+    newactp = NULL;
+  if (__sigaction(sig, newactp, oact) == -1)
     return -1;
   if (oact != NULL) oact->sa_handler = sighandler[sig];
-  sighandler[sig] = act->sa_handler;
+  if (act)
+    sighandler[sig] = act->sa_handler;
   return 0;
 }