RFA: Don't use obsavestring in dwarf2read

RFA: Don't use obsavestring in dwarf2read

[Daniel Jacobowitz]

This patch is pretty self-explanatory, and pretty effective: With -readnow
to force immediate loading of full symbols, this is good for 3% startup time
and 30% memory savings (that's 100MB out of 330MB!) for a gdb session
against "monotone".  We already rely on the lifetimes of this data, so
there's no point in duplicating it onto another obstack with the exact same
lifetime.

OK?

[My current C++ work may have significant memory and startup time impact. 
I'm trying to clean house at the same time, so that I don't introduce a net
loss.  This is low-hanging fruit; higher-hanging fruit will take somewhat
longer.]

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2004-01-11  Daniel Jacobowitz  <drow@mvista.com>

	* dwarf2read.c: Add comment describing memory lifetimes.
	(dwarf2_add_field, dwarf2_add_member_fn, read_structure_scope)
	(read_enumeration, new_symbol): Don't use obsavestring.

Index: dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.117
diff -u -p -r1.117 dwarf2read.c
--- dwarf2read.c	13 Dec 2003 22:29:06 -0000	1.117
+++ dwarf2read.c	12 Jan 2004 01:52:13 -0000
@@ -1,5 +1,5 @@
 /* DWARF 2 debugging format support for GDB.
-   Copyright 1994, 1995, 1996, 1997, 1998, 1999, 2000, 2001, 2002, 2003
+   Copyright 1994, 1995, 1996, 1997, 1998, 1999, 2000, 2001, 2002, 2003, 2004
    Free Software Foundation, Inc.
 
    Adapted by Gary Funck (gary@intrepid.com), Intrepid Technology,
@@ -54,6 +54,16 @@
 #define DWARF2_REG_TO_REGNUM(REG) (REG)
 #endif
 
+/* A note on memory usage: at the present time, this code reads the debug
+   info sections into the objfile's psymbol_obstack.  A definite improvement
+   for startup time, on platforms which do not emit relocations for debug
+   sections, would be to use mmap instead.
+   
+   In either case, the sections should remain loaded until the objfile is
+   released, and pointers into the section data can be used for any other
+   data associated to the objfile (symbol names, type names, location expressions
+   to name a few).  */
+
 #if 0
 /* .debug_info header for a compilation unit
    Because of alignment constraints, this structure has padding and cannot
@@ -2436,8 +2446,7 @@ dwarf2_add_field (struct field_info *fip
       attr = dwarf_attr (die, DW_AT_name);
       if (attr && DW_STRING (attr))
 	fieldname = DW_STRING (attr);
-      fp->name = obsavestring (fieldname, strlen (fieldname),
-			       &objfile->type_obstack);
+      fp->name = fieldname;
 
       /* Change accessibility for artificial fields (e.g. virtual table
          pointer or virtual base class pointer) to private.  */
@@ -2468,11 +2477,9 @@ dwarf2_add_field (struct field_info *fip
       /* Get physical name.  */
       physname = dwarf2_linkage_name (die);
 
-      SET_FIELD_PHYSNAME (*fp, obsavestring (physname, strlen (physname),
-					     &objfile->type_obstack));
+      SET_FIELD_PHYSNAME (*fp, physname);
       FIELD_TYPE (*fp) = die_type (die, cu);
-      FIELD_NAME (*fp) = obsavestring (fieldname, strlen (fieldname),
-				       &objfile->type_obstack);
+      FIELD_NAME (*fp) = fieldname;
     }
   else if (die->tag == DW_TAG_inheritance)
     {
@@ -2640,8 +2647,7 @@ dwarf2_add_member_fn (struct field_info 
 
   /* Fill in the member function field info.  */
   fnp = &new_fnfield->fnfield;
-  fnp->physname = obsavestring (physname, strlen (physname),
-				&objfile->type_obstack);
+  fnp->physname = physname;
   fnp->type = alloc_type (objfile);
   if (die->type && TYPE_CODE (die->type) == TYPE_CODE_FUNC)
     {
@@ -2778,11 +2784,7 @@ read_structure_scope (struct die_info *d
   INIT_CPLUS_SPECIFIC (type);
   attr = dwarf_attr (die, DW_AT_name);
   if (attr && DW_STRING (attr))
-    {
-      TYPE_TAG_NAME (type) = obsavestring (DW_STRING (attr),
-					   strlen (DW_STRING (attr)),
-					   &objfile->type_obstack);
-    }
+    TYPE_TAG_NAME (type) = DW_STRING (attr);
 
   if (die->tag == DW_TAG_structure_type)
     {
@@ -2944,11 +2946,7 @@ read_enumeration (struct die_info *die, 
   TYPE_CODE (type) = TYPE_CODE_ENUM;
   attr = dwarf_attr (die, DW_AT_name);
   if (attr && DW_STRING (attr))
-    {
-      TYPE_TAG_NAME (type) = obsavestring (DW_STRING (attr),
-					   strlen (DW_STRING (attr)),
-					   &objfile->type_obstack);
-    }
+    TYPE_TAG_NAME (type) = DW_STRING (attr);
 
   attr = dwarf_attr (die, DW_AT_byte_size);
   if (attr)
@@ -5325,10 +5323,7 @@ new_symbol (struct die_info *die, struct
 	      *typedef_sym = *sym;
 	      SYMBOL_DOMAIN (typedef_sym) = VAR_DOMAIN;
 	      if (TYPE_NAME (SYMBOL_TYPE (sym)) == 0)
-		TYPE_NAME (SYMBOL_TYPE (sym)) =
-		  obsavestring (DEPRECATED_SYMBOL_NAME (sym),
-				strlen (DEPRECATED_SYMBOL_NAME (sym)),
-				&objfile->type_obstack);
+		TYPE_NAME (SYMBOL_TYPE (sym)) = DEPRECATED_SYMBOL_NAME (sym);
 	      add_symbol_to_list (typedef_sym, list_in_scope);
 	    }
 	  break;

Re: RFA: Don't use obsavestring in dwarf2read

[Daniel Jacobowitz]

On Sun, Jan 11, 2004 at 08:57:26PM -0500, Daniel Jacobowitz wrote:
> This patch is pretty self-explanatory, and pretty effective: With -readnow
> to force immediate loading of full symbols, this is good for 3% startup time
> and 30% memory savings (that's 100MB out of 330MB!) for a gdb session
> against "monotone".  We already rely on the lifetimes of this data, so
> there's no point in duplicating it onto another obstack with the exact same
> lifetime.
> 
> OK?
> 
> [My current C++ work may have significant memory and startup time impact. 
> I'm trying to clean house at the same time, so that I don't introduce a net
> loss.  This is low-hanging fruit; higher-hanging fruit will take somewhat
> longer.]

Updated for Michael's comments, and to fix merge issues (and a new
introduction of obsavestring).  I also updated the leading comment to
mention that symbols and types can now point into each other's
obstacks.

OK?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

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

	* dwarf2read.c: Add comment describing memory lifetimes.
	(dwarf2_add_field, dwarf2_add_member_fn, read_structure_scope)
	(read_enumeration, new_symbol): Don't use obsavestring.

Index: dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.130
diff -u -p -r1.130 dwarf2read.c
--- dwarf2read.c	28 Jan 2004 18:43:06 -0000	1.130
+++ dwarf2read.c	2 Feb 2004 18:19:45 -0000
@@ -55,6 +55,21 @@
 #define DWARF2_REG_TO_REGNUM(REG) (REG)
 #endif
 
+/* A note on memory usage: at the present time, this code reads the debug
+   info sections into the objfile's psymbol_obstack.  A definite improvement
+   for startup time, on platforms which do not emit relocations for debug
+   sections, would be to use mmap instead.
+   
+   In either case, the sections should remain loaded until the objfile is
+   released, and pointers into the section data can be used for any other
+   data associated to the objfile (symbol names, type names, location expressions
+   to name a few).
+
+   Also, this code occasionally sets symbol names to point at type names, and
+   vice versa.  This can cause symbols pointing into the type obstack and
+   types pointing into the symbol obstack.  They have the same lifetime
+   in current GDB so this is harmless, and prevents wasteful duplication.  */
+
 #if 0
 /* .debug_info header for a compilation unit
    Because of alignment constraints, this structure has padding and cannot
@@ -2665,8 +2680,7 @@ dwarf2_add_field (struct field_info *fip
       attr = dwarf2_attr (die, DW_AT_name, cu);
       if (attr && DW_STRING (attr))
 	fieldname = DW_STRING (attr);
-      fp->name = obsavestring (fieldname, strlen (fieldname),
-			       &objfile->type_obstack);
+      fp->name = fieldname;
 
       /* Change accessibility for artificial fields (e.g. virtual table
          pointer or virtual base class pointer) to private.  */
@@ -2697,11 +2711,9 @@ dwarf2_add_field (struct field_info *fip
       /* Get physical name.  */
       physname = dwarf2_linkage_name (die, cu);
 
-      SET_FIELD_PHYSNAME (*fp, obsavestring (physname, strlen (physname),
-					     &objfile->type_obstack));
+      SET_FIELD_PHYSNAME (*fp, physname ? physname : "");
       FIELD_TYPE (*fp) = die_type (die, cu);
-      FIELD_NAME (*fp) = obsavestring (fieldname, strlen (fieldname),
-				       &objfile->type_obstack);
+      FIELD_NAME (*fp) = fieldname;
     }
   else if (die->tag == DW_TAG_inheritance)
     {
@@ -2869,8 +2881,7 @@ dwarf2_add_member_fn (struct field_info 
 
   /* Fill in the member function field info.  */
   fnp = &new_fnfield->fnfield;
-  fnp->physname = obsavestring (physname, strlen (physname),
-				&objfile->type_obstack);
+  fnp->physname = physname ? physname : "";
   fnp->type = alloc_type (objfile);
   if (die->type && TYPE_CODE (die->type) == TYPE_CODE_FUNC)
     {
@@ -3046,8 +3057,7 @@ read_structure_scope (struct die_info *d
 	}
       else
 	{
-	  TYPE_TAG_NAME (type) = obsavestring (name, strlen (name),
-					       &objfile->type_obstack);
+	  TYPE_TAG_NAME (type) = name;
 	  need_to_update_name = (cu->language == language_cplus);
 	}
     }
@@ -3263,10 +3273,7 @@ read_enumeration (struct die_info *die, 
 					   name);
 	}
       else
-	{
-	  TYPE_TAG_NAME (type) = obsavestring (name, strlen (name),
-					       &objfile->type_obstack);
-	}
+	TYPE_TAG_NAME (type) = name;
     }
 
   attr = dwarf2_attr (die, DW_AT_byte_size, cu);
@@ -5679,10 +5686,7 @@ new_symbol (struct die_info *die, struct
 		  /* FIXME: carlton/2003-11-10: Should this use
 		     SYMBOL_SET_NAMES instead?  (The same problem also
 		     arises a further down in the function.)  */
-		  SYMBOL_LINKAGE_NAME (sym)
-		    = obsavestring (TYPE_TAG_NAME (type),
-				    strlen (TYPE_TAG_NAME (type)),
-				    &objfile->symbol_obstack);
+		  SYMBOL_LINKAGE_NAME (sym) = TYPE_TAG_NAME (type);
 		}
 	    }
 
@@ -5714,10 +5718,7 @@ new_symbol (struct die_info *die, struct
 		*typedef_sym = *sym;
 		SYMBOL_DOMAIN (typedef_sym) = VAR_DOMAIN;
 		if (TYPE_NAME (SYMBOL_TYPE (sym)) == 0)
-		  TYPE_NAME (SYMBOL_TYPE (sym)) =
-		    obsavestring (SYMBOL_NATURAL_NAME (sym),
-				  strlen (SYMBOL_NATURAL_NAME (sym)),
-				  &objfile->type_obstack);
+		  TYPE_NAME (SYMBOL_TYPE (sym)) = SYMBOL_NATURAL_NAME (sym);
 		add_symbol_to_list (typedef_sym, list_to_add);
 	      }
 	  }

Re: RFA: Don't use obsavestring in dwarf2read

[Elena Zannoni]

Daniel Jacobowitz writes:
 > On Sun, Jan 11, 2004 at 08:57:26PM -0500, Daniel Jacobowitz wrote:
 > > This patch is pretty self-explanatory, and pretty effective: With -readnow
 > > to force immediate loading of full symbols, this is good for 3% startup time
 > > and 30% memory savings (that's 100MB out of 330MB!) for a gdb session
 > > against "monotone".  We already rely on the lifetimes of this data, so
 > > there's no point in duplicating it onto another obstack with the exact same
 > > lifetime.
 > > 
 > > OK?
 > > 
 > > [My current C++ work may have significant memory and startup time impact. 
 > > I'm trying to clean house at the same time, so that I don't introduce a net
 > > loss.  This is low-hanging fruit; higher-hanging fruit will take somewhat
 > > longer.]
 > 
 > Updated for Michael's comments, and to fix merge issues (and a new
 > introduction of obsavestring).  I also updated the leading comment to
 > mention that symbols and types can now point into each other's
 > obstacks.


I am not comfortable with this micro-optimization.

The purpose and design of the objfile obstacks would become confusing.
TYPE_TAG_NAME, for instance, would be now allocated on the
type_obstack in all files except for dwarf2read.c. And the
crosspollination between different obstacks also is perplexing. I
don't think that assuming that they will always have the same lifetime
is safe, given they are intentionally separate.

However you do raise some good points. Do we need 3 separate obstacks for
each object file? If they all have the same lifetime, maybe not.
Also are the obstacks a good idea in general? 

[BTW why are only few obstack properly initialized?]

How do you get to 30% savings from these changes?

Re: RFA: Don't use obsavestring in dwarf2read

[Daniel Jacobowitz]

On Thu, Feb 05, 2004 at 02:26:44PM -0500, Elena Zannoni wrote:
> Daniel Jacobowitz writes:
>  > On Sun, Jan 11, 2004 at 08:57:26PM -0500, Daniel Jacobowitz wrote:
>  > > This patch is pretty self-explanatory, and pretty effective: With -readnow
>  > > to force immediate loading of full symbols, this is good for 3% startup time
>  > > and 30% memory savings (that's 100MB out of 330MB!) for a gdb session
>  > > against "monotone".  We already rely on the lifetimes of this data, so
>  > > there's no point in duplicating it onto another obstack with the exact same
>  > > lifetime.
>  > > 
>  > > OK?
>  > > 
>  > > [My current C++ work may have significant memory and startup time impact. 
>  > > I'm trying to clean house at the same time, so that I don't introduce a net
>  > > loss.  This is low-hanging fruit; higher-hanging fruit will take somewhat
>  > > longer.]
>  > 
>  > Updated for Michael's comments, and to fix merge issues (and a new
>  > introduction of obsavestring).  I also updated the leading comment to
>  > mention that symbols and types can now point into each other's
>  > obstacks.
> 
> 
> I am not comfortable with this micro-optimization.
> 
> The purpose and design of the objfile obstacks would become confusing.
> TYPE_TAG_NAME, for instance, would be now allocated on the
> type_obstack in all files except for dwarf2read.c. And the
> crosspollination between different obstacks also is perplexing. I
> don't think that assuming that they will always have the same lifetime
> is safe, given they are intentionally separate.
> 
> However you do raise some good points. Do we need 3 separate obstacks for
> each object file? If they all have the same lifetime, maybe not.
> Also are the obstacks a good idea in general? 

The obstacks themselves are probably a good idea.  Once upon a time,
Peter informed me, there was a plan to free the psymbol obstack when
all symbols had been read in; but that doesn't seem like a useful
optimization, and I can't think offhand of any use for separate symbol
and type obstacks.  I wouldn't object to having a per-objfile obstack
instead, and un-seperating them.

> [BTW why are only few obstack properly initialized?]

Which do you mean?

> How do you get to 30% savings from these changes?

Take a look at how much of the memory usage of GDB on a large C++
application is for storing names.  For monotone, .debug_str is almost
three times the size of .debug_info, at a whopping 40MB.  That's where
the biggest savings comes from: using pointers directly into
.debug_str.  Because of the GNU LD string merging optimizations, that
probably accounts for 80MB or so of the savings.

Another large portion comes from not duplicating the names of types in
the typedef symbols associated with the type.  One was on type_obstack,
the other on symbol_obstack.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: RFA: Don't use obsavestring in dwarf2read

[Elena Zannoni]

Daniel Jacobowitz writes:
 > On Thu, Feb 05, 2004 at 02:26:44PM -0500, Elena Zannoni wrote:
 > > Daniel Jacobowitz writes:
 > >  > On Sun, Jan 11, 2004 at 08:57:26PM -0500, Daniel Jacobowitz wrote:
 > >  > > This patch is pretty self-explanatory, and pretty effective: With -readnow
 > >  > > to force immediate loading of full symbols, this is good for 3% startup time
 > >  > > and 30% memory savings (that's 100MB out of 330MB!) for a gdb session
 > >  > > against "monotone".  We already rely on the lifetimes of this data, so
 > >  > > there's no point in duplicating it onto another obstack with the exact same
 > >  > > lifetime.
 > >  > > 
 > >  > > OK?
 > >  > > 
 > >  > > [My current C++ work may have significant memory and startup time impact. 
 > >  > > I'm trying to clean house at the same time, so that I don't introduce a net
 > >  > > loss.  This is low-hanging fruit; higher-hanging fruit will take somewhat
 > >  > > longer.]
 > >  > 
 > >  > Updated for Michael's comments, and to fix merge issues (and a new
 > >  > introduction of obsavestring).  I also updated the leading comment to
 > >  > mention that symbols and types can now point into each other's
 > >  > obstacks.
 > > 
 > > 
 > > I am not comfortable with this micro-optimization.
 > > 
 > > The purpose and design of the objfile obstacks would become confusing.
 > > TYPE_TAG_NAME, for instance, would be now allocated on the
 > > type_obstack in all files except for dwarf2read.c. And the
 > > crosspollination between different obstacks also is perplexing. I
 > > don't think that assuming that they will always have the same lifetime
 > > is safe, given they are intentionally separate.
 > > 
 > > However you do raise some good points. Do we need 3 separate obstacks for
 > > each object file? If they all have the same lifetime, maybe not.
 > > Also are the obstacks a good idea in general? 
 > 
 > The obstacks themselves are probably a good idea.  Once upon a time,
 > Peter informed me, there was a plan to free the psymbol obstack when
 > all symbols had been read in; but that doesn't seem like a useful
 > optimization, and I can't think offhand of any use for separate symbol
 > and type obstacks.  I wouldn't object to having a per-objfile obstack
 > instead, and un-seperating them.

I think it would be worthwhile to see how much doing that would save us.

 > 
 > > [BTW why are only few obstack properly initialized?]
 > 
 > Which do you mean?
 > 

I grepped for obstack_init, and only a few obstacks call that
function. Form the obstack doco, it seems that it needs to be
called. I wonder if the function was introduced later on in libiberty,
as an afterthought.

 > > How do you get to 30% savings from these changes?
 > 
 > Take a look at how much of the memory usage of GDB on a large C++
 > application is for storing names.  For monotone, .debug_str is almost
 > three times the size of .debug_info, at a whopping 40MB.  That's where
 > the biggest savings comes from: using pointers directly into
 > .debug_str.  Because of the GNU LD string merging optimizations, that
 > probably accounts for 80MB or so of the savings.
 > 

Ah, ok, it's because of the nature of the program you were handling. I
was trying to imagine how the overhead of obstack themselves could be
that large. It seems to me that this is a good argument for an 'on
demand' symbol reading implementaion.  But, yes the various dwarf2
sections are already in the psymbol_obstack. And we are duplicating
that again on the type_obstack. :-( 

 > Another large portion comes from not duplicating the names of types in
 > the typedef symbols associated with the type.  One was on type_obstack,
 > the other on symbol_obstack.
 > 

Right; this would also go away if we unify the obstacks.

Re: RFA: Don't use obsavestring in dwarf2read

[Daniel Jacobowitz]

On Thu, Feb 05, 2004 at 03:34:30PM -0500, Elena Zannoni wrote:
>  > The obstacks themselves are probably a good idea.  Once upon a time,
>  > Peter informed me, there was a plan to free the psymbol obstack when
>  > all symbols had been read in; but that doesn't seem like a useful
>  > optimization, and I can't think offhand of any use for separate symbol
>  > and type obstacks.  I wouldn't object to having a per-objfile obstack
>  > instead, and un-seperating them.
> 
> I think it would be worthwhile to see how much doing that would save us.

Well, it wouldn't save anything by itself - there's immeasurable
overhead to the obstacks.  It would let us eliminate this sort of
duplication, but they're pretty tricky to identify; it took me a couple
of hours to convince myself about this set of 'em.

>  > > [BTW why are only few obstack properly initialized?]
>  > 
>  > Which do you mean?
>  > 
> 
> I grepped for obstack_init, and only a few obstacks call that
> function. Form the obstack doco, it seems that it needs to be
> called. I wonder if the function was introduced later on in libiberty,
> as an afterthought.

It looks like obstack_specify_allocation and obstack_init fill the same
role.  The objfile's obstacks use the former.

> Ah, ok, it's because of the nature of the program you were handling. I
> was trying to imagine how the overhead of obstack themselves could be
> that large. It seems to me that this is a good argument for an 'on
> demand' symbol reading implementaion.  But, yes the various dwarf2
> sections are already in the psymbol_obstack. And we are duplicating
> that again on the type_obstack. :-( 

Right.  I'm not sure how much of this can be done on demand that isn't
already; if I wasn't clear about this, the 100MB was a worst-case
number (-readnow).  Without -readnow it's much less.

>  > Another large portion comes from not duplicating the names of types in
>  > the typedef symbols associated with the type.  One was on type_obstack,
>  > the other on symbol_obstack.
>  > 
> 
> Right; this would also go away if we unify the obstacks.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: RFA: Don't use obsavestring in dwarf2read

[Elena Zannoni]

Daniel Jacobowitz writes:
 > On Thu, Feb 05, 2004 at 03:34:30PM -0500, Elena Zannoni wrote:
 > >  > The obstacks themselves are probably a good idea.  Once upon a time,
 > >  > Peter informed me, there was a plan to free the psymbol obstack when
 > >  > all symbols had been read in; but that doesn't seem like a useful
 > >  > optimization, and I can't think offhand of any use for separate symbol
 > >  > and type obstacks.  I wouldn't object to having a per-objfile obstack
 > >  > instead, and un-seperating them.
 > > 
 > > I think it would be worthwhile to see how much doing that would save us.
 > 
 > Well, it wouldn't save anything by itself - there's immeasurable
 > overhead to the obstacks.  It would let us eliminate this sort of
 > duplication, but they're pretty tricky to identify; it took me a couple
 > of hours to convince myself about this set of 'em.
 > 

I meant in general, yes.  Since the possible 'shorcuts' are difficult
to identify, and they are only for dwarf2 (that I've looked at) I am
bit worried about the cross pointers. I am thinking to kill the triad.

 > >  > > [BTW why are only few obstack properly initialized?]
 > >  > 
 > >  > Which do you mean?
 > >  > 
 > > 
 > > I grepped for obstack_init, and only a few obstacks call that
 > > function. Form the obstack doco, it seems that it needs to be
 > > called. I wonder if the function was introduced later on in libiberty,
 > > as an afterthought.
 > 
 > It looks like obstack_specify_allocation and obstack_init fill the same
 > role.  The objfile's obstacks use the former.
 > 

Which is dumb:

# define obstack_init(h) \
  _obstack_begin ((h), 0, 0, \
		  (void *(*) ()) obstack_chunk_alloc, (void (*) ()) obstack_chunk_free)

# define obstack_specify_allocation(h, size, alignment, chunkfun, freefun) \
  _obstack_begin ((h), (size), (alignment), \
		    (void *(*) ()) (chunkfun), (void (*) ()) (freefun))


all the calls to obstack_specify_allocation use xmalloc and xfree, but
we also have:

/* Unless explicitly specified, GDB obstacks always use xmalloc() and
   xfree().  */
#define obstack_chunk_alloc xmalloc
#define obstack_chunk_free xfree

There is only a call to obstack_specify_allocation that specifies size
and alignment.

I'd prefer them to use just one method. I'll clean up some.

 > > Ah, ok, it's because of the nature of the program you were handling. I
 > > was trying to imagine how the overhead of obstack themselves could be
 > > that large. It seems to me that this is a good argument for an 'on
 > > demand' symbol reading implementaion.  But, yes the various dwarf2
 > > sections are already in the psymbol_obstack. And we are duplicating
 > > that again on the type_obstack. :-( 
 > 
 > Right.  I'm not sure how much of this can be done on demand that isn't
 > already; if I wasn't clear about this, the 100MB was a worst-case
 > number (-readnow).  Without -readnow it's much less.
 > 

ah. Yep, you didn't mention readnow.

Re: RFA: Don't use obsavestring in dwarf2read

[Daniel Jacobowitz]

On Thu, Feb 05, 2004 at 06:16:55PM -0500, Elena Zannoni wrote:
> Daniel Jacobowitz writes:
>  > On Thu, Feb 05, 2004 at 03:34:30PM -0500, Elena Zannoni wrote:
>  > >  > The obstacks themselves are probably a good idea.  Once upon a time,
>  > >  > Peter informed me, there was a plan to free the psymbol obstack when
>  > >  > all symbols had been read in; but that doesn't seem like a useful
>  > >  > optimization, and I can't think offhand of any use for separate symbol
>  > >  > and type obstacks.  I wouldn't object to having a per-objfile obstack
>  > >  > instead, and un-seperating them.
>  > > 
>  > > I think it would be worthwhile to see how much doing that would save us.
>  > 
>  > Well, it wouldn't save anything by itself - there's immeasurable
>  > overhead to the obstacks.  It would let us eliminate this sort of
>  > duplication, but they're pretty tricky to identify; it took me a couple
>  > of hours to convince myself about this set of 'em.
>  > 
> 
> I meant in general, yes.  Since the possible 'shorcuts' are difficult
> to identify, and they are only for dwarf2 (that I've looked at) I am
> bit worried about the cross pointers. I am thinking to kill the triad.

Now that you've done this, I've respun the patch.  I also had to remove
two consts - the value returns by DW_STRING should be const in an ideal
world, but that won't work until the symbol and type names are const
also.  I also re-ran tests, just in case - no regressions.

FYI, I just tested this with straight C - debug information for glibc's
libc.so.6 (also with -readnow).  It's good for 10MB savings out of a
total of 120MB used.  Not bad.

Is this version OK?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

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

	* dwarf2read.c: Add comment describing memory lifetimes.
	(dwarf2_add_field, dwarf2_add_member_fn, read_structure_scope)
	(read_enumeration, new_symbol): Don't use obsavestring.

Index: dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.133
diff -u -p -r1.133 dwarf2read.c
--- dwarf2read.c	7 Feb 2004 23:13:47 -0000	1.133
+++ dwarf2read.c	8 Feb 2004 04:31:38 -0000
@@ -55,6 +55,16 @@
 #define DWARF2_REG_TO_REGNUM(REG) (REG)
 #endif
 
+/* A note on memory usage: at the present time, this code reads the debug
+   info sections into the objfile's objfile_obstack.  A definite improvement
+   for startup time, on platforms which do not emit relocations for debug
+   sections, would be to use mmap instead.
+
+   In either case, the sections should remain loaded until the objfile is
+   released, and pointers into the section data can be used for any other
+   data associated to the objfile (symbol names, type names, location expressions
+   to name a few).  */
+
 #if 0
 /* .debug_info header for a compilation unit
    Because of alignment constraints, this structure has padding and cannot
@@ -2665,8 +2675,7 @@ dwarf2_add_field (struct field_info *fip
       attr = dwarf2_attr (die, DW_AT_name, cu);
       if (attr && DW_STRING (attr))
 	fieldname = DW_STRING (attr);
-      fp->name = obsavestring (fieldname, strlen (fieldname),
-			       &objfile->objfile_obstack);
+      fp->name = fieldname;
 
       /* Change accessibility for artificial fields (e.g. virtual table
          pointer or virtual base class pointer) to private.  */
@@ -2697,11 +2706,9 @@ dwarf2_add_field (struct field_info *fip
       /* Get physical name.  */
       physname = dwarf2_linkage_name (die, cu);
 
-      SET_FIELD_PHYSNAME (*fp, obsavestring (physname, strlen (physname),
-					     &objfile->objfile_obstack));
+      SET_FIELD_PHYSNAME (*fp, physname ? physname : "");
       FIELD_TYPE (*fp) = die_type (die, cu);
-      FIELD_NAME (*fp) = obsavestring (fieldname, strlen (fieldname),
-				       &objfile->objfile_obstack);
+      FIELD_NAME (*fp) = fieldname;
     }
   else if (die->tag == DW_TAG_inheritance)
     {
@@ -2869,8 +2876,7 @@ dwarf2_add_member_fn (struct field_info 
 
   /* Fill in the member function field info.  */
   fnp = &new_fnfield->fnfield;
-  fnp->physname = obsavestring (physname, strlen (physname),
-				&objfile->objfile_obstack);
+  fnp->physname = physname ? physname : "";
   fnp->type = alloc_type (objfile);
   if (die->type && TYPE_CODE (die->type) == TYPE_CODE_FUNC)
     {
@@ -3001,7 +3007,7 @@ read_structure_scope (struct die_info *d
   struct objfile *objfile = cu->objfile;
   struct type *type;
   struct attribute *attr;
-  const char *name = NULL;
+  char *name = NULL;
   const char *previous_prefix = processing_current_prefix;
   struct cleanup *back_to = NULL;
   /* This says whether or not we want to try to update the structure's
@@ -3046,8 +3052,7 @@ read_structure_scope (struct die_info *d
 	}
       else
 	{
-	  TYPE_TAG_NAME (type) = obsavestring (name, strlen (name),
-					       &objfile->objfile_obstack);
+	  TYPE_TAG_NAME (type) = name;
 	  need_to_update_name = (cu->language == language_cplus);
 	}
     }
@@ -3252,7 +3257,7 @@ read_enumeration (struct die_info *die, 
   attr = dwarf2_attr (die, DW_AT_name, cu);
   if (attr && DW_STRING (attr))
     {
-      const char *name = DW_STRING (attr);
+      char *name = DW_STRING (attr);
 
       if (processing_has_namespace_info)
 	{
@@ -3263,10 +3268,7 @@ read_enumeration (struct die_info *die, 
 					   name);
 	}
       else
-	{
-	  TYPE_TAG_NAME (type) = obsavestring (name, strlen (name),
-					       &objfile->objfile_obstack);
-	}
+	TYPE_TAG_NAME (type) = name;
     }
 
   attr = dwarf2_attr (die, DW_AT_byte_size, cu);
@@ -5678,11 +5680,8 @@ new_symbol (struct die_info *die, struct
 		{
 		  /* FIXME: carlton/2003-11-10: Should this use
 		     SYMBOL_SET_NAMES instead?  (The same problem also
-		     arises a further down in the function.)  */
-		  SYMBOL_LINKAGE_NAME (sym)
-		    = obsavestring (TYPE_TAG_NAME (type),
-				    strlen (TYPE_TAG_NAME (type)),
-				    &objfile->objfile_obstack);
+		     arises further down in this function.)  */
+		  SYMBOL_LINKAGE_NAME (sym) = TYPE_TAG_NAME (type);
 		}
 	    }
 
@@ -5714,10 +5713,7 @@ new_symbol (struct die_info *die, struct
 		*typedef_sym = *sym;
 		SYMBOL_DOMAIN (typedef_sym) = VAR_DOMAIN;
 		if (TYPE_NAME (SYMBOL_TYPE (sym)) == 0)
-		  TYPE_NAME (SYMBOL_TYPE (sym)) =
-		    obsavestring (SYMBOL_NATURAL_NAME (sym),
-				  strlen (SYMBOL_NATURAL_NAME (sym)),
-				  &objfile->objfile_obstack);
+		  TYPE_NAME (SYMBOL_TYPE (sym)) = SYMBOL_NATURAL_NAME (sym);
 		add_symbol_to_list (typedef_sym, list_to_add);
 	      }
 	  }

Re: RFA: Don't use obsavestring in dwarf2read

[Elena Zannoni]

Daniel Jacobowitz writes:
 >  
 > +/* A note on memory usage: at the present time, this code reads the debug
 > +   info sections into the objfile's objfile_obstack.  A definite improvement
 > +   for startup time, on platforms which do not emit relocations for debug
 > +   sections, would be to use mmap instead.
 > +
 > +   In either case, the sections should remain loaded until the objfile is
 > +   released, and pointers into the section data can be used for any other
 > +   data associated to the objfile (symbol names, type names, location expressions
 > +   to name a few).  */
 > +


There is a similar comment right before the dwarf2_pinfo structure,
can you somehow either unify the two or 'link' them together? Both peices
of information should be available together.  Actually having this
comment just before an ifdeffed out section of code, makes me wonder
if it won't be overlooked.

 >  #if 0
 >  /* .debug_info header for a compilation unit
 >     Because of alignment constraints, this structure has padding and cannot



 > @@ -2665,8 +2675,7 @@ dwarf2_add_field (struct field_info *fip
 >        attr = dwarf2_attr (die, DW_AT_name, cu);
 >        if (attr && DW_STRING (attr))
 >  	fieldname = DW_STRING (attr);
 > -      fp->name = obsavestring (fieldname, strlen (fieldname),
 > -			       &objfile->objfile_obstack);
 > +      fp->name = fieldname;
 >  

Can you please add a clear comment above the assignment about filename being on the obstack?

 >        /* Change accessibility for artificial fields (e.g. virtual table
 >           pointer or virtual base class pointer) to private.  */
 > @@ -2697,11 +2706,9 @@ dwarf2_add_field (struct field_info *fip
 >        /* Get physical name.  */
 >        physname = dwarf2_linkage_name (die, cu);
 >  
 > -      SET_FIELD_PHYSNAME (*fp, obsavestring (physname, strlen (physname),
 > -					     &objfile->objfile_obstack));
 > +      SET_FIELD_PHYSNAME (*fp, physname ? physname : "");


same here

 >        FIELD_TYPE (*fp) = die_type (die, cu);
 > -      FIELD_NAME (*fp) = obsavestring (fieldname, strlen (fieldname),
 > -				       &objfile->objfile_obstack);
 > +      FIELD_NAME (*fp) = fieldname;

and here

 >      }
 >    else if (die->tag == DW_TAG_inheritance)
 >      {
 > @@ -2869,8 +2876,7 @@ dwarf2_add_member_fn (struct field_info 
 >  
 >    /* Fill in the member function field info.  */
 >    fnp = &new_fnfield->fnfield;
 > -  fnp->physname = obsavestring (physname, strlen (physname),
 > -				&objfile->objfile_obstack);
 > +  fnp->physname = physname ? physname : "";

and here .... and everywhere else?


otherwise ok.

Re: RFA: Don't use obsavestring in dwarf2read

[Daniel Jacobowitz]

Sorry 'bout the delay getting back to this... I got distracted by
working on intercu and thought I'd already done it.

On Mon, Feb 16, 2004 at 10:01:04AM -0500, Elena Zannoni wrote:
> There is a similar comment right before the dwarf2_pinfo structure,
> can you somehow either unify the two or 'link' them together? Both peices
> of information should be available together.  Actually having this
> comment just before an ifdeffed out section of code, makes me wonder
> if it won't be overlooked.

I've moved it up above the first non-#include (which is a #define) so
that should be a little clearer.  I also moved the relevant bits of the
dwarf2_pinfo comment up to join it.

> Can you please add a clear comment above the assignment about filename being on the obstack?

Sure.

This is what I've committed to HEAD only - just comment changes from
the version you OK'd.  Let me know if the new comments are still
lacking.  Still no testsuite result changes on i386-linux, as expected.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

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

	* dwarf2read.c: Add comment describing memory lifetimes.
	(struct dwarf2_pinfo): Update comment.
	(dwarf2_add_field, dwarf2_add_member_fn, read_structure_scope)
	(read_enumeration, new_symbol): Don't use obsavestring.

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	5 Mar 2004 03:17:40 -0000
@@ -50,6 +50,20 @@
 #include "gdb_assert.h"
 #include <sys/types.h>
 
+/* A note on memory usage for this file.
+   
+   At the present time, this code reads the debug info sections into
+   the objfile's objfile_obstack.  A definite improvement for startup
+   time, on platforms which do not emit relocations for debug
+   sections, would be to use mmap instead.  The object's complete
+   debug information is loaded into memory, partly to simplify
+   absolute DIE references.
+
+   Whether using obstacks or mmap, the sections should remain loaded
+   until the objfile is released, and pointers into the section data
+   can be used for any other data associated to the objfile (symbol
+   names, type names, location expressions to name a few).  */
+
 #ifndef DWARF2_REG_TO_REGNUM
 #define DWARF2_REG_TO_REGNUM(REG) (REG)
 #endif
@@ -444,12 +458,11 @@ static int isreg;		/* Object lives in re
 
 /* We put a pointer to this structure in the read_symtab_private field
    of the psymtab.
-   The complete dwarf information for an objfile is kept in the
-   objfile_obstack, so that absolute die references can be handled.
+
    Most of the information in this structure is related to an entire
-   object file and could be passed via the sym_private field of the objfile.
-   It is however conceivable that dwarf2 might not be the only type
-   of symbols read from an object file.  */
+   object file and could be passed via the sym_private field of the
+   objfile.  It is possible to have both dwarf2 and some other form
+   of debug symbols in one object file.  */
 
 struct dwarf2_pinfo
   {
@@ -2664,8 +2677,10 @@ dwarf2_add_field (struct field_info *fip
       attr = dwarf2_attr (die, DW_AT_name, cu);
       if (attr && DW_STRING (attr))
 	fieldname = DW_STRING (attr);
-      fp->name = obsavestring (fieldname, strlen (fieldname),
-			       &objfile->objfile_obstack);
+
+      /* The name is already allocated along with this objfile, so we don't
+	 need to duplicate it for the type.  */
+      fp->name = fieldname;
 
       /* Change accessibility for artificial fields (e.g. virtual table
          pointer or virtual base class pointer) to private.  */
@@ -2696,11 +2711,11 @@ dwarf2_add_field (struct field_info *fip
       /* Get physical name.  */
       physname = dwarf2_linkage_name (die, cu);
 
-      SET_FIELD_PHYSNAME (*fp, obsavestring (physname, strlen (physname),
-					     &objfile->objfile_obstack));
+      /* The name is already allocated along with this objfile, so we don't
+	 need to duplicate it for the type.  */
+      SET_FIELD_PHYSNAME (*fp, physname ? physname : "");
       FIELD_TYPE (*fp) = die_type (die, cu);
-      FIELD_NAME (*fp) = obsavestring (fieldname, strlen (fieldname),
-				       &objfile->objfile_obstack);
+      FIELD_NAME (*fp) = fieldname;
     }
   else if (die->tag == DW_TAG_inheritance)
     {
@@ -2868,8 +2883,9 @@ dwarf2_add_member_fn (struct field_info 
 
   /* Fill in the member function field info.  */
   fnp = &new_fnfield->fnfield;
-  fnp->physname = obsavestring (physname, strlen (physname),
-				&objfile->objfile_obstack);
+  /* The name is already allocated along with this objfile, so we don't
+     need to duplicate it for the type.  */
+  fnp->physname = physname ? physname : "";
   fnp->type = alloc_type (objfile);
   if (die->type && TYPE_CODE (die->type) == TYPE_CODE_FUNC)
     {
@@ -3000,7 +3016,7 @@ read_structure_scope (struct die_info *d
   struct objfile *objfile = cu->objfile;
   struct type *type;
   struct attribute *attr;
-  const char *name = NULL;
+  char *name = NULL;
   const char *previous_prefix = processing_current_prefix;
   struct cleanup *back_to = NULL;
   /* This says whether or not we want to try to update the structure's
@@ -3045,8 +3061,9 @@ read_structure_scope (struct die_info *d
 	}
       else
 	{
-	  TYPE_TAG_NAME (type) = obsavestring (name, strlen (name),
-					       &objfile->objfile_obstack);
+	  /* The name is already allocated along with this objfile, so
+	     we don't need to duplicate it for the type.  */
+	  TYPE_TAG_NAME (type) = name;
 	  need_to_update_name = (cu->language == language_cplus);
 	}
     }
@@ -3251,7 +3268,7 @@ read_enumeration (struct die_info *die, 
   attr = dwarf2_attr (die, DW_AT_name, cu);
   if (attr && DW_STRING (attr))
     {
-      const char *name = DW_STRING (attr);
+      char *name = DW_STRING (attr);
 
       if (processing_has_namespace_info)
 	{
@@ -3263,8 +3280,9 @@ read_enumeration (struct die_info *die, 
 	}
       else
 	{
-	  TYPE_TAG_NAME (type) = obsavestring (name, strlen (name),
-					       &objfile->objfile_obstack);
+	  /* The name is already allocated along with this objfile, so
+	     we don't need to duplicate it for the type.  */
+	  TYPE_TAG_NAME (type) = name;
 	}
     }
 
@@ -5677,11 +5695,11 @@ new_symbol (struct die_info *die, struct
 		{
 		  /* FIXME: carlton/2003-11-10: Should this use
 		     SYMBOL_SET_NAMES instead?  (The same problem also
-		     arises a further down in the function.)  */
-		  SYMBOL_LINKAGE_NAME (sym)
-		    = obsavestring (TYPE_TAG_NAME (type),
-				    strlen (TYPE_TAG_NAME (type)),
-				    &objfile->objfile_obstack);
+		     arises further down in this function.)  */
+		  /* The type's name is already allocated along with
+		     this objfile, so we don't need to duplicate it
+		     for the symbol.  */
+		  SYMBOL_LINKAGE_NAME (sym) = TYPE_TAG_NAME (type);
 		}
 	    }
 
@@ -5712,11 +5730,11 @@ new_symbol (struct die_info *die, struct
 				 sizeof (struct symbol));
 		*typedef_sym = *sym;
 		SYMBOL_DOMAIN (typedef_sym) = VAR_DOMAIN;
+		/* The symbol's name is already allocated along with
+		   this objfile, so we don't need to duplicate it for
+		   the type.  */
 		if (TYPE_NAME (SYMBOL_TYPE (sym)) == 0)
-		  TYPE_NAME (SYMBOL_TYPE (sym)) =
-		    obsavestring (SYMBOL_NATURAL_NAME (sym),
-				  strlen (SYMBOL_NATURAL_NAME (sym)),
-				  &objfile->objfile_obstack);
+		  TYPE_NAME (SYMBOL_TYPE (sym)) = SYMBOL_NATURAL_NAME (sym);
 		add_symbol_to_list (typedef_sym, list_to_add);
 	      }
 	  }

Re: RFA: Don't use obsavestring in dwarf2read

[Daniel Jacobowitz]

Sorry 'bout the delay getting back to this... I got distracted by
working on intercu and thought I'd already done it.

On Mon, Feb 16, 2004 at 10:01:04AM -0500, Elena Zannoni wrote:
> There is a similar comment right before the dwarf2_pinfo structure,
> can you somehow either unify the two or 'link' them together? Both peices
> of information should be available together.  Actually having this
> comment just before an ifdeffed out section of code, makes me wonder
> if it won't be overlooked.

I've moved it up above the first non-#include (which is a #define) so
that should be a little clearer.  I also moved the relevant bits of the
dwarf2_pinfo comment up to join it.

> Can you please add a clear comment above the assignment about filename being on the obstack?

Sure.

This is what I've committed to HEAD only - just comment changes from
the version you OK'd.  Let me know if the new comments are still
lacking.  Still no testsuite result changes on i386-linux, as expected.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

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

	* dwarf2read.c: Add comment describing memory lifetimes.
	(struct dwarf2_pinfo): Update comment.
	(dwarf2_add_field, dwarf2_add_member_fn, read_structure_scope)
	(read_enumeration, new_symbol): Don't use obsavestring.

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	5 Mar 2004 03:17:40 -0000
@@ -50,6 +50,20 @@
 #include "gdb_assert.h"
 #include <sys/types.h>
 
+/* A note on memory usage for this file.
+   
+   At the present time, this code reads the debug info sections into
+   the objfile's objfile_obstack.  A definite improvement for startup
+   time, on platforms which do not emit relocations for debug
+   sections, would be to use mmap instead.  The object's complete
+   debug information is loaded into memory, partly to simplify
+   absolute DIE references.
+
+   Whether using obstacks or mmap, the sections should remain loaded
+   until the objfile is released, and pointers into the section data
+   can be used for any other data associated to the objfile (symbol
+   names, type names, location expressions to name a few).  */
+
 #ifndef DWARF2_REG_TO_REGNUM
 #define DWARF2_REG_TO_REGNUM(REG) (REG)
 #endif
@@ -444,12 +458,11 @@ static int isreg;		/* Object lives in re
 
 /* We put a pointer to this structure in the read_symtab_private field
    of the psymtab.
-   The complete dwarf information for an objfile is kept in the
-   objfile_obstack, so that absolute die references can be handled.
+
    Most of the information in this structure is related to an entire
-   object file and could be passed via the sym_private field of the objfile.
-   It is however conceivable that dwarf2 might not be the only type
-   of symbols read from an object file.  */
+   object file and could be passed via the sym_private field of the
+   objfile.  It is possible to have both dwarf2 and some other form
+   of debug symbols in one object file.  */
 
 struct dwarf2_pinfo
   {
@@ -2664,8 +2677,10 @@ dwarf2_add_field (struct field_info *fip
       attr = dwarf2_attr (die, DW_AT_name, cu);
       if (attr && DW_STRING (attr))
 	fieldname = DW_STRING (attr);
-      fp->name = obsavestring (fieldname, strlen (fieldname),
-			       &objfile->objfile_obstack);
+
+      /* The name is already allocated along with this objfile, so we don't
+	 need to duplicate it for the type.  */
+      fp->name = fieldname;
 
       /* Change accessibility for artificial fields (e.g. virtual table
          pointer or virtual base class pointer) to private.  */
@@ -2696,11 +2711,11 @@ dwarf2_add_field (struct field_info *fip
       /* Get physical name.  */
       physname = dwarf2_linkage_name (die, cu);
 
-      SET_FIELD_PHYSNAME (*fp, obsavestring (physname, strlen (physname),
-					     &objfile->objfile_obstack));
+      /* The name is already allocated along with this objfile, so we don't
+	 need to duplicate it for the type.  */
+      SET_FIELD_PHYSNAME (*fp, physname ? physname : "");
       FIELD_TYPE (*fp) = die_type (die, cu);
-      FIELD_NAME (*fp) = obsavestring (fieldname, strlen (fieldname),
-				       &objfile->objfile_obstack);
+      FIELD_NAME (*fp) = fieldname;
     }
   else if (die->tag == DW_TAG_inheritance)
     {
@@ -2868,8 +2883,9 @@ dwarf2_add_member_fn (struct field_info 
 
   /* Fill in the member function field info.  */
   fnp = &new_fnfield->fnfield;
-  fnp->physname = obsavestring (physname, strlen (physname),
-				&objfile->objfile_obstack);
+  /* The name is already allocated along with this objfile, so we don't
+     need to duplicate it for the type.  */
+  fnp->physname = physname ? physname : "";
   fnp->type = alloc_type (objfile);
   if (die->type && TYPE_CODE (die->type) == TYPE_CODE_FUNC)
     {
@@ -3000,7 +3016,7 @@ read_structure_scope (struct die_info *d
   struct objfile *objfile = cu->objfile;
   struct type *type;
   struct attribute *attr;
-  const char *name = NULL;
+  char *name = NULL;
   const char *previous_prefix = processing_current_prefix;
   struct cleanup *back_to = NULL;
   /* This says whether or not we want to try to update the structure's
@@ -3045,8 +3061,9 @@ read_structure_scope (struct die_info *d
 	}
       else
 	{
-	  TYPE_TAG_NAME (type) = obsavestring (name, strlen (name),
-					       &objfile->objfile_obstack);
+	  /* The name is already allocated along with this objfile, so
+	     we don't need to duplicate it for the type.  */
+	  TYPE_TAG_NAME (type) = name;
 	  need_to_update_name = (cu->language == language_cplus);
 	}
     }
@@ -3251,7 +3268,7 @@ read_enumeration (struct die_info *die, 
   attr = dwarf2_attr (die, DW_AT_name, cu);
   if (attr && DW_STRING (attr))
     {
-      const char *name = DW_STRING (attr);
+      char *name = DW_STRING (attr);
 
       if (processing_has_namespace_info)
 	{
@@ -3263,8 +3280,9 @@ read_enumeration (struct die_info *die, 
 	}
       else
 	{
-	  TYPE_TAG_NAME (type) = obsavestring (name, strlen (name),
-					       &objfile->objfile_obstack);
+	  /* The name is already allocated along with this objfile, so
+	     we don't need to duplicate it for the type.  */
+	  TYPE_TAG_NAME (type) = name;
 	}
     }
 
@@ -5677,11 +5695,11 @@ new_symbol (struct die_info *die, struct
 		{
 		  /* FIXME: carlton/2003-11-10: Should this use
 		     SYMBOL_SET_NAMES instead?  (The same problem also
-		     arises a further down in the function.)  */
-		  SYMBOL_LINKAGE_NAME (sym)
-		    = obsavestring (TYPE_TAG_NAME (type),
-				    strlen (TYPE_TAG_NAME (type)),
-				    &objfile->objfile_obstack);
+		     arises further down in this function.)  */
+		  /* The type's name is already allocated along with
+		     this objfile, so we don't need to duplicate it
+		     for the symbol.  */
+		  SYMBOL_LINKAGE_NAME (sym) = TYPE_TAG_NAME (type);
 		}
 	    }
 
@@ -5712,11 +5730,11 @@ new_symbol (struct die_info *die, struct
 				 sizeof (struct symbol));
 		*typedef_sym = *sym;
 		SYMBOL_DOMAIN (typedef_sym) = VAR_DOMAIN;
+		/* The symbol's name is already allocated along with
+		   this objfile, so we don't need to duplicate it for
+		   the type.  */
 		if (TYPE_NAME (SYMBOL_TYPE (sym)) == 0)
-		  TYPE_NAME (SYMBOL_TYPE (sym)) =
-		    obsavestring (SYMBOL_NATURAL_NAME (sym),
-				  strlen (SYMBOL_NATURAL_NAME (sym)),
-				  &objfile->objfile_obstack);
+		  TYPE_NAME (SYMBOL_TYPE (sym)) = SYMBOL_NATURAL_NAME (sym);
 		add_symbol_to_list (typedef_sym, list_to_add);
 	      }
 	  }

Re: RFA: Don't use obsavestring in dwarf2read

[Michael Elizabeth Chastain]

Proofread, not tested.  Looks okay to me.

Michael C

===

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

	* dwarf2read.c: Add comment describing memory lifetimes.
	(dwarf2_add_field, dwarf2_add_member_fn, read_structure_scope)
	(read_enumeration, new_symbol): Don't use obsavestring.

Re: RFA: Don't use obsavestring in dwarf2read

[Michael Elizabeth Chastain]

Lightly proof-read, looks okay from my vantage point.

Michael C

===

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

	* dwarf2read.c: Add comment describing memory lifetimes.
	(dwarf2_add_field, dwarf2_add_member_fn, read_structure_scope)
	(read_enumeration, new_symbol): Don't use obsavestring.

Re: RFA: Don't use obsavestring in dwarf2read

[Michael Elizabeth Chastain]

I proofread this.  I didn't think it worthwhile to run the test suite,
because almost all tests are simple "load file / run / exit" that would
not catch memory lifetime bugs.

I have one concern.  read_string and read_indirect_string return NULL to
indicate a zero-length string.  obstack_savestring understands this and
copies an empty string "" into the obstack.  In most of your changes,
the string pointer is protected against being NULL, but in other
changes, it is possible for the string pointer to be NULL.  If the
string is NULL, then your code will produce slightly different results
than previously, which is a bit scary in the symtab reader.

So could you do this in two places:

  SET_FIELD_PHYSNAME (*fp, physname ? physname : "");

  fnp->physname = physname ? physname : "";

Other than that, it looks okay to me.  I stared at it for a while to
check that everything is coming from psymbol_obstack and symbol_obstack,
not dwarf2_tmp_obstack or alloca'd memory.

Michael C