[PATCH 0/7 v2] Class-fy partial_die_info

[PATCH 0/7 v2] Class-fy partial_die_info

[Yao Qi]

When I fix some issues related to dwarf, I class-fy partial_die_info as
part of the fix.  The class-fication bits can go upstream first.

This is the v2 of this patch series, v1 can be found
https://sourceware.org/ml/gdb-patches/2018-01/msg00505.html Changes
in v2 are,

 - Don't move the location of partial_die_info::fixup_called, as it may
   increase the memory usage,
 - Add some comments to read_partial_die,

*** BLURB HERE ***

Yao Qi (7):
  Re-write partial_die_info allocation in load_partial_dies
  Don't check abbrev is NULL in read_partial_die
  Change find_partial_die_in_comp_unit to dwarf2_cu::find_partial_die
  Class-fy partial_die_info
  Remove one argument abbrev_len in read_partial_die
  Move fixup_partial_die to partial_die_info::fixup
  Move read_partial_die to partial_die_info::read

 gdb/dwarf2read.c | 337 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 183 insertions(+), 154 deletions(-)

-- 
1.9.1

[PATCH 3/7] Change find_partial_die_in_comp_unit to dwarf2_cu::find_partial_die

[Yao Qi]

This patch changes find_partial_die_in_comp_unit to a dwarf2_cu method
find_partial_die.

gdb:

2018-01-11  Yao Qi  <yao.qi@linaro.org>

	* dwarf2read.c (struct dwarf2_cu) <find_partial_die>: New method.
	(find_partial_die_in_comp_unit): Change it to
	dwarf2_cu::find_partial_die.
	(find_partial_die): Update.
---
 gdb/dwarf2read.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 147c7ca..f386873 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -785,6 +785,8 @@ struct dwarf2_cu
      this information, but later versions do.  */
 
   unsigned int processing_has_namespace_info : 1;
+
+  struct partial_die_info *find_partial_die (sect_offset sect_off);
 };
 
 /* Persistent data held for a compilation unit, even when not
@@ -18701,15 +18703,15 @@ read_partial_die (const struct die_reader_specs *reader,
 
 /* Find a cached partial DIE at OFFSET in CU.  */
 
-static struct partial_die_info *
-find_partial_die_in_comp_unit (sect_offset sect_off, struct dwarf2_cu *cu)
+struct partial_die_info *
+dwarf2_cu::find_partial_die (sect_offset sect_off)
 {
   struct partial_die_info *lookup_die = NULL;
   struct partial_die_info part_die;
 
   part_die.sect_off = sect_off;
   lookup_die = ((struct partial_die_info *)
-		htab_find_with_hash (cu->partial_dies, &part_die,
+		htab_find_with_hash (partial_dies, &part_die,
 				     to_underlying (sect_off)));
 
   return lookup_die;
@@ -18732,7 +18734,7 @@ find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
   if (offset_in_dwz == cu->per_cu->is_dwz
       && offset_in_cu_p (&cu->header, sect_off))
     {
-      pd = find_partial_die_in_comp_unit (sect_off, cu);
+      pd = cu->find_partial_die (sect_off);
       if (pd != NULL)
 	return pd;
       /* We missed recording what we needed.
@@ -18756,7 +18758,7 @@ find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
 	load_partial_comp_unit (per_cu);
 
       per_cu->cu->last_used = 0;
-      pd = find_partial_die_in_comp_unit (sect_off, per_cu->cu);
+      pd = per_cu->cu->find_partial_die (sect_off);
     }
 
   /* If we didn't find it, and not all dies have been loaded,
@@ -18774,7 +18776,7 @@ find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
 	 set.  */
       load_partial_comp_unit (per_cu);
 
-      pd = find_partial_die_in_comp_unit (sect_off, per_cu->cu);
+      pd = per_cu->cu->find_partial_die (sect_off);
     }
 
   if (pd == NULL)
-- 
1.9.1

[PATCH 1/7] Re-write partial_die_info allocation in load_partial_dies

[Yao Qi]

load_partial_dies has a "while (1)" loop to visit each die, and create
partial_die_info if needed in each iteration, like this,

  part_die = XOBNEW (&cu->comp_unit_obstack, struct partial_die_info);

  while (1)
   {
      if (foo1) continue;

      if (foo2) continue;

      read_partial_die (, , part_die, ,);

      ....
      part_die = XOBNEW (&cu->comp_unit_obstack, struct partial_die_info);
   };

the code was written in a way that spaces are allocated on necessary on
cu->comp_unit_obstack.  I want to class-fy partial_die_info, but
partial_die_info ctor can't follow XOBNEW immediately, so this patch
rewrite this loop to:

  while (1)
   {
      if (foo1) continue;

      if (foo2) continue;

      struct partial_die_info pdi;
      read_partial_die (, , &pdi, ,);

      part_die = XOBNEW (&cu->comp_unit_obstack, struct partial_die_info);
      memcpy (part_die, &pdi, sizeof (pdi));
   };

we create a local variable pdi, if we need it, call XOBNEW, and copy.
This also reduce one XOBNEW call.  I measured the number of XOBNEW call in
load_partial_dies when gdb reads dwarf2read.o, without this patch, it is
18827, and with this patch, it is 18826.

gdb:

2018-01-18  Yao Qi  <yao.qi@linaro.org>

	* dwarf2read.c (load_partial_dies): Move the location of XOBNEW.
---
 gdb/dwarf2read.c | 55 +++++++++++++++++++++++++------------------------------
 1 file changed, 25 insertions(+), 30 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 6cdb963..81b42cf 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -18216,7 +18216,6 @@ load_partial_dies (const struct die_reader_specs *reader,
 {
   struct dwarf2_cu *cu = reader->cu;
   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;
   unsigned int bytes_read;
   unsigned int load_all = 0;
@@ -18238,8 +18237,6 @@ load_partial_dies (const struct die_reader_specs *reader,
 			    hashtab_obstack_allocate,
 			    dummy_obstack_deallocate);
 
-  part_die = XOBNEW (&cu->comp_unit_obstack, struct partial_die_info);
-
   while (1)
     {
       abbrev_info *abbrev = peek_die_abbrev (*reader, info_ptr, &bytes_read);
@@ -18248,15 +18245,8 @@ load_partial_dies (const struct die_reader_specs *reader,
       if (abbrev == NULL)
 	{
 	  if (--nesting_level == 0)
-	    {
-	      /* PART_DIE was probably the last thing allocated on the
-		 comp_unit_obstack, so we could call obstack_free
-		 here.  We don't do that because the waste is small,
-		 and will be cleaned up when we're done with this
-		 compilation unit.  This way, we're also more robust
-		 against other users of the comp_unit_obstack.  */
-	      return first_die;
-	    }
+	    return first_die;
+
 	  info_ptr += bytes_read;
 	  last_die = parent_die;
 	  parent_die = parent_die->die_parent;
@@ -18314,7 +18304,10 @@ load_partial_dies (const struct die_reader_specs *reader,
 	  continue;
 	}
 
-      info_ptr = read_partial_die (reader, part_die, abbrev, bytes_read,
+      struct partial_die_info pdi;
+
+      memset (&pdi, 0, sizeof (pdi));
+      info_ptr = read_partial_die (reader, &pdi, abbrev, bytes_read,
 				   info_ptr);
 
       /* This two-pass algorithm for processing partial symbols has a
@@ -18334,18 +18327,18 @@ load_partial_dies (const struct die_reader_specs *reader,
 	 of them, for a language without namespaces), can be processed
 	 directly.  */
       if (parent_die == NULL
-	  && part_die->has_specification == 0
-	  && part_die->is_declaration == 0
-	  && ((part_die->tag == DW_TAG_typedef && !part_die->has_children)
-	      || part_die->tag == DW_TAG_base_type
-	      || part_die->tag == DW_TAG_subrange_type))
-	{
-	  if (building_psymtab && part_die->name != NULL)
-	    add_psymbol_to_list (part_die->name, strlen (part_die->name), 0,
+	  && pdi.has_specification == 0
+	  && pdi.is_declaration == 0
+	  && ((pdi.tag == DW_TAG_typedef && !pdi.has_children)
+	      || pdi.tag == DW_TAG_base_type
+	      || pdi.tag == DW_TAG_subrange_type))
+	{
+	  if (building_psymtab && pdi.name != NULL)
+	    add_psymbol_to_list (pdi.name, strlen (pdi.name), 0,
 				 VAR_DOMAIN, LOC_TYPEDEF,
 				 &objfile->static_psymbols,
 				 0, cu->language, objfile);
-	  info_ptr = locate_pdi_sibling (reader, part_die, info_ptr);
+	  info_ptr = locate_pdi_sibling (reader, &pdi, info_ptr);
 	  continue;
 	}
 
@@ -18357,37 +18350,41 @@ load_partial_dies (const struct die_reader_specs *reader,
 	 it could not find the child DIEs referenced later, this is checked
 	 above.  In correct DWARF DW_TAG_typedef should have no children.  */
 
-      if (part_die->tag == DW_TAG_typedef && part_die->has_children)
+      if (pdi.tag == DW_TAG_typedef && pdi.has_children)
 	complaint (&symfile_complaints,
 		   _("DW_TAG_typedef has childen - GCC PR debug/47510 bug "
 		     "- DIE at 0x%x [in module %s]"),
-		   to_underlying (part_die->sect_off), objfile_name (objfile));
+		   to_underlying (pdi.sect_off), objfile_name (objfile));
 
       /* If we're at the second level, and we're an enumerator, and
 	 our parent has no specification (meaning possibly lives in a
 	 namespace elsewhere), then we can add the partial symbol now
 	 instead of queueing it.  */
-      if (part_die->tag == DW_TAG_enumerator
+      if (pdi.tag == DW_TAG_enumerator
 	  && parent_die != NULL
 	  && parent_die->die_parent == NULL
 	  && parent_die->tag == DW_TAG_enumeration_type
 	  && parent_die->has_specification == 0)
 	{
-	  if (part_die->name == NULL)
+	  if (pdi.name == NULL)
 	    complaint (&symfile_complaints,
 		       _("malformed enumerator DIE ignored"));
 	  else if (building_psymtab)
-	    add_psymbol_to_list (part_die->name, strlen (part_die->name), 0,
+	    add_psymbol_to_list (pdi.name, strlen (pdi.name), 0,
 				 VAR_DOMAIN, LOC_CONST,
 				 cu->language == language_cplus
 				 ? &objfile->global_psymbols
 				 : &objfile->static_psymbols,
 				 0, cu->language, objfile);
 
-	  info_ptr = locate_pdi_sibling (reader, part_die, info_ptr);
+	  info_ptr = locate_pdi_sibling (reader, &pdi, info_ptr);
 	  continue;
 	}
 
+      struct partial_die_info *part_die
+	  = XOBNEW (&cu->comp_unit_obstack, struct partial_die_info);
+
+      memcpy (part_die, &pdi, sizeof (pdi));
       /* We'll save this DIE so link it in.  */
       part_die->die_parent = parent_die;
       part_die->die_sibling = NULL;
@@ -18439,8 +18436,6 @@ load_partial_dies (const struct die_reader_specs *reader,
 	  *slot = part_die;
 	}
 
-      part_die = XOBNEW (&cu->comp_unit_obstack, struct partial_die_info);
-
       /* For some DIEs we want to follow their children (if any).  For C
 	 we have no reason to follow the children of structures; for other
 	 languages we have to, so that we can get at method physnames
-- 
1.9.1

[PATCH 5/7] Remove one argument abbrev_len in read_partial_die

[Yao Qi]

gdb:

2018-01-11  Yao Qi  <yao.qi@linaro.org>

	* dwarf2read.c (read_partial_die): Update the declaration.
	(load_partial_dies): Caller update.
	(read_partial_die): Remove one argument abbrev_len.
---
 gdb/dwarf2read.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index b01d9f3..333a890 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -1834,7 +1834,6 @@ static struct partial_die_info *load_partial_dies
 static const gdb_byte *read_partial_die (const struct die_reader_specs *,
 					 struct partial_die_info *,
 					 struct abbrev_info *,
-					 unsigned int,
 					 const gdb_byte *);
 
 static struct partial_die_info *find_partial_die (sect_offset, int,
@@ -18348,8 +18347,8 @@ load_partial_dies (const struct die_reader_specs *reader,
       struct partial_die_info pdi ((sect_offset) (info_ptr - reader->buffer),
 				   abbrev);
 
-      info_ptr = read_partial_die (reader, &pdi, abbrev, bytes_read,
-				   info_ptr);
+      info_ptr = read_partial_die (reader, &pdi, abbrev,
+				   (const gdb_byte *) info_ptr + bytes_read);
 
       /* This two-pass algorithm for processing partial symbols has a
 	 high cost in cache pressure.  Thus, handle some simple cases
@@ -18524,13 +18523,13 @@ partial_die_info::partial_die_info (sect_offset sect_off_,
 {
 }
 
-/* Read a minimal amount of information into the minimal die structure.  */
+/* Read a minimal amount of information into the minimal die structure.
+   INFO_PTR should point just after the initial uleb128 of a DIE.  */
 
 static const gdb_byte *
 read_partial_die (const struct die_reader_specs *reader,
 		  struct partial_die_info *part_die,
-		  struct abbrev_info *abbrev, unsigned int abbrev_len,
-		  const gdb_byte *info_ptr)
+		  struct abbrev_info *abbrev, const gdb_byte *info_ptr)
 {
   struct dwarf2_cu *cu = reader->cu;
   struct dwarf2_per_objfile *dwarf2_per_objfile
@@ -18543,8 +18542,6 @@ read_partial_die (const struct die_reader_specs *reader,
   int has_high_pc_attr = 0;
   int high_pc_relative = 0;
 
-  info_ptr += abbrev_len;
-
   for (i = 0; i < abbrev->num_attrs; ++i)
     {
       info_ptr = read_attribute (reader, &attr, &abbrev->attrs[i], info_ptr);
-- 
1.9.1

[PATCH 4/7] Class-fy partial_die_info

[Yao Qi]

This patch is to class-fy partial_die_info.  Two things special here,

 - disable assignment operator, but keep copy ctor, which is used in
   load_partial_dies,
 - have a private ctor which is only used by dwarf2_cu::find_partial_die,
   I don't want other code use it, so make it private,

gdb:

2018-01-11  Yao Qi  <yao.qi@linaro.org>

	* dwarf2read.c (struct partial_die_info): Add ctor, delete
	assignment operator.
	(load_partial_dies): Use ctor and copy ctor.
	(read_partial_die): Update.
	(dwarf2_cu::find_partial_die): Use ctor.
---
 gdb/dwarf2read.c | 88 +++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 62 insertions(+), 26 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index f386873..b01d9f3 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -1402,16 +1402,23 @@ file_entry::include_dir (const line_header *lh) const
 
 /* When we construct a partial symbol table entry we only
    need this much information.  */
-struct partial_die_info
+struct partial_die_info : public allocate_on_obstack
   {
+    partial_die_info (sect_offset sect_off, struct abbrev_info *abbrev);
+
+    /* Disable assign but still keep copy ctor, which is needed
+       load_partial_dies.   */
+    partial_die_info& operator=(const partial_die_info& rhs) = delete;
+
     /* Offset of this DIE.  */
-    sect_offset sect_off;
+    const sect_offset sect_off;
 
     /* DWARF-2 tag for this DIE.  */
-    ENUM_BITFIELD(dwarf_tag) tag : 16;
+    const ENUM_BITFIELD(dwarf_tag) tag : 16;
 
     /* Assorted flags describing the data found in this DIE.  */
-    unsigned int has_children : 1;
+    const unsigned int has_children : 1;
+
     unsigned int is_external : 1;
     unsigned int is_declaration : 1;
     unsigned int has_type : 1;
@@ -1446,15 +1453,15 @@ struct partial_die_info
 
     /* The name of this DIE.  Normally the value of DW_AT_name, but
        sometimes a default name for unnamed DIEs.  */
-    const char *name;
+    const char *name = nullptr;
 
     /* The linkage name, if present.  */
-    const char *linkage_name;
+    const char *linkage_name = nullptr;
 
     /* The scope to prepend to our children.  This is generally
        allocated on the comp_unit_obstack, so will disappear
        when this compilation unit leaves the cache.  */
-    const char *scope;
+    const char *scope = nullptr;
 
     /* Some data associated with the partial DIE.  The tag determines
        which field is live.  */
@@ -1464,26 +1471,58 @@ struct partial_die_info
       struct dwarf_block *locdesc;
       /* The offset of an import, for DW_TAG_imported_unit.  */
       sect_offset sect_off;
-    } d;
+    } d {};
 
     /* If HAS_PC_INFO, the PC range associated with this DIE.  */
-    CORE_ADDR lowpc;
-    CORE_ADDR highpc;
+    CORE_ADDR lowpc = 0;
+    CORE_ADDR highpc = 0;
 
     /* Pointer into the info_buffer (or types_buffer) pointing at the target of
        DW_AT_sibling, if any.  */
     /* NOTE: This member isn't strictly necessary, read_partial_die could
        return DW_AT_sibling values to its caller load_partial_dies.  */
-    const gdb_byte *sibling;
+    const gdb_byte *sibling = nullptr;
 
     /* If HAS_SPECIFICATION, the offset of the DIE referred to by
        DW_AT_specification (or DW_AT_abstract_origin or
        DW_AT_extension).  */
-    sect_offset spec_offset;
+    sect_offset spec_offset {};
 
     /* Pointers to this DIE's parent, first child, and next sibling,
        if any.  */
-    struct partial_die_info *die_parent, *die_child, *die_sibling;
+    struct partial_die_info *die_parent = nullptr;
+    struct partial_die_info *die_child = nullptr;
+    struct partial_die_info *die_sibling = nullptr;
+
+    friend struct partial_die_info *
+    dwarf2_cu::find_partial_die (sect_offset sect_off);
+
+  private:
+    /* Only need to do look up in dwarf2_cu::find_partial_die.  */
+    partial_die_info (sect_offset sect_off)
+      : partial_die_info (sect_off, DW_TAG_padding, 0)
+    {
+    }
+
+    partial_die_info (sect_offset sect_off_, enum dwarf_tag tag_,
+		      int has_children_)
+      : sect_off (sect_off_), tag (tag_), has_children (has_children_)
+    {
+      is_external = 0;
+      is_declaration = 0;
+      has_type = 0;
+      has_specification = 0;
+      has_pc_info = 0;
+      may_be_inlined = 0;
+      main_subprogram = 0;
+      scope_set = 0;
+      has_byte_size = 0;
+      has_const_value = 0;
+      has_template_arguments = 0;
+      fixup_called = 0;
+      is_dwz = 0;
+      spec_is_dwz = 0;
+    }
   };
 
 /* This data structure holds the information of an abbrev.  */
@@ -18306,9 +18345,9 @@ load_partial_dies (const struct die_reader_specs *reader,
 	  continue;
 	}
 
-      struct partial_die_info pdi;
+      struct partial_die_info pdi ((sect_offset) (info_ptr - reader->buffer),
+				   abbrev);
 
-      memset (&pdi, 0, sizeof (pdi));
       info_ptr = read_partial_die (reader, &pdi, abbrev, bytes_read,
 				   info_ptr);
 
@@ -18384,9 +18423,8 @@ load_partial_dies (const struct die_reader_specs *reader,
 	}
 
       struct partial_die_info *part_die
-	  = XOBNEW (&cu->comp_unit_obstack, struct partial_die_info);
+	= new (&cu->comp_unit_obstack) partial_die_info (pdi);
 
-      memcpy (part_die, &pdi, sizeof (pdi));
       /* We'll save this DIE so link it in.  */
       part_die->die_parent = parent_die;
       part_die->die_sibling = NULL;
@@ -18480,6 +18518,12 @@ load_partial_dies (const struct die_reader_specs *reader,
     }
 }
 
+partial_die_info::partial_die_info (sect_offset sect_off_,
+				    struct abbrev_info *abbrev)
+  : partial_die_info (sect_off_, abbrev->tag, abbrev->has_children)
+{
+}
+
 /* Read a minimal amount of information into the minimal die structure.  */
 
 static const gdb_byte *
@@ -18499,15 +18543,8 @@ read_partial_die (const struct die_reader_specs *reader,
   int has_high_pc_attr = 0;
   int high_pc_relative = 0;
 
-  memset (part_die, 0, sizeof (struct partial_die_info));
-
-  part_die->sect_off = (sect_offset) (info_ptr - buffer);
-
   info_ptr += abbrev_len;
 
-  part_die->tag = abbrev->tag;
-  part_die->has_children = abbrev->has_children;
-
   for (i = 0; i < abbrev->num_attrs; ++i)
     {
       info_ptr = read_attribute (reader, &attr, &abbrev->attrs[i], info_ptr);
@@ -18707,9 +18744,8 @@ struct partial_die_info *
 dwarf2_cu::find_partial_die (sect_offset sect_off)
 {
   struct partial_die_info *lookup_die = NULL;
-  struct partial_die_info part_die;
+  struct partial_die_info part_die (sect_off);
 
-  part_die.sect_off = sect_off;
   lookup_die = ((struct partial_die_info *)
 		htab_find_with_hash (partial_dies, &part_die,
 				     to_underlying (sect_off)));
-- 
1.9.1

[PATCH 2/7] Don't check abbrev is NULL in read_partial_die

[Yao Qi]

'abbrev' won't be NULL, so don't check it.

gdb:

2018-01-11  Yao Qi  <yao.qi@linaro.org>

	* dwarf2read.c (read_partial_die): Remove the code checking abbrev
	is NULL.
---
 gdb/dwarf2read.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 81b42cf..147c7ca 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -18503,9 +18503,6 @@ read_partial_die (const struct die_reader_specs *reader,
 
   info_ptr += abbrev_len;
 
-  if (abbrev == NULL)
-    return info_ptr;
-
   part_die->tag = abbrev->tag;
   part_die->has_children = abbrev->has_children;
 
-- 
1.9.1

Re: [PATCH 2/7] Don't check abbrev is NULL in read_partial_die

[Simon Marchi]

On 2018-02-22 10:36 AM, Yao Qi wrote:
> 'abbrev' won't be NULL, so don't check it.
> 
> gdb:

I would suggest changing abbrev type to a const reference then.

Simon

[PATCH 7/7] Move read_partial_die to partial_die_info::read

[Yao Qi]

gdb:

2018-01-11  Yao Qi  <yao.qi@linaro.org>

	* dwarf2read.c (struct partial_die_info) <read>: New method.
	(read_partial_die): Remove the declaration.
	(load_partial_dies): Update.
	(partial_die_info::partial_die_info):
	(read_partial_die): Change it to partial_die_info::read.
---
 gdb/dwarf2read.c | 107 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 56 insertions(+), 51 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 7823c3b..d9eb648 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -1415,6 +1415,12 @@ struct partial_die_info : public allocate_on_obstack
        name.  */
     void fixup (struct dwarf2_cu *cu);
 
+    /* Read a minimal amount of information into the minimal die
+       structure.  */
+    const gdb_byte *read (const struct die_reader_specs *reader,
+			  struct abbrev_info *abbrev,
+			  const gdb_byte *info_ptr);
+
     /* Offset of this DIE.  */
     const sect_offset sect_off;
 
@@ -1484,8 +1490,8 @@ struct partial_die_info : public allocate_on_obstack
 
     /* Pointer into the info_buffer (or types_buffer) pointing at the target of
        DW_AT_sibling, if any.  */
-    /* NOTE: This member isn't strictly necessary, read_partial_die could
-       return DW_AT_sibling values to its caller load_partial_dies.  */
+    /* NOTE: This member isn't strictly necessary, partial_die_info::read
+       could return DW_AT_sibling values to its caller load_partial_dies.  */
     const gdb_byte *sibling = nullptr;
 
     /* If HAS_SPECIFICATION, the offset of the DIE referred to by
@@ -1836,11 +1842,6 @@ static unsigned int peek_abbrev_code (bfd *, const gdb_byte *);
 static struct partial_die_info *load_partial_dies
   (const struct die_reader_specs *, const gdb_byte *, int);
 
-static const gdb_byte *read_partial_die (const struct die_reader_specs *,
-					 struct partial_die_info *,
-					 struct abbrev_info *,
-					 const gdb_byte *);
-
 static struct partial_die_info *find_partial_die (sect_offset, int,
 						  struct dwarf2_cu *);
 
@@ -14837,7 +14838,7 @@ dwarf2_get_pc_bounds (struct die_info *die, CORE_ADDR *lowpc,
 	return PC_BOUNDS_NOT_PRESENT;
     }
 
-  /* read_partial_die has also the strict LOW < HIGH requirement.  */
+  /* partial_die_info::read has also the strict LOW < HIGH requirement.  */
   if (high <= low)
     return PC_BOUNDS_INVALID;
 
@@ -18349,8 +18350,7 @@ load_partial_dies (const struct die_reader_specs *reader,
       struct partial_die_info pdi ((sect_offset) (info_ptr - reader->buffer),
 				   abbrev);
 
-      info_ptr = read_partial_die (reader, &pdi, abbrev,
-				   (const gdb_byte *) info_ptr + bytes_read);
+      info_ptr = pdi.read (reader, abbrev, info_ptr + bytes_read);
 
       /* This two-pass algorithm for processing partial symbols has a
 	 high cost in cache pressure.  Thus, handle some simple cases
@@ -18528,24 +18528,22 @@ partial_die_info::partial_die_info (sect_offset sect_off_,
 /* Read a minimal amount of information into the minimal die structure.
    INFO_PTR should point just after the initial uleb128 of a DIE.  */
 
-static const gdb_byte *
-read_partial_die (const struct die_reader_specs *reader,
-		  struct partial_die_info *part_die,
-		  struct abbrev_info *abbrev, const gdb_byte *info_ptr)
+const gdb_byte *
+partial_die_info::read (const struct die_reader_specs *reader,
+			struct abbrev_info *abbrev, const gdb_byte *info_ptr)
 {
   struct dwarf2_cu *cu = reader->cu;
   struct dwarf2_per_objfile *dwarf2_per_objfile
     = cu->per_cu->dwarf2_per_objfile;
-  struct objfile *objfile = dwarf2_per_objfile->objfile;
-  const gdb_byte *buffer = reader->buffer;
   unsigned int i;
-  struct attribute attr;
   int has_low_pc_attr = 0;
   int has_high_pc_attr = 0;
   int high_pc_relative = 0;
 
   for (i = 0; i < abbrev->num_attrs; ++i)
     {
+      struct attribute attr;
+
       info_ptr = read_attribute (reader, &attr, &abbrev->attrs[i], info_ptr);
 
       /* Store the data if it is of an attribute we want to keep in a
@@ -18553,7 +18551,7 @@ read_partial_die (const struct die_reader_specs *reader,
       switch (attr.name)
 	{
 	case DW_AT_name:
-	  switch (part_die->tag)
+	  switch (tag)
 	    {
 	    case DW_TAG_compile_unit:
 	    case DW_TAG_partial_unit:
@@ -18564,12 +18562,16 @@ read_partial_die (const struct die_reader_specs *reader,
 	    case DW_TAG_enumerator:
 	      /* These tags always have simple identifiers already; no need
 		 to canonicalize them.  */
-	      part_die->name = DW_STRING (&attr);
+	      name = DW_STRING (&attr);
 	      break;
 	    default:
-	      part_die->name
-		= dwarf2_canonicalize_name (DW_STRING (&attr), cu,
-					    &objfile->per_bfd->storage_obstack);
+	      {
+		struct objfile *objfile = dwarf2_per_objfile->objfile;
+
+		name
+		  = dwarf2_canonicalize_name (DW_STRING (&attr), cu,
+					      &objfile->per_bfd->storage_obstack);
+	      }
 	      break;
 	    }
 	  break;
@@ -18579,16 +18581,16 @@ read_partial_die (const struct die_reader_specs *reader,
 	     assume they will be the same, and we only store the last
 	     one we see.  */
 	  if (cu->language == language_ada)
-	    part_die->name = DW_STRING (&attr);
-	  part_die->linkage_name = DW_STRING (&attr);
+	    name = DW_STRING (&attr);
+	  linkage_name = DW_STRING (&attr);
 	  break;
 	case DW_AT_low_pc:
 	  has_low_pc_attr = 1;
-	  part_die->lowpc = attr_value_as_address (&attr);
+	  lowpc = attr_value_as_address (&attr);
 	  break;
 	case DW_AT_high_pc:
 	  has_high_pc_attr = 1;
-	  part_die->highpc = attr_value_as_address (&attr);
+	  highpc = attr_value_as_address (&attr);
 	  if (cu->header.version >= 4 && attr_form_is_constant (&attr))
 		high_pc_relative = 1;
 	  break;
@@ -18596,7 +18598,7 @@ read_partial_die (const struct die_reader_specs *reader,
           /* Support the .debug_loc offsets.  */
           if (attr_form_is_block (&attr))
             {
-	       part_die->d.locdesc = DW_BLOCK (&attr);
+	       d.locdesc = DW_BLOCK (&attr);
             }
           else if (attr_form_is_section_offset (&attr))
             {
@@ -18609,20 +18611,20 @@ read_partial_die (const struct die_reader_specs *reader,
             }
 	  break;
 	case DW_AT_external:
-	  part_die->is_external = DW_UNSND (&attr);
+	  is_external = DW_UNSND (&attr);
 	  break;
 	case DW_AT_declaration:
-	  part_die->is_declaration = DW_UNSND (&attr);
+	  is_declaration = DW_UNSND (&attr);
 	  break;
 	case DW_AT_type:
-	  part_die->has_type = 1;
+	  has_type = 1;
 	  break;
 	case DW_AT_abstract_origin:
 	case DW_AT_specification:
 	case DW_AT_extension:
-	  part_die->has_specification = 1;
-	  part_die->spec_offset = dwarf2_get_ref_die_offset (&attr);
-	  part_die->spec_is_dwz = (attr.form == DW_FORM_GNU_ref_alt
+	  has_specification = 1;
+	  spec_offset = dwarf2_get_ref_die_offset (&attr);
+	  spec_is_dwz = (attr.form == DW_FORM_GNU_ref_alt
 				   || cu->per_cu->is_dwz);
 	  break;
 	case DW_AT_sibling:
@@ -18633,6 +18635,7 @@ read_partial_die (const struct die_reader_specs *reader,
 		       _("ignoring absolute DW_AT_sibling"));
 	  else
 	    {
+	      const gdb_byte *buffer = reader->buffer;
 	      sect_offset off = dwarf2_get_ref_die_offset (&attr);
 	      const gdb_byte *sibling_ptr = buffer + to_underlying (off);
 
@@ -18642,14 +18645,14 @@ read_partial_die (const struct die_reader_specs *reader,
 	      else if (sibling_ptr > reader->buffer_end)
 		dwarf2_section_buffer_overflow_complaint (reader->die_section);
 	      else
-		part_die->sibling = sibling_ptr;
+		sibling = sibling_ptr;
 	    }
 	  break;
         case DW_AT_byte_size:
-          part_die->has_byte_size = 1;
+          has_byte_size = 1;
           break;
         case DW_AT_const_value:
-          part_die->has_const_value = 1;
+          has_const_value = 1;
           break;
 	case DW_AT_calling_convention:
 	  /* DWARF doesn't provide a way to identify a program's source-level
@@ -18668,25 +18671,25 @@ read_partial_die (const struct die_reader_specs *reader,
 	     compatibility.  */
 	  if (DW_UNSND (&attr) == DW_CC_program
 	      && cu->language == language_fortran)
-	    part_die->main_subprogram = 1;
+	    main_subprogram = 1;
 	  break;
 	case DW_AT_inline:
 	  if (DW_UNSND (&attr) == DW_INL_inlined
 	      || DW_UNSND (&attr) == DW_INL_declared_inlined)
-	    part_die->may_be_inlined = 1;
+	    may_be_inlined = 1;
 	  break;
 
 	case DW_AT_import:
-	  if (part_die->tag == DW_TAG_imported_unit)
+	  if (tag == DW_TAG_imported_unit)
 	    {
-	      part_die->d.sect_off = dwarf2_get_ref_die_offset (&attr);
-	      part_die->is_dwz = (attr.form == DW_FORM_GNU_ref_alt
+	      d.sect_off = dwarf2_get_ref_die_offset (&attr);
+	      is_dwz = (attr.form == DW_FORM_GNU_ref_alt
 				  || cu->per_cu->is_dwz);
 	    }
 	  break;
 
 	case DW_AT_main_subprogram:
-	  part_die->main_subprogram = DW_UNSND (&attr);
+	  main_subprogram = DW_UNSND (&attr);
 	  break;
 
 	default:
@@ -18695,7 +18698,7 @@ read_partial_die (const struct die_reader_specs *reader,
     }
 
   if (high_pc_relative)
-    part_die->highpc += part_die->lowpc;
+    highpc += lowpc;
 
   if (has_low_pc_attr && has_high_pc_attr)
     {
@@ -18707,31 +18710,33 @@ read_partial_die (const struct die_reader_specs *reader,
 	 labels are not in the output, so the relocs get a value of 0.
 	 If this is a discarded function, mark the pc bounds as invalid,
 	 so that GDB will ignore it.  */
-      if (part_die->lowpc == 0 && !dwarf2_per_objfile->has_section_at_zero)
+      if (lowpc == 0 && !dwarf2_per_objfile->has_section_at_zero)
 	{
+	  struct objfile *objfile = dwarf2_per_objfile->objfile;
 	  struct gdbarch *gdbarch = get_objfile_arch (objfile);
 
 	  complaint (&symfile_complaints,
 		     _("DW_AT_low_pc %s is zero "
 		       "for DIE at 0x%x [in module %s]"),
-		     paddress (gdbarch, part_die->lowpc),
-		     to_underlying (part_die->sect_off), objfile_name (objfile));
+		     paddress (gdbarch, lowpc),
+		     to_underlying (sect_off), objfile_name (objfile));
 	}
       /* dwarf2_get_pc_bounds has also the strict low < high requirement.  */
-      else if (part_die->lowpc >= part_die->highpc)
+      else if (lowpc >= highpc)
 	{
+	  struct objfile *objfile = dwarf2_per_objfile->objfile;
 	  struct gdbarch *gdbarch = get_objfile_arch (objfile);
 
 	  complaint (&symfile_complaints,
 		     _("DW_AT_low_pc %s is not < DW_AT_high_pc %s "
 		       "for DIE at 0x%x [in module %s]"),
-		     paddress (gdbarch, part_die->lowpc),
-		     paddress (gdbarch, part_die->highpc),
-		     to_underlying (part_die->sect_off),
+		     paddress (gdbarch, lowpc),
+		     paddress (gdbarch, highpc),
+		     to_underlying (sect_off),
 		     objfile_name (objfile));
 	}
       else
-	part_die->has_pc_info = 1;
+	has_pc_info = 1;
     }
 
   return info_ptr;
-- 
1.9.1

[PATCH 6/7] Move fixup_partial_die to partial_die_info::fixup

[Yao Qi]

fixup_partial_die can be a partial_die_info method fixup.

gdb:

2018-01-11  Yao Qi  <yao.qi@linaro.org>

	* dwarf2read.c (struct partial_die_info) <fixup>: New method.
	(fixup_partial_die): Remove declaration.
	(scan_partial_symbols): Update.
	(partial_die_parent_scope): Likewise.
	(partial_die_full_name): Likewise.
	(fixup_partial_die): Change it to partial_die_info::fixup.
---
 gdb/dwarf2read.c | 73 +++++++++++++++++++++++++++-----------------------------
 1 file changed, 35 insertions(+), 38 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 333a890..7823c3b 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -1410,6 +1410,11 @@ struct partial_die_info : public allocate_on_obstack
        load_partial_dies.   */
     partial_die_info& operator=(const partial_die_info& rhs) = delete;
 
+    /* Adjust the partial die before generating a symbol for it.  This
+       function may set the is_external flag or change the DIE's
+       name.  */
+    void fixup (struct dwarf2_cu *cu);
+
     /* Offset of this DIE.  */
     const sect_offset sect_off;
 
@@ -1442,7 +1447,7 @@ struct partial_die_info : public allocate_on_obstack
     /* Flag set if any of the DIE's children are template arguments.  */
     unsigned int has_template_arguments : 1;
 
-    /* Flag set if fixup_partial_die has been called on this die.  */
+    /* Flag set if fixup has been called on this die.  */
     unsigned int fixup_called : 1;
 
     /* Flag set if DW_TAG_imported_unit uses DW_FORM_GNU_ref_alt.  */
@@ -1839,9 +1844,6 @@ static const gdb_byte *read_partial_die (const struct die_reader_specs *,
 static struct partial_die_info *find_partial_die (sect_offset, int,
 						  struct dwarf2_cu *);
 
-static void fixup_partial_die (struct partial_die_info *,
-			       struct dwarf2_cu *);
-
 static const gdb_byte *read_attribute (const struct die_reader_specs *,
 				       struct attribute *, struct attr_abbrev *,
 				       const gdb_byte *);
@@ -9082,7 +9084,7 @@ scan_partial_symbols (struct partial_die_info *first_die, CORE_ADDR *lowpc,
 
   while (pdi != NULL)
     {
-      fixup_partial_die (pdi, cu);
+      pdi->fixup (cu);
 
       /* Anonymous namespaces or modules have no name but have interesting
 	 children, so we need to look at them.  Ditto for anonymous
@@ -9218,7 +9220,7 @@ partial_die_parent_scope (struct partial_die_info *pdi,
   if (parent->scope_set)
     return parent->scope;
 
-  fixup_partial_die (parent, cu);
+  parent->fixup (cu);
 
   grandparent_scope = partial_die_parent_scope (parent, cu);
 
@@ -9283,7 +9285,7 @@ partial_die_full_name (struct partial_die_info *pdi,
      types here will be reused if full symbols are loaded later.  */
   if (pdi->has_template_arguments)
     {
-      fixup_partial_die (pdi, cu);
+      pdi->fixup (cu);
 
       if (pdi->name != NULL && strchr (pdi->name, '<') == NULL)
 	{
@@ -9592,7 +9594,7 @@ add_partial_subprogram (struct partial_die_info *pdi,
       pdi = pdi->die_child;
       while (pdi != NULL)
 	{
-	  fixup_partial_die (pdi, cu);
+	  pdi->fixup (cu);
 	  if (pdi->tag == DW_TAG_subprogram
 	      || pdi->tag == DW_TAG_inlined_subroutine
 	      || pdi->tag == DW_TAG_lexical_block)
@@ -18874,45 +18876,40 @@ guess_partial_die_structure_name (struct partial_die_info *struct_pdi,
     }
 }
 
-/* Adjust PART_DIE before generating a symbol for it.  This function
-   may set the is_external flag or change the DIE's name.  */
-
-static void
-fixup_partial_die (struct partial_die_info *part_die,
-		   struct dwarf2_cu *cu)
+void
+partial_die_info::fixup (struct dwarf2_cu *cu)
 {
   /* Once we've fixed up a die, there's no point in doing so again.
      This also avoids a memory leak if we were to call
      guess_partial_die_structure_name multiple times.  */
-  if (part_die->fixup_called)
+  if (fixup_called)
     return;
 
   /* If we found a reference attribute and the DIE has no name, try
      to find a name in the referred to DIE.  */
 
-  if (part_die->name == NULL && part_die->has_specification)
+  if (name == NULL && has_specification)
     {
       struct partial_die_info *spec_die;
 
-      spec_die = find_partial_die (part_die->spec_offset,
-				   part_die->spec_is_dwz, cu);
+      spec_die = find_partial_die (spec_offset, spec_is_dwz, cu);
 
-      fixup_partial_die (spec_die, cu);
+      spec_die->fixup (cu);
 
       if (spec_die->name)
 	{
-	  part_die->name = spec_die->name;
+	  name = spec_die->name;
 
 	  /* Copy DW_AT_external attribute if it is set.  */
 	  if (spec_die->is_external)
-	    part_die->is_external = spec_die->is_external;
+	    is_external = spec_die->is_external;
 	}
     }
 
   /* Set default names for some unnamed DIEs.  */
 
-  if (part_die->name == NULL && part_die->tag == DW_TAG_namespace)
-    part_die->name = CP_ANONYMOUS_NAMESPACE_STR;
+  if (name == NULL && tag == DW_TAG_namespace)
+    name = CP_ANONYMOUS_NAMESPACE_STR;
 
   /* If there is no parent die to provide a namespace, and there are
      children, see if we can determine the namespace from their linkage
@@ -18920,25 +18917,25 @@ fixup_partial_die (struct partial_die_info *part_die,
   if (cu->language == language_cplus
       && !VEC_empty (dwarf2_section_info_def,
 		     cu->per_cu->dwarf2_per_objfile->types)
-      && part_die->die_parent == NULL
-      && part_die->has_children
-      && (part_die->tag == DW_TAG_class_type
-	  || part_die->tag == DW_TAG_structure_type
-	  || part_die->tag == DW_TAG_union_type))
-    guess_partial_die_structure_name (part_die, cu);
+      && die_parent == NULL
+      && has_children
+      && (tag == DW_TAG_class_type
+	  || tag == DW_TAG_structure_type
+	  || tag == DW_TAG_union_type))
+    guess_partial_die_structure_name (this, cu);
 
   /* GCC might emit a nameless struct or union that has a linkage
      name.  See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47510.  */
-  if (part_die->name == NULL
-      && (part_die->tag == DW_TAG_class_type
-	  || part_die->tag == DW_TAG_interface_type
-	  || part_die->tag == DW_TAG_structure_type
-	  || part_die->tag == DW_TAG_union_type)
-      && part_die->linkage_name != NULL)
+  if (name == NULL
+      && (tag == DW_TAG_class_type
+	  || tag == DW_TAG_interface_type
+	  || tag == DW_TAG_structure_type
+	  || tag == DW_TAG_union_type)
+      && linkage_name != NULL)
     {
       char *demangled;
 
-      demangled = gdb_demangle (part_die->linkage_name, DMGL_TYPES);
+      demangled = gdb_demangle (linkage_name, DMGL_TYPES);
       if (demangled)
 	{
 	  const char *base;
@@ -18952,7 +18949,7 @@ fixup_partial_die (struct partial_die_info *part_die,
 	    base = demangled;
 
 	  struct objfile *objfile = cu->per_cu->dwarf2_per_objfile->objfile;
-	  part_die->name
+	  name
 	    = ((const char *)
 	       obstack_copy0 (&objfile->per_bfd->storage_obstack,
 			      base, strlen (base)));
@@ -18960,7 +18957,7 @@ fixup_partial_die (struct partial_die_info *part_die,
 	}
     }
 
-  part_die->fixup_called = 1;
+  fixup_called = 1;
 }
 
 /* Read an attribute value described by an attribute form.  */
-- 
1.9.1

Re: [PATCH 0/7 v2] Class-fy partial_die_info

[Simon Marchi]

On 2018-02-22 10:36 AM, Yao Qi wrote:
> When I fix some issues related to dwarf, I class-fy partial_die_info as
> part of the fix.  The class-fication bits can go upstream first.
> 
> This is the v2 of this patch series, v1 can be found
> https://sourceware.org/ml/gdb-patches/2018-01/msg00505.html Changes
> in v2 are,
> 
>  - Don't move the location of partial_die_info::fixup_called, as it may
>    increase the memory usage,
>  - Add some comments to read_partial_die,
> 
> *** BLURB HERE ***
> 
> Yao Qi (7):
>   Re-write partial_die_info allocation in load_partial_dies
>   Don't check abbrev is NULL in read_partial_die
>   Change find_partial_die_in_comp_unit to dwarf2_cu::find_partial_die
>   Class-fy partial_die_info
>   Remove one argument abbrev_len in read_partial_die
>   Move fixup_partial_die to partial_die_info::fixup
>   Move read_partial_die to partial_die_info::read
> 
>  gdb/dwarf2read.c | 337 ++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 183 insertions(+), 154 deletions(-)
> 

I went through the series again, I sent a comment on patch 2,
otherwise LGTM.

Simon

Re: [PATCH 0/7 v2] Class-fy partial_die_info

[Yao Qi]

On Sun, Feb 25, 2018 at 11:52 PM, Simon Marchi <simark@simark.ca> wrote:
>
> I went through the series again, I sent a comment on patch 2,
> otherwise LGTM.
>

Thanks for the review, Simon.  Patch 2 is updated per your comment.  I
pushed them in.

-- 
Yao (齐尧)