Avoid obstack_free in cp-namespace.c

Avoid obstack_free in cp-namespace.c

[Daniel Jacobowitz]

Hi David,

What do you think of this change?  It makes the assumption that
lookup_block_symbol will not allocate anything on the objfile obstack, which
is no longer true in a patch I'm testing.  I really dislike obstack_free for
this exact reason.

[For the curious I've audited the remaining uses of obstack_free in GDB. 
The ones in jv-lang.c and stabsread.c are suspicious but seem to be OK, and
the rest are fine except for this one - they release whole obstacks.]

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

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

	* cp-namespace.c (check_one_possible_namespace_symbol): Don't use
	obstack_free.

Index: cp-namespace.c
===================================================================
RCS file: /big/fsf/rsync/src-cvs/src/gdb/cp-namespace.c,v
retrieving revision 1.11
diff -u -p -r1.11 cp-namespace.c
--- cp-namespace.c	7 Feb 2004 23:13:47 -0000	1.11
+++ cp-namespace.c	9 Feb 2004 21:03:31 -0000
@@ -783,14 +783,20 @@ check_one_possible_namespace_symbol (con
 				     struct objfile *objfile)
 {
   struct block *block = get_possible_namespace_block (objfile);
-  char *name_copy = obsavestring (name, len, &objfile->objfile_obstack);
-  struct symbol *sym = lookup_block_symbol (block, name_copy, NULL,
-					    VAR_DOMAIN);
+  char *name_copy = alloca (len + 1);
+  struct symbol *sym;
+
+  memcpy (name_copy, name, len);
+  name_copy[len] = '\0';
+  sym = lookup_block_symbol (block, name_copy, NULL, VAR_DOMAIN);
 
   if (sym == NULL)
     {
-      struct type *type = init_type (TYPE_CODE_NAMESPACE, 0, 0,
-				     name_copy, objfile);
+      struct type *type;
+      name_copy = obsavestring (name, len, &objfile->objfile_obstack);
+
+      type = init_type (TYPE_CODE_NAMESPACE, 0, 0, name_copy, objfile);
+
       TYPE_TAG_NAME (type) = TYPE_NAME (type);
 
       sym = obstack_alloc (&objfile->objfile_obstack, sizeof (struct symbol));
@@ -806,11 +812,7 @@ check_one_possible_namespace_symbol (con
       return 0;
     }
   else
-    {
-      obstack_free (&objfile->objfile_obstack, name_copy);
-
-      return 1;
-    }
+    return 1;
 }
 
 /* Look for a symbol named NAME in all the possible namespace blocks.

Re: Avoid obstack_free in cp-namespace.c

[David Carlton]

On Mon, 9 Feb 2004 16:10:10 -0500, Daniel Jacobowitz <drow@mvista.com> said:

> What do you think of this change?  It makes the assumption that
> lookup_block_symbol will not allocate anything on the objfile
> obstack, which is no longer true in a patch I'm testing.  I really
> dislike obstack_free for this exact reason.

Makes sense to me.

David Carlton
carlton@kealia.com

Re: Avoid obstack_free in cp-namespace.c

[Daniel Jacobowitz]

On Mon, Feb 09, 2004 at 01:15:28PM -0800, David Carlton wrote:
> On Mon, 9 Feb 2004 16:10:10 -0500, Daniel Jacobowitz <drow@mvista.com> said:
> 
> > What do you think of this change?  It makes the assumption that
> > lookup_block_symbol will not allocate anything on the objfile
> > obstack, which is no longer true in a patch I'm testing.  I really
> > dislike obstack_free for this exact reason.
> 
> Makes sense to me.

Checked in.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: Avoid obstack_free in cp-namespace.c

[Elena Zannoni]

Daniel Jacobowitz writes:
 > 
 > [For the curious I've audited the remaining uses of obstack_free in GDB. 
 > The ones in jv-lang.c and stabsread.c are suspicious but seem to be OK, and
 > the rest are fine except for this one - they release whole obstacks.]

I already did this in my cleanups. For the curious, you might as well
explain the whole story. The obstack_free function takes a pointer to
where in the stack it must start the deletion from (much like the
cleanup chains have a pointer to where to start the cleanups). The
obstack_free's in jv-lang.c and stabsread.c are done after some local
stuff is allocated to the obstack. Only the local stuff is deleted
from the obstack. Of course this assumes that nothing between the
obstack_alloc and the obstack_free allocates anything else on the same
obstack. For this reason other files use temporary obstacks. Maybe
jv-lang.c and stabsread.c could do the same. This one had the same
assumption.

Releasing the whole obstack is not safe per se. It is safe only under
the condition that we know the obstacks memory is no longer
needed. In GDB we blow away the obstacks only when we get rid of the
whole objfile(s). 

elena

Re: Avoid obstack_free in cp-namespace.c

[Eli Zaretskii]

> From: Elena Zannoni <ezannoni@redhat.com>
> Date: Mon, 9 Feb 2004 17:39:19 -0500
> 
> For the curious, you might as well explain the whole story.

Thanks.  It would be nice if this could find its way into
gdbint.texinfo, sigh...

Re: Avoid obstack_free in cp-namespace.c

[Eli Zaretskii]

> Date: Mon, 9 Feb 2004 16:10:10 -0500
> From: Daniel Jacobowitz <drow@mvista.com>
> 
> -  char *name_copy = obsavestring (name, len, &objfile->objfile_obstack);
> -  struct symbol *sym = lookup_block_symbol (block, name_copy, NULL,
> -					    VAR_DOMAIN);
> +  char *name_copy = alloca (len + 1);
> +  struct symbol *sym;
> +
> +  memcpy (name_copy, name, len);
> +  name_copy[len] = '\0';

How large can the value of `len' be?  I believe we have some coding
guidelines against using alloca for allocations that are too large.

Re: Avoid obstack_free in cp-namespace.c

[Elena Zannoni]

Eli Zaretskii writes:
 > > From: Elena Zannoni <ezannoni@redhat.com>
 > > Date: Mon, 9 Feb 2004 17:39:19 -0500
 > > 
 > > For the curious, you might as well explain the whole story.
 > 
 > Thanks.  It would be nice if this could find its way into
 > gdbint.texinfo, sigh...
 > 

your wish is my command.....

Re: Avoid obstack_free in cp-namespace.c

[Eli Zaretskii]

> From: Elena Zannoni <ezannoni@redhat.com>
> Date: Tue, 10 Feb 2004 12:25:04 -0500
>  > 
>  > Thanks.  It would be nice if this could find its way into
>  > gdbint.texinfo, sigh...
>  > 
> 
> your wish is my command.....

Please, never a command: a request.

Thanks.