[RFA 0/6] Some DWARF reader polishing

classic Classic list List threaded Threaded
28 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[RFA 0/6] Some DWARF reader polishing

Tom Tromey-2
While working on a feature for Rust debugging
(https://github.com/rust-lang/rust/issues/32920 if you're curious), I
found a few things to clean up that made my patch a bit simpler to
write.

Since these stand alone reasonably well, I am sending them now.

Regression tested by the buildbot.

Tom

Reply | Threaded
Open this post in threaded view
|

[RFA 1/6] Unify new_symbol and new_symbol_full

Tom Tromey-2
This patch unifies new_symbol with new_symbol_full, replacing a
wrapper function with a default parameter.

2018-01-05  Tom Tromey  <[hidden email]>

        * dwarf2read.c (dwarf2_compute_name): Update comment.
        (read_func_scope, read_variable): Update.
        (new_symbol): Remove.
        (new_symbol_full): Rename to new_symbol.
---
 gdb/ChangeLog    |  7 +++++++
 gdb/dwarf2read.c | 29 +++++++++--------------------
 2 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 24ccfe6882..5ad43d963a 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2018-01-05  Tom Tromey  <[hidden email]>
+
+ * dwarf2read.c (dwarf2_compute_name): Update comment.
+ (read_func_scope, read_variable): Update.
+ (new_symbol): Remove.
+ (new_symbol_full): Rename to new_symbol.
+
 2018-01-05  Pedro Alves  <[hidden email]>
 
  PR gdb/18653
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index a3028e5c52..92c4903241 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -1862,10 +1862,7 @@ static struct compunit_symtab *dwarf2_start_symtab (struct dwarf2_cu *,
     CORE_ADDR);
 
 static struct symbol *new_symbol (struct die_info *, struct type *,
-  struct dwarf2_cu *);
-
-static struct symbol *new_symbol_full (struct die_info *, struct type *,
-       struct dwarf2_cu *, struct symbol *);
+  struct dwarf2_cu *, struct symbol * = NULL);
 
 static void dwarf2_const_value (const struct attribute *, struct symbol *,
  struct dwarf2_cu *);
@@ -10831,7 +10828,7 @@ dwarf2_compute_name (const char *name,
      but otherwise compute it by typename_concat inside GDB.
      FIXME: Actually this is not really true, or at least not always true.
      It's all very confusing.  SYMBOL_SET_NAMES doesn't try to demangle
-     Fortran names because there is no mangling standard.  So new_symbol_full
+     Fortran names because there is no mangling standard.  So new_symbol
      will set the demangled name to the result of dwarf2_full_name, and it is
      the demangled name that GDB uses if it exists.  */
   if (cu->language == language_ada
@@ -11104,8 +11101,8 @@ dwarf2_physname (const char *name, struct die_info *die, struct dwarf2_cu *cu)
 
       if (cu->language == language_go)
  {
-  /* This is a lie, but we already lie to the caller new_symbol_full.
-     new_symbol_full assumes we return the mangled name.
+  /* This is a lie, but we already lie to the caller new_symbol.
+     new_symbol assumes we return the mangled name.
      This just undoes that lie until things are cleaned up.  */
  }
       else
@@ -13731,8 +13728,8 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
     }
 
   newobj = push_context (0, lowpc);
-  newobj->name = new_symbol_full (die, read_type_die (die, cu), cu,
-       (struct symbol *) templ_func);
+  newobj->name = new_symbol (die, read_type_die (die, cu), cu,
+     (struct symbol *) templ_func);
 
   /* If there is a location expression for DW_AT_frame_base, record
      it.  */
@@ -14287,7 +14284,7 @@ read_variable (struct die_info *die, struct dwarf2_cu *cu)
  }
     }
 
-  new_symbol_full (die, NULL, cu, storage);
+  new_symbol (die, NULL, cu, storage);
 }
 
 /* Call CALLBACK from DW_AT_ranges attribute value OFFSET
@@ -21183,8 +21180,8 @@ var_decode_location (struct attribute *attr, struct symbol *sym,
    NULL, allocate a new symbol on the objfile's obstack.  */
 
 static struct symbol *
-new_symbol_full (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
- struct symbol *space)
+new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
+    struct symbol *space)
 {
   struct objfile *objfile = cu->objfile;
   struct gdbarch *gdbarch = get_objfile_arch (objfile);
@@ -21564,14 +21561,6 @@ new_symbol_full (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
   return (sym);
 }
 
-/* A wrapper for new_symbol_full that always allocates a new symbol.  */
-
-static struct symbol *
-new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu)
-{
-  return new_symbol_full (die, type, cu, NULL);
-}
-
 /* Given an attr with a DW_FORM_dataN value in host byte order,
    zero-extend it as appropriate for the symbol's type.  The DWARF
    standard (v4) is not entirely clear about the meaning of using
--
2.13.6

Reply | Threaded
Open this post in threaded view
|

[RFA 2/6] Allocate abbrev_table with new

Tom Tromey-2
In reply to this post by Tom Tromey-2
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.

2018-01-05  Tom Tromey  <[hidden email]>

        * dwarf2read.c (class auto_free_abbrev_table): New.
        (struct abbrev_table): Add constructor, destructor.
        <alloc_abbrev, add_abbrev, lookup_abbrev>: Declare methods.
        (read_cutu_die_from_dwo): Add abbrev_table parameter.
        (init_cutu_and_read_dies): Update.
        (init_cutu_and_read_dies_no_follow): Use auto_free_abbrev_table.
        (build_type_psymtabs_1, peek_die_abbrev, read_full_die_1):
        Update.
        (abbrev_table::alloc_abbrev): Rename from
        abbrev_table_alloc_abbrev.
        (abbrev_table::add_abbrev): Rename from abbrev_table_add_abbrev.
        (abbrev_table::lookup_abbrev): Rename from
        abbrev_table_lookup_abbrev.
        (abbrev_table_read_table): Return a unique_ptr.
        (abbrev_table_free, abbrev_table_free_cleanup): Remove.
        (dwarf2_read_abbrevs): Update.
        (dwarf2_free_abbrev_table): Change argument type to dwarf2_cu*.
---
 gdb/ChangeLog    |  20 ++++++
 gdb/dwarf2read.c | 190 +++++++++++++++++++++++++++----------------------------
 2 files changed, 112 insertions(+), 98 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 5ad43d963a..339a4e728a 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,25 @@
 2018-01-05  Tom Tromey  <[hidden email]>
 
+ * dwarf2read.c (class auto_free_abbrev_table): New.
+ (struct abbrev_table): Add constructor, destructor.
+ <alloc_abbrev, add_abbrev, lookup_abbrev>: Declare methods.
+ (read_cutu_die_from_dwo): Add abbrev_table parameter.
+ (init_cutu_and_read_dies): Update.
+ (init_cutu_and_read_dies_no_follow): Use auto_free_abbrev_table.
+ (build_type_psymtabs_1, peek_die_abbrev, read_full_die_1):
+ Update.
+ (abbrev_table::alloc_abbrev): Rename from
+ abbrev_table_alloc_abbrev.
+ (abbrev_table::add_abbrev): Rename from abbrev_table_add_abbrev.
+ (abbrev_table::lookup_abbrev): Rename from
+ abbrev_table_lookup_abbrev.
+ (abbrev_table_read_table): Return a unique_ptr.
+ (abbrev_table_free, abbrev_table_free_cleanup): Remove.
+ (dwarf2_read_abbrevs): Update.
+ (dwarf2_free_abbrev_table): Change argument type to dwarf2_cu*.
+
+2018-01-05  Tom Tromey  <[hidden email]>
+
  * dwarf2read.c (dwarf2_compute_name): Update comment.
  (read_func_scope, read_variable): Update.
  (new_symbol): Remove.
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 92c4903241..7fd68c54fa 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -774,6 +774,28 @@ 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.  */
@@ -1497,12 +1519,35 @@ struct attr_abbrev
 
 struct abbrev_table
 {
+  abbrev_table (sect_offset off)
+    : sect_off (off)
+  {
+    abbrevs =
+      XOBNEWVEC (&abbrev_obstack, struct abbrev_info *, ABBREV_HASH_SIZE);
+    memset (abbrevs, 0, ABBREV_HASH_SIZE * sizeof (struct abbrev_info *));
+  }
+
+  DISABLE_COPY_AND_ASSIGN (abbrev_table);
+
+  /* Allocate space for a struct abbrev_info object in
+     ABBREV_TABLE.  */
+  struct abbrev_info *alloc_abbrev ();
+
+  /* Add an abbreviation to the table.  */
+  void add_abbrev (unsigned int abbrev_number, struct abbrev_info *abbrev);
+
+  /* Look up an abbrev in the table.
+     Returns NULL if the abbrev is not found.  */
+
+  struct abbrev_info *lookup_abbrev (unsigned int abbrev_number);
+
+
   /* Where the abbrev table came from.
      This is used as a sanity check when the table is used.  */
   sect_offset sect_off;
 
   /* Storage for the abbrev table.  */
-  struct obstack abbrev_obstack;
+  auto_obstack abbrev_obstack;
 
   /* Hash table of abbrevs.
      This is an array of size ABBREV_HASH_SIZE allocated in abbrev_obstack.
@@ -1739,21 +1784,12 @@ static void dwarf2_read_symtab (struct partial_symtab *,
 
 static void psymtab_to_symtab_1 (struct partial_symtab *);
 
-static struct abbrev_info *abbrev_table_lookup_abbrev
-  (const struct abbrev_table *, unsigned int);
-
-static struct abbrev_table *abbrev_table_read_table
+static std::unique_ptr<struct abbrev_table> abbrev_table_read_table
   (struct dwarf2_section_info *, sect_offset);
 
-static void abbrev_table_free (struct abbrev_table *);
-
-static void abbrev_table_free_cleanup (void *);
-
 static void dwarf2_read_abbrevs (struct dwarf2_cu *,
  struct dwarf2_section_info *);
 
-static void dwarf2_free_abbrev_table (void *);
-
 static unsigned int peek_abbrev_code (bfd *, const gdb_byte *);
 
 static struct partial_die_info *load_partial_dies
@@ -7368,6 +7404,7 @@ init_cu_die_reader (struct die_reader_specs *reader,
    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.
    The result is non-zero if a valid (non-dummy) DIE was found.  */
 
 static int
@@ -7379,7 +7416,8 @@ read_cutu_die_from_dwo (struct dwarf2_per_cu_data *this_cu,
  struct die_reader_specs *result_reader,
  const gdb_byte **result_info_ptr,
  struct die_info **result_comp_unit_die,
- int *result_has_children)
+ int *result_has_children,
+ gdb::optional<auto_free_abbrev_table> *abbrev_table)
 {
   struct objfile *objfile = dwarf2_per_objfile->objfile;
   struct dwarf2_cu *cu = this_cu->cu;
@@ -7501,7 +7539,7 @@ read_cutu_die_from_dwo (struct dwarf2_per_cu_data *this_cu,
  init_cutu_and_read_dies owns it.  */
       dwarf2_read_abbrevs (cu, dwo_abbrev_section);
       /* Ensure the DWO abbrev table gets freed.  */
-      make_cleanup (dwarf2_free_abbrev_table, cu);
+      abbrev_table->emplace (cu);
     }
   else
     {
@@ -7661,12 +7699,14 @@ 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;
   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) == 0)
+      &comp_unit_die, &has_children,
+      &abbrev_table) == 0)
     {
       /* Dummy die.  */
       do_cleanups (cleanups);
@@ -7688,10 +7728,6 @@ init_tu_and_read_dwo_dies (struct dwarf2_per_cu_data *this_cu,
      caller clean it up when finished with it.  */
   discard_cleanups (free_cu_cleanup);
 
-  /* We can only discard free_cu_cleanup and all subsequent cleanups.
-     So we have to manually free the abbrev table.  */
-  dwarf2_free_abbrev_table (cu);
-
   /* Link this CU into read_in_chain.  */
   this_cu->cu->read_in_chain = dwarf2_per_objfile->read_in_chain;
   dwarf2_per_objfile->read_in_chain = this_cu;
@@ -7852,6 +7888,7 @@ init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
      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;
   if (abbrev_table != NULL)
     {
       gdb_assert (cu->abbrev_table == NULL);
@@ -7861,7 +7898,7 @@ init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
   else if (cu->abbrev_table == NULL)
     {
       dwarf2_read_abbrevs (cu, abbrev_section);
-      make_cleanup (dwarf2_free_abbrev_table, cu);
+      abbrev_table_freer.emplace (cu);
     }
   else if (rereading_dwo_cu)
     {
@@ -7878,6 +7915,7 @@ init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
      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;
   if (attr)
     {
       struct dwo_unit *dwo_unit;
@@ -7897,7 +7935,8 @@ init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
       abbrev_table != NULL,
       comp_unit_die, NULL,
       &reader, &info_ptr,
-      &dwo_comp_unit_die, &has_children) == 0)
+      &dwo_comp_unit_die, &has_children,
+      &abbrev_table_freer_2) == 0)
     {
       /* Dummy die.  */
       do_cleanups (cleanups);
@@ -7927,10 +7966,6 @@ init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
      caller clean it up when finished with it.  */
   discard_cleanups (free_cu_cleanup);
 
-  /* We can only discard free_cu_cleanup and all subsequent cleanups.
-     So we have to manually free the abbrev table.  */
-  dwarf2_free_abbrev_table (cu);
-
   /* Link this CU into read_in_chain.  */
   this_cu->cu->read_in_chain = dwarf2_per_objfile->read_in_chain;
   dwarf2_per_objfile->read_in_chain = this_cu;
@@ -8011,7 +8046,7 @@ init_cutu_and_read_dies_no_follow (struct dwarf2_per_cu_data *this_cu,
     }
 
   dwarf2_read_abbrevs (&cu, abbrev_section);
-  make_cleanup (dwarf2_free_abbrev_table, &cu);
+  auto_free_abbrev_table abbrev_table_freer (&cu);
 
   init_cu_die_reader (&reader, &cu, section, dwo_file);
   info_ptr = read_full_die (&reader, &comp_unit_die, info_ptr, &has_children);
@@ -8483,7 +8518,7 @@ build_type_psymtabs_1 (void)
 {
   struct tu_stats *tu_stats = &dwarf2_per_objfile->tu_stats;
   struct cleanup *cleanups;
-  struct abbrev_table *abbrev_table;
+  std::unique_ptr<struct abbrev_table> abbrev_table;
   sect_offset abbrev_offset;
   struct tu_abbrev_offset *sorted_by_abbrev;
   int i;
@@ -8534,8 +8569,6 @@ build_type_psymtabs_1 (void)
  sizeof (struct tu_abbrev_offset), sort_tu_by_abbrev_offset);
 
   abbrev_offset = (sect_offset) ~(unsigned) 0;
-  abbrev_table = NULL;
-  make_cleanup (abbrev_table_free_cleanup, &abbrev_table);
 
   for (i = 0; i < dwarf2_per_objfile->n_type_units; ++i)
     {
@@ -8545,13 +8578,6 @@ build_type_psymtabs_1 (void)
       if (abbrev_table == NULL
   || tu->abbrev_offset != abbrev_offset)
  {
-  if (abbrev_table != NULL)
-    {
-      abbrev_table_free (abbrev_table);
-      /* Reset to NULL in case abbrev_table_read_table throws
- an error: abbrev_table_free_cleanup will get called.  */
-      abbrev_table = NULL;
-    }
   abbrev_offset = tu->abbrev_offset;
   abbrev_table =
     abbrev_table_read_table (&dwarf2_per_objfile->abbrev,
@@ -8559,8 +8585,8 @@ build_type_psymtabs_1 (void)
   ++tu_stats->nr_uniq_abbrev_tables;
  }
 
-      init_cutu_and_read_dies (&tu->sig_type->per_cu, abbrev_table, 0, 0,
-       build_type_psymtabs_reader, NULL);
+      init_cutu_and_read_dies (&tu->sig_type->per_cu, abbrev_table.get (),
+       0, 0, build_type_psymtabs_reader, NULL);
     }
 
   do_cleanups (cleanups);
@@ -9514,7 +9540,7 @@ peek_die_abbrev (const gdb_byte *info_ptr, unsigned int *bytes_read,
   if (abbrev_number == 0)
     return NULL;
 
-  abbrev = abbrev_table_lookup_abbrev (cu->abbrev_table, abbrev_number);
+  abbrev = cu->abbrev_table->lookup_abbrev (abbrev_number);
   if (!abbrev)
     {
       error (_("Dwarf Error: Could not find abbrev number %d in %s"
@@ -17851,7 +17877,7 @@ read_full_die_1 (const struct die_reader_specs *reader,
       return info_ptr;
     }
 
-  abbrev = abbrev_table_lookup_abbrev (cu->abbrev_table, abbrev_number);
+  abbrev = cu->abbrev_table->lookup_abbrev (abbrev_number);
   if (!abbrev)
     error (_("Dwarf Error: could not find abbrev number %d [in module %s]"),
    abbrev_number,
@@ -17912,12 +17938,12 @@ read_full_die (const struct die_reader_specs *reader,
 
 /* Allocate space for a struct abbrev_info object in ABBREV_TABLE.  */
 
-static struct abbrev_info *
-abbrev_table_alloc_abbrev (struct abbrev_table *abbrev_table)
+struct abbrev_info *
+abbrev_table::alloc_abbrev ()
 {
   struct abbrev_info *abbrev;
 
-  abbrev = XOBNEW (&abbrev_table->abbrev_obstack, struct abbrev_info);
+  abbrev = XOBNEW (&abbrev_obstack, struct abbrev_info);
   memset (abbrev, 0, sizeof (struct abbrev_info));
 
   return abbrev;
@@ -17925,30 +17951,28 @@ abbrev_table_alloc_abbrev (struct abbrev_table *abbrev_table)
 
 /* Add an abbreviation to the table.  */
 
-static void
-abbrev_table_add_abbrev (struct abbrev_table *abbrev_table,
- unsigned int abbrev_number,
- struct abbrev_info *abbrev)
+void
+abbrev_table::add_abbrev (unsigned int abbrev_number,
+  struct abbrev_info *abbrev)
 {
   unsigned int hash_number;
 
   hash_number = abbrev_number % ABBREV_HASH_SIZE;
-  abbrev->next = abbrev_table->abbrevs[hash_number];
-  abbrev_table->abbrevs[hash_number] = abbrev;
+  abbrev->next = abbrevs[hash_number];
+  abbrevs[hash_number] = abbrev;
 }
 
 /* Look up an abbrev in the table.
    Returns NULL if the abbrev is not found.  */
 
-static struct abbrev_info *
-abbrev_table_lookup_abbrev (const struct abbrev_table *abbrev_table,
-    unsigned int abbrev_number)
+struct abbrev_info *
+abbrev_table::lookup_abbrev (unsigned int abbrev_number)
 {
   unsigned int hash_number;
   struct abbrev_info *abbrev;
 
   hash_number = abbrev_number % ABBREV_HASH_SIZE;
-  abbrev = abbrev_table->abbrevs[hash_number];
+  abbrev = abbrevs[hash_number];
 
   while (abbrev)
     {
@@ -17961,13 +17985,12 @@ abbrev_table_lookup_abbrev (const struct abbrev_table *abbrev_table,
 
 /* Read in an abbrev table.  */
 
-static struct abbrev_table *
+static std::unique_ptr<struct abbrev_table>
 abbrev_table_read_table (struct dwarf2_section_info *section,
  sect_offset sect_off)
 {
   struct objfile *objfile = dwarf2_per_objfile->objfile;
   bfd *abfd = get_section_bfd_owner (section);
-  struct abbrev_table *abbrev_table;
   const gdb_byte *abbrev_ptr;
   struct abbrev_info *cur_abbrev;
   unsigned int abbrev_number, bytes_read, abbrev_name;
@@ -17975,14 +17998,8 @@ abbrev_table_read_table (struct dwarf2_section_info *section,
   struct attr_abbrev *cur_attrs;
   unsigned int allocated_attrs;
 
-  abbrev_table = XNEW (struct abbrev_table);
-  abbrev_table->sect_off = sect_off;
-  obstack_init (&abbrev_table->abbrev_obstack);
-  abbrev_table->abbrevs =
-    XOBNEWVEC (&abbrev_table->abbrev_obstack, struct abbrev_info *,
-       ABBREV_HASH_SIZE);
-  memset (abbrev_table->abbrevs, 0,
-  ABBREV_HASH_SIZE * sizeof (struct abbrev_info *));
+  std::unique_ptr<struct abbrev_table> abbrev_table
+    (new struct abbrev_table (sect_off));
 
   dwarf2_read_section (objfile, section);
   abbrev_ptr = section->buffer + to_underlying (sect_off);
@@ -17995,7 +18012,7 @@ abbrev_table_read_table (struct dwarf2_section_info *section,
   /* Loop until we reach an abbrev number of 0.  */
   while (abbrev_number)
     {
-      cur_abbrev = abbrev_table_alloc_abbrev (abbrev_table);
+      cur_abbrev = abbrev_table->alloc_abbrev ();
 
       /* read in abbrev header */
       cur_abbrev->number = abbrev_number;
@@ -18050,7 +18067,7 @@ abbrev_table_read_table (struct dwarf2_section_info *section,
       memcpy (cur_abbrev->attrs, cur_attrs,
       cur_abbrev->num_attrs * sizeof (struct attr_abbrev));
 
-      abbrev_table_add_abbrev (abbrev_table, abbrev_number, cur_abbrev);
+      abbrev_table->add_abbrev (abbrev_number, cur_abbrev);
 
       /* Get next abbreviation.
          Under Irix6 the abbreviations for a compilation unit are not
@@ -18063,7 +18080,7 @@ abbrev_table_read_table (struct dwarf2_section_info *section,
  break;
       abbrev_number = read_unsigned_leb128 (abfd, abbrev_ptr, &bytes_read);
       abbrev_ptr += bytes_read;
-      if (abbrev_table_lookup_abbrev (abbrev_table, abbrev_number) != NULL)
+      if (abbrev_table->lookup_abbrev (abbrev_number) != NULL)
  break;
     }
 
@@ -18071,52 +18088,29 @@ abbrev_table_read_table (struct dwarf2_section_info *section,
   return abbrev_table;
 }
 
-/* Free the resources held by ABBREV_TABLE.  */
-
-static void
-abbrev_table_free (struct abbrev_table *abbrev_table)
-{
-  obstack_free (&abbrev_table->abbrev_obstack, NULL);
-  xfree (abbrev_table);
-}
-
-/* Same as abbrev_table_free but as a cleanup.
-   We pass in a pointer to the pointer to the table so that we can
-   set the pointer to NULL when we're done.  It also simplifies
-   build_type_psymtabs_1.  */
-
-static void
-abbrev_table_free_cleanup (void *table_ptr)
-{
-  struct abbrev_table **abbrev_table_ptr = (struct abbrev_table **) table_ptr;
-
-  if (*abbrev_table_ptr != NULL)
-    abbrev_table_free (*abbrev_table_ptr);
-  *abbrev_table_ptr = NULL;
-}
-
 /* 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);
+  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 (void *ptr_to_cu)
+dwarf2_free_abbrev_table (struct dwarf2_cu *cu)
 {
-  struct dwarf2_cu *cu = (struct dwarf2_cu *) ptr_to_cu;
-
   if (cu->abbrev_table != NULL)
-    abbrev_table_free (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;
+    {
+      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
--
2.13.6

Reply | Threaded
Open this post in threaded view
|

[RFA 3/6] Allocate dwarf2_cu with new

Tom Tromey-2
In reply to this post by Tom Tromey-2
This changes dwarf2_cu to be allocated with new, and fixes up the
users.

2018-01-05  Tom Tromey  <[hidden email]>

        * dwarf2read.c (struct dwarf2_cu): Add constructor, destructor.
        <free_abbrev_table>: Declare method.
        (~auto_free_abbrev_table): Update.
        (init_tu_and_read_dwo_dies): Use unique_ptr.  Remove cleanups.
        (init_cutu_and_read_dies): Likewise.
        (init_cutu_and_read_dies_no_follow): Update.
        (dwarf2_free_abbrev_table): Remove.
        (dwarf2_cu::dwarf2_cu): New.  Rename from init_one_comp_unit.
        (dwarf2_cu::~dwarf2_cu): New.
        (dwarf2_cu::free_abbrev_table): New method.
        (free_heap_comp_unit, free_stack_comp_unit): Remove.
        (age_cached_comp_units, free_one_cached_comp_unit): Use delete.
---
 gdb/ChangeLog    |  15 ++++
 gdb/dwarf2read.c | 247 +++++++++++++++++++++----------------------------------
 2 files changed, 107 insertions(+), 155 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 339a4e728a..04000eacae 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,20 @@
 2018-01-05  Tom Tromey  <[hidden email]>
 
+ * dwarf2read.c (struct dwarf2_cu): Add constructor, destructor.
+ <free_abbrev_table>: Declare method.
+ (~auto_free_abbrev_table): Update.
+ (init_tu_and_read_dwo_dies): Use unique_ptr.  Remove cleanups.
+ (init_cutu_and_read_dies): Likewise.
+ (init_cutu_and_read_dies_no_follow): Update.
+ (dwarf2_free_abbrev_table): Remove.
+ (dwarf2_cu::dwarf2_cu): New.  Rename from init_one_comp_unit.
+ (dwarf2_cu::~dwarf2_cu): New.
+ (dwarf2_cu::free_abbrev_table): New method.
+ (free_heap_comp_unit, free_stack_comp_unit): Remove.
+ (age_cached_comp_units, free_one_cached_comp_unit): Use delete.
+
+2018-01-05  Tom Tromey  <[hidden email]>
+
  * dwarf2read.c (class auto_free_abbrev_table): New.
  (struct abbrev_table): Add constructor, destructor.
  <alloc_abbrev, add_abbrev, lookup_abbrev>: Declare methods.
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 7fd68c54fa..400080b208 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -639,23 +639,31 @@ DEF_VEC_O (delayed_method_info);
 /* Internal state when decoding a particular compilation unit.  */
 struct dwarf2_cu
 {
+  dwarf2_cu (struct dwarf2_per_cu_data *per_cu);
+  ~dwarf2_cu ();
+
+  DISABLE_COPY_AND_ASSIGN (dwarf2_cu);
+
+  void free_abbrev_table ();
+
+
   /* The objfile containing this compilation unit.  */
   struct objfile *objfile;
 
   /* The header of the compilation unit.  */
-  struct comp_unit_head header;
+  struct comp_unit_head header {};
 
   /* Base address of this compilation unit.  */
-  CORE_ADDR base_address;
+  CORE_ADDR base_address = 0;
 
   /* Non-zero if base_address has been set.  */
-  int base_known;
+  int base_known = 0;
 
   /* The language we are debugging.  */
-  enum language language;
-  const struct language_defn *language_defn;
+  enum language language = language_unknown;
+  const struct language_defn *language_defn = nullptr;
 
-  const char *producer;
+  const char *producer = nullptr;
 
   /* The generic symbol table building routines have separate lists for
      file scope symbols and all all other scopes (local scopes).  So
@@ -666,60 +674,60 @@ struct dwarf2_cu
      first local scope, and all other local scopes as nested local
      scopes, and worked fine.  Check to see if we really need to
      distinguish these in buildsym.c.  */
-  struct pending **list_in_scope;
+  struct pending **list_in_scope = nullptr;
 
   /* 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;
+  struct abbrev_table *abbrev_table = nullptr;
 
   /* Hash table holding all the loaded partial DIEs
      with partial_die->offset.SECT_OFF as hash.  */
-  htab_t partial_dies;
+  htab_t partial_dies = nullptr;
 
   /* Storage for things with the same lifetime as this read-in compilation
      unit, including partial DIEs.  */
-  struct obstack comp_unit_obstack;
+  auto_obstack comp_unit_obstack;
 
   /* When multiple dwarf2_cu structures are living in memory, this field
      chains them all together, so that they can be released efficiently.
      We will probably also want a generation counter so that most-recently-used
      compilation units are cached...  */
-  struct dwarf2_per_cu_data *read_in_chain;
+  struct dwarf2_per_cu_data *read_in_chain = nullptr;
 
   /* Backlink to our per_cu entry.  */
   struct dwarf2_per_cu_data *per_cu;
 
   /* How many compilation units ago was this CU last referenced?  */
-  int last_used;
+  int last_used = 0;
 
   /* A hash table of DIE cu_offset for following references with
      die_info->offset.sect_off as hash.  */
-  htab_t die_hash;
+  htab_t die_hash = nullptr;
 
   /* Full DIEs if read in.  */
-  struct die_info *dies;
+  struct die_info *dies = nullptr;
 
   /* A set of pointers to dwarf2_per_cu_data objects for compilation
      units referenced by this one.  Only set during full symbol processing;
      partial symbol tables do not have dependencies.  */
-  htab_t dependencies;
+  htab_t dependencies = nullptr;
 
   /* Header data from the line table, during full symbol processing.  */
-  struct line_header *line_header;
+  struct line_header *line_header = nullptr;
   /* Non-NULL if LINE_HEADER is owned by this DWARF_CU.  Otherwise,
      it's owned by dwarf2_per_objfile::line_header_hash.  If non-NULL,
      this is the DW_TAG_compile_unit die for this CU.  We'll hold on
      to the line header as long as this DIE is being processed.  See
      process_die_scope.  */
-  die_info *line_header_die_owner;
+  die_info *line_header_die_owner = nullptr;
 
   /* A list of methods which need to have physnames computed
      after all type information has been read.  */
-  VEC (delayed_method_info) *method_list;
+  VEC (delayed_method_info) *method_list = nullptr;
 
   /* To be copied to symtab->call_site_htab.  */
-  htab_t call_site_htab;
+  htab_t call_site_htab = nullptr;
 
   /* Non-NULL if this CU came from a DWO file.
      There is an invariant here that is important to remember:
@@ -730,12 +738,12 @@ struct dwarf2_cu
      is moot), or there is and either we're not going to read it (in which
      case this is NULL) or there is and we are reading it (in which case this
      is non-NULL).  */
-  struct dwo_unit *dwo_unit;
+  struct dwo_unit *dwo_unit = nullptr;
 
   /* The DW_AT_addr_base attribute if present, zero otherwise
      (zero is a valid value though).
      Note this value comes from the Fission stub CU/TU's DIE.  */
-  ULONGEST addr_base;
+  ULONGEST addr_base = 0;
 
   /* The DW_AT_ranges_base attribute if present, zero otherwise
      (zero is a valid value though).
@@ -747,7 +755,7 @@ struct dwarf2_cu
      DW_AT_ranges appeared in the DW_TAG_compile_unit of DWO DIEs: then
      DW_AT_ranges_base *would* have to be applied, and we'd have to care
      whether the DW_AT_ranges attribute came from the skeleton or DWO.  */
-  ULONGEST ranges_base;
+  ULONGEST ranges_base = 0;
 
   /* Mark used when releasing cached dies.  */
   unsigned int mark : 1;
@@ -774,8 +782,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
@@ -788,7 +794,7 @@ public:
 
   ~auto_free_abbrev_table ()
   {
-    dwarf2_free_abbrev_table (m_cu);
+    m_cu->free_abbrev_table ();
   }
 
 private:
@@ -2141,8 +2147,6 @@ static const gdb_byte *skip_one_die (const struct die_reader_specs *reader,
      const gdb_byte *info_ptr,
      struct abbrev_info *abbrev);
 
-static void free_stack_comp_unit (void *);
-
 static hashval_t partial_die_hash (const void *item);
 
 static int partial_die_eq (const void *item_lhs, const void *item_rhs);
@@ -2150,15 +2154,10 @@ static int partial_die_eq (const void *item_lhs, const void *item_rhs);
 static struct dwarf2_per_cu_data *dwarf2_find_containing_comp_unit
   (sect_offset sect_off, unsigned int offset_in_dwz, struct objfile *objfile);
 
-static void init_one_comp_unit (struct dwarf2_cu *cu,
- struct dwarf2_per_cu_data *per_cu);
-
 static void prepare_one_comp_unit (struct dwarf2_cu *cu,
    struct die_info *comp_unit_die,
    enum language pretend_language);
 
-static void free_heap_comp_unit (void *);
-
 static void free_cached_comp_units (void *);
 
 static void age_cached_comp_units (void);
@@ -2442,7 +2441,7 @@ dwarf2_per_objfile::free_cached_comp_units ()
     {
       dwarf2_per_cu_data *next_cu = per_cu->cu->read_in_chain;
 
-      free_heap_comp_unit (per_cu->cu);
+      delete per_cu->cu;
       *last_chain = next_cu;
       per_cu = next_cu;
     }
@@ -7543,7 +7542,7 @@ read_cutu_die_from_dwo (struct dwarf2_per_cu_data *this_cu,
     }
   else
     {
-      dwarf2_free_abbrev_table (cu);
+      cu->free_abbrev_table ();
       dwarf2_read_abbrevs (cu, dwo_abbrev_section);
       /* Leave any existing abbrev table cleanup as is.  */
     }
@@ -7649,12 +7648,7 @@ lookup_dwo_unit (struct dwarf2_per_cu_data *this_cu,
 
 /* Subroutine of init_cutu_and_read_dies to simplify it.
    See it for a description of the parameters.
-   Read a TU directly from a DWO file, bypassing the stub.
-
-   Note: This function could be a little bit simpler if we shared cleanups
-   with our caller, init_cutu_and_read_dies.  That's generally a fragile thing
-   to do, so we keep this function self-contained.  Or we could move this
-   into our caller, but it's complex enough already.  */
+   Read a TU directly from a DWO file, bypassing the stub.  */
 
 static void
 init_tu_and_read_dwo_dies (struct dwarf2_per_cu_data *this_cu,
@@ -7663,8 +7657,8 @@ init_tu_and_read_dwo_dies (struct dwarf2_per_cu_data *this_cu,
    void *data)
 {
   struct dwarf2_cu *cu;
+  std::unique_ptr<dwarf2_cu> new_cu;
   struct signatured_type *sig_type;
-  struct cleanup *cleanups, *free_cu_cleanup = NULL;
   struct die_reader_specs reader;
   const gdb_byte *info_ptr;
   struct die_info *comp_unit_die;
@@ -7676,8 +7670,6 @@ init_tu_and_read_dwo_dies (struct dwarf2_per_cu_data *this_cu,
   sig_type = (struct signatured_type *) this_cu;
   gdb_assert (sig_type->dwo_unit != NULL);
 
-  cleanups = make_cleanup (null_cleanup, NULL);
-
   if (use_existing_cu && this_cu->cu != NULL)
     {
       gdb_assert (this_cu->cu->dwo_unit == sig_type->dwo_unit);
@@ -7689,10 +7681,8 @@ init_tu_and_read_dwo_dies (struct dwarf2_per_cu_data *this_cu,
     {
       /* If !use_existing_cu, this_cu->cu must be NULL.  */
       gdb_assert (this_cu->cu == NULL);
-      cu = XNEW (struct dwarf2_cu);
-      init_one_comp_unit (cu, this_cu);
-      /* If an error occurs while loading, release our storage.  */
-      free_cu_cleanup = make_cleanup (free_heap_comp_unit, cu);
+      new_cu.reset (new dwarf2_cu (this_cu));
+      cu = new_cu.get ();
     }
 
   /* A future optimization, if needed, would be to use an existing
@@ -7709,7 +7699,6 @@ init_tu_and_read_dwo_dies (struct dwarf2_per_cu_data *this_cu,
       &abbrev_table) == 0)
     {
       /* Dummy die.  */
-      do_cleanups (cleanups);
       return;
     }
 
@@ -7720,23 +7709,19 @@ init_tu_and_read_dwo_dies (struct dwarf2_per_cu_data *this_cu,
      but the alternative is making the latter more complex.
      This function is only for the special case of using DWO files directly:
      no point in overly complicating the general case just to handle this.  */
-  if (free_cu_cleanup != NULL)
+  if (new_cu != NULL && keep)
     {
-      if (keep)
- {
-  /* We've successfully allocated this compilation unit.  Let our
-     caller clean it up when finished with it.  */
-  discard_cleanups (free_cu_cleanup);
+      /* We've successfully allocated this compilation unit.  Let our
+ caller clean it up when finished with it.  We have to
+ manually free the abbrev table.  */
+      cu->free_abbrev_table ();
 
-  /* Link this CU into read_in_chain.  */
-  this_cu->cu->read_in_chain = dwarf2_per_objfile->read_in_chain;
-  dwarf2_per_objfile->read_in_chain = this_cu;
- }
-      else
- do_cleanups (free_cu_cleanup);
+      /* Link this CU into read_in_chain.  */
+      this_cu->cu->read_in_chain = dwarf2_per_objfile->read_in_chain;
+      dwarf2_per_objfile->read_in_chain = this_cu;
+      /* The chain owns it now.  */
+      new_cu.release ();
     }
-
-  do_cleanups (cleanups);
 }
 
 /* Initialize a CU (or TU) and read its DIEs.
@@ -7771,7 +7756,6 @@ init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
   struct die_info *comp_unit_die;
   int has_children;
   struct attribute *attr;
-  struct cleanup *cleanups, *free_cu_cleanup = NULL;
   struct signatured_type *sig_type = NULL;
   struct dwarf2_section_info *abbrev_section;
   /* Non-zero if CU currently points to a DWO file and we need to
@@ -7799,8 +7783,6 @@ init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
       return;
     }
 
-  cleanups = make_cleanup (null_cleanup, NULL);
-
   /* This is cheap if the section is already read in.  */
   dwarf2_read_section (objfile, section);
 
@@ -7808,6 +7790,7 @@ init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
 
   abbrev_section = get_abbrev_section_for_cu (this_cu);
 
+  std::unique_ptr<dwarf2_cu> new_cu;
   if (use_existing_cu && this_cu->cu != NULL)
     {
       cu = this_cu->cu;
@@ -7824,10 +7807,8 @@ init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
     {
       /* If !use_existing_cu, this_cu->cu must be NULL.  */
       gdb_assert (this_cu->cu == NULL);
-      cu = XNEW (struct dwarf2_cu);
-      init_one_comp_unit (cu, this_cu);
-      /* If an error occurs while loading, release our storage.  */
-      free_cu_cleanup = make_cleanup (free_heap_comp_unit, cu);
+      new_cu.reset (new dwarf2_cu (this_cu));
+      cu = new_cu.get ();
     }
 
   /* Get the header.  */
@@ -7879,7 +7860,6 @@ init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
   if (info_ptr >= begin_info_ptr + this_cu->length
       || peek_abbrev_code (abfd, info_ptr) == 0)
     {
-      do_cleanups (cleanups);
       return;
     }
 
@@ -7902,7 +7882,7 @@ init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
     }
   else if (rereading_dwo_cu)
     {
-      dwarf2_free_abbrev_table (cu);
+      cu->free_abbrev_table ();
       dwarf2_read_abbrevs (cu, abbrev_section);
     }
 
@@ -7939,7 +7919,6 @@ init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
       &abbrev_table_freer_2) == 0)
     {
       /* Dummy die.  */
-      do_cleanups (cleanups);
       return;
     }
   comp_unit_die = dwo_comp_unit_die;
@@ -7958,23 +7937,14 @@ init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
   die_reader_func (&reader, info_ptr, comp_unit_die, has_children, data);
 
   /* Done, clean up.  */
-  if (free_cu_cleanup != NULL)
+  if (new_cu != NULL && keep)
     {
-      if (keep)
- {
-  /* We've successfully allocated this compilation unit.  Let our
-     caller clean it up when finished with it.  */
-  discard_cleanups (free_cu_cleanup);
-
-  /* Link this CU into read_in_chain.  */
-  this_cu->cu->read_in_chain = dwarf2_per_objfile->read_in_chain;
-  dwarf2_per_objfile->read_in_chain = this_cu;
- }
-      else
- do_cleanups (free_cu_cleanup);
+      /* Link this CU into read_in_chain.  */
+      this_cu->cu->read_in_chain = dwarf2_per_objfile->read_in_chain;
+      dwarf2_per_objfile->read_in_chain = this_cu;
+      /* The chain owns it now.  */
+      new_cu.release ();
     }
-
-  do_cleanups (cleanups);
 }
 
 /* Read CU/TU THIS_CU but do not follow DW_AT_GNU_dwo_name if present.
@@ -8003,10 +7973,8 @@ init_cutu_and_read_dies_no_follow (struct dwarf2_per_cu_data *this_cu,
   struct dwarf2_section_info *section = this_cu->section;
   bfd *abfd = get_section_bfd_owner (section);
   struct dwarf2_section_info *abbrev_section;
-  struct dwarf2_cu cu;
   const gdb_byte *begin_info_ptr, *info_ptr;
   struct die_reader_specs reader;
-  struct cleanup *cleanups;
   struct die_info *comp_unit_die;
   int has_children;
 
@@ -8024,9 +7992,7 @@ init_cutu_and_read_dies_no_follow (struct dwarf2_per_cu_data *this_cu,
   /* This is cheap if the section is already read in.  */
   dwarf2_read_section (objfile, section);
 
-  init_one_comp_unit (&cu, this_cu);
-
-  cleanups = make_cleanup (free_stack_comp_unit, &cu);
+  struct dwarf2_cu cu (this_cu);
 
   begin_info_ptr = info_ptr = section->buffer + to_underlying (this_cu->sect_off);
   info_ptr = read_and_check_comp_unit_head (&cu.header, section,
@@ -8041,7 +8007,6 @@ init_cutu_and_read_dies_no_follow (struct dwarf2_per_cu_data *this_cu,
   if (info_ptr >= begin_info_ptr + this_cu->length
       || peek_abbrev_code (abfd, info_ptr) == 0)
     {
-      do_cleanups (cleanups);
       return;
     }
 
@@ -8052,8 +8017,6 @@ init_cutu_and_read_dies_no_follow (struct dwarf2_per_cu_data *this_cu,
   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);
-
-  do_cleanups (cleanups);
 }
 
 /* Read a CU/TU, except that this does not look for DW_AT_GNU_dwo_name and
@@ -18098,20 +18061,6 @@ dwarf2_read_abbrevs (struct dwarf2_cu *cu,
     = (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.  */
@@ -24949,14 +24898,39 @@ dwarf2_find_containing_comp_unit (sect_offset sect_off,
 
 /* Initialize dwarf2_cu CU, owned by PER_CU.  */
 
-static void
-init_one_comp_unit (struct dwarf2_cu *cu, struct dwarf2_per_cu_data *per_cu)
+dwarf2_cu::dwarf2_cu (struct dwarf2_per_cu_data *per_cu_)
+  : objfile (per_cu_->objfile),
+    per_cu (per_cu_),
+    mark (0),
+    has_loclist (0),
+    checked_producer (0),
+    producer_is_gxx_lt_4_6 (0),
+    producer_is_gcc_lt_4_3 (0),
+    producer_is_icc_lt_14 (0),
+    processing_has_namespace_info (0)
 {
-  memset (cu, 0, sizeof (*cu));
-  per_cu->cu = cu;
-  cu->per_cu = per_cu;
-  cu->objfile = per_cu->objfile;
-  obstack_init (&cu->comp_unit_obstack);
+  per_cu->cu = this;
+}
+
+/* Destroy a dwarf2_cu.  */
+
+dwarf2_cu::~dwarf2_cu ()
+{
+  per_cu->cu = NULL;
+}
+
+/* Free just the abbrev table.  */
+
+void
+dwarf2_cu::free_abbrev_table ()
+{
+  if (abbrev_table != NULL)
+    {
+      delete 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.  */
+      abbrev_table = NULL;
+    }
 }
 
 /* Initialize basic fields of dwarf_cu CU according to DIE COMP_UNIT_DIE.  */
@@ -24980,43 +24954,6 @@ prepare_one_comp_unit (struct dwarf2_cu *cu, struct die_info *comp_unit_die,
   cu->producer = dwarf2_string_attr (comp_unit_die, DW_AT_producer, cu);
 }
 
-/* Release one cached compilation unit, CU.  We unlink it from the tree
-   of compilation units, but we don't remove it from the read_in_chain;
-   the caller is responsible for that.
-   NOTE: DATA is a void * because this function is also used as a
-   cleanup routine.  */
-
-static void
-free_heap_comp_unit (void *data)
-{
-  struct dwarf2_cu *cu = (struct dwarf2_cu *) data;
-
-  gdb_assert (cu->per_cu != NULL);
-  cu->per_cu->cu = NULL;
-  cu->per_cu = NULL;
-
-  obstack_free (&cu->comp_unit_obstack, NULL);
-
-  xfree (cu);
-}
-
-/* This cleanup function is passed the address of a dwarf2_cu on the stack
-   when we're finished with it.  We can't free the pointer itself, but be
-   sure to unlink it from the cache.  Also release any associated storage.  */
-
-static void
-free_stack_comp_unit (void *data)
-{
-  struct dwarf2_cu *cu = (struct dwarf2_cu *) data;
-
-  gdb_assert (cu->per_cu != NULL);
-  cu->per_cu->cu = NULL;
-  cu->per_cu = NULL;
-
-  obstack_free (&cu->comp_unit_obstack, NULL);
-  cu->partial_dies = NULL;
-}
-
 /* Free all cached compilation units.  */
 
 static void
@@ -25053,7 +24990,7 @@ age_cached_comp_units (void)
 
       if (!per_cu->cu->mark)
  {
-  free_heap_comp_unit (per_cu->cu);
+  delete per_cu->cu;
   *last_chain = next_cu;
  }
       else
@@ -25080,7 +25017,7 @@ free_one_cached_comp_unit (struct dwarf2_per_cu_data *target_per_cu)
 
       if (per_cu == target_per_cu)
  {
-  free_heap_comp_unit (per_cu->cu);
+  delete per_cu->cu;
   per_cu->cu = NULL;
   *last_chain = next_cu;
   break;
--
2.13.6

Reply | Threaded
Open this post in threaded view
|

[RFA 4/6] Change dwarf2_cu::method_info to be a std::vector

Tom Tromey-2
In reply to this post by Tom Tromey-2
This changes the type of dwarf2_cu::method_info and fixes up the uses.
In order to remove cleanups from process_full_comp_unit and
process_full_type_unit, psymtab_include_file_name also had to be
changed to avoid leaving dangling cleanups.

2018-01-05  Tom Tromey  <[hidden email]>

        * dwarf2read.c (delayed_method_info): Remove typedef.
        (dwarf2_cu::method_info): Now a std::vector.
        (add_to_method_list): Update.
        (free_delayed_list): Remove.
        (compute_delayed_physnames): Update.
        (process_full_comp_unit, process_full_type_unit): Clear the method
        list.  Remove cleanups.
        (psymtab_include_file_name): Add name_holder parameter.  Use
        unique_xmalloc_ptr.
        (dwarf_decode_lines): Update.
---
 gdb/ChangeLog    | 13 ++++++++
 gdb/dwarf2read.c | 93 ++++++++++++++++++++++----------------------------------
 2 files changed, 49 insertions(+), 57 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 04000eacae..0c878d9aba 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,18 @@
 2018-01-05  Tom Tromey  <[hidden email]>
 
+ * dwarf2read.c (delayed_method_info): Remove typedef.
+ (dwarf2_cu::method_info): Now a std::vector.
+ (add_to_method_list): Update.
+ (free_delayed_list): Remove.
+ (compute_delayed_physnames): Update.
+ (process_full_comp_unit, process_full_type_unit): Clear the method
+ list.  Remove cleanups.
+ (psymtab_include_file_name): Add name_holder parameter.  Use
+ unique_xmalloc_ptr.
+ (dwarf_decode_lines): Update.
+
+2018-01-05  Tom Tromey  <[hidden email]>
+
  * dwarf2read.c (struct dwarf2_cu): Add constructor, destructor.
  <free_abbrev_table>: Declare method.
  (~auto_free_abbrev_table): Update.
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 400080b208..998c8479ea 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -633,9 +633,6 @@ struct delayed_method_info
   struct die_info *die;
 };
 
-typedef struct delayed_method_info delayed_method_info;
-DEF_VEC_O (delayed_method_info);
-
 /* Internal state when decoding a particular compilation unit.  */
 struct dwarf2_cu
 {
@@ -724,7 +721,7 @@ struct dwarf2_cu
 
   /* A list of methods which need to have physnames computed
      after all type information has been read.  */
-  VEC (delayed_method_info) *method_list = nullptr;
+  std::vector<delayed_method_info> method_list;
 
   /* To be copied to symtab->call_site_htab.  */
   htab_t call_site_htab = nullptr;
@@ -10051,20 +10048,7 @@ add_to_method_list (struct type *type, int fnfield_index, int index,
   mi.index = index;
   mi.name = name;
   mi.die = die;
-  VEC_safe_push (delayed_method_info, cu->method_list, &mi);
-}
-
-/* A cleanup for freeing the delayed method list.  */
-
-static void
-free_delayed_list (void *ptr)
-{
-  struct dwarf2_cu *cu = (struct dwarf2_cu *) ptr;
-  if (cu->method_list != NULL)
-    {
-      VEC_free (delayed_method_info, cu->method_list);
-      cu->method_list = NULL;
-    }
+  cu->method_list.push_back (mi);
 }
 
 /* Check whether [PHYSNAME, PHYSNAME+LEN) ends with a modifier like
@@ -10093,21 +10077,18 @@ check_modifier (const char *physname, size_t &len, const char (&mod)[N])
 static void
 compute_delayed_physnames (struct dwarf2_cu *cu)
 {
-  int i;
-  struct delayed_method_info *mi;
-
   /* Only C++ delays computing physnames.  */
-  if (VEC_empty (delayed_method_info, cu->method_list))
+  if (cu->method_list.empty ())
     return;
   gdb_assert (cu->language == language_cplus);
 
-  for (i = 0; VEC_iterate (delayed_method_info, cu->method_list, i, mi) ; ++i)
+  for (struct delayed_method_info &mi : cu->method_list)
     {
       const char *physname;
       struct fn_fieldlist *fn_flp
- = &TYPE_FN_FIELDLIST (mi->type, mi->fnfield_index);
-      physname = dwarf2_physname (mi->name, mi->die, cu);
-      TYPE_FN_FIELD_PHYSNAME (fn_flp->fn_fields, mi->index)
+ = &TYPE_FN_FIELDLIST (mi.type, mi.fnfield_index);
+      physname = dwarf2_physname (mi.name, mi.die, cu);
+      TYPE_FN_FIELD_PHYSNAME (fn_flp->fn_fields, mi.index)
  = physname ? physname : "";
 
       /* Since there's no tag to indicate whether a method is a
@@ -10122,14 +10103,17 @@ compute_delayed_physnames (struct dwarf2_cu *cu)
       if (physname[len] == ')') /* shortcut */
  break;
       else if (check_modifier (physname, len, " const"))
- TYPE_FN_FIELD_CONST (fn_flp->fn_fields, mi->index) = 1;
+ TYPE_FN_FIELD_CONST (fn_flp->fn_fields, mi.index) = 1;
       else if (check_modifier (physname, len, " volatile"))
- TYPE_FN_FIELD_VOLATILE (fn_flp->fn_fields, mi->index) = 1;
+ TYPE_FN_FIELD_VOLATILE (fn_flp->fn_fields, mi.index) = 1;
       else
  break;
     }
  }
     }
+
+  /* The list is no longer needed.  */
+  cu->method_list.clear ();
 }
 
 /* Go objects should be embedded in a DW_TAG_module DIE,
@@ -10364,7 +10348,6 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu,
   struct gdbarch *gdbarch = get_objfile_arch (objfile);
   CORE_ADDR lowpc, highpc;
   struct compunit_symtab *cust;
-  struct cleanup *delayed_list_cleanup;
   CORE_ADDR baseaddr;
   struct block *static_block;
   CORE_ADDR addr;
@@ -10373,7 +10356,9 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu,
 
   buildsym_init ();
   scoped_free_pendings free_pending;
-  delayed_list_cleanup = make_cleanup (free_delayed_list, cu);
+
+  /* Clear the list here in case something was left over.  */
+  cu->method_list.clear ();
 
   cu->list_in_scope = &file_symbols;
 
@@ -10391,7 +10376,6 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu,
      should be complete, and it should now be safe to compute all of the
      physnames.  */
   compute_delayed_physnames (cu);
-  do_cleanups (delayed_list_cleanup);
 
   /* Some compilers don't define a DW_AT_high_pc attribute for the
      compilation unit.  If the DW_AT_high_pc is missing, synthesize
@@ -10466,7 +10450,6 @@ process_full_type_unit (struct dwarf2_per_cu_data *per_cu,
   struct dwarf2_cu *cu = per_cu->cu;
   struct objfile *objfile = per_cu->objfile;
   struct compunit_symtab *cust;
-  struct cleanup *delayed_list_cleanup;
   struct signatured_type *sig_type;
 
   gdb_assert (per_cu->is_debug_types);
@@ -10474,7 +10457,9 @@ process_full_type_unit (struct dwarf2_per_cu_data *per_cu,
 
   buildsym_init ();
   scoped_free_pendings free_pending;
-  delayed_list_cleanup = make_cleanup (free_delayed_list, cu);
+
+  /* Clear the list here in case something was left over.  */
+  cu->method_list.clear ();
 
   cu->list_in_scope = &file_symbols;
 
@@ -10492,7 +10477,6 @@ process_full_type_unit (struct dwarf2_per_cu_data *per_cu,
      should be complete, and it should now be safe to compute all of the
      physnames.  */
   compute_delayed_physnames (cu);
-  do_cleanups (delayed_list_cleanup);
 
   /* TUs share symbol tables.
      If this is the first TU to use this symtab, complete the construction
@@ -20219,25 +20203,24 @@ dwarf_decode_line_header (sect_offset sect_off, struct dwarf2_cu *cu)
    Return the file name of the psymtab for included file FILE_INDEX
    in line header LH of PST.
    COMP_DIR is the compilation directory (DW_AT_comp_dir) or NULL if unknown.
-   If space for the result is malloc'd, it will be freed by a cleanup.
-   Returns NULL if FILE_INDEX should be ignored, i.e., it is pst->filename.
-
-   The function creates dangling cleanup registration.  */
+   If space for the result is malloc'd, *NAME_HOLDER will be set.
+   Returns NULL if FILE_INDEX should be ignored, i.e., it is pst->filename.  */
 
 static const char *
 psymtab_include_file_name (const struct line_header *lh, int file_index,
    const struct partial_symtab *pst,
-   const char *comp_dir)
+   const char *comp_dir,
+   gdb::unique_xmalloc_ptr<char> *name_holder)
 {
   const file_entry &fe = lh->file_names[file_index];
   const char *include_name = fe.name;
   const char *include_name_to_compare = include_name;
   const char *pst_filename;
-  char *copied_name = NULL;
   int file_is_pst;
 
   const char *dir_name = fe.include_dir (lh);
 
+  gdb::unique_xmalloc_ptr<char> hold_compare;
   if (!IS_ABSOLUTE_PATH (include_name)
       && (dir_name != NULL || comp_dir != NULL))
     {
@@ -20264,36 +20247,30 @@ psymtab_include_file_name (const struct line_header *lh, int file_index,
 
       if (dir_name != NULL)
  {
-  char *tem = concat (dir_name, SLASH_STRING,
-      include_name, (char *)NULL);
-
-  make_cleanup (xfree, tem);
-  include_name = tem;
+  name_holder->reset (concat (dir_name, SLASH_STRING,
+      include_name, (char *) NULL));
+  include_name = name_holder->get ();
   include_name_to_compare = include_name;
  }
       if (!IS_ABSOLUTE_PATH (include_name) && comp_dir != NULL)
  {
-  char *tem = concat (comp_dir, SLASH_STRING,
-      include_name, (char *)NULL);
-
-  make_cleanup (xfree, tem);
-  include_name_to_compare = tem;
+  hold_compare.reset (concat (comp_dir, SLASH_STRING,
+      include_name, (char *) NULL));
+  include_name_to_compare = hold_compare.get ();
  }
     }
 
   pst_filename = pst->filename;
+  gdb::unique_xmalloc_ptr<char> copied_name;
   if (!IS_ABSOLUTE_PATH (pst_filename) && pst->dirname != NULL)
     {
-      copied_name = concat (pst->dirname, SLASH_STRING,
-    pst_filename, (char *)NULL);
-      pst_filename = copied_name;
+      copied_name.reset (concat (pst->dirname, SLASH_STRING,
+ pst_filename, (char *) NULL));
+      pst_filename = copied_name.get ();
     }
 
   file_is_pst = FILENAME_CMP (include_name_to_compare, pst_filename) == 0;
 
-  if (copied_name != NULL)
-    xfree (copied_name);
-
   if (file_is_pst)
     return NULL;
   return include_name;
@@ -20951,8 +20928,10 @@ dwarf_decode_lines (struct line_header *lh, const char *comp_dir,
       for (file_index = 0; file_index < lh->file_names.size (); file_index++)
         if (lh->file_names[file_index].included_p == 1)
           {
+    gdb::unique_xmalloc_ptr<char> name_holder;
     const char *include_name =
-      psymtab_include_file_name (lh, file_index, pst, comp_dir);
+      psymtab_include_file_name (lh, file_index, pst, comp_dir,
+ &name_holder);
     if (include_name != NULL)
               dwarf2_create_include_psymtab (include_name, pst, objfile);
           }
--
2.13.6

Reply | Threaded
Open this post in threaded view
|

[RFA 5/6] Remove objfile argument from add_dyn_prop

Tom Tromey-2
In reply to this post by Tom Tromey-2
The objfile argument to add_dyn_prop is redundant, so this patch
removes it.

2018-01-05  Tom Tromey  <[hidden email]>

        * gdbtypes.h (add_dyn_prop): Remove objfile parameter.
        * gdbtypes.c (add_dyn_prop): Remove objfile parameter.
        (create_array_type_with_stride): Update.
        * dwarf2read.c (set_die_type): Update.
---
 gdb/ChangeLog    | 7 +++++++
 gdb/dwarf2read.c | 6 +++---
 gdb/gdbtypes.c   | 7 +++----
 gdb/gdbtypes.h   | 5 ++---
 4 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0c878d9aba..e9b6023047 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,12 @@
 2018-01-05  Tom Tromey  <[hidden email]>
 
+ * gdbtypes.h (add_dyn_prop): Remove objfile parameter.
+ * gdbtypes.c (add_dyn_prop): Remove objfile parameter.
+ (create_array_type_with_stride): Update.
+ * dwarf2read.c (set_die_type): Update.
+
+2018-01-05  Tom Tromey  <[hidden email]>
+
  * dwarf2read.c (delayed_method_info): Remove typedef.
  (dwarf2_cu::method_info): Now a std::vector.
  (add_to_method_list): Update.
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 998c8479ea..3807251970 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -25116,7 +25116,7 @@ set_die_type (struct die_info *die, struct type *type, struct dwarf2_cu *cu)
   if (attr_form_is_block (attr))
     {
       if (attr_to_dynamic_prop (attr, die, cu, &prop))
-        add_dyn_prop (DYN_PROP_ALLOCATED, prop, type, objfile);
+        add_dyn_prop (DYN_PROP_ALLOCATED, prop, type);
     }
   else if (attr != NULL)
     {
@@ -25131,7 +25131,7 @@ set_die_type (struct die_info *die, struct type *type, struct dwarf2_cu *cu)
   if (attr_form_is_block (attr))
     {
       if (attr_to_dynamic_prop (attr, die, cu, &prop))
-        add_dyn_prop (DYN_PROP_ASSOCIATED, prop, type, objfile);
+        add_dyn_prop (DYN_PROP_ASSOCIATED, prop, type);
     }
   else if (attr != NULL)
     {
@@ -25144,7 +25144,7 @@ set_die_type (struct die_info *die, struct type *type, struct dwarf2_cu *cu)
   /* Read DW_AT_data_location and set in type.  */
   attr = dwarf2_attr (die, DW_AT_data_location, cu);
   if (attr_to_dynamic_prop (attr, die, cu, &prop))
-    add_dyn_prop (DYN_PROP_DATA_LOCATION, prop, type, objfile);
+    add_dyn_prop (DYN_PROP_DATA_LOCATION, prop, type);
 
   if (dwarf2_per_objfile->die_type_hash == NULL)
     {
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 7ba62df474..c438696217 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -1166,8 +1166,7 @@ create_array_type_with_stride (struct type *result_type,
     (struct field *) TYPE_ZALLOC (result_type, sizeof (struct field));
   TYPE_INDEX_TYPE (result_type) = range_type;
   if (byte_stride_prop != NULL)
-    add_dyn_prop (DYN_PROP_BYTE_STRIDE, *byte_stride_prop, result_type,
-  TYPE_OBJFILE (result_type));
+    add_dyn_prop (DYN_PROP_BYTE_STRIDE, *byte_stride_prop, result_type);
   else if (bit_stride > 0)
     TYPE_FIELD_BITSIZE (result_type, 0) = bit_stride;
 
@@ -2305,13 +2304,13 @@ get_dyn_prop (enum dynamic_prop_node_kind prop_kind, const struct type *type)
 
 void
 add_dyn_prop (enum dynamic_prop_node_kind prop_kind, struct dynamic_prop prop,
-              struct type *type, struct objfile *objfile)
+              struct type *type)
 {
   struct dynamic_prop_list *temp;
 
   gdb_assert (TYPE_OBJFILE_OWNED (type));
 
-  temp = XOBNEW (&objfile->objfile_obstack, struct dynamic_prop_list);
+  temp = XOBNEW (&TYPE_OBJFILE (type)->objfile_obstack, struct dynamic_prop_list);
   temp->prop_kind = prop_kind;
   temp->prop = prop;
   temp->next = TYPE_DYN_PROP_LIST (type);
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 5942b5ad48..712b16b698 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1841,11 +1841,10 @@ extern struct dynamic_prop *get_dyn_prop
 /* * Given a dynamic property PROP of a given KIND, add this dynamic
    property to the given TYPE.
 
-   This function assumes that TYPE is objfile-owned, and that OBJFILE
-   is the TYPE's objfile.  */
+   This function assumes that TYPE is objfile-owned.  */
 extern void add_dyn_prop
   (enum dynamic_prop_node_kind kind, struct dynamic_prop prop,
-   struct type *type, struct objfile *objfile);
+   struct type *type);
 
 extern void remove_dyn_prop (enum dynamic_prop_node_kind prop_kind,
                              struct type *type);
--
2.13.6

Reply | Threaded
Open this post in threaded view
|

[RFA 6/6] Remove symbolp typedef

Tom Tromey-2
In reply to this post by Tom Tromey-2
This removes the symbolp typedef from dwarf2read.c.  It is no longer
used.

2018-01-05  Tom Tromey  <[hidden email]>

        * dwarf2read.c (symbolp): Remove typedef.  Don't instantiate VEC.
---
 gdb/ChangeLog    | 4 ++++
 gdb/dwarf2read.c | 3 ---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index e9b6023047..f8aa10d50a 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,9 @@
 2018-01-05  Tom Tromey  <[hidden email]>
 
+ * dwarf2read.c (symbolp): Remove typedef.  Don't instantiate VEC.
+
+2018-01-05  Tom Tromey  <[hidden email]>
+
  * gdbtypes.h (add_dyn_prop): Remove objfile parameter.
  * gdbtypes.c (add_dyn_prop): Remove objfile parameter.
  (create_array_type_with_stride): Update.
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 3807251970..93e2419cc9 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -87,9 +87,6 @@
 #include <set>
 #include <forward_list>
 
-typedef struct symbol *symbolp;
-DEF_VEC_P (symbolp);
-
 /* When == 1, print basic high level tracing messages.
    When > 1, be more verbose.
    This is in contrast to the low level DIE reading of dwarf_die_debug.  */
--
2.13.6

Reply | Threaded
Open this post in threaded view
|

Re: [RFA 2/6] Allocate abbrev_table with new

Simon Marchi-2
In reply to this post by Tom Tromey-2
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 <[hidden email]>
Date: Sat, 6 Jan 2018 22:44:29 -0500
Subject: [PATCH] Suggestion

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

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)
--
2.7.4



Reply | Threaded
Open this post in threaded view
|

Re: [RFA 1/6] Unify new_symbol and new_symbol_full

Simon Marchi-2
In reply to this post by Tom Tromey-2
On 2018-01-05 07:26 PM, Tom Tromey wrote:

> This patch unifies new_symbol with new_symbol_full, replacing a
> wrapper function with a default parameter.
>
> 2018-01-05  Tom Tromey  <[hidden email]>
>
> * dwarf2read.c (dwarf2_compute_name): Update comment.
> (read_func_scope, read_variable): Update.
> (new_symbol): Remove.
> (new_symbol_full): Rename to new_symbol.
> ---
>  gdb/ChangeLog    |  7 +++++++
>  gdb/dwarf2read.c | 29 +++++++++--------------------
>  2 files changed, 16 insertions(+), 20 deletions(-)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 24ccfe6882..5ad43d963a 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,10 @@
> +2018-01-05  Tom Tromey  <[hidden email]>
> +
> + * dwarf2read.c (dwarf2_compute_name): Update comment.
> + (read_func_scope, read_variable): Update.
> + (new_symbol): Remove.
> + (new_symbol_full): Rename to new_symbol.
> +
>  2018-01-05  Pedro Alves  <[hidden email]>
>  
>   PR gdb/18653
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index a3028e5c52..92c4903241 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -1862,10 +1862,7 @@ static struct compunit_symtab *dwarf2_start_symtab (struct dwarf2_cu *,
>      CORE_ADDR);
>  
>  static struct symbol *new_symbol (struct die_info *, struct type *,
> -  struct dwarf2_cu *);
> -
> -static struct symbol *new_symbol_full (struct die_info *, struct type *,
> -       struct dwarf2_cu *, struct symbol *);
> +  struct dwarf2_cu *, struct symbol * = NULL);
>  
>  static void dwarf2_const_value (const struct attribute *, struct symbol *,
>   struct dwarf2_cu *);
> @@ -10831,7 +10828,7 @@ dwarf2_compute_name (const char *name,
>       but otherwise compute it by typename_concat inside GDB.
>       FIXME: Actually this is not really true, or at least not always true.
>       It's all very confusing.  SYMBOL_SET_NAMES doesn't try to demangle
> -     Fortran names because there is no mangling standard.  So new_symbol_full
> +     Fortran names because there is no mangling standard.  So new_symbol
>       will set the demangled name to the result of dwarf2_full_name, and it is
>       the demangled name that GDB uses if it exists.  */
>    if (cu->language == language_ada
> @@ -11104,8 +11101,8 @@ dwarf2_physname (const char *name, struct die_info *die, struct dwarf2_cu *cu)
>  
>        if (cu->language == language_go)
>   {
> -  /* This is a lie, but we already lie to the caller new_symbol_full.
> -     new_symbol_full assumes we return the mangled name.
> +  /* This is a lie, but we already lie to the caller new_symbol.
> +     new_symbol assumes we return the mangled name.
>       This just undoes that lie until things are cleaned up.  */
>   }
>        else
> @@ -13731,8 +13728,8 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
>      }
>  
>    newobj = push_context (0, lowpc);
> -  newobj->name = new_symbol_full (die, read_type_die (die, cu), cu,
> -       (struct symbol *) templ_func);
> +  newobj->name = new_symbol (die, read_type_die (die, cu), cu,
> +     (struct symbol *) templ_func);
>  
>    /* If there is a location expression for DW_AT_frame_base, record
>       it.  */
> @@ -14287,7 +14284,7 @@ read_variable (struct die_info *die, struct dwarf2_cu *cu)
>   }
>      }
>  
> -  new_symbol_full (die, NULL, cu, storage);
> +  new_symbol (die, NULL, cu, storage);
>  }
>  
>  /* Call CALLBACK from DW_AT_ranges attribute value OFFSET
> @@ -21183,8 +21180,8 @@ var_decode_location (struct attribute *attr, struct symbol *sym,
>     NULL, allocate a new symbol on the objfile's obstack.  */
>  
>  static struct symbol *
> -new_symbol_full (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
> - struct symbol *space)
> +new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
> +    struct symbol *space)
>  {
>    struct objfile *objfile = cu->objfile;
>    struct gdbarch *gdbarch = get_objfile_arch (objfile);
> @@ -21564,14 +21561,6 @@ new_symbol_full (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>    return (sym);
>  }
>  
> -/* A wrapper for new_symbol_full that always allocates a new symbol.  */
> -
> -static struct symbol *
> -new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu)
> -{
> -  return new_symbol_full (die, type, cu, NULL);
> -}
> -
>  /* Given an attr with a DW_FORM_dataN value in host byte order,
>     zero-extend it as appropriate for the symbol's type.  The DWARF
>     standard (v4) is not entirely clear about the meaning of using
>

LGTM, this one can go in by itself I think.

Simon
Reply | Threaded
Open this post in threaded view
|

Re: [RFA 2/6] Allocate abbrev_table with new

Simon Marchi-2
In reply to this post by Simon Marchi-2
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
Reply | Threaded
Open this post in threaded view
|

Re: [RFA 2/6] Allocate abbrev_table with new

Tom Tromey-2
>>>>> "Simon" == Simon Marchi <[hidden email]> writes:

Simon> and here's your series plus my suggestion, rebased on master:
Simon> https://github.com/simark/binutils-gdb/tree/tromey/dwarf-plus-suggestion

Thanks.  Here it is squashed with a new ChangeLog.
I ran the new series through the buildbot as well.

Tom

commit 346e83177f3f9dc9fe7021f5f59325dda1e8d200
Author: Tom Tromey <[hidden email]>
Date:   Fri Jan 5 17:26:17 2018 -0700

    Allocate abbrev_table with new
   
    This changes dwarf2read.c to allocate abbrev tables using "new", and
    then updates the users.
   
    This version of the patch incorporates the changes that Simon
    implemented.  These changes simplify the ownership rules for abbrev
    tables.
   
    2018-01-05  Tom Tromey  <[hidden email]>
                Simon Marchi  <[hidden email]>
   
            * dwarf2read.c (struct dwarf2_cu) <abbrev_table>: Remove.
            (struct die_reader_specs) <abbrev_table>: New member.
            (struct abbrev_table): Add constructor.
            <alloc_abbrev, add_abbrev, lookup_abbrev>: Declare.
            <abbrev_obstack>: Now an auto_obstack.
            (abbrev_table_up): New typedef.
            (init_cu_die_reader): Add abbrev_table parameter.
            (read_cutu_die_from_dwo): Remove abbrev_table_provided parameter.
            Add result_dwo_abbrev_table.
            (init_tu_and_read_dwo_dies, init_cutu_and_read_dies)
            (init_cutu_and_read_dies_no_follow, build_type_psymtabs_1):
            Update.
            (peek_die_abbrev): Take die_reader_specs, not dwarf_cu as
            parameter.
            (skip_children): Update.
            (abbrev_table::alloc_abbrev): Rename from
            abbrev_table_alloc_abbrev.
            (abbrev_table::add_abbrev): Rename from abbrev_table_add_abbrev.
            (abbrev_table::lookup_abbrev): Rename from
            abbrev_table_lookup_abbrev.
            (abbrev_table_read_table): Return abbrev_table_up.
            (abbrev_table_free, abbrev_table_free_cleanup)
            (dwarf2_read_abbrevs, dwarf2_free_abbrev_table): Remove.
            (load_partial_dies): Update.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 6e4b285077..20df6c9ef5 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,4 +1,32 @@
 2018-01-05  Tom Tromey  <[hidden email]>
+    Simon Marchi  <[hidden email]>
+
+ * dwarf2read.c (struct dwarf2_cu) <abbrev_table>: Remove.
+ (struct die_reader_specs) <abbrev_table>: New member.
+ (struct abbrev_table): Add constructor.
+ <alloc_abbrev, add_abbrev, lookup_abbrev>: Declare.
+ <abbrev_obstack>: Now an auto_obstack.
+ (abbrev_table_up): New typedef.
+ (init_cu_die_reader): Add abbrev_table parameter.
+ (read_cutu_die_from_dwo): Remove abbrev_table_provided parameter.
+ Add result_dwo_abbrev_table.
+ (init_tu_and_read_dwo_dies, init_cutu_and_read_dies)
+ (init_cutu_and_read_dies_no_follow, build_type_psymtabs_1):
+ Update.
+ (peek_die_abbrev): Take die_reader_specs, not dwarf_cu as
+ parameter.
+ (skip_children): Update.
+ (abbrev_table::alloc_abbrev): Rename from
+ abbrev_table_alloc_abbrev.
+ (abbrev_table::add_abbrev): Rename from abbrev_table_add_abbrev.
+ (abbrev_table::lookup_abbrev): Rename from
+ abbrev_table_lookup_abbrev.
+ (abbrev_table_read_table): Return abbrev_table_up.
+ (abbrev_table_free, abbrev_table_free_cleanup)
+ (dwarf2_read_abbrevs, dwarf2_free_abbrev_table): Remove.
+ (load_partial_dies): Update.
+
+2018-01-05  Tom Tromey  <[hidden email]>
 
  * dwarf2read.c (dwarf2_compute_name): Update comment.
  (read_func_scope, read_variable): Update.
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 45f7feecde..d278aeb30d 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -687,11 +687,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;
@@ -1258,6 +1253,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.  */
@@ -1514,12 +1512,35 @@ struct attr_abbrev
 
 struct abbrev_table
 {
+  abbrev_table (sect_offset off)
+    : sect_off (off)
+  {
+    abbrevs =
+      XOBNEWVEC (&abbrev_obstack, struct abbrev_info *, ABBREV_HASH_SIZE);
+    memset (abbrevs, 0, ABBREV_HASH_SIZE * sizeof (struct abbrev_info *));
+  }
+
+  DISABLE_COPY_AND_ASSIGN (abbrev_table);
+
+  /* Allocate space for a struct abbrev_info object in
+     ABBREV_TABLE.  */
+  struct abbrev_info *alloc_abbrev ();
+
+  /* Add an abbreviation to the table.  */
+  void add_abbrev (unsigned int abbrev_number, struct abbrev_info *abbrev);
+
+  /* Look up an abbrev in the table.
+     Returns NULL if the abbrev is not found.  */
+
+  struct abbrev_info *lookup_abbrev (unsigned int abbrev_number);
+
+
   /* Where the abbrev table came from.
      This is used as a sanity check when the table is used.  */
   sect_offset sect_off;
 
   /* Storage for the abbrev table.  */
-  struct obstack abbrev_obstack;
+  auto_obstack abbrev_obstack;
 
   /* Hash table of abbrevs.
      This is an array of size ABBREV_HASH_SIZE allocated in abbrev_obstack.
@@ -1528,6 +1549,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
   {
@@ -1757,22 +1780,10 @@ static void dwarf2_read_symtab (struct partial_symtab *,
 
 static void psymtab_to_symtab_1 (struct partial_symtab *);
 
-static struct abbrev_info *abbrev_table_lookup_abbrev
-  (const struct abbrev_table *, unsigned int);
-
-static struct abbrev_table *abbrev_table_read_table
+static abbrev_table_up abbrev_table_read_table
   (struct dwarf2_per_objfile *dwarf2_per_objfile, struct dwarf2_section_info *,
    sect_offset);
 
-static void abbrev_table_free (struct abbrev_table *);
-
-static void abbrev_table_free_cleanup (void *);
-
-static void dwarf2_read_abbrevs (struct dwarf2_cu *,
- struct dwarf2_section_info *);
-
-static void dwarf2_free_abbrev_table (void *);
-
 static unsigned int peek_abbrev_code (bfd *, const gdb_byte *);
 
 static struct partial_die_info *load_partial_dies
@@ -7421,7 +7432,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);
@@ -7431,6 +7443,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.
@@ -7446,25 +7459,26 @@ 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.
+   *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)
+ int *result_has_children,
+ abbrev_table_up *result_dwo_abbrev_table)
 {
   struct dwarf2_per_objfile *dwarf2_per_objfile = this_cu->dwarf2_per_objfile;
   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;
@@ -7527,13 +7541,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)
     {
@@ -7576,22 +7589,11 @@ 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.  */
-      make_cleanup (dwarf2_free_abbrev_table, 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 (dwarf2_per_objfile, 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
@@ -7745,12 +7747,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.  */
 
+  /* 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) == 0)
+      &comp_unit_die, &has_children,
+      &dwo_abbrev_table) == 0)
     {
       /* Dummy die.  */
       do_cleanups (cleanups);
@@ -7772,10 +7778,6 @@ init_tu_and_read_dwo_dies (struct dwarf2_per_cu_data *this_cu,
      caller clean it up when finished with it.  */
   discard_cleanups (free_cu_cleanup);
 
-  /* We can only discard free_cu_cleanup and all subsequent cleanups.
-     So we have to manually free the abbrev table.  */
-  dwarf2_free_abbrev_table (cu);
-
   /* Link this CU into read_in_chain.  */
   this_cu->cu->read_in_chain = dwarf2_per_objfile->read_in_chain;
   dwarf2_per_objfile->read_in_chain = this_cu;
@@ -7936,35 +7938,32 @@ 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.  */
+     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);
-      make_cleanup (dwarf2_free_abbrev_table, cu);
-    }
-  else if (rereading_dwo_cu)
-    {
-      dwarf2_free_abbrev_table (cu);
-      dwarf2_read_abbrevs (cu, abbrev_section);
+      abbrev_table_holder
+ = abbrev_table_read_table (dwarf2_per_objfile, 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);
+  abbrev_table_up dwo_abbrev_table;
   if (attr)
     {
       struct dwo_unit *dwo_unit;
@@ -7981,10 +7980,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) == 0)
+      &dwo_comp_unit_die, &has_children,
+      &dwo_abbrev_table) == 0)
     {
       /* Dummy die.  */
       do_cleanups (cleanups);
@@ -8014,10 +8013,6 @@ init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
      caller clean it up when finished with it.  */
   discard_cleanups (free_cu_cleanup);
 
-  /* We can only discard free_cu_cleanup and all subsequent cleanups.
-     So we have to manually free the abbrev table.  */
-  dwarf2_free_abbrev_table (cu);
-
   /* Link this CU into read_in_chain.  */
   this_cu->cu->read_in_chain = dwarf2_per_objfile->read_in_chain;
   dwarf2_per_objfile->read_in_chain = this_cu;
@@ -8099,10 +8094,11 @@ init_cutu_and_read_dies_no_follow (struct dwarf2_per_cu_data *this_cu,
       return;
     }
 
-  dwarf2_read_abbrevs (&cu, abbrev_section);
-  make_cleanup (dwarf2_free_abbrev_table, &cu);
+  abbrev_table_up abbrev_table
+    = abbrev_table_read_table (dwarf2_per_objfile, 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);
@@ -8578,7 +8574,7 @@ build_type_psymtabs_1 (struct dwarf2_per_objfile *dwarf2_per_objfile)
 {
   struct tu_stats *tu_stats = &dwarf2_per_objfile->tu_stats;
   struct cleanup *cleanups;
-  struct abbrev_table *abbrev_table;
+  abbrev_table_up abbrev_table;
   sect_offset abbrev_offset;
   struct tu_abbrev_offset *sorted_by_abbrev;
   int i;
@@ -8630,8 +8626,6 @@ build_type_psymtabs_1 (struct dwarf2_per_objfile *dwarf2_per_objfile)
  sizeof (struct tu_abbrev_offset), sort_tu_by_abbrev_offset);
 
   abbrev_offset = (sect_offset) ~(unsigned) 0;
-  abbrev_table = NULL;
-  make_cleanup (abbrev_table_free_cleanup, &abbrev_table);
 
   for (i = 0; i < dwarf2_per_objfile->n_type_units; ++i)
     {
@@ -8641,13 +8635,6 @@ build_type_psymtabs_1 (struct dwarf2_per_objfile *dwarf2_per_objfile)
       if (abbrev_table == NULL
   || tu->abbrev_offset != abbrev_offset)
  {
-  if (abbrev_table != NULL)
-    {
-      abbrev_table_free (abbrev_table);
-      /* Reset to NULL in case abbrev_table_read_table throws
- an error: abbrev_table_free_cleanup will get called.  */
-      abbrev_table = NULL;
-    }
   abbrev_offset = tu->abbrev_offset;
   abbrev_table =
     abbrev_table_read_table (dwarf2_per_objfile,
@@ -8656,8 +8643,8 @@ build_type_psymtabs_1 (struct dwarf2_per_objfile *dwarf2_per_objfile)
   ++tu_stats->nr_uniq_abbrev_tables;
  }
 
-      init_cutu_and_read_dies (&tu->sig_type->per_cu, abbrev_table, 0, 0,
-       build_type_psymtabs_reader, NULL);
+      init_cutu_and_read_dies (&tu->sig_type->per_cu, abbrev_table.get (),
+       0, 0, build_type_psymtabs_reader, NULL);
     }
 
   do_cleanups (cleanups);
@@ -9606,25 +9593,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->per_cu->dwarf2_per_objfile->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 = abbrev_table_lookup_abbrev (cu->abbrev_table, 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"
@@ -9643,13 +9631,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
@@ -18015,7 +18001,7 @@ read_full_die_1 (const struct die_reader_specs *reader,
       return info_ptr;
     }
 
-  abbrev = abbrev_table_lookup_abbrev (cu->abbrev_table, 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,
@@ -18076,12 +18062,12 @@ read_full_die (const struct die_reader_specs *reader,
 
 /* Allocate space for a struct abbrev_info object in ABBREV_TABLE.  */
 
-static struct abbrev_info *
-abbrev_table_alloc_abbrev (struct abbrev_table *abbrev_table)
+struct abbrev_info *
+abbrev_table::alloc_abbrev ()
 {
   struct abbrev_info *abbrev;
 
-  abbrev = XOBNEW (&abbrev_table->abbrev_obstack, struct abbrev_info);
+  abbrev = XOBNEW (&abbrev_obstack, struct abbrev_info);
   memset (abbrev, 0, sizeof (struct abbrev_info));
 
   return abbrev;
@@ -18089,30 +18075,28 @@ abbrev_table_alloc_abbrev (struct abbrev_table *abbrev_table)
 
 /* Add an abbreviation to the table.  */
 
-static void
-abbrev_table_add_abbrev (struct abbrev_table *abbrev_table,
- unsigned int abbrev_number,
- struct abbrev_info *abbrev)
+void
+abbrev_table::add_abbrev (unsigned int abbrev_number,
+  struct abbrev_info *abbrev)
 {
   unsigned int hash_number;
 
   hash_number = abbrev_number % ABBREV_HASH_SIZE;
-  abbrev->next = abbrev_table->abbrevs[hash_number];
-  abbrev_table->abbrevs[hash_number] = abbrev;
+  abbrev->next = abbrevs[hash_number];
+  abbrevs[hash_number] = abbrev;
 }
 
 /* Look up an abbrev in the table.
    Returns NULL if the abbrev is not found.  */
 
-static struct abbrev_info *
-abbrev_table_lookup_abbrev (const struct abbrev_table *abbrev_table,
-    unsigned int abbrev_number)
+struct abbrev_info *
+abbrev_table::lookup_abbrev (unsigned int abbrev_number)
 {
   unsigned int hash_number;
   struct abbrev_info *abbrev;
 
   hash_number = abbrev_number % ABBREV_HASH_SIZE;
-  abbrev = abbrev_table->abbrevs[hash_number];
+  abbrev = abbrevs[hash_number];
 
   while (abbrev)
     {
@@ -18125,14 +18109,13 @@ abbrev_table_lookup_abbrev (const struct abbrev_table *abbrev_table,
 
 /* Read in an abbrev table.  */
 
-static struct abbrev_table *
+static abbrev_table_up
 abbrev_table_read_table (struct dwarf2_per_objfile *dwarf2_per_objfile,
  struct dwarf2_section_info *section,
  sect_offset sect_off)
 {
   struct objfile *objfile = dwarf2_per_objfile->objfile;
   bfd *abfd = get_section_bfd_owner (section);
-  struct abbrev_table *abbrev_table;
   const gdb_byte *abbrev_ptr;
   struct abbrev_info *cur_abbrev;
   unsigned int abbrev_number, bytes_read, abbrev_name;
@@ -18140,14 +18123,7 @@ abbrev_table_read_table (struct dwarf2_per_objfile *dwarf2_per_objfile,
   struct attr_abbrev *cur_attrs;
   unsigned int allocated_attrs;
 
-  abbrev_table = XNEW (struct abbrev_table);
-  abbrev_table->sect_off = sect_off;
-  obstack_init (&abbrev_table->abbrev_obstack);
-  abbrev_table->abbrevs =
-    XOBNEWVEC (&abbrev_table->abbrev_obstack, struct abbrev_info *,
-       ABBREV_HASH_SIZE);
-  memset (abbrev_table->abbrevs, 0,
-  ABBREV_HASH_SIZE * sizeof (struct abbrev_info *));
+  abbrev_table_up abbrev_table (new struct abbrev_table (sect_off));
 
   dwarf2_read_section (objfile, section);
   abbrev_ptr = section->buffer + to_underlying (sect_off);
@@ -18160,7 +18136,7 @@ abbrev_table_read_table (struct dwarf2_per_objfile *dwarf2_per_objfile,
   /* Loop until we reach an abbrev number of 0.  */
   while (abbrev_number)
     {
-      cur_abbrev = abbrev_table_alloc_abbrev (abbrev_table);
+      cur_abbrev = abbrev_table->alloc_abbrev ();
 
       /* read in abbrev header */
       cur_abbrev->number = abbrev_number;
@@ -18215,7 +18191,7 @@ abbrev_table_read_table (struct dwarf2_per_objfile *dwarf2_per_objfile,
       memcpy (cur_abbrev->attrs, cur_attrs,
       cur_abbrev->num_attrs * sizeof (struct attr_abbrev));
 
-      abbrev_table_add_abbrev (abbrev_table, abbrev_number, cur_abbrev);
+      abbrev_table->add_abbrev (abbrev_number, cur_abbrev);
 
       /* Get next abbreviation.
          Under Irix6 the abbreviations for a compilation unit are not
@@ -18228,7 +18204,7 @@ abbrev_table_read_table (struct dwarf2_per_objfile *dwarf2_per_objfile,
  break;
       abbrev_number = read_unsigned_leb128 (abfd, abbrev_ptr, &bytes_read);
       abbrev_ptr += bytes_read;
-      if (abbrev_table_lookup_abbrev (abbrev_table, abbrev_number) != NULL)
+      if (abbrev_table->lookup_abbrev (abbrev_number) != NULL)
  break;
     }
 
@@ -18236,55 +18212,6 @@ abbrev_table_read_table (struct dwarf2_per_objfile *dwarf2_per_objfile,
   return abbrev_table;
 }
 
-/* Free the resources held by ABBREV_TABLE.  */
-
-static void
-abbrev_table_free (struct abbrev_table *abbrev_table)
-{
-  obstack_free (&abbrev_table->abbrev_obstack, NULL);
-  xfree (abbrev_table);
-}
-
-/* Same as abbrev_table_free but as a cleanup.
-   We pass in a pointer to the pointer to the table so that we can
-   set the pointer to NULL when we're done.  It also simplifies
-   build_type_psymtabs_1.  */
-
-static void
-abbrev_table_free_cleanup (void *table_ptr)
-{
-  struct abbrev_table **abbrev_table_ptr = (struct abbrev_table **) table_ptr;
-
-  if (*abbrev_table_ptr != NULL)
-    abbrev_table_free (*abbrev_table_ptr);
-  *abbrev_table_ptr = NULL;
-}
-
-/* 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 (cu->per_cu->dwarf2_per_objfile, abbrev_section,
-     cu->header.abbrev_sect_off);
-}
-
-/* Release the memory used by the abbrev table for a compilation unit.  */
-
-static void
-dwarf2_free_abbrev_table (void *ptr_to_cu)
-{
-  struct dwarf2_cu *cu = (struct dwarf2_cu *) ptr_to_cu;
-
-  if (cu->abbrev_table != NULL)
-    abbrev_table_free (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.  */
 
@@ -18327,7 +18254,6 @@ load_partial_dies (const struct die_reader_specs *reader,
   struct objfile *objfile = cu->per_cu->dwarf2_per_objfile->objfile;
   struct partial_die_info *part_die;
   struct partial_die_info *parent_die, *last_die, *first_die = NULL;
-  struct abbrev_info *abbrev;
   unsigned int bytes_read;
   unsigned int load_all = 0;
   int nesting_level = 1;
@@ -18352,7 +18278,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)
Reply | Threaded
Open this post in threaded view
|

Re: [RFA 2/6] Allocate abbrev_table with new

Yao Qi
Tom Tromey <[hidden email]> writes:

>    /* Where the abbrev table came from.
>       This is used as a sanity check when the table is used.  */
>    sect_offset sect_off;

It can be const now.

--
Yao (齐尧)
Reply | Threaded
Open this post in threaded view
|

Re: [RFA 3/6] Allocate dwarf2_cu with new

Yao Qi
In reply to this post by Tom Tromey-2
Tom Tromey <[hidden email]> writes:

>  /* Internal state when decoding a particular compilation unit.  */
>  struct dwarf2_cu
>  {
> +  dwarf2_cu (struct dwarf2_per_cu_data *per_cu);

Add "explicit"?

> +  ~dwarf2_cu ();
> +
> +  DISABLE_COPY_AND_ASSIGN (dwarf2_cu);
> +
> +  void free_abbrev_table ();

Move comments to dwarf2_free_abbrev_table here?

--
Yao (齐尧)
Reply | Threaded
Open this post in threaded view
|

Re: [RFA 2/6] Allocate abbrev_table with new

Simon Marchi
In reply to this post by Tom Tromey-2
On 2018-01-09 13:56, Tom Tromey wrote:

>>>>>> "Simon" == Simon Marchi <[hidden email]> writes:
>
> Simon> and here's your series plus my suggestion, rebased on master:
> Simon>
> https://github.com/simark/binutils-gdb/tree/tromey/dwarf-plus-suggestion
>
> Thanks.  Here it is squashed with a new ChangeLog.
> I ran the new series through the buildbot as well.
>
> Tom

Thanks, it LGTM.
Reply | Threaded
Open this post in threaded view
|

Re: [RFA 5/6] Remove objfile argument from add_dyn_prop

Yao Qi
In reply to this post by Tom Tromey-2
Tom Tromey <[hidden email]> writes:

> @@ -25116,7 +25116,7 @@ set_die_type (struct die_info *die, struct type *type, struct dwarf2_cu *cu)
>    if (attr_form_is_block (attr))
>      {
>        if (attr_to_dynamic_prop (attr, die, cu, &prop))
> -        add_dyn_prop (DYN_PROP_ALLOCATED, prop, type, objfile);
> +        add_dyn_prop (DYN_PROP_ALLOCATED, prop, type);
>      }

Before the patch, the objfile is
cu->per_cu->dwarf2_per_objfile->objfile, but after the patch, the
objfile is TYPE_OBJFILE (type), are they equivalent?

--
Yao (齐尧)
Reply | Threaded
Open this post in threaded view
|

Re: [RFA 2/6] Allocate abbrev_table with new

Tom Tromey-2
In reply to this post by Yao Qi
>> /* Where the abbrev table came from.
>> This is used as a sanity check when the table is used.  */
>> sect_offset sect_off;

Yao> It can be const now.

I made this change.  I also made the constructor explicit.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [RFA 3/6] Allocate dwarf2_cu with new

Tom Tromey-2
In reply to this post by Yao Qi
>>>>> "Yao" == Yao Qi <[hidden email]> writes:

Yao> Add "explicit"?

I made this change.

>> +  ~dwarf2_cu ();
>> +
>> +  DISABLE_COPY_AND_ASSIGN (dwarf2_cu);
>> +
>> +  void free_abbrev_table ();

Yao> Move comments to dwarf2_free_abbrev_table here?

This is gone with Simon's changes; I'll update the ChangeLog and send
the updated patch.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [RFA 3/6] Allocate dwarf2_cu with new

Tom Tromey-2
>>>>> "Tom" == Tom Tromey <[hidden email]> writes:

Tom> This is gone with Simon's changes; I'll update the ChangeLog and send
Tom> the updated patch.

Here it is.  This went through the buildbot the second time around.

Tom

commit bcaf61d8ff6a149785924223151df8ded30e8d7a
Author: Simon Marchi <[hidden email]>
Date:   Sun Jan 7 11:41:09 2018 -0500

    Allocate dwarf2_cu with new
   
    This changes dwarf2_cu to be allocated with new, and fixes up the
    users.
   
    2018-01-05  Tom Tromey  <[hidden email]>
                Simon Marchi  <[hidden email]>
   
            * dwarf2read.c (struct dwarf2_cu): Add constructor, destructor.
            (dwarf2_per_objfile::free_cached_comp_units)
            (init_tu_and_read_dwo_dies, init_cutu_and_read_dies)
            (init_cutu_and_read_dies_no_follow): Update.
            (dwarf2_cu::dwarf2_cu): Rename from init_one_comp_unit.
            (dwarf2_cu::~dwarf2_cu): New.
            (free_heap_comp_unit, free_stack_comp_unit): Remove.
            (age_cached_comp_units, free_one_cached_comp_unit): Update.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index e129879148..8947f01024 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,6 +1,18 @@
 2018-01-05  Tom Tromey  <[hidden email]>
     Simon Marchi  <[hidden email]>
 
+ * dwarf2read.c (struct dwarf2_cu): Add constructor, destructor.
+ (dwarf2_per_objfile::free_cached_comp_units)
+ (init_tu_and_read_dwo_dies, init_cutu_and_read_dies)
+ (init_cutu_and_read_dies_no_follow): Update.
+ (dwarf2_cu::dwarf2_cu): Rename from init_one_comp_unit.
+ (dwarf2_cu::~dwarf2_cu): New.
+ (free_heap_comp_unit, free_stack_comp_unit): Remove.
+ (age_cached_comp_units, free_one_cached_comp_unit): Update.
+
+2018-01-05  Tom Tromey  <[hidden email]>
+    Simon Marchi  <[hidden email]>
+
  * dwarf2read.c (struct dwarf2_cu) <abbrev_table>: Remove.
  (struct die_reader_specs) <abbrev_table>: New member.
  (struct abbrev_table): Add constructor.
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 292bba2aed..aa84691516 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -661,20 +661,25 @@ DEF_VEC_O (delayed_method_info);
 /* Internal state when decoding a particular compilation unit.  */
 struct dwarf2_cu
 {
+  explicit dwarf2_cu (struct dwarf2_per_cu_data *per_cu);
+  ~dwarf2_cu ();
+
+  DISABLE_COPY_AND_ASSIGN (dwarf2_cu);
+
   /* The header of the compilation unit.  */
-  struct comp_unit_head header;
+  struct comp_unit_head header {};
 
   /* Base address of this compilation unit.  */
-  CORE_ADDR base_address;
+  CORE_ADDR base_address = 0;
 
   /* Non-zero if base_address has been set.  */
-  int base_known;
+  int base_known = 0;
 
   /* The language we are debugging.  */
-  enum language language;
-  const struct language_defn *language_defn;
+  enum language language = language_unknown;
+  const struct language_defn *language_defn = nullptr;
 
-  const char *producer;
+  const char *producer = nullptr;
 
   /* The generic symbol table building routines have separate lists for
      file scope symbols and all all other scopes (local scopes).  So
@@ -685,55 +690,55 @@ struct dwarf2_cu
      first local scope, and all other local scopes as nested local
      scopes, and worked fine.  Check to see if we really need to
      distinguish these in buildsym.c.  */
-  struct pending **list_in_scope;
+  struct pending **list_in_scope = nullptr;
 
   /* Hash table holding all the loaded partial DIEs
      with partial_die->offset.SECT_OFF as hash.  */
-  htab_t partial_dies;
+  htab_t partial_dies = nullptr;
 
   /* Storage for things with the same lifetime as this read-in compilation
      unit, including partial DIEs.  */
-  struct obstack comp_unit_obstack;
+  auto_obstack comp_unit_obstack;
 
   /* When multiple dwarf2_cu structures are living in memory, this field
      chains them all together, so that they can be released efficiently.
      We will probably also want a generation counter so that most-recently-used
      compilation units are cached...  */
-  struct dwarf2_per_cu_data *read_in_chain;
+  struct dwarf2_per_cu_data *read_in_chain = nullptr;
 
   /* Backlink to our per_cu entry.  */
   struct dwarf2_per_cu_data *per_cu;
 
   /* How many compilation units ago was this CU last referenced?  */
-  int last_used;
+  int last_used = 0;
 
   /* A hash table of DIE cu_offset for following references with
      die_info->offset.sect_off as hash.  */
-  htab_t die_hash;
+  htab_t die_hash = nullptr;
 
   /* Full DIEs if read in.  */
-  struct die_info *dies;
+  struct die_info *dies = nullptr;
 
   /* A set of pointers to dwarf2_per_cu_data objects for compilation
      units referenced by this one.  Only set during full symbol processing;
      partial symbol tables do not have dependencies.  */
-  htab_t dependencies;
+  htab_t dependencies = nullptr;
 
   /* Header data from the line table, during full symbol processing.  */
-  struct line_header *line_header;
+  struct line_header *line_header = nullptr;
   /* Non-NULL if LINE_HEADER is owned by this DWARF_CU.  Otherwise,
      it's owned by dwarf2_per_objfile::line_header_hash.  If non-NULL,
      this is the DW_TAG_compile_unit die for this CU.  We'll hold on
      to the line header as long as this DIE is being processed.  See
      process_die_scope.  */
-  die_info *line_header_die_owner;
+  die_info *line_header_die_owner = nullptr;
 
   /* A list of methods which need to have physnames computed
      after all type information has been read.  */
-  VEC (delayed_method_info) *method_list;
+  VEC (delayed_method_info) *method_list = nullptr;
 
   /* To be copied to symtab->call_site_htab.  */
-  htab_t call_site_htab;
+  htab_t call_site_htab = nullptr;
 
   /* Non-NULL if this CU came from a DWO file.
      There is an invariant here that is important to remember:
@@ -744,12 +749,12 @@ struct dwarf2_cu
      is moot), or there is and either we're not going to read it (in which
      case this is NULL) or there is and we are reading it (in which case this
      is non-NULL).  */
-  struct dwo_unit *dwo_unit;
+  struct dwo_unit *dwo_unit = nullptr;
 
   /* The DW_AT_addr_base attribute if present, zero otherwise
      (zero is a valid value though).
      Note this value comes from the Fission stub CU/TU's DIE.  */
-  ULONGEST addr_base;
+  ULONGEST addr_base = 0;
 
   /* The DW_AT_ranges_base attribute if present, zero otherwise
      (zero is a valid value though).
@@ -761,7 +766,7 @@ struct dwarf2_cu
      DW_AT_ranges appeared in the DW_TAG_compile_unit of DWO DIEs: then
      DW_AT_ranges_base *would* have to be applied, and we'd have to care
      whether the DW_AT_ranges attribute came from the skeleton or DWO.  */
-  ULONGEST ranges_base;
+  ULONGEST ranges_base = 0;
 
   /* Mark used when releasing cached dies.  */
   unsigned int mark : 1;
@@ -2138,8 +2143,6 @@ static const gdb_byte *skip_one_die (const struct die_reader_specs *reader,
      const gdb_byte *info_ptr,
      struct abbrev_info *abbrev);
 
-static void free_stack_comp_unit (void *);
-
 static hashval_t partial_die_hash (const void *item);
 
 static int partial_die_eq (const void *item_lhs, const void *item_rhs);
@@ -2148,15 +2151,10 @@ static struct dwarf2_per_cu_data *dwarf2_find_containing_comp_unit
   (sect_offset sect_off, unsigned int offset_in_dwz,
    struct dwarf2_per_objfile *dwarf2_per_objfile);
 
-static void init_one_comp_unit (struct dwarf2_cu *cu,
- struct dwarf2_per_cu_data *per_cu);
-
 static void prepare_one_comp_unit (struct dwarf2_cu *cu,
    struct die_info *comp_unit_die,
    enum language pretend_language);
 
-static void free_heap_comp_unit (void *);
-
 static void free_cached_comp_units (void *);
 
 static void age_cached_comp_units (struct dwarf2_per_objfile *dwarf2_per_objfile);
@@ -2449,7 +2447,7 @@ dwarf2_per_objfile::free_cached_comp_units ()
     {
       dwarf2_per_cu_data *next_cu = per_cu->cu->read_in_chain;
 
-      free_heap_comp_unit (per_cu->cu);
+      delete per_cu->cu;
       *last_chain = next_cu;
       per_cu = next_cu;
     }
@@ -7696,12 +7694,7 @@ lookup_dwo_unit (struct dwarf2_per_cu_data *this_cu,
 
 /* Subroutine of init_cutu_and_read_dies to simplify it.
    See it for a description of the parameters.
-   Read a TU directly from a DWO file, bypassing the stub.
-
-   Note: This function could be a little bit simpler if we shared cleanups
-   with our caller, init_cutu_and_read_dies.  That's generally a fragile thing
-   to do, so we keep this function self-contained.  Or we could move this
-   into our caller, but it's complex enough already.  */
+   Read a TU directly from a DWO file, bypassing the stub.  */
 
 static void
 init_tu_and_read_dwo_dies (struct dwarf2_per_cu_data *this_cu,
@@ -7709,9 +7702,8 @@ init_tu_and_read_dwo_dies (struct dwarf2_per_cu_data *this_cu,
    die_reader_func_ftype *die_reader_func,
    void *data)
 {
-  struct dwarf2_cu *cu;
+  std::unique_ptr<dwarf2_cu> new_cu;
   struct signatured_type *sig_type;
-  struct cleanup *cleanups, *free_cu_cleanup = NULL;
   struct die_reader_specs reader;
   const gdb_byte *info_ptr;
   struct die_info *comp_unit_die;
@@ -7724,12 +7716,9 @@ init_tu_and_read_dwo_dies (struct dwarf2_per_cu_data *this_cu,
   sig_type = (struct signatured_type *) this_cu;
   gdb_assert (sig_type->dwo_unit != NULL);
 
-  cleanups = make_cleanup (null_cleanup, NULL);
-
   if (use_existing_cu && this_cu->cu != NULL)
     {
       gdb_assert (this_cu->cu->dwo_unit == sig_type->dwo_unit);
-      cu = this_cu->cu;
       /* There's no need to do the rereading_dwo_cu handling that
  init_cutu_and_read_dies does since we don't read the stub.  */
     }
@@ -7737,10 +7726,7 @@ init_tu_and_read_dwo_dies (struct dwarf2_per_cu_data *this_cu,
     {
       /* If !use_existing_cu, this_cu->cu must be NULL.  */
       gdb_assert (this_cu->cu == NULL);
-      cu = XNEW (struct dwarf2_cu);
-      init_one_comp_unit (cu, this_cu);
-      /* If an error occurs while loading, release our storage.  */
-      free_cu_cleanup = make_cleanup (free_heap_comp_unit, cu);
+      new_cu.reset (new dwarf2_cu (this_cu));
     }
 
   /* A future optimization, if needed, would be to use an existing
@@ -7759,7 +7745,6 @@ init_tu_and_read_dwo_dies (struct dwarf2_per_cu_data *this_cu,
       &dwo_abbrev_table) == 0)
     {
       /* Dummy die.  */
-      do_cleanups (cleanups);
       return;
     }
 
@@ -7770,23 +7755,14 @@ init_tu_and_read_dwo_dies (struct dwarf2_per_cu_data *this_cu,
      but the alternative is making the latter more complex.
      This function is only for the special case of using DWO files directly:
      no point in overly complicating the general case just to handle this.  */
-  if (free_cu_cleanup != NULL)
+  if (new_cu != NULL && keep)
     {
-      if (keep)
- {
-  /* We've successfully allocated this compilation unit.  Let our
-     caller clean it up when finished with it.  */
-  discard_cleanups (free_cu_cleanup);
-
-  /* Link this CU into read_in_chain.  */
-  this_cu->cu->read_in_chain = dwarf2_per_objfile->read_in_chain;
-  dwarf2_per_objfile->read_in_chain = this_cu;
- }
-      else
- do_cleanups (free_cu_cleanup);
+      /* Link this CU into read_in_chain.  */
+      this_cu->cu->read_in_chain = dwarf2_per_objfile->read_in_chain;
+      dwarf2_per_objfile->read_in_chain = this_cu;
+      /* The chain owns it now.  */
+      new_cu.release ();
     }
-
-  do_cleanups (cleanups);
 }
 
 /* Initialize a CU (or TU) and read its DIEs.
@@ -7822,7 +7798,6 @@ init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
   struct die_info *comp_unit_die;
   int has_children;
   struct attribute *attr;
-  struct cleanup *cleanups, *free_cu_cleanup = NULL;
   struct signatured_type *sig_type = NULL;
   struct dwarf2_section_info *abbrev_section;
   /* Non-zero if CU currently points to a DWO file and we need to
@@ -7850,8 +7825,6 @@ init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
       return;
     }
 
-  cleanups = make_cleanup (null_cleanup, NULL);
-
   /* This is cheap if the section is already read in.  */
   dwarf2_read_section (objfile, section);
 
@@ -7859,6 +7832,7 @@ init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
 
   abbrev_section = get_abbrev_section_for_cu (this_cu);
 
+  std::unique_ptr<dwarf2_cu> new_cu;
   if (use_existing_cu && this_cu->cu != NULL)
     {
       cu = this_cu->cu;
@@ -7875,10 +7849,8 @@ init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
     {
       /* If !use_existing_cu, this_cu->cu must be NULL.  */
       gdb_assert (this_cu->cu == NULL);
-      cu = XNEW (struct dwarf2_cu);
-      init_one_comp_unit (cu, this_cu);
-      /* If an error occurs while loading, release our storage.  */
-      free_cu_cleanup = make_cleanup (free_heap_comp_unit, cu);
+      new_cu.reset (new dwarf2_cu (this_cu));
+      cu = new_cu.get ();
     }
 
   /* Get the header.  */
@@ -7932,7 +7904,6 @@ init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
   if (info_ptr >= begin_info_ptr + this_cu->length
       || peek_abbrev_code (abfd, info_ptr) == 0)
     {
-      do_cleanups (cleanups);
       return;
     }
 
@@ -7986,7 +7957,6 @@ init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
       &dwo_abbrev_table) == 0)
     {
       /* Dummy die.  */
-      do_cleanups (cleanups);
       return;
     }
   comp_unit_die = dwo_comp_unit_die;
@@ -8005,23 +7975,14 @@ init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
   die_reader_func (&reader, info_ptr, comp_unit_die, has_children, data);
 
   /* Done, clean up.  */
-  if (free_cu_cleanup != NULL)
+  if (new_cu != NULL && keep)
     {
-      if (keep)
- {
-  /* We've successfully allocated this compilation unit.  Let our
-     caller clean it up when finished with it.  */
-  discard_cleanups (free_cu_cleanup);
-
-  /* Link this CU into read_in_chain.  */
-  this_cu->cu->read_in_chain = dwarf2_per_objfile->read_in_chain;
-  dwarf2_per_objfile->read_in_chain = this_cu;
- }
-      else
- do_cleanups (free_cu_cleanup);
+      /* Link this CU into read_in_chain.  */
+      this_cu->cu->read_in_chain = dwarf2_per_objfile->read_in_chain;
+      dwarf2_per_objfile->read_in_chain = this_cu;
+      /* The chain owns it now.  */
+      new_cu.release ();
     }
-
-  do_cleanups (cleanups);
 }
 
 /* Read CU/TU THIS_CU but do not follow DW_AT_GNU_dwo_name if present.
@@ -8051,10 +8012,8 @@ init_cutu_and_read_dies_no_follow (struct dwarf2_per_cu_data *this_cu,
   struct dwarf2_section_info *section = this_cu->section;
   bfd *abfd = get_section_bfd_owner (section);
   struct dwarf2_section_info *abbrev_section;
-  struct dwarf2_cu cu;
   const gdb_byte *begin_info_ptr, *info_ptr;
   struct die_reader_specs reader;
-  struct cleanup *cleanups;
   struct die_info *comp_unit_die;
   int has_children;
 
@@ -8072,9 +8031,7 @@ init_cutu_and_read_dies_no_follow (struct dwarf2_per_cu_data *this_cu,
   /* This is cheap if the section is already read in.  */
   dwarf2_read_section (objfile, section);
 
-  init_one_comp_unit (&cu, this_cu);
-
-  cleanups = make_cleanup (free_stack_comp_unit, &cu);
+  struct dwarf2_cu cu (this_cu);
 
   begin_info_ptr = info_ptr = section->buffer + to_underlying (this_cu->sect_off);
   info_ptr = read_and_check_comp_unit_head (dwarf2_per_objfile,
@@ -8090,7 +8047,6 @@ init_cutu_and_read_dies_no_follow (struct dwarf2_per_cu_data *this_cu,
   if (info_ptr >= begin_info_ptr + this_cu->length
       || peek_abbrev_code (abfd, info_ptr) == 0)
     {
-      do_cleanups (cleanups);
       return;
     }
 
@@ -8102,8 +8058,6 @@ init_cutu_and_read_dies_no_follow (struct dwarf2_per_cu_data *this_cu,
   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);
-
-  do_cleanups (cleanups);
 }
 
 /* Read a CU/TU, except that this does not look for DW_AT_GNU_dwo_name and
@@ -25119,13 +25073,24 @@ dwarf2_find_containing_comp_unit (sect_offset sect_off,
 
 /* Initialize dwarf2_cu CU, owned by PER_CU.  */
 
-static void
-init_one_comp_unit (struct dwarf2_cu *cu, struct dwarf2_per_cu_data *per_cu)
+dwarf2_cu::dwarf2_cu (struct dwarf2_per_cu_data *per_cu_)
+  : per_cu (per_cu_),
+    mark (0),
+    has_loclist (0),
+    checked_producer (0),
+    producer_is_gxx_lt_4_6 (0),
+    producer_is_gcc_lt_4_3 (0),
+    producer_is_icc_lt_14 (0),
+    processing_has_namespace_info (0)
 {
-  memset (cu, 0, sizeof (*cu));
-  per_cu->cu = cu;
-  cu->per_cu = per_cu;
-  obstack_init (&cu->comp_unit_obstack);
+  per_cu->cu = this;
+}
+
+/* Destroy a dwarf2_cu.  */
+
+dwarf2_cu::~dwarf2_cu ()
+{
+  per_cu->cu = NULL;
 }
 
 /* Initialize basic fields of dwarf_cu CU according to DIE COMP_UNIT_DIE.  */
@@ -25149,43 +25114,6 @@ prepare_one_comp_unit (struct dwarf2_cu *cu, struct die_info *comp_unit_die,
   cu->producer = dwarf2_string_attr (comp_unit_die, DW_AT_producer, cu);
 }
 
-/* Release one cached compilation unit, CU.  We unlink it from the tree
-   of compilation units, but we don't remove it from the read_in_chain;
-   the caller is responsible for that.
-   NOTE: DATA is a void * because this function is also used as a
-   cleanup routine.  */
-
-static void
-free_heap_comp_unit (void *data)
-{
-  struct dwarf2_cu *cu = (struct dwarf2_cu *) data;
-
-  gdb_assert (cu->per_cu != NULL);
-  cu->per_cu->cu = NULL;
-  cu->per_cu = NULL;
-
-  obstack_free (&cu->comp_unit_obstack, NULL);
-
-  xfree (cu);
-}
-
-/* This cleanup function is passed the address of a dwarf2_cu on the stack
-   when we're finished with it.  We can't free the pointer itself, but be
-   sure to unlink it from the cache.  Also release any associated storage.  */
-
-static void
-free_stack_comp_unit (void *data)
-{
-  struct dwarf2_cu *cu = (struct dwarf2_cu *) data;
-
-  gdb_assert (cu->per_cu != NULL);
-  cu->per_cu->cu = NULL;
-  cu->per_cu = NULL;
-
-  obstack_free (&cu->comp_unit_obstack, NULL);
-  cu->partial_dies = NULL;
-}
-
 /* Free all cached compilation units.  */
 
 static void
@@ -25225,7 +25153,7 @@ age_cached_comp_units (struct dwarf2_per_objfile *dwarf2_per_objfile)
 
       if (!per_cu->cu->mark)
  {
-  free_heap_comp_unit (per_cu->cu);
+  delete per_cu->cu;
   *last_chain = next_cu;
  }
       else
@@ -25254,7 +25182,7 @@ free_one_cached_comp_unit (struct dwarf2_per_cu_data *target_per_cu)
 
       if (per_cu == target_per_cu)
  {
-  free_heap_comp_unit (per_cu->cu);
+  delete per_cu->cu;
   per_cu->cu = NULL;
   *last_chain = next_cu;
   break;
Reply | Threaded
Open this post in threaded view
|

Re: [RFA 5/6] Remove objfile argument from add_dyn_prop

Tom Tromey-2
In reply to this post by Yao Qi
>>>>> "Yao" == Yao Qi <[hidden email]> writes:

Yao> Tom Tromey <[hidden email]> writes:
>> @@ -25116,7 +25116,7 @@ set_die_type (struct die_info *die, struct type *type, struct dwarf2_cu *cu)
>> if (attr_form_is_block (attr))
>> {
>> if (attr_to_dynamic_prop (attr, die, cu, &prop))
>> -        add_dyn_prop (DYN_PROP_ALLOCATED, prop, type, objfile);
>> +        add_dyn_prop (DYN_PROP_ALLOCATED, prop, type);
>> }

Yao> Before the patch, the objfile is
Yao> cu-> per_cu->dwarf2_per_objfile->objfile, but after the patch, the
Yao> objfile is TYPE_OBJFILE (type), are they equivalent?

If they were not the same, then that was already a bug, because
types generally have the restriction that they can only point to static
data or to other objects allocated on the same objfile.  Cross-objfile
pointers are not allowed, to avoid dangling pointers when an objfile is
destroyed.

Now, a given invocation of the DWARF reader generally deals with a
single objfile.  So I believe they were the same.  But if they were not,
then this patch actually improves the situation.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [RFA 3/6] Allocate dwarf2_cu with new

Simon Marchi-2
In reply to this post by Tom Tromey-2
On 2018-01-10 11:28 AM, Tom Tromey wrote:

>>>>>> "Tom" == Tom Tromey <[hidden email]> writes:
>
> Tom> This is gone with Simon's changes; I'll update the ChangeLog and send
> Tom> the updated patch.
>
> Here it is.  This went through the buildbot the second time around.
>
> Tom
>
> commit bcaf61d8ff6a149785924223151df8ded30e8d7a
> Author: Simon Marchi <[hidden email]>
> Date:   Sun Jan 7 11:41:09 2018 -0500
>
>     Allocate dwarf2_cu with new
>    
>     This changes dwarf2_cu to be allocated with new, and fixes up the
>     users.
>    
>     2018-01-05  Tom Tromey  <[hidden email]>
>                 Simon Marchi  <[hidden email]>
>    
>             * dwarf2read.c (struct dwarf2_cu): Add constructor, destructor.
>             (dwarf2_per_objfile::free_cached_comp_units)
>             (init_tu_and_read_dwo_dies, init_cutu_and_read_dies)
>             (init_cutu_and_read_dies_no_follow): Update.
>             (dwarf2_cu::dwarf2_cu): Rename from init_one_comp_unit.
>             (dwarf2_cu::~dwarf2_cu): New.
>             (free_heap_comp_unit, free_stack_comp_unit): Remove.
>             (age_cached_comp_units, free_one_cached_comp_unit): Update.

Hi Tom,

This LGTM, I just noted two formatting nits.

> @@ -7932,7 +7904,6 @@ init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
>    if (info_ptr >= begin_info_ptr + this_cu->length
>        || peek_abbrev_code (abfd, info_ptr) == 0)
>      {
> -      do_cleanups (cleanups);
>        return;
>      }

You can remove the curly braces here.

> @@ -8090,7 +8047,6 @@ init_cutu_and_read_dies_no_follow (struct dwarf2_per_cu_data *this_cu,
>    if (info_ptr >= begin_info_ptr + this_cu->length
>        || peek_abbrev_code (abfd, info_ptr) == 0)
>      {
> -      do_cleanups (cleanups);
>        return;
>      }

Here too.

Simon
12