[RFC][patch] Avoid repeated calls to solib_add on initial attach.

[RFC][patch] Avoid repeated calls to solib_add on initial attach.

[Paul Pluzhnikov]

Greetings,

Following up on my earlier "slow on high-latency links" message:
http://sourceware.org/ml/gdb-patches/2011-07/msg00391.html ...

Attached patch avoids calling solib_add twice when initially attaching
inferior.

I am not entirely happy about this patch, but don't have a better idea
for a fix, and do want to avoid repeated rescans of the shared library list.

(Some of our executables use 4000+ shared libraries, and the time in
solib_add does add up.)

Tested on Linux/x86_64, no regressions.

Comments?

Thanks,
--
Paul Pluzhnikov



2011-07-15  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* inferior.h (struct inferior): Add solib_add_generation.
	* infcmd.c (post_create_inferior): Only call solib_add if not
	already done.
	* solib.c (solib_add): Increment solib_add_generation.



Index: inferior.h
===================================================================
RCS file: /cvs/src/src/gdb/inferior.h,v
retrieving revision 1.160
diff -u -p -r1.160 inferior.h
--- inferior.h	3 Jun 2011 15:32:44 -0000	1.160
+++ inferior.h	15 Jul 2011 20:10:20 -0000
@@ -536,6 +536,9 @@ struct inferior
      if any catching is necessary.  */
   int total_syscalls_count;
 
+  /* This counts the number of solib_add() calls performed.  */
+  int solib_add_generation;
+
   /* Per inferior data-pointers required by other GDB modules.  */
   void **data;
   unsigned num_data;
Index: solib.c
===================================================================
RCS file: /cvs/src/src/gdb/solib.c,v
retrieving revision 1.149
diff -u -p -r1.149 solib.c
--- solib.c	30 Jun 2011 19:29:54 -0000	1.149
+++ solib.c	15 Jul 2011 20:10:20 -0000
@@ -914,6 +914,8 @@ solib_add (char *pattern, int from_tty,
 {
   struct so_list *gdb;
 
+  current_inferior ()->solib_add_generation++;
+
   if (pattern)
     {
       char *re_err = re_comp (pattern);
Index: infcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/infcmd.c,v
retrieving revision 1.287
diff -u -p -r1.287 infcmd.c
--- infcmd.c	30 May 2011 18:04:32 -0000	1.287
+++ infcmd.c	15 Jul 2011 20:50:56 -0000
@@ -398,6 +398,7 @@ void
 post_create_inferior (struct target_ops *target, int from_tty)
 {
   volatile struct gdb_exception ex;
+  int solib_add_generation;
 
   /* Be sure we own the terminal in case write operations are performed.  */ 
   target_terminal_ours ();
@@ -419,6 +420,7 @@ post_create_inferior (struct target_ops 
   if (ex.reason < 0 && ex.error != NOT_AVAILABLE_ERROR)
     throw_exception (ex);
 
+  solib_add_generation = current_inferior ()->solib_add_generation;
   if (exec_bfd)
     {
       /* Create the hooks to handle shared library load and unload
@@ -432,14 +434,16 @@ post_create_inferior (struct target_ops 
 
   /* If the solist is global across processes, there's no need to
      refetch it here.  */
-  if (exec_bfd && !gdbarch_has_global_solist (target_gdbarch))
+  if (exec_bfd && !gdbarch_has_global_solist (target_gdbarch)
+      && current_inferior ()->solib_add_generation == solib_add_generation)
     {
       /* Sometimes the platform-specific hook loads initial shared
 	 libraries, and sometimes it doesn't.  If it doesn't FROM_TTY will be
 	 incorrectly 0 but such solib targets should be fixed anyway.  If we
 	 made all the inferior hook methods consistent, this call could be
 	 removed.  Call it only after the solib target has been initialized by
-	 solib_create_inferior_hook.  */
+	 solib_create_inferior_hook.  Only do this if not alreay done from
+	 inside solib_create_inferior_hook.  */
 
 #ifdef SOLIB_ADD
       SOLIB_ADD (NULL, 0, target, auto_solib_add);

Re: [RFC][patch] Avoid repeated calls to solib_add on initial attach.

[Gary Benson]

Paul Pluzhnikov wrote:
> Following up on my earlier "slow on high-latency links" message:
> http://sourceware.org/ml/gdb-patches/2011-07/msg00391.html ...
> 
> Attached patch avoids calling solib_add twice when initially attaching
> inferior.
> 
> I am not entirely happy about this patch, but don't have a better
> idea for a fix, and do want to avoid repeated rescans of the shared
> library list.
> 
> (Some of our executables use 4000+ shared libraries, and the time in
> solib_add does add up.)
> 
> Tested on Linux/x86_64, no regressions.
> 
> Comments?

I've had a look at this and aside from the typo noted below it seems
fine.  I didn't see any speedup on the 1000 library testcase I've been
using for my own stuff (the bulk of the time seemed to be printing all
the "Loaded symbols for X.so" and "Reading symbols from X.so"
messages) but maybe this is because I'm running everything locally and
possibly you are not?  In any event, the second solib_add call was
skipped as expected.

>  	 made all the inferior hook methods consistent, this call could be
>  	 removed.  Call it only after the solib target has been initialized by
> -	 solib_create_inferior_hook.  */
> +	 solib_create_inferior_hook.  Only do this if not alreay done from
                                                          ^^^^^^
							  Typo

Cheers,
Gary

-- 
http://gbenson.net/

Re: [RFC][patch] Avoid repeated calls to solib_add on initial attach.

[Pedro Alves]

[-- Attachment #1: Type: Text/Plain, Size: 7988 bytes --]

On Friday 15 July 2011 21:52:09, Paul Pluzhnikov wrote:
> Greetings,
> 
> Following up on my earlier "slow on high-latency links" message:
> http://sourceware.org/ml/gdb-patches/2011-07/msg00391.html ...
> 
> Attached patch avoids calling solib_add twice when initially attaching
> inferior.
> 
> I am not entirely happy about this patch, but don't have a better idea
> for a fix, and do want to avoid repeated rescans of the shared library list.

Me neigher, apart from doing as the comment says:

>   /* Sometimes the platform-specific hook loads initial shared
>	 libraries, and sometimes it doesn't.  If it doesn't FROM_TTY will be
>	 incorrectly 0 but such solib targets should be fixed anyway.  If we
>	 made all the inferior hook methods consistent, this call could be
>	 removed.  Call it only after the solib target has been initialized by
>	 solib_create_inferior_hook.  */

Or, making retrieving the dso list cheap enough to not care about the
multiple calls.  By e.g., caching more things, and making
target_read_string not super dumb (as you've yourself proposed).

Can you give a try on the attached patches and see how much
a difference they make for you?  We wrote these along
with yesterday's tracepoint's one, exactly to speed up
remote shared library loading/debugging on Android.  Patch 2
has the most dramatic affect; patches 1 and 3 make some
difference, though not as big.

Luis wrote at the time:

for patch 1:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
This patch handles the caching of the AUXV block. Once loaded, GDB has 
access to the whole block of information. Queries can be made without 
the need to read the whole block from the remote target again). Some 
fields still require reading from the target (like the AT_PLATFORM 
field), but these tend to be smaller in size.

The cache is invalidated whenever any of these events occur: 
inferior_exit, inferior_appeared and executable_changed.

The data is kept in the following structure:

struct auxv_info
{
   LONGEST length;
   gdb_byte *data;
};

The inferior_data machinery is used to keep per-inferior caches of the AUXV.

Regressions:

I regtested this patch and found no regressions on x86-32/64.

Performance gains:

This patch reduces the overhead a bit since read_program_header queries 
the AUXV machinery three times. There are other calls as well. Though 
the performance gains are small, they're still noticeable.


for patch 2:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
This patch (based on Pedro's patch) tries to optimize string reads from 
the target. Currently GDB reads 4 bytes at a time, and that's a big 
overhead if we're reading long path names. Experimenting with a few 
other block sizes, 64-bytes blocks seem to work great. This patch 
basically does the following:

- Stabilish bigger block sizes for the string reads.

- Replace target_read_memory with target_read_partial (Pedro's 
suggestion) to make sure we're testing for the terminating char at every 
tiny read (for targets that only support smaller packets).

Worth mentioning that i did try using "hints" for the sizes, so we could 
adjust the block size to be close to the estimated length of the path 
names. This works nicely for long path names, but if we have a scenario 
where there's a pattern like short/long/short/long, i'm inclined to say 
it would perform just the same as using a standard 64-bytes block.

Regressions:
---

None. Regtested on x86-32/64

Performance gains:
---

This is by far the patch that gives the most performance since it allows 
the reads to be performed in bigger blocks. This is specially 
interesting for remote targets, but also speeds up local targets a little.

for patch 3:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
This patch caches the program header
information so we don't need to load it everytime. I've also used the 
inferior_data machinery to store such information. Additionaly, since 
it's a cache, i removed code that frees the allocated buffer after its 
use. That buffer is only deallocated inside the cleanup function instead 
of in functions that use this buffer.

Regressions:
---

Regtested the patch on x86-32/64 and found no additional errors.

Performance gains:
---

Reduces the overhead a little by caching the headers. I did some tests 
and noticed only a small improvement, but the function seems to be 
called a few times during the life of the process. It's worth just to 
reduce the number of unnecessary reads.

> 
> (Some of our executables use 4000+ shared libraries, and the time in
> solib_add does add up.)
> 
> Tested on Linux/x86_64, no regressions.
> 
> Comments?
> 
> Thanks,
> --
> Paul Pluzhnikov
> 
> 
> 
> 2011-07-15  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
> 	* inferior.h (struct inferior): Add solib_add_generation.
> 	* infcmd.c (post_create_inferior): Only call solib_add if not
> 	already done.
> 	* solib.c (solib_add): Increment solib_add_generation.
> 
> 
> 
> Index: inferior.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/inferior.h,v
> retrieving revision 1.160
> diff -u -p -r1.160 inferior.h
> --- inferior.h	3 Jun 2011 15:32:44 -0000	1.160
> +++ inferior.h	15 Jul 2011 20:10:20 -0000
> @@ -536,6 +536,9 @@ struct inferior
>       if any catching is necessary.  */
>    int total_syscalls_count;
>  
> +  /* This counts the number of solib_add() calls performed.  */
> +  int solib_add_generation;
> +
>    /* Per inferior data-pointers required by other GDB modules.  */
>    void **data;
>    unsigned num_data;
> Index: solib.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/solib.c,v
> retrieving revision 1.149
> diff -u -p -r1.149 solib.c
> --- solib.c	30 Jun 2011 19:29:54 -0000	1.149
> +++ solib.c	15 Jul 2011 20:10:20 -0000
> @@ -914,6 +914,8 @@ solib_add (char *pattern, int from_tty,
>  {
>    struct so_list *gdb;
>  
> +  current_inferior ()->solib_add_generation++;
> +
>    if (pattern)
>      {
>        char *re_err = re_comp (pattern);
> Index: infcmd.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infcmd.c,v
> retrieving revision 1.287
> diff -u -p -r1.287 infcmd.c
> --- infcmd.c	30 May 2011 18:04:32 -0000	1.287
> +++ infcmd.c	15 Jul 2011 20:50:56 -0000
> @@ -398,6 +398,7 @@ void
>  post_create_inferior (struct target_ops *target, int from_tty)
>  {
>    volatile struct gdb_exception ex;
> +  int solib_add_generation;
>  
>    /* Be sure we own the terminal in case write operations are performed.  */ 
>    target_terminal_ours ();
> @@ -419,6 +420,7 @@ post_create_inferior (struct target_ops 
>    if (ex.reason < 0 && ex.error != NOT_AVAILABLE_ERROR)
>      throw_exception (ex);
>  
> +  solib_add_generation = current_inferior ()->solib_add_generation;
>    if (exec_bfd)
>      {
>        /* Create the hooks to handle shared library load and unload
> @@ -432,14 +434,16 @@ post_create_inferior (struct target_ops 
>  
>    /* If the solist is global across processes, there's no need to
>       refetch it here.  */
> -  if (exec_bfd && !gdbarch_has_global_solist (target_gdbarch))
> +  if (exec_bfd && !gdbarch_has_global_solist (target_gdbarch)
> +      && current_inferior ()->solib_add_generation == solib_add_generation)
>      {
>        /* Sometimes the platform-specific hook loads initial shared
>  	 libraries, and sometimes it doesn't.  If it doesn't FROM_TTY will be
>  	 incorrectly 0 but such solib targets should be fixed anyway.  If we
>  	 made all the inferior hook methods consistent, this call could be
>  	 removed.  Call it only after the solib target has been initialized by
> -	 solib_create_inferior_hook.  */
> +	 solib_create_inferior_hook.  Only do this if not alreay done from
> +	 inside solib_create_inferior_hook.  */
>  
>  #ifdef SOLIB_ADD
>        SOLIB_ADD (NULL, 0, target, auto_solib_add);
> 

-- 
Pedro Alves

[-- Attachment #2: 1-cached_auxv.diff --]
[-- Type: text/x-patch, Size: 5728 bytes --]

2011-07-20  Pedro Alves  <pedro@codesourcery.com>
	    Luis Machado  <lgustavo@codesourcery.com>

	gdb/
	* auxv.c: Include observer.h.
	(auxv_inferior_data_cleanup): New.
	(invalidate_auxv_cache_inf): New.
	(invalidate_auxv_cache): New.
	(get_auxv_inferior_data): New.
	(auxv_inferior_data): New static global.
	(auxv_info): New structure.
	(target_auxv_search): Use get_auxv_inferior_data instead of
	target_read_alloc and don't free cached buffers.
	(fprint_target_auxv): Likewise
	(_initialize_auxv): Register per-inferior auxv cache and register
	observers to invalidate auxv cache when needed.

---
 gdb/auxv.c |  116 ++++++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 100 insertions(+), 16 deletions(-)

Index: src/gdb/auxv.c
===================================================================
--- src.orig/gdb/auxv.c	2011-01-13 15:07:17.236075004 +0000
+++ src/gdb/auxv.c	2011-07-20 14:57:01.733657565 +0100
@@ -26,6 +26,7 @@
 #include "valprint.h"
 #include "gdb_assert.h"
 #include "gdbcore.h"
+#include "observer.h"
 
 #include "auxv.h"
 #include "elf/common.h"
@@ -277,6 +278,77 @@ target_auxv_parse (struct target_ops *op
   return default_auxv_parse (ops, readptr, endptr, typep, valp);
 }
 
+
+/* Per-inferior data key for auxv.  */
+static const struct inferior_data *auxv_inferior_data;
+
+/*  Auxiliary Vector information structure.  This is used by GDB
+    for caching purposes for each inferior.  This helps reduce the
+    overhead of transfering data from a remote target to the local host.  */
+struct auxv_info
+{
+  LONGEST length;
+  gdb_byte *data;
+};
+
+/* Handles the cleanup of the auxv cache for inferior INF.  ARG is ignored.
+   Frees whatever allocated space there is to be freed and sets INF's auxv cache
+   data pointer to NULL.
+
+   This function is called when the following events occur: inferior_appeared,
+   inferior_exit and executable_changed.  */
+
+static void
+auxv_inferior_data_cleanup (struct inferior *inf, void *arg)
+{
+  struct auxv_info *info;
+
+  info = inferior_data (inf, auxv_inferior_data);
+  if (info != NULL)
+    {
+      xfree (info->data);
+      xfree (info);
+      set_inferior_data (inf, auxv_inferior_data, NULL);
+    }
+}
+
+/* Invalidate INF's auxv cache.  */
+
+static void
+invalidate_auxv_cache_inf (struct inferior *inf)
+{
+  auxv_inferior_data_cleanup (inf, NULL);
+}
+
+/* Invalidate current inferior's auxv cache.  */
+
+static void
+invalidate_auxv_cache (void)
+{
+  invalidate_auxv_cache_inf (current_inferior ());
+}
+
+/* Fetch the auxv object from inferior INF.  If auxv is cached already,
+   return a pointer to the cache.  If not, fetch the auxv object from the
+   target and cache it.  This function always returns a valid INFO pointer.  */
+
+static struct auxv_info *
+get_auxv_inferior_data (struct inferior *inf, struct target_ops *ops)
+{
+  struct auxv_info *info;
+
+  info = inferior_data (inf, auxv_inferior_data);
+  if (info == NULL)
+    {
+      info = XZALLOC (struct auxv_info);
+      info->length = target_read_alloc (ops, TARGET_OBJECT_AUXV,
+					NULL, &info->data);
+      set_inferior_data (inf, auxv_inferior_data, info);
+    }
+
+  return info;
+}
+
 /* Extract the auxiliary vector entry with a_type matching MATCH.
    Return zero if no such entry was found, or -1 if there was
    an error getting the information.  On success, return 1 after
@@ -286,28 +358,30 @@ target_auxv_search (struct target_ops *o
 {
   CORE_ADDR type, val;
   gdb_byte *data;
-  LONGEST n = target_read_alloc (ops, TARGET_OBJECT_AUXV, NULL, &data);
-  gdb_byte *ptr = data;
+  gdb_byte *ptr;
+  struct auxv_info *info;
+
+  info = get_auxv_inferior_data (current_inferior (), ops);
 
-  if (n <= 0)
-    return n;
+  data = info->data;
+  ptr = data;
+
+  if (info->length <= 0)
+    return info->length;
 
   while (1)
-    switch (target_auxv_parse (ops, &ptr, data + n, &type, &val))
+    switch (target_auxv_parse (ops, &ptr, data + info->length, &type, &val))
       {
       case 1:			/* Here's an entry, check it.  */
 	if (type == match)
 	  {
-	    xfree (data);
 	    *valp = val;
 	    return 1;
 	  }
 	break;
       case 0:			/* End of the vector.  */
-	xfree (data);
 	return 0;
       default:			/* Bogosity.  */
-	xfree (data);
 	return -1;
       }
 
@@ -321,15 +395,18 @@ fprint_target_auxv (struct ui_file *file
 {
   CORE_ADDR type, val;
   gdb_byte *data;
-  LONGEST len = target_read_alloc (ops, TARGET_OBJECT_AUXV, NULL,
-				   &data);
-  gdb_byte *ptr = data;
+  gdb_byte *ptr;
+  struct auxv_info *info;
   int ents = 0;
 
-  if (len <= 0)
-    return len;
+  info = get_auxv_inferior_data (current_inferior (), ops);
+
+  data = info->data;
+  ptr = data;
+  if (info->length <= 0)
+    return info->length;
 
-  while (target_auxv_parse (ops, &ptr, data + len, &type, &val) > 0)
+  while (target_auxv_parse (ops, &ptr, data + info->length, &type, &val) > 0)
     {
       const char *name = "???";
       const char *description = "";
@@ -418,8 +495,6 @@ fprint_target_auxv (struct ui_file *file
 	break;
     }
 
-  xfree (data);
-
   return ents;
 }
 
@@ -448,4 +523,13 @@ _initialize_auxv (void)
   add_info ("auxv", info_auxv_command,
 	    _("Display the inferior's auxiliary vector.\n\
 This is information provided by the operating system at program startup."));
+
+  /* Set an auxv cache per-inferior.  */
+  auxv_inferior_data
+    = register_inferior_data_with_cleanup (auxv_inferior_data_cleanup);
+
+  /* Observers used to invalidate the auxv cache when needed.  */
+  observer_attach_inferior_exit (invalidate_auxv_cache_inf);
+  observer_attach_inferior_appeared (invalidate_auxv_cache_inf);
+  observer_attach_executable_changed (invalidate_auxv_cache);
 }

[-- Attachment #3: 2-target_read_string.diff --]
[-- Type: text/x-patch, Size: 2931 bytes --]

2011-07-20  Pedro Alves  <pedro@codesourcery.com>
	    Luis Machado  <lgustavo@codesourcery.com>

	gdb/
	* target.c (target_read_partial): Declare prototype.
	(target_read_string): Use 64-bytes blocks for reads and use
	target_read_partial instead of target_read_memory.

---
 gdb/target.c |   45 +++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

Index: src/gdb/target.c
===================================================================
--- src.orig/gdb/target.c	2011-06-07 18:54:30.078164934 +0100
+++ src/gdb/target.c	2011-07-20 14:57:39.933657572 +0100
@@ -88,6 +88,11 @@ static LONGEST target_xfer_partial (stru
 				    void *readbuf, const void *writebuf,
 				    ULONGEST offset, LONGEST len);
 
+static LONGEST target_read_partial (struct target_ops *ops,
+				    enum target_object object,
+				    const char *annex, gdb_byte *buf,
+				    ULONGEST offset, LONGEST len);
+
 static struct gdbarch *default_thread_architecture (struct target_ops *ops,
 						    ptid_t ptid);
 
@@ -1185,55 +1190,51 @@ target_translate_tls_address (struct obj
 int
 target_read_string (CORE_ADDR memaddr, char **string, int len, int *errnop)
 {
-  int tlen, origlen, offset, i;
-  gdb_byte buf[4];
-  int errcode = 0;
-  char *buffer;
-  int buffer_allocated;
-  char *bufptr;
-  unsigned int nbytes_read = 0;
+  int tlen, i, errcode = 0, buffer_allocated;
+  gdb_byte buf[64];
+  char *buffer, *bufptr;
+  unsigned int nbytes_read = 0, raw_read = 0;
+  LONGEST xfered;
 
   gdb_assert (string);
 
   /* Small for testing.  */
-  buffer_allocated = 4;
+  buffer_allocated = 64;
   buffer = xmalloc (buffer_allocated);
   bufptr = buffer;
 
-  origlen = len;
-
   while (len > 0)
     {
-      tlen = MIN (len, 4 - (memaddr & 3));
-      offset = memaddr & 3;
+      tlen = MIN (len, 64);
 
-      errcode = target_read_memory (memaddr & ~3, buf, sizeof buf);
-      if (errcode != 0)
+      xfered = target_read_partial (current_target.beneath,
+				    TARGET_OBJECT_MEMORY,
+				    NULL, buf,
+				    memaddr, tlen);
+      if (xfered <= 0)
 	{
 	  /* The transfer request might have crossed the boundary to an
 	     unallocated region of memory.  Retry the transfer, requesting
 	     a single byte.  */
 	  tlen = 1;
-	  offset = 0;
 	  errcode = target_read_memory (memaddr, buf, 1);
 	  if (errcode != 0)
 	    goto done;
 	}
+      else
+	raw_read += tlen;
 
-      if (bufptr - buffer + tlen > buffer_allocated)
+      if (raw_read > buffer_allocated)
 	{
-	  unsigned int bytes;
-
-	  bytes = bufptr - buffer;
 	  buffer_allocated *= 2;
 	  buffer = xrealloc (buffer, buffer_allocated);
-	  bufptr = buffer + bytes;
+	  bufptr = buffer + raw_read - tlen;
 	}
 
       for (i = 0; i < tlen; i++)
 	{
-	  *bufptr++ = buf[i + offset];
-	  if (buf[i + offset] == '\000')
+	  *bufptr++ = buf[i];
+	  if (buf[i] == '\000')
 	    {
 	      nbytes_read += i + 1;
 	      goto done;

[-- Attachment #4: 3-program_headers.diff --]
[-- Type: text/x-patch, Size: 6400 bytes --]

2011-07-20  Pedro Alves  <pedro@codesourcery.com>
	    Luis Machado  <lgustavo@codesourcery.com>

	gdb/
	* solib-svr4.c (pheaders_inferior_data): New static global.
	(pheader_info): New structure.
	(pheaders_info): Likewise.
	(pheaders_inferior_data_cleanup): New function.
	(invalidate_pheaders_cache_inf): Likewise.
	(invalidate_pheaders_cache): Likewise.
	(get_pheaders_inferior_data): Likewise.
	(read_program_header): Use cached program header information
	if available, otherwise load and cache it.
	(_initialize_svr4_solib): Register per-inferior program headers
	cache and register observers to invalidate it when needed.

---
 gdb/solib-svr4.c |  145 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 142 insertions(+), 3 deletions(-)

Index: src/gdb/solib-svr4.c
===================================================================
--- src.orig/gdb/solib-svr4.c	2011-07-20 09:33:50.833654174 +0100
+++ src/gdb/solib-svr4.c	2011-07-20 14:58:07.633657577 +0100
@@ -460,6 +460,99 @@ bfd_lookup_symbol (bfd *abfd, const char
   return symaddr;
 }
 
+/* Per-inferior program headers data key.  */
+static const struct inferior_data *pheaders_inferior_data;
+
+/* Cache structure for a single program header.  */
+struct pheader_info
+{
+  gdb_byte *data;
+  int sect_size;
+  int arch_size;
+};
+
+/* Cache structure for multiple program headers.  */
+struct pheaders_info
+{
+  struct pheader_info *header_minus_one;
+  struct pheader_info *header_two;
+};
+
+/* Handles the cleanup of program headers' cache for inferior INF.  ARG is ignored.
+   Frees whatever allocated space there is to be freed and sets INF's program header
+   cache data pointers to NULL.
+
+   This function is called when the following events occur: inferior_appeared and
+   executable_changed.  */
+
+static void
+pheaders_inferior_data_cleanup (struct inferior *inf, void *arg)
+{
+  struct pheaders_info *info;
+
+  info = inferior_data (inf, pheaders_inferior_data);
+  if (info != NULL)
+    {
+      if (info->header_minus_one != NULL)
+	{
+	  xfree (info->header_minus_one->data);
+	  xfree (info->header_minus_one);
+	  info->header_minus_one = NULL;
+	}
+
+      if (info->header_two != NULL)
+	{
+	  xfree (info->header_two->data);
+	  xfree (info->header_two);
+	  info->header_two = NULL;
+	}
+
+      xfree (info);
+      set_inferior_data (inf, pheaders_inferior_data, NULL);
+    }
+}
+
+/* Invalidate INF's program headers' cache.  */
+
+static void
+invalidate_pheaders_cache_inf (struct inferior *inf)
+{
+  pheaders_inferior_data_cleanup (inf, NULL);
+}
+
+/* Invalidate current inferior's program headers' cache.  */
+
+static void
+invalidate_pheaders_cache (void)
+{
+  invalidate_pheaders_cache_inf (current_inferior ());
+}
+
+/* Fetch a pointer to the cached program headers for INF.  If the program headers
+   are already cached, return a pointer to the cache.  If not, just allocate an empty
+   cache data structure and let the calling function handle updating the cache with
+   current data. This function always returns a valid INFO pointer.  */
+
+static struct pheaders_info *
+get_pheaders_inferior_data (struct inferior *inf)
+{
+  struct pheaders_info *info;
+
+  info = inferior_data (inf, pheaders_inferior_data);
+  if (info == NULL)
+    {
+      info = XZALLOC (struct pheaders_info);
+
+      /* Make sure these are empty so the calling function
+	 understands it and provides data for the cache.  */
+      info->header_minus_one = NULL;
+      info->header_two = NULL;
+
+      set_inferior_data (inf, pheaders_inferior_data, info);
+    }
+
+  return info;
+}
 
 /* Read program header TYPE from inferior memory.  The header is found
    by scanning the OS auxillary vector.
@@ -481,6 +574,31 @@ read_program_header (int type, int *p_se
   CORE_ADDR sect_addr;
   gdb_byte *buf;
 
+  struct pheader_info *header_minus_one = NULL;
+  struct pheader_info *header_two = NULL;
+  struct pheaders_info *all_pheaders =
+			get_pheaders_inferior_data (current_inferior ());
+
+  header_minus_one = all_pheaders->header_minus_one;
+
+  /* Check for cached information and return it if available.  Load
+  information from target otherwise.  */
+  if (type == -1 && header_minus_one != NULL)
+    {
+      *p_sect_size = header_minus_one->sect_size;
+      *p_arch_size = header_minus_one->arch_size;
+      return header_minus_one->data;
+    }
+
+  header_two = all_pheaders->header_two;
+
+  if (type == 2 && header_two != NULL)
+    {
+      *p_sect_size = header_two->sect_size;
+      *p_arch_size = header_two->arch_size;
+      return header_two->data;
+    }
+
   /* Get required auxv elements from target.  */
   if (target_auxv_search (&current_target, AT_PHDR, &at_phdr) <= 0)
     return 0;
@@ -571,6 +689,22 @@ read_program_header (int type, int *p_se
   if (p_sect_size)
     *p_sect_size = sect_size;
 
+  /* Update the cache with header information.  */
+  if (type == -1)
+    {
+      all_pheaders->header_minus_one = xmalloc (sizeof (*header_minus_one));
+      all_pheaders->header_minus_one->arch_size = arch_size;
+      all_pheaders->header_minus_one->sect_size = sect_size;
+      all_pheaders->header_minus_one->data = buf;
+    }
+  else if (type == 2)
+    {
+      all_pheaders->header_two = xmalloc (sizeof (*header_minus_one));
+      all_pheaders->header_two->arch_size = arch_size;
+      all_pheaders->header_two->sect_size = sect_size;
+      all_pheaders->header_two->data = buf;
+    }
+
   return buf;
 }
 
@@ -755,12 +889,10 @@ scan_dyntag_auxv (int dyntag, CORE_ADDR
 	if (ptr)
 	  *ptr = dyn_ptr;
 
-	xfree (bufstart);
 	return 1;
       }
   }
 
-  xfree (bufstart);
   return 0;
 }
 
@@ -2044,7 +2176,6 @@ svr4_exec_displacement (CORE_ADDR *displ
 	    ok = 0;
 	}
 
-      xfree (buf);
       xfree (buf2);
 
       if (!ok)
@@ -2470,4 +2601,12 @@ _initialize_svr4_solib (void)
   svr4_so_ops.lookup_lib_global_symbol = elf_lookup_lib_symbol;
   svr4_so_ops.same = svr4_same;
   svr4_so_ops.keep_data_in_core = svr4_keep_data_in_core;
+
+  /* Set cache of program headers per-inferior.  */
+  pheaders_inferior_data
+    = register_inferior_data_with_cleanup (pheaders_inferior_data_cleanup);
+
+  /* Observers used to invalidate the program headers' cache when needed.  */
+  observer_attach_inferior_appeared (invalidate_pheaders_cache_inf);
+  observer_attach_executable_changed (invalidate_pheaders_cache);
 }

Re: [RFC][patch] Avoid repeated calls to solib_add on initial attach.

[Paul Pluzhnikov]

On Wed, Jul 20, 2011 at 7:37 AM, Pedro Alves <pedro@codesourcery.com> wrote:
> On Friday 15 July 2011 21:52:09, Paul Pluzhnikov wrote:
>> Greetings,
>>
>> Following up on my earlier "slow on high-latency links" message:
>> http://sourceware.org/ml/gdb-patches/2011-07/msg00391.html ...
>>
>> Attached patch avoids calling solib_add twice when initially attaching
>> inferior.
>>
>> I am not entirely happy about this patch, but don't have a better idea
>> for a fix, and do want to avoid repeated rescans of the shared library list.
>
> Me neigher, apart from doing as the comment says:
>
>>   /* Sometimes the platform-specific hook loads initial shared
>>        libraries, and sometimes it doesn't.  If it doesn't FROM_TTY will be
>>        incorrectly 0 but such solib targets should be fixed anyway.  If we
>>        made all the inferior hook methods consistent, this call could be
>>        removed.  Call it only after the solib target has been initialized by
>>        solib_create_inferior_hook.  */
>
> Or, making retrieving the dso list cheap enough to not care about the
> multiple calls.  By e.g., caching more things, and making
> target_read_string not super dumb (as you've yourself proposed).

I think DSO list with 4K entries and 200ms packet response time is not
going to ever become cheap enough.  Fixing all inferior hook methods
to be consistent and always load initial DSO list sounds promising,
but has the practical problem: I don't have access to many systems.
I'll check what I can do there ...

> Can you give a try on the attached patches and see how much
> a difference they make for you?

Thanks, I'll try them.

> Patch 2 has the most dramatic affect;

Yes, I've already tried equivalent patch (except I set the default
size to 128), and (as expected) it did make a big difference.

Going back to the new packet proposal to speed up target_read_string;
do you have any additional comments on

  qStr {addr},{maxlen},{terminator}

format?  Should {terminator} be optional? Should I avoid it altogether
(it doesn't solve any current problem, so perhaps it's over-design) ?

Thanks,
-- 
Paul Pluzhnikov

Re: [RFC][patch] Avoid repeated calls to solib_add on initial attach.

[Pedro Alves]

On Wednesday 20 July 2011 17:04:11, Paul Pluzhnikov wrote:
> On Wed, Jul 20, 2011 at 7:37 AM, Pedro Alves <pedro@codesourcery.com> wrote:
> > On Friday 15 July 2011 21:52:09, Paul Pluzhnikov wrote:
> >> Greetings,
> >>
> >> Following up on my earlier "slow on high-latency links" message:
> >> http://sourceware.org/ml/gdb-patches/2011-07/msg00391.html ...
> >>
> >> Attached patch avoids calling solib_add twice when initially attaching
> >> inferior.
> >>
> >> I am not entirely happy about this patch, but don't have a better idea
> >> for a fix, and do want to avoid repeated rescans of the shared library list.
> >
> > Me neigher, apart from doing as the comment says:
> >
> >>   /* Sometimes the platform-specific hook loads initial shared
> >>        libraries, and sometimes it doesn't.  If it doesn't FROM_TTY will be
> >>        incorrectly 0 but such solib targets should be fixed anyway.  If we
> >>        made all the inferior hook methods consistent, this call could be
> >>        removed.  Call it only after the solib target has been initialized by
> >>        solib_create_inferior_hook.  */
> >
> > Or, making retrieving the dso list cheap enough to not care about the
> > multiple calls.  By e.g., caching more things, and making
> > target_read_string not super dumb (as you've yourself proposed).
> 
> I think DSO list with 4K entries and 200ms packet response time is not
> going to ever become cheap enough. 

Yeah.  

> Fixing all inferior hook methods to be consistent and always load
> initial DSO list sounds promising, but has the practical problem: I don't
> have access to many systems.  I'll check what I can do there ...

I'm not certain of the amount of work here.  I'm okay with
your approach as fallback (you should make the counter
unsigned to avoid undefined signed overflow).

> 
> > Can you give a try on the attached patches and see how much
> > a difference they make for you?
> 
> Thanks, I'll try them.
> 
> > Patch 2 has the most dramatic affect;
> 
> Yes, I've already tried equivalent patch (except I set the default
> size to 128), and (as expected) it did make a big difference.
> 
> Going back to the new packet proposal to speed up target_read_string;
> do you have any additional comments on
> 
>   qStr {addr},{maxlen},{terminator}
> 
> format?  Should {terminator} be optional? Should I avoid it altogether
> (it doesn't solve any current problem, so perhaps it's over-design) ?

We considered a packet like this too, but decided that bumping
target_read_string first and seeing if anything breaks would be
better.  Haven't heard back of any breakage or slowdown so far.
I propose we do the same upstream.

-- 
Pedro Alves

Re: [RFC][patch] Avoid repeated calls to solib_add on initial attach.

[Paul Pluzhnikov]

On Wed, Jul 20, 2011 at 9:35 AM, Pedro Alves <pedro@codesourcery.com> wrote:

>>   qStr {addr},{maxlen},{terminator}
...
> We considered a packet like this too, but decided that bumping
> target_read_string first and seeing if anything breaks would be
> better.  Haven't heard back of any breakage or slowdown so far.
> I propose we do the same upstream.

But do you have any targets where reading individual words is slow?

I don't know about any such targets first-hand, but I've heard that
JTAG reading could be exceedingly slow.

Another possible alternative is to make this a run-time parameter:
'maintenance set string-read-size 64' or some such. Default to 64 and
let JTAG people dial it down if it ever becomes a problem for them?

Thanks for your comments.

-- 
Paul Pluzhnikov

Re: [RFC][patch] Avoid repeated calls to solib_add on initial attach.

[Pedro Alves]

On Wednesday 20 July 2011 17:44:43, Paul Pluzhnikov wrote:
> On Wed, Jul 20, 2011 at 9:35 AM, Pedro Alves <pedro@codesourcery.com> wrote:
> 
> >>   qStr {addr},{maxlen},{terminator}
> ...
> > We considered a packet like this too, but decided that bumping
> > target_read_string first and seeing if anything breaks would be
> > better.  Haven't heard back of any breakage or slowdown so far.
> > I propose we do the same upstream.
> 
> But do you have any targets where reading individual words is slow?

You mean slower than reading ~16 words?  I doubt there's any where
it matters over the latency saved for the current callers of
target_read_string.  Maybe it would matter if target_read_string
and read_string were merged.  I don't object to the new packet;
I'm merely suggesting avoiding work until proven necessary.  The
idea is off course sound.

> I don't know about any such targets first-hand, but I've heard that
> JTAG reading could be exceedingly slow.

We use a bunch of different probes, and so do our customers.  I
don't know offhand which are slow, but they're there for sure.

> Another possible alternative is to make this a run-time parameter:
> 'maintenance set string-read-size 64' or some such. Default to 64 and
> let JTAG people dial it down if it ever becomes a problem for them?

I don't envision this being necessary, but I've certainly been
wrong before...

-- 
Pedro Alves

Re: [RFC][patch] Avoid repeated calls to solib_add on initial attach.

[Pedro Alves]

On Wednesday 20 July 2011 17:35:12, Pedro Alves wrote:
> > Fixing all inferior hook methods to be consistent and always load
> > initial DSO list sounds promising, but has the practical problem: I don't
> > have access to many systems.  I'll check what I can do there ...
> 
> I'm not certain of the amount of work here.  I'm okay with
> your approach as fallback (you should make the counter
> unsigned to avoid undefined signed overflow).

(Meant to say before but forgot... the DSO list is per
program space, not inferior -- you should move the
counter to struct program_space)

-- 
Pedro Alves

Re: [RFC][patch] Avoid repeated calls to solib_add on initial attach.

[Paul Pluzhnikov]

[-- Attachment #1: Type: text/plain, Size: 593 bytes --]

On Wed, Jul 20, 2011 at 11:03 AM, Pedro Alves <pedro@codesourcery.com> wrote:

> (Meant to say before but forgot... the DSO list is per
> program space, not inferior -- you should move the
> counter to struct program_space)

Thanks for comments.

Revised patch attached. Tested on Linux/x86_64 with no regressions.


-- 
Paul Pluzhnikov


2011-07-22  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* progspace.h (struct program_space): Add solib_add_generation.
	* infcmd.c (post_create_inferior): Only call solib_add if not
	already done.
	* solib.c (solib_add): Increment solib_add_generation.

[-- Attachment #2: gdb-suppress-repeated-solib-add-20110722.txt --]
[-- Type: text/plain, Size: 3261 bytes --]

Index: progspace.h
===================================================================
RCS file: /cvs/src/src/gdb/progspace.h,v
retrieving revision 1.7
diff -u -p -r1.7 progspace.h
--- progspace.h	10 Jan 2011 20:38:50 -0000	1.7
+++ progspace.h	22 Jul 2011 17:03:07 -0000
@@ -185,6 +185,9 @@ struct program_space
        solib.c.  */
     struct so_list *so_list;
 
+    /* Number of calls to solib_add.  */
+    unsigned solib_add_generation;
+
     /* Per pspace data-pointers required by other GDB modules.  */
     void **data;
     unsigned num_data;
Index: infcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/infcmd.c,v
retrieving revision 1.287
diff -u -p -r1.287 infcmd.c
--- infcmd.c	30 May 2011 18:04:32 -0000	1.287
+++ infcmd.c	22 Jul 2011 17:03:07 -0000
@@ -421,6 +421,9 @@ post_create_inferior (struct target_ops 
 
   if (exec_bfd)
     {
+      const unsigned solib_add_generation
+	= current_program_space->solib_add_generation;
+
       /* Create the hooks to handle shared library load and unload
 	 events.  */
 #ifdef SOLIB_CREATE_INFERIOR_HOOK
@@ -428,24 +431,29 @@ post_create_inferior (struct target_ops 
 #else
       solib_create_inferior_hook (from_tty);
 #endif
-    }
-
-  /* If the solist is global across processes, there's no need to
-     refetch it here.  */
-  if (exec_bfd && !gdbarch_has_global_solist (target_gdbarch))
-    {
-      /* Sometimes the platform-specific hook loads initial shared
-	 libraries, and sometimes it doesn't.  If it doesn't FROM_TTY will be
-	 incorrectly 0 but such solib targets should be fixed anyway.  If we
-	 made all the inferior hook methods consistent, this call could be
-	 removed.  Call it only after the solib target has been initialized by
-	 solib_create_inferior_hook.  */
 
+      if (current_program_space->solib_add_generation == solib_add_generation)
+	{
+	  /* The platform-specific hook should load initial shared libraries,
+	     but didn't.  FROM_TTY will be incorrectly 0 but such solib
+	     targets should be fixed anyway.  Call it only after the solib
+	     target has been initialized by solib_create_inferior_hook.  */
+
+	  if (info_verbose)
+	    warning (_("platform-specific solib_create_inferior_hook did "
+		       "not load initial shared libraries."));
+
+	  /* If the solist is global across processes, there's no need to
+	     refetch it here.  */
+	  if (!gdbarch_has_global_solist (target_gdbarch))
+	    {
 #ifdef SOLIB_ADD
-      SOLIB_ADD (NULL, 0, target, auto_solib_add);
+	      SOLIB_ADD (NULL, 0, target, auto_solib_add);
 #else
-      solib_add (NULL, 0, target, auto_solib_add);
+	      solib_add (NULL, 0, target, auto_solib_add);
 #endif
+	    }
+	}
     }
 
   /* If the user sets watchpoints before execution having started,
Index: solib.c
===================================================================
RCS file: /cvs/src/src/gdb/solib.c,v
retrieving revision 1.149
diff -u -p -r1.149 solib.c
--- solib.c	30 Jun 2011 19:29:54 -0000	1.149
+++ solib.c	22 Jul 2011 17:03:07 -0000
@@ -914,6 +914,8 @@ solib_add (char *pattern, int from_tty,
 {
   struct so_list *gdb;
 
+  current_program_space->solib_add_generation++;
+
   if (pattern)
     {
       char *re_err = re_comp (pattern);

Re: [RFC][patch] Avoid repeated calls to solib_add on initial attach.

[Tom Tromey]

Paul> Going back to the new packet proposal to speed up target_read_string;
Paul> do you have any additional comments on
Paul>   qStr {addr},{maxlen},{terminator}
Paul> format?  Should {terminator} be optional? Should I avoid it altogether
Paul> (it doesn't solve any current problem, so perhaps it's over-design) ?

I haven't run across code in gdb that needs to read memory using a
delimiter other than \0.  So I would suggest leaving it out.

Tom

Re: [RFC][patch] Avoid repeated calls to solib_add on initial attach.

[Pedro Alves]

On Friday 22 July 2011 18:26:59, Tom Tromey wrote:
> Paul> Going back to the new packet proposal to speed up target_read_string;
> Paul> do you have any additional comments on
> Paul>   qStr {addr},{maxlen},{terminator}
> Paul> format?  Should {terminator} be optional? Should I avoid it altogether
> Paul> (it doesn't solve any current problem, so perhaps it's over-design) ?
> 
> I haven't run across code in gdb that needs to read memory using a
> delimiter other than \0.  So I would suggest leaving it out.

I think we should at least have a terminator width for wide chars.
If we're adding this, we should keep in mind valprint.c:read_string.

(It occured to be that we can emulate this packet
with two packets: qSearch:memory (target_search_memory),
to look for the terminator, like a remote strlen, and then a
normal memory read, though that would not be as efficient,
and certainly would be worse for small strings.)

-- 
Pedro Alves

Re: [RFC][patch] Avoid repeated calls to solib_add on initial attach.

[Pedro Alves]

On Friday 22 July 2011 18:18:06, Paul Pluzhnikov wrote:
> 2011-07-22  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
>         * progspace.h (struct program_space): Add solib_add_generation.
>         * infcmd.c (post_create_inferior): Only call solib_add if not
>         already done.
>         * solib.c (solib_add): Increment solib_add_generation.

Okay.

-- 
Pedro Alves