Re: __libc_lock_lock_recursive et al

[Roland McGrath <roland at frob dot com>]

It certainly works fine for linuxthreads, and I didn't mean to suggest
otherwise.  The problem is in the use of the libc-lock.h source interface,
which exists to be a portability layer for a potential variety of
implementations.  As I said, the comment is correct about the need for a
counting mutex (for the reasons the comment says); I have no argument with
that.  In linuxthreads, normal mutexes and counting mutexes have the same
data type, so some sloppy uses of the source interfaces can result in
perfectly correct compiled code.  Initially I was afraid that actually all
mutexes were counting mutexes in linuxthreads and therefore it would be
hard to figure out which uses ought to be using which interface.  Now that
I've had time to look at the code in detail, I see that there is a
different initializer and so it's easy to find the (very few) uses and fix
them.

I'd like to check in the following changes.  This adds a struct wrapper
around the recursive mutex type in linuxthreads so that the compiler will
flag type errors or warnings if the *_recursive macros are used on a lock
declared non-recursive, or vice versa.  This should have no effect at all
on the compiled code.  After doing that it was easy to find and fix the few
uses of __libc_lock_lock that should be __libc_lock_lock_recursive et al.


Ok?



2001-08-22  Roland McGrath  <roland@frob.com>

	* sysdeps/generic/ldsodefs.h (_dl_load_lock): Declare it here with
	__libc_lock_define_recursive.
	* elf/dl-open.c: Don't declare it here any more.
	* elf/dl-close.c: Likewise.
	* elf/dl-lookup.c: Likewise.
	* elf/dl-iteratephdr.c: Likewise.
	* elf/dl-lookup.c (add_dependency): Use __libc_lock_lock_recursive and
	__libc_lock_unlock_recursive.
	* elf/dl-close.c (_dl_close): Likewise
	* elf/dl-iteratephdr.c (__dl_iterate_phdr): Likewise
	* elf/dl-open.c (_dl_open): Likewise

	* sysdeps/generic/bits/libc-lock.h
	(__libc_lock_define_recursive): New macro.
	* sysdeps/generic/bits/stdio-lock.h (_IO_lock_t): Use it.
	(_IO_lock_lock): Use __libc_lock_lock_recursive.
	(_IO_lock_unlock): Use __libc_lock_unlock_recursive.


linuxthreads:

2001-08-22  Roland McGrath  <roland@frob.com>

	* sysdeps/pthread/bits/stdio-lock.h: Include <bits/libc-lock.h>
	instead of <pthread.h>.
	(_IO_lock_t): Define this typedef using __libc_lock_define_recursive.
	(_IO_lock_initializer): Add braces.
	(_IO_lock_lock): Use __libc_lock_lock_recursive.
	(_IO_lock_unlock): Use __libc_lock_unlock_recursive.

	* sysdeps/pthread/bits/libc-lock.h (__libc_lock_recursive_t): New type.
	(__libc_lock_define_initialized_recursive): Use it.
	(__libc_lock_init_recursive): Likewise.
	(__libc_lock_fini_recursive): Likewise.
	(__libc_lock_lock_recursive): Likewise.
	(__libc_lock_trylock_recursive): Likewise.
	(__libc_lock_unlock_recursive): Likewise.
	(__libc_lock_define_recursive): New macro.


Index: elf/dl-close.c
===================================================================
RCS file: /cvs/glibc/libc/elf/dl-close.c,v
retrieving revision 1.68
diff -u -r1.68 dl-close.c
--- dl-close.c	2001/08/04 14:25:29	1.68
+++ dl-close.c	2001/08/23 01:00:17
@@ -32,11 +32,6 @@
 typedef void (*fini_t) (void);
 
 
-/* During the program run we must not modify the global data of
-   loaded shared object simultanously in two threads.  Therefore we
-   protect `dlopen' and `dlclose' in dlclose.c.  */
-__libc_lock_define (extern, _dl_load_lock)
-
 void
 internal_function
 _dl_close (void *_map)
@@ -61,7 +56,7 @@
     _dl_signal_error (0, map->l_name, N_("shared object not open"));
 
   /* Acquire the lock.  */
-  __libc_lock_lock (_dl_load_lock);
+  __libc_lock_lock_recursive (_dl_load_lock);
 
   /* Decrement the reference count.  */
   if (map->l_opencount > 1 || map->l_type != lt_loaded)
@@ -80,7 +75,7 @@
       /* One decrement the object itself, not the dependencies.  */
       --map->l_opencount;
 
-      __libc_lock_unlock (_dl_load_lock);
+      __libc_lock_unlock_recursive (_dl_load_lock);
       return;
     }
 
@@ -277,7 +272,7 @@
   free (list);
 
   /* Release the lock.  */
-  __libc_lock_unlock (_dl_load_lock);
+  __libc_lock_unlock_recursive (_dl_load_lock);
 }
 
 
Index: elf/dl-iteratephdr.c
===================================================================
RCS file: /cvs/glibc/libc/elf/dl-iteratephdr.c,v
retrieving revision 1.1
diff -u -r1.1 dl-iteratephdr.c
--- dl-iteratephdr.c	2001/07/25 20:42:47	1.1
+++ dl-iteratephdr.c	2001/08/23 01:00:17
@@ -23,8 +23,6 @@
 #include <stddef.h>
 #include <bits/libc-lock.h>
 
-__libc_lock_define (extern, _dl_load_lock)
-
 int
 __dl_iterate_phdr (int (*callback) (struct dl_phdr_info *info,
 				    size_t size, void *data), void *data)
@@ -34,7 +32,7 @@
   int ret = 0;
 
   /* Make sure we are alone.  */
-  __libc_lock_lock (_dl_load_lock);
+  __libc_lock_lock_recursive (_dl_load_lock);
 
   for (l = _dl_loaded; l != NULL; l = l->l_next)
     {
@@ -51,7 +49,7 @@
     }
 
   /* Release the lock.  */
-  __libc_lock_unlock (_dl_load_lock);
+  __libc_lock_unlock_recursive (_dl_load_lock);
 
   return ret;
 }
Index: elf/dl-lookup.c
===================================================================
RCS file: /cvs/glibc/libc/elf/dl-lookup.c,v
retrieving revision 1.78
diff -u -r1.78 dl-lookup.c
--- dl-lookup.c	2001/07/06 04:54:46	1.78
+++ dl-lookup.c	2001/08/23 01:00:18
@@ -61,16 +61,7 @@
 /* Statistics function.  */
 unsigned long int _dl_num_relocations;
 
-/* During the program run we must not modify the global data of
-   loaded shared object simultanously in two threads.  Therefore we
-   protect `_dl_open' and `_dl_close' in dl-close.c.
-
-   This must be a recursive lock since the initializer function of
-   the loaded object might as well require a call to this function.
-   At this time it is not anymore a problem to modify the tables.  */
-__libc_lock_define (extern, _dl_load_lock)
 
-
 /* We have two different situations when looking up a simple: with or
    without versioning.  gcc is not able to optimize a single function
    definition serving for both purposes so we define two functions.  */
@@ -92,7 +83,7 @@
   int result = 0;
 
   /* Make sure nobody can unload the object while we are at it.  */
-  __libc_lock_lock (_dl_load_lock);
+  __libc_lock_lock_recursive (_dl_load_lock);
 
   /* Determine whether UNDEF_MAP already has a reference to MAP.  First
      look in the normal dependencies.  */
@@ -174,7 +165,7 @@
     }
 
   /* Release the lock.  */
-  __libc_lock_unlock (_dl_load_lock);
+  __libc_lock_unlock_recursive (_dl_load_lock);
 
   return result;
 }
Index: elf/dl-open.c
===================================================================
RCS file: /cvs/glibc/libc/elf/dl-open.c,v
retrieving revision 1.75
diff -u -r1.75 dl-open.c
--- dl-open.c	2001/08/11 02:39:07	1.75
+++ dl-open.c	2001/08/23 01:00:18
@@ -58,15 +58,6 @@
 static void show_scope (struct link_map *new);
 #endif
 
-/* During the program run we must not modify the global data of
-   loaded shared object simultanously in two threads.  Therefore we
-   protect `_dl_open' and `_dl_close' in dl-close.c.
-
-   This must be a recursive lock since the initializer function of
-   the loaded object might as well require a call to this function.
-   At this time it is not anymore a problem to modify the tables.  */
-__libc_lock_define (extern, _dl_load_lock)
-
 extern size_t _dl_platformlen;
 
 /* We must be carefull not to leave us in an inconsistent state.  Thus we
@@ -349,7 +340,7 @@
     _dl_signal_error (EINVAL, file, N_("invalid mode for dlopen()"));
 
   /* Make sure we are alone.  */
-  __libc_lock_lock (_dl_load_lock);
+  __libc_lock_lock_recursive (_dl_load_lock);
 
   args.file = file;
   args.mode = mode;
@@ -363,7 +354,7 @@
 #endif
 
   /* Release the lock.  */
-  __libc_lock_unlock (_dl_load_lock);
+  __libc_lock_unlock_recursive (_dl_load_lock);
 
   if (errstring)
     {
Index: linuxthreads/sysdeps/pthread/bits/libc-lock.h
===================================================================
RCS file: /cvs/glibc/libc/linuxthreads/sysdeps/pthread/bits/libc-lock.h,v
retrieving revision 1.17
diff -u -r1.17 libc-lock.h
--- libc-lock.h	2001/01/28 16:39:07	1.17
+++ libc-lock.h	2001/08/23 01:00:18
@@ -26,8 +26,10 @@
 #ifdef _LIBC
 typedef pthread_mutex_t __libc_lock_t;
 typedef pthread_rwlock_t __libc_rwlock_t;
+typedef struct { pthread_mutex_t mutex; } __libc_lock_recursive_t;
 #else
 typedef struct __libc_lock_opaque__ __libc_lock_t;
+typedef struct __libc_lock_recursive_opaque__ __libc_lock_recursive_t;
 typedef struct __libc_rwlock_opaque__ __libc_rwlock_t;
 #endif
 
@@ -45,6 +47,8 @@
   CLASS __libc_lock_t NAME;
 #define __libc_rwlock_define(CLASS,NAME) \
   CLASS __libc_rwlock_t NAME;
+#define __libc_lock_define_recursive(CLASS,NAME) \
+  CLASS __libc_lock_recursive_t NAME;
 
 /* Define an initialized lock variable NAME with storage class CLASS.
 
@@ -68,8 +72,8 @@
 
 /* Define an initialized recursive lock variable NAME with storage
    class CLASS.  */
-#define __libc_lock_define_initialized_recursive(CLASS,NAME) \
-  CLASS __libc_lock_t NAME = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
+#define __libc_lock_define_initialized_recursive(CLS,NAME) \
+  CLS __libc_lock_recursive_t NAME = {PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP};
 
 /* Initialize the named lock variable, leaving it in a consistent, unlocked
    state.  */
@@ -86,7 +90,7 @@
 	pthread_mutexattr_t __attr;					      \
 	__pthread_mutexattr_init (&__attr);				      \
 	__pthread_mutexattr_settype (&__attr, PTHREAD_MUTEX_RECURSIVE_NP); \
-	__pthread_mutex_init (&(NAME), &__attr);			      \
+	__pthread_mutex_init (&(NAME).mutex, &__attr);			      \
 	__pthread_mutexattr_destroy (&__attr);				      \
       }									      \
   } while (0);
@@ -100,7 +104,7 @@
   (__pthread_rwlock_destroy != NULL ? __pthread_rwlock_destroy (&(NAME)) : 0);
 
 /* Finalize recursive named lock.  */
-#define __libc_lock_fini_recursive(NAME) __libc_lock_fini (NAME)
+#define __libc_lock_fini_recursive(NAME) __libc_lock_fini ((NAME).mutex)
 
 /* Lock the named lock variable.  */
 #define __libc_lock_lock(NAME) \
@@ -111,7 +115,7 @@
   (__pthread_rwlock_wrlock != NULL ? __pthread_rwlock_wrlock (&(NAME)) : 0);
 
 /* Lock the recursive named lock variable.  */
-#define __libc_lock_lock_recursive(NAME) __libc_lock_lock (NAME)
+#define __libc_lock_lock_recursive(NAME) __libc_lock_lock ((NAME).mutex)
 
 /* Try to lock the named lock variable.  */
 #define __libc_lock_trylock(NAME) \
@@ -124,7 +128,7 @@
    ? __pthread_rwlock_trywrlock (&(NAME)) : 0)
 
 /* Try to lock the recursive named lock variable.  */
-#define __libc_lock_trylock_recursive(NAME) __libc_lock_trylock (NAME)
+#define __libc_lock_trylock_recursive(NAME) __libc_lock_trylock ((NAME).mutex)
 
 /* Unlock the named lock variable.  */
 #define __libc_lock_unlock(NAME) \
@@ -133,7 +137,7 @@
   (__pthread_rwlock_unlock != NULL ? __pthread_rwlock_unlock (&(NAME)) : 0);
 
 /* Unlock the recursive named lock variable.  */
-#define __libc_lock_unlock_recursive(NAME) __libc_lock_unlock (NAME)
+#define __libc_lock_unlock_recursive(NAME) __libc_lock_unlock ((NAME).mutex)
 
 
 /* Define once control variable.  */
Index: linuxthreads/sysdeps/pthread/bits/stdio-lock.h
===================================================================
RCS file: /cvs/glibc/libc/linuxthreads/sysdeps/pthread/bits/stdio-lock.h,v
retrieving revision 1.5
diff -u -r1.5 stdio-lock.h
--- stdio-lock.h	2001/07/24 01:06:59	1.5
+++ stdio-lock.h	2001/08/23 01:00:18
@@ -17,14 +17,19 @@
    write to the Free Software Foundation, Inc., 59 Temple Place - Suite 330,
    Boston, MA 02111-1307, USA.  */
 
-#include <pthread.h>
+#include <bits/libc-lock.h>
 
-typedef pthread_mutex_t _IO_lock_t;
+__libc_lock_define_recursive (typedef, _IO_lock_t)
 
 /* We need recursive (counting) mutexes.  */
-#define _IO_lock_initializer PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP
+#define _IO_lock_initializer	{PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP}
 
+#define _IO_lock_init(_name)	__libc_lock_init_recursive (_name)
+#define _IO_lock_fini(_name)	__libc_lock_fini_recursive (_name)
+#define _IO_lock_lock(_name)	__libc_lock_lock_recursive (_name)
+#define _IO_lock_unlock(_name)	__libc_lock_unlock_recursive (_name)
 
+
 #define _IO_cleanup_region_start(_fct, _fp) \
   { struct _pthread_cleanup_buffer _buffer;				      \
     int _avail = (((_fp)->_flags & _IO_USER_LOCK) == 0			      \
@@ -40,11 +45,3 @@
     }
 #define _IO_cleanup_region_end(_doit) \
      __libc_cleanup_region_end (_doit)
-#define _IO_lock_init(_name) \
-     __libc_lock_init_recursive (_name)
-#define _IO_lock_fini(_name) \
-     __libc_lock_fini_recursive (_name)
-#define _IO_lock_lock(_name) \
-     __libc_lock_lock (_name)
-#define _IO_lock_unlock(_name) \
-     __libc_lock_unlock (_name)
Index: sysdeps/generic/ldsodefs.h
===================================================================
RCS file: /cvs/glibc/libc/sysdeps/generic/ldsodefs.h,v
retrieving revision 1.27
diff -u -r1.27 ldsodefs.h
--- ldsodefs.h	2001/07/06 04:55:49	1.27
+++ ldsodefs.h	2001/08/23 01:00:19
@@ -31,6 +31,7 @@
 #include <dlfcn.h>
 #include <link.h>
 #include <dl-lookupcfg.h>
+#include <bits/libc-lock.h>
 
 __BEGIN_DECLS
 
@@ -227,6 +228,16 @@
 
 /* OS-dependent function to open the zero-fill device.  */
 extern int _dl_sysdep_open_zero_fill (void); /* dl-sysdep.c */
+
+
+/* During the program run we must not modify the global data of
+   loaded shared object simultanously in two threads.  Therefore we
+   protect `_dl_open' and `_dl_close' in dl-close.c.
+
+   This must be a recursive lock since the initializer function of
+   the loaded object might as well require a call to this function.
+   At this time it is not anymore a problem to modify the tables.  */
+__libc_lock_define_recursive (extern, _dl_load_lock)
 
 
 /* Write message on the debug file descriptor.  The parameters are
Index: sysdeps/generic/bits/libc-lock.h
===================================================================
RCS file: /cvs/glibc/libc/sysdeps/generic/bits/libc-lock.h,v
retrieving revision 1.4
diff -u -r1.4 libc-lock.h
--- libc-lock.h	2001/07/06 04:55:50	1.4
+++ libc-lock.h	2001/08/23 01:00:19
@@ -1,5 +1,5 @@
 /* libc-internal interface for mutex locks.  Stub version.
-   Copyright (C) 1996, 1997, 1999, 2000 Free Software Foundation, Inc.
+   Copyright (C) 1996,97,99,2000,01 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
    The GNU C Library is free software; you can redistribute it and/or
@@ -29,6 +29,7 @@
    begins with a `*'), because its storage size will not be known outside
    of libc.  */
 #define __libc_lock_define(CLASS,NAME)
+#define __libc_lock_define_recursive(CLASS,NAME)
 #define __libc_rwlock_define(CLASS,NAME)
 
 /* Define an initialized lock variable NAME with storage class CLASS.  */
Index: sysdeps/generic/bits/stdio-lock.h
===================================================================
RCS file: /cvs/glibc/libc/sysdeps/generic/bits/stdio-lock.h,v
retrieving revision 1.4
diff -u -r1.4 stdio-lock.h
--- stdio-lock.h	2001/07/24 01:07:00	1.4
+++ stdio-lock.h	2001/08/23 01:00:19
@@ -22,27 +22,24 @@
 
 #include <bits/libc-lock.h>
 
-__libc_lock_define (typedef, _IO_lock_t)
+__libc_lock_define_recursive (typedef, _IO_lock_t)
 
 /* We need recursive (counting) mutexes.  */
 #define _IO_lock_initializer ...
 #error libio needs recursive mutexes for _IO_MTSAFE_IO
 
+#define _IO_lock_init(_name)	__libc_lock_init_recursive (_name)
+#define _IO_lock_fini(_name)	__libc_lock_fini_recursive (_name)
+#define _IO_lock_lock(_name)	__libc_lock_lock_recursive (_name)
+#define _IO_lock_unlock(_name)	__libc_lock_unlock_recursive (_name)
 
+
 #define _IO_cleanup_region_start(_fct, _fp) \
      __libc_cleanup_region_start (_fct, _fp)
 #define _IO_cleanup_region_start_noarg(_fct) \
      __libc_cleanup_region_start (_fct, NULL)
 #define _IO_cleanup_region_end(_doit) \
      __libc_cleanup_region_end (_doit)
-#define _IO_lock_init(_name) \
-     __libc_lock_init_recursive (_name)
-#define _IO_lock_fini(_name) \
-     __libc_lock_fini_recursive (_name)
-#define _IO_lock_lock(_name) \
-     __libc_lock_lock (_name)
-#define _IO_lock_unlock(_name) \
-     __libc_lock_unlock (_name)
 
 
 #endif /* bits/stdio-lock.h */