Re: enabling caching for dl_iterate_phdr()

Re: enabling caching for dl_iterate_phdr()

[Roland McGrath]

>   >> Would it be possible to add two atomic counters,
>   >> dl_load_additions and dl_load_removals, which would get
>   >> incremented whenever something is a phdr is added to/removed from
>   >> the list that is traversed by dl_iterate_phdr()?
> 
>   Roland> What interface do you propose for exposing such counters?
>   Roland> We won't be adding any published interfaces using variables.
> 
> I think that's OK.  Do you think it would be OK to add a function
> which returns the address of one of several "atomic counter" variables?

That is a reasonable kind of interface in general.  The trouble here is
that glibc does not have any exported interfaces using atomic counters.
The right type to use for these is machine-dependent; `int' is not the
proper thing to be using in the interface.  The types used in atomic.h are
fine, but that is not an exported interface.  We don't want to export it in
that form.  Unless you want to wait for that, this seems like a blocker for
that style of interface.

>   enum {
> 	AC_DL_LOAD_ADDITIONS,
> 	AC_DL_LOAD_REMOVALS
>   } libc_atomic_counter_t;

For an exported interface using a parameter like this does not seem like
the best plan to me.  If new flavors are added, this is not well-expressed
for ABI compatibility.  To me, the simplest way to do compatibility issues
right is to use single-purpose entrypoints with separate names so symbol
versioning will just take care of it.  So, add a single entrypoint
specifically for these two counters now.  If you change it in the future,
you can add entrypoints or change the old entrypoint signature under the
same name using symbol versioning.

> In any case, if a callback-based approach is preferred, I can live
> with that, too.

I don't claim it's the best interface.  But that is the approach that
doesn't have other blocking issues right now.

Re: enabling caching for dl_iterate_phdr()

[Jakub Jelinek]

On Fri, Jan 16, 2004 at 04:57:26PM -0800, Roland McGrath wrote:
> > In any case, if a callback-based approach is preferred, I can live
> > with that, too.
> 
> I don't claim it's the best interface.  But that is the approach that
> doesn't have other blocking issues right now.

How about extending struct dl_phdr_info and passing an counter in it
to dl_iterate_phdr's callback?
The structure can be extended at the end, because dl_iterate_phdr
passes its size to the callback as well.
The callback is called with dl_load_lock held, so the counter doesn't
even have to be atomic.
libgcc_s would just see if size includes the additional field in the
first callback, if yes, it would record that counter and on subsequent
dl_iterate_phdr call it would first check whether the counter did not
change and if it is the same, it could use cached info.

	Jakub

Re: enabling caching for dl_iterate_phdr()

[Roland McGrath]

> How about extending struct dl_phdr_info and passing an counter in it
> to dl_iterate_phdr's callback?

That is easy enough.  OTOH, it's arguably not much harder for the callback
to just compare the results with saved results from the last run and decide
that way no new work has to be done.  That works with existing glibc's too.

Re: enabling caching for dl_iterate_phdr()

[David Mosberger]

>>>>> On Sat, 17 Jan 2004 00:29:31 +0100, Jakub Jelinek <jakub@redhat.com> said:

  Jakub> On Fri, Jan 16, 2004 at 04:57:26PM -0800, Roland McGrath wrote:
  >> > In any case, if a callback-based approach is preferred, I can live
  >> > with that, too.

  >> I don't claim it's the best interface.  But that is the approach that
  >> doesn't have other blocking issues right now.

  Jakub> How about extending struct dl_phdr_info and passing an counter in it
  Jakub> to dl_iterate_phdr's callback?
  Jakub> The structure can be extended at the end, because dl_iterate_phdr
  Jakub> passes its size to the callback as well.
  Jakub> The callback is called with dl_load_lock held, so the counter doesn't
  Jakub> even have to be atomic.
  Jakub> libgcc_s would just see if size includes the additional field in the
  Jakub> first callback, if yes, it would record that counter and on subsequent
  Jakub> dl_iterate_phdr call it would first check whether the counter did not
  Jakub> change and if it is the same, it could use cached info.

Sounds good to me.  I assume it'd be OK to have separate "additions"
and "removals" counters?  Only the latter is needed for avoiding stale
values in a cache, but the former helps performance, because
dl_iterate_phdr() can be stopped early if we know that the list didn't
change.

I'd be happy to prototype the necessary code, but at the moment, glibc
doesn't build for me on ia64 (it crashes in rpcgen).  I assume that's
due to the stuff Uli is working on.

	--david

Re: enabling caching for dl_iterate_phdr()

[David Mosberger]

>>>>> On Sat, 17 Jan 2004 00:29:31 +0100, Jakub Jelinek <jakub@redhat.com> said:

  Jakub> How about extending struct dl_phdr_info and passing an
  Jakub> counter in it to dl_iterate_phdr's callback?

How about the attached patch?  It seems to work for me at least for
this simple test program:

$ cat ~/tmp/t.c
#include <link.h>
#include <stdio.h>

static int callback (struct dl_phdr_info *info, size_t size, void *ptr) {
  printf ("  size = %Zu ", size);
  printf ("dlpi_adds = %u ", info->dlpi_adds);
  printf ("dlpi_subs = %u\n", info->dlpi_subs);
  return -1;
}

int main (int argc, char **argv) {
  void *handle; int ret;

  dl_iterate_phdr (callback, NULL);
  handle = dlopen ("libunwind.so", RTLD_LAZY);
  printf ("dlopen(libunwind.so)=%p\n", handle);
  dl_iterate_phdr (callback, NULL);
  ret = dlclose (handle);
  printf ("dlclose(libunwind.so)=%d\n", ret);
  dl_iterate_phdr (callback, NULL);
  return 0;
}
$ gcc -D_GNU_SOURCE -O -Wall t.c -I/tmp/davidm/usr/include/ -ldl
$ ./a.out
  size = 40 dlpi_adds = 4 dlpi_subs = 0
dlopen(libunwind.so)=0x20000008002e0030
  size = 40 dlpi_adds = 5 dlpi_subs = 0
dlclose(libunwind.so)=0
  size = 40 dlpi_adds = 5 dlpi_subs = 1

In terms of performance, this is clearly more expensive than just
checking a variable.  For libunwind, the approach adds about 5% to the
time required to initialize a stack unwind, but just hitting once in
the cache during the actual unwind will easily make up for the extra
cost, so I think it's OK.

Comments?

	--david

Index: elf/dl-close.c
===================================================================
RCS file: /cvs/glibc/libc/elf/dl-close.c,v
retrieving revision 1.98
diff -u -r1.98 dl-close.c
--- elf/dl-close.c	27 Apr 2003 06:19:09 -0000	1.98
+++ elf/dl-close.c	22 Jan 2004 00:26:54 -0000
@@ -319,6 +319,7 @@
   /* Notify the debugger we are about to remove some loaded objects.  */
   _r_debug.r_state = RT_DELETE;
   _dl_debug_state ();
+  ++GL(dl_load_subs);
 
 #ifdef USE_TLS
   size_t tls_free_start;
Index: elf/dl-iteratephdr.c
===================================================================
RCS file: /cvs/glibc/libc/elf/dl-iteratephdr.c,v
retrieving revision 1.9
diff -u -r1.9 dl-iteratephdr.c
--- elf/dl-iteratephdr.c	22 Oct 2003 07:09:41 -0000	1.9
+++ elf/dl-iteratephdr.c	22 Jan 2004 00:26:54 -0000
@@ -48,6 +48,8 @@
       info.dlpi_name = l->l_name;
       info.dlpi_phdr = l->l_phdr;
       info.dlpi_phnum = l->l_phnum;
+      info.dlpi_adds = GL(dl_load_adds);
+      info.dlpi_subs = GL(dl_load_subs);
       ret = callback (&info, sizeof (struct dl_phdr_info), data);
       if (ret)
 	break;
@@ -84,6 +86,8 @@
       info.dlpi_name = "";
       info.dlpi_phdr = _dl_phdr;
       info.dlpi_phnum = _dl_phnum;
+      info.dlpi_adds = GL(dl_load_adds);
+      info.dlpi_subs = GL(dl_load_subs);
       ret = (*callback) (&info, sizeof (struct dl_phdr_info), data);
       if (ret)
 	return ret;
Index: elf/dl-object.c
===================================================================
RCS file: /cvs/glibc/libc/elf/dl-object.c,v
retrieving revision 1.36
diff -u -r1.36 dl-object.c
--- elf/dl-object.c	25 Apr 2003 09:06:56 -0000	1.36
+++ elf/dl-object.c	22 Jan 2004 00:26:54 -0000
@@ -83,6 +83,7 @@
   else
     GL(dl_loaded) = new;
   ++GL(dl_nloaded);
+  ++GL(dl_load_adds);
 
   /* If we have no loader the new object acts as it.  */
   if (loader == NULL)
Index: elf/dl-support.c
===================================================================
RCS file: /cvs/glibc/libc/elf/dl-support.c,v
retrieving revision 1.79
diff -u -r1.79 dl-support.c
--- elf/dl-support.c	13 Jan 2004 15:41:27 -0000	1.79
+++ elf/dl-support.c	22 Jan 2004 00:26:54 -0000
@@ -71,6 +71,11 @@
 /* Number of object in the _dl_loaded list.  */
 unsigned int _dl_nloaded;
 
+/* Incremented whenever something may have been added to dl_loaded. */
+unsigned int _dl_load_adds;
+/* Incremented whenever something may have been removed from dl_loaded. */
+unsigned int _dl_load_subs;
+
 /* Fake scope.  In dynamically linked binaries this is the scope of the
    main application but here we don't have something like this.  So
    create a fake scope containing nothing.  */
Index: elf/link.h
===================================================================
RCS file: /cvs/glibc/libc/elf/link.h,v
retrieving revision 1.76
diff -u -r1.76 link.h
--- elf/link.h	16 Sep 2003 05:48:04 -0000	1.76
+++ elf/link.h	22 Jan 2004 00:26:54 -0000
@@ -100,6 +100,12 @@
     const char *dlpi_name;
     const ElfW(Phdr) *dlpi_phdr;
     ElfW(Half) dlpi_phnum;
+
+    /* Note: older versions of libc do not provide the following
+       members.  Check the SIZE argument pass to the dl_iterate_phdr()
+       callback to determine whether or not they areprovided.  */
+    unsigned int dlpi_adds; /* incremented when phdrs may have been added */
+    unsigned int dlpi_subs; /* incremented when phdrs may have been removed */
   };
 
 __BEGIN_DECLS
Index: include/link.h
===================================================================
RCS file: /cvs/glibc/libc/include/link.h,v
retrieving revision 1.29
diff -u -r1.29 link.h
--- include/link.h	13 Jan 2004 08:27:52 -0000	1.29
+++ include/link.h	22 Jan 2004 00:26:55 -0000
@@ -293,6 +293,12 @@
     const char *dlpi_name;
     const ElfW(Phdr) *dlpi_phdr;
     ElfW(Half) dlpi_phnum;
+
+    /* Note: older versions of libc do not provide the following
+       members.  Check the SIZE argument pass to the dl_iterate_phdr()
+       callback to determine whether or not they areprovided.  */
+    unsigned int dlpi_adds; /* incremented when phdrs may have been added */
+    unsigned int dlpi_subs; /* incremented when phdrs may have been removed */
   };
 
 extern int dl_iterate_phdr (int (*callback) (struct dl_phdr_info *info,
Index: sysdeps/generic/ldsodefs.h
===================================================================
RCS file: /cvs/glibc/libc/sysdeps/generic/ldsodefs.h,v
retrieving revision 1.87
diff -u -r1.87 ldsodefs.h
--- sysdeps/generic/ldsodefs.h	15 Jan 2004 06:37:40 -0000	1.87
+++ sysdeps/generic/ldsodefs.h	22 Jan 2004 00:26:55 -0000
@@ -265,6 +265,11 @@
   EXTERN const char *_dl_platform;
   EXTERN size_t _dl_platformlen;
 
+  /* Incremented whenever something may have been added to dl_loaded.  */
+  EXTERN unsigned int _dl_load_adds;
+  /* Incremented whenever something may have been removed from dl_loaded.  */
+  EXTERN unsigned int _dl_load_subs;
+
 #ifndef MAP_ANON
   /* File descriptor referring to the zero-fill device.  */
   EXTERN int _dl_zerofd;

Re: enabling caching for dl_iterate_phdr()

[Jakub Jelinek]

On Wed, Jan 21, 2004 at 04:35:49PM -0800, David Mosberger wrote:
> --- elf/link.h	16 Sep 2003 05:48:04 -0000	1.76
> +++ elf/link.h	22 Jan 2004 00:26:54 -0000
> @@ -100,6 +100,12 @@
>      const char *dlpi_name;
>      const ElfW(Phdr) *dlpi_phdr;
>      ElfW(Half) dlpi_phnum;
> +
> +    /* Note: older versions of libc do not provide the following
> +       members.  Check the SIZE argument pass to the dl_iterate_phdr()
> +       callback to determine whether or not they areprovided.  */
> +    unsigned int dlpi_adds; /* incremented when phdrs may have been added */
> +    unsigned int dlpi_subs; /* incremented when phdrs may have been removed */

I'd make the counters unsigned long long instead to avoid wrap around.

Also, could you write a testcase for it? dl_iterate_phdr, then dlopen some
shlib in elf, then dl_iterate_phdr again, check the counters, dlclose
etc.

	Jakub

Re: enabling caching for dl_iterate_phdr()

[David Mosberger]

>>>>> On Thu, 22 Jan 2004 07:29:45 +0100, Jakub Jelinek <jakub@redhat.com> said:

  Jakub> I'd make the counters unsigned long long instead to avoid
  Jakub> wrap around.

Fine by me.

  Jakub> Also, could you write a testcase for it? dl_iterate_phdr,
  Jakub> then dlopen some shlib in elf, then dl_iterate_phdr again,
  Jakub> check the counters, dlclose etc.

Sure.

Attached is an updated patch.  "make check subdirs=elf" passes all
tests (including the new tst-dlmodcount test).  If it looks OK,
please apply.

	--david

ChangeLog

2004-01-23  David Mosberger  <davidm@hpl.hp.com>

	* sysdeps/generic/ldsodefs.h (struct rtld_global): Add members
	_dl_load_adds and _dl_load_subs.
	* elf/dl-support.c (_dl_load_adds): New variable.
	(_dl_load_subs): Likewise.
	* elf/dl-object.c (_dl_new_object): Increment dl_load_adds.
	* elf/dl-close.c (_dl_close): Increment dl_load_subs.

	* elf/link.h (struct dl_phdr_info): Add members dlpi_adds and
	dlpi_subs.
	* include/link.h: Likewise.
	* elf/dl-iteratephdr.c (__dl_iterate_phdr): Initialize dlpi_adds
	and dlpi_subs members.
	(dl_iterate_phdr): Likewise.

	* elf/tst-dlmodcount.c: New file.
	* elf/Makefile (distribute): Mention tst-dlmodcount.c.
	(tests): If build-shared, mention tst-dlmodcount.
	($(objpfx)tst-dlmodcount): If build-shared, build and
	run tst-dlmodcount.

Index: elf/Makefile
--- elf/Makefile
+++ elf/Makefile
@@ -80,7 +80,7 @@
 		   reldep9.c reldep9mod1.c reldep9mod2.c reldep9mod3.c \
 		   tst-array1.exp tst-array2.exp tst-array4.exp \
 		   tst-array2dep.c \
-		   tst-execstack-mod.c \
+		   tst-execstack-mod.c tst-dlmodcount.c \
 		   check-textrel.c dl-sysdep.h
 
 CFLAGS-dl-runtime.c = -fexceptions -fasynchronous-unwind-tables
@@ -151,7 +151,7 @@
 	 restest2 next dblload dblunload reldep5 reldep6 reldep7 reldep8 \
 	 circleload1 tst-tls3 tst-tls4 tst-tls5 tst-tls6 tst-tls7 tst-tls8 \
 	 tst-tls10 tst-tls11 tst-tls12 tst-tls13 tst-tls14 tst-align \
-	 $(tests-execstack-$(have-z-execstack))
+	 $(tests-execstack-$(have-z-execstack)) tst-dlmodcount
 #	 reldep9
 test-srcs = tst-pathopt
 tests-vis-yes = vismain
@@ -713,4 +713,8 @@
 	  $(sort $(wildcard $(common-objpfx)*/lib*.so \
 			    $(common-objpfx)iconvdata/*.so)) > $@
 generated += check-textrel check-textrel.out
+
+$(objpfx)tst-dlmodcount: $(libdl)
+$(objpfx)tst-dlmodcount.out: $(test-modules)
+
 endif
Index: elf/dl-close.c
--- elf/dl-close.c
+++ elf/dl-close.c
@@ -319,6 +319,7 @@
   /* Notify the debugger we are about to remove some loaded objects.  */
   _r_debug.r_state = RT_DELETE;
   _dl_debug_state ();
+  ++GL(dl_load_subs);
 
 #ifdef USE_TLS
   size_t tls_free_start;
Index: elf/dl-iteratephdr.c
--- elf/dl-iteratephdr.c
+++ elf/dl-iteratephdr.c
@@ -48,6 +48,8 @@
       info.dlpi_name = l->l_name;
       info.dlpi_phdr = l->l_phdr;
       info.dlpi_phnum = l->l_phnum;
+      info.dlpi_adds = GL(dl_load_adds);
+      info.dlpi_subs = GL(dl_load_subs);
       ret = callback (&info, sizeof (struct dl_phdr_info), data);
       if (ret)
 	break;
@@ -84,6 +86,8 @@
       info.dlpi_name = "";
       info.dlpi_phdr = _dl_phdr;
       info.dlpi_phnum = _dl_phnum;
+      info.dlpi_adds = GL(dl_load_adds);
+      info.dlpi_subs = GL(dl_load_subs);
       ret = (*callback) (&info, sizeof (struct dl_phdr_info), data);
       if (ret)
 	return ret;
Index: elf/dl-object.c
--- elf/dl-object.c
+++ elf/dl-object.c
@@ -83,6 +83,7 @@
   else
     GL(dl_loaded) = new;
   ++GL(dl_nloaded);
+  ++GL(dl_load_adds);
 
   /* If we have no loader the new object acts as it.  */
   if (loader == NULL)
Index: elf/dl-support.c
--- elf/dl-support.c
+++ elf/dl-support.c
@@ -71,6 +71,11 @@
 /* Number of object in the _dl_loaded list.  */
 unsigned int _dl_nloaded;
 
+/* Incremented whenever something may have been added to dl_loaded. */
+unsigned long long _dl_load_adds;
+/* Incremented whenever something may have been removed from dl_loaded. */
+unsigned long long _dl_load_subs;
+
 /* Fake scope.  In dynamically linked binaries this is the scope of the
    main application but here we don't have something like this.  So
    create a fake scope containing nothing.  */
Index: elf/link.h
--- elf/link.h
+++ elf/link.h
@@ -100,6 +100,12 @@
     const char *dlpi_name;
     const ElfW(Phdr) *dlpi_phdr;
     ElfW(Half) dlpi_phnum;
+
+    /* Note: older versions of libc do not provide the following
+       members.  Check the SIZE argument pass to the dl_iterate_phdr()
+       callback to determine whether or not they areprovided.  */
+    unsigned long long dlpi_adds; /* incr. when phdrs may have been added */
+    unsigned long long dlpi_subs; /* incr. when phdrs may have been removed */
   };
 
 __BEGIN_DECLS
Index: elf/tst-dlmodcount.c
--- /dev/null
+++ elf/tst-dlmodcount.c
@@ -0,0 +1,107 @@
+/* Copyright (C) 2004 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+   Contributed by David Mosberger <davidm@hpl.hp.com>, 2004.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library 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
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, write to the Free
+   Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
+   02111-1307 USA.  */
+
+#include <link.h>
+#include <stdio.h>
+
+#define SET	0
+#define ADD	1
+#define REMOVE	2
+
+#define leq(l,r)	(((r) - (l)) <= ~0ULL/2)
+
+static int
+callback (struct dl_phdr_info *info, size_t size, void *ptr)
+{
+  static int last_adds = 0, last_subs = 0;
+  intptr_t cmd = (intptr_t) ptr;
+
+  printf ("  size = %Zu\n", size);
+  if (size < (offsetof (struct dl_phdr_info, dlpi_subs)
+	      + sizeof (info->dlpi_subs)))
+    {
+      fprintf (stderr, "dl_iterate_phdr failed to pass dlpi_adds/dlpi_subs\n");
+      exit (5);
+    }
+
+  printf ("  dlpi_adds = %Lu dlpi_subs = %Lu\n",
+	  info->dlpi_adds, info->dlpi_subs);
+
+  switch (cmd)
+    {
+    case SET:
+      break;
+
+    case ADD:
+      if (leq (info->dlpi_adds, last_adds))
+	{
+	  fprintf (stderr, "dlpi_adds failed to get incremented!\n");
+	  exit (3);
+	}
+      break;
+
+    case REMOVE:
+      if (leq (info->dlpi_subs, last_subs))
+	{
+	  fprintf (stderr, "dlpi_subs failed to get incremented!\n");
+	  exit (4);
+	}
+      break;
+    }
+  last_adds = info->dlpi_adds;
+  last_subs = info->dlpi_subs;
+  return -1;
+}
+
+static void *
+load (const char *path)
+{
+  void *handle;
+
+  printf ("loading `%s'\n", path);
+  handle = dlopen (path, RTLD_LAZY);
+  if (!handle)
+    exit (1);
+  dl_iterate_phdr (callback, (void *)(intptr_t) ADD);
+  return handle;
+}
+
+static void
+unload (const char *path, void *handle)
+{
+  int ret;
+
+  printf ("unloading `%s'\n", path);
+  if (dlclose (handle) < 0)
+    exit (2);
+  dl_iterate_phdr (callback, (void *)(intptr_t) REMOVE);
+}
+
+int
+main (int argc, char **argv)
+{
+  void *handle1, *handle2;
+
+  dl_iterate_phdr (callback, (void *)(intptr_t) SET);
+  handle1 = load ("firstobj.so");
+  handle2 = load ("globalmod1.so");
+  unload ("firstobj.so", handle1);
+  unload ("globalmod1.so", handle2);
+  return 0;
+}
Index: include/link.h
--- include/link.h
+++ include/link.h
@@ -293,6 +293,12 @@
     const char *dlpi_name;
     const ElfW(Phdr) *dlpi_phdr;
     ElfW(Half) dlpi_phnum;
+
+    /* Note: older versions of libc do not provide the following
+       members.  Check the SIZE argument pass to the dl_iterate_phdr()
+       callback to determine whether or not they areprovided.  */
+    unsigned long long dlpi_adds; /* incr. when phdrs may have been added */
+    unsigned long long dlpi_subs; /* incr. when phdrs may have been removed */
   };
 
 extern int dl_iterate_phdr (int (*callback) (struct dl_phdr_info *info,
Index: sysdeps/generic/ldsodefs.h
--- sysdeps/generic/ldsodefs.h
+++ sysdeps/generic/ldsodefs.h
@@ -265,6 +265,11 @@
   EXTERN const char *_dl_platform;
   EXTERN size_t _dl_platformlen;
 
+  /* Incremented whenever something may have been added to dl_loaded.  */
+  EXTERN unsigned long long _dl_load_adds;
+  /* Incremented whenever something may have been removed from dl_loaded.  */
+  EXTERN unsigned long long _dl_load_subs;
+
 #ifndef MAP_ANON
   /* File descriptor referring to the zero-fill device.  */
   EXTERN int _dl_zerofd;

Re: enabling caching for dl_iterate_phdr()

[Ulrich Drepper]

David Mosberger wrote:

> Attached is an updated patch.  "make check subdirs=elf" passes all
> tests (including the new tst-dlmodcount test).

I've applied the patch.

-- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖

Re: enabling caching for dl_iterate_phdr()

[David Mosberger]

>>>>> On Sat, 24 Jan 2004 12:11:14 -0800, Ulrich Drepper <drepper@redhat.com> said:

  Uli> David Mosberger wrote:
  >> Attached is an updated patch.  "make check subdirs=elf" passes
  >> all tests (including the new tst-dlmodcount test).

  Uli> I've applied the patch.

Thanks!

In the meantime, it occurred to me that we can get rid of one of the
"unsigned long long" variables by taking advantage of the fact that
"_dl_nloaded == _dl_load_adds - _dl_load_subs" (provided there are
never more than 0xffffffff shared objects loaded).  Apart from saving
8 bytes of global data, it also has the advantage that it makes code
maintenance a bit easier.  The rule is very simple now: wherever
_dl_nloaded gets incremented, _dl_load_adds needs to be incremented as
well.

If it looks OK to you, please apply (it was "make subdirs=elf check" tested).

Thanks,

	--david

ChangeLog

2004-01-26  David Mosberger  <davidm@hpl.hp.com>

	* elf/link.h (struct dl_phdr_info): Fix typo in comment.
	* elf/rtld.c (dl_main): After incrementing dl_nloaded, also
	increment dl_load_adds.
	* elf/dl-iteratephdr.c (__dl_iterate_phdr): Replace GL(dl_load_subs)
	with equivalent GL(dl_load_adds - GL(dl_nloaded).
	(dl_iterate_phdr): Likewise.
	* elf/dl-close.c (_dl_close): Delete increment of GL(dl_load_subs).
	* elf/dl-support.c: Remove global variable _dl_load_subs.
	* sysdeps/generic/ldsodefs.h (struct rtld_global): Remove
	_dl_load_subs member.

Index: elf/dl-close.c
===================================================================
RCS file: /cvs/glibc/libc/elf/dl-close.c,v
retrieving revision 1.99
diff -u -r1.99 dl-close.c
--- elf/dl-close.c	24 Jan 2004 20:07:38 -0000	1.99
+++ elf/dl-close.c	26 Jan 2004 21:42:35 -0000
@@ -319,7 +319,6 @@
   /* Notify the debugger we are about to remove some loaded objects.  */
   _r_debug.r_state = RT_DELETE;
   _dl_debug_state ();
-  ++GL(dl_load_subs);
 
 #ifdef USE_TLS
   size_t tls_free_start;
Index: elf/dl-iteratephdr.c
===================================================================
RCS file: /cvs/glibc/libc/elf/dl-iteratephdr.c,v
retrieving revision 1.10
diff -u -r1.10 dl-iteratephdr.c
--- elf/dl-iteratephdr.c	24 Jan 2004 20:07:51 -0000	1.10
+++ elf/dl-iteratephdr.c	26 Jan 2004 21:42:35 -0000
@@ -49,7 +49,7 @@
       info.dlpi_phdr = l->l_phdr;
       info.dlpi_phnum = l->l_phnum;
       info.dlpi_adds = GL(dl_load_adds);
-      info.dlpi_subs = GL(dl_load_subs);
+      info.dlpi_subs = GL(dl_load_adds) - GL(dl_nloaded);
       ret = callback (&info, sizeof (struct dl_phdr_info), data);
       if (ret)
 	break;
@@ -87,7 +87,7 @@
       info.dlpi_phdr = _dl_phdr;
       info.dlpi_phnum = _dl_phnum;
       info.dlpi_adds = GL(dl_load_adds);
-      info.dlpi_subs = GL(dl_load_subs);
+      info.dlpi_subs = GL(dl_load_adds) - GL(dl_nloaded);
       ret = (*callback) (&info, sizeof (struct dl_phdr_info), data);
       if (ret)
 	return ret;
Index: elf/dl-support.c
===================================================================
RCS file: /cvs/glibc/libc/elf/dl-support.c,v
retrieving revision 1.80
diff -u -r1.80 dl-support.c
--- elf/dl-support.c	24 Jan 2004 20:08:29 -0000	1.80
+++ elf/dl-support.c	26 Jan 2004 21:42:35 -0000
@@ -73,8 +73,6 @@
 
 /* Incremented whenever something may have been added to dl_loaded. */
 unsigned long long _dl_load_adds;
-/* Incremented whenever something may have been removed from dl_loaded. */
-unsigned long long _dl_load_subs;
 
 /* Fake scope.  In dynamically linked binaries this is the scope of the
    main application but here we don't have something like this.  So
Index: elf/link.h
===================================================================
RCS file: /cvs/glibc/libc/elf/link.h,v
retrieving revision 1.77
diff -u -r1.77 link.h
--- elf/link.h	24 Jan 2004 20:08:50 -0000	1.77
+++ elf/link.h	26 Jan 2004 21:42:35 -0000
@@ -103,7 +103,7 @@
 
     /* Note: the next two members were introduced after the first
        version of this structure was available.  Check the SIZE
-       argument pass to the dl_iterate_phdr() callback to determine
+       argument passed to the dl_iterate_phdr() callback to determine
        whether or not they are provided.  */
 
     /* Incremented when a new object may have been added.  */
Index: elf/rtld.c
===================================================================
RCS file: /cvs/glibc/libc/elf/rtld.c,v
retrieving revision 1.310
diff -u -r1.310 rtld.c
--- elf/rtld.c	24 Jan 2004 08:33:44 -0000	1.310
+++ elf/rtld.c	26 Jan 2004 21:42:35 -0000
@@ -1020,6 +1020,7 @@
   GL(dl_loaded)->l_next = &GL(dl_rtld_map);
   GL(dl_rtld_map).l_prev = GL(dl_loaded);
   ++GL(dl_nloaded);
+  ++GL(dl_load_adds);
 
   /* If LD_USE_LOAD_BIAS env variable has not been seen, default
      to not using bias for non-prelinked PIEs and libraries
Index: sysdeps/generic/ldsodefs.h
===================================================================
RCS file: /cvs/glibc/libc/sysdeps/generic/ldsodefs.h,v
retrieving revision 1.89
diff -u -r1.89 ldsodefs.h
--- sysdeps/generic/ldsodefs.h	24 Jan 2004 20:10:18 -0000	1.89
+++ sysdeps/generic/ldsodefs.h	26 Jan 2004 21:42:36 -0000
@@ -267,8 +267,6 @@
 
   /* Incremented whenever something may have been added to dl_loaded.  */
   EXTERN unsigned long long _dl_load_adds;
-  /* Incremented whenever something may have been removed from dl_loaded.  */
-  EXTERN unsigned long long _dl_load_subs;
 
 #ifndef MAP_ANON
   /* File descriptor referring to the zero-fill device.  */

Re: enabling caching for dl_iterate_phdr()

[Ulrich Drepper]

David Mosberger wrote:

> In the meantime, it occurred to me that we can get rid of one of the
> "unsigned long long" variables by taking advantage of the fact that
> "_dl_nloaded == _dl_load_adds - _dl_load_subs"

_dl_nloaded is not available outside ld.so.

-- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖

Re: enabling caching for dl_iterate_phdr()

[Roland McGrath]

> David Mosberger wrote:
> 
> > In the meantime, it occurred to me that we can get rid of one of the
> > "unsigned long long" variables by taking advantage of the fact that
> > "_dl_nloaded == _dl_load_adds - _dl_load_subs"
> 
> _dl_nloaded is not available outside ld.so.

It's part of rtld_global, just like _dl_load_adds is.
We are only talking about dl_iterate_phdr's implementation here.

Re: enabling caching for dl_iterate_phdr()

[David Mosberger]

>>>>> On Mon, 26 Jan 2004 14:26:07 -0800, Ulrich Drepper <drepper@redhat.com> said:

  Uli> David Mosberger wrote:
  >> In the meantime, it occurred to me that we can get rid of one of
  >> the "unsigned long long" variables by taking advantage of the
  >> fact that "_dl_nloaded == _dl_load_adds - _dl_load_subs"

  Uli> _dl_nloaded is not available outside ld.so.

I'm probably missing something obvious, but why is this a problem?  As
far as I can see, _dl_nloaded has the same visibility as _dl_load_subs.

	--david

Re: enabling caching for dl_iterate_phdr()

[Roland McGrath]

> Would it be possible to add two atomic counters, dl_load_additions and
> dl_load_removals, which would get incremented whenever something is a
> phdr is added to/removed from the list that is traversed by
> dl_iterate_phdr()?

What interface do you propose for exposing such counters?
We won't be adding any published interfaces using variables.

> Any opinions?  I guess the alternative that was mentioned in the past
> was to use callbacks, but that seems very dangerous in regards to
> deadlock etc.

If your callback functions do nothing but increment atomic counters of your
own, then there is no problem.

enabling caching for dl_iterate_phdr()

[David Mosberger]

Would it be possible to add two atomic counters, dl_load_additions and
dl_load_removals, which would get incremented whenever something is a
phdr is added to/removed from the list that is traversed by
dl_iterate_phdr()?

The motivation for this is that this would let me solve a
long-standing problem with libunwind where I'd like to cache
unwind-info, but cannot do so safely because there is no way for
libunwind to detect efficiently when a shared object may have gotten
unloaded.  Having both the "additions" and "removals" counters would
make it possible to cache both negative and positive information,
though I'd be quite happy if there was at least a "removals" counter.

Any opinions?  I guess the alternative that was mentioned in the past
was to use callbacks, but that seems very dangerous in regards to
deadlock etc.

	--david