[RFA,2/6] Allocate abbrev_table with new

Message ID b735c70f-8170-2ad1-ebef-04678c3a066c@ericsson.com
State New, archived
Headers

Commit Message

Simon Marchi Jan. 7, 2018, 5:28 a.m. UTC
  On 2018-01-05 07:26 PM, Tom Tromey wrote:
> This changes dwarf2read.c to allocate abbrev tables using "new", and
> then updates the users.
> 
> This is somewhat complicated because ownership rules for abbrev tables
> are obscure and involve a more than usual amount of cleanup
> manipulation.

Hi Tom,

After staring at this code for longer than I had planned, I came to the
conclusion that it would be much simpler if dwarf2_cu did not contain a
pointer to the abbrev_table.  In fact, I think that dwarf2_cu::abbrev_table
is actually a disguised function parameter that should be passed alongside
the cu, not in it.

This is made clear by the fact that we are setting cu->abbrev_table just
before calling the DIE-reading function, and reset it just after (with a
cleanup).  The dwarf2_cu::abbrev_table is only useful for the duration of
a function call, and then reset.

The dwarf2_cu structure never actually owns the abbrev_table, it's always
some frame in the call stack that does.  But they own it indirectly via
dwarf2_cu::abbrev_table and some cleanup that will free the table and
reset that field (dwarf2_free_abbrev_table, or auto_free_abbrev_table with
your patch).

The parameters for the DIE-reading operations are passed through the
die_reader_specs structure, so I think it would make sense to put the
reference to the abbrev_table there (this structure really exists to
avoid having super long parameter lists).  This way, each frame that
reads in an abbrev_table owns it through a unique_ptr and passes it to
DIE-reading functions through parameter (through die_reader_specs).  That
seems more straightforward.

Here's a patch that applies over this one, that illustrates the idea.  Let
me know what you think.

Simon


From 60ec3e20ef44be977ea402d8dfb7050317857c39 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Sat, 6 Jan 2018 22:44:29 -0500
Subject: [PATCH] Suggestion

---
 gdb/dwarf2read.c | 188 +++++++++++++++++--------------------------------------
 1 file changed, 59 insertions(+), 129 deletions(-)
  

Comments

Simon Marchi Jan. 7, 2018, 4:55 p.m. UTC | #1
On 2018-01-07 12:28 AM, Simon Marchi wrote:
> On 2018-01-05 07:26 PM, Tom Tromey wrote:
>> This changes dwarf2read.c to allocate abbrev tables using "new", and
>> then updates the users.
>>
>> This is somewhat complicated because ownership rules for abbrev tables
>> are obscure and involve a more than usual amount of cleanup
>> manipulation.
> 
> Hi Tom,
> 
> After staring at this code for longer than I had planned, I came to the
> conclusion that it would be much simpler if dwarf2_cu did not contain a
> pointer to the abbrev_table.  In fact, I think that dwarf2_cu::abbrev_table
> is actually a disguised function parameter that should be passed alongside
> the cu, not in it.
> 
> This is made clear by the fact that we are setting cu->abbrev_table just
> before calling the DIE-reading function, and reset it just after (with a
> cleanup).  The dwarf2_cu::abbrev_table is only useful for the duration of
> a function call, and then reset.
> 
> The dwarf2_cu structure never actually owns the abbrev_table, it's always
> some frame in the call stack that does.  But they own it indirectly via
> dwarf2_cu::abbrev_table and some cleanup that will free the table and
> reset that field (dwarf2_free_abbrev_table, or auto_free_abbrev_table with
> your patch).
> 
> The parameters for the DIE-reading operations are passed through the
> die_reader_specs structure, so I think it would make sense to put the
> reference to the abbrev_table there (this structure really exists to
> avoid having super long parameter lists).  This way, each frame that
> reads in an abbrev_table owns it through a unique_ptr and passes it to
> DIE-reading functions through parameter (through die_reader_specs).  That
> seems more straightforward.
> 
> Here's a patch that applies over this one, that illustrates the idea.  Let
> me know what you think.
> 
> Simon

Hi Tom,

I just pushed some dwarf2read cleanup patches I had posted a while ago, that
I wanted to push after the 8.1 branch creation.  It impacts this series a little
bit, so I thought I would help you rebase it.  Here's your original series rebased
on master:

https://github.com/simark/binutils-gdb/tree/tromey/dwarf

and here's your series plus my suggestion, rebased on master:

https://github.com/simark/binutils-gdb/tree/tromey/dwarf-plus-suggestion

Simon
  

Patch

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 7fd68c5..fb19de8 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -668,11 +668,6 @@  struct dwarf2_cu
      distinguish these in buildsym.c.  */
   struct pending **list_in_scope;

-  /* The abbrev table for this CU.
-     Normally this points to the abbrev table in the objfile.
-     But if DWO_UNIT is non-NULL this is the abbrev table in the DWO file.  */
-  struct abbrev_table *abbrev_table;
-
   /* Hash table holding all the loaded partial DIEs
      with partial_die->offset.SECT_OFF as hash.  */
   htab_t partial_dies;
@@ -774,28 +769,6 @@  struct dwarf2_cu
   unsigned int processing_has_namespace_info : 1;
 };

-static void dwarf2_free_abbrev_table (struct dwarf2_cu *);
-
-/* Free an abbrev table on destruction.  */
-
-class auto_free_abbrev_table
-{
-public:
-  auto_free_abbrev_table (struct dwarf2_cu *cu)
-    : m_cu (cu)
-  {
-  }
-
-  ~auto_free_abbrev_table ()
-  {
-    dwarf2_free_abbrev_table (m_cu);
-  }
-
-private:
-
-  struct dwarf2_cu *m_cu;
-};
-
 /* Persistent data held for a compilation unit, even when not
    processing it.  We put a pointer to this structure in the
    read_symtab_private field of the psymtab.  */
@@ -1263,6 +1236,9 @@  struct die_reader_specs

   /* The value of the DW_AT_comp_dir attribute.  */
   const char *comp_dir;
+
+  /* The abbreviation table to use when reading the DIEs.  */
+  struct abbrev_table *abbrev_table;
 };

 /* Type of function passed to init_cutu_and_read_dies, et.al.  */
@@ -1556,6 +1532,8 @@  struct abbrev_table
   struct abbrev_info **abbrevs;
 };

+typedef std::unique_ptr<struct abbrev_table> abbrev_table_up;
+
 /* Attributes have a name and a value.  */
 struct attribute
   {
@@ -1784,12 +1762,9 @@  static void dwarf2_read_symtab (struct partial_symtab *,

 static void psymtab_to_symtab_1 (struct partial_symtab *);

-static std::unique_ptr<struct abbrev_table> abbrev_table_read_table
+static abbrev_table_up abbrev_table_read_table
   (struct dwarf2_section_info *, sect_offset);

-static void dwarf2_read_abbrevs (struct dwarf2_cu *,
-				 struct dwarf2_section_info *);
-
 static unsigned int peek_abbrev_code (bfd *, const gdb_byte *);

 static struct partial_die_info *load_partial_dies
@@ -7377,7 +7352,8 @@  static void
 init_cu_die_reader (struct die_reader_specs *reader,
 		    struct dwarf2_cu *cu,
 		    struct dwarf2_section_info *section,
-		    struct dwo_file *dwo_file)
+		    struct dwo_file *dwo_file,
+		    struct abbrev_table *abbrev_table)
 {
   gdb_assert (section->readin && section->buffer != NULL);
   reader->abfd = get_section_bfd_owner (section);
@@ -7387,6 +7363,7 @@  init_cu_die_reader (struct die_reader_specs *reader,
   reader->buffer = section->buffer;
   reader->buffer_end = section->buffer + section->size;
   reader->comp_dir = NULL;
+  reader->abbrev_table = abbrev_table;
 }

 /* Subroutine of init_cutu_and_read_dies to simplify it.
@@ -7402,26 +7379,25 @@  init_cu_die_reader (struct die_reader_specs *reader,
    STUB_COMP_DIR may be non-NULL.
    *RESULT_READER,*RESULT_INFO_PTR,*RESULT_COMP_UNIT_DIE,*RESULT_HAS_CHILDREN
    are filled in with the info of the DIE from the DWO file.
-   ABBREV_TABLE_PROVIDED is non-zero if the caller of init_cutu_and_read_dies
-   provided an abbrev table to use.
-   *ABBREV_TABLE may be filled in with a new abbrev table.
+   *RESULT_DWO_ABBREV_TABLE will be filled in with the abbrev table allocated
+   from the dwo.  Since *RESULT_READER references this abbrev table, it must be
+   kept around for at least as long as *RESULT_READER.
+
    The result is non-zero if a valid (non-dummy) DIE was found.  */

 static int
 read_cutu_die_from_dwo (struct dwarf2_per_cu_data *this_cu,
 			struct dwo_unit *dwo_unit,
-			int abbrev_table_provided,
 			struct die_info *stub_comp_unit_die,
 			const char *stub_comp_dir,
 			struct die_reader_specs *result_reader,
 			const gdb_byte **result_info_ptr,
 			struct die_info **result_comp_unit_die,
 			int *result_has_children,
-			gdb::optional<auto_free_abbrev_table> *abbrev_table)
+			abbrev_table_up *result_dwo_abbrev_table)
 {
   struct objfile *objfile = dwarf2_per_objfile->objfile;
   struct dwarf2_cu *cu = this_cu->cu;
-  struct dwarf2_section_info *section;
   bfd *abfd;
   const gdb_byte *begin_info_ptr, *info_ptr;
   struct attribute *comp_dir, *stmt_list, *low_pc, *high_pc, *ranges;
@@ -7484,13 +7460,12 @@  read_cutu_die_from_dwo (struct dwarf2_per_cu_data *this_cu,

   /* Set up for reading the DWO CU/TU.  */
   cu->dwo_unit = dwo_unit;
-  section = dwo_unit->section;
+  dwarf2_section_info *section = dwo_unit->section;
   dwarf2_read_section (objfile, section);
   abfd = get_section_bfd_owner (section);
   begin_info_ptr = info_ptr = (section->buffer
 			       + to_underlying (dwo_unit->sect_off));
   dwo_abbrev_section = &dwo_unit->dwo_file->sections.abbrev;
-  init_cu_die_reader (result_reader, cu, section, dwo_unit->dwo_file);

   if (this_cu->is_debug_types)
     {
@@ -7531,22 +7506,10 @@  read_cutu_die_from_dwo (struct dwarf2_per_cu_data *this_cu,
       dwo_unit->length = get_cu_length (&cu->header);
     }

-  /* Replace the CU's original abbrev table with the DWO's.
-     Reminder: We can't read the abbrev table until we've read the header.  */
-  if (abbrev_table_provided)
-    {
-      /* Don't free the provided abbrev table, the caller of
-	 init_cutu_and_read_dies owns it.  */
-      dwarf2_read_abbrevs (cu, dwo_abbrev_section);
-      /* Ensure the DWO abbrev table gets freed.  */
-      abbrev_table->emplace (cu);
-    }
-  else
-    {
-      dwarf2_free_abbrev_table (cu);
-      dwarf2_read_abbrevs (cu, dwo_abbrev_section);
-      /* Leave any existing abbrev table cleanup as is.  */
-    }
+  *result_dwo_abbrev_table
+    = abbrev_table_read_table (dwo_abbrev_section, cu->header.abbrev_sect_off);
+  init_cu_die_reader (result_reader, cu, section, dwo_unit->dwo_file,
+		      result_dwo_abbrev_table->get ());

   /* Read in the die, but leave space to copy over the attributes
      from the stub.  This has the benefit of simplifying the rest of
@@ -7699,14 +7662,16 @@  init_tu_and_read_dwo_dies (struct dwarf2_per_cu_data *this_cu,
      abbrev table.  When reading DWOs with skeletonless TUs, all the TUs
      could share abbrev tables.  */

-  gdb::optional<auto_free_abbrev_table> abbrev_table;
+  /* The abbreviation table used by READER, this must live at least as long as
+     READER.  */
+  abbrev_table_up dwo_abbrev_table;
+
   if (read_cutu_die_from_dwo (this_cu, sig_type->dwo_unit,
-			      0 /* abbrev_table_provided */,
 			      NULL /* stub_comp_unit_die */,
 			      sig_type->dwo_unit->dwo_file->comp_dir,
 			      &reader, &info_ptr,
 			      &comp_unit_die, &has_children,
-			      &abbrev_table) == 0)
+			      &dwo_abbrev_table) == 0)
     {
       /* Dummy die.  */
       do_cleanups (cleanups);
@@ -7885,37 +7850,31 @@  init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,

   /* If we don't have them yet, read the abbrevs for this compilation unit.
      And if we need to read them now, make sure they're freed when we're
-     done.  Note that it's important that if the CU had an abbrev table
-     on entry we don't free it when we're done: Somewhere up the call stack
-     it may be in use.  */
-  gdb::optional<auto_free_abbrev_table> abbrev_table_freer;
+     done (own the table through ABBREV_TABLE_HOLDER).  */
+  abbrev_table_up abbrev_table_holder;
   if (abbrev_table != NULL)
+    gdb_assert (cu->header.abbrev_sect_off == abbrev_table->sect_off);
+  else
     {
-      gdb_assert (cu->abbrev_table == NULL);
-      gdb_assert (cu->header.abbrev_sect_off == abbrev_table->sect_off);
-      cu->abbrev_table = abbrev_table;
-    }
-  else if (cu->abbrev_table == NULL)
-    {
-      dwarf2_read_abbrevs (cu, abbrev_section);
-      abbrev_table_freer.emplace (cu);
-    }
-  else if (rereading_dwo_cu)
-    {
-      dwarf2_free_abbrev_table (cu);
-      dwarf2_read_abbrevs (cu, abbrev_section);
+      abbrev_table_holder
+	= abbrev_table_read_table (abbrev_section, cu->header.abbrev_sect_off);
+      abbrev_table = abbrev_table_holder.get ();
     }

   /* Read the top level CU/TU die.  */
-  init_cu_die_reader (&reader, cu, section, NULL);
+  init_cu_die_reader (&reader, cu, section, NULL, abbrev_table);
   info_ptr = read_full_die (&reader, &comp_unit_die, info_ptr, &has_children);

   /* If we are in a DWO stub, process it and then read in the "real" CU/TU
-     from the DWO file.
+     from the DWO file.  read_cutu_die_from_dwo will allocate the abbreviation
+     table from the DWO file and pass the ownership over to us.  It will be
+     referenced from READER, so we must make sure to free it after we're done
+     with READER.
+
      Note that if USE_EXISTING_OK != 0, and THIS_CU->cu already contains a
      DWO CU, that this test will fail (the attribute will not be present).  */
   attr = dwarf2_attr (comp_unit_die, DW_AT_GNU_dwo_name, cu);
-  gdb::optional<auto_free_abbrev_table> abbrev_table_freer_2;
+  abbrev_table_up dwo_abbrev_table;
   if (attr)
     {
       struct dwo_unit *dwo_unit;
@@ -7932,11 +7891,10 @@  init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
       if (dwo_unit != NULL)
 	{
 	  if (read_cutu_die_from_dwo (this_cu, dwo_unit,
-				      abbrev_table != NULL,
 				      comp_unit_die, NULL,
 				      &reader, &info_ptr,
 				      &dwo_comp_unit_die, &has_children,
-				      &abbrev_table_freer_2) == 0)
+				      &dwo_abbrev_table) == 0)
 	    {
 	      /* Dummy die.  */
 	      do_cleanups (cleanups);
@@ -8045,10 +8003,10 @@  init_cutu_and_read_dies_no_follow (struct dwarf2_per_cu_data *this_cu,
       return;
     }

-  dwarf2_read_abbrevs (&cu, abbrev_section);
-  auto_free_abbrev_table abbrev_table_freer (&cu);
+  abbrev_table_up abbrev_table
+    = abbrev_table_read_table (abbrev_section, cu.header.abbrev_sect_off);

-  init_cu_die_reader (&reader, &cu, section, dwo_file);
+  init_cu_die_reader (&reader, &cu, section, dwo_file, abbrev_table.get ());
   info_ptr = read_full_die (&reader, &comp_unit_die, info_ptr, &has_children);

   die_reader_func (&reader, info_ptr, comp_unit_die, has_children, data);
@@ -8518,7 +8476,7 @@  build_type_psymtabs_1 (void)
 {
   struct tu_stats *tu_stats = &dwarf2_per_objfile->tu_stats;
   struct cleanup *cleanups;
-  std::unique_ptr<struct abbrev_table> abbrev_table;
+  abbrev_table_up abbrev_table;
   sect_offset abbrev_offset;
   struct tu_abbrev_offset *sorted_by_abbrev;
   int i;
@@ -9522,25 +9480,26 @@  peek_abbrev_code (bfd *abfd, const gdb_byte *info_ptr)
   return read_unsigned_leb128 (abfd, info_ptr, &bytes_read);
 }

-/* Read the initial uleb128 in the die at INFO_PTR in compilation unit CU.
+/* Read the initial uleb128 in the die at INFO_PTR in compilation unit
+   READER::CU.  Use READER::ABBREV_TABLE to lookup any abbreviation.
+
    Return the corresponding abbrev, or NULL if the number is zero (indicating
    an empty DIE).  In either case *BYTES_READ will be set to the length of
    the initial number.  */

 static struct abbrev_info *
-peek_die_abbrev (const gdb_byte *info_ptr, unsigned int *bytes_read,
-		 struct dwarf2_cu *cu)
+peek_die_abbrev (const die_reader_specs &reader,
+		 const gdb_byte *info_ptr, unsigned int *bytes_read)
 {
+  dwarf2_cu *cu = reader.cu;
   bfd *abfd = cu->objfile->obfd;
-  unsigned int abbrev_number;
-  struct abbrev_info *abbrev;
-
-  abbrev_number = read_unsigned_leb128 (abfd, info_ptr, bytes_read);
+  unsigned int abbrev_number
+    = read_unsigned_leb128 (abfd, info_ptr, bytes_read);

   if (abbrev_number == 0)
     return NULL;

-  abbrev = cu->abbrev_table->lookup_abbrev (abbrev_number);
+  abbrev_info *abbrev = reader.abbrev_table->lookup_abbrev (abbrev_number);
   if (!abbrev)
     {
       error (_("Dwarf Error: Could not find abbrev number %d in %s"
@@ -9559,13 +9518,11 @@  peek_die_abbrev (const gdb_byte *info_ptr, unsigned int *bytes_read,
 static const gdb_byte *
 skip_children (const struct die_reader_specs *reader, const gdb_byte *info_ptr)
 {
-  struct dwarf2_cu *cu = reader->cu;
-  struct abbrev_info *abbrev;
-  unsigned int bytes_read;
-
   while (1)
     {
-      abbrev = peek_die_abbrev (info_ptr, &bytes_read, cu);
+      unsigned int bytes_read;
+      abbrev_info *abbrev = peek_die_abbrev (*reader, info_ptr, &bytes_read);
+
       if (abbrev == NULL)
 	return info_ptr + bytes_read;
       else
@@ -17877,7 +17834,7 @@  read_full_die_1 (const struct die_reader_specs *reader,
       return info_ptr;
     }

-  abbrev = cu->abbrev_table->lookup_abbrev (abbrev_number);
+  abbrev = reader->abbrev_table->lookup_abbrev (abbrev_number);
   if (!abbrev)
     error (_("Dwarf Error: could not find abbrev number %d [in module %s]"),
 	   abbrev_number,
@@ -17985,7 +17942,7 @@  abbrev_table::lookup_abbrev (unsigned int abbrev_number)

 /* Read in an abbrev table.  */

-static std::unique_ptr<struct abbrev_table>
+static abbrev_table_up
 abbrev_table_read_table (struct dwarf2_section_info *section,
 			 sect_offset sect_off)
 {
@@ -17998,8 +17955,7 @@  abbrev_table_read_table (struct dwarf2_section_info *section,
   struct attr_abbrev *cur_attrs;
   unsigned int allocated_attrs;

-  std::unique_ptr<struct abbrev_table> abbrev_table
-    (new struct abbrev_table (sect_off));
+  abbrev_table_up abbrev_table (new struct abbrev_table (sect_off));

   dwarf2_read_section (objfile, section);
   abbrev_ptr = section->buffer + to_underlying (sect_off);
@@ -18088,31 +18044,6 @@  abbrev_table_read_table (struct dwarf2_section_info *section,
   return abbrev_table;
 }

-/* Read the abbrev table for CU from ABBREV_SECTION.  */
-
-static void
-dwarf2_read_abbrevs (struct dwarf2_cu *cu,
-		     struct dwarf2_section_info *abbrev_section)
-{
-  cu->abbrev_table
-    = (abbrev_table_read_table (abbrev_section, cu->header.abbrev_sect_off)
-       .release ());
-}
-
-/* Release the memory used by the abbrev table for a compilation unit.  */
-
-static void
-dwarf2_free_abbrev_table (struct dwarf2_cu *cu)
-{
-  if (cu->abbrev_table != NULL)
-    {
-      delete cu->abbrev_table;
-      /* Set this to NULL so that we SEGV if we try to read it later,
-	 and also because free_comp_unit verifies this is NULL.  */
-      cu->abbrev_table = NULL;
-    }
-}
-
 /* Returns nonzero if TAG represents a type that we might generate a partial
    symbol for.  */

@@ -18155,7 +18086,6 @@  load_partial_dies (const struct die_reader_specs *reader,
   struct objfile *objfile = cu->objfile;
   struct partial_die_info *part_die;
   struct partial_die_info *parent_die, *lasét_die, *first_die = NULL;
-  struct abbrev_info *abbrev;
   unsigned int bytes_read;
   unsigned int load_all = 0;
   int nesting_level = 1;
@@ -18180,7 +18110,7 @@  load_partial_dies (const struct die_reader_specs *reader,

   while (1)
     {
-      abbrev = peek_die_abbrev (info_ptr, &bytes_read, cu);
+      abbrev_info *abbrev = peek_die_abbrev (*reader, info_ptr, &bytes_read);

       /* A NULL abbrev means the end of a series of children.  */
       if (abbrev == NULL)