[PATCH 0/3] Move dwarf2_cu to its own file

[PATCH 0/3] Move dwarf2_cu to its own file

[Tom Tromey]

This short series moves dwarf2_cu to a new header, and then moves some
of the methods and helper functions to a new .c file as well.

This is just a minor cleanup coming from another series I'm working
on.  I think that in general we should be splitting up dwarf2/read.c.
It is still the largest source file in gdb by a large margin.

Regression tested on x86-64 Fedora 32.

Tom

[PATCH 1/3] Move dwarf2_cu to new header file

[Tom Tromey]

This moves dwarf2_cu and one supporting data structure to a new header
file.  The main goal, as always with this kind of change, is to make
the DWARF reader a bit more understandable.

gdb/ChangeLog
2021-03-27  Tom Tromey  <tom@tromey.com>

	* Makefile.in (HFILES_NO_SRCDIR): Add dwarf2/cu.h.
	* dwarf2/read.c (struct delayed_method_info, struct dwarf2_cu):
	Move to cu.h.
	* dwarf2/cu.h: New file.
---
 gdb/ChangeLog     |   7 ++
 gdb/Makefile.in   |   1 +
 gdb/dwarf2/cu.h   | 270 ++++++++++++++++++++++++++++++++++++++++++++++
 gdb/dwarf2/read.c | 245 +----------------------------------------
 4 files changed, 279 insertions(+), 244 deletions(-)
 create mode 100644 gdb/dwarf2/cu.h

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 3318c1a5215..359fd11a4de 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1273,6 +1273,7 @@ HFILES_NO_SRCDIR = \
 	dictionary.h \
 	disasm.h \
 	dummy-frame.h \
+	dwarf2/cu.h \
 	dwarf2/frame-tailcall.h \
 	dwarf2/frame.h \
 	dwarf2/expr.h \
diff --git a/gdb/dwarf2/cu.h b/gdb/dwarf2/cu.h
new file mode 100644
index 00000000000..9fb2d61163a
--- /dev/null
+++ b/gdb/dwarf2/cu.h
@@ -0,0 +1,270 @@
+/* DWARF CU data structure
+
+   Copyright (C) 2021 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef GDB_DWARF2_CU_H
+#define GDB_DWARF2_CU_H
+
+#include "buildsym.h"
+#include "dwarf2/comp-unit.h"
+#include "gdbsupport/gdb_optional.h"
+
+/* Type used for delaying computation of method physnames.
+   See comments for compute_delayed_physnames.  */
+struct delayed_method_info
+{
+  /* The type to which the method is attached, i.e., its parent class.  */
+  struct type *type;
+
+  /* The index of the method in the type's function fieldlists.  */
+  int fnfield_index;
+
+  /* The index of the method in the fieldlist.  */
+  int index;
+
+  /* The name of the DIE.  */
+  const char *name;
+
+  /*  The DIE associated with this method.  */
+  struct die_info *die;
+};
+
+/* Internal state when decoding a particular compilation unit.  */
+struct dwarf2_cu
+{
+  explicit dwarf2_cu (dwarf2_per_cu_data *per_cu,
+		      dwarf2_per_objfile *per_objfile);
+
+  DISABLE_COPY_AND_ASSIGN (dwarf2_cu);
+
+  /* TU version of handle_DW_AT_stmt_list for read_type_unit_scope.
+     Create the set of symtabs used by this TU, or if this TU is sharing
+     symtabs with another TU and the symtabs have already been created
+     then restore those symtabs in the line header.
+     We don't need the pc/line-number mapping for type units.  */
+  void setup_type_unit_groups (struct die_info *die);
+
+  /* Start a symtab for DWARF.  NAME, COMP_DIR, LOW_PC are passed to the
+     buildsym_compunit constructor.  */
+  struct compunit_symtab *start_symtab (const char *name,
+					const char *comp_dir,
+					CORE_ADDR low_pc);
+
+  /* Reset the builder.  */
+  void reset_builder () { m_builder.reset (); }
+
+  /* Return a type that is a generic pointer type, the size of which
+     matches the address size given in the compilation unit header for
+     this CU.  */
+  struct type *addr_type () const;
+
+  /* Find an integer type the same size as the address size given in
+     the compilation unit header for this CU.  UNSIGNED_P controls if
+     the integer is unsigned or not.  */
+  struct type *addr_sized_int_type (bool unsigned_p) const;
+
+  /* The header of the compilation unit.  */
+  struct comp_unit_head header {};
+
+  /* Base address of this compilation unit.  */
+  gdb::optional<CORE_ADDR> base_address;
+
+  /* The language we are debugging.  */
+  enum language language = language_unknown;
+  const struct language_defn *language_defn = nullptr;
+
+  const char *producer = nullptr;
+
+private:
+  /* The symtab builder for this CU.  This is only non-NULL when full
+     symbols are being read.  */
+  std::unique_ptr<buildsym_compunit> m_builder;
+
+public:
+  /* The generic symbol table building routines have separate lists for
+     file scope symbols and all all other scopes (local scopes).  So
+     we need to select the right one to pass to add_symbol_to_list().
+     We do it by keeping a pointer to the correct list in list_in_scope.
+
+     FIXME: The original dwarf code just treated the file scope as the
+     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 = nullptr;
+
+  /* Hash table holding all the loaded partial DIEs
+     with partial_die->offset.SECT_OFF as hash.  */
+  htab_t partial_dies = nullptr;
+
+  /* Storage for things with the same lifetime as this read-in compilation
+     unit, including partial DIEs.  */
+  auto_obstack comp_unit_obstack;
+
+  /* Backlink to our per_cu entry.  */
+  struct dwarf2_per_cu_data *per_cu;
+
+  /* The dwarf2_per_objfile that owns this.  */
+  dwarf2_per_objfile *per_objfile;
+
+  /* How many compilation units ago was this CU last referenced?  */
+  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 = nullptr;
+
+  /* Full DIEs if read in.  */
+  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 = nullptr;
+
+  /* Header data from the line table, during full symbol processing.  */
+  struct line_header *line_header = nullptr;
+  /* Non-NULL if LINE_HEADER is owned by this DWARF_CU.  Otherwise,
+     it's owned by dwarf2_per_bfd::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 = nullptr;
+
+  /* A list of methods which need to have physnames computed
+     after all type information has been read.  */
+  std::vector<delayed_method_info> method_list;
+
+  /* To be copied to symtab->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:
+     Except for attributes copied from the top level DIE in the "main"
+     (or "stub") file in preparation for reading the DWO file
+     (e.g., DW_AT_addr_base), we KISS: there is only *one* CU.
+     Either there isn't a DWO file (in which case this is NULL and the point
+     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 = nullptr;
+
+  /* The DW_AT_addr_base (DW_AT_GNU_addr_base) attribute if present.
+     Note this value comes from the Fission stub CU/TU's DIE.  */
+  gdb::optional<ULONGEST> addr_base;
+
+  /* The DW_AT_GNU_ranges_base attribute, if present.
+
+     This is only relevant in the context of pre-DWARF 5 split units.  In this
+     context, there is a .debug_ranges section in the linked executable,
+     containing all the ranges data for all the compilation units.  Each
+     skeleton/stub unit has (if needed) a DW_AT_GNU_ranges_base attribute that
+     indicates the base of its contribution to that section.  The DW_AT_ranges
+     attributes in the split-unit are of the form DW_FORM_sec_offset and point
+     into the .debug_ranges section of the linked file.  However, they are not
+     "true" DW_FORM_sec_offset, because they are relative to the base of their
+     compilation unit's contribution, rather than relative to the beginning of
+     the section.  The DW_AT_GNU_ranges_base value must be added to it to make
+     it relative to the beginning of the section.
+
+     Note that the value is zero when we are not in a pre-DWARF 5 split-unit
+     case, so this value can be added without needing to know whether we are in
+     this case or not.
+
+     N.B. If a DW_AT_ranges attribute is found on the DW_TAG_compile_unit in the
+     skeleton/stub, it must not have the base added, as it already points to the
+     right place.  And since the DW_TAG_compile_unit DIE in the split-unit can't
+     have a DW_AT_ranges attribute, we can use the
+
+       die->tag != DW_AT_compile_unit
+
+     to determine whether the base should be added or not.  */
+  ULONGEST gnu_ranges_base = 0;
+
+  /* The DW_AT_rnglists_base attribute, if present.
+
+     This is used when processing attributes of form DW_FORM_rnglistx in
+     non-split units.  Attributes of this form found in a split unit don't
+     use it, as split-unit files have their own non-shared .debug_rnglists.dwo
+     section.  */
+  ULONGEST rnglists_base = 0;
+
+  /* The DW_AT_loclists_base attribute if present.  */
+  ULONGEST loclist_base = 0;
+
+  /* When reading debug info generated by older versions of rustc, we
+     have to rewrite some union types to be struct types with a
+     variant part.  This rewriting must be done after the CU is fully
+     read in, because otherwise at the point of rewriting some struct
+     type might not have been fully processed.  So, we keep a list of
+     all such types here and process them after expansion.  */
+  std::vector<struct type *> rust_unions;
+
+  /* The DW_AT_str_offsets_base attribute if present.  For DWARF 4 version DWO
+     files, the value is implicitly zero.  For DWARF 5 version DWO files, the
+     value is often implicit and is the size of the header of
+     .debug_str_offsets section (8 or 4, depending on the address size).  */
+  gdb::optional<ULONGEST> str_offsets_base;
+
+  /* Mark used when releasing cached dies.  */
+  bool mark : 1;
+
+  /* This CU references .debug_loc.  See the symtab->locations_valid field.
+     This test is imperfect as there may exist optimized debug code not using
+     any location list and still facing inlining issues if handled as
+     unoptimized code.  For a future better test see GCC PR other/32998.  */
+  bool has_loclist : 1;
+
+  /* These cache the results for producer_is_* fields.  CHECKED_PRODUCER is true
+     if all the producer_is_* fields are valid.  This information is cached
+     because profiling CU expansion showed excessive time spent in
+     producer_is_gxx_lt_4_6.  */
+  bool checked_producer : 1;
+  bool producer_is_gxx_lt_4_6 : 1;
+  bool producer_is_gcc_lt_4_3 : 1;
+  bool producer_is_icc : 1;
+  bool producer_is_icc_lt_14 : 1;
+  bool producer_is_codewarrior : 1;
+
+  /* When true, the file that we're processing is known to have
+     debugging info for C++ namespaces.  GCC 3.3.x did not produce
+     this information, but later versions do.  */
+
+  bool processing_has_namespace_info : 1;
+
+  struct partial_die_info *find_partial_die (sect_offset sect_off);
+
+  /* If this CU was inherited by another CU (via specification,
+     abstract_origin, etc), this is the ancestor CU.  */
+  dwarf2_cu *ancestor;
+
+  /* Get the buildsym_compunit for this CU.  */
+  buildsym_compunit *get_builder ()
+  {
+    /* If this CU has a builder associated with it, use that.  */
+    if (m_builder != nullptr)
+      return m_builder.get ();
+
+    /* Otherwise, search ancestors for a valid builder.  */
+    if (ancestor != nullptr)
+      return ancestor->get_builder ();
+
+    return nullptr;
+  }
+};
+
+#endif /* GDB_DWARF2_CU_H */
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index b324541ee9f..b11fd0580f7 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -33,6 +33,7 @@
 #include "dwarf2/abbrev.h"
 #include "dwarf2/attribute.h"
 #include "dwarf2/comp-unit.h"
+#include "dwarf2/cu.h"
 #include "dwarf2/index-cache.h"
 #include "dwarf2/index-common.h"
 #include "dwarf2/leb.h"
@@ -49,7 +50,6 @@
 #include "gdbtypes.h"
 #include "objfiles.h"
 #include "dwarf2.h"
-#include "buildsym.h"
 #include "demangle.h"
 #include "gdb-demangle.h"
 #include "filenames.h"	/* for DOSish file names */
@@ -409,249 +409,6 @@ struct loclists_rnglists_header
   unsigned int offset_entry_count;
 };
 
-/* Type used for delaying computation of method physnames.
-   See comments for compute_delayed_physnames.  */
-struct delayed_method_info
-{
-  /* The type to which the method is attached, i.e., its parent class.  */
-  struct type *type;
-
-  /* The index of the method in the type's function fieldlists.  */
-  int fnfield_index;
-
-  /* The index of the method in the fieldlist.  */
-  int index;
-
-  /* The name of the DIE.  */
-  const char *name;
-
-  /*  The DIE associated with this method.  */
-  struct die_info *die;
-};
-
-/* Internal state when decoding a particular compilation unit.  */
-struct dwarf2_cu
-{
-  explicit dwarf2_cu (dwarf2_per_cu_data *per_cu,
-		      dwarf2_per_objfile *per_objfile);
-
-  DISABLE_COPY_AND_ASSIGN (dwarf2_cu);
-
-  /* TU version of handle_DW_AT_stmt_list for read_type_unit_scope.
-     Create the set of symtabs used by this TU, or if this TU is sharing
-     symtabs with another TU and the symtabs have already been created
-     then restore those symtabs in the line header.
-     We don't need the pc/line-number mapping for type units.  */
-  void setup_type_unit_groups (struct die_info *die);
-
-  /* Start a symtab for DWARF.  NAME, COMP_DIR, LOW_PC are passed to the
-     buildsym_compunit constructor.  */
-  struct compunit_symtab *start_symtab (const char *name,
-					const char *comp_dir,
-					CORE_ADDR low_pc);
-
-  /* Reset the builder.  */
-  void reset_builder () { m_builder.reset (); }
-
-  /* Return a type that is a generic pointer type, the size of which
-     matches the address size given in the compilation unit header for
-     this CU.  */
-  struct type *addr_type () const;
-
-  /* Find an integer type the same size as the address size given in
-     the compilation unit header for this CU.  UNSIGNED_P controls if
-     the integer is unsigned or not.  */
-  struct type *addr_sized_int_type (bool unsigned_p) const;
-
-  /* The header of the compilation unit.  */
-  struct comp_unit_head header {};
-
-  /* Base address of this compilation unit.  */
-  gdb::optional<CORE_ADDR> base_address;
-
-  /* The language we are debugging.  */
-  enum language language = language_unknown;
-  const struct language_defn *language_defn = nullptr;
-
-  const char *producer = nullptr;
-
-private:
-  /* The symtab builder for this CU.  This is only non-NULL when full
-     symbols are being read.  */
-  std::unique_ptr<buildsym_compunit> m_builder;
-
-public:
-  /* The generic symbol table building routines have separate lists for
-     file scope symbols and all all other scopes (local scopes).  So
-     we need to select the right one to pass to add_symbol_to_list().
-     We do it by keeping a pointer to the correct list in list_in_scope.
-
-     FIXME: The original dwarf code just treated the file scope as the
-     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 = nullptr;
-
-  /* Hash table holding all the loaded partial DIEs
-     with partial_die->offset.SECT_OFF as hash.  */
-  htab_t partial_dies = nullptr;
-
-  /* Storage for things with the same lifetime as this read-in compilation
-     unit, including partial DIEs.  */
-  auto_obstack comp_unit_obstack;
-
-  /* Backlink to our per_cu entry.  */
-  struct dwarf2_per_cu_data *per_cu;
-
-  /* The dwarf2_per_objfile that owns this.  */
-  dwarf2_per_objfile *per_objfile;
-
-  /* How many compilation units ago was this CU last referenced?  */
-  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 = nullptr;
-
-  /* Full DIEs if read in.  */
-  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 = nullptr;
-
-  /* Header data from the line table, during full symbol processing.  */
-  struct line_header *line_header = nullptr;
-  /* Non-NULL if LINE_HEADER is owned by this DWARF_CU.  Otherwise,
-     it's owned by dwarf2_per_bfd::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 = nullptr;
-
-  /* A list of methods which need to have physnames computed
-     after all type information has been read.  */
-  std::vector<delayed_method_info> method_list;
-
-  /* To be copied to symtab->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:
-     Except for attributes copied from the top level DIE in the "main"
-     (or "stub") file in preparation for reading the DWO file
-     (e.g., DW_AT_addr_base), we KISS: there is only *one* CU.
-     Either there isn't a DWO file (in which case this is NULL and the point
-     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 = nullptr;
-
-  /* The DW_AT_addr_base (DW_AT_GNU_addr_base) attribute if present.
-     Note this value comes from the Fission stub CU/TU's DIE.  */
-  gdb::optional<ULONGEST> addr_base;
-
-  /* The DW_AT_GNU_ranges_base attribute, if present.
-
-     This is only relevant in the context of pre-DWARF 5 split units.  In this
-     context, there is a .debug_ranges section in the linked executable,
-     containing all the ranges data for all the compilation units.  Each
-     skeleton/stub unit has (if needed) a DW_AT_GNU_ranges_base attribute that
-     indicates the base of its contribution to that section.  The DW_AT_ranges
-     attributes in the split-unit are of the form DW_FORM_sec_offset and point
-     into the .debug_ranges section of the linked file.  However, they are not
-     "true" DW_FORM_sec_offset, because they are relative to the base of their
-     compilation unit's contribution, rather than relative to the beginning of
-     the section.  The DW_AT_GNU_ranges_base value must be added to it to make
-     it relative to the beginning of the section.
-
-     Note that the value is zero when we are not in a pre-DWARF 5 split-unit
-     case, so this value can be added without needing to know whether we are in
-     this case or not.
-
-     N.B. If a DW_AT_ranges attribute is found on the DW_TAG_compile_unit in the
-     skeleton/stub, it must not have the base added, as it already points to the
-     right place.  And since the DW_TAG_compile_unit DIE in the split-unit can't
-     have a DW_AT_ranges attribute, we can use the
-
-       die->tag != DW_AT_compile_unit
-
-     to determine whether the base should be added or not.  */
-  ULONGEST gnu_ranges_base = 0;
-
-  /* The DW_AT_rnglists_base attribute, if present.
-
-     This is used when processing attributes of form DW_FORM_rnglistx in
-     non-split units.  Attributes of this form found in a split unit don't
-     use it, as split-unit files have their own non-shared .debug_rnglists.dwo
-     section.  */
-  ULONGEST rnglists_base = 0;
-
-  /* The DW_AT_loclists_base attribute if present.  */
-  ULONGEST loclist_base = 0;
-
-  /* When reading debug info generated by older versions of rustc, we
-     have to rewrite some union types to be struct types with a
-     variant part.  This rewriting must be done after the CU is fully
-     read in, because otherwise at the point of rewriting some struct
-     type might not have been fully processed.  So, we keep a list of
-     all such types here and process them after expansion.  */
-  std::vector<struct type *> rust_unions;
-
-  /* The DW_AT_str_offsets_base attribute if present.  For DWARF 4 version DWO
-     files, the value is implicitly zero.  For DWARF 5 version DWO files, the
-     value is often implicit and is the size of the header of
-     .debug_str_offsets section (8 or 4, depending on the address size).  */
-  gdb::optional<ULONGEST> str_offsets_base;
-
-  /* Mark used when releasing cached dies.  */
-  bool mark : 1;
-
-  /* This CU references .debug_loc.  See the symtab->locations_valid field.
-     This test is imperfect as there may exist optimized debug code not using
-     any location list and still facing inlining issues if handled as
-     unoptimized code.  For a future better test see GCC PR other/32998.  */
-  bool has_loclist : 1;
-
-  /* These cache the results for producer_is_* fields.  CHECKED_PRODUCER is true
-     if all the producer_is_* fields are valid.  This information is cached
-     because profiling CU expansion showed excessive time spent in
-     producer_is_gxx_lt_4_6.  */
-  bool checked_producer : 1;
-  bool producer_is_gxx_lt_4_6 : 1;
-  bool producer_is_gcc_lt_4_3 : 1;
-  bool producer_is_icc : 1;
-  bool producer_is_icc_lt_14 : 1;
-  bool producer_is_codewarrior : 1;
-
-  /* When true, the file that we're processing is known to have
-     debugging info for C++ namespaces.  GCC 3.3.x did not produce
-     this information, but later versions do.  */
-
-  bool processing_has_namespace_info : 1;
-
-  struct partial_die_info *find_partial_die (sect_offset sect_off);
-
-  /* If this CU was inherited by another CU (via specification,
-     abstract_origin, etc), this is the ancestor CU.  */
-  dwarf2_cu *ancestor;
-
-  /* Get the buildsym_compunit for this CU.  */
-  buildsym_compunit *get_builder ()
-  {
-    /* If this CU has a builder associated with it, use that.  */
-    if (m_builder != nullptr)
-      return m_builder.get ();
-
-    /* Otherwise, search ancestors for a valid builder.  */
-    if (ancestor != nullptr)
-      return ancestor->get_builder ();
-
-    return nullptr;
-  }
-};
-
 /* A struct that can be used as a hash key for tables based on DW_AT_stmt_list.
    This includes type_unit_group and quick_file_names.  */
 
-- 
2.26.2

Re: [PATCH 1/3] Move dwarf2_cu to new header file

[Simon Marchi]

On 2021-03-27 3:56 p.m., Tom Tromey wrote:
> This moves dwarf2_cu and one supporting data structure to a new header
> file.  The main goal, as always with this kind of change, is to make
> the DWARF reader a bit more understandable.

The only thing that is a bit unclear is that we now have:

  - dwarf2/cu.h
  - dwarf2/comp-unit.h

where "cu" and "comp-unit" mean the same thing.  So it's not clear what
is meant for what.

Simon

Re: [PATCH 1/3] Move dwarf2_cu to new header file

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> On 2021-03-27 3:56 p.m., Tom Tromey wrote:
>> This moves dwarf2_cu and one supporting data structure to a new header
>> file.  The main goal, as always with this kind of change, is to make
>> the DWARF reader a bit more understandable.

Simon> The only thing that is a bit unclear is that we now have:

Simon>   - dwarf2/cu.h
Simon>   - dwarf2/comp-unit.h

Simon> where "cu" and "comp-unit" mean the same thing.  So it's not clear what
Simon> is meant for what.

Thanks.  I did not even consider this.
I don't really know what to do about it, though.  The naming problem is
already in the code.

Tom

Re: [PATCH 1/3] Move dwarf2_cu to new header file

[Simon Marchi]

On 2021-03-28 1:43 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> On 2021-03-27 3:56 p.m., Tom Tromey wrote:
>>> This moves dwarf2_cu and one supporting data structure to a new header
>>> file.  The main goal, as always with this kind of change, is to make
>>> the DWARF reader a bit more understandable.
> 
> Simon> The only thing that is a bit unclear is that we now have:
> 
> Simon>   - dwarf2/cu.h
> Simon>   - dwarf2/comp-unit.h
> 
> Simon> where "cu" and "comp-unit" mean the same thing.  So it's not clear what
> Simon> is meant for what.
> 
> Thanks.  I did not even consider this.
> I don't really know what to do about it, though.  The naming problem is
> already in the code.

comp-unit.h is only about the header (struct comp_unit_head), so it could
be renamed comp-unit-head.h.

Simon

Re: [PATCH 1/3] Move dwarf2_cu to new header file

[Tom Tromey]

Simon> comp-unit.h is only about the header (struct comp_unit_head), so it could
Simon> be renamed comp-unit-head.h.

At first I balked because there's also a .c, but I guess I can rename
them both.

Tom

Re: [PATCH 1/3] Move dwarf2_cu to new header file

[Tom Tromey]

Simon> comp-unit.h is only about the header (struct comp_unit_head), so it could
Simon> be renamed comp-unit-head.h.

Tom> At first I balked because there's also a .c, but I guess I can rename
Tom> them both.

I finally did it.  Let me know what you think.

Tom

commit 7a105aab4dae6d4b49f3d41928ac65129278ae04
Author: Tom Tromey <tom@tromey.com>
Date:   Sun May 16 06:35:29 2021 -0600

    Rename dwarf2/comp-unit.h
    
    Simon pointed out that dwarf2/cu.h and dwarf2/comp-unit.h seemingly
    mean the same thing.  He suggested renaming the latter to
    comp-unit-head.h, which is what this patch does.
    
    gdb/ChangeLog
    2021-05-16  Tom Tromey  <tom@tromey.com>
    
            * dwarf2/read.h: Update include.
            * dwarf2/read.c: Update include.
            * dwarf2/line-header.c: Update include.
            * dwarf2/cu.h: Update include.
            * dwarf2/comp-unit-head.h: Rename from comp-unit.h.
            * dwarf2/comp-unit-head.c: Rename from comp-unit.c.
            * Makefile.in (COMMON_SFILES): Update.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 82b7e130e9e..dbb9dde4d04 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,13 @@
+2021-05-16  Tom Tromey  <tom@tromey.com>
+
+	* dwarf2/read.h: Update include.
+	* dwarf2/read.c: Update include.
+	* dwarf2/line-header.c: Update include.
+	* dwarf2/cu.h: Update include.
+	* dwarf2/comp-unit-head.h: Rename from comp-unit.h.
+	* dwarf2/comp-unit-head.c: Rename from comp-unit.c.
+	* Makefile.in (COMMON_SFILES): Update.
+
 2021-03-28  Tom Tromey  <tom@tromey.com>
 
 	* dwarf2/read.c (maybe_queue_comp_unit)
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 1f37fe43024..a9fade803b0 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1025,7 +1025,7 @@ COMMON_SFILES = \
 	dummy-frame.c \
 	dwarf2/abbrev.c \
 	dwarf2/attribute.c \
-	dwarf2/comp-unit.c \
+	dwarf2/comp-unit-head.c \
 	dwarf2/cu.c \
 	dwarf2/dwz.c \
 	dwarf2/expr.c \
diff --git a/gdb/dwarf2/comp-unit.c b/gdb/dwarf2/comp-unit-head.c
similarity index 98%
rename from gdb/dwarf2/comp-unit.c
rename to gdb/dwarf2/comp-unit-head.c
index e22f2e9a91c..8cc741df595 100644
--- a/gdb/dwarf2/comp-unit.c
+++ b/gdb/dwarf2/comp-unit-head.c
@@ -25,13 +25,13 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
-#include "dwarf2/comp-unit.h"
+#include "dwarf2/comp-unit-head.h"
 #include "dwarf2/leb.h"
 #include "dwarf2/read.h"
 #include "dwarf2/section.h"
 #include "dwarf2/stringify.h"
 
-/* See comp-unit.h.  */
+/* See comp-unit-head.h.  */
 
 const gdb_byte *
 read_comp_unit_head (struct comp_unit_head *cu_header,
@@ -172,7 +172,7 @@ error_check_comp_unit_head (dwarf2_per_objfile *per_objfile,
 	   filename);
 }
 
-/* See comp-unit.h.  */
+/* See comp-unit-head.h.  */
 
 const gdb_byte *
 read_and_check_comp_unit_head (dwarf2_per_objfile *per_objfile,
diff --git a/gdb/dwarf2/comp-unit.h b/gdb/dwarf2/comp-unit-head.h
similarity index 100%
rename from gdb/dwarf2/comp-unit.h
rename to gdb/dwarf2/comp-unit-head.h
diff --git a/gdb/dwarf2/cu.h b/gdb/dwarf2/cu.h
index 83a4aac3f08..ff56ec5527b 100644
--- a/gdb/dwarf2/cu.h
+++ b/gdb/dwarf2/cu.h
@@ -21,7 +21,7 @@
 #define GDB_DWARF2_CU_H
 
 #include "buildsym.h"
-#include "dwarf2/comp-unit.h"
+#include "dwarf2/comp-unit-head.h"
 #include "gdbsupport/gdb_optional.h"
 
 /* Type used for delaying computation of method physnames.
diff --git a/gdb/dwarf2/line-header.c b/gdb/dwarf2/line-header.c
index 7575297f966..07a8f5aa109 100644
--- a/gdb/dwarf2/line-header.c
+++ b/gdb/dwarf2/line-header.c
@@ -18,7 +18,7 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
-#include "dwarf2/comp-unit.h"
+#include "dwarf2/comp-unit-head.h"
 #include "dwarf2/leb.h"
 #include "dwarf2/line-header.h"
 #include "dwarf2/read.h"
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 841cf6c471f..956b8377bb9 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -32,7 +32,7 @@
 #include "dwarf2/read.h"
 #include "dwarf2/abbrev.h"
 #include "dwarf2/attribute.h"
-#include "dwarf2/comp-unit.h"
+#include "dwarf2/comp-unit-head.h"
 #include "dwarf2/cu.h"
 #include "dwarf2/index-cache.h"
 #include "dwarf2/index-common.h"
diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index 80f0e3ee13b..436a7b4c91b 100644
--- a/gdb/dwarf2/read.h
+++ b/gdb/dwarf2/read.h
@@ -22,7 +22,7 @@
 
 #include <queue>
 #include <unordered_map>
-#include "dwarf2/comp-unit.h"
+#include "dwarf2/comp-unit-head.h"
 #include "dwarf2/index-cache.h"
 #include "dwarf2/section.h"
 #include "filename-seen-cache.h"

Re: [PATCH 1/3] Move dwarf2_cu to new header file

[Simon Marchi]

On 2021-05-16 8:43 a.m., Tom Tromey wrote:
> Simon> comp-unit.h is only about the header (struct comp_unit_head), so it could
> Simon> be renamed comp-unit-head.h.
> 
> Tom> At first I balked because there's also a .c, but I guess I can rename
> Tom> them both.
> 
> I finally did it.  Let me know what you think.
> 
> Tom

Cool, thanks :).  LGTM.

Simon

[PATCH 2/3] Move some dwarf2_cu methods to new file

[Tom Tromey]

This moves some of the dwarf2_cu methods to a new file, dwarf2/cu.c.

gdb/ChangeLog
2021-03-27  Tom Tromey  <tom@tromey.com>

	* dwarf2/read.c (dwarf2_cu::addr_sized_int_type)
	(dwarf2_cu::start_symtab, dwarf2_cu::addr_type)
	(dwarf2_cu::dwarf2_cu): Move to cu.c.
	* dwarf2/cu.c: New file.
	* Makefile.in (COMMON_SFILES): Add dwarf2/cu.c.
---
 gdb/ChangeLog     |  8 +++++
 gdb/Makefile.in   |  1 +
 gdb/dwarf2/cu.c   | 89 +++++++++++++++++++++++++++++++++++++++++++++++
 gdb/dwarf2/read.c | 67 -----------------------------------
 4 files changed, 98 insertions(+), 67 deletions(-)
 create mode 100644 gdb/dwarf2/cu.c

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 359fd11a4de..6c1165eaf92 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1027,6 +1027,7 @@ COMMON_SFILES = \
 	dwarf2/abbrev.c \
 	dwarf2/attribute.c \
 	dwarf2/comp-unit.c \
+	dwarf2/cu.c \
 	dwarf2/dwz.c \
 	dwarf2/expr.c \
 	dwarf2/frame-tailcall.c \
diff --git a/gdb/dwarf2/cu.c b/gdb/dwarf2/cu.c
new file mode 100644
index 00000000000..4f13f4f9677
--- /dev/null
+++ b/gdb/dwarf2/cu.c
@@ -0,0 +1,89 @@
+/* DWARF CU data structure
+
+   Copyright (C) 2021 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "defs.h"
+#include "dwarf2/cu.h"
+#include "dwarf2/read.h"
+
+/* Initialize dwarf2_cu to read PER_CU, in the context of PER_OBJFILE.  */
+
+dwarf2_cu::dwarf2_cu (dwarf2_per_cu_data *per_cu,
+		      dwarf2_per_objfile *per_objfile)
+  : per_cu (per_cu),
+    per_objfile (per_objfile),
+    mark (false),
+    has_loclist (false),
+    checked_producer (false),
+    producer_is_gxx_lt_4_6 (false),
+    producer_is_gcc_lt_4_3 (false),
+    producer_is_icc (false),
+    producer_is_icc_lt_14 (false),
+    producer_is_codewarrior (false),
+    processing_has_namespace_info (false)
+{
+}
+
+/* See cu.h.  */
+
+struct type *
+dwarf2_cu::addr_sized_int_type (bool unsigned_p) const
+{
+  int addr_size = this->per_cu->addr_size ();
+  return this->per_objfile->int_type (addr_size, unsigned_p);
+}
+
+/* Start a symtab for DWARF.  NAME, COMP_DIR, LOW_PC are passed to the
+   buildsym_compunit constructor.  */
+
+struct compunit_symtab *
+dwarf2_cu::start_symtab (const char *name, const char *comp_dir,
+			 CORE_ADDR low_pc)
+{
+  gdb_assert (m_builder == nullptr);
+
+  m_builder.reset (new struct buildsym_compunit
+		   (this->per_objfile->objfile,
+		    name, comp_dir, language, low_pc));
+
+  list_in_scope = get_builder ()->get_file_symbols ();
+
+  get_builder ()->record_debugformat ("DWARF 2");
+  get_builder ()->record_producer (producer);
+
+  processing_has_namespace_info = false;
+
+  return get_builder ()->get_compunit_symtab ();
+}
+
+/* See read.h.  */
+
+struct type *
+dwarf2_cu::addr_type () const
+{
+  struct objfile *objfile = this->per_objfile->objfile;
+  struct type *void_type = objfile_type (objfile)->builtin_void;
+  struct type *addr_type = lookup_pointer_type (void_type);
+  int addr_size = this->per_cu->addr_size ();
+
+  if (TYPE_LENGTH (addr_type) == addr_size)
+    return addr_type;
+
+  addr_type = addr_sized_int_type (addr_type->is_unsigned ());
+  return addr_type;
+}
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index b11fd0580f7..adb5442c9ab 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -18727,15 +18727,6 @@ dwarf2_per_objfile::int_type (int size_in_bytes, bool unsigned_p) const
   gdb_assert_not_reached ("unable to find suitable integer type");
 }
 
-/* See read.h.  */
-
-struct type *
-dwarf2_cu::addr_sized_int_type (bool unsigned_p) const
-{
-  int addr_size = this->per_cu->addr_size ();
-  return this->per_objfile->int_type (addr_size, unsigned_p);
-}
-
 /* Read the DW_AT_type attribute for a sub-range.  If this attribute is not
    present (which is valid) then compute the default type based on the
    compilation units address size.  */
@@ -21832,29 +21823,6 @@ dwarf2_start_subfile (struct dwarf2_cu *cu, const char *filename,
   cu->get_builder ()->start_subfile (filename);
 }
 
-/* Start a symtab for DWARF.  NAME, COMP_DIR, LOW_PC are passed to the
-   buildsym_compunit constructor.  */
-
-struct compunit_symtab *
-dwarf2_cu::start_symtab (const char *name, const char *comp_dir,
-			 CORE_ADDR low_pc)
-{
-  gdb_assert (m_builder == nullptr);
-
-  m_builder.reset (new struct buildsym_compunit
-		   (this->per_objfile->objfile,
-		    name, comp_dir, language, low_pc));
-
-  list_in_scope = get_builder ()->get_file_symbols ();
-
-  get_builder ()->record_debugformat ("DWARF 2");
-  get_builder ()->record_producer (producer);
-
-  processing_has_namespace_info = false;
-
-  return get_builder ()->get_compunit_symtab ();
-}
-
 static void
 var_decode_location (struct attribute *attr, struct symbol *sym,
 		     struct dwarf2_cu *cu)
@@ -24707,23 +24675,6 @@ dwarf2_per_cu_data::ref_addr_size () const
     return header->offset_size;
 }
 
-/* See read.h.  */
-
-struct type *
-dwarf2_cu::addr_type () const
-{
-  struct objfile *objfile = this->per_objfile->objfile;
-  struct type *void_type = objfile_type (objfile)->builtin_void;
-  struct type *addr_type = lookup_pointer_type (void_type);
-  int addr_size = this->per_cu->addr_size ();
-
-  if (TYPE_LENGTH (addr_type) == addr_size)
-    return addr_type;
-
-  addr_type = addr_sized_int_type (addr_type->is_unsigned ());
-  return addr_type;
-}
-
 /* A helper function for dwarf2_find_containing_comp_unit that returns
    the index of the result, and that searches a vector.  It will
    return a result even if the offset in question does not actually
@@ -24842,24 +24793,6 @@ run_test ()
 
 #endif /* GDB_SELF_TEST */
 
-/* Initialize dwarf2_cu to read PER_CU, in the context of PER_OBJFILE.  */
-
-dwarf2_cu::dwarf2_cu (dwarf2_per_cu_data *per_cu,
-		      dwarf2_per_objfile *per_objfile)
-  : per_cu (per_cu),
-    per_objfile (per_objfile),
-    mark (false),
-    has_loclist (false),
-    checked_producer (false),
-    producer_is_gxx_lt_4_6 (false),
-    producer_is_gcc_lt_4_3 (false),
-    producer_is_icc (false),
-    producer_is_icc_lt_14 (false),
-    producer_is_codewarrior (false),
-    processing_has_namespace_info (false)
-{
-}
-
 /* Initialize basic fields of dwarf_cu CU according to DIE COMP_UNIT_DIE.  */
 
 static void
-- 
2.26.2

[PATCH 3/3] Change dwarf2_cu marking to use methods

[Tom Tromey]

This changes the dwarf2_cu marking functions to be methods on
dwarf2_cu.

gdb/ChangeLog
2021-03-27  Tom Tromey  <tom@tromey.com>

	* dwarf2/read.c (maybe_queue_comp_unit)
	(dwarf2_per_objfile::age_comp_units): Update.
	(dwarf2_add_dependence, dwarf2_mark_helper, dwarf2_mark): Move to
	dwarf2_cu methods.
	* dwarf2/cu.h (struct dwarf2_cu) <mark, clear_mark, is_marked,
	add_dependence>: New methods.
	<m_dependencies, m_mark>: Rename, adding "m_" prefix.
	* dwarf2/cu.c (dwarf2_cu::dwarf2_cu): Update.
	(dwarf2_mark_helper): New function.
	(dwarf2_cu::mark, dwarf2_cu::add_dependence): New methods.
---
 gdb/ChangeLog     | 13 ++++++++
 gdb/dwarf2/cu.c   | 52 ++++++++++++++++++++++++++++++-
 gdb/dwarf2/cu.h   | 22 +++++++++++--
 gdb/dwarf2/read.c | 78 +++--------------------------------------------
 4 files changed, 88 insertions(+), 77 deletions(-)

diff --git a/gdb/dwarf2/cu.c b/gdb/dwarf2/cu.c
index 4f13f4f9677..2451df4f5b6 100644
--- a/gdb/dwarf2/cu.c
+++ b/gdb/dwarf2/cu.c
@@ -27,7 +27,7 @@ dwarf2_cu::dwarf2_cu (dwarf2_per_cu_data *per_cu,
 		      dwarf2_per_objfile *per_objfile)
   : per_cu (per_cu),
     per_objfile (per_objfile),
-    mark (false),
+    m_mark (false),
     has_loclist (false),
     checked_producer (false),
     producer_is_gxx_lt_4_6 (false),
@@ -87,3 +87,53 @@ dwarf2_cu::addr_type () const
   addr_type = addr_sized_int_type (addr_type->is_unsigned ());
   return addr_type;
 }
+
+/* A hashtab traversal function that marks the dependent CUs.  */
+
+static int
+dwarf2_mark_helper (void **slot, void *data)
+{
+  dwarf2_per_cu_data *per_cu = (dwarf2_per_cu_data *) *slot;
+  dwarf2_per_objfile *per_objfile = (dwarf2_per_objfile *) data;
+  dwarf2_cu *cu = per_objfile->get_cu (per_cu);
+
+  /* cu->m_dependencies references may not yet have been ever read if
+     QUIT aborts reading of the chain.  As such dependencies remain
+     valid it is not much useful to track and undo them during QUIT
+     cleanups.  */
+  if (cu != nullptr)
+    cu->mark ();
+  return 1;
+}
+
+/* See dwarf2/cu.h.  */
+
+void
+dwarf2_cu::mark ()
+{
+  if (!m_mark)
+    {
+      m_mark = true;
+      if (m_dependencies != nullptr)
+	htab_traverse (m_dependencies, dwarf2_mark_helper, per_objfile);
+    }
+}
+
+/* See dwarf2/cu.h.  */
+
+void
+dwarf2_cu::add_dependence (struct dwarf2_per_cu_data *ref_per_cu)
+{
+  void **slot;
+
+  if (m_dependencies == nullptr)
+    m_dependencies
+      = htab_create_alloc_ex (5, htab_hash_pointer, htab_eq_pointer,
+			      NULL, &comp_unit_obstack,
+			      hashtab_obstack_allocate,
+			      dummy_obstack_deallocate);
+
+  slot = htab_find_slot (m_dependencies, ref_per_cu, INSERT);
+  if (*slot == nullptr)
+    *slot = ref_per_cu;
+}
diff --git a/gdb/dwarf2/cu.h b/gdb/dwarf2/cu.h
index 9fb2d61163a..e469c5881c3 100644
--- a/gdb/dwarf2/cu.h
+++ b/gdb/dwarf2/cu.h
@@ -78,6 +78,24 @@ struct dwarf2_cu
      the integer is unsigned or not.  */
   struct type *addr_sized_int_type (bool unsigned_p) const;
 
+  /* Mark this CU as used.  */
+  void mark ();
+
+  /* Clear the mark on this CU.  */
+  void clear_mark ()
+  {
+    m_mark = false;
+  }
+
+  /* True if this CU has been marked.  */
+  bool is_marked () const
+  {
+    return m_mark;
+  }
+
+  /* Add a dependence relationship from this cu to REF_PER_CU.  */
+  void add_dependence (struct dwarf2_per_cu_data *ref_per_cu);
+
   /* The header of the compilation unit.  */
   struct comp_unit_head header {};
 
@@ -134,7 +152,7 @@ struct dwarf2_cu
   /* 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 = nullptr;
+  htab_t m_dependencies = nullptr;
 
   /* Header data from the line table, during full symbol processing.  */
   struct line_header *line_header = nullptr;
@@ -221,7 +239,7 @@ struct dwarf2_cu
   gdb::optional<ULONGEST> str_offsets_base;
 
   /* Mark used when releasing cached dies.  */
-  bool mark : 1;
+  bool m_mark : 1;
 
   /* This CU references .debug_loc.  See the symtab->locations_valid field.
      This test is imperfect as there may exist optimized debug code not using
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index adb5442c9ab..f5a5db86b01 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -1415,11 +1415,6 @@ static void process_full_comp_unit (dwarf2_cu *cu,
 static void process_full_type_unit (dwarf2_cu *cu,
 				    enum language pretend_language);
 
-static void dwarf2_add_dependence (struct dwarf2_cu *,
-				   struct dwarf2_per_cu_data *);
-
-static void dwarf2_mark (struct dwarf2_cu *);
-
 static struct type *get_die_type_at_offset (sect_offset,
 					    dwarf2_per_cu_data *per_cu,
 					    dwarf2_per_objfile *per_objfile);
@@ -8891,7 +8886,7 @@ maybe_queue_comp_unit (struct dwarf2_cu *dependent_cu,
   /* Mark the dependence relation so that we don't flush PER_CU
      too early.  */
   if (dependent_cu != NULL)
-    dwarf2_add_dependence (dependent_cu, per_cu);
+    dependent_cu->add_dependence (per_cu);
 
   /* If it's already on the queue, we have nothing to do.  */
   if (per_cu->queued)
@@ -24852,7 +24847,7 @@ dwarf2_per_objfile::age_comp_units ()
 
   /* Start by clearing all marks.  */
   for (auto pair : m_dwarf2_cus)
-    pair.second->mark = false;
+    pair.second->clear_mark ();
 
   /* Traverse all CUs, mark them and their dependencies if used recently
      enough.  */
@@ -24862,7 +24857,7 @@ dwarf2_per_objfile::age_comp_units ()
 
       cu->last_used++;
       if (cu->last_used <= dwarf_max_cache_age)
-	dwarf2_mark (cu);
+	cu->mark ();
     }
 
   /* Delete all CUs still not marked.  */
@@ -24870,7 +24865,7 @@ dwarf2_per_objfile::age_comp_units ()
     {
       dwarf2_cu *cu = it->second;
 
-      if (!cu->mark)
+      if (!cu->is_marked ())
 	{
 	  dwarf_read_debug_printf_v ("deleting old CU %s",
 				     sect_offset_str (cu->per_cu->sect_off));
@@ -25071,71 +25066,6 @@ get_die_type (struct die_info *die, struct dwarf2_cu *cu)
   return get_die_type_at_offset (die->sect_off, cu->per_cu, cu->per_objfile);
 }
 
-/* Add a dependence relationship from CU to REF_PER_CU.  */
-
-static void
-dwarf2_add_dependence (struct dwarf2_cu *cu,
-		       struct dwarf2_per_cu_data *ref_per_cu)
-{
-  void **slot;
-
-  if (cu->dependencies == NULL)
-    cu->dependencies
-      = htab_create_alloc_ex (5, htab_hash_pointer, htab_eq_pointer,
-			      NULL, &cu->comp_unit_obstack,
-			      hashtab_obstack_allocate,
-			      dummy_obstack_deallocate);
-
-  slot = htab_find_slot (cu->dependencies, ref_per_cu, INSERT);
-  if (*slot == NULL)
-    *slot = ref_per_cu;
-}
-
-/* Subroutine of dwarf2_mark to pass to htab_traverse.
-   Set the mark field in every compilation unit in the
-   cache that we must keep because we are keeping CU.
-
-   DATA is the dwarf2_per_objfile object in which to look up CUs.  */
-
-static int
-dwarf2_mark_helper (void **slot, void *data)
-{
-  dwarf2_per_cu_data *per_cu = (dwarf2_per_cu_data *) *slot;
-  dwarf2_per_objfile *per_objfile = (dwarf2_per_objfile *) data;
-  dwarf2_cu *cu = per_objfile->get_cu (per_cu);
-
-  /* cu->dependencies references may not yet have been ever read if QUIT aborts
-     reading of the chain.  As such dependencies remain valid it is not much
-     useful to track and undo them during QUIT cleanups.  */
-  if (cu == nullptr)
-    return 1;
-
-  if (cu->mark)
-    return 1;
-
-  cu->mark = true;
-
-  if (cu->dependencies != nullptr)
-    htab_traverse (cu->dependencies, dwarf2_mark_helper, per_objfile);
-
-  return 1;
-}
-
-/* Set the mark field in CU and in every other compilation unit in the
-   cache that we must keep because we are keeping CU.  */
-
-static void
-dwarf2_mark (struct dwarf2_cu *cu)
-{
-  if (cu->mark)
-    return;
-
-  cu->mark = true;
-
-  if (cu->dependencies != nullptr)
-    htab_traverse (cu->dependencies, dwarf2_mark_helper, cu->per_objfile);
-}
-
 /* Trivial hash function for partial_die_info: the hash value of a DIE
    is its offset in .debug_info for this objfile.  */
 
-- 
2.26.2

Re: [PATCH 3/3] Change dwarf2_cu marking to use methods

[Simon Marchi]

> @@ -134,7 +152,7 @@ struct dwarf2_cu
>    /* 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 = nullptr;
> +  htab_t m_dependencies = nullptr;

That can probably be moved to "private:".

The series LGTM, other than the concern I raised on patch 1, which is
really minor (and could be dealt with later).

Simon

Re: [PATCH 3/3] Change dwarf2_cu marking to use methods

[Tom Tromey]

>> @@ -134,7 +152,7 @@ struct dwarf2_cu
>> /* 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 = nullptr;
>> +  htab_t m_dependencies = nullptr;

Simon> That can probably be moved to "private:".

I made this change.

Simon> The series LGTM, other than the concern I raised on patch 1, which is
Simon> really minor (and could be dealt with later).

If we can think of a better name, it's simpler to do it now.
I will probably forget about it if we leave it.

Tom

Re: [PATCH 0/3] Move dwarf2_cu to its own file

[Simon Marchi]

On 2021-03-27 3:56 p.m., Tom Tromey wrote:
> This short series moves dwarf2_cu to a new header, and then moves some
> of the methods and helper functions to a new .c file as well.
> 
> This is just a minor cleanup coming from another series I'm working
> on.  I think that in general we should be splitting up dwarf2/read.c.
> It is still the largest source file in gdb by a large margin.

I recently attempted to move everything index-reading-related from
dwarf2/read.c to other files:

- dwarf2/index-gdb.{c,h}
- dwarf2/index-debug-names.{c,h}
- dwarf2/index-common.{c,h}

I think this would be nice because the index-reading stuff makes a big
chunk of dwarf2/read.c.  And it's not always clear to me that some
functions with a "generic" name, like dw2_map_expand_apply, are only
used for index-reading.

The only problem was dw2_get_file_names, which uses cutu_reader to read
some DIEs... cutu_reader is local to dwarf2/read.c, and I was not sure
if it would be a good idea to try to export it.

I am wondering if you attempted to do this before.

Simon

Re: [PATCH 0/3] Move dwarf2_cu to its own file

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> I recently attempted to move everything index-reading-related from
Simon> dwarf2/read.c to other files:

Simon> - dwarf2/index-gdb.{c,h}
Simon> - dwarf2/index-debug-names.{c,h}
Simon> - dwarf2/index-common.{c,h}

Simon> I think this would be nice because the index-reading stuff makes a big
Simon> chunk of dwarf2/read.c.  And it's not always clear to me that some
Simon> functions with a "generic" name, like dw2_map_expand_apply, are only
Simon> used for index-reading.

Simon> The only problem was dw2_get_file_names, which uses cutu_reader to read
Simon> some DIEs... cutu_reader is local to dwarf2/read.c, and I was not sure
Simon> if it would be a good idea to try to export it.

Simon> I am wondering if you attempted to do this before.

I don't think I attempted that specifically, but I have taken stabs at
moving other bits out, without real success.

I find the DWARF reader kind of depressingly spaghetti-like.
cutu_reader seems to overlap with die_reader_specs and with dwarf2_cu
itself -- they are all objects created temporarily while reading in a
CU.  However, they have different lifetimes and are passed to different
things.  Parts of dwarf2_cu are used by psymtab creation, but parts only
by full CU expansion.  The cutu_reader startup code is difficult to
follow.  Etc.

I've tried a few times to clean this up, but there are a lot of
scenarios to handle (fission, dwz, debug_types) and it's hard to be sure
that one is doing the right thing.

I do have a bunch of small cleanup patches kicking around, though I
haven't prepared them for submission.  (For example I have some patches
to try to clean up the comp-unit-head stuff in dwarf2_per_cu_data, and
avoid all the redundant fields in that object; but here in the end I
think I convinced myself that the dwarf2_cu still needs a separate copy
due to Fission, and kind of lost heart for finishing the work.)


Lately I've been working on replacing the DWARF psymtab reader.  My goal
here is to speed up gdb startup, and so far this approach looks promising.

I wanted to make this new scanner multi-threaded, but due to the
cutu_reader / dwarf2_cu stuff, this looks a bit tricky.

I did take one step toward this by having gdb read all the abbrevs up
front, so there aren't abbrev table lifetime issues.  I don't know if
this is advisable, though.  Maybe it's better to have one abbrev table
cache per thread instead.

To tie this back to the current discussion, a lot of this code ended up
in dwarf2/read.c again, simply because of the need for a cutu_reader.
So, moving that out, if possible, would at least let us break up read.c
a little.

I think my ideal here would be that preparing to scan DIEs from a CU
would be simple.  There could be some kind of base class that the reader
(either the CU expansion reader, or the psymtab reader / new scanner)
could subclass.  This would take a dwarf2_per_cu_data parameter and the
lifetime handling would be up to the user.  (That is, no mandatory cache
aging stuff.)  No idea if this is practical.

Tom