[RFA/dwarf] Don't process types multiple times

[RFA/dwarf] Don't process types multiple times

[Daniel Jacobowitz]

This independent patch is a performance and correctness fix for full symbol
processing.  My description from the intercu branch posting:

 There are two ways that we can process a general DIE: process_die or
 read_type_die.  Children of particular DIE types may be processed directly,
 but these are the only major dispatch points.  It's interesting to notice
 that almost everything called from read_type_die starts with "if (die->type)
 return": everything but enumeration types and aggregate types, in fact.
 This means that if the first reference to an enumeration or aggregate type
 is a DW_AT_type or DW_AT_containing_type in a DIE numerically before the
 type's DIE, we'll end up calling new_symbol for it twice.

 Fixing this saves about 8% memory and 4% time from gdb -readnow libc.so.6,
 a lot of duplicate entries on maint print symbols which I vaguely recall
 being confused about but never investigated, and some serious problems for
 inter-CU support.  Without this, types could be added to the wrong symbol
 table.

Tested i686-pc-linux-gnu, no regressions.  OK to commit?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2004-02-24  Daniel Jacobowitz  <drow@mvista.com>

	* dwarf2read.c (read_structure_scope): Don't create a symbol
	or call process_die.  Return immediately if die->type is set.
	Call read_type_die before dwarf2_add_member_fn.
	(process_structure_scope): New function.
	(read_enumeration_type, read_enumeration_scope): New functions,
	broken out from read_enumeration.  Don't create the enumeration
	type if it has already been created.
	(read_enumeration): Removed.
	(process_die): Call process_structure_scope, read_enumeration_type,
	and read_enumeration_scope.  Just call new_symbol for base and
	subrange types.  Add a comment about other type dies.
	(read_type_die): Call read_enumeration_type.

Index: dwarf2read.c
===================================================================
RCS file: /big/fsf/rsync/src-cvs/src/gdb/dwarf2read.c,v
retrieving revision 1.135
diff -u -p -r1.135 dwarf2read.c
--- dwarf2read.c	21 Feb 2004 02:13:35 -0000	1.135
+++ dwarf2read.c	25 Feb 2004 04:29:12 -0000
@@ -806,6 +806,8 @@ static void dwarf2_attach_fn_fields_to_t
 
 static void read_structure_scope (struct die_info *, struct dwarf2_cu *);
 
+static void process_structure_scope (struct die_info *, struct dwarf2_cu *);
+
 static void read_common_block (struct die_info *, struct dwarf2_cu *);
 
 static void read_namespace (struct die_info *die, struct dwarf2_cu *);
@@ -813,7 +815,9 @@ static void read_namespace (struct die_i
 static const char *namespace_name (struct die_info *die,
 				   int *is_anonymous, struct dwarf2_cu *);
 
-static void read_enumeration (struct die_info *, struct dwarf2_cu *);
+static void read_enumeration_type (struct die_info *, struct dwarf2_cu *);
+
+static void read_enumeration_scope (struct die_info *, struct dwarf2_cu *);
 
 static struct type *dwarf_base_type (int, int, struct dwarf2_cu *);
 
@@ -1950,10 +1954,16 @@ process_die (struct die_info *die, struc
     case DW_TAG_structure_type:
     case DW_TAG_union_type:
       read_structure_scope (die, cu);
+      process_structure_scope (die, cu);
       break;
     case DW_TAG_enumeration_type:
-      read_enumeration (die, cu);
+      read_enumeration_type (die, cu);
+      read_enumeration_scope (die, cu);
       break;
+
+    /* FIXME: These initialize die->type, but do not create a symbol
+       or process any children.  Therefore it doesn't do anything that
+       won't be done on-demand by read_type_die.  */
     case DW_TAG_subroutine_type:
       read_subroutine_type (die, cu);
       break;
@@ -1972,21 +1982,19 @@ process_die (struct die_info *die, struc
     case DW_TAG_string_type:
       read_tag_string_type (die, cu);
       break;
+    /* END FIXME */
+
     case DW_TAG_base_type:
       read_base_type (die, cu);
-      if (dwarf2_attr (die, DW_AT_name, cu))
-	{
-	  /* Add a typedef symbol for the base type definition.  */
-	  new_symbol (die, die->type, cu);
-	}
+      /* Add a typedef symbol for the type definition, if it has a
+	 DW_AT_name.  */
+      new_symbol (die, die->type, cu);
       break;
     case DW_TAG_subrange_type:
       read_subrange_type (die, cu);
-      if (dwarf2_attr (die, DW_AT_name, cu))
-       {
-         /* Add a typedef symbol for the base type definition.  */
-         new_symbol (die, die->type, cu);
-       }
+      /* Add a typedef symbol for the type definition, if it has a
+         DW_AT_name.  */
+      new_symbol (die, die->type, cu);
       break;
     case DW_TAG_common_block:
       read_common_block (die, cu);
@@ -3008,6 +3016,9 @@ read_structure_scope (struct die_info *d
      any.  */
   int need_to_update_name = 0;
 
+  if (die->type)
+    return;
+
   type = alloc_type (objfile);
 
   INIT_CPLUS_SPECIFIC (type);
@@ -3106,7 +3117,7 @@ read_structure_scope (struct die_info *d
 	  else if (child_die->tag == DW_TAG_subprogram)
 	    {
 	      /* C++ member function. */
-	      process_die (child_die, cu);
+	      read_type_die (child_die, cu);
 	      dwarf2_add_member_fn (&fi, child_die, type, cu);
 	      if (need_to_update_name)
 		{
@@ -3149,10 +3160,6 @@ read_structure_scope (struct die_info *d
 	      /* C++ base class field.  */
 	      dwarf2_add_field (&fi, child_die, cu);
 	    }
-	  else
-	    {
-	      process_die (child_die, cu);
-	    }
 	  child_die = sibling_die (child_die);
 	}
 
@@ -3209,8 +3216,6 @@ read_structure_scope (struct die_info *d
 	    }
 	}
 
-      new_symbol (die, type, cu);
-
       do_cleanups (back_to);
     }
   else
@@ -3224,26 +3229,53 @@ read_structure_scope (struct die_info *d
     do_cleanups (back_to);
 }
 
-/* Given a pointer to a die which begins an enumeration, process all
-   the dies that define the members of the enumeration.
+static void
+process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
+{
+  struct objfile *objfile = cu->objfile;
+  const char *previous_prefix = processing_current_prefix;
 
-   This will be much nicer in draft 6 of the DWARF spec when our
-   members will be dies instead squished into the DW_AT_element_list
-   attribute.
+  if (TYPE_TAG_NAME (die->type) != NULL)
+    processing_current_prefix = TYPE_TAG_NAME (die->type);
 
-   NOTE: We reverse the order of the element list.  */
+  if (die->child != NULL && ! die_is_declaration (die, cu))
+    {
+      struct die_info *child_die;
+
+      child_die = die->child;
+
+      while (child_die && child_die->tag)
+	{
+	  if (child_die->tag == DW_TAG_member
+	      || child_die->tag == DW_TAG_variable
+	      || child_die->tag == DW_TAG_inheritance)
+	    {
+	      /* Do nothing.  */
+	    }
+	  else
+	    process_die (child_die, cu);
+
+	  child_die = sibling_die (child_die);
+	}
+
+      new_symbol (die, die->type, cu);
+    }
+
+  processing_current_prefix = previous_prefix;
+}
+
+/* Given a DW_AT_enumeration_type die, set its type.  We do not
+   complete the type's fields yet, or create any symbols.  */
 
 static void
-read_enumeration (struct die_info *die, struct dwarf2_cu *cu)
+read_enumeration_type (struct die_info *die, struct dwarf2_cu *cu)
 {
   struct objfile *objfile = cu->objfile;
-  struct die_info *child_die;
   struct type *type;
-  struct field *fields;
   struct attribute *attr;
-  struct symbol *sym;
-  int num_fields;
-  int unsigned_enum = 1;
+
+  if (die->type)
+    return;
 
   type = alloc_type (objfile);
 
@@ -3278,6 +3310,26 @@ read_enumeration (struct die_info *die, 
       TYPE_LENGTH (type) = 0;
     }
 
+  die->type = type;
+}
+
+/* Given a pointer to a die which begins an enumeration, process all
+   the dies that define the members of the enumeration, and create the
+   symbol for the enumeration type.
+
+   NOTE: We reverse the order of the element list.  */
+
+static void
+read_enumeration_scope (struct die_info *die, struct dwarf2_cu *cu)
+{
+  struct objfile *objfile = cu->objfile;
+  struct die_info *child_die;
+  struct field *fields;
+  struct attribute *attr;
+  struct symbol *sym;
+  int num_fields;
+  int unsigned_enum = 1;
+
   num_fields = 0;
   fields = NULL;
   if (die->child != NULL)
@@ -3294,7 +3346,7 @@ read_enumeration (struct die_info *die, 
 	      attr = dwarf2_attr (child_die, DW_AT_name, cu);
 	      if (attr)
 		{
-		  sym = new_symbol (child_die, type, cu);
+		  sym = new_symbol (child_die, die->type, cu);
 		  if (SYMBOL_VALUE (sym) < 0)
 		    unsigned_enum = 0;
 
@@ -3321,18 +3373,18 @@ read_enumeration (struct die_info *die, 
 
       if (num_fields)
 	{
-	  TYPE_NFIELDS (type) = num_fields;
-	  TYPE_FIELDS (type) = (struct field *)
-	    TYPE_ALLOC (type, sizeof (struct field) * num_fields);
-	  memcpy (TYPE_FIELDS (type), fields,
+	  TYPE_NFIELDS (die->type) = num_fields;
+	  TYPE_FIELDS (die->type) = (struct field *)
+	    TYPE_ALLOC (die->type, sizeof (struct field) * num_fields);
+	  memcpy (TYPE_FIELDS (die->type), fields,
 		  sizeof (struct field) * num_fields);
 	  xfree (fields);
 	}
       if (unsigned_enum)
-	TYPE_FLAGS (type) |= TYPE_FLAG_UNSIGNED;
+	TYPE_FLAGS (die->type) |= TYPE_FLAG_UNSIGNED;
     }
-  die->type = type;
-  new_symbol (die, type, cu);
+
+  new_symbol (die, die->type, cu);
 }
 
 /* Extract all information from a DW_TAG_array_type DIE and put it in
@@ -6012,7 +6064,7 @@ read_type_die (struct die_info *die, str
       read_structure_scope (die, cu);
       break;
     case DW_TAG_enumeration_type:
-      read_enumeration (die, cu);
+      read_enumeration_type (die, cu);
       break;
     case DW_TAG_subprogram:
     case DW_TAG_subroutine_type:

Re: [RFA/dwarf] Don't process types multiple times

[David Carlton]

On Tue, 24 Feb 2004 23:42:06 -0500, Daniel Jacobowitz <drow@false.org> said:

> There are two ways that we can process a general DIE: process_die or
> read_type_die.  Children of particular DIE types may be processed
> directly, but these are the only major dispatch points.  It's
> interesting to notice that almost everything called from
> read_type_die starts with "if (die->type) return": everything but
> enumeration types and aggregate types, in fact.  This means that if
> the first reference to an enumeration or aggregate type is a
> DW_AT_type or DW_AT_containing_type in a DIE numerically before the
> type's DIE, we'll end up calling new_symbol for it twice.

Yeah, that had been bothering me for a while, too.

In the FIXME comment, could you add your name and the date?

David Carlton
carlton@kealia.com

Re: [RFA/dwarf] Don't process types multiple times

[Daniel Jacobowitz]

On Tue, Feb 24, 2004 at 11:42:06PM -0500, Daniel Jacobowitz wrote:
> This independent patch is a performance and correctness fix for full symbol
> processing.  My description from the intercu branch posting:
> 
>  There are two ways that we can process a general DIE: process_die or
>  read_type_die.  Children of particular DIE types may be processed directly,
>  but these are the only major dispatch points.  It's interesting to notice
>  that almost everything called from read_type_die starts with "if (die->type)
>  return": everything but enumeration types and aggregate types, in fact.
>  This means that if the first reference to an enumeration or aggregate type
>  is a DW_AT_type or DW_AT_containing_type in a DIE numerically before the
>  type's DIE, we'll end up calling new_symbol for it twice.
> 
>  Fixing this saves about 8% memory and 4% time from gdb -readnow libc.so.6,
>  a lot of duplicate entries on maint print symbols which I vaguely recall
>  being confused about but never investigated, and some serious problems for
>  inter-CU support.  Without this, types could be added to the wrong symbol
>  table.
> 
> Tested i686-pc-linux-gnu, no regressions.  OK to commit?

Ping.  I would like to apply this bug fix for GDB 6.1.


-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA/dwarf] Don't process types multiple times

[Daniel Jacobowitz]

On Tue, Feb 24, 2004 at 11:42:06PM -0500, Daniel Jacobowitz wrote:
> This independent patch is a performance and correctness fix for full symbol
> processing.  My description from the intercu branch posting:
> 
>  There are two ways that we can process a general DIE: process_die or
>  read_type_die.  Children of particular DIE types may be processed directly,
>  but these are the only major dispatch points.  It's interesting to notice
>  that almost everything called from read_type_die starts with "if (die->type)
>  return": everything but enumeration types and aggregate types, in fact.
>  This means that if the first reference to an enumeration or aggregate type
>  is a DW_AT_type or DW_AT_containing_type in a DIE numerically before the
>  type's DIE, we'll end up calling new_symbol for it twice.
> 
>  Fixing this saves about 8% memory and 4% time from gdb -readnow libc.so.6,
>  a lot of duplicate entries on maint print symbols which I vaguely recall
>  being confused about but never investigated, and some serious problems for
>  inter-CU support.  Without this, types could be added to the wrong symbol
>  table.
> 
> Tested i686-pc-linux-gnu, no regressions.  OK to commit?

Ping.  I would like to apply this bug fix for GDB 6.1.


-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA/dwarf] Don't process types multiple times

[Jim Blandy]

Daniel Jacobowitz <drow@false.org> writes:
> This independent patch is a performance and correctness fix for full symbol
> processing.  My description from the intercu branch posting:
> 
>  There are two ways that we can process a general DIE: process_die or
>  read_type_die.  Children of particular DIE types may be processed directly,
>  but these are the only major dispatch points.  It's interesting to notice
>  that almost everything called from read_type_die starts with "if (die->type)
>  return": everything but enumeration types and aggregate types, in fact.
>  This means that if the first reference to an enumeration or aggregate type
>  is a DW_AT_type or DW_AT_containing_type in a DIE numerically before the
>  type's DIE, we'll end up calling new_symbol for it twice.
> 
>  Fixing this saves about 8% memory and 4% time from gdb -readnow libc.so.6,
>  a lot of duplicate entries on maint print symbols which I vaguely recall
>  being confused about but never investigated, and some serious problems for
>  inter-CU support.  Without this, types could be added to the wrong symbol
>  table.
> 
> Tested i686-pc-linux-gnu, no regressions.  OK to commit?

So the essential idea here is to make sure that we only create symbols
when we reach them via the main traversal, not when we happen to reach
them by following references with die_type.  The current code relies
on the existing read_structure_scope and read_enumeration functions to
both read the type and create the symbol, so it can't distinguish
between the two cases.  Your patch creates a separate function for
each role, and ensures that the symbol creation functions are only
called from the main traversal.

Is that right?  If so, please commit, to trunk and branch.

One suggestion: for the aggregate types, it seems like we're using
"read_structure_scope" to mean "create the type object, but don't call
new_symbol", and "process_structure_scope" to mean "go ahead and
create the new symbols".  But for the enumerations, the two
corresponding functions are called "read_enumeration_type" and
"read_enumeration_scope".  Their functions are parallel, but the names
don't reflect it.  So how about:


                 don't make sym                       make sym

struct:        read_structure_scope             process_structure_scope
enum:          read_enumeration_scope           process_enumeration_scope

or:

struct:        read_structure_type              process_structure_scope
enum:          read_enumeration_type            process_enumeration_scope

Re: [RFA/dwarf] Don't process types multiple times

[Andrew Cagney]

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

> Is that right?  If so, please commit, to trunk and branch.

Jim,

Is this fixing a regression from GDB 6.0?

Andrew


[-- Attachment #2: Attached Message --]
[-- Type: message/rfc822, Size: 3682 bytes --]

From: Andrew Haley <aph@redhat.com>
To: gdb@sources.redhat.com
Cc: java@gcc.gnu.org, "Boehm, Hans" <hans.boehm@hp.com>
Subject: Re: gdb vs. gcj
Date: Wed, 10 Mar 2004 10:46:13 +0000
Message-ID: <16462.61941.71827.936295@cuddles.cambridge.redhat.com>

CCd to gdb for info.

Hans, can you provide some more info?  When does it crash?  Can you
provide a gdb backtrace?

Andrew.


----------------------------------------------------------------------------------------
From: Ranjit Mathew <rmathew@hotmail.com>
Sender: java-owner@gcc.gnu.org
To: java@gcc.gnu.org
Subject: Re: gdb vs. gcj
Date: Wed, 10 Mar 2004 12:05:44 +0530

Boehm, Hans wrote:
> I'm having problems with various versions of gdb crashing with SIGSEGV when trying to debug
> any executables that include libgcj (cvs trunk from about a week ago).
> 
> I've seen this on both X86 and Itanium, both with older RedHat releases (8.0 and 7.1).
> 
> Does this ring any bells?  Did I miss something?

If GDB is crashing for you, it *could be* due to the
recent location lists related changes in GCC that are
properly handled only by the currently in CVS (and to
be released as 6.1) GDB.

See:

    http://gcc.gnu.org/gcc-3.5/changes.html

HTH,
Ranjit.

-- 
Ranjit Mathew          Email: rmathew AT hotmail DOT com

Bangalore, INDIA.      Web: http://ranjitmathew.tripod.com/

Re: [RFA/dwarf] Don't process types multiple times

[Andrew Cagney]

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

> Is that right?  If so, please commit, to trunk and branch.

Jim,

Is this fixing a regression from GDB 6.0?

Andrew


[-- Attachment #2: Attached Message --]
[-- Type: message/rfc822, Size: 3682 bytes --]

From: Andrew Haley <aph@redhat.com>
To: gdb@sources.redhat.com
Cc: java@gcc.gnu.org, "Boehm, Hans" <hans.boehm@hp.com>
Subject: Re: gdb vs. gcj
Date: Wed, 10 Mar 2004 10:46:13 +0000
Message-ID: <16462.61941.71827.936295@cuddles.cambridge.redhat.com>

CCd to gdb for info.

Hans, can you provide some more info?  When does it crash?  Can you
provide a gdb backtrace?

Andrew.


----------------------------------------------------------------------------------------
From: Ranjit Mathew <rmathew@hotmail.com>
Sender: java-owner@gcc.gnu.org
To: java@gcc.gnu.org
Subject: Re: gdb vs. gcj
Date: Wed, 10 Mar 2004 12:05:44 +0530

Boehm, Hans wrote:
> I'm having problems with various versions of gdb crashing with SIGSEGV when trying to debug
> any executables that include libgcj (cvs trunk from about a week ago).
> 
> I've seen this on both X86 and Itanium, both with older RedHat releases (8.0 and 7.1).
> 
> Does this ring any bells?  Did I miss something?

If GDB is crashing for you, it *could be* due to the
recent location lists related changes in GCC that are
properly handled only by the currently in CVS (and to
be released as 6.1) GDB.

See:

    http://gcc.gnu.org/gcc-3.5/changes.html

HTH,
Ranjit.

-- 
Ranjit Mathew          Email: rmathew AT hotmail DOT com

Bangalore, INDIA.      Web: http://ranjitmathew.tripod.com/

Re: [RFA/dwarf] Don't process types multiple times

[Daniel Jacobowitz]

On Fri, Mar 12, 2004 at 06:44:59PM -0500, Jim Blandy wrote:
> 
> Daniel Jacobowitz <drow@false.org> writes:
> > This independent patch is a performance and correctness fix for full symbol
> > processing.  My description from the intercu branch posting:
> > 
> >  There are two ways that we can process a general DIE: process_die or
> >  read_type_die.  Children of particular DIE types may be processed directly,
> >  but these are the only major dispatch points.  It's interesting to notice
> >  that almost everything called from read_type_die starts with "if (die->type)
> >  return": everything but enumeration types and aggregate types, in fact.
> >  This means that if the first reference to an enumeration or aggregate type
> >  is a DW_AT_type or DW_AT_containing_type in a DIE numerically before the
> >  type's DIE, we'll end up calling new_symbol for it twice.
> > 
> >  Fixing this saves about 8% memory and 4% time from gdb -readnow libc.so.6,
> >  a lot of duplicate entries on maint print symbols which I vaguely recall
> >  being confused about but never investigated, and some serious problems for
> >  inter-CU support.  Without this, types could be added to the wrong symbol
> >  table.
> > 
> > Tested i686-pc-linux-gnu, no regressions.  OK to commit?
> 
> So the essential idea here is to make sure that we only create symbols
> when we reach them via the main traversal, not when we happen to reach
> them by following references with die_type.  The current code relies
> on the existing read_structure_scope and read_enumeration functions to
> both read the type and create the symbol, so it can't distinguish
> between the two cases.  Your patch creates a separate function for
> each role, and ensures that the symbol creation functions are only
> called from the main traversal.
> 
> Is that right?  If so, please commit, to trunk and branch.

Precisely.  I've committed it with your and David's suggested cosmetic
changes - I like the revised function names much better.  Here's the
final patch.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2004-03-14  Daniel Jacobowitz  <drow@mvista.com>

	* dwarf2read.c (read_structure_type): Rename from
	read_structure_scope.  Don't create a symbol or call process_die. 
	Return immediately if die->type is set.  Call read_type_die before
	dwarf2_add_member_fn.
	(process_structure_scope): New function.
	(read_enumeration_type, process_enumeration_scope): New functions,
	broken out from read_enumeration.  Don't create the enumeration
	type if it has already been created.
	(read_enumeration): Removed.
	(process_die): Call read_structure_type, process_structure_scope,
	read_enumeration_type, and process_enumeration_scope.  Just call
	new_symbol for base and subrange types.  Add a comment about other
	type dies.
	(read_type_die): Call read_enumeration_type.
	(add_partial_structure, new_symbol): Update comments.

Index: dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.135
diff -u -p -r1.135 dwarf2read.c
--- dwarf2read.c	21 Feb 2004 02:13:35 -0000	1.135
+++ dwarf2read.c	14 Mar 2004 20:48:54 -0000
@@ -804,7 +804,9 @@ static void dwarf2_add_member_fn (struct
 static void dwarf2_attach_fn_fields_to_type (struct field_info *,
 					     struct type *, struct dwarf2_cu *);
 
-static void read_structure_scope (struct die_info *, struct dwarf2_cu *);
+static void read_structure_type (struct die_info *, struct dwarf2_cu *);
+
+static void process_structure_scope (struct die_info *, struct dwarf2_cu *);
 
 static void read_common_block (struct die_info *, struct dwarf2_cu *);
 
@@ -813,7 +815,9 @@ static void read_namespace (struct die_i
 static const char *namespace_name (struct die_info *die,
 				   int *is_anonymous, struct dwarf2_cu *);
 
-static void read_enumeration (struct die_info *, struct dwarf2_cu *);
+static void read_enumeration_type (struct die_info *, struct dwarf2_cu *);
+
+static void process_enumeration_scope (struct die_info *, struct dwarf2_cu *);
 
 static struct type *dwarf_base_type (int, int, struct dwarf2_cu *);
 
@@ -1673,7 +1677,7 @@ add_partial_structure (struct partial_di
 	 what template types look like, because the demangler
 	 frequently doesn't give the same name as the debug info.  We
 	 could fix this by only using the demangled name to get the
-	 prefix (but see comment in read_structure_scope).  */
+	 prefix (but see comment in read_structure_type).  */
 
       /* FIXME: carlton/2004-01-23: If NAMESPACE equals "", we have
 	 the appropriate debug information, so it would be nice to be
@@ -1949,11 +1953,17 @@ process_die (struct die_info *die, struc
     case DW_TAG_class_type:
     case DW_TAG_structure_type:
     case DW_TAG_union_type:
-      read_structure_scope (die, cu);
+      read_structure_type (die, cu);
+      process_structure_scope (die, cu);
       break;
     case DW_TAG_enumeration_type:
-      read_enumeration (die, cu);
+      read_enumeration_type (die, cu);
+      process_enumeration_scope (die, cu);
       break;
+
+    /* FIXME drow/2004-03-14: These initialize die->type, but do not create
+       a symbol or process any children.  Therefore it doesn't do anything
+       that won't be done on-demand by read_type_die.  */
     case DW_TAG_subroutine_type:
       read_subroutine_type (die, cu);
       break;
@@ -1972,21 +1982,19 @@ process_die (struct die_info *die, struc
     case DW_TAG_string_type:
       read_tag_string_type (die, cu);
       break;
+    /* END FIXME */
+
     case DW_TAG_base_type:
       read_base_type (die, cu);
-      if (dwarf2_attr (die, DW_AT_name, cu))
-	{
-	  /* Add a typedef symbol for the base type definition.  */
-	  new_symbol (die, die->type, cu);
-	}
+      /* Add a typedef symbol for the type definition, if it has a
+	 DW_AT_name.  */
+      new_symbol (die, die->type, cu);
       break;
     case DW_TAG_subrange_type:
       read_subrange_type (die, cu);
-      if (dwarf2_attr (die, DW_AT_name, cu))
-       {
-         /* Add a typedef symbol for the base type definition.  */
-         new_symbol (die, die->type, cu);
-       }
+      /* Add a typedef symbol for the type definition, if it has a
+         DW_AT_name.  */
+      new_symbol (die, die->type, cu);
       break;
     case DW_TAG_common_block:
       read_common_block (die, cu);
@@ -2995,7 +3003,7 @@ dwarf2_attach_fn_fields_to_type (struct 
    suppresses creating a symbol table entry itself).  */
 
 static void
-read_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
+read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
 {
   struct objfile *objfile = cu->objfile;
   struct type *type;
@@ -3008,6 +3016,9 @@ read_structure_scope (struct die_info *d
      any.  */
   int need_to_update_name = 0;
 
+  if (die->type)
+    return;
+
   type = alloc_type (objfile);
 
   INIT_CPLUS_SPECIFIC (type);
@@ -3106,7 +3117,7 @@ read_structure_scope (struct die_info *d
 	  else if (child_die->tag == DW_TAG_subprogram)
 	    {
 	      /* C++ member function. */
-	      process_die (child_die, cu);
+	      read_type_die (child_die, cu);
 	      dwarf2_add_member_fn (&fi, child_die, type, cu);
 	      if (need_to_update_name)
 		{
@@ -3149,10 +3160,6 @@ read_structure_scope (struct die_info *d
 	      /* C++ base class field.  */
 	      dwarf2_add_field (&fi, child_die, cu);
 	    }
-	  else
-	    {
-	      process_die (child_die, cu);
-	    }
 	  child_die = sibling_die (child_die);
 	}
 
@@ -3209,8 +3216,6 @@ read_structure_scope (struct die_info *d
 	    }
 	}
 
-      new_symbol (die, type, cu);
-
       do_cleanups (back_to);
     }
   else
@@ -3224,26 +3229,53 @@ read_structure_scope (struct die_info *d
     do_cleanups (back_to);
 }
 
-/* Given a pointer to a die which begins an enumeration, process all
-   the dies that define the members of the enumeration.
+static void
+process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
+{
+  struct objfile *objfile = cu->objfile;
+  const char *previous_prefix = processing_current_prefix;
 
-   This will be much nicer in draft 6 of the DWARF spec when our
-   members will be dies instead squished into the DW_AT_element_list
-   attribute.
+  if (TYPE_TAG_NAME (die->type) != NULL)
+    processing_current_prefix = TYPE_TAG_NAME (die->type);
 
-   NOTE: We reverse the order of the element list.  */
+  if (die->child != NULL && ! die_is_declaration (die, cu))
+    {
+      struct die_info *child_die;
+
+      child_die = die->child;
+
+      while (child_die && child_die->tag)
+	{
+	  if (child_die->tag == DW_TAG_member
+	      || child_die->tag == DW_TAG_variable
+	      || child_die->tag == DW_TAG_inheritance)
+	    {
+	      /* Do nothing.  */
+	    }
+	  else
+	    process_die (child_die, cu);
+
+	  child_die = sibling_die (child_die);
+	}
+
+      new_symbol (die, die->type, cu);
+    }
+
+  processing_current_prefix = previous_prefix;
+}
+
+/* Given a DW_AT_enumeration_type die, set its type.  We do not
+   complete the type's fields yet, or create any symbols.  */
 
 static void
-read_enumeration (struct die_info *die, struct dwarf2_cu *cu)
+read_enumeration_type (struct die_info *die, struct dwarf2_cu *cu)
 {
   struct objfile *objfile = cu->objfile;
-  struct die_info *child_die;
   struct type *type;
-  struct field *fields;
   struct attribute *attr;
-  struct symbol *sym;
-  int num_fields;
-  int unsigned_enum = 1;
+
+  if (die->type)
+    return;
 
   type = alloc_type (objfile);
 
@@ -3278,6 +3310,26 @@ read_enumeration (struct die_info *die, 
       TYPE_LENGTH (type) = 0;
     }
 
+  die->type = type;
+}
+
+/* Given a pointer to a die which begins an enumeration, process all
+   the dies that define the members of the enumeration, and create the
+   symbol for the enumeration type.
+
+   NOTE: We reverse the order of the element list.  */
+
+static void
+process_enumeration_scope (struct die_info *die, struct dwarf2_cu *cu)
+{
+  struct objfile *objfile = cu->objfile;
+  struct die_info *child_die;
+  struct field *fields;
+  struct attribute *attr;
+  struct symbol *sym;
+  int num_fields;
+  int unsigned_enum = 1;
+
   num_fields = 0;
   fields = NULL;
   if (die->child != NULL)
@@ -3294,7 +3346,7 @@ read_enumeration (struct die_info *die, 
 	      attr = dwarf2_attr (child_die, DW_AT_name, cu);
 	      if (attr)
 		{
-		  sym = new_symbol (child_die, type, cu);
+		  sym = new_symbol (child_die, die->type, cu);
 		  if (SYMBOL_VALUE (sym) < 0)
 		    unsigned_enum = 0;
 
@@ -3321,18 +3373,18 @@ read_enumeration (struct die_info *die, 
 
       if (num_fields)
 	{
-	  TYPE_NFIELDS (type) = num_fields;
-	  TYPE_FIELDS (type) = (struct field *)
-	    TYPE_ALLOC (type, sizeof (struct field) * num_fields);
-	  memcpy (TYPE_FIELDS (type), fields,
+	  TYPE_NFIELDS (die->type) = num_fields;
+	  TYPE_FIELDS (die->type) = (struct field *)
+	    TYPE_ALLOC (die->type, sizeof (struct field) * num_fields);
+	  memcpy (TYPE_FIELDS (die->type), fields,
 		  sizeof (struct field) * num_fields);
 	  xfree (fields);
 	}
       if (unsigned_enum)
-	TYPE_FLAGS (type) |= TYPE_FLAG_UNSIGNED;
+	TYPE_FLAGS (die->type) |= TYPE_FLAG_UNSIGNED;
     }
-  die->type = type;
-  new_symbol (die, type, cu);
+
+  new_symbol (die, die->type, cu);
 }
 
 /* Extract all information from a DW_TAG_array_type DIE and put it in
@@ -5666,7 +5718,7 @@ new_symbol (struct die_info *die, struct
 
 	  /* Make sure that the symbol includes appropriate enclosing
 	     classes/namespaces in its name.  These are calculated in
-	     read_structure_scope, and the correct name is saved in
+	     read_structure_type, and the correct name is saved in
 	     the type.  */
 
 	  if (cu->language == language_cplus)
@@ -6009,10 +6061,10 @@ read_type_die (struct die_info *die, str
     case DW_TAG_class_type:
     case DW_TAG_structure_type:
     case DW_TAG_union_type:
-      read_structure_scope (die, cu);
+      read_structure_type (die, cu);
       break;
     case DW_TAG_enumeration_type:
-      read_enumeration (die, cu);
+      read_enumeration_type (die, cu);
       break;
     case DW_TAG_subprogram:
     case DW_TAG_subroutine_type:

Re: [RFA/dwarf] Don't process types multiple times

[Daniel Jacobowitz]

On Fri, Mar 12, 2004 at 06:44:59PM -0500, Jim Blandy wrote:
> 
> Daniel Jacobowitz <drow@false.org> writes:
> > This independent patch is a performance and correctness fix for full symbol
> > processing.  My description from the intercu branch posting:
> > 
> >  There are two ways that we can process a general DIE: process_die or
> >  read_type_die.  Children of particular DIE types may be processed directly,
> >  but these are the only major dispatch points.  It's interesting to notice
> >  that almost everything called from read_type_die starts with "if (die->type)
> >  return": everything but enumeration types and aggregate types, in fact.
> >  This means that if the first reference to an enumeration or aggregate type
> >  is a DW_AT_type or DW_AT_containing_type in a DIE numerically before the
> >  type's DIE, we'll end up calling new_symbol for it twice.
> > 
> >  Fixing this saves about 8% memory and 4% time from gdb -readnow libc.so.6,
> >  a lot of duplicate entries on maint print symbols which I vaguely recall
> >  being confused about but never investigated, and some serious problems for
> >  inter-CU support.  Without this, types could be added to the wrong symbol
> >  table.
> > 
> > Tested i686-pc-linux-gnu, no regressions.  OK to commit?
> 
> So the essential idea here is to make sure that we only create symbols
> when we reach them via the main traversal, not when we happen to reach
> them by following references with die_type.  The current code relies
> on the existing read_structure_scope and read_enumeration functions to
> both read the type and create the symbol, so it can't distinguish
> between the two cases.  Your patch creates a separate function for
> each role, and ensures that the symbol creation functions are only
> called from the main traversal.
> 
> Is that right?  If so, please commit, to trunk and branch.

Precisely.  I've committed it with your and David's suggested cosmetic
changes - I like the revised function names much better.  Here's the
final patch.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2004-03-14  Daniel Jacobowitz  <drow@mvista.com>

	* dwarf2read.c (read_structure_type): Rename from
	read_structure_scope.  Don't create a symbol or call process_die. 
	Return immediately if die->type is set.  Call read_type_die before
	dwarf2_add_member_fn.
	(process_structure_scope): New function.
	(read_enumeration_type, process_enumeration_scope): New functions,
	broken out from read_enumeration.  Don't create the enumeration
	type if it has already been created.
	(read_enumeration): Removed.
	(process_die): Call read_structure_type, process_structure_scope,
	read_enumeration_type, and process_enumeration_scope.  Just call
	new_symbol for base and subrange types.  Add a comment about other
	type dies.
	(read_type_die): Call read_enumeration_type.
	(add_partial_structure, new_symbol): Update comments.

Index: dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.135
diff -u -p -r1.135 dwarf2read.c
--- dwarf2read.c	21 Feb 2004 02:13:35 -0000	1.135
+++ dwarf2read.c	14 Mar 2004 20:48:54 -0000
@@ -804,7 +804,9 @@ static void dwarf2_add_member_fn (struct
 static void dwarf2_attach_fn_fields_to_type (struct field_info *,
 					     struct type *, struct dwarf2_cu *);
 
-static void read_structure_scope (struct die_info *, struct dwarf2_cu *);
+static void read_structure_type (struct die_info *, struct dwarf2_cu *);
+
+static void process_structure_scope (struct die_info *, struct dwarf2_cu *);
 
 static void read_common_block (struct die_info *, struct dwarf2_cu *);
 
@@ -813,7 +815,9 @@ static void read_namespace (struct die_i
 static const char *namespace_name (struct die_info *die,
 				   int *is_anonymous, struct dwarf2_cu *);
 
-static void read_enumeration (struct die_info *, struct dwarf2_cu *);
+static void read_enumeration_type (struct die_info *, struct dwarf2_cu *);
+
+static void process_enumeration_scope (struct die_info *, struct dwarf2_cu *);
 
 static struct type *dwarf_base_type (int, int, struct dwarf2_cu *);
 
@@ -1673,7 +1677,7 @@ add_partial_structure (struct partial_di
 	 what template types look like, because the demangler
 	 frequently doesn't give the same name as the debug info.  We
 	 could fix this by only using the demangled name to get the
-	 prefix (but see comment in read_structure_scope).  */
+	 prefix (but see comment in read_structure_type).  */
 
       /* FIXME: carlton/2004-01-23: If NAMESPACE equals "", we have
 	 the appropriate debug information, so it would be nice to be
@@ -1949,11 +1953,17 @@ process_die (struct die_info *die, struc
     case DW_TAG_class_type:
     case DW_TAG_structure_type:
     case DW_TAG_union_type:
-      read_structure_scope (die, cu);
+      read_structure_type (die, cu);
+      process_structure_scope (die, cu);
       break;
     case DW_TAG_enumeration_type:
-      read_enumeration (die, cu);
+      read_enumeration_type (die, cu);
+      process_enumeration_scope (die, cu);
       break;
+
+    /* FIXME drow/2004-03-14: These initialize die->type, but do not create
+       a symbol or process any children.  Therefore it doesn't do anything
+       that won't be done on-demand by read_type_die.  */
     case DW_TAG_subroutine_type:
       read_subroutine_type (die, cu);
       break;
@@ -1972,21 +1982,19 @@ process_die (struct die_info *die, struc
     case DW_TAG_string_type:
       read_tag_string_type (die, cu);
       break;
+    /* END FIXME */
+
     case DW_TAG_base_type:
       read_base_type (die, cu);
-      if (dwarf2_attr (die, DW_AT_name, cu))
-	{
-	  /* Add a typedef symbol for the base type definition.  */
-	  new_symbol (die, die->type, cu);
-	}
+      /* Add a typedef symbol for the type definition, if it has a
+	 DW_AT_name.  */
+      new_symbol (die, die->type, cu);
       break;
     case DW_TAG_subrange_type:
       read_subrange_type (die, cu);
-      if (dwarf2_attr (die, DW_AT_name, cu))
-       {
-         /* Add a typedef symbol for the base type definition.  */
-         new_symbol (die, die->type, cu);
-       }
+      /* Add a typedef symbol for the type definition, if it has a
+         DW_AT_name.  */
+      new_symbol (die, die->type, cu);
       break;
     case DW_TAG_common_block:
       read_common_block (die, cu);
@@ -2995,7 +3003,7 @@ dwarf2_attach_fn_fields_to_type (struct 
    suppresses creating a symbol table entry itself).  */
 
 static void
-read_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
+read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
 {
   struct objfile *objfile = cu->objfile;
   struct type *type;
@@ -3008,6 +3016,9 @@ read_structure_scope (struct die_info *d
      any.  */
   int need_to_update_name = 0;
 
+  if (die->type)
+    return;
+
   type = alloc_type (objfile);
 
   INIT_CPLUS_SPECIFIC (type);
@@ -3106,7 +3117,7 @@ read_structure_scope (struct die_info *d
 	  else if (child_die->tag == DW_TAG_subprogram)
 	    {
 	      /* C++ member function. */
-	      process_die (child_die, cu);
+	      read_type_die (child_die, cu);
 	      dwarf2_add_member_fn (&fi, child_die, type, cu);
 	      if (need_to_update_name)
 		{
@@ -3149,10 +3160,6 @@ read_structure_scope (struct die_info *d
 	      /* C++ base class field.  */
 	      dwarf2_add_field (&fi, child_die, cu);
 	    }
-	  else
-	    {
-	      process_die (child_die, cu);
-	    }
 	  child_die = sibling_die (child_die);
 	}
 
@@ -3209,8 +3216,6 @@ read_structure_scope (struct die_info *d
 	    }
 	}
 
-      new_symbol (die, type, cu);
-
       do_cleanups (back_to);
     }
   else
@@ -3224,26 +3229,53 @@ read_structure_scope (struct die_info *d
     do_cleanups (back_to);
 }
 
-/* Given a pointer to a die which begins an enumeration, process all
-   the dies that define the members of the enumeration.
+static void
+process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
+{
+  struct objfile *objfile = cu->objfile;
+  const char *previous_prefix = processing_current_prefix;
 
-   This will be much nicer in draft 6 of the DWARF spec when our
-   members will be dies instead squished into the DW_AT_element_list
-   attribute.
+  if (TYPE_TAG_NAME (die->type) != NULL)
+    processing_current_prefix = TYPE_TAG_NAME (die->type);
 
-   NOTE: We reverse the order of the element list.  */
+  if (die->child != NULL && ! die_is_declaration (die, cu))
+    {
+      struct die_info *child_die;
+
+      child_die = die->child;
+
+      while (child_die && child_die->tag)
+	{
+	  if (child_die->tag == DW_TAG_member
+	      || child_die->tag == DW_TAG_variable
+	      || child_die->tag == DW_TAG_inheritance)
+	    {
+	      /* Do nothing.  */
+	    }
+	  else
+	    process_die (child_die, cu);
+
+	  child_die = sibling_die (child_die);
+	}
+
+      new_symbol (die, die->type, cu);
+    }
+
+  processing_current_prefix = previous_prefix;
+}
+
+/* Given a DW_AT_enumeration_type die, set its type.  We do not
+   complete the type's fields yet, or create any symbols.  */
 
 static void
-read_enumeration (struct die_info *die, struct dwarf2_cu *cu)
+read_enumeration_type (struct die_info *die, struct dwarf2_cu *cu)
 {
   struct objfile *objfile = cu->objfile;
-  struct die_info *child_die;
   struct type *type;
-  struct field *fields;
   struct attribute *attr;
-  struct symbol *sym;
-  int num_fields;
-  int unsigned_enum = 1;
+
+  if (die->type)
+    return;
 
   type = alloc_type (objfile);
 
@@ -3278,6 +3310,26 @@ read_enumeration (struct die_info *die, 
       TYPE_LENGTH (type) = 0;
     }
 
+  die->type = type;
+}
+
+/* Given a pointer to a die which begins an enumeration, process all
+   the dies that define the members of the enumeration, and create the
+   symbol for the enumeration type.
+
+   NOTE: We reverse the order of the element list.  */
+
+static void
+process_enumeration_scope (struct die_info *die, struct dwarf2_cu *cu)
+{
+  struct objfile *objfile = cu->objfile;
+  struct die_info *child_die;
+  struct field *fields;
+  struct attribute *attr;
+  struct symbol *sym;
+  int num_fields;
+  int unsigned_enum = 1;
+
   num_fields = 0;
   fields = NULL;
   if (die->child != NULL)
@@ -3294,7 +3346,7 @@ read_enumeration (struct die_info *die, 
 	      attr = dwarf2_attr (child_die, DW_AT_name, cu);
 	      if (attr)
 		{
-		  sym = new_symbol (child_die, type, cu);
+		  sym = new_symbol (child_die, die->type, cu);
 		  if (SYMBOL_VALUE (sym) < 0)
 		    unsigned_enum = 0;
 
@@ -3321,18 +3373,18 @@ read_enumeration (struct die_info *die, 
 
       if (num_fields)
 	{
-	  TYPE_NFIELDS (type) = num_fields;
-	  TYPE_FIELDS (type) = (struct field *)
-	    TYPE_ALLOC (type, sizeof (struct field) * num_fields);
-	  memcpy (TYPE_FIELDS (type), fields,
+	  TYPE_NFIELDS (die->type) = num_fields;
+	  TYPE_FIELDS (die->type) = (struct field *)
+	    TYPE_ALLOC (die->type, sizeof (struct field) * num_fields);
+	  memcpy (TYPE_FIELDS (die->type), fields,
 		  sizeof (struct field) * num_fields);
 	  xfree (fields);
 	}
       if (unsigned_enum)
-	TYPE_FLAGS (type) |= TYPE_FLAG_UNSIGNED;
+	TYPE_FLAGS (die->type) |= TYPE_FLAG_UNSIGNED;
     }
-  die->type = type;
-  new_symbol (die, type, cu);
+
+  new_symbol (die, die->type, cu);
 }
 
 /* Extract all information from a DW_TAG_array_type DIE and put it in
@@ -5666,7 +5718,7 @@ new_symbol (struct die_info *die, struct
 
 	  /* Make sure that the symbol includes appropriate enclosing
 	     classes/namespaces in its name.  These are calculated in
-	     read_structure_scope, and the correct name is saved in
+	     read_structure_type, and the correct name is saved in
 	     the type.  */
 
 	  if (cu->language == language_cplus)
@@ -6009,10 +6061,10 @@ read_type_die (struct die_info *die, str
     case DW_TAG_class_type:
     case DW_TAG_structure_type:
     case DW_TAG_union_type:
-      read_structure_scope (die, cu);
+      read_structure_type (die, cu);
       break;
     case DW_TAG_enumeration_type:
-      read_enumeration (die, cu);
+      read_enumeration_type (die, cu);
       break;
     case DW_TAG_subprogram:
     case DW_TAG_subroutine_type:

Re: [RFA/dwarf] Don't process types multiple times

[Jim Blandy]

Daniel Jacobowitz <drow@false.org> writes:
> This independent patch is a performance and correctness fix for full symbol
> processing.  My description from the intercu branch posting:
> 
>  There are two ways that we can process a general DIE: process_die or
>  read_type_die.  Children of particular DIE types may be processed directly,
>  but these are the only major dispatch points.  It's interesting to notice
>  that almost everything called from read_type_die starts with "if (die->type)
>  return": everything but enumeration types and aggregate types, in fact.
>  This means that if the first reference to an enumeration or aggregate type
>  is a DW_AT_type or DW_AT_containing_type in a DIE numerically before the
>  type's DIE, we'll end up calling new_symbol for it twice.
> 
>  Fixing this saves about 8% memory and 4% time from gdb -readnow libc.so.6,
>  a lot of duplicate entries on maint print symbols which I vaguely recall
>  being confused about but never investigated, and some serious problems for
>  inter-CU support.  Without this, types could be added to the wrong symbol
>  table.
> 
> Tested i686-pc-linux-gnu, no regressions.  OK to commit?

So the essential idea here is to make sure that we only create symbols
when we reach them via the main traversal, not when we happen to reach
them by following references with die_type.  The current code relies
on the existing read_structure_scope and read_enumeration functions to
both read the type and create the symbol, so it can't distinguish
between the two cases.  Your patch creates a separate function for
each role, and ensures that the symbol creation functions are only
called from the main traversal.

Is that right?  If so, please commit, to trunk and branch.

One suggestion: for the aggregate types, it seems like we're using
"read_structure_scope" to mean "create the type object, but don't call
new_symbol", and "process_structure_scope" to mean "go ahead and
create the new symbols".  But for the enumerations, the two
corresponding functions are called "read_enumeration_type" and
"read_enumeration_scope".  Their functions are parallel, but the names
don't reflect it.  So how about:


                 don't make sym                       make sym

struct:        read_structure_scope             process_structure_scope
enum:          read_enumeration_scope           process_enumeration_scope

or:

struct:        read_structure_type              process_structure_scope
enum:          read_enumeration_type            process_enumeration_scope