Re: [rfa] Make the partial DIE cache more robust

Re: [rfa] Make the partial DIE cache more robust

[Jim Blandy]

This patch:

http://sourceware.org/ml/gdb-patches/2005-04/msg00066.html

is approved, if it still applies.

Re: [rfa] Make the partial DIE cache more robust

[Daniel Jacobowitz]

On Tue, Feb 07, 2006 at 03:11:45PM -0800, Jim Blandy wrote:
> This patch:
> 
> http://sourceware.org/ml/gdb-patches/2005-04/msg00066.html
> 
> is approved, if it still applies.

Thanks.  It does, and I've just committed it after retesting
on x86_64-pc-linux-gnu.

I believe I've just suckered Paul into updating the rest of the patch
series this originally came from, which was written to support
using GDB, and running the GDB testsuite, with ARM's proprietary
compilers.  With a bit of luck, we'll be submitting more of those
soon :-)

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfa] Make the partial DIE cache more robust

[Joel Brobecker]

Daniel,

> > (I know I can't approve patches, but it looks great). If it helps, I can
> > see if I can merge it into 6.3 and give it a swirl with our Ada compiler
> > and our Ada examples, where this will likely be useful.
> 
> Please do - this should obsolete the changes you told me about a couple
> of weeks ago.

FWIW, I finally found some time to test this change with our Ada
compiler, and it works great. It indeed obsoletes the changes you
refered to.

-- 
Joel

Re: [rfa] Make the partial DIE cache more robust

[Daniel Jacobowitz]

On Thu, Apr 07, 2005 at 10:30:00AM -0400, Daniel Jacobowitz wrote:
> There have been several GDB and GCC bug reports about the partial DIE cache. 
> GDB only saves DIEs of the types that it expects to find
> specification/abstract-origin references to; when GCC emits an unexpected
> reference, GDB is stuck.
> 
> This patch gets GDB unstuck.  We still load only the DIEs we expect to need;
> and this patch includes a change similar to Manoj's, to expand the set of
> "expected" DIEs to include DW_TAG_member.  However, if we try to load a DIE
> and can not find it in cache, but we can find its CU, we will now go back
> and reload the compilation unit without any heuristics.
> 
> An example of this is any file-scope reference to a function-local DIE;
> normally we don't walk into functions when loading partial DIEs, because
> it's only necessary in rare cases.  For instance the ARM RVCT compiler will
> emit out-of-line definitions of methods for function-scope classes.  The
> class definition and member function declarations are inside the function,
> but the member function definitions are outside (in fact, they're in a
> different DW_TAG_compile_unit).
> 
> Another example is GCC PR debug/20805.  Whether or not the debug information
> in that PR is formally correct, reducing the ways in which GDB can choke on
> invalid input would be a good thing.
> 
> Is this patch OK?

Hi Elena,

Have you had a chance to review this patch?

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [rfa] Make the partial DIE cache more robust

[Daniel Jacobowitz]

On Fri, Apr 08, 2005 at 11:48:39AM -0700, Joel Brobecker wrote:
> > 2005-04-07  Daniel Jacobowitz  <dan@codesourcery.com>
> > 
> > 	* dwarf2read.c (struct dwarf2_per_cu_data): Reduce length to
> > 	30 bits.  Add load_all_dies flag.
> > 	(load_partial_dies): Load all DIEs if per_cu->load_all_dies is set.
> > 	Load DW_TAG_member by default.  Remove internal_error call.
> > 	(find_partial_die): Reload the compilation unit if we can not find
> > 	a DIE in the cache.  Call internal_error here if we still can not
> > 	find the DIE.
> 
> (I know I can't approve patches, but it looks great). If it helps, I can
> see if I can merge it into 6.3 and give it a swirl with our Ada compiler
> and our Ada examples, where this will likely be useful.

Please do - this should obsolete the changes you told me about a couple
of weeks ago.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [rfa] Make the partial DIE cache more robust

[Joel Brobecker]

> 2005-04-07  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* dwarf2read.c (struct dwarf2_per_cu_data): Reduce length to
> 	30 bits.  Add load_all_dies flag.
> 	(load_partial_dies): Load all DIEs if per_cu->load_all_dies is set.
> 	Load DW_TAG_member by default.  Remove internal_error call.
> 	(find_partial_die): Reload the compilation unit if we can not find
> 	a DIE in the cache.  Call internal_error here if we still can not
> 	find the DIE.

(I know I can't approve patches, but it looks great). If it helps, I can
see if I can merge it into 6.3 and give it a swirl with our Ada compiler
and our Ada examples, where this will likely be useful.

-- 
Joel

[rfa] Make the partial DIE cache more robust

[Daniel Jacobowitz]

There have been several GDB and GCC bug reports about the partial DIE cache. 
GDB only saves DIEs of the types that it expects to find
specification/abstract-origin references to; when GCC emits an unexpected
reference, GDB is stuck.

This patch gets GDB unstuck.  We still load only the DIEs we expect to need;
and this patch includes a change similar to Manoj's, to expand the set of
"expected" DIEs to include DW_TAG_member.  However, if we try to load a DIE
and can not find it in cache, but we can find its CU, we will now go back
and reload the compilation unit without any heuristics.

An example of this is any file-scope reference to a function-local DIE;
normally we don't walk into functions when loading partial DIEs, because
it's only necessary in rare cases.  For instance the ARM RVCT compiler will
emit out-of-line definitions of methods for function-scope classes.  The
class definition and member function declarations are inside the function,
but the member function definitions are outside (in fact, they're in a
different DW_TAG_compile_unit).

Another example is GCC PR debug/20805.  Whether or not the debug information
in that PR is formally correct, reducing the ways in which GDB can choke on
invalid input would be a good thing.

Is this patch OK?

-- 
Daniel Jacobowitz
CodeSourcery, LLC

2005-04-07  Daniel Jacobowitz  <dan@codesourcery.com>

	* dwarf2read.c (struct dwarf2_per_cu_data): Reduce length to
	30 bits.  Add load_all_dies flag.
	(load_partial_dies): Load all DIEs if per_cu->load_all_dies is set.
	Load DW_TAG_member by default.  Remove internal_error call.
	(find_partial_die): Reload the compilation unit if we can not find
	a DIE in the cache.  Call internal_error here if we still can not
	find the DIE.

Index: dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.181
diff -u -p -r1.181 dwarf2read.c
--- dwarf2read.c	9 Mar 2005 06:03:14 -0000	1.181
+++ dwarf2read.c	7 Apr 2005 14:14:30 -0000
@@ -360,16 +360,22 @@ struct dwarf2_cu
 
 struct dwarf2_per_cu_data
 {
-  /* The start offset and length of this compilation unit.  2**31-1
+  /* The start offset and length of this compilation unit.  2**30-1
      bytes should suffice to store the length of any compilation unit
      - if it doesn't, GDB will fall over anyway.  */
   unsigned long offset;
-  unsigned long length : 31;
+  unsigned long length : 30;
 
   /* Flag indicating this compilation unit will be read in before
      any of the current compilation units are processed.  */
   unsigned long queued : 1;
 
+  /* This flag will be set if we need to load absolutely all DIEs
+     for this compilation unit, instead of just the ones we think
+     are interesting.  It gets set if we look for a DIE in the
+     hash table and don't find it.  */
+  unsigned int load_all_dies : 1;
+
   /* Set iff currently read in.  */
   struct dwarf2_cu *cu;
 
@@ -5131,12 +5137,16 @@ load_partial_dies (bfd *abfd, char *info
   struct partial_die_info *parent_die, *last_die, *first_die = NULL;
   struct abbrev_info *abbrev;
   unsigned int bytes_read;
+  unsigned int load_all = 0;
 
   int nesting_level = 1;
 
   parent_die = NULL;
   last_die = NULL;
 
+  if (cu->per_cu && cu->per_cu->load_all_dies)
+    load_all = 1;
+
   cu->partial_dies
     = htab_create_alloc_ex (cu->header.length / 12,
 			    partial_die_hash,
@@ -5172,12 +5182,17 @@ load_partial_dies (bfd *abfd, char *info
 	  continue;
 	}
 
-      /* Check whether this DIE is interesting enough to save.  */
-      if (!is_type_tag_for_partial (abbrev->tag)
+      /* Check whether this DIE is interesting enough to save.  Normally
+	 we would not be interested in members here, but there may be
+	 later variables referencing them via DW_AT_specification (for
+	 static members).  */
+      if (!load_all
+	  && !is_type_tag_for_partial (abbrev->tag)
 	  && abbrev->tag != DW_TAG_enumerator
 	  && abbrev->tag != DW_TAG_subprogram
 	  && abbrev->tag != DW_TAG_variable
-	  && abbrev->tag != DW_TAG_namespace)
+	  && abbrev->tag != DW_TAG_namespace
+	  && abbrev->tag != DW_TAG_member)
 	{
 	  /* Otherwise we skip to the next sibling, if any.  */
 	  info_ptr = skip_one_die (info_ptr + bytes_read, abbrev, cu);
@@ -5277,9 +5292,11 @@ load_partial_dies (bfd *abfd, char *info
 
 	 Adding more things than necessary to the hash table is harmless
 	 except for the performance cost.  Adding too few will result in
-	 internal errors in find_partial_die.  */
+	 wasted time in find_partial_die, when we reread the compilation
+	 unit with load_all_dies set.  */
 
-      if (abbrev->tag == DW_TAG_subprogram
+      if (load_all
+	  || abbrev->tag == DW_TAG_subprogram
 	  || abbrev->tag == DW_TAG_variable
 	  || abbrev->tag == DW_TAG_namespace
 	  || part_die->is_declaration)
@@ -5299,7 +5316,8 @@ load_partial_dies (bfd *abfd, char *info
 	 languages we have to, both so that we can get at method physnames
 	 to infer fully qualified class names, and for DW_AT_specification.  */
       if (last_die->has_children
-	  && (last_die->tag == DW_TAG_namespace
+	  && (load_all
+	      || last_die->tag == DW_TAG_namespace
 	      || last_die->tag == DW_TAG_enumeration_type
 	      || (cu->language != language_c
 		  && (last_die->tag == DW_TAG_class_type
@@ -5451,10 +5469,6 @@ find_partial_die_in_comp_unit (unsigned 
   part_die.offset = offset;
   lookup_die = htab_find_with_hash (cu->partial_dies, &part_die, offset);
 
-  if (lookup_die == NULL)
-    internal_error (__FILE__, __LINE__,
-		    _("could not find partial DIE in cache\n"));
-
   return lookup_die;
 }
 
@@ -5463,11 +5477,16 @@ find_partial_die_in_comp_unit (unsigned 
 static struct partial_die_info *
 find_partial_die (unsigned long offset, struct dwarf2_cu *cu)
 {
-  struct dwarf2_per_cu_data *per_cu;
+  struct dwarf2_per_cu_data *per_cu = NULL;
+  struct partial_die_info *pd = NULL;
 
   if (offset >= cu->header.offset
       && offset < cu->header.offset + cu->header.length)
-    return find_partial_die_in_comp_unit (offset, cu);
+    {
+      pd = find_partial_die_in_comp_unit (offset, cu);
+      if (pd != NULL)
+	return pd;
+    }
 
   per_cu = dwarf2_find_containing_comp_unit (offset, cu->objfile);
 
@@ -5479,7 +5498,42 @@ find_partial_die (unsigned long offset, 
     }
 
   per_cu->cu->last_used = 0;
-  return find_partial_die_in_comp_unit (offset, per_cu->cu);
+  pd = find_partial_die_in_comp_unit (offset, per_cu->cu);
+
+  if (pd == NULL && per_cu->load_all_dies == 0)
+    {
+      struct cleanup *back_to;
+      struct partial_die_info comp_unit_die;
+      struct abbrev_info *abbrev;
+      unsigned int bytes_read;
+      char *info_ptr;
+
+      per_cu->load_all_dies = 1;
+
+      /* Re-read the DIEs.  */
+      back_to = make_cleanup (null_cleanup, 0);
+      if (per_cu->cu->dwarf2_abbrevs == NULL)
+	{
+	  dwarf2_read_abbrevs (per_cu->cu->objfile->obfd, per_cu->cu);
+	  back_to = make_cleanup (dwarf2_free_abbrev_table, per_cu->cu);
+	}
+      info_ptr = per_cu->cu->header.first_die_ptr;
+      abbrev = peek_die_abbrev (info_ptr, &bytes_read, per_cu->cu);
+      info_ptr = read_partial_die (&comp_unit_die, abbrev, bytes_read,
+				   per_cu->cu->objfile->obfd, info_ptr,
+				   per_cu->cu);
+      if (comp_unit_die.has_children)
+	load_partial_dies (per_cu->cu->objfile->obfd, info_ptr, 0, per_cu->cu);
+      do_cleanups (back_to);
+
+      pd = find_partial_die_in_comp_unit (offset, per_cu->cu);
+    }
+
+  if (pd == NULL)
+    internal_error (__FILE__, __LINE__,
+		    _("could not find partial DIE 0x%lx in cache [from module %s]\n"),
+		    offset, bfd_get_filename (cu->objfile->obfd));
+  return pd;
 }
 
 /* Adjust PART_DIE before generating a symbol for it.  This function