Re: pthread headers

[Neal H. Walfield]

> Maybe you want to consider this patch.

This patch is mostly incorrect.


> diff -ur hurd.old/libpthread/include/pthread/pthread.h 
> hurd/libpthread/include/pthread/pthread.h
> --- hurd.old/libpthread/include/pthread/pthread.h     2002-11-10 
> 18:00:40.000000000 +0100
> +++ hurd/libpthread/include/pthread/pthread.h 2003-03-09 14:41:03.000000000 
> +0100
> @@ -420,31 +420,35 @@
>  
>  # ifdef __USE_EXTERN_INLINES
>  
> -extern inline int
> +# ifndef _EXTERN_INLINE
> +#  define _EXTERN_INLINE extern __inline
> +# endif
> +
> +_EXTERN_INLINE int
>  pthread_spin_destroy (pthread_spinlock_t *__lock)
>  {
>    return __pthread_spin_destroy (__lock);
>  }

This change is wrong.  This code is only included if
__USE_EXTERN_INLINE is defined, i.e. if the includer explicitly asks
for extern inline.  Constructs such as FOO_EXTERN_INLINE are useful
when you need a version of a function with extern inline and one
without.  This is not the case here.  pthread_spin_destory is declared
immediately above this code and if you look at
libpthread/pthread/pt-spin-inlines.c, you see that
pthread_spin_destory is a weak alias to __pthread_spin_destroy.  In
this header file, pthread_spin_destroy is an extern inline which skips
the weak linkage parameter.  If __USE_EXTERN_INLINES was defined and
_EXTERN_INLINE is set to the empty string, then two versions of
pthread_spin_destoy will be present and possibly cause conflicts.

In brief, let's not use commonly understood idioms in different
contexts.  In this code, we only need to s/inline/__inline/.

> diff -ur hurd.old/libpthread/sysdeps/generic/bits/mutex.h 
> hurd/libpthread/sysdeps/generic/bits/mutex.h
> --- hurd.old/libpthread/sysdeps/generic/bits/mutex.h  2002-10-11 
> 01:05:05.000000000 +0200
> +++ hurd/libpthread/sysdeps/generic/bits/mutex.h      2003-03-09 
> 14:37:34.000000000 +0100
> @@ -108,7 +108,7 @@
>    return _pthread_mutex_lock (__mutex);
>  }
>  
> -extern inline int
> +_EXTERN_INLINE int
>  __pthread_mutex_trylock (struct __pthread_mutex *__mutex)
>  {
>    extern int _pthread_mutex_trylock (struct __pthread_mutex *);
> @@ -120,13 +120,13 @@
>    return _pthread_mutex_trylock (__mutex);
>  }

Ditto here, consider, e.g. sysdeps/generic/pt-mutex-trylock.c.

> diff -ur hurd.old/libpthread/sysdeps/generic/bits/pthread.h 
> hurd/libpthread/sysdeps/generic/bits/pthread.h
> --- hurd.old/libpthread/sysdeps/generic/bits/pthread.h        2002-10-11 
> 01:05:05.000000000 +0200
> +++ hurd/libpthread/sysdeps/generic/bits/pthread.h    2003-03-09 
> 14:44:48.000000000 +0100
> @@ -22,12 +22,20 @@
>  
>  typedef int pthread_t;
>  
> +#ifdef __USE_EXTERN_INLINES
> +
> +# ifndef _EXTERN_INLINE
> +#  define _EXTERN_INLINE extern __inline
> +# endif
> +
>  /* Return true if __T1 and __T2 both name the same thread.  Otherwise,
>     false.  */
> -extern inline int
> +_EXTERN_INLINE int
>  pthread_equal (pthread_t __t1, pthread_t __t2)
>  {
>    return __t1 == __t2;
>  }

Again, we only need this implementation for the extern inline case
(cf sysdeps/generic/pt-equal.c).

>  
> +#endif /* __USE_EXTERN_INLINES  */
> +
>  #endif /* bits/pthread.h */
> diff -ur hurd.old/libpthread/sysdeps/i386/bits/spin-lock.h 
> hurd/libpthread/sysdeps/i386/bits/spin-lock.h
> --- hurd.old/libpthread/sysdeps/i386/bits/spin-lock.h 2002-10-11 
> 01:05:05.000000000 +0200
> +++ hurd/libpthread/sysdeps/i386/bits/spin-lock.h     2003-03-09 
> 14:49:55.000000000 +0100
> @@ -74,10 +74,10 @@
>    return __locked ? __EBUSY : 0;
>  }
>  
> -extern inline int __pthread_spin_lock (__pthread_spinlock_t *__lock);
> +__PT_SPIN_INLINE int __pthread_spin_lock (__pthread_spinlock_t *__lock);
>  extern int _pthread_spin_lock (__pthread_spinlock_t *__lock);
>  
> -extern inline int
> +__PT_SPIN_INLINE int
>  __pthread_spin_lock (__pthread_spinlock_t *__lock)
>  {
>    if (__pthread_spin_trylock (__lock))

Like the first two.

> diff -ur hurd.old/libpthread/sysdeps/l4/hurd/pt-sysdep.h 
> hurd/libpthread/sysdeps/l4/hurd/pt-sysdep.h
> --- hurd.old/libpthread/sysdeps/l4/hurd/pt-sysdep.h   2002-10-11 
> 01:05:05.000000000 +0200
> +++ hurd/libpthread/sysdeps/l4/hurd/pt-sysdep.h       2003-03-09 
> 14:48:11.000000000 +0100
> @@ -34,17 +34,25 @@
>    L4_ThreadId_t threadid; \
>    L4_Word_t my_errno;
>  
> -extern inline struct __pthread *
> +#ifdef __USE_EXTERN_INLINES
> +
> +# ifndef _EXTERN_INLINE
> +#  define _EXTERN_INLINE extern __inline
> +# endif
> +
> +_EXTERN_INLINE struct __pthread *
>  _pthread_self (void)
>  {
>    return (struct __pthread *) L4_MyUserDefinedHandle ();
>  }
>  
> -extern inline void
> +_EXTERN_INLINE void
>  __pthread_stack_dealloc (void *stackaddr, size_t stacksize)
>  {
>    /* XXX: can only implement this once we have a working memory manager.  */
>    return;
>  }
>  
> +#endif /* __USE_EXTERN_INLINES  */
> +
>  #endif /* pt-sysdep.h */

This is not a public header file so we don't need to worry about
compiling with -ansi.  However, there is a bug here: this needs to
have the always inline attribute added.

> diff -ur hurd.old/libpthread/sysdeps/mach/bits/spin-lock.h 
> hurd/libpthread/sysdeps/mach/bits/spin-lock.h
> --- hurd.old/libpthread/sysdeps/mach/bits/spin-lock.h 2002-10-11 
> 01:05:05.000000000 +0200
> +++ hurd/libpthread/sysdeps/mach/bits/spin-lock.h     2003-03-09 
> 14:50:51.000000000 +0100
> @@ -70,10 +70,10 @@
>    return __spin_try_lock (__lock) ? 0 : __EBUSY;
>  }
>  
> -extern inline int __pthread_spin_lock (__pthread_spinlock_t *__lock);
> +__PT_SPIN_INLINE int __pthread_spin_lock (__pthread_spinlock_t *__lock);
>  extern int _pthread_spin_lock (__pthread_spinlock_t *__lock);
>  
> -extern inline int
> +__PT_SPIN_INLINE int
>  __pthread_spin_lock (__pthread_spinlock_t *__lock)
>  {
>    if (__pthread_spin_trylock (__lock))

Like the first two hunks, we only need to s/inline/__inline/.

> diff -ur hurd.old/libpthread/sysdeps/mach/hurd/pt-sysdep.h 
> hurd/libpthread/sysdeps/mach/hurd/pt-sysdep.h
> --- hurd.old/libpthread/sysdeps/mach/hurd/pt-sysdep.h 2002-11-19 
> 18:00:53.000000000 +0100
> +++ hurd/libpthread/sysdeps/mach/hurd/pt-sysdep.h     2003-03-09 
> 14:54:49.000000000 +0100
> @@ -52,12 +52,21 @@
>            thread;                                                  \
>           })
>  
> -extern inline void
> +
> +#ifdef __USE_EXTERN_INLINES
> +                                                                             
>    
> +# ifndef _EXTERN_INLINE
> +#  define _EXTERN_INLINE extern __inline
> +# endif
> +
> +_EXTERN_INLINE void
>  __pthread_stack_dealloc (void *stackaddr, size_t stacksize)
>  {
>    __vm_deallocate (__mach_task_self (), (vm_offset_t) stackaddr, stacksize);
>  }
>  
> +#endif /* __USE_EXTERN_INLINES  */
> +
>  /* Change thread THREAD's program counter to PC if SET_PC is true and
>     its stack pointer to SP if SET_IP is true.  */
>  extern int __thread_set_pcsp (thread_t thread,

This has the same problem as sysdeps/l4/hurd/pt-sysdep.h: it is
internal and these are the only definitions, hence we need the always
inline attribute.

Thanks,
Neal