[RFC/RFA] Per-objfile data mechanism

[RFC/RFA] Per-objfile data mechanism

[Mark Kettenis]

This patch adds a per-objfile data mechanism to GDB, and uses that to
store DWARF2 frame info instead of abusing objfile->sym_private.  The
latter could lead to problems on certain platforms according to
Daniel.

Comments?  OK to check this in?  Do we want this on the release branch
too?

Mark


Index: ChangeLog
from  Mark Kettenis  <kettenis@gnu.org>
 
	* objfiles.h (struct objfile): Add memebers `data' and `num_data'.
	(register_objfile_data, set_objfile_data, objfile_data): New
	prototypes.
	* objfiles.c (objfile_alloc_data, objfile_free_data): New
	prototypes.
	(allocate_objfile): Call objfile_alloc_data.
	(free_objfile): Call objfile_free_data.
	(struct objfile_data): New.
	(struct objfile_data_registration): New.
	(struct objfile_data_registry): New.
	(objfile_data_registry): New variable.
	(register_objfile_data): New function.
	(objfile_alloc_data, objfile_free_data): New functions.
	(set_objfile_data, objfile_data): New functions.
	* dwarf2-frame.c (dwarf2_frame_data): New variable.
	(dwarf2_frame_find_fde, add_fde): Use new per-objfile data mechanism.
	(_initialize_dwarf2_frame): New function and prototype.

 
Index: objfiles.h
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.h,v
retrieving revision 1.22
diff -u -p -r1.22 objfiles.h
--- objfiles.h 24 Mar 2003 03:54:48 -0000 1.22
+++ objfiles.h 13 Jul 2003 17:16:04 -0000
@@ -379,6 +379,13 @@ struct objfile
 
     void *obj_private;
 
+    /* Per objfile data-pointers required by other GDB modules.  */
+    /* FIXME: kettenis/20030711: This mechanism could replace
+       sym_stab_info, sym_private and obj_private entirely.  */
+
+    void **data;
+    unsigned num_data;
+
     /* Set of relocation offsets to apply to each section.
        Currently on the psymbol_obstack (which makes no sense, but I'm
        not sure it's harming anything).
@@ -564,6 +571,16 @@ extern struct obj_section *find_pc_sect_
 extern int in_plt_section (CORE_ADDR, char *);
 
 extern int is_in_import_list (char *, struct objfile *);
+
+/* Keep a registry of per-objfile data-pointers required by other GDB
+   modules.  */
+
+extern const struct objfile_data *register_objfile_data (void);
+extern void set_objfile_data (struct objfile *objfile,
+			      const struct objfile_data *data, void *value);
+extern void *objfile_data (struct objfile *objfile,
+			   const struct objfile_data *data);
+\f
 
 /* Traverse all object files.  ALL_OBJFILES_SAFE works even if you delete
    the objfile during the traversal.  */
Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.33
diff -u -p -r1.33 objfiles.c
--- objfiles.c 11 Jun 2003 23:29:47 -0000 1.33
+++ objfiles.c 13 Jul 2003 17:16:05 -0000
@@ -34,6 +34,7 @@
 #include "target.h"
 #include "bcache.h"
 
+#include "gdb_assert.h"
 #include <sys/types.h>
 #include "gdb_stat.h"
 #include <fcntl.h>
@@ -61,6 +62,9 @@ static void *map_to_file (int);
 
 static void add_to_objfile_sections (bfd *, sec_ptr, void *);
 
+static void objfile_alloc_data (struct objfile *objfile);
+static void objfile_free_data (struct objfile *objfile);
+
 /* Externally visible variables that are owned by this module.
    See declarations in objfile.h for more info. */
 
@@ -302,6 +306,8 @@ allocate_objfile (bfd *abfd, int flags)
       terminate_minimal_symbol_table (objfile);
     }
 
+  objfile_alloc_data (objfile);
+
   /* Update the per-objfile information that comes from the bfd, ensuring
      that any data that is reference is saved in the per-objfile data
      region. */
@@ -561,6 +567,7 @@ free_objfile (struct objfile *objfile)
 
   if (objfile != NULL)
     {
+      objfile_free_data (objfile);
       if (objfile->name != NULL)
 	{
 	  xmfree (objfile->md, objfile->name);
@@ -1097,4 +1104,74 @@ is_in_import_list (char *name, struct ob
       return 1;
   return 0;
 }
+\f
+
+/* Keep a registry of per-objfile data-pointers required by other GDB
+   modules.  */
+
+struct objfile_data
+{
+  unsigned index;
+};
+
+struct objfile_data_registration
+{
+  struct objfile_data *data;
+  struct objfile_data_registration *next;
+};
+  
+struct objfile_data_registry
+{
+  struct objfile_data_registration *registrations;
+  unsigned num_registrations;
+};
+
+static struct objfile_data_registry objfile_data_registry = { NULL, 0 };
+
+const struct objfile_data *
+register_objfile_data (void)
+{
+  struct objfile_data_registration **curr;
+
+  /* Append new registration.  */
+  for (curr = &objfile_data_registry.registrations;
+       *curr != NULL; curr = &(*curr)->next);
 
+  *curr = XMALLOC (struct objfile_data_registration);
+  (*curr)->next = NULL;
+  (*curr)->data = XMALLOC (struct objfile_data);
+  (*curr)->data->index = objfile_data_registry.num_registrations++;
+
+  return (*curr)->data;
+}
+
+static void
+objfile_alloc_data (struct objfile *objfile)
+{
+  gdb_assert (objfile->data == NULL);
+  objfile->num_data = objfile_data_registry.num_registrations;
+  objfile->data = XCALLOC (objfile->num_data, void *);
+}
+
+static void
+objfile_free_data (struct objfile *objfile)
+{
+  gdb_assert (objfile->data != NULL);
+  xfree (objfile->data);
+  objfile->data = NULL;
+}
+
+void
+set_objfile_data (struct objfile *objfile, const struct objfile_data *data,
+		  void *value)
+{
+  gdb_assert (data->index < objfile->num_data);
+  objfile->data[data->index] = value;
+}
+
+void *
+objfile_data (struct objfile *objfile, const struct objfile_data *data)
+{
+  gdb_assert (data->index < objfile->num_data);
+  return objfile->data[data->index];
+}
Index: dwarf2-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2-frame.c,v
retrieving revision 1.8
diff -u -p -r1.8 dwarf2-frame.c
--- dwarf2-frame.c 11 Jul 2003 16:22:17 -0000 1.8
+++ dwarf2-frame.c 13 Jul 2003 17:16:21 -0000
@@ -785,6 +785,8 @@ struct comp_unit
   bfd_vma dbase;
 };
 
+const struct objfile_data *dwarf2_frame_data;
+
 static unsigned int
 read_1_byte (bfd *bfd, char *buf)
 {
@@ -1029,7 +1031,7 @@ dwarf2_frame_find_fde (CORE_ADDR *pc)
 
       offset = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
       
-      fde = objfile->sym_private;
+      fde = objfile_data (objfile, dwarf2_frame_data);
       while (fde)
 	{
 	  if (*pc >= fde->initial_location + offset
@@ -1049,8 +1051,8 @@ dwarf2_frame_find_fde (CORE_ADDR *pc)
 static void
 add_fde (struct comp_unit *unit, struct dwarf2_fde *fde)
 {
-  fde->next = unit->objfile->sym_private;
-  unit->objfile->sym_private = fde;
+  fde->next = objfile_data (unit->objfile, dwarf2_frame_data);
+  set_objfile_data (unit->objfile, dwarf2_frame_data, fde);
 }
 
 #ifdef CC_HAS_LONG_LONG
@@ -1445,4 +1447,13 @@ dwarf2_build_frame_info (struct objfile 
       while (frame_ptr < unit.dwarf_frame_buffer + unit.dwarf_frame_size)
 	frame_ptr = decode_frame_entry (&unit, frame_ptr, 0);
     }
+}
+
+/* Provide a prototype to silence -Wmissing-prototypes.  */
+void _initialize_dwarf2_frame (void);
+
+void
+_initialize_dwarf2_frame (void)
+{
+  dwarf2_frame_data = register_objfile_data ();
 }

Re: [RFC/RFA] Per-objfile data mechanism

[David Carlton]

On Sun, 13 Jul 2003 19:17:04 +0200 (CEST), Mark Kettenis <kettenis@chello.nl> said:

> This patch adds a per-objfile data mechanism to GDB, and uses that to
> store DWARF2 frame info instead of abusing objfile->sym_private.  The
> latter could lead to problems on certain platforms according to
> Daniel.

What are the advantages/disadvantages of doing this as opposed to just
adding more members to struct objfile?

David Carlton
carlton@kealia.com

Re: [RFC/RFA] Per-objfile data mechanism

[Daniel Jacobowitz]

On Sun, Jul 13, 2003 at 07:17:04PM +0200, Mark Kettenis wrote:
> This patch adds a per-objfile data mechanism to GDB, and uses that to
> store DWARF2 frame info instead of abusing objfile->sym_private.  The
> latter could lead to problems on certain platforms according to
> Daniel.

Cygwin appears to be affected by this, so we should definitely reach
some conclusion before 6.0.

> Comments?  OK to check this in?  Do we want this on the release branch
> too?

The concept is nice, but I share David's concern.  The way you
implemented this, every objfile always pays the cost of the extra
storage; there's an abstraction benefit, but a memory and simplicity
penalty.  I could go either way.

We should reach a decision one way or the other, though.  I needed a
new per-objfile datum just yesterday.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFC/RFA] Per-objfile data mechanism

[David Carlton]

On Tue, 15 Jul 2003 12:17:29 -0400, Daniel Jacobowitz <drow@mvista.com> said:

> The concept is nice, but I share David's concern.

I'm not sure what my concern is; I'm just curious. :-) I guess my
inchoate attitude (as an interested observer who has a patch waiting
for approval that adds per-objfile data) is that I don't mind at all
adding new data to every objfile: there just aren't enough of them to
worry about.  The advantage of doing that as a member is that its
existence is right there in objfiles.h for anybody to look at.  The
advantage of Mark's mechanism is that, if the data is only used by
one file, then you don't have to clutter objfiles.h with it.

I was also going to write, based on a cursory misreading of Mark's
patch, that it simplified memory management in some circumstances, but
now that I look at it more closely, I think I just misread the patch.
(I may still be misreading the patch; my head is spinning with other
things.)  Would it be possible/beneficial to modify the mechanism to
provide an optional per-datum cleanup function as well?

David Carlton
carlton@kealia.com

Re: [RFC/RFA] Per-objfile data mechanism

[Andrew Cagney]

> On Sun, Jul 13, 2003 at 07:17:04PM +0200, Mark Kettenis wrote:
> 
>> This patch adds a per-objfile data mechanism to GDB, and uses that to
>> store DWARF2 frame info instead of abusing objfile->sym_private.  The
>> latter could lead to problems on certain platforms according to
>> Daniel.
> 
> 
> Cygwin appears to be affected by this, so we should definitely reach
> some conclusion before 6.0.
> 
> 
>> Comments?  OK to check this in?  Do we want this on the release branch
>> too?
> 
> 
> The concept is nice, but I share David's concern.  The way you
> implemented this, every objfile always pays the cost of the extra
> storage; there's an abstraction benefit, but a memory and simplicity
> penalty.  I could go either way.

Some how, I don't see a per-object file penalty being a problem :-)

Andrew

Re: [RFC/RFA] Per-objfile data mechanism

[Daniel Jacobowitz]

On Tue, Jul 15, 2003 at 09:48:31AM -0700, David Carlton wrote:
> On Tue, 15 Jul 2003 12:17:29 -0400, Daniel Jacobowitz <drow@mvista.com> said:
> 
> > The concept is nice, but I share David's concern.
> 
> I'm not sure what my concern is; I'm just curious. :-) I guess my
> inchoate attitude (as an interested observer who has a patch waiting
> for approval that adds per-objfile data) is that I don't mind at all
> adding new data to every objfile: there just aren't enough of them to
> worry about.  The advantage of doing that as a member is that its
> existence is right there in objfiles.h for anybody to look at.  The
> advantage of Mark's mechanism is that, if the data is only used by
> one file, then you don't have to clutter objfiles.h with it.
> 
> I was also going to write, based on a cursory misreading of Mark's
> patch, that it simplified memory management in some circumstances, but
> now that I look at it more closely, I think I just misread the patch.
> (I may still be misreading the patch; my head is spinning with other
> things.)  Would it be possible/beneficial to modify the mechanism to
> provide an optional per-datum cleanup function as well?

Sure.  And that's the best reason I've heard so far for doing it this
way.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFC/RFA] Per-objfile data mechanism

[Elena Zannoni]

Mark Kettenis writes:
 > This patch adds a per-objfile data mechanism to GDB, and uses that to
 > store DWARF2 frame info instead of abusing objfile->sym_private.  The
 > latter could lead to problems on certain platforms according to
 > Daniel.

yes, xcoffread and coffread heavily rely on the sym_private field.

 > 
 > Comments?  OK to check this in?  Do we want this on the release branch
 > too?
 > 

Seems ok. I really like the idea of getting rid of the other *_private
and stab only data stuff. 

elena


 > Mark
 > 
 > 
 > Index: ChangeLog
 > from  Mark Kettenis  <kettenis@gnu.org>
 >  
 > 	* objfiles.h (struct objfile): Add memebers `data' and `num_data'.
 > 	(register_objfile_data, set_objfile_data, objfile_data): New
 > 	prototypes.
 > 	* objfiles.c (objfile_alloc_data, objfile_free_data): New
 > 	prototypes.
 > 	(allocate_objfile): Call objfile_alloc_data.
 > 	(free_objfile): Call objfile_free_data.
 > 	(struct objfile_data): New.
 > 	(struct objfile_data_registration): New.
 > 	(struct objfile_data_registry): New.
 > 	(objfile_data_registry): New variable.
 > 	(register_objfile_data): New function.
 > 	(objfile_alloc_data, objfile_free_data): New functions.
 > 	(set_objfile_data, objfile_data): New functions.
 > 	* dwarf2-frame.c (dwarf2_frame_data): New variable.
 > 	(dwarf2_frame_find_fde, add_fde): Use new per-objfile data mechanism.
 > 	(_initialize_dwarf2_frame): New function and prototype.
 > 
 >  
 > Index: objfiles.h
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/objfiles.h,v
 > retrieving revision 1.22
 > diff -u -p -r1.22 objfiles.h
 > --- objfiles.h 24 Mar 2003 03:54:48 -0000 1.22
 > +++ objfiles.h 13 Jul 2003 17:16:04 -0000
 > @@ -379,6 +379,13 @@ struct objfile
 >  
 >      void *obj_private;
 >  
 > +    /* Per objfile data-pointers required by other GDB modules.  */
 > +    /* FIXME: kettenis/20030711: This mechanism could replace
 > +       sym_stab_info, sym_private and obj_private entirely.  */
 > +
 > +    void **data;
 > +    unsigned num_data;
 > +
 >      /* Set of relocation offsets to apply to each section.
 >         Currently on the psymbol_obstack (which makes no sense, but I'm
 >         not sure it's harming anything).
 > @@ -564,6 +571,16 @@ extern struct obj_section *find_pc_sect_
 >  extern int in_plt_section (CORE_ADDR, char *);
 >  
 >  extern int is_in_import_list (char *, struct objfile *);
 > +
 > +/* Keep a registry of per-objfile data-pointers required by other GDB
 > +   modules.  */
 > +
 > +extern const struct objfile_data *register_objfile_data (void);
 > +extern void set_objfile_data (struct objfile *objfile,
 > +			      const struct objfile_data *data, void *value);
 > +extern void *objfile_data (struct objfile *objfile,
 > +			   const struct objfile_data *data);
 > +\f
 >  
 >  /* Traverse all object files.  ALL_OBJFILES_SAFE works even if you delete
 >     the objfile during the traversal.  */
 > Index: objfiles.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/objfiles.c,v
 > retrieving revision 1.33
 > diff -u -p -r1.33 objfiles.c
 > --- objfiles.c 11 Jun 2003 23:29:47 -0000 1.33
 > +++ objfiles.c 13 Jul 2003 17:16:05 -0000
 > @@ -34,6 +34,7 @@
 >  #include "target.h"
 >  #include "bcache.h"
 >  
 > +#include "gdb_assert.h"
 >  #include <sys/types.h>
 >  #include "gdb_stat.h"
 >  #include <fcntl.h>
 > @@ -61,6 +62,9 @@ static void *map_to_file (int);
 >  
 >  static void add_to_objfile_sections (bfd *, sec_ptr, void *);
 >  
 > +static void objfile_alloc_data (struct objfile *objfile);
 > +static void objfile_free_data (struct objfile *objfile);
 > +
 >  /* Externally visible variables that are owned by this module.
 >     See declarations in objfile.h for more info. */
 >  
 > @@ -302,6 +306,8 @@ allocate_objfile (bfd *abfd, int flags)
 >        terminate_minimal_symbol_table (objfile);
 >      }
 >  
 > +  objfile_alloc_data (objfile);
 > +
 >    /* Update the per-objfile information that comes from the bfd, ensuring
 >       that any data that is reference is saved in the per-objfile data
 >       region. */
 > @@ -561,6 +567,7 @@ free_objfile (struct objfile *objfile)
 >  
 >    if (objfile != NULL)
 >      {
 > +      objfile_free_data (objfile);
 >        if (objfile->name != NULL)
 >  	{
 >  	  xmfree (objfile->md, objfile->name);
 > @@ -1097,4 +1104,74 @@ is_in_import_list (char *name, struct ob
 >        return 1;
 >    return 0;
 >  }
 > +\f
 > +
 > +/* Keep a registry of per-objfile data-pointers required by other GDB
 > +   modules.  */
 > +
 > +struct objfile_data
 > +{
 > +  unsigned index;
 > +};
 > +
 > +struct objfile_data_registration
 > +{
 > +  struct objfile_data *data;
 > +  struct objfile_data_registration *next;
 > +};
 > +  
 > +struct objfile_data_registry
 > +{
 > +  struct objfile_data_registration *registrations;
 > +  unsigned num_registrations;
 > +};
 > +
 > +static struct objfile_data_registry objfile_data_registry = { NULL, 0 };
 > +
 > +const struct objfile_data *
 > +register_objfile_data (void)
 > +{
 > +  struct objfile_data_registration **curr;
 > +
 > +  /* Append new registration.  */
 > +  for (curr = &objfile_data_registry.registrations;
 > +       *curr != NULL; curr = &(*curr)->next);
 >  
 > +  *curr = XMALLOC (struct objfile_data_registration);
 > +  (*curr)->next = NULL;
 > +  (*curr)->data = XMALLOC (struct objfile_data);
 > +  (*curr)->data->index = objfile_data_registry.num_registrations++;
 > +
 > +  return (*curr)->data;
 > +}
 > +
 > +static void
 > +objfile_alloc_data (struct objfile *objfile)
 > +{
 > +  gdb_assert (objfile->data == NULL);
 > +  objfile->num_data = objfile_data_registry.num_registrations;
 > +  objfile->data = XCALLOC (objfile->num_data, void *);
 > +}
 > +
 > +static void
 > +objfile_free_data (struct objfile *objfile)
 > +{
 > +  gdb_assert (objfile->data != NULL);
 > +  xfree (objfile->data);
 > +  objfile->data = NULL;
 > +}
 > +
 > +void
 > +set_objfile_data (struct objfile *objfile, const struct objfile_data *data,
 > +		  void *value)
 > +{
 > +  gdb_assert (data->index < objfile->num_data);
 > +  objfile->data[data->index] = value;
 > +}
 > +
 > +void *
 > +objfile_data (struct objfile *objfile, const struct objfile_data *data)
 > +{
 > +  gdb_assert (data->index < objfile->num_data);
 > +  return objfile->data[data->index];
 > +}
 > Index: dwarf2-frame.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/dwarf2-frame.c,v
 > retrieving revision 1.8
 > diff -u -p -r1.8 dwarf2-frame.c
 > --- dwarf2-frame.c 11 Jul 2003 16:22:17 -0000 1.8
 > +++ dwarf2-frame.c 13 Jul 2003 17:16:21 -0000
 > @@ -785,6 +785,8 @@ struct comp_unit
 >    bfd_vma dbase;
 >  };
 >  
 > +const struct objfile_data *dwarf2_frame_data;
 > +
 >  static unsigned int
 >  read_1_byte (bfd *bfd, char *buf)
 >  {
 > @@ -1029,7 +1031,7 @@ dwarf2_frame_find_fde (CORE_ADDR *pc)
 >  
 >        offset = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
 >        
 > -      fde = objfile->sym_private;
 > +      fde = objfile_data (objfile, dwarf2_frame_data);
 >        while (fde)
 >  	{
 >  	  if (*pc >= fde->initial_location + offset
 > @@ -1049,8 +1051,8 @@ dwarf2_frame_find_fde (CORE_ADDR *pc)
 >  static void
 >  add_fde (struct comp_unit *unit, struct dwarf2_fde *fde)
 >  {
 > -  fde->next = unit->objfile->sym_private;
 > -  unit->objfile->sym_private = fde;
 > +  fde->next = objfile_data (unit->objfile, dwarf2_frame_data);
 > +  set_objfile_data (unit->objfile, dwarf2_frame_data, fde);
 >  }
 >  
 >  #ifdef CC_HAS_LONG_LONG
 > @@ -1445,4 +1447,13 @@ dwarf2_build_frame_info (struct objfile 
 >        while (frame_ptr < unit.dwarf_frame_buffer + unit.dwarf_frame_size)
 >  	frame_ptr = decode_frame_entry (&unit, frame_ptr, 0);
 >      }
 > +}
 > +
 > +/* Provide a prototype to silence -Wmissing-prototypes.  */
 > +void _initialize_dwarf2_frame (void);
 > +
 > +void
 > +_initialize_dwarf2_frame (void)
 > +{
 > +  dwarf2_frame_data = register_objfile_data ();
 >  }

Re: [RFC/RFA] Per-objfile data mechanism

[Mark Kettenis]

   From: David Carlton <carlton@kealia.com>
   Date: Tue, 15 Jul 2003 09:48:31 -0700

   On Tue, 15 Jul 2003 12:17:29 -0400, Daniel Jacobowitz
   <drow@mvista.com> said:

   > The concept is nice, but I share David's concern.

   I was also going to write, based on a cursory misreading of Mark's
   patch, that it simplified memory management in some circumstances, but
   now that I look at it more closely, I think I just misread the patch.
   (I may still be misreading the patch; my head is spinning with other
   things.)  Would it be possible/beneficial to modify the mechanism to
   provide an optional per-datum cleanup function as well?

I quite deliberately left per-datum initializers and destructors out
to encourage the use of the per-objfile obstacks.  But they can always
be added if they're needed.

So what's the final verdict.  Should my patch go in, or do people have
concrete ideas about necessary improvements or alternative
implementations?

Mark

Re: [RFC/RFA] Per-objfile data mechanism

[David Carlton]

On Sun, 10 Aug 2003 21:03:02 +0200 (CEST), Mark Kettenis <kettenis@chello.nl> said:
> From: David Carlton <carlton@kealia.com>
> Date: Tue, 15 Jul 2003 09:48:31 -0700

>> I was also going to write, based on a cursory misreading of Mark's
>> patch, that it simplified memory management in some circumstances,
>> but now that I look at it more closely, I think I just misread the
>> patch.  (I may still be misreading the patch; my head is spinning
>> with other things.)  Would it be possible/beneficial to modify the
>> mechanism to provide an optional per-datum cleanup function as
>> well?

> I quite deliberately left per-datum initializers and destructors out
> to encourage the use of the per-objfile obstacks.  But they can
> always be added if they're needed.

The concrete reason for that suggestion is that I have a patch
awaiting review adding some per-objfile data that consists of an
expanding hash table; that can't be handled with an obstack.  In
general, I get the feeling that we're moving a bit more to data
structures that are less obstack friendly, but who knows.  Having said
that:

> So what's the final verdict.  Should my patch go in, or do people
> have concrete ideas about necessary improvements or alternative
> implementations?

I certainly wouldn't want to stand in the way of putting it in now: if
we do decide we want to add per-datum cleanup mechanisms to your
patch, we can do it just as easily after the patch has been applied as
before the patch has been applied.

David Carlton
carlton@kealia.com

Re: [RFC/RFA] Per-objfile data mechanism

[Andrew Cagney]

> 
> The concrete reason for that suggestion is that I have a patch
> awaiting review adding some per-objfile data that consists of an
> expanding hash table; that can't be handled with an obstack.  In
> general, I get the feeling that we're moving a bit more to data
> structures that are less obstack friendly, but who knows.  Having said
> that:

So I'm not the only one looking at the N different structures mapping 
onto partial symbols :-)  They appear to bloat the overhead of a partial 
symbol to the point where a more unified structure and a full symbol 
might be smaller!

I just converted gdbarch to an obstack and encountered two occasions 
where xmrealloc would have made my life a little easier.  Instead of 
proposing the use of mmalloc (and hence xmrealloc) though, I modified 
the algorithms / structures a little and avoided the problem.

Is it possible that the same situtation is being encountered here?  A 
growable hash table can be implemented without needing to reclaim memory 
- something more along the lines of a btree?

Andrew

Re: [RFC/RFA] Per-objfile data mechanism

[David Carlton]

On Tue, 12 Aug 2003 16:08:37 -0400, Andrew Cagney <ac131313@redhat.com> said:

> I just converted gdbarch to an obstack and encountered two occasions
> where xmrealloc would have made my life a little easier.  Instead of
> proposing the use of mmalloc (and hence xmrealloc) though, I modified
> the algorithms / structures a little and avoided the problem.

> Is it possible that the same situtation is being encountered here?  A
> growable hash table can be implemented without needing to reclaim
> memory - something more along the lines of a btree?

Sure, I could replace the data structure in question by a different
one which is more obstack-friendly; it would be work, and the
resulting code would initially be less reliable, but it could be done.
But I guess I don't understand why obstacks are supposed to be so
wonderful.  They're useful if you're allocating zillions of small
objects that should all disappear at the same time, but I don't see
the value of trying to fit all of our data structures into them.

David Carlton
carlton@kealia.com

Re: [RFC/RFA] Per-objfile data mechanism

[Daniel Jacobowitz]

On Sun, Aug 10, 2003 at 09:03:02PM +0200, Mark Kettenis wrote:
>    From: David Carlton <carlton@kealia.com>
>    Date: Tue, 15 Jul 2003 09:48:31 -0700
> 
>    On Tue, 15 Jul 2003 12:17:29 -0400, Daniel Jacobowitz
>    <drow@mvista.com> said:
> 
>    > The concept is nice, but I share David's concern.
> 
>    I was also going to write, based on a cursory misreading of Mark's
>    patch, that it simplified memory management in some circumstances, but
>    now that I look at it more closely, I think I just misread the patch.
>    (I may still be misreading the patch; my head is spinning with other
>    things.)  Would it be possible/beneficial to modify the mechanism to
>    provide an optional per-datum cleanup function as well?
> 
> I quite deliberately left per-datum initializers and destructors out
> to encourage the use of the per-objfile obstacks.  But they can always
> be added if they're needed.
> 
> So what's the final verdict.  Should my patch go in, or do people have
> concrete ideas about necessary improvements or alternative
> implementations?

As far as I'm concerned, it should go in.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFC/RFA] Per-objfile data mechanism

[Mark Kettenis]

   From: David Carlton <carlton@kealia.com>
   Date: Tue, 12 Aug 2003 13:51:38 -0700

   > I just converted gdbarch to an obstack and encountered two occasions
   > where xmrealloc would have made my life a little easier.  Instead of
   > proposing the use of mmalloc (and hence xmrealloc) though, I modified
   > the algorithms / structures a little and avoided the problem.

   > Is it possible that the same situtation is being encountered here?  A
   > growable hash table can be implemented without needing to reclaim
   > memory - something more along the lines of a btree?

   Sure, I could replace the data structure in question by a different
   one which is more obstack-friendly; it would be work, and the
   resulting code would initially be less reliable, but it could be done.
   But I guess I don't understand why obstacks are supposed to be so
   wonderful.  They're useful if you're allocating zillions of small
   objects that should all disappear at the same time, but I don't see
   the value of trying to fit all of our data structures into them.

OK, this discussion is moving away from the origional question.  I
concluded that we want the per-objfile data, and that, we might want a
destructor mechanism.  I checked in my origional patch, since the
destructor mechanism can easily be added later.

It is my understanding that this patch fixes some real bugs, and that
we should consider adding this to 6.0.  Having it on mainline is a
first step.

Mark


Index: ChangeLog
from  Mark Kettenis  <kettenis@gnu.org>

	* objfiles.h (struct objfile): Add memebers `data' and `num_data'.
	(register_objfile_data, set_objfile_data, objfile_data): New
	prototypes.
	* objfiles.c (objfile_alloc_data, objfile_free_data): New
	prototypes.
	(allocate_objfile): Call objfile_alloc_data.
	(free_objfile): Call objfile_free_data.
	(struct objfile_data): New.
	(struct objfile_data_registration): New.
	(struct objfile_data_registry): New.
	(objfile_data_registry): New variable.
	(register_objfile_data): New function.
	(objfile_alloc_data, objfile_free_data): New functions.
	(set_objfile_data, objfile_data): New functions.
	* dwarf2-frame.c (dwarf2_frame_data): New variable.
	(dwarf2_frame_find_fde, add_fde): Use new per-objfile data mechanism.
	(_initialize_dwarf2_frame): New function and prototype.

Index: dwarf2-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2-frame.c,v
retrieving revision 1.10
diff -u -p -r1.10 dwarf2-frame.c
--- dwarf2-frame.c 18 Jul 2003 19:59:27 -0000 1.10
+++ dwarf2-frame.c 21 Aug 2003 22:33:51 -0000
@@ -780,6 +780,8 @@ struct comp_unit
   bfd_vma dbase;
 };
 
+const struct objfile_data *dwarf2_frame_data;
+
 static unsigned int
 read_1_byte (bfd *bfd, char *buf)
 {
@@ -1024,7 +1026,7 @@ dwarf2_frame_find_fde (CORE_ADDR *pc)
 
       offset = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
       
-      fde = objfile->sym_private;
+      fde = objfile_data (objfile, dwarf2_frame_data);
       while (fde)
 	{
 	  if (*pc >= fde->initial_location + offset
@@ -1044,8 +1046,8 @@ dwarf2_frame_find_fde (CORE_ADDR *pc)
 static void
 add_fde (struct comp_unit *unit, struct dwarf2_fde *fde)
 {
-  fde->next = unit->objfile->sym_private;
-  unit->objfile->sym_private = fde;
+  fde->next = objfile_data (unit->objfile, dwarf2_frame_data);
+  set_objfile_data (unit->objfile, dwarf2_frame_data, fde);
 }
 
 #ifdef CC_HAS_LONG_LONG
@@ -1440,4 +1442,13 @@ dwarf2_build_frame_info (struct objfile 
       while (frame_ptr < unit.dwarf_frame_buffer + unit.dwarf_frame_size)
 	frame_ptr = decode_frame_entry (&unit, frame_ptr, 0);
     }
+}
+
+/* Provide a prototype to silence -Wmissing-prototypes.  */
+void _initialize_dwarf2_frame (void);
+
+void
+_initialize_dwarf2_frame (void)
+{
+  dwarf2_frame_data = register_objfile_data ();
 }
Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.33
diff -u -p -r1.33 objfiles.c
--- objfiles.c 11 Jun 2003 23:29:47 -0000 1.33
+++ objfiles.c 21 Aug 2003 22:33:51 -0000
@@ -34,6 +34,7 @@
 #include "target.h"
 #include "bcache.h"
 
+#include "gdb_assert.h"
 #include <sys/types.h>
 #include "gdb_stat.h"
 #include <fcntl.h>
@@ -61,6 +62,9 @@ static void *map_to_file (int);
 
 static void add_to_objfile_sections (bfd *, sec_ptr, void *);
 
+static void objfile_alloc_data (struct objfile *objfile);
+static void objfile_free_data (struct objfile *objfile);
+
 /* Externally visible variables that are owned by this module.
    See declarations in objfile.h for more info. */
 
@@ -302,6 +306,8 @@ allocate_objfile (bfd *abfd, int flags)
       terminate_minimal_symbol_table (objfile);
     }
 
+  objfile_alloc_data (objfile);
+
   /* Update the per-objfile information that comes from the bfd, ensuring
      that any data that is reference is saved in the per-objfile data
      region. */
@@ -561,6 +567,7 @@ free_objfile (struct objfile *objfile)
 
   if (objfile != NULL)
     {
+      objfile_free_data (objfile);
       if (objfile->name != NULL)
 	{
 	  xmfree (objfile->md, objfile->name);
@@ -1097,4 +1104,74 @@ is_in_import_list (char *name, struct ob
       return 1;
   return 0;
 }
+\f
+
+/* Keep a registry of per-objfile data-pointers required by other GDB
+   modules.  */
+
+struct objfile_data
+{
+  unsigned index;
+};
+
+struct objfile_data_registration
+{
+  struct objfile_data *data;
+  struct objfile_data_registration *next;
+};
+  
+struct objfile_data_registry
+{
+  struct objfile_data_registration *registrations;
+  unsigned num_registrations;
+};
+
+static struct objfile_data_registry objfile_data_registry = { NULL, 0 };
+
+const struct objfile_data *
+register_objfile_data (void)
+{
+  struct objfile_data_registration **curr;
+
+  /* Append new registration.  */
+  for (curr = &objfile_data_registry.registrations;
+       *curr != NULL; curr = &(*curr)->next);
 
+  *curr = XMALLOC (struct objfile_data_registration);
+  (*curr)->next = NULL;
+  (*curr)->data = XMALLOC (struct objfile_data);
+  (*curr)->data->index = objfile_data_registry.num_registrations++;
+
+  return (*curr)->data;
+}
+
+static void
+objfile_alloc_data (struct objfile *objfile)
+{
+  gdb_assert (objfile->data == NULL);
+  objfile->num_data = objfile_data_registry.num_registrations;
+  objfile->data = XCALLOC (objfile->num_data, void *);
+}
+
+static void
+objfile_free_data (struct objfile *objfile)
+{
+  gdb_assert (objfile->data != NULL);
+  xfree (objfile->data);
+  objfile->data = NULL;
+}
+
+void
+set_objfile_data (struct objfile *objfile, const struct objfile_data *data,
+		  void *value)
+{
+  gdb_assert (data->index < objfile->num_data);
+  objfile->data[data->index] = value;
+}
+
+void *
+objfile_data (struct objfile *objfile, const struct objfile_data *data)
+{
+  gdb_assert (data->index < objfile->num_data);
+  return objfile->data[data->index];
+}
Index: objfiles.h
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.h,v
retrieving revision 1.22
diff -u -p -r1.22 objfiles.h
--- objfiles.h 24 Mar 2003 03:54:48 -0000 1.22
+++ objfiles.h 21 Aug 2003 22:33:52 -0000
@@ -379,6 +379,13 @@ struct objfile
 
     void *obj_private;
 
+    /* Per objfile data-pointers required by other GDB modules.  */
+    /* FIXME: kettenis/20030711: This mechanism could replace
+       sym_stab_info, sym_private and obj_private entirely.  */
+
+    void **data;
+    unsigned num_data;
+
     /* Set of relocation offsets to apply to each section.
        Currently on the psymbol_obstack (which makes no sense, but I'm
        not sure it's harming anything).
@@ -564,6 +571,16 @@ extern struct obj_section *find_pc_sect_
 extern int in_plt_section (CORE_ADDR, char *);
 
 extern int is_in_import_list (char *, struct objfile *);
+
+/* Keep a registry of per-objfile data-pointers required by other GDB
+   modules.  */
+
+extern const struct objfile_data *register_objfile_data (void);
+extern void set_objfile_data (struct objfile *objfile,
+			      const struct objfile_data *data, void *value);
+extern void *objfile_data (struct objfile *objfile,
+			   const struct objfile_data *data);
+\f
 
 /* Traverse all object files.  ALL_OBJFILES_SAFE works even if you delete
    the objfile during the traversal.  */

Re: [RFC/RFA] Per-objfile data mechanism

[Daniel Jacobowitz]

On Fri, Aug 22, 2003 at 12:41:46AM +0200, Mark Kettenis wrote:
>    From: David Carlton <carlton@kealia.com>
>    Date: Tue, 12 Aug 2003 13:51:38 -0700
> 
>    > I just converted gdbarch to an obstack and encountered two occasions
>    > where xmrealloc would have made my life a little easier.  Instead of
>    > proposing the use of mmalloc (and hence xmrealloc) though, I modified
>    > the algorithms / structures a little and avoided the problem.
> 
>    > Is it possible that the same situtation is being encountered here?  A
>    > growable hash table can be implemented without needing to reclaim
>    > memory - something more along the lines of a btree?
> 
>    Sure, I could replace the data structure in question by a different
>    one which is more obstack-friendly; it would be work, and the
>    resulting code would initially be less reliable, but it could be done.
>    But I guess I don't understand why obstacks are supposed to be so
>    wonderful.  They're useful if you're allocating zillions of small
>    objects that should all disappear at the same time, but I don't see
>    the value of trying to fit all of our data structures into them.
> 
> OK, this discussion is moving away from the origional question.  I
> concluded that we want the per-objfile data, and that, we might want a
> destructor mechanism.  I checked in my origional patch, since the
> destructor mechanism can easily be added later.
> 
> It is my understanding that this patch fixes some real bugs, and that
> we should consider adding this to 6.0.  Having it on mainline is a
> first step.

Wandering through my todo list...

Mark, how do you feel about moving this to the branch now?  It looks
good on mainline.

> Index: ChangeLog
> from  Mark Kettenis  <kettenis@gnu.org>
> 
> 	* objfiles.h (struct objfile): Add memebers `data' and `num_data'.
> 	(register_objfile_data, set_objfile_data, objfile_data): New
> 	prototypes.
> 	* objfiles.c (objfile_alloc_data, objfile_free_data): New
> 	prototypes.
> 	(allocate_objfile): Call objfile_alloc_data.
> 	(free_objfile): Call objfile_free_data.
> 	(struct objfile_data): New.
> 	(struct objfile_data_registration): New.
> 	(struct objfile_data_registry): New.
> 	(objfile_data_registry): New variable.
> 	(register_objfile_data): New function.
> 	(objfile_alloc_data, objfile_free_data): New functions.
> 	(set_objfile_data, objfile_data): New functions.
> 	* dwarf2-frame.c (dwarf2_frame_data): New variable.
> 	(dwarf2_frame_find_fde, add_fde): Use new per-objfile data mechanism.
> 	(_initialize_dwarf2_frame): New function and prototype.
> 
> Index: dwarf2-frame.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/dwarf2-frame.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 dwarf2-frame.c
> --- dwarf2-frame.c 18 Jul 2003 19:59:27 -0000 1.10
> +++ dwarf2-frame.c 21 Aug 2003 22:33:51 -0000
> @@ -780,6 +780,8 @@ struct comp_unit
>    bfd_vma dbase;
>  };
>  
> +const struct objfile_data *dwarf2_frame_data;
> +
>  static unsigned int
>  read_1_byte (bfd *bfd, char *buf)
>  {
> @@ -1024,7 +1026,7 @@ dwarf2_frame_find_fde (CORE_ADDR *pc)
>  
>        offset = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
>        
> -      fde = objfile->sym_private;
> +      fde = objfile_data (objfile, dwarf2_frame_data);
>        while (fde)
>  	{
>  	  if (*pc >= fde->initial_location + offset
> @@ -1044,8 +1046,8 @@ dwarf2_frame_find_fde (CORE_ADDR *pc)
>  static void
>  add_fde (struct comp_unit *unit, struct dwarf2_fde *fde)
>  {
> -  fde->next = unit->objfile->sym_private;
> -  unit->objfile->sym_private = fde;
> +  fde->next = objfile_data (unit->objfile, dwarf2_frame_data);
> +  set_objfile_data (unit->objfile, dwarf2_frame_data, fde);
>  }
>  
>  #ifdef CC_HAS_LONG_LONG
> @@ -1440,4 +1442,13 @@ dwarf2_build_frame_info (struct objfile 
>        while (frame_ptr < unit.dwarf_frame_buffer + unit.dwarf_frame_size)
>  	frame_ptr = decode_frame_entry (&unit, frame_ptr, 0);
>      }
> +}
> +
> +/* Provide a prototype to silence -Wmissing-prototypes.  */
> +void _initialize_dwarf2_frame (void);
> +
> +void
> +_initialize_dwarf2_frame (void)
> +{
> +  dwarf2_frame_data = register_objfile_data ();
>  }
> Index: objfiles.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/objfiles.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 objfiles.c
> --- objfiles.c 11 Jun 2003 23:29:47 -0000 1.33
> +++ objfiles.c 21 Aug 2003 22:33:51 -0000
> @@ -34,6 +34,7 @@
>  #include "target.h"
>  #include "bcache.h"
>  
> +#include "gdb_assert.h"
>  #include <sys/types.h>
>  #include "gdb_stat.h"
>  #include <fcntl.h>
> @@ -61,6 +62,9 @@ static void *map_to_file (int);
>  
>  static void add_to_objfile_sections (bfd *, sec_ptr, void *);
>  
> +static void objfile_alloc_data (struct objfile *objfile);
> +static void objfile_free_data (struct objfile *objfile);
> +
>  /* Externally visible variables that are owned by this module.
>     See declarations in objfile.h for more info. */
>  
> @@ -302,6 +306,8 @@ allocate_objfile (bfd *abfd, int flags)
>        terminate_minimal_symbol_table (objfile);
>      }
>  
> +  objfile_alloc_data (objfile);
> +
>    /* Update the per-objfile information that comes from the bfd, ensuring
>       that any data that is reference is saved in the per-objfile data
>       region. */
> @@ -561,6 +567,7 @@ free_objfile (struct objfile *objfile)
>  
>    if (objfile != NULL)
>      {
> +      objfile_free_data (objfile);
>        if (objfile->name != NULL)
>  	{
>  	  xmfree (objfile->md, objfile->name);
> @@ -1097,4 +1104,74 @@ is_in_import_list (char *name, struct ob
>        return 1;
>    return 0;
>  }
> +\f
> +
> +/* Keep a registry of per-objfile data-pointers required by other GDB
> +   modules.  */
> +
> +struct objfile_data
> +{
> +  unsigned index;
> +};
> +
> +struct objfile_data_registration
> +{
> +  struct objfile_data *data;
> +  struct objfile_data_registration *next;
> +};
> +  
> +struct objfile_data_registry
> +{
> +  struct objfile_data_registration *registrations;
> +  unsigned num_registrations;
> +};
> +
> +static struct objfile_data_registry objfile_data_registry = { NULL, 0 };
> +
> +const struct objfile_data *
> +register_objfile_data (void)
> +{
> +  struct objfile_data_registration **curr;
> +
> +  /* Append new registration.  */
> +  for (curr = &objfile_data_registry.registrations;
> +       *curr != NULL; curr = &(*curr)->next);
>  
> +  *curr = XMALLOC (struct objfile_data_registration);
> +  (*curr)->next = NULL;
> +  (*curr)->data = XMALLOC (struct objfile_data);
> +  (*curr)->data->index = objfile_data_registry.num_registrations++;
> +
> +  return (*curr)->data;
> +}
> +
> +static void
> +objfile_alloc_data (struct objfile *objfile)
> +{
> +  gdb_assert (objfile->data == NULL);
> +  objfile->num_data = objfile_data_registry.num_registrations;
> +  objfile->data = XCALLOC (objfile->num_data, void *);
> +}
> +
> +static void
> +objfile_free_data (struct objfile *objfile)
> +{
> +  gdb_assert (objfile->data != NULL);
> +  xfree (objfile->data);
> +  objfile->data = NULL;
> +}
> +
> +void
> +set_objfile_data (struct objfile *objfile, const struct objfile_data *data,
> +		  void *value)
> +{
> +  gdb_assert (data->index < objfile->num_data);
> +  objfile->data[data->index] = value;
> +}
> +
> +void *
> +objfile_data (struct objfile *objfile, const struct objfile_data *data)
> +{
> +  gdb_assert (data->index < objfile->num_data);
> +  return objfile->data[data->index];
> +}
> Index: objfiles.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/objfiles.h,v
> retrieving revision 1.22
> diff -u -p -r1.22 objfiles.h
> --- objfiles.h 24 Mar 2003 03:54:48 -0000 1.22
> +++ objfiles.h 21 Aug 2003 22:33:52 -0000
> @@ -379,6 +379,13 @@ struct objfile
>  
>      void *obj_private;
>  
> +    /* Per objfile data-pointers required by other GDB modules.  */
> +    /* FIXME: kettenis/20030711: This mechanism could replace
> +       sym_stab_info, sym_private and obj_private entirely.  */
> +
> +    void **data;
> +    unsigned num_data;
> +
>      /* Set of relocation offsets to apply to each section.
>         Currently on the psymbol_obstack (which makes no sense, but I'm
>         not sure it's harming anything).
> @@ -564,6 +571,16 @@ extern struct obj_section *find_pc_sect_
>  extern int in_plt_section (CORE_ADDR, char *);
>  
>  extern int is_in_import_list (char *, struct objfile *);
> +
> +/* Keep a registry of per-objfile data-pointers required by other GDB
> +   modules.  */
> +
> +extern const struct objfile_data *register_objfile_data (void);
> +extern void set_objfile_data (struct objfile *objfile,
> +			      const struct objfile_data *data, void *value);
> +extern void *objfile_data (struct objfile *objfile,
> +			   const struct objfile_data *data);
> +\f
>  
>  /* Traverse all object files.  ALL_OBJFILES_SAFE works even if you delete
>     the objfile during the traversal.  */
> 

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer