[basic-improvements] Remove __gthread_key_dtor

[basic-improvements] Remove __gthread_key_dtor

[Zack Weinberg]

The __gthread_key_dtor routine does nothing useful in any gthr-*.h
implementation.  This patch removes it, which in turn allows some
minor simplifications in unwind-sjlj.c.

Tested by building an i386-linux -> i386-aout cross compiler,
which includes a compilation of unwind-sjlj.c.

zw

	* gthr.h, gthr-dce.h, gthr-posix.h, gthr-rtems.h,
	gthr-solaris.h, gthr-win32.h: Remove __gthread_key_dtor.
	* unwind-sjlj.c (fc_key_dtor): Delete.
	(fc_key_init): Adjust __gthread_key_create call to match.

===================================================================
Index: gthr-dce.h
--- gthr-dce.h	1 Oct 2002 17:31:48 -0000	1.17.8.1
+++ gthr-dce.h	29 Oct 2002 22:20:12 -0000
@@ -425,13 +425,6 @@ __gthread_key_create (__gthread_key_t *k
 }
 
 static inline int
-__gthread_key_dtor (UNUSED (__gthread_key_t key), UNUSED (void *ptr))
-{
-  /* Nothing needed.  */
-  return 0;
-}
-
-static inline int
 __gthread_key_delete (UNUSED (__gthread_key_t key))
 {
   /* Operation is not supported.  */
===================================================================
Index: gthr-posix.h
--- gthr-posix.h	1 Oct 2002 17:31:48 -0000	1.22.8.1
+++ gthr-posix.h	29 Oct 2002 22:20:12 -0000
@@ -450,16 +450,6 @@ __gthread_key_create (__gthread_key_t *k
 }
 
 static inline int
-__gthread_key_dtor (__gthread_key_t key, void *ptr)
-{
-  /* Just reset the key value to zero.  */
-  if (ptr)
-    return pthread_setspecific (key, 0);
-  else
-    return 0;
-}
-
-static inline int
 __gthread_key_delete (__gthread_key_t key)
 {
   return pthread_key_delete (key);
===================================================================
Index: gthr-rtems.h
--- gthr-rtems.h	4 Jun 2002 11:30:45 -0000	1.7
+++ gthr-rtems.h	29 Oct 2002 22:20:12 -0000
@@ -53,7 +53,6 @@ typedef void *__gthread_mutex_t;
 /* generic per task variables */
 extern int rtems_gxx_once (__gthread_once_t *once, void (*func) (void));
 extern int rtems_gxx_key_create (__gthread_key_t *key, void (*dtor) (void *));
-extern int rtems_gxx_key_dtor (__gthread_key_t key, void *ptr);
 extern int rtems_gxx_key_delete (__gthread_key_t key);
 extern void *rtems_gxx_getspecific (__gthread_key_t key);
 extern int rtems_gxx_setspecific (__gthread_key_t key, const void *ptr);
@@ -83,12 +82,6 @@ static inline int
 __gthread_key_create (__gthread_key_t *key, void (*dtor) (void *))
 {
   return rtems_gxx_key_create( key, dtor );
-}
-
-static inline int
-__gthread_key_dtor (__gthread_key_t key, void *ptr)
-{
-   return rtems_gxx_key_dtor(key, ptr);
 }
 
 static inline int
===================================================================
Index: gthr-solaris.h
--- gthr-solaris.h	4 Jun 2002 11:30:46 -0000	1.13
+++ gthr-solaris.h	29 Oct 2002 22:20:12 -0000
@@ -418,13 +418,6 @@ __gthread_key_create (__gthread_key_t *k
 }
 
 static inline int
-__gthread_key_dtor (__gthread_key_t key, void *ptr)
-{
-  /* Nothing needed.  */
-  return 0;
-}
-
-static inline int
 __gthread_key_delete (__gthread_key_t key)
 {
   /* Not possible.  */
===================================================================
Index: gthr-win32.h
--- gthr-win32.h	1 Oct 2002 17:31:48 -0000	1.17.8.1
+++ gthr-win32.h	29 Oct 2002 22:20:12 -0000
@@ -396,13 +396,6 @@ __gthread_key_create (__gthread_key_t *k
   return __gthr_win32_key_create (key, dtor);
 }
 
-static inline int
-__gthread_key_dtor (__gthread_key_t key, void *ptr)
-{
-  /* Nothing needed.  */
-  return 0;
-}
-  
  static inline int
 __gthread_key_delete (__gthread_key_t key)
 {
@@ -509,15 +502,6 @@ __gthread_key_create (__gthread_key_t *k
   else
     status = (int) GetLastError ();
   return status;
-}
-
-/* Currently, this routine is called only for Mingw runtime, and if
-   -mthreads option is chosen to link in the thread support DLL.  */
-static inline int
-__gthread_key_dtor (__gthread_key_t key, void *ptr)
-{
-  /* Nothing needed.  */
-  return 0;
 }
 
 static inline int
===================================================================
Index: gthr.h
--- gthr.h	4 Jun 2002 11:30:44 -0000	1.12
+++ gthr.h	29 Oct 2002 22:20:12 -0000
@@ -62,8 +62,6 @@ Software Foundation, 59 Temple Place - S
      int __gthread_key_create (__gthread_key_t *keyp, void (*dtor) (void *))
      int __gthread_key_delete (__gthread_key_t key)
 
-     int __gthread_key_dtor (__gthread_key_t key, void *ptr)
-
      void *__gthread_getspecific (__gthread_key_t key)
      int __gthread_setspecific (__gthread_key_t key, const void *ptr)
 
===================================================================
Index: unwind-sjlj.c
--- unwind-sjlj.c	17 Sep 2002 22:58:48 -0000	1.8.8.1
+++ unwind-sjlj.c	29 Oct 2002 22:20:12 -0000
@@ -92,15 +92,9 @@ static __gthread_key_t fc_key;
 static int use_fc_key = -1;
 
 static void
-fc_key_dtor (void *ptr)
-{
-  __gthread_key_dtor (fc_key, ptr);
-}
-
-static void
 fc_key_init (void)
 {
-  use_fc_key = __gthread_key_create (&fc_key, fc_key_dtor) == 0;
+  use_fc_key = __gthread_key_create (&fc_key, 0) == 0;
 }
 
 static void

Re: [basic-improvements] Remove __gthread_key_dtor

[Mark Mitchell]

--On Tuesday, October 29, 2002 02:33:46 PM -0800 Zack Weinberg 
<zack@codesourcery.com> wrote:

> The __gthread_key_dtor routine does nothing useful in any gthr-*.h
> implementation.  This patch removes it, which in turn allows some
> minor simplifications in unwind-sjlj.c.
>
> Tested by building an i386-linux -> i386-aout cross compiler,
> which includes a compilation of unwind-sjlj.c.
>

OK.  Thanks,

-- 
Mark Mitchell                mark@codesourcery.com
CodeSourcery, LLC            http://www.codesourcery.com

Re: [basic-improvements] Remove __gthread_key_dtor

[Richard Henderson]

On Tue, Oct 29, 2002 at 02:33:46PM -0800, Zack Weinberg wrote:
> The __gthread_key_dtor routine does nothing useful in any gthr-*.h
> implementation.  This patch removes it, which in turn allows some
> minor simplifications in unwind-sjlj.c.

Um, well, it *should* do something on the playstation version.
I guess that's sitting on a branch somewhere waiting on echristo
to submit it?


r~

Re: [basic-improvements] Remove __gthread_key_dtor

[Zack Weinberg]

On Thu, Oct 31, 2002 at 11:18:28AM -0800, Richard Henderson wrote:
> On Tue, Oct 29, 2002 at 02:33:46PM -0800, Zack Weinberg wrote:
> > The __gthread_key_dtor routine does nothing useful in any gthr-*.h
> > implementation.  This patch removes it, which in turn allows some
> > minor simplifications in unwind-sjlj.c.
> 
> Um, well, it *should* do something on the playstation version.
> I guess that's sitting on a branch somewhere waiting on echristo
> to submit it?

I have never seen the code you're referring to.  Can you give a brief
rundown on what needs to be done?

zw

Re: [basic-improvements] Remove __gthread_key_dtor

[Jason R Thorpe]

On Thu, Oct 31, 2002 at 11:18:28AM -0800, Richard Henderson wrote:

 > Um, well, it *should* do something on the playstation version.
 > I guess that's sitting on a branch somewhere waiting on echristo
 > to submit it?

Yes, echristo should submit it .. that way I can stop bugging him
about it :-)

(...and that way I can stop using an ancient compiler for NetBSD on
the playstation2)

-- 
        -- Jason R. Thorpe <thorpej@wasabisystems.com>

Re: [basic-improvements] Remove __gthread_key_dtor

[Richard Henderson]

On Thu, Oct 31, 2002 at 11:42:29AM -0800, Zack Weinberg wrote:
> I have never seen the code you're referring to.  Can you give a brief
> rundown on what needs to be done?

The tx5900 is one of the mips parts that doesn't have ll/sc,
or indeed any atomic operations at all, so you have to have
help from the OS.

Sony has a library through which you can create, lock and 
unlock mutex objects.  If you don't destroy one of these,
you leak it.

Eric has more details if you need them.


r~

Re: [basic-improvements] Remove __gthread_key_dtor

[Zack Weinberg]

On Thu, Oct 31, 2002 at 01:02:29PM -0800, Richard Henderson wrote:
> 
> The tx5900 is one of the mips parts that doesn't have ll/sc,
> or indeed any atomic operations at all, so you have to have
> help from the OS.
> 
> Sony has a library through which you can create, lock and 
> unlock mutex objects.  If you don't destroy one of these,
> you leak it.
> 
> Eric has more details if you need them.

Okay, but what I don't get is why this translates to a requirement
that every registered TSD destructor hook call back into gthr.h.
In fact, I don't see what TSD has to do with mutexes at all.

zw

Re: [basic-improvements] Remove __gthread_key_dtor

[Eric Christopher]

Zack,

> Okay, but what I don't get is why this translates to a requirement
> that every registered TSD destructor hook call back into gthr.h.
> In fact, I don't see what TSD has to do with mutexes at all.
> 

re mutexes: Sony only has mutexes and not a real thread system at all.
So, we get to "invent" things via locking and mutexes.

Now, I may have a problem with my implementation based on rth's email,
but here's the dtor function as it currently exists:

static inline int
__gthread_key_dtor (__gthread_key_t key, void *ptr)
{
  /* Just reset the key value to zero.  */
  if (ptr)
    return __gthread_setspecific (key, 0);
  else
    return 0;
}

I'll attach and old version of the file since it's currently undergoing
development. The mutex init function is incorrect, but that's mostly
because I couldn't find a good listing on the proper options to have :)
(There are only 253 mutexes available and we're trying to figure out
exactly how many can be used by gcc to decide whether or not we can just
lock a mutex or have to use a single mutex and spin on it... - advice
here would be wonderful.

-eric


-- 
Yeah, I used to play basketball...


/* Threads compatibility routines for libgcc2 for the Emotion Engine.  */
/* Compile this one with gcc.  */
/* Copyright (C) 2002 Free Software Foundation, Inc.
   Contributed by Sony Computer Entertainment Inc. and
		  Eric Christopher  <echristo@redhat.com>.

This file is part of GCC.

GCC is free software; you can redistribute it and/or modify it under
the terms of the GNU General Public License as published by the Free
Software Foundation; either version 2, or (at your option) any later
version.

GCC is distributed in the hope that it will be useful, but WITHOUT ANY
WARRANTY; without even the implied warranty of MERCHANTABILITY or
FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
for more details.

You should have received a copy of the GNU General Public License
along with GCC; see the file COPYING.  If not, write to the Free
Software Foundation, 59 Temple Place - Suite 330, Boston, MA
02111-1307, USA.  */

/* As a special exception, if you link this library with other files,
   some of which are compiled with GCC, to produce an executable,
   this library does not by itself cause the resulting executable
   to be covered by the GNU General Public License.
   This exception does not however invalidate any other reasons why
   the executable file might be covered by the GNU General Public License.  */

#ifndef GCC_GTHR_EEKERNEL_H
#define GCC_GTHR_EEKERNEL_H

/* EE kernel threads specific definitions. */

#define __GTHREADS 1

typedef int __gthread_key_t;
typedef int __gthread_once_t;
typedef int __gthread_mutex_t;
typedef struct SemaParam
  {
    int currentCount;
    int maxCount;
    int initCount;
    int numWaitThreads;
    unsigned int attr;
    unsigned int option;
  }SemaParam;

#define __GTHREAD_ONCE_INIT 0
#define __GTHREAD_MUTEX_INIT 0

#define __GTHREAD_MUTEX_INIT_FUNCTION __gthread_mutex_init_function

#define THR_ID_NUM 256
#define KEY_NUM 8
static void *key_value[THR_ID_NUM][KEY_NUM] = { { 0 }, { 0 } };
static int   key_use[KEY_NUM] = { 0 };

/* Mutex for locking key bitmap.  */
static __gthread_mutex_t key_mutex;

#ifdef __cplusplus
extern "C"
{
#endif
extern int GetThreadId(void);
extern int WaitSema(int);
extern int PollSema(int);
extern int SignalSema(int);
extern int CreateSema(struct SemaParam *);
#ifdef __cplusplus
}
#endif

extern int __sce_eh_sema_id;

static inline int
__gthread_active_p ()
{
  return 1;
}

static inline int
__gthread_once (__gthread_once_t *once, void (*func) ())
{
  return 0;
}

static inline void
__gthread_mutex_init_function (__gthread_mutex_t *mutex)
{
  SemaParam defaultSema;

  defaultSema.currentCount = 0;
  defaultSema.initCount = 0;
  defaultSema.attr = 0;
  *mutex = CreateSema (&defaultSema);
}

static inline int
__gthread_mutex_lock (__gthread_mutex_t *mutex)
{
  if (WaitSema(*mutex) == -1)
    return -1;
  else
    return 0;
}

static inline int
__gthread_mutex_trylock (__gthread_mutex_t *mutex)
{
  if (PollSema(*mutex) == -1)
    return -1;

  return 0;
}

static inline int
__gthread_mutex_unlock (__gthread_mutex_t *mutex)
{
  if (SignalSema(*mutex) == -1)
    return -1;

  return 0;
}

static inline int
__gthread_key_create (__gthread_key_t *keyp, void (*dtor) (void *))
{
  int i;

  __gthread_mutex_lock (&key_mutex);
  for (i = 0; i < KEY_NUM; i++)
    {
      if (!key_use[i] && dtor != 0)
        {
          key_use[i] = 1;
          __gthread_mutex_unlock (&key_mutex);
          *keyp = i;
          return 0;
        }
    }
  __gthread_mutex_unlock (&key_mutex);
  return -1;
}

static inline int
__gthread_key_delete (__gthread_key_t key)
{
  int id;

  __gthread_mutex_lock (&key_mutex);

  /* Make sure we don't have a bogus key number.  */
  if (key > KEY_NUM)
    {
      __gthread_mutex_unlock (&key_mutex);
      return -1;
    }

  if (key_use[key] != 0)
    key_use[key] = 0;

  /* Set the value of the key to 0 in all threads,
     so that if a key is later used it's value is NULL.  */
  for (id = 0; id < THR_ID_NUM; id++)
    key_value[id][key] = 0;

  __gthread_mutex_unlock (&key_mutex);
  return 0;
}

static inline void *
__gthread_getspecific (__gthread_key_t key)
{
  if (key > KEY_NUM || !key_use[key])
    return 0;

  return key_value[GetThreadId ()][key];
}

static inline int
__gthread_setspecific (__gthread_key_t key, const void *ptr)
{
  if (key > KEY_NUM || !key_use[key])
    return -1;

  key_value[GetThreadId ()][key] = (void *)ptr;
  return 0;
}

static inline int
__gthread_key_dtor (__gthread_key_t key, void *ptr)
{
  /* Just reset the key value to zero.  */
  if (ptr)
    return __gthread_setspecific (key, 0);
  else
    return 0;
}

#endif /* not GCC_GTHR_EEKERNEL_H */

Re: [basic-improvements] Remove __gthread_key_dtor

[Eric Christopher]

On Thu, 2002-10-31 at 12:04, Jason R Thorpe wrote:
> On Thu, Oct 31, 2002 at 11:18:28AM -0800, Richard Henderson wrote:
> 
>  > Um, well, it *should* do something on the playstation version.
>  > I guess that's sitting on a branch somewhere waiting on echristo
>  > to submit it?
> 
> Yes, echristo should submit it .. that way I can stop bugging him
> about it :-)
> 

Yes yes... it's on TheList (tm).

(actually it's really the last mips based toolchain I need to submit - I
just need time to bring it forward in time a bit - it's a little old
right now)

-eric

-- 
Yeah, I used to play basketball...

Re: [basic-improvements] Remove __gthread_key_dtor

[Zack Weinberg]

On Thu, Oct 31, 2002 at 02:31:21PM -0800, Eric Christopher wrote:
> 
> re mutexes: Sony only has mutexes and not a real thread system at all.
> So, we get to "invent" things via locking and mutexes.

Understood.  In fact, looking over your code, I just wrote some very
similar stuff for VxWorks (which has threads, but neither once_t nor
thread-specific data as POSIX defines it).

> Now, I may have a problem with my implementation based on rth's email,
> but here's the dtor function as it currently exists:
> 
> static inline int
> __gthread_key_dtor (__gthread_key_t key, void *ptr)
> {
>   /* Just reset the key value to zero.  */
>   if (ptr)
>     return __gthread_setspecific (key, 0);
>   else
>     return 0;
> }

This implementation is identical with several of the ones I calmly
wiped out of existence as "does nothing useful".  The thread
destruction logic is (should be) responsible for doing this.

I see a number of problems with your fake TSD implementation, but
nothing that requires you to have a __gthread_key_dtor routine.  See
below.

> static void *key_value[THR_ID_NUM][KEY_NUM] = { { 0 }, { 0 } };
> static int   key_use[KEY_NUM] = { 0 };
> 
> /* Mutex for locking key bitmap.  */
> static __gthread_mutex_t key_mutex;

There needs to be just one version of these variables, but gthr.h is
included all over the place - this needs to go out-of-line, into a
libgcc.a extra part, probably along with the code that uses it.

I take it there can be only 256 threads?

> static inline int
> __gthread_once (__gthread_once_t *once, void (*func) ())
> {
>   return 0;
> }

You're going to want a real version of this function.  It's used in
the unwinder, but also...

> static inline int
> __gthread_key_create (__gthread_key_t *keyp, void (*dtor) (void *))
> {
>   int i;
> 
>   __gthread_mutex_lock (&key_mutex);
>   for (i = 0; i < KEY_NUM; i++)
>     {
>       if (!key_use[i] && dtor != 0)
>         {
>           key_use[i] = 1;
>           __gthread_mutex_unlock (&key_mutex);
>           *keyp = i;
>           return 0;
>         }
>     }
>   __gthread_mutex_unlock (&key_mutex);
>   return -1;
> }

where does key_mutex get initialized?  I have the same requirement on
mutexes in VxWorks (must use an initialization function) and wound up
using __gthread_once to call __gthread_mutex_init_function on the
mutex used to protect the keys table.

There is no requirement that dtor be nonzero, and you threw away its
value; this will cause memory leaks.  You're going to need to find
some way to get a callback run at thread exit time.  And that callback
can do the clear-out-the-value thing.

> static inline int
> __gthread_setspecific (__gthread_key_t key, const void *ptr)
> {
>   if (key > KEY_NUM || !key_use[key])
>     return -1;
> 
>   key_value[GetThreadId ()][key] = (void *)ptr;
>   return 0;
> }

This races with key_delete.  You'll have to have it take the lock, too.

zw

Re: [basic-improvements] Remove __gthread_key_dtor

[Eric Christopher]

Zack,

> 
> > static void *key_value[THR_ID_NUM][KEY_NUM] = { { 0 }, { 0 } };
> > static int   key_use[KEY_NUM] = { 0 };
> > 
> > /* Mutex for locking key bitmap.  */
> > static __gthread_mutex_t key_mutex;
> 
> There needs to be just one version of these variables, but gthr.h is
> included all over the place - this needs to go out-of-line, into a
> libgcc.a extra part, probably along with the code that uses it.
> 

Hrm. I was noticing that other ports use this method and that's why I
did - I also didn't see a way for it to work otherwise since there's no
general .c mechanism for it.

> I take it there can be only 256 threads?
> 

Heh. Threads? No. I've got semaphores and only semaphores.

> > static inline int
> > __gthread_once (__gthread_once_t *once, void (*func) ())
> > {
> >   return 0;
> > }
> 
> You're going to want a real version of this function.  It's used in
> the unwinder, but also...
> 

OK.

> > static inline int
> > __gthread_key_create (__gthread_key_t *keyp, void (*dtor) (void *))
> > {
> >   int i;
> > 
> >   __gthread_mutex_lock (&key_mutex);
> >   for (i = 0; i < KEY_NUM; i++)
> >     {
> >       if (!key_use[i] && dtor != 0)
> >         {
> >           key_use[i] = 1;
> >           __gthread_mutex_unlock (&key_mutex);
> >           *keyp = i;
> >           return 0;
> >         }
> >     }
> >   __gthread_mutex_unlock (&key_mutex);
> >   return -1;
> > }
> 
> where does key_mutex get initialized?  I have the same requirement on
> mutexes in VxWorks (must use an initialization function) and wound up
> using __gthread_once to call __gthread_mutex_init_function on the
> mutex used to protect the keys table.
> 

oops. thanks :)

> There is no requirement that dtor be nonzero, and you threw away its
> value; this will cause memory leaks.  You're going to need to find
> some way to get a callback run at thread exit time.  And that callback
> can do the clear-out-the-value thing.
> 

hrm. If I could do that I could also free semaphores which would help me
avoid the 253 limit that I currently have. I was under the impression
that this was not possible though - at least not without rewriting the
machinery. 

> > static inline int
> > __gthread_setspecific (__gthread_key_t key, const void *ptr)
> > {
> >   if (key > KEY_NUM || !key_use[key])
> >     return -1;
> > 
> >   key_value[GetThreadId ()][key] = (void *)ptr;
> >   return 0;
> > }
> 
> This races with key_delete.  You'll have to have it take the lock, too.
> 

Thanks.

-eric

-- 
Yeah, I used to play basketball...

Re: [basic-improvements] Remove __gthread_key_dtor

[Zack Weinberg]

On Mon, Nov 04, 2002 at 12:41:08PM -0800, Eric Christopher wrote:
> > 
> > There needs to be just one version of these variables, but gthr.h is
> > included all over the place - this needs to go out-of-line, into a
> > libgcc.a extra part, probably along with the code that uses it.
> 
> Hrm. I was noticing that other ports use this method and that's why I
> did - I also didn't see a way for it to work otherwise since there's no
> general .c mechanism for it.

You create a .c file in config/ARCH, and mention it in the LIB2ADD
variable in your t-TARGET file.

> > I take it there can be only 256 threads?
> 
> Heh. Threads? No. I've got semaphores and only semaphores.

Er, what's this GetThreadId() then?

> > There is no requirement that dtor be nonzero, and you threw away its
> > value; this will cause memory leaks.  You're going to need to find
> > some way to get a callback run at thread exit time.  And that callback
> > can do the clear-out-the-value thing.
> 
> hrm. If I could do that I could also free semaphores which would help me
> avoid the 253 limit that I currently have. I was under the impression
> that this was not possible though - at least not without rewriting the
> machinery. 

If the underlying API doesn't support this, you're out of luck.  But
if there _is_ a way to get a callback run at thread exit time, you can
just set one up in your init-once routine.

Take a look at gthr-vxworks.h, config/vxlib.c, and config/t-vxworks on
the gcc-3_4-basic-improvements-branch (after I commit them, which will
happen in the next twenty minutes or so) -- they should give you some
ideas.

zw

Re: [basic-improvements] Remove __gthread_key_dtor

[Eric Christopher]

> > Hrm. I was noticing that other ports use this method and that's why I
> > did - I also didn't see a way for it to work otherwise since there's no
> > general .c mechanism for it.
> 

Right. Wasn't thinking clearly. :)


> If the underlying API doesn't support this, you're out of luck.  But
> if there _is_ a way to get a callback run at thread exit time, you can
> just set one up in your init-once routine.
> 
> Take a look at gthr-vxworks.h, config/vxlib.c, and config/t-vxworks on
> the gcc-3_4-basic-improvements-branch (after I commit them, which will
> happen in the next twenty minutes or so) -- they should give you some
> ideas.
> 

Cool. Will do.

Thanks.

-eric

-- 
Yeah, I used to play basketball...