[PATCH 0/7] Class-fy partial_die_info

[PATCH 0/7] Class-fy partial_die_info

[Yao Qi]

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

Regression tested on x86_64-linux.

*** 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 | 339 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 185 insertions(+), 154 deletions(-)

-- 
1.9.1

[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 | 108 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 57 insertions(+), 51 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 9b6f2ec..02e4431 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -1415,6 +1415,12 @@ struct partial_die_info
        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;
 
@@ -1481,8 +1487,8 @@ struct partial_die_info
 
     /* 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 *);
 
@@ -14804,7 +14805,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;
 
@@ -18316,8 +18317,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,
-				   (const gdb_byte *) info_ptr + bytes_read);
+      info_ptr = pdi.read (reader, 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
@@ -18496,24 +18497,22 @@ partial_die_info::partial_die_info (sect_offset sect_off_,
 
 /* Read a minimal amount of information into the minimal die structure.  */
 
-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
@@ -18521,7 +18520,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:
@@ -18532,12 +18531,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;
@@ -18547,16 +18550,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;
@@ -18564,7 +18567,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))
             {
@@ -18577,20 +18580,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:
@@ -18601,6 +18604,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);
 
@@ -18610,14 +18614,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
@@ -18636,25 +18640,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:
@@ -18663,7 +18667,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)
     {
@@ -18675,31 +18679,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

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

[Simon Marchi]

On 2018-01-25 04:38 AM, Yao Qi wrote:
> 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 | 108 +++++++++++++++++++++++++++++--------------------------
>  1 file changed, 57 insertions(+), 51 deletions(-)
> 
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 9b6f2ec..02e4431 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -1415,6 +1415,12 @@ struct partial_die_info
>         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;
>  
> @@ -1481,8 +1487,8 @@ struct partial_die_info
>  
>      /* 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 *);
>  
> @@ -14804,7 +14805,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;
>  
> @@ -18316,8 +18317,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,
> -				   (const gdb_byte *) info_ptr + bytes_read);
> +      info_ptr = pdi.read (reader, abbrev,
> +				 (const gdb_byte *) info_ptr + bytes_read);

Nit: can you remove the unnecessary cast while at it?

Otherwise the patch LGTM (the patches I didn't comment on also LGTM).

Simon

[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 0853282..937faa2 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
@@ -18668,15 +18670,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;
@@ -18699,7 +18701,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.
@@ -18723,7 +18725,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,
@@ -18741,7 +18743,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 96026a8..4d8958a 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -18183,7 +18183,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;
@@ -18205,8 +18204,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);
@@ -18215,15 +18212,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;
@@ -18281,7 +18271,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
@@ -18301,18 +18294,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;
 	}
 
@@ -18324,37 +18317,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;
@@ -18406,8 +18403,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 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 4d8958a..0853282 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -18470,9 +18470,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

[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 | 86 ++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 62 insertions(+), 24 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 937faa2..9b4d09e 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -1404,14 +1404,21 @@ file_entry::include_dir (const line_header *lh) const
    need this much information.  */
 struct partial_die_info
   {
+    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.  */
@@ -18273,9 +18312,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);
 
@@ -18353,7 +18392,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);
 
-      memcpy (part_die, &pdi, sizeof (pdi));
+      part_die = new (part_die) partial_die_info (pdi);
+
       /* We'll save this DIE so link it in.  */
       part_die->die_parent = parent_die;
       part_die->die_sibling = NULL;
@@ -18447,6 +18487,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 *
@@ -18466,15 +18512,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);
@@ -18674,9 +18713,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

Re: [PATCH 4/7] Class-fy partial_die_info

[Yao Qi]

Tom Tromey <tom@tromey.com> writes:

> I wonder if it would make sense to have an "operator new" implementation
> that allocates directly on an obstack.  It could use

Yes, we can have an "operator new",

> std::is_trivially_destructible to enforce the rule that objects on an
> obstack can't really be destroyed.  This would eliminate the separate
> XOBNEW, which is maybe a potential source of errors; and would also make
> it harder to accidentally add a destructor to objects allocated this way

but why dtor must be trivial?  We can have "operator new" and "operator
delete", the former allocate spaces on obstack and the latter doesn't
de-allocate space.  It doesn't matter dtor is trivial or not.  I may
miss something here.

Further, I think IWBN to have a  class which has new/delete operator,
and other classes can inherit it.  What do you think the patch below?

-- 
Yao (齐尧)
From 9f20b2690cd1c83a3bdcd41ea59f07dfef0da522 Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Fri, 26 Jan 2018 11:57:34 +0000
Subject: [PATCH] New class allocate_on_obstack

This patch adds a new class allocate_on_obstack, and let dwarf2_per_objfile
inherit it, so that dwarf2_per_objfile is automatically allocated on
obstack, and "delete dwarf2_per_objfile" doesn't de-allocate any space.

gdb:

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

	* dwarf2read.c (dwarf2_per_objfile): Inherit allocate_on_obstack.
	(dwarf2_free_objfile): Use delete.
	* gdb_obstack.h (allocate_on_obstack): New.

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 96026a8..5c45bdf 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -380,7 +380,7 @@ struct tu_stats
 /* Collection of data recorded per objfile.
    This hangs off of dwarf2_objfile_data_key.  */
 
-struct dwarf2_per_objfile
+struct dwarf2_per_objfile : public allocate_on_obstack
 {
   /* Construct a dwarf2_per_objfile for OBJFILE.  NAMES points to the
      dwarf2 section names, or is NULL if the standard ELF names are
@@ -2467,10 +2467,9 @@ dwarf2_has_info (struct objfile *objfile,
   if (dwarf2_per_objfile == NULL)
     {
       /* Initialize per-objfile state.  */
-      struct dwarf2_per_objfile *data
-	= XOBNEW (&objfile->objfile_obstack, struct dwarf2_per_objfile);
-
-      dwarf2_per_objfile = new (data) struct dwarf2_per_objfile (objfile, names);
+      dwarf2_per_objfile
+	= new (&objfile->objfile_obstack) struct dwarf2_per_objfile (objfile,
+								     names);
       set_dwarf2_per_objfile (objfile, dwarf2_per_objfile);
     }
   return (!dwarf2_per_objfile->info.is_virtual
@@ -25168,10 +25167,7 @@ dwarf2_free_objfile (struct objfile *objfile)
   struct dwarf2_per_objfile *dwarf2_per_objfile
     = get_dwarf2_per_objfile (objfile);
 
-  if (dwarf2_per_objfile == NULL)
-    return;
-
-  dwarf2_per_objfile->~dwarf2_per_objfile ();
+  delete dwarf2_per_objfile;
 }
 
 /* A set of CU "per_cu" pointer, DIE offset, and GDB type pointer.
diff --git a/gdb/gdb_obstack.h b/gdb/gdb_obstack.h
index 12a90c3..b239ce6 100644
--- a/gdb/gdb_obstack.h
+++ b/gdb/gdb_obstack.h
@@ -78,4 +78,18 @@ struct auto_obstack : obstack
   { obstack_free (this, obstack_base (this)); }
 };
 
+/* Objects are allocated on obstack instead of heap.  */
+
+struct allocate_on_obstack
+{
+  void* operator new (size_t size, struct obstack *obstack)
+  {
+    return obstack_alloc (obstack, size);
+  }
+
+  void operator delete (void* memory)
+  {
+  }
+};
+
 #endif

Re: [PATCH 4/7] Class-fy partial_die_info

[Tom Tromey]

>> std::is_trivially_destructible to enforce the rule that objects on an
>> obstack can't really be destroyed.  This would eliminate the separate
>> XOBNEW, which is maybe a potential source of errors; and would also make
>> it harder to accidentally add a destructor to objects allocated this way

Yao> but why dtor must be trivial?  We can have "operator new" and "operator
Yao> delete", the former allocate spaces on obstack and the latter doesn't
Yao> de-allocate space.  It doesn't matter dtor is trivial or not.  I may
Yao> miss something here.

My thinking was that if something is allocated on an obstack, then
presumably the destructor will never be run.  So, it's best to avoid
confusion by requiring a trivial destructor.

I suppose it would be possible to track objects and destroy them, but if
that's done, then it sort of eliminates the point of an obstack -- it
would be just as convenient at that point to use the ordinary new.

Yao> Further, I think IWBN to have a  class which has new/delete operator,
Yao> and other classes can inherit it.  What do you think the patch below?

I suppose this makes sense if you know that all objects of a given type
must be allocated on an obstack.

Tom

Re: [PATCH 4/7] Class-fy partial_die_info

[Simon Marchi]

On 2018-01-25 04:38 AM, Yao Qi wrote:
> 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,

Hi Yao,

From what I understand, the only reason to have that private constructor is
to construct a temporary partial_die_info object used to search in the htab,
is that right?  If so, converting that htab_t to an std::unordered_map would
remove the need for all this, since you don't need to construct an object
to search it.  See the diff below that applies on top of this patch.

It's not thoroughly tested and I am not sure of the validity of the
per_cu->cu->partial_dies.empty () call in find_partial_die, but I think it
should work.  Plus, it adds some type-safety, which I am a big fan of.

But otherwise, the patch is fine with me.

Simon


From dacaf6028e921efeb8c7420db7f663e5b4d7e8f1 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Sun, 28 Jan 2018 19:47:01 -0500
Subject: [PATCH] use std::unordered_map

---
 gdb/dwarf2read.c | 115 +++++++++++++------------------------------------------
 1 file changed, 26 insertions(+), 89 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 8996f49bae..330ca87a1d 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -652,6 +652,8 @@ struct delayed_method_info
   struct die_info *die;
 };

+struct partial_die_info;
+
 /* Internal state when decoding a particular compilation unit.  */
 struct dwarf2_cu
 {
@@ -686,9 +688,9 @@ struct dwarf2_cu
      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;
+  /* Hash map holding all the loaded partial DIEs
+     with their section offset as the key.  */
+  std::unordered_map<sect_offset, partial_die_info *> partial_dies;

   /* Storage for things with the same lifetime as this read-in compilation
      unit, including partial DIEs.  */
@@ -1493,36 +1495,6 @@ struct partial_die_info
     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.  */
@@ -2180,10 +2152,6 @@ static const gdb_byte *skip_one_die (const struct die_reader_specs *reader,
 				     const gdb_byte *info_ptr,
 				     struct abbrev_info *abbrev);

-static hashval_t partial_die_hash (const void *item);
-
-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 dwarf2_per_objfile *dwarf2_per_objfile);
@@ -18259,14 +18227,7 @@ load_partial_dies (const struct die_reader_specs *reader,
   if (cu->per_cu->load_all_dies)
     load_all = 1;

-  cu->partial_dies
-    = htab_create_alloc_ex (cu->header.length / 12,
-			    partial_die_hash,
-			    partial_die_eq,
-			    NULL,
-			    &cu->comp_unit_obstack,
-			    hashtab_obstack_allocate,
-			    dummy_obstack_deallocate);
+  cu->partial_dies.clear ();

   while (1)
     {
@@ -18459,14 +18420,7 @@ load_partial_dies (const struct die_reader_specs *reader,
 	  || abbrev->tag == DW_TAG_variable
 	  || abbrev->tag == DW_TAG_namespace
 	  || part_die->is_declaration)
-	{
-	  void **slot;
-
-	  slot = htab_find_slot_with_hash (cu->partial_dies, part_die,
-					   to_underlying (part_die->sect_off),
-					   INSERT);
-	  *slot = part_die;
-	}
+	cu->partial_dies[part_die->sect_off] = part_die;

       /* 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
@@ -18512,8 +18466,22 @@ 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)
-{
+  : sect_off (sect_off_), tag (abbrev->tag), has_children (abbrev->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;
 }

 /* Read a minimal amount of information into the minimal die structure.  */
@@ -18735,14 +18703,9 @@ read_partial_die (const struct die_reader_specs *reader,
 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 (sect_off);
-
-  lookup_die = ((struct partial_die_info *)
-		htab_find_with_hash (partial_dies, &part_die,
-				     to_underlying (sect_off)));
+  auto it = partial_dies.find (sect_off);

-  return lookup_die;
+  return it != partial_dies.end () ? it->second : nullptr;
 }

 /* Find a partial DIE at OFFSET, which may or may not be in CU,
@@ -18782,7 +18745,7 @@ find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
       per_cu = dwarf2_find_containing_comp_unit (sect_off, offset_in_dwz,
 						 dwarf2_per_objfile);

-      if (per_cu->cu == NULL || per_cu->cu->partial_dies == NULL)
+      if (per_cu->cu == NULL || per_cu->cu->partial_dies.empty ())
 	load_partial_comp_unit (per_cu);

       per_cu->cu->last_used = 0;
@@ -25483,32 +25446,6 @@ dwarf2_clear_marks (struct dwarf2_per_cu_data *per_cu)
     }
 }

-/* Trivial hash function for partial_die_info: the hash value of a DIE
-   is its offset in .debug_info for this objfile.  */
-
-static hashval_t
-partial_die_hash (const void *item)
-{
-  const struct partial_die_info *part_die
-    = (const struct partial_die_info *) item;
-
-  return to_underlying (part_die->sect_off);
-}
-
-/* Trivial comparison function for partial_die_info structures: two DIEs
-   are equal if they have the same offset.  */
-
-static int
-partial_die_eq (const void *item_lhs, const void *item_rhs)
-{
-  const struct partial_die_info *part_die_lhs
-    = (const struct partial_die_info *) item_lhs;
-  const struct partial_die_info *part_die_rhs
-    = (const struct partial_die_info *) item_rhs;
-
-  return part_die_lhs->sect_off == part_die_rhs->sect_off;
-}
-
 static struct cmd_list_element *set_dwarf_cmdlist;
 static struct cmd_list_element *show_dwarf_cmdlist;

-- 
2.16.1

Re: [PATCH 4/7] Class-fy partial_die_info

[Yao Qi]

Simon Marchi <simark@simark.ca> writes:

> From what I understand, the only reason to have that private constructor is
> to construct a temporary partial_die_info object used to search in the htab,
> is that right?  If so, converting that htab_t to an std::unordered_map would

Right.

>
> -  /* Hash table holding all the loaded partial DIEs
> -     with partial_die->offset.SECT_OFF as hash.  */
> -  htab_t partial_dies = nullptr;
> +  /* Hash map holding all the loaded partial DIEs
> +     with their section offset as the key.  */
> +  std::unordered_map<sect_offset, partial_die_info *> partial_dies;
>

This doesn't compile with my g++ 4.9, as library doesn't provide
std::hash<T> specialization for enumeration types.  It is available
since C++ 14.  http://en.cppreference.com/w/cpp/utility/hash

I can change it to

std::unordered_map<std::underlying_type<sect_offset>::type,
		     partial_die_info *> partial_dies;

to fix the compiler errors.

> @@ -18259,14 +18227,7 @@ load_partial_dies (const struct die_reader_specs *reader,
>    if (cu->per_cu->load_all_dies)
>      load_all = 1;
>
> -  cu->partial_dies
> -    = htab_create_alloc_ex (cu->header.length / 12,
> -			    partial_die_hash,
> -			    partial_die_eq,
> -			    NULL,
> -			    &cu->comp_unit_obstack,
> -			    hashtab_obstack_allocate,
> -			    dummy_obstack_deallocate);
> +  cu->partial_dies.clear ();

This changes the behavior, without this patch, cu->partial_dies's
elements are allocated on cu->comp_unit_obstack.  After this patch,
elements are allocated on heap.  I don't know how does it affect
performance.

-- 
Yao (齐尧)

Re: [PATCH 4/7] Class-fy partial_die_info

[Pedro Alves]

On 01/30/2018 10:49 AM, Yao Qi wrote:
> Simon Marchi <simark@simark.ca> writes:

>> -  /* Hash table holding all the loaded partial DIEs
>> -     with partial_die->offset.SECT_OFF as hash.  */
>> -  htab_t partial_dies = nullptr;
>> +  /* Hash map holding all the loaded partial DIEs
>> +     with their section offset as the key.  */
>> +  std::unordered_map<sect_offset, partial_die_info *> partial_dies;
>>
> This doesn't compile with my g++ 4.9, as library doesn't provide
> std::hash<T> specialization for enumeration types.  It is available
> since C++ 14.  http://en.cppreference.com/w/cpp/utility/hash
> 
> I can change it to
> 
> std::unordered_map<std::underlying_type<sect_offset>::type,
> 		     partial_die_info *> partial_dies;
> 
> to fix the compiler errors.

Note: in cases like these, we don't need to forgo using the
(strong) enum as key type.  unordered_map's third (defaulted) template
parameter type is the hasher to use.  And we have a convenience
hasher for this:

 [pushed] Add gdb::hash_enum
 https://sourceware.org/ml/gdb-patches/2017-12/msg00210.html

Currently used in dwarf2read.c:

  std::unordered_map<sect_offset,
                     dwarf2_per_cu_data *,
                     gdb::hash_enum<sect_offset>>

Thanks,
Pedro Alves

Re: [PATCH 4/7] Class-fy partial_die_info

[Pedro Alves]

On 01/29/2018 01:15 AM, Simon Marchi wrote:

> From what I understand, the only reason to have that private constructor is
> to construct a temporary partial_die_info object used to search in the htab,
> is that right?  If so, converting that htab_t to an std::unordered_map would
> remove the need for all this, since you don't need to construct an object
> to search it.  See the diff below that applies on top of this patch.
> 
> It's not thoroughly tested and I am not sure of the validity of the
> per_cu->cu->partial_dies.empty () call in find_partial_die, but I think it
> should work.  Plus, it adds some type-safety, which I am a big fan of.
> 
> But otherwise, the patch is fine with me.

Careful here.  This could do with some benchmarking.  The DWARF reading code
is performance (both timing and memory) sensitive.  This is trading an open
addressing hash table (htab_t), for a node-based closed addressing hash table.
The keys/elements in the map are small so I'd expect this to make
a difference.  Also, this is trading a in-principle cache-friendly
obstack allocation scheme for the standard new allocator.

Thanks,
Pedro Alves

Re: [PATCH 4/7] Class-fy partial_die_info

[Simon Marchi]

On 2018-01-30 06:39, Pedro Alves wrote:
> On 01/29/2018 01:15 AM, Simon Marchi wrote:
> 
>> From what I understand, the only reason to have that private 
>> constructor is
>> to construct a temporary partial_die_info object used to search in the 
>> htab,
>> is that right?  If so, converting that htab_t to an std::unordered_map 
>> would
>> remove the need for all this, since you don't need to construct an 
>> object
>> to search it.  See the diff below that applies on top of this patch.
>> 
>> It's not thoroughly tested and I am not sure of the validity of the
>> per_cu->cu->partial_dies.empty () call in find_partial_die, but I 
>> think it
>> should work.  Plus, it adds some type-safety, which I am a big fan of.
>> 
>> But otherwise, the patch is fine with me.
> 
> Careful here.  This could do with some benchmarking.  The DWARF reading 
> code
> is performance (both timing and memory) sensitive.  This is trading an 
> open
> addressing hash table (htab_t), for a node-based closed addressing hash 
> table.
> The keys/elements in the map are small so I'd expect this to make
> a difference.  Also, this is trading a in-principle cache-friendly
> obstack allocation scheme for the standard new allocator.

Ah, indeed.  I thought that unordered_map would be implemented the same 
way as htab_t, but I see it's not the case.  Doing some quick tests on a 
big binary, it increases the time reading the symbols from an average of 
37 seconds to an average of 42 seconds.

I understand the different hash table implementation having an impact, 
but I don't really understand how the allocation scheme can have a 
meaningful impact.  The partial_die_info objects are still allocated on 
the obstack, aren't they?  So it's just the space for the table itself 
that isn't on the objstack, but I don't see why that would make a 
difference.

If we want to have a data structure with the same kind of performance as 
htab_t but with type-safety in the future, is your vision that we'll 
have to implement it ourselves?  Should we make a wrapper around htab_t?

Simon

Re: [PATCH 4/7] Class-fy partial_die_info

[Yao Qi]

Simon Marchi <simark@simark.ca> writes:

> Ah, indeed.  I thought that unordered_map would be implemented the
> same way as htab_t, but I see it's not the case.  Doing some quick
> tests on a big binary, it increases the time reading the symbols from
> an average of 37 seconds to an average of 42 seconds.
>
> I understand the different hash table implementation having an impact,
> but I don't really understand how the allocation scheme can have a
> meaningful impact.  The partial_die_info objects are still allocated
> on the obstack, aren't they?  So it's just the space for the table
> itself that isn't on the objstack, but I don't see why that would make
> a difference.

Hi Simon,
We have some perf test cases, in gdb.perf, but they may not cover the
path we are discussing here.  If you want to run them, do these things,

$ cd gdb/testsuite
$ make -j10 build-perf RUNTESTFLAGS="MONSTER=y gmonster1.exp"

// this takes a while to generate many executable files,

$ make  check-perf RUNTESTFLAGS="MONSTER=y gmonster1-null-lookup.exp gmonster1-print-cerr.exp gmonster1-runto-main.exp gmonster1-pervasive-typedef.exp gmonster1-ptype-string.exp gmonster1-select-file.exp"  GDB_PERFTEST_MODE=run

// it takes one hour on my aarch64-linux box

You can get the performance number in perftest.sum.

I run these perf tests, with and without Simon's patch (htab_t ->
std::unordered_map), on aarch64-linux, I don't see speed and space
change.  Again, these existing test cases may not cover the path.

gmonster1:gmonster-null-lookup cpu_time 10-cus 0.0008508 0.0013708
gmonster1:gmonster-null-lookup cpu_time 100-cus 0.0047922 0.0030584
gmonster1:gmonster-null-lookup cpu_time 1000-cus 0.0400274 0.0397188
gmonster1:gmonster-null-lookup cpu_time 10000-cus 0.3885292 0.3862456
gmonster1:gmonster-null-lookup wall_time 10-cus 0.000862598419189 0.00137987136841
gmonster1:gmonster-null-lookup wall_time 100-cus 0.00480117797852 0.00306539535522
gmonster1:gmonster-null-lookup wall_time 1000-cus 0.0400432109833 0.0397333621979
gmonster1:gmonster-null-lookup wall_time 10000-cus 0.388552331924 0.386282110214
gmonster1:gmonster-null-lookup vmsize 10-cus 35900 35896
gmonster1:gmonster-null-lookup vmsize 100-cus 64992 64972
gmonster1:gmonster-null-lookup vmsize 1000-cus 364748 364760
gmonster1:gmonster-null-lookup vmsize 10000-cus 3313284 3313360
gmonster1:gmonster-pervasive-typedef cpu_time 10-cus 0.0682848 0.0737696
gmonster1:gmonster-pervasive-typedef cpu_time 100-cus 0.5843324 0.6391266
gmonster1:gmonster-pervasive-typedef cpu_time 1000-cus 6.061932 6.621443
gmonster1:gmonster-pervasive-typedef cpu_time 10000-cus 62.6619226 67.1514486
gmonster1:gmonster-pervasive-typedef wall_time 10-cus 0.0683585643768 0.0737894058228
gmonster1:gmonster-pervasive-typedef wall_time 100-cus 0.585014867783 0.641395568848
gmonster1:gmonster-pervasive-typedef wall_time 1000-cus 6.07634234428 6.62292981148
gmonster1:gmonster-pervasive-typedef wall_time 10000-cus 62.6738821507 67.2341232777
gmonster1:gmonster-pervasive-typedef vmsize 10-cus 32381 32368
gmonster1:gmonster-pervasive-typedef vmsize 100-cus 41131 41084
gmonster1:gmonster-pervasive-typedef vmsize 1000-cus 129726 129553
gmonster1:gmonster-pervasive-typedef vmsize 10000-cus 1007898 1007878
gmonster1:gmonster-print-cerr cpu_time 10-cus 0.0011148 0.0011168
gmonster1:gmonster-print-cerr cpu_time 100-cus 0.0056498 0.0056002
gmonster1:gmonster-print-cerr cpu_time 1000-cus 0.0502508 0.0948982
gmonster1:gmonster-print-cerr cpu_time 10000-cus 0.4922956 0.4948586
gmonster1:gmonster-print-cerr wall_time 10-cus 0.00112357139587 0.00112380981445
gmonster1:gmonster-print-cerr wall_time 100-cus 0.00565934181213 0.00561075210571
gmonster1:gmonster-print-cerr wall_time 1000-cus 0.0502710342407 0.0949506282806
gmonster1:gmonster-print-cerr wall_time 10000-cus 0.492320823669 0.494928407669
gmonster1:gmonster-print-cerr vmsize 10-cus 32508 32500
gmonster1:gmonster-print-cerr vmsize 100-cus 41308 41300
gmonster1:gmonster-print-cerr vmsize 1000-cus 128944 128948
gmonster1:gmonster-print-cerr vmsize 10000-cus 993324 993316
gmonster1:gmonster-runto-main cpu_time 10-cus 0.0360472 0.0231514
gmonster1:gmonster-runto-main cpu_time 100-cus 0.5829484 0.5236148
gmonster1:gmonster-runto-main cpu_time 1000-cus 9.146062 7.9914552
gmonster1:gmonster-runto-main cpu_time 10000-cus 134.266728 134.1361918
gmonster1:gmonster-runto-main wall_time 10-cus 0.042032957077 0.0313341617584
gmonster1:gmonster-runto-main wall_time 100-cus 0.588344860077 0.530299949646
gmonster1:gmonster-runto-main wall_time 1000-cus 9.15567464828 7.99887242317
gmonster1:gmonster-runto-main wall_time 10000-cus 134.285585785 134.15385747
gmonster1:gmonster-runto-main vmsize 10-cus 32503 32496
gmonster1:gmonster-runto-main vmsize 100-cus 41308 41296
gmonster1:gmonster-runto-main vmsize 1000-cus 128960 128952
gmonster1:gmonster-runto-main vmsize 10000-cus 993336 993296
gmonster1:gmonster-select-file cpu_time 10-cus 0.0972798 0.1028992
gmonster1:gmonster-select-file cpu_time 100-cus 0.886951 0.9285964
gmonster1:gmonster-select-file cpu_time 1000-cus 9.166904 9.7175802
gmonster1:gmonster-select-file cpu_time 10000-cus 92.5872488 95.2634352
gmonster1:gmonster-select-file wall_time 10-cus 0.097380399704 0.103002214432
gmonster1:gmonster-select-file wall_time 100-cus 0.887106800079 0.929784965515
gmonster1:gmonster-select-file wall_time 1000-cus 9.17858901024 9.72046675682
gmonster1:gmonster-select-file wall_time 10000-cus 92.6326120853 95.3189713955
gmonster1:gmonster-select-file vmsize 10-cus 32367 32384
gmonster1:gmonster-select-file vmsize 100-cus 41064 41056
gmonster1:gmonster-select-file vmsize 1000-cus 128694 128703
gmonster1:gmonster-select-file vmsize 10000-cus 993052 993048

-- 
Yao (齐尧)

Re: [PATCH 4/7] Class-fy partial_die_info

[Pedro Alves]

On 01/31/2018 03:46 AM, Simon Marchi wrote:
> On 2018-01-30 06:39, Pedro Alves wrote:
>> On 01/29/2018 01:15 AM, Simon Marchi wrote:
>>
>>> From what I understand, the only reason to have that private constructor is
>>> to construct a temporary partial_die_info object used to search in the htab,
>>> is that right?  If so, converting that htab_t to an std::unordered_map would
>>> remove the need for all this, since you don't need to construct an object
>>> to search it.  See the diff below that applies on top of this patch.
>>>
>>> It's not thoroughly tested and I am not sure of the validity of the
>>> per_cu->cu->partial_dies.empty () call in find_partial_die, but I think it
>>> should work.  Plus, it adds some type-safety, which I am a big fan of.
>>>
>>> But otherwise, the patch is fine with me.
>>
>> Careful here.  This could do with some benchmarking.  The DWARF reading code
>> is performance (both timing and memory) sensitive.  This is trading an open
>> addressing hash table (htab_t), for a node-based closed addressing hash table.
>> The keys/elements in the map are small so I'd expect this to make
>> a difference.  Also, this is trading a in-principle cache-friendly
>> obstack allocation scheme for the standard new allocator.
> 
> Ah, indeed.  I thought that unordered_map would be implemented the same way as htab_t, but I see it's not the case.  Doing some quick tests on a big binary, it increases the time reading the symbols from an average of 37 seconds to an average of 42 seconds.
> 
> I understand the different hash table implementation having an impact, but I don't really understand how the allocation scheme can have a meaningful impact.  The partial_die_info objects are still allocated on the obstack, aren't they?  So it's just the space for the table itself that isn't on the objstack, but I don't see why that would make a difference.

Well, that's the thing.  unordered_map is going to reach to the heap to
allocate the buckets and nodes.  And the heap is slow.  And there's a
memory size cost for each element too, along with that reducing
cache locality.

unordered_map is really inefficient for this use case.  We have a hash map
of small objects that is filled in once, and after initial fill, we don't
do random insert/remove of elements from this map.  I think.

> 
> If we want to have a data structure with the same kind of performance as htab_t but with type-safety in the future, is your vision that we'll have to implement it ourselves?  Should we make a wrapper around htab_t?

Well, my vision is to investigate alternatives.  :-)  There are many options
out there.  If you google around for hash table benchmarks you'll find several.
E.g.: <https://tessil.github.io/2016/08/29/benchmark-hopscotch-map.html>.

So there are good alternatives out there that we could import and use.
AFAIK, libiberty's htab_t is actually quite good, and GCC has a C++-ified
version of that.  See gcc/hash-table.h, gcc/hash-map.h, gcc/hash-set.h:

 /* This file implements a typed hash table.
    The implementation borrows from libiberty's htab_t in hashtab.h.

So sharing that code with GCC may make sense, as it's already in
"the family".

One thing I dislike about GCC's hash containers above is that
their API isn't modeled on the standard's, which can lead to
confusion if you're used to the standard containers, or if you're
experimenting/benchmarking different containers for some
use case.  You have things like:

  /* This function clears all entries in this hash table.  */
   void empty () { if (elements ()) empty_slow (); }

while in the std containers, empty() is a predicate...

It may be that GCC's hash/map/set could do with C++11-fication too.
Really I haven't investigated much.  I'm just aware of their
existence.

(A while ago I found reading this discussion quite educational:
  <https://bugs.ruby-lang.org/issues/12142>)

Thanks,
Pedro Alves

[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 | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 9b4d09e..0ee102a 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,
@@ -18315,8 +18314,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
@@ -18498,8 +18497,7 @@ partial_die_info::partial_die_info (sect_offset sect_off_,
 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
@@ -18512,8 +18510,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

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

[Simon Marchi]

On 2018-01-25 04:38 AM, Yao Qi wrote:
> 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 | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 9b4d09e..0ee102a 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,
> @@ -18315,8 +18314,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
> @@ -18498,8 +18497,7 @@ partial_die_info::partial_die_info (sect_offset sect_off_,
>  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
> @@ -18512,8 +18510,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);
> 

I didn't see the value of this change at first, but it makes the meaning of info_ptr
the same in read_partial_die as in skip_one_die, so I think it's good.  Could you
improve the documentation of read_partial_die to clarify that INFO_PTR points
after the abbrev code?  The skip_one_die doc already specifies that, so you could
just steal it from there.

Simon

[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, and field
fixup_called can be private.

gdb:

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

	* dwarf2read.c (struct partial_die_info) <fixup>: New method.
	(struct partial_die_info) <fixup_called>: Change it to
	m_fixup_called.
	(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 | 79 +++++++++++++++++++++++++++-----------------------------
 1 file changed, 38 insertions(+), 41 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 0ee102a..9b6f2ec 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -1410,6 +1410,11 @@ struct partial_die_info
        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,9 +1447,6 @@ struct partial_die_info
     /* 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.  */
-    unsigned int fixup_called : 1;
-
     /* Flag set if DW_TAG_imported_unit uses DW_FORM_GNU_ref_alt.  */
     unsigned int is_dwz : 1;
 
@@ -1519,10 +1521,13 @@ struct partial_die_info
       has_byte_size = 0;
       has_const_value = 0;
       has_template_arguments = 0;
-      fixup_called = 0;
+      m_fixup_called = false;
       is_dwz = 0;
       spec_is_dwz = 0;
     }
+
+    /* Flag set if fixup has been called on this die.  */
+    bool m_fixup_called : 1;
   };
 
 /* This data structure holds the information of an abbrev.  */
@@ -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 *);
@@ -9025,7 +9027,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
@@ -9161,7 +9163,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);
 
@@ -9226,7 +9228,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)
 	{
@@ -9535,7 +9537,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)
@@ -18842,45 +18844,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 (m_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
@@ -18888,25 +18885,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;
@@ -18920,7 +18917,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)));
@@ -18928,7 +18925,7 @@ fixup_partial_die (struct partial_die_info *part_die,
 	}
     }
 
-  part_die->fixup_called = 1;
+  m_fixup_called = true;
 }
 
 /* Read an attribute value described by an attribute form.  */
-- 
1.9.1

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

[Pedro Alves]

On 01/25/2018 09:38 AM, Yao Qi wrote:

> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 0ee102a..9b6f2ec 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -1410,6 +1410,11 @@ struct partial_die_info
>         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,9 +1447,6 @@ struct partial_die_info
>      /* 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.  */
> -    unsigned int fixup_called : 1;
> -
>      /* Flag set if DW_TAG_imported_unit uses DW_FORM_GNU_ref_alt.  */
>      unsigned int is_dwz : 1;
>  
> @@ -1519,10 +1521,13 @@ struct partial_die_info
>        has_byte_size = 0;
>        has_const_value = 0;
>        has_template_arguments = 0;
> -      fixup_called = 0;
> +      m_fixup_called = false;
>        is_dwz = 0;
>        spec_is_dwz = 0;
>      }
> +
> +    /* Flag set if fixup has been called on this die.  */
> +    bool m_fixup_called : 1;
>    };
Currently, the boolean bits were all together for packing.
This moves the bit away from the others, after some pointer
fields, I think.  This is probably growing the size of
the structure with some padding holes.

Thanks,
Pedro Alves

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

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Currently, the boolean bits were all together for packing.
> This moves the bit away from the others, after some pointer
> fields, I think.  This is probably growing the size of
> the structure with some padding holes.

OK, I discard the chunk of changing fixup_called to private field.
Still keep it public.

-- 
Yao (齐尧)
From bcd3aae658a748bb662fcdebd173ee189d0eaef8 Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Thu, 11 Jan 2018 11:45:44 +0000
Subject: [PATCH] Move fixup_partial_die to partial_die_info::fixup

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.

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 0ee102a..087588a 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -1410,6 +1410,11 @@ struct partial_die_info
        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
     /* 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 *);
@@ -9025,7 +9027,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
@@ -9161,7 +9163,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);
 
@@ -9226,7 +9228,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)
 	{
@@ -9535,7 +9537,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)
@@ -18842,45 +18844,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
@@ -18888,25 +18885,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;
@@ -18920,7 +18917,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)));
@@ -18928,7 +18925,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.  */

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

[Joel Brobecker]

Hi Yao,

On Thu, Jan 25, 2018 at 09:38:27AM +0000, Yao Qi wrote:
> When I fix some issue related to dwarf, I class-fy partial_die_info as
> part of the fix.  The class-fy bits can go upstream first.
> 
> Regression tested on x86_64-linux.
> 
> *** 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 | 339 ++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 185 insertions(+), 154 deletions(-)

What's the advantage of turning this struct into a class?
Is it memory management?

(sorry if it's obvious from the code - C++ dummy, here)

-- 
Joel

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

[Yao Qi]

On Thu, Jan 25, 2018 at 12:05 PM, Joel Brobecker <brobecker@adacore.com> wrote:
>
> What's the advantage of turning this struct into a class?
> Is it memory management?
>
> (sorry if it's obvious from the code - C++ dummy, here)
>

I am fixing PR 22531, that is GDB should coalesce consecutive line
table entries in some cases.  However, one exception here is that
GCC emits two entries to mark the end of prologue.  In this case, GDB
can't coalesce them.

I wrote some unit tests to dwarf_decode_lines_1,
with an input to provide dwarf line table, and verify dwarf_decode_lines_1
can get line table correctly.  In order to write unit tests, I want to easily
create the context for each unit test, like dwarf2_per_cu_data, dwarf2_cu,
line_header, etc.  So I have to class-fy them, and class-fy these structs
requires class-fy other structs, it turns out that I nearly class-fy all structs
in dwarf2read.c.  This patch series is just part of the whole work.

-- 
Yao (齐尧)