Re: [PATCH] Cache program headers

[Abhijit Halder <abhijit dot k dot halder at gmail dot com>]

On Fri, Aug 19, 2011 at 10:05 AM, Luis Machado
<lgustavo@codesourcery.com> wrote:
> Hi,
>
> This is also another attempt to speed up debugging. This patch caches the
> program headers so that further reads are cheap.
>
> Luis
>

2011-08-19  Pedro Alves  <pedro@codesourcery.com>
	    Luis Machado  <lgustavo@codesourcery.com>

	* 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.

Index: gdb/solib-svr4.c
===================================================================
--- gdb/solib-svr4.c.orig	2011-08-18 12:13:58.753749146 -0300
+++ gdb/solib-svr4.c	2011-08-19 11:21:45.989749146 -0300
@@ -449,6 +449,99 @@
   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.
@@ -470,6 +563,31 @@
   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)
I think the above statement should be written like this:
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)
As said earlier:
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;
@@ -560,6 +678,22 @@
   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;
 }

@@ -744,12 +878,10 @@
 	if (ptr)
 	  *ptr = dyn_ptr;

-	xfree (bufstart);
 	return 1;
       }
   }

-  xfree (bufstart);
   return 0;
 }

@@ -1943,7 +2075,6 @@
 	    ok = 0;
 	}

-      xfree (buf);
       xfree (buf2);

       if (!ok)
@@ -2346,4 +2477,12 @@
   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);
 }