[1/4] Remove some explicit memory management from dwarf2read.c

Message ID 20200107220754.24796-2-tromey@adacore.com
State New
Headers show
Series
  • Minor DWARF memory management cleanups
Related show

Commit Message

Tom Tromey Jan. 7, 2020, 10:07 p.m.
I noticed a few spots in dwarf2read.c that could be improved by moving
to unique_xmalloc_ptr or, in one case, std::vector.

gdb/ChangeLog
2020-01-07  Tom Tromey  <tromey@adacore.com>

	* dwarf2read.c (add_partial_symbol): Use unique_xmalloc_ptr.
	(dwarf2_compute_name, open_dwo_file): Likewise.
	(process_enumeration_scope): Use std::vector.
	(guess_partial_die_structure_name): Use unique_xmalloc_ptr.
	(partial_die_info::fixup, dwarf2_start_subfile)
	(guess_full_die_structure_name, dwarf2_name): Likewise.
	(determine_prefix): Update.
	(guess_full_die_structure_name): Make return type const.

Change-Id: I1cb278c608041ef36ef1f77c7e7565c921038d08
---
 gdb/ChangeLog    |  11 ++++
 gdb/dwarf2read.c | 131 ++++++++++++++++++-----------------------------
 2 files changed, 62 insertions(+), 80 deletions(-)

-- 
2.21.0

Comments

Simon Marchi Jan. 8, 2020, 3:44 a.m. | #1
On 2020-01-07 5:07 p.m., Tom Tromey wrote:
> I noticed a few spots in dwarf2read.c that could be improved by moving

> to unique_xmalloc_ptr or, in one case, std::vector.

> 

> gdb/ChangeLog

> 2020-01-07  Tom Tromey  <tromey@adacore.com>

> 

> 	* dwarf2read.c (add_partial_symbol): Use unique_xmalloc_ptr.

> 	(dwarf2_compute_name, open_dwo_file): Likewise.

> 	(process_enumeration_scope): Use std::vector.

> 	(guess_partial_die_structure_name): Use unique_xmalloc_ptr.

> 	(partial_die_info::fixup, dwarf2_start_subfile)

> 	(guess_full_die_structure_name, dwarf2_name): Likewise.

> 	(determine_prefix): Update.

> 	(guess_full_die_structure_name): Make return type const.

> 

> Change-Id: I1cb278c608041ef36ef1f77c7e7565c921038d08


Thanks, this LGTM.  I noted some suggestions below.

> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c

> index 63d533a4459..9fd2b7715b1 100644

> --- a/gdb/dwarf2read.c

> +++ b/gdb/dwarf2read.c

> @@ -8954,13 +8954,13 @@ add_partial_symbol (struct partial_die_info *pdi, struct dwarf2_cu *cu)

>    CORE_ADDR addr = 0;

>    const char *actual_name = NULL;

>    CORE_ADDR baseaddr;

> -  char *built_actual_name;

>  

>    baseaddr = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));

>  

> -  built_actual_name = partial_die_full_name (pdi, cu);

> +  gdb::unique_xmalloc_ptr<char> built_actual_name

> +    (partial_die_full_name (pdi, cu));


You could make partial_die_full_name return a gdb::unique_xmalloc_ptr while at
it, since this is the sole user.  This can then stay an assignment:

  gdb::unique_xmalloc_ptr<char> built_actual_name
    = partial_die_full_name (pdi, cu);

> @@ -16566,34 +16555,26 @@ process_enumeration_scope (struct die_info *die, struct dwarf2_cu *cu)

>  		{

>  		  sym = new_symbol (child_die, this_type, cu);

>  

> -		  if ((num_fields % DW_FIELD_ALLOC_CHUNK) == 0)

> -		    {

> -		      fields = (struct field *)

> -			xrealloc (fields,

> -				  (num_fields + DW_FIELD_ALLOC_CHUNK)

> -				  * sizeof (struct field));

> -		    }


You can actually delete DW_FIELD_ALLOC_CHUNK, defined higher in dwarf2read.c.

Simon
Tom Tromey Jan. 8, 2020, 6:11 p.m. | #2
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:


Simon> You could make partial_die_full_name return a gdb::unique_xmalloc_ptr while at
Simon> it, since this is the sole user.  This can then stay an assignment:

Simon>   gdb::unique_xmalloc_ptr<char> built_actual_name
Simon>     = partial_die_full_name (pdi, cu);

...

Simon> You can actually delete DW_FIELD_ALLOC_CHUNK, defined higher in dwarf2read.c.

Thanks for the review.  I made these changes.

Tom

Patch

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 63d533a4459..9fd2b7715b1 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -8954,13 +8954,13 @@  add_partial_symbol (struct partial_die_info *pdi, struct dwarf2_cu *cu)
   CORE_ADDR addr = 0;
   const char *actual_name = NULL;
   CORE_ADDR baseaddr;
-  char *built_actual_name;
 
   baseaddr = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
 
-  built_actual_name = partial_die_full_name (pdi, cu);
+  gdb::unique_xmalloc_ptr<char> built_actual_name
+    (partial_die_full_name (pdi, cu));
   if (built_actual_name != NULL)
-    actual_name = built_actual_name;
+    actual_name = built_actual_name.get ();
 
   if (actual_name == NULL)
     actual_name = pdi->name;
@@ -9053,10 +9053,7 @@  add_partial_symbol (struct partial_die_info *pdi, struct dwarf2_cu *cu)
 	  /* Static Variable.  Skip symbols whose value we cannot know (those
 	     without location descriptors or constant values).  */
 	  if (!has_loc && !pdi->has_const_value)
-	    {
-	      xfree (built_actual_name);
-	      return;
-	    }
+	    return;
 
 	  add_psymbol_to_list (actual_name,
 			       built_actual_name != NULL,
@@ -9106,10 +9103,7 @@  add_partial_symbol (struct partial_die_info *pdi, struct dwarf2_cu *cu)
          union or class entry that does not have a byte size attribute
          and that has a DW_AT_declaration attribute."  */
       if (!pdi->has_byte_size && pdi->is_declaration)
-	{
-	  xfree (built_actual_name);
-	  return;
-	}
+	return;
 
       /* NOTE: carlton/2003-10-07: See comment in new_symbol about
 	 static vs. global.  */
@@ -9134,8 +9128,6 @@  add_partial_symbol (struct partial_die_info *pdi, struct dwarf2_cu *cu)
     default:
       break;
     }
-
-  xfree (built_actual_name);
 }
 
 /* Read a partial die corresponding to a namespace; also, add a symbol
@@ -10912,11 +10904,10 @@  dwarf2_compute_name (const char *name,
 	  prefix = determine_prefix (die, cu);
 	  if (*prefix != '\0')
 	    {
-	      char *prefixed_name = typename_concat (NULL, prefix, name,
-						     physname, cu);
+	      gdb::unique_xmalloc_ptr<char> prefixed_name
+		(typename_concat (NULL, prefix, name, physname, cu));
 
-	      buf.puts (prefixed_name);
-	      xfree (prefixed_name);
+	      buf.puts (prefixed_name.get ());
 	    }
 	  else
 	    buf.puts (name);
@@ -12958,16 +12949,15 @@  open_dwo_file (struct dwarf2_per_objfile *dwarf2_per_objfile,
 
   if (comp_dir != NULL)
     {
-      char *path_to_try = concat (comp_dir, SLASH_STRING,
-				  file_name, (char *) NULL);
+      gdb::unique_xmalloc_ptr<char> path_to_try
+	(concat (comp_dir, SLASH_STRING, file_name, (char *) NULL));
 
       /* NOTE: If comp_dir is a relative path, this will also try the
 	 search path, which seems useful.  */
       gdb_bfd_ref_ptr abfd (try_open_dwop_file (dwarf2_per_objfile,
-						path_to_try,
+						path_to_try.get (),
 						0 /*is_dwp*/,
 						1 /*search_cwd*/));
-      xfree (path_to_try);
       if (abfd != NULL)
 	return abfd;
     }
@@ -16548,8 +16538,7 @@  process_enumeration_scope (struct die_info *die, struct dwarf2_cu *cu)
     {
       struct die_info *child_die;
       struct symbol *sym;
-      struct field *fields = NULL;
-      int num_fields = 0;
+      std::vector<struct field> fields;
       const char *name;
 
       child_die = die->child;
@@ -16566,34 +16555,26 @@  process_enumeration_scope (struct die_info *die, struct dwarf2_cu *cu)
 		{
 		  sym = new_symbol (child_die, this_type, cu);
 
-		  if ((num_fields % DW_FIELD_ALLOC_CHUNK) == 0)
-		    {
-		      fields = (struct field *)
-			xrealloc (fields,
-				  (num_fields + DW_FIELD_ALLOC_CHUNK)
-				  * sizeof (struct field));
-		    }
-
-		  FIELD_NAME (fields[num_fields]) = sym->linkage_name ();
-		  FIELD_TYPE (fields[num_fields]) = NULL;
-		  SET_FIELD_ENUMVAL (fields[num_fields], SYMBOL_VALUE (sym));
-		  FIELD_BITSIZE (fields[num_fields]) = 0;
+		  fields.emplace_back ();
+		  struct field &field = fields.back ();
 
-		  num_fields++;
+		  FIELD_NAME (field) = sym->linkage_name ();
+		  FIELD_TYPE (field) = NULL;
+		  SET_FIELD_ENUMVAL (field, SYMBOL_VALUE (sym));
+		  FIELD_BITSIZE (field) = 0;
 		}
 	    }
 
 	  child_die = sibling_die (child_die);
 	}
 
-      if (num_fields)
+      if (!fields.empty ())
 	{
-	  TYPE_NFIELDS (this_type) = num_fields;
+	  TYPE_NFIELDS (this_type) = fields.size ();
 	  TYPE_FIELDS (this_type) = (struct field *)
-	    TYPE_ALLOC (this_type, sizeof (struct field) * num_fields);
-	  memcpy (TYPE_FIELDS (this_type), fields,
-		  sizeof (struct field) * num_fields);
-	  xfree (fields);
+	    TYPE_ALLOC (this_type, sizeof (struct field) * fields.size ());
+	  memcpy (TYPE_FIELDS (this_type), fields.data (),
+		  sizeof (struct field) * fields.size ());
 	}
     }
 
@@ -19345,16 +19326,15 @@  guess_partial_die_structure_name (struct partial_die_info *struct_pdi,
       if (child_pdi->tag == DW_TAG_subprogram
 	  && child_pdi->linkage_name != NULL)
 	{
-	  char *actual_class_name
-	    = language_class_name_from_physname (cu->language_defn,
-						 child_pdi->linkage_name);
+	  gdb::unique_xmalloc_ptr<char> actual_class_name
+	    (language_class_name_from_physname (cu->language_defn,
+						child_pdi->linkage_name));
 	  if (actual_class_name != NULL)
 	    {
 	      struct objfile *objfile = cu->per_cu->dwarf2_per_objfile->objfile;
 	      struct_pdi->name
 		= obstack_strdup (&objfile->per_bfd->storage_obstack,
-				  actual_class_name);
-	      xfree (actual_class_name);
+				  actual_class_name.get ());
 	    }
 	  break;
 	}
@@ -19419,24 +19399,22 @@  partial_die_info::fixup (struct dwarf2_cu *cu)
 	  || tag == DW_TAG_union_type)
       && linkage_name != NULL)
     {
-      char *demangled;
-
-      demangled = gdb_demangle (linkage_name, DMGL_TYPES);
-      if (demangled)
+      gdb::unique_xmalloc_ptr<char> demangled
+	(gdb_demangle (linkage_name, DMGL_TYPES));
+      if (demangled != nullptr)
 	{
 	  const char *base;
 
 	  /* Strip any leading namespaces/classes, keep only the base name.
 	     DW_AT_name for named DIEs does not contain the prefixes.  */
-	  base = strrchr (demangled, ':');
-	  if (base && base > demangled && base[-1] == ':')
+	  base = strrchr (demangled.get (), ':');
+	  if (base && base > demangled.get () && base[-1] == ':')
 	    base++;
 	  else
-	    base = demangled;
+	    base = demangled.get ();
 
 	  struct objfile *objfile = cu->per_cu->dwarf2_per_objfile->objfile;
 	  name = obstack_strdup (&objfile->per_bfd->storage_obstack, base);
-	  xfree (demangled);
 	}
     }
 
@@ -21716,7 +21694,7 @@  static void
 dwarf2_start_subfile (struct dwarf2_cu *cu, const char *filename,
 		      const char *dirname)
 {
-  char *copy = NULL;
+  gdb::unique_xmalloc_ptr<char> copy;
 
   /* In order not to lose the line information directory,
      we concatenate it to the filename when it makes sense.
@@ -21727,14 +21705,11 @@  dwarf2_start_subfile (struct dwarf2_cu *cu, const char *filename,
 
   if (!IS_ABSOLUTE_PATH (filename) && dirname != NULL)
     {
-      copy = concat (dirname, SLASH_STRING, filename, (char *)NULL);
-      filename = copy;
+      copy.reset (concat (dirname, SLASH_STRING, filename, (char *) NULL));
+      filename = copy.get ();
     }
 
   cu->get_builder ()->start_subfile (filename);
-
-  if (copy != NULL)
-    xfree (copy);
 }
 
 /* Start a symtab for DWARF.  NAME, COMP_DIR, LOW_PC are passed to the
@@ -22702,7 +22677,7 @@  read_type_die_1 (struct die_info *die, struct dwarf2_cu *cu)
    This is the full-die version of guess_partial_die_structure_name.
    In this case we know DIE has no useful parent.  */
 
-static char *
+static const char *
 guess_full_die_structure_name (struct die_info *die, struct dwarf2_cu *cu)
 {
   struct die_info *spec_die;
@@ -22728,33 +22703,32 @@  guess_full_die_structure_name (struct die_info *die, struct dwarf2_cu *cu)
 
 	  if (linkage_name != NULL)
 	    {
-	      char *actual_name
-		= language_class_name_from_physname (cu->language_defn,
-						     linkage_name);
-	      char *name = NULL;
+	      gdb::unique_xmalloc_ptr<char> actual_name
+		(language_class_name_from_physname (cu->language_defn,
+						    linkage_name));
+	      const char *name = NULL;
 
 	      if (actual_name != NULL)
 		{
 		  const char *die_name = dwarf2_name (die, cu);
 
 		  if (die_name != NULL
-		      && strcmp (die_name, actual_name) != 0)
+		      && strcmp (die_name, actual_name.get ()) != 0)
 		    {
 		      /* Strip off the class name from the full name.
 			 We want the prefix.  */
 		      int die_name_len = strlen (die_name);
-		      int actual_name_len = strlen (actual_name);
+		      int actual_name_len = strlen (actual_name.get ());
+		      const char *ptr = actual_name.get ();
 
 		      /* Test for '::' as a sanity check.  */
 		      if (actual_name_len > die_name_len + 2
-			  && actual_name[actual_name_len
-					 - die_name_len - 1] == ':')
+			  && ptr[actual_name_len - die_name_len - 1] == ':')
 			name = obstack_strndup (
 			  &objfile->per_bfd->storage_obstack,
-			  actual_name, actual_name_len - die_name_len - 2);
+			  ptr, actual_name_len - die_name_len - 2);
 		    }
 		}
-	      xfree (actual_name);
 	      return name;
 	    }
 	}
@@ -22941,7 +22915,7 @@  determine_prefix (struct die_info *die, struct dwarf2_cu *cu)
 		|| die->tag == DW_TAG_structure_type
 		|| die->tag == DW_TAG_union_type))
 	  {
-	    char *name = guess_full_die_structure_name (die, cu);
+	    const char *name = guess_full_die_structure_name (die, cu);
 	    if (name != NULL)
 	      return name;
 	  }
@@ -23115,8 +23089,6 @@  dwarf2_name (struct die_info *die, struct dwarf2_cu *cu)
 	 http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47510.  */
       if (!attr || DW_STRING (attr) == NULL)
 	{
-	  char *demangled = NULL;
-
 	  attr = dw2_linkage_name_attr (die, cu);
 	  if (attr == NULL || DW_STRING (attr) == NULL)
 	    return NULL;
@@ -23124,18 +23096,17 @@  dwarf2_name (struct die_info *die, struct dwarf2_cu *cu)
 	  /* Avoid demangling DW_STRING (attr) the second time on a second
 	     call for the same DIE.  */
 	  if (!DW_STRING_IS_CANONICAL (attr))
-	    demangled = gdb_demangle (DW_STRING (attr), DMGL_TYPES);
-
-	  if (demangled)
 	    {
+	      gdb::unique_xmalloc_ptr<char> demangled
+		(gdb_demangle (DW_STRING (attr), DMGL_TYPES));
+
 	      const char *base;
 
 	      /* FIXME: we already did this for the partial symbol... */
 	      DW_STRING (attr)
 		= obstack_strdup (&objfile->per_bfd->storage_obstack,
-				  demangled);
+				  demangled.get ());
 	      DW_STRING_IS_CANONICAL (attr) = 1;
-	      xfree (demangled);
 
 	      /* Strip any leading namespaces/classes, keep only the base name.
 		 DW_AT_name for named DIEs does not contain the prefixes.  */