[patch] find_partial_die cleanups

[patch] find_partial_die cleanups

[Doug Evans]

Hi.

I have more radical changes queued up for Fission,
ref: http://gcc.gnu.org/wiki/DebugFission

but I want to check in some preliminary changes so that there's a
better base to review the patches from, plus make some general improvements
along the way.

This patch is standalone, and so I'm not submitting it as part of a set.
I will check this in in a few days if there are no objections.

Regression tested on amd64-linux, with this patch applied to exercise the
   if (pd == NULL && per_cu->load_all_dies == 0)
code path, ref: http://sourceware.org/bugzilla/show_bug.cgi?id=13961

diff -u -p -r1.628 dwarf2read.c
--- dwarf2read.c	7 Apr 2012 19:35:17 -0000	1.628
+++ dwarf2read.c	9 Apr 2012 16:01:55 -0000
@@ -9722,11 +9721,11 @@ load_partial_dies (bfd *abfd, gdb_byte *
 	 unit with load_all_dies set.  */
 
       if (load_all
-	  || abbrev->tag == DW_TAG_constant
+	  /*|| abbrev->tag == DW_TAG_constant
 	  || abbrev->tag == DW_TAG_subprogram
 	  || abbrev->tag == DW_TAG_variable
 	  || abbrev->tag == DW_TAG_namespace
-	  || part_die->is_declaration)
+	  || part_die->is_declaration*/)
 	{
 	  void **slot;
 


2012-04-09  Doug Evans  <dje@google.com>

	* dwarf2read.c (dwarf2_per_cu_data). Clarify comment.
	(load_partial_dies): Clarify comment.
	(find_partial_die): Support rereading type units.
	Clarify CU handling, if we know offset is in CU, don't search for the
	containing CU.  Add comment regarding memory waste.

Index: dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.628
diff -u -p -r1.628 dwarf2read.c
--- dwarf2read.c	7 Apr 2012 19:35:17 -0000	1.628
+++ dwarf2read.c	9 Apr 2012 16:08:58 -0000
@@ -415,9 +415,9 @@ struct dwarf2_per_cu_data
      any of the current compilation units are processed.  */
   unsigned int 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
+  /* This flag will be set when reading partial DIEs 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;
 
@@ -9516,7 +9516,6 @@ load_partial_dies (bfd *abfd, gdb_byte *
   struct abbrev_info *abbrev;
   unsigned int bytes_read;
   unsigned int load_all = 0;
-
   int nesting_level = 1;
 
   parent_die = NULL;
@@ -9579,7 +9578,7 @@ load_partial_dies (bfd *abfd, gdb_byte *
 	    }
 	}
 
-      /* We only recurse into subprograms looking for template arguments.
+      /* We only recurse into c++ subprograms looking for template arguments.
 	 Skip their other children.  */
       if (!load_all
 	  && cu->language == language_cplus
@@ -9996,28 +9995,33 @@ find_partial_die (sect_offset offset, st
   struct dwarf2_per_cu_data *per_cu = NULL;
   struct partial_die_info *pd = NULL;
 
-  if (cu->per_cu->debug_types_section)
-    {
-      pd = find_partial_die_in_comp_unit (offset, cu);
-      if (pd != NULL)
-	return pd;
-      goto not_found;
-    }
-
   if (offset_in_cu_p (&cu->header, offset))
     {
       pd = find_partial_die_in_comp_unit (offset, cu);
       if (pd != NULL)
 	return pd;
+      /* We missed recording what we needed.
+	 Load all dies and try again.  */
+      per_cu = cu->per_cu;
     }
+  else
+    {
+      /* TUs don't reference other CUs/TUs (except via type signatures).  */
+      if (cu->per_cu->debug_types_section)
+	{
+	  error (_("Dwarf Error: Type Unit at offset 0x%lx contains"
+		   " external reference to offset 0x%lx [in module %s].\n"),
+		 (long) cu->header.offset.sect_off, (long) offset.sect_off,
+		 bfd_get_filename (objfile->obfd));
+	}
+      per_cu = dwarf2_find_containing_comp_unit (offset, objfile);
 
-  per_cu = dwarf2_find_containing_comp_unit (offset, objfile);
-
-  if (per_cu->cu == NULL || per_cu->cu->partial_dies == NULL)
-    load_partial_comp_unit (per_cu);
+      if (per_cu->cu == NULL || per_cu->cu->partial_dies == NULL)
+	load_partial_comp_unit (per_cu);
 
-  per_cu->cu->last_used = 0;
-  pd = find_partial_die_in_comp_unit (offset, per_cu->cu);
+      per_cu->cu->last_used = 0;
+      pd = find_partial_die_in_comp_unit (offset, per_cu->cu);
+    }
 
   if (pd == NULL && per_cu->load_all_dies == 0)
     {
@@ -10026,28 +10030,34 @@ find_partial_die (sect_offset offset, st
       struct abbrev_info *abbrev;
       unsigned int bytes_read;
       char *info_ptr;
+      struct dwarf2_section_info* sec;
 
       per_cu->load_all_dies = 1;
 
-      /* Re-read the DIEs.  */
+      if (per_cu->debug_types_section)
+	sec = per_cu->debug_types_section;
+      else
+	sec = &dwarf2_per_objfile->info;
+
+      /* Re-read the DIEs, this time reading all of them.
+	 NOTE: We don't discard the previous set of DIEs.
+	 This doesn't happen very often so it's (hopefully) not a problem.  */
       back_to = make_cleanup (null_cleanup, 0);
       if (per_cu->cu->dwarf2_abbrevs == NULL)
 	{
 	  dwarf2_read_abbrevs (per_cu->cu);
 	  make_cleanup (dwarf2_free_abbrev_table, per_cu->cu);
 	}
-      info_ptr = (dwarf2_per_objfile->info.buffer
+      info_ptr = (sec->buffer
 		  + per_cu->cu->header.offset.sect_off
 		  + per_cu->cu->header.first_die_offset.cu_off);
       abbrev = peek_die_abbrev (info_ptr, &bytes_read, per_cu->cu);
       info_ptr = read_partial_die (&comp_unit_die, abbrev, bytes_read,
-				   objfile->obfd,
-				   dwarf2_per_objfile->info.buffer, info_ptr,
+				   objfile->obfd, sec->buffer, info_ptr,
 				   per_cu->cu);
       if (comp_unit_die.has_children)
-	load_partial_dies (objfile->obfd,
-			   dwarf2_per_objfile->info.buffer, info_ptr,
-			   0, per_cu->cu);
+	load_partial_dies (objfile->obfd, sec->buffer, info_ptr, 0,
+			   per_cu->cu);
       do_cleanups (back_to);
 
       pd = find_partial_die_in_comp_unit (offset, per_cu->cu);

Re: [patch] find_partial_die cleanups

[Tom Tromey]

>>>>> "Doug" == Doug Evans <dje@google.com> writes:

Doug> but I want to check in some preliminary changes so that there's a
Doug> better base to review the patches from, plus make some general
Doug> improvements along the way.

Sounds great.

Doug> This patch is standalone, and so I'm not submitting it as part of a set.
Doug> I will check this in in a few days if there are no objections.

No objections here, just a question and a nit.

Doug> +	  error (_("Dwarf Error: Type Unit at offset 0x%lx contains"
Doug> +		   " external reference to offset 0x%lx [in module %s].\n"),
Doug> +		 (long) cu->header.offset.sect_off, (long) offset.sect_off,
Doug> +		 bfd_get_filename (objfile->obfd));

Why an error and not a complaint?

I ask because errors have a pretty drastic effect on debuginfo reading.

Doug> +      struct dwarf2_section_info* sec;
 
Wrong "*" placement.

Tom

Re: [patch] find_partial_die cleanups

[Doug Evans]

On Mon, Apr 9, 2012 at 11:16 AM, Tom Tromey <tromey@redhat.com> wrote:
> Doug> +   error (_("Dwarf Error: Type Unit at offset 0x%lx contains"
> Doug> +            " external reference to offset 0x%lx [in module %s].\n"),
> Doug> +          (long) cu->header.offset.sect_off, (long) offset.sect_off,
> Doug> +          bfd_get_filename (objfile->obfd));
>
> Why an error and not a complaint?
>
> I ask because errors have a pretty drastic effect on debuginfo reading.

"Right on queue." :-)
Indeed.  And I would have used a complaint but the callers aren't
expecting a NULL return and since there is ample precedent, I went
with error.
I'm happy to go throught all the callers and bubble up the
consequences, but my first pass at this gave me pause.

> Doug> +      struct dwarf2_section_info* sec;
>
> Wrong "*" placement.

Heh, righto.

Re: [patch] find_partial_die cleanups

[Tom Tromey]

Doug> And I would have used a complaint but the callers aren't
Doug> expecting a NULL return and since there is ample precedent, I went
Doug> with error.

Existing precedent is good enough for me.

Tom

Re: [patch] find_partial_die cleanups

[Doug Evans]

On Mon, Apr 9, 2012 at 11:48 AM, Tom Tromey <tromey@redhat.com> wrote:
> Doug> And I would have used a complaint but the callers aren't
> Doug> expecting a NULL return and since there is ample precedent, I went
> Doug> with error.
>
> Existing precedent is good enough for me.

Thanks, committed.