[PATCH 0/6] Avoid manual memory management in gdb/compile/

[PATCH 0/6] Avoid manual memory management in gdb/compile/

[Tom Tromey]

I started working on a series to remove the use of
bfd_map_over_sections, but got sucked into cleaning up memory
management in gdb/compile instead.  This series is the result.

Tom

[PATCH 1/6] Remove some manual memory management from compile interface

[Tom Tromey]

This changes gdb's compile code to use std::vector in a couple of
places, rather than manual memory management.

gdb/ChangeLog
2020-08-08  Tom Tromey  <tom@tromey.com>

	* compile/compile-cplus-types.c
	(compile_cplus_convert_struct_or_union): Use std::vector.
	(compile_cplus_convert_func): Likewise.
	* compile/compile-c-types.c (convert_func): Use std::vector.
---
 gdb/ChangeLog                     |  7 ++++++
 gdb/compile/compile-c-types.c     |  4 ++--
 gdb/compile/compile-cplus-types.c | 39 +++++++++++++++----------------
 3 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/gdb/compile/compile-c-types.c b/gdb/compile/compile-c-types.c
index 2b25783bb00..7816ececb25 100644
--- a/gdb/compile/compile-c-types.c
+++ b/gdb/compile/compile-c-types.c
@@ -176,13 +176,13 @@ convert_func (compile_c_instance *context, struct type *type)
   return_type = context->convert_type (target_type);
 
   array.n_elements = type->num_fields ();
-  array.elements = XNEWVEC (gcc_type, type->num_fields ());
+  std::vector<gcc_type> elements (array.n_elements);
+  array.elements = elements.data ();
   for (i = 0; i < type->num_fields (); ++i)
     array.elements[i] = context->convert_type (type->field (i).type ());
 
   result = context->plugin ().build_function_type (return_type,
 						   &array, is_varargs);
-  xfree (array.elements);
 
   return result;
 }
diff --git a/gdb/compile/compile-cplus-types.c b/gdb/compile/compile-cplus-types.c
index 02df7ab90e6..00eaaf3fa68 100644
--- a/gdb/compile/compile-cplus-types.c
+++ b/gdb/compile/compile-cplus-types.c
@@ -848,33 +848,33 @@ compile_cplus_convert_struct_or_union (compile_cplus_instance *instance,
   gcc_type result;
   if (type->code () == TYPE_CODE_STRUCT)
     {
-      struct gcc_vbase_array bases;
       int num_baseclasses = TYPE_N_BASECLASSES (type);
+      std::vector<gcc_type> elements (num_baseclasses);
+      std::vector<enum gcc_cp_symbol_kind> flags (num_baseclasses);
 
-      memset (&bases, 0, sizeof (bases));
+      struct gcc_vbase_array bases {};
+      bases.elements = elements.data ();
 
       if (num_baseclasses > 0)
 	{
-	  bases.elements = XNEWVEC (gcc_type, num_baseclasses);
-	  bases.flags = XNEWVEC (enum gcc_cp_symbol_kind, num_baseclasses);
+	  bases.flags = flags.data ();
 	  bases.n_elements = num_baseclasses;
-	  for (int i = 0; i < num_baseclasses; ++i)
-	    {
-	      struct type *base_type = TYPE_BASECLASS (type, i);
-
-	      bases.flags[i] = GCC_CP_SYMBOL_BASECLASS
-		| get_field_access_flag (type, i)
-		| (BASETYPE_VIA_VIRTUAL (type, i)
-		   ? GCC_CP_FLAG_BASECLASS_VIRTUAL
-		   : GCC_CP_FLAG_BASECLASS_NOFLAG);
-	      bases.elements[i] = instance->convert_type (base_type);
-	    }
+	}
+
+      for (int i = 0; i < num_baseclasses; ++i)
+	{
+	  struct type *base_type = TYPE_BASECLASS (type, i);
+
+	  bases.flags[i] = (GCC_CP_SYMBOL_BASECLASS
+			    | get_field_access_flag (type, i)
+			    | (BASETYPE_VIA_VIRTUAL (type, i)
+			       ? GCC_CP_FLAG_BASECLASS_VIRTUAL
+			       : GCC_CP_FLAG_BASECLASS_NOFLAG));
+	  bases.elements[i] = instance->convert_type (base_type);
 	}
 
       result = instance->plugin ().start_class_type
 	(name.get (), resuld, &bases, filename, line);
-      xfree (bases.flags);
-      xfree (bases.elements);
     }
   else
     {
@@ -985,8 +985,8 @@ compile_cplus_convert_func (compile_cplus_instance *instance,
      types.  Those are impossible in C, though.  */
   gcc_type return_type = instance->convert_type (target_type);
 
-  struct gcc_type_array array =
-    { type->num_fields (), XNEWVEC (gcc_type, type->num_fields ()) };
+  std::vector<gcc_type> elements (type->num_fields ());
+  struct gcc_type_array array = { type->num_fields (), elements.data () };
   int artificials = 0;
   for (int i = 0; i < type->num_fields (); ++i)
     {
@@ -1006,7 +1006,6 @@ compile_cplus_convert_func (compile_cplus_instance *instance,
      with some minsyms like printf (compile-cplus.exp has examples).  */
   gcc_type result = instance->plugin ().build_function_type
     (return_type, &array, is_varargs);
-  xfree (array.elements);
   return result;
 }
 
-- 
2.17.2

[PATCH 2/6] Use new/delete for do_module_cleanup

[Tom Tromey]

This changes do_module_cleanup to use new and delete.  It also removes
the use of the struct hack from this object -- this requires more
allocations for now, but this will be removed in a subsequent patch.

gdb/ChangeLog
2020-08-08  Tom Tromey  <tom@tromey.com>

	* compile/compile-object-run.c (struct do_module_cleanup): Add
	constructor, destructor.
	<objfile_name_string>: Don't use struct hack.
	(do_module_cleanup): Use delete.
	(compile_object_run): Use new.
---
 gdb/ChangeLog                    |  8 ++++++++
 gdb/compile/compile-object-run.c | 24 ++++++++++++++++--------
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/gdb/compile/compile-object-run.c b/gdb/compile/compile-object-run.c
index a2f39900053..4a18655d488 100644
--- a/gdb/compile/compile-object-run.c
+++ b/gdb/compile/compile-object-run.c
@@ -32,6 +32,17 @@
 
 struct do_module_cleanup
 {
+  do_module_cleanup () = default;
+
+  ~do_module_cleanup ()
+  {
+    delete munmap_list_head;
+    xfree (source_file);
+    xfree (objfile_name_string);
+  }
+
+  DISABLE_COPY_AND_ASSIGN (do_module_cleanup);
+
   /* Boolean to set true upon a call of do_module_cleanup.
      The pointer may be NULL.  */
   int *executedp;
@@ -51,7 +62,7 @@ struct do_module_cleanup
   struct munmap_list *munmap_list_head;
 
   /* objfile_name of our objfile.  */
-  char objfile_name_string[1];
+  char *objfile_name_string;
 };
 
 /* Cleanup everything after the inferior function dummy frame gets
@@ -96,13 +107,11 @@ do_module_cleanup (void *arg, int registers_valid)
 
   /* Delete the .c file.  */
   unlink (data->source_file);
-  xfree (data->source_file);
-
-  delete data->munmap_list_head;
 
   /* Delete the .o file.  */
   unlink (data->objfile_name_string);
-  xfree (data);
+
+  delete data;
 }
 
 /* Perform inferior call of MODULE.  This function may throw an error.
@@ -122,11 +131,10 @@ compile_object_run (struct compile_module *module)
   CORE_ADDR regs_addr = module->regs_addr;
   struct objfile *objfile = module->objfile;
 
-  data = (struct do_module_cleanup *) xmalloc (sizeof (*data)
-					       + strlen (objfile_name_s));
+  data = new struct do_module_cleanup;
   data->executedp = &executed;
   data->source_file = xstrdup (module->source_file);
-  strcpy (data->objfile_name_string, objfile_name_s);
+  data->objfile_name_string = xstrdup (objfile_name_s);
   data->scope = module->scope;
   data->scope_data = module->scope_data;
   data->out_value_type = module->out_value_type;
-- 
2.17.2

[PATCH 3/6] Introduce and use compile_module_up

[Tom Tromey]

This introduces compile_module_up, a unique pointer for
compile_module, and changes a few spots to use it.

gdb/ChangeLog
2020-08-08  Tom Tromey  <tom@tromey.com>

	* compile/compile.c (eval_compile_command): Update.
	* compile/compile-object-run.h (compile_object_run): Take a
	compile_module_up.
	* compile/compile-object-run.c (compile_object_run): Take a
	compile_module_up.
	* compile/compile-object-load.h (struct compile_module): Add
	constructor, destructor.
	(compile_module_up): New typedef.
	(compile_object_load): Return compile_object_up.
	* compile/compile-object-load.c (compile_object_load): Return
	compile_module_up.
---
 gdb/ChangeLog                     | 14 ++++++++++++++
 gdb/compile/compile-object-load.c |  5 ++---
 gdb/compile/compile-object-load.h | 12 +++++++++++-
 gdb/compile/compile-object-run.c  |  4 +---
 gdb/compile/compile-object-run.h  |  2 +-
 gdb/compile/compile.c             |  7 +++----
 6 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/gdb/compile/compile-object-load.c b/gdb/compile/compile-object-load.c
index 2f416079025..dff10fe94f4 100644
--- a/gdb/compile/compile-object-load.c
+++ b/gdb/compile/compile-object-load.c
@@ -582,7 +582,7 @@ store_regs (struct type *regs_type, CORE_ADDR regs_base)
    COMPILE_I_PRINT_ADDRESS_SCOPE when COMPILE_I_PRINT_VALUE_SCOPE
    should have been used instead.  */
 
-struct compile_module *
+compile_module_up
 compile_object_load (const compile_file_names &file_names,
 		     enum compile_i_scope_types scope, void *scope_data)
 {
@@ -594,7 +594,6 @@ compile_object_load (const compile_file_names &file_names,
   long storage_needed;
   asymbol **symbol_table, **symp;
   long number_of_symbols, missing_symbols;
-  struct compile_module *retval;
   struct type *regs_type, *out_value_type = NULL;
   char **matching;
   struct objfile *objfile;
@@ -790,7 +789,7 @@ compile_object_load (const compile_file_names &file_names,
 			    paddress (target_gdbarch (), out_value_addr));
     }
 
-  retval = XNEW (struct compile_module);
+  compile_module_up retval (new struct compile_module);
   retval->objfile = objfile_holder.release ();
   retval->source_file = xstrdup (file_names.source_file ());
   retval->func_sym = func_sym;
diff --git a/gdb/compile/compile-object-load.h b/gdb/compile/compile-object-load.h
index c4adc719141..9def29ebc9b 100644
--- a/gdb/compile/compile-object-load.h
+++ b/gdb/compile/compile-object-load.h
@@ -46,6 +46,13 @@ struct munmap_list
 
 struct compile_module
 {
+  compile_module () = default;
+
+  DISABLE_COPY_AND_ASSIGN (compile_module);
+
+  compile_module &operator= (compile_module &&other) = default;
+  compile_module (compile_module &&other) = default;
+
   /* objfile for the compiled module.  */
   struct objfile *objfile;
 
@@ -77,7 +84,10 @@ struct compile_module
   struct munmap_list *munmap_list_head;
 };
 
-extern struct compile_module *compile_object_load
+/* A unique pointer for a compile_module.  */
+typedef std::unique_ptr<compile_module> compile_module_up;
+
+extern compile_module_up compile_object_load
   (const compile_file_names &fnames,
    enum compile_i_scope_types scope, void *scope_data);
 
diff --git a/gdb/compile/compile-object-run.c b/gdb/compile/compile-object-run.c
index 4a18655d488..5a680a6723f 100644
--- a/gdb/compile/compile-object-run.c
+++ b/gdb/compile/compile-object-run.c
@@ -121,7 +121,7 @@ do_module_cleanup (void *arg, int registers_valid)
    longer touch MODULE's memory after this function has been called.  */
 
 void
-compile_object_run (struct compile_module *module)
+compile_object_run (compile_module_up &&module)
 {
   struct value *func_val;
   struct do_module_cleanup *data;
@@ -142,8 +142,6 @@ compile_object_run (struct compile_module *module)
   data->munmap_list_head = module->munmap_list_head;
 
   xfree (module->source_file);
-  xfree (module);
-  module = NULL;
 
   try
     {
diff --git a/gdb/compile/compile-object-run.h b/gdb/compile/compile-object-run.h
index 9311d02c379..45375db654f 100644
--- a/gdb/compile/compile-object-run.h
+++ b/gdb/compile/compile-object-run.h
@@ -19,6 +19,6 @@
 
 #include "compile-object-load.h"
 
-extern void compile_object_run (struct compile_module *module);
+extern void compile_object_run (compile_module_up &&module);
 
 #endif /* COMPILE_COMPILE_OBJECT_RUN_H */
diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c
index 0c29a0476e7..0f830f6db71 100644
--- a/gdb/compile/compile.c
+++ b/gdb/compile/compile.c
@@ -820,14 +820,13 @@ void
 eval_compile_command (struct command_line *cmd, const char *cmd_string,
 		      enum compile_i_scope_types scope, void *scope_data)
 {
-  struct compile_module *compile_module;
-
   compile_file_names fnames = compile_to_object (cmd, cmd_string, scope);
 
   gdb::unlinker object_remover (fnames.object_file ());
   gdb::unlinker source_remover (fnames.source_file ());
 
-  compile_module = compile_object_load (fnames, scope, scope_data);
+  compile_module_up compile_module = compile_object_load (fnames, scope,
+							  scope_data);
   if (compile_module == NULL)
     {
       gdb_assert (scope == COMPILE_I_PRINT_ADDRESS_SCOPE);
@@ -840,7 +839,7 @@ eval_compile_command (struct command_line *cmd, const char *cmd_string,
   source_remover.keep ();
   object_remover.keep ();
 
-  compile_object_run (compile_module);
+  compile_object_run (std::move (compile_module));
 }
 
 /* See compile/compile-internal.h.  */
-- 
2.17.2

[PATCH 4/6] Transfer module ownership to do_module_cleanup

[Tom Tromey]

This changes the do_module_cleanup structure to simply hold on to the
module itself.  This lets us remove most members from
do_module_cleanup.

gdb/ChangeLog
2020-08-08  Tom Tromey  <tom@tromey.com>

	* compile/compile-object-run.c (struct do_module_cleanup): Add
	parameters to constructor.  Update destructor.
	<source_file, scope, scope_data, out_value_type, out_value_addr,
	munmap_list_head, objfile_name_string>: Remove.
	<module>: New member.
	(do_module_cleanup): Update.
	(compile_object_run): Update.
---
 gdb/ChangeLog                    | 10 +++++
 gdb/compile/compile-object-run.c | 67 ++++++++++++--------------------
 2 files changed, 35 insertions(+), 42 deletions(-)

diff --git a/gdb/compile/compile-object-run.c b/gdb/compile/compile-object-run.c
index 5a680a6723f..ac0a995fee9 100644
--- a/gdb/compile/compile-object-run.c
+++ b/gdb/compile/compile-object-run.c
@@ -32,13 +32,16 @@
 
 struct do_module_cleanup
 {
-  do_module_cleanup () = default;
+  do_module_cleanup (int *ptr, compile_module_up &&mod)
+    : executedp (ptr),
+      module (std::move (mod))
+  {
+  }
 
   ~do_module_cleanup ()
   {
-    delete munmap_list_head;
-    xfree (source_file);
-    xfree (objfile_name_string);
+    delete module->munmap_list_head;
+    xfree (module->source_file);
   }
 
   DISABLE_COPY_AND_ASSIGN (do_module_cleanup);
@@ -47,22 +50,8 @@ struct do_module_cleanup
      The pointer may be NULL.  */
   int *executedp;
 
-  /* .c file OBJFILE was built from.  It needs to be xfree-d.  */
-  char *source_file;
-
-  /* Copy from struct compile_module.  */
-  enum compile_i_scope_types scope;
-  void *scope_data;
-
-  /* Copy from struct compile_module.  */
-  struct type *out_value_type;
-  CORE_ADDR out_value_addr;
-
-  /* Copy from struct compile_module.  */
-  struct munmap_list *munmap_list_head;
-
-  /* objfile_name of our objfile.  */
-  char *objfile_name_string;
+  /* The compile module.  */
+  compile_module_up module;
 };
 
 /* Cleanup everything after the inferior function dummy frame gets
@@ -80,22 +69,26 @@ do_module_cleanup (void *arg, int registers_valid)
 
       /* This code cannot be in compile_object_run as OUT_VALUE_TYPE
 	 no longer exists there.  */
-      if (data->scope == COMPILE_I_PRINT_ADDRESS_SCOPE
-	  || data->scope == COMPILE_I_PRINT_VALUE_SCOPE)
+      if (data->module->scope == COMPILE_I_PRINT_ADDRESS_SCOPE
+	  || data->module->scope == COMPILE_I_PRINT_VALUE_SCOPE)
 	{
 	  struct value *addr_value;
-	  struct type *ptr_type = lookup_pointer_type (data->out_value_type);
+	  struct type *ptr_type
+	    = lookup_pointer_type (data->module->out_value_type);
 
-	  addr_value = value_from_pointer (ptr_type, data->out_value_addr);
+	  addr_value = value_from_pointer (ptr_type,
+					   data->module->out_value_addr);
 
 	  /* SCOPE_DATA would be stale unless EXECUTEDP != NULL.  */
-	  compile_print_value (value_ind (addr_value), data->scope_data);
+	  compile_print_value (value_ind (addr_value),
+			       data->module->scope_data);
 	}
     }
 
+  const char *objfile_name_s = objfile_name (data->module->objfile);
   for (objfile *objfile : current_program_space->objfiles ())
     if ((objfile->flags & OBJF_USERLOADED) == 0
-        && (strcmp (objfile_name (objfile), data->objfile_name_string) == 0))
+        && (strcmp (objfile_name (objfile), objfile_name_s) == 0))
       {
 	objfile->unlink ();
 
@@ -106,10 +99,10 @@ do_module_cleanup (void *arg, int registers_valid)
       }
 
   /* Delete the .c file.  */
-  unlink (data->source_file);
+  unlink (data->module->source_file);
 
   /* Delete the .o file.  */
-  unlink (data->objfile_name_string);
+  unlink (objfile_name_s);
 
   delete data;
 }
@@ -125,23 +118,12 @@ compile_object_run (compile_module_up &&module)
 {
   struct value *func_val;
   struct do_module_cleanup *data;
-  const char *objfile_name_s = objfile_name (module->objfile);
   int dtor_found, executed = 0;
   struct symbol *func_sym = module->func_sym;
   CORE_ADDR regs_addr = module->regs_addr;
   struct objfile *objfile = module->objfile;
 
-  data = new struct do_module_cleanup;
-  data->executedp = &executed;
-  data->source_file = xstrdup (module->source_file);
-  data->objfile_name_string = xstrdup (objfile_name_s);
-  data->scope = module->scope;
-  data->scope_data = module->scope_data;
-  data->out_value_type = module->out_value_type;
-  data->out_value_addr = module->out_value_addr;
-  data->munmap_list_head = module->munmap_list_head;
-
-  xfree (module->source_file);
+  data = new struct do_module_cleanup (&executed, std::move (module));
 
   try
     {
@@ -169,9 +151,10 @@ compile_object_run (compile_module_up &&module)
 	}
       if (func_type->num_fields () >= 2)
 	{
-	  gdb_assert (data->out_value_addr != 0);
+	  gdb_assert (data->module->out_value_addr != 0);
 	  vargs[current_arg] = value_from_pointer
-	       (func_type->field (current_arg).type (), data->out_value_addr);
+	       (func_type->field (current_arg).type (),
+		data->module->out_value_addr);
 	  ++current_arg;
 	}
       gdb_assert (current_arg == func_type->num_fields ());
-- 
2.17.2

[PATCH 5/6] Simplify compile_module cleanup

[Tom Tromey]

This simplifies compile_module cleanup by removing the need to
explicitly free anything.  struct setup_sections_data is also cleaned
up a bit.

gdb/ChangeLog
2020-08-08  Tom Tromey  <tom@tromey.com>

	* compile/compile-object-run.c (do_module_cleanup)
	<~do_module_cleanup> :Remove.
	(do_module_cleanup): Update.
	* compile/compile-object-load.h (struct munmap_list): Add move
	assignment operator.
	<source_file>: Now a std::string.
	<munmap_list>: Rename.  No longer a pointer.
	* compile/compile-object-load.c (struct setup_sections_data): Add
	constructor.
	<setup_one_section>: Declare.
	<munmap_list>: Move earlier.
	<m_bfd>: New member.
	<m_last_size, m_last_section_first, m_last_prot,
	m_last_max_alignment>: Rename, add initializers where needed.
	(setup_sections_data::setup_one_section): Rename from
	setup_sections.  Update.
	(compile_object_load): Update.  Don't use bfd_map_over_sections.
---
 gdb/ChangeLog                     |  20 ++++++
 gdb/compile/compile-object-load.c | 103 +++++++++++++++++-------------
 gdb/compile/compile-object-load.h |   8 ++-
 gdb/compile/compile-object-run.c  |   8 +--
 4 files changed, 83 insertions(+), 56 deletions(-)

diff --git a/gdb/compile/compile-object-load.c b/gdb/compile/compile-object-load.c
index dff10fe94f4..bde3b742db4 100644
--- a/gdb/compile/compile-object-load.c
+++ b/gdb/compile/compile-object-load.c
@@ -62,34 +62,50 @@ munmap_list::~munmap_list ()
     }
 }
 
-/* Helper data for setup_sections.  */
+/* A data structure that is used to lay out sections of our objfile in
+   inferior memory.  */
 
 struct setup_sections_data
 {
+  explicit setup_sections_data (bfd *abfd)
+    : m_bfd (abfd),
+      m_last_section_first (abfd->sections)
+  {
+  }
+
+  /* Place all ABFD sections next to each other obeying all
+     constraints.  */
+  void setup_one_section (asection *sect);
+
+  /* List of inferior mmap ranges where setup_sections should add its
+     next range.  */
+  struct munmap_list munmap_list;
+
+private:
+
+  /* The BFD.  */
+  bfd *m_bfd;
+
   /* Size of all recent sections with matching LAST_PROT.  */
-  CORE_ADDR last_size;
+  CORE_ADDR m_last_size = 0;
 
   /* First section matching LAST_PROT.  */
-  asection *last_section_first;
+  asection *m_last_section_first;
 
   /* Memory protection like the prot parameter of gdbarch_infcall_mmap. */
-  unsigned last_prot;
+  unsigned m_last_prot = -1;
 
   /* Maximum of alignments of all sections matching LAST_PROT.
      This value is always at least 1.  This value is always a power of 2.  */
-  CORE_ADDR last_max_alignment;
+  CORE_ADDR m_last_max_alignment = -1;
 
-  /* List of inferior mmap ranges where setup_sections should add its
-     next range.  */
-  std::unique_ptr<struct munmap_list> munmap_list;
 };
 
-/* Place all ABFD sections next to each other obeying all constraints.  */
+/* See setup_sections_data.  */
 
-static void
-setup_sections (bfd *abfd, asection *sect, void *data_voidp)
+void
+setup_sections_data::setup_one_section (asection *sect)
 {
-  struct setup_sections_data *data = (struct setup_sections_data *) data_voidp;
   CORE_ADDR alignment;
   unsigned prot;
 
@@ -112,7 +128,7 @@ setup_sections (bfd *abfd, asection *sect, void *data_voidp)
       if (compile_debug)
 	fprintf_unfiltered (gdb_stdlog,
 			    "module \"%s\" section \"%s\" size %s prot %u\n",
-			    bfd_get_filename (abfd),
+			    bfd_get_filename (m_bfd),
 			    bfd_section_name (sect),
 			    paddress (target_gdbarch (),
 				      bfd_section_size (sect)),
@@ -122,55 +138,55 @@ setup_sections (bfd *abfd, asection *sect, void *data_voidp)
     prot = -1;
 
   if (sect == NULL
-      || (data->last_prot != prot && bfd_section_size (sect) != 0))
+      || (m_last_prot != prot && bfd_section_size (sect) != 0))
     {
       CORE_ADDR addr;
       asection *sect_iter;
 
-      if (data->last_size != 0)
+      if (m_last_size != 0)
 	{
-	  addr = gdbarch_infcall_mmap (target_gdbarch (), data->last_size,
-				       data->last_prot);
-	  data->munmap_list->add (addr, data->last_size);
+	  addr = gdbarch_infcall_mmap (target_gdbarch (), m_last_size,
+				       m_last_prot);
+	  munmap_list.add (addr, m_last_size);
 	  if (compile_debug)
 	    fprintf_unfiltered (gdb_stdlog,
 				"allocated %s bytes at %s prot %u\n",
-				paddress (target_gdbarch (), data->last_size),
+				paddress (target_gdbarch (), m_last_size),
 				paddress (target_gdbarch (), addr),
-				data->last_prot);
+				m_last_prot);
 	}
       else
 	addr = 0;
 
-      if ((addr & (data->last_max_alignment - 1)) != 0)
+      if ((addr & (m_last_max_alignment - 1)) != 0)
 	error (_("Inferior compiled module address %s "
 		 "is not aligned to BFD required %s."),
 	       paddress (target_gdbarch (), addr),
-	       paddress (target_gdbarch (), data->last_max_alignment));
+	       paddress (target_gdbarch (), m_last_max_alignment));
 
-      for (sect_iter = data->last_section_first; sect_iter != sect;
+      for (sect_iter = m_last_section_first; sect_iter != sect;
 	   sect_iter = sect_iter->next)
 	if ((bfd_section_flags (sect_iter) & SEC_ALLOC) != 0)
 	  bfd_set_section_vma (sect_iter, addr + bfd_section_vma (sect_iter));
 
-      data->last_size = 0;
-      data->last_section_first = sect;
-      data->last_prot = prot;
-      data->last_max_alignment = 1;
+      m_last_size = 0;
+      m_last_section_first = sect;
+      m_last_prot = prot;
+      m_last_max_alignment = 1;
     }
 
   if (sect == NULL)
     return;
 
   alignment = ((CORE_ADDR) 1) << bfd_section_alignment (sect);
-  data->last_max_alignment = std::max (data->last_max_alignment, alignment);
+  m_last_max_alignment = std::max (m_last_max_alignment, alignment);
 
-  data->last_size = (data->last_size + alignment - 1) & -alignment;
+  m_last_size = (m_last_size + alignment - 1) & -alignment;
 
-  bfd_set_section_vma (sect, data->last_size);
+  bfd_set_section_vma (sect, m_last_size);
 
-  data->last_size += bfd_section_size (sect);
-  data->last_size = (data->last_size + alignment - 1) & -alignment;
+  m_last_size += bfd_section_size (sect);
+  m_last_size = (m_last_size + alignment - 1) & -alignment;
 }
 
 /* Helper for link_callbacks callbacks vector.  */
@@ -586,7 +602,6 @@ compile_module_up
 compile_object_load (const compile_file_names &file_names,
 		     enum compile_i_scope_types scope, void *scope_data)
 {
-  struct setup_sections_data setup_sections_data;
   CORE_ADDR regs_addr, out_value_addr = 0;
   struct symbol *func_sym;
   struct type *func_type;
@@ -616,14 +631,10 @@ compile_object_load (const compile_file_names &file_names,
   if ((bfd_get_file_flags (abfd.get ()) & (EXEC_P | DYNAMIC)) != 0)
     error (_("\"%s\": not in object format."), filename.get ());
 
-  setup_sections_data.last_size = 0;
-  setup_sections_data.last_section_first = abfd->sections;
-  setup_sections_data.last_prot = -1;
-  setup_sections_data.last_max_alignment = 1;
-  setup_sections_data.munmap_list.reset (new struct munmap_list);
-
-  bfd_map_over_sections (abfd.get (), setup_sections, &setup_sections_data);
-  setup_sections (abfd.get (), NULL, &setup_sections_data);
+  struct setup_sections_data setup_sections_data (abfd.get ());
+  for (asection *sect = abfd->sections; sect != nullptr; sect = sect->next)
+    setup_sections_data.setup_one_section (sect);
+  setup_sections_data.setup_one_section (nullptr);
 
   storage_needed = bfd_get_symtab_upper_bound (abfd.get ());
   if (storage_needed < 0)
@@ -757,7 +768,7 @@ compile_object_load (const compile_file_names &file_names,
 					TYPE_LENGTH (regs_type),
 					GDB_MMAP_PROT_READ);
       gdb_assert (regs_addr != 0);
-      setup_sections_data.munmap_list->add (regs_addr, TYPE_LENGTH (regs_type));
+      setup_sections_data.munmap_list.add (regs_addr, TYPE_LENGTH (regs_type));
       if (compile_debug)
 	fprintf_unfiltered (gdb_stdlog,
 			    "allocated %s bytes at %s for registers\n",
@@ -779,8 +790,8 @@ compile_object_load (const compile_file_names &file_names,
 					     (GDB_MMAP_PROT_READ
 					      | GDB_MMAP_PROT_WRITE));
       gdb_assert (out_value_addr != 0);
-      setup_sections_data.munmap_list->add (out_value_addr,
-					    TYPE_LENGTH (out_value_type));
+      setup_sections_data.munmap_list.add (out_value_addr,
+					   TYPE_LENGTH (out_value_type));
       if (compile_debug)
 	fprintf_unfiltered (gdb_stdlog,
 			    "allocated %s bytes at %s for printed value\n",
@@ -791,14 +802,14 @@ compile_object_load (const compile_file_names &file_names,
 
   compile_module_up retval (new struct compile_module);
   retval->objfile = objfile_holder.release ();
-  retval->source_file = xstrdup (file_names.source_file ());
+  retval->source_file = file_names.source_file ();
   retval->func_sym = func_sym;
   retval->regs_addr = regs_addr;
   retval->scope = scope;
   retval->scope_data = scope_data;
   retval->out_value_type = out_value_type;
   retval->out_value_addr = out_value_addr;
-  retval->munmap_list_head = setup_sections_data.munmap_list.release ();
+  retval->munmap_list = std::move (setup_sections_data.munmap_list);
 
   return retval;
 }
diff --git a/gdb/compile/compile-object-load.h b/gdb/compile/compile-object-load.h
index 9def29ebc9b..0254390109e 100644
--- a/gdb/compile/compile-object-load.h
+++ b/gdb/compile/compile-object-load.h
@@ -29,6 +29,8 @@ struct munmap_list
 
   DISABLE_COPY_AND_ASSIGN (munmap_list);
 
+  munmap_list &operator= (munmap_list &&) = default;
+
   /* Add a region to the list.  */
   void add (CORE_ADDR addr, CORE_ADDR size);
 
@@ -56,8 +58,8 @@ struct compile_module
   /* objfile for the compiled module.  */
   struct objfile *objfile;
 
-  /* .c file OBJFILE was built from.  It needs to be xfree-d.  */
-  char *source_file;
+  /* .c file OBJFILE was built from.  */
+  std::string source_file;
 
   /* Inferior function GCC_FE_WRAPPER_FUNCTION.  */
   struct symbol *func_sym;
@@ -81,7 +83,7 @@ struct compile_module
   CORE_ADDR out_value_addr;
 
   /* Track inferior memory reserved by inferior mmap.  */
-  struct munmap_list *munmap_list_head;
+  struct munmap_list munmap_list;
 };
 
 /* A unique pointer for a compile_module.  */
diff --git a/gdb/compile/compile-object-run.c b/gdb/compile/compile-object-run.c
index ac0a995fee9..10867eee30c 100644
--- a/gdb/compile/compile-object-run.c
+++ b/gdb/compile/compile-object-run.c
@@ -38,12 +38,6 @@ struct do_module_cleanup
   {
   }
 
-  ~do_module_cleanup ()
-  {
-    delete module->munmap_list_head;
-    xfree (module->source_file);
-  }
-
   DISABLE_COPY_AND_ASSIGN (do_module_cleanup);
 
   /* Boolean to set true upon a call of do_module_cleanup.
@@ -99,7 +93,7 @@ do_module_cleanup (void *arg, int registers_valid)
       }
 
   /* Delete the .c file.  */
-  unlink (data->module->source_file);
+  unlink (data->module->source_file.c_str ());
 
   /* Delete the .o file.  */
   unlink (objfile_name_s);
-- 
2.17.2

[PATCH 6/6] Avoid manual memory management of argv arrays in gdb/compile

[Tom Tromey]

This changes gdb/compile to use gdb_argv directly, rather than
manually managing the arrays itself.  A few new helpers are added to
gdb_argv.

gdb/ChangeLog
2020-08-08  Tom Tromey  <tom@tromey.com>

	* utils.h (class gdb_argv): Add move operators.
	<append>: New methods.
	* compile/compile.c (build_argc_argv): Remove.
	(compile_args_argc): Remove.
	(compile_args_argv): Change type.
	(set_compile_args): Simplify.
	(append_args): Remove.
	(filter_args): Remove argcp parameter.
	(get_args): Return gdb_argv.  Simplify.
	(compile_to_object): Update.
---
 gdb/ChangeLog         | 13 ++++++++
 gdb/compile/compile.c | 75 ++++++++++---------------------------------
 gdb/utils.h           | 43 +++++++++++++++++++++++++
 3 files changed, 73 insertions(+), 58 deletions(-)

diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c
index 0f830f6db71..cc91d24fe1a 100644
--- a/gdb/compile/compile.c
+++ b/gdb/compile/compile.c
@@ -493,33 +493,18 @@ get_expr_block_and_pc (CORE_ADDR *pc)
   return block;
 }
 
-/* Call buildargv (via gdb_argv), set its result for S into *ARGVP but
-   calculate also the number of parsed arguments into *ARGCP.  If
-   buildargv has returned NULL then *ARGCP is set to zero.  */
-
-static void
-build_argc_argv (const char *s, int *argcp, char ***argvp)
-{
-  gdb_argv args (s);
-
-  *argcp = args.count ();
-  *argvp = args.release ();
-}
-
 /* String for 'set compile-args' and 'show compile-args'.  */
 static char *compile_args;
 
-/* Parsed form of COMPILE_ARGS.  COMPILE_ARGS_ARGV is NULL terminated.  */
-static int compile_args_argc;
-static char **compile_args_argv;
+/* Parsed form of COMPILE_ARGS.  */
+static gdb_argv compile_args_argv;
 
 /* Implement 'set compile-args'.  */
 
 static void
 set_compile_args (const char *args, int from_tty, struct cmd_list_element *c)
 {
-  freeargv (compile_args_argv);
-  build_argc_argv (compile_args, &compile_args_argc, &compile_args_argv);
+  compile_args_argv = gdb_argv (compile_args);
 }
 
 /* Implement 'show compile-args'.  */
@@ -533,21 +518,6 @@ show_compile_args (struct ui_file *file, int from_tty,
 		    value);
 }
 
-/* Append ARGC and ARGV (as parsed by build_argc_argv) to *ARGCP and *ARGVP.
-   ARGCP+ARGVP can be zero+NULL and also ARGC+ARGV can be zero+NULL.  */
-
-static void
-append_args (int *argcp, char ***argvp, int argc, char **argv)
-{
-  int argi;
-
-  *argvp = XRESIZEVEC (char *, *argvp, (*argcp + argc + 1));
-
-  for (argi = 0; argi < argc; argi++)
-    (*argvp)[(*argcp)++] = xstrdup (argv[argi]);
-  (*argvp)[(*argcp)] = NULL;
-}
-
 /* String for 'set compile-gcc' and 'show compile-gcc'.  */
 static char *compile_gcc;
 
@@ -586,10 +556,10 @@ get_selected_pc_producer_options (void)
   return cs;
 }
 
-/* Filter out unwanted options from *ARGCP and ARGV.  */
+/* Filter out unwanted options from ARGV.  */
 
 static void
-filter_args (int *argcp, char **argv)
+filter_args (char **argv)
 {
   char **destv;
 
@@ -599,7 +569,6 @@ filter_args (int *argcp, char **argv)
       if (strcmp (*argv, "-fpreprocessed") == 0)
 	{
 	  xfree (*argv);
-	  (*argcp)--;
 	  continue;
 	}
       *destv++ = *argv;
@@ -627,35 +596,26 @@ filter_args (int *argcp, char **argv)
    appended last so as to override any of the arguments automatically
    generated above.  */
 
-static void
-get_args (const compile_instance *compiler, struct gdbarch *gdbarch,
-	  int *argcp, char ***argvp)
+static gdb_argv
+get_args (const compile_instance *compiler, struct gdbarch *gdbarch)
 {
   const char *cs_producer_options;
-  int argc_compiler;
-  char **argv_compiler;
 
-  build_argc_argv (gdbarch_gcc_target_options (gdbarch).c_str (),
-		   argcp, argvp);
+  gdb_argv result (gdbarch_gcc_target_options (gdbarch).c_str ());
 
   cs_producer_options = get_selected_pc_producer_options ();
   if (cs_producer_options != NULL)
     {
-      int argc_producer;
-      char **argv_producer;
+      gdb_argv argv_producer (cs_producer_options);
+      filter_args (argv_producer.get ());
 
-      build_argc_argv (cs_producer_options, &argc_producer, &argv_producer);
-      filter_args (&argc_producer, argv_producer);
-      append_args (argcp, argvp, argc_producer, argv_producer);
-      freeargv (argv_producer);
+      result.append (std::move (argv_producer));
     }
 
-  build_argc_argv (compiler->gcc_target_options ().c_str (),
-		   &argc_compiler, &argv_compiler);
-  append_args (argcp, argvp, argc_compiler, argv_compiler);
-  freeargv (argv_compiler);
+  result.append (gdb_argv (compiler->gcc_target_options ().c_str ()));
+  result.append (compile_args_argv);
 
-  append_args (argcp, argvp, compile_args_argc, compile_args_argv);
+  return result;
 }
 
 /* A helper function suitable for use as the "print_callback" in the
@@ -677,8 +637,6 @@ compile_to_object (struct command_line *cmd, const char *cmd_string,
 {
   const struct block *expr_block;
   CORE_ADDR trash_pc, expr_pc;
-  int argc;
-  char **argv;
   int ok;
   struct gdbarch *gdbarch = get_current_arch ();
   std::string triplet_rx;
@@ -750,8 +708,9 @@ compile_to_object (struct command_line *cmd, const char *cmd_string,
     }
 
   /* Set compiler command-line arguments.  */
-  get_args (compiler.get (), gdbarch, &argc, &argv);
-  gdb_argv argv_holder (argv);
+  gdb_argv argv_holder = get_args (compiler.get (), gdbarch);
+  int argc = argv_holder.count ();
+  char **argv = argv_holder.get ();
 
   gdb::unique_xmalloc_ptr<char> error_message;
   error_message.reset (compiler->set_arguments (argc, argv,
diff --git a/gdb/utils.h b/gdb/utils.h
index 3434ff1caa2..29ce0125035 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -164,6 +164,20 @@ class gdb_argv
   gdb_argv (const gdb_argv &) = delete;
   gdb_argv &operator= (const gdb_argv &) = delete;
 
+  gdb_argv &operator= (gdb_argv &&other)
+  {
+    freeargv (m_argv);
+    m_argv = other.m_argv;
+    other.m_argv = nullptr;
+    return *this;
+  }
+
+  gdb_argv (gdb_argv &&other)
+  {
+    m_argv = other.m_argv;
+    other.m_argv = nullptr;
+  }
+
   ~gdb_argv ()
   {
     freeargv (m_argv);
@@ -210,6 +224,35 @@ class gdb_argv
     return m_argv[arg];
   }
 
+  /* Append arguments to this array.  */
+  void append (gdb_argv &&other)
+  {
+    int size = count ();
+    int argc = other.count ();
+    m_argv = XRESIZEVEC (char *, m_argv, (size + argc + 1));
+
+    for (int argi = 0; argi < argc; argi++)
+      {
+	/* Transfer ownership of the string.  */
+	m_argv[size++] = other.m_argv[argi];
+	/* Ensure that destruction of OTHER works correctly.  */
+	other.m_argv[argi] = nullptr;
+      }
+    m_argv[size] = nullptr;
+  }
+
+  /* Append arguments to this array.  */
+  void append (const gdb_argv &other)
+  {
+    int size = count ();
+    int argc = other.count ();
+    m_argv = XRESIZEVEC (char *, m_argv, (size + argc + 1));
+
+    for (int argi = 0; argi < argc; argi++)
+      m_argv[size++] = xstrdup (other.m_argv[argi]);
+    m_argv[size] = nullptr;
+  }
+
   /* The iterator type.  */
 
   typedef char **iterator;
-- 
2.17.2

Re: [PATCH 1/6] Remove some manual memory management from compile interface

[Simon Marchi]

> diff --git a/gdb/compile/compile-cplus-types.c b/gdb/compile/compile-cplus-types.c
> index 02df7ab90e6..00eaaf3fa68 100644
> --- a/gdb/compile/compile-cplus-types.c
> +++ b/gdb/compile/compile-cplus-types.c
> @@ -848,33 +848,33 @@ compile_cplus_convert_struct_or_union (compile_cplus_instance *instance,
>    gcc_type result;
>    if (type->code () == TYPE_CODE_STRUCT)
>      {
> -      struct gcc_vbase_array bases;
>        int num_baseclasses = TYPE_N_BASECLASSES (type);
> +      std::vector<gcc_type> elements (num_baseclasses);
> +      std::vector<enum gcc_cp_symbol_kind> flags (num_baseclasses);
>  
> -      memset (&bases, 0, sizeof (bases));
> +      struct gcc_vbase_array bases {};
> +      bases.elements = elements.data ();
>  
>        if (num_baseclasses > 0)

You could now remove this `if (num_baseclasses > 0)`, there's no reason for it anymore I think.

Otherwise, LGTM.

Simon

Re: [PP?] [PATCH 2/6] Use new/delete for do_module_cleanup

[Simon Marchi]

On 2020-08-09 9:52 a.m., Tom Tromey wrote:
> This changes do_module_cleanup to use new and delete.  It also removes
> the use of the struct hack from this object -- this requires more
> allocations for now, but this will be removed in a subsequent patch.
> 
> gdb/ChangeLog
> 2020-08-08  Tom Tromey  <tom@tromey.com>
> 
> 	* compile/compile-object-run.c (struct do_module_cleanup): Add
> 	constructor, destructor.
> 	<objfile_name_string>: Don't use struct hack.
> 	(do_module_cleanup): Use delete.
> 	(compile_object_run): Use new.
> ---
>  gdb/ChangeLog                    |  8 ++++++++
>  gdb/compile/compile-object-run.c | 24 ++++++++++++++++--------
>  2 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/compile/compile-object-run.c b/gdb/compile/compile-object-run.c
> index a2f39900053..4a18655d488 100644
> --- a/gdb/compile/compile-object-run.c
> +++ b/gdb/compile/compile-object-run.c
> @@ -32,6 +32,17 @@
>  
>  struct do_module_cleanup
>  {
> +  do_module_cleanup () = default;
> +
> +  ~do_module_cleanup ()
> +  {
> +    delete munmap_list_head;
> +    xfree (source_file);
> +    xfree (objfile_name_string);
> +  }
> +
> +  DISABLE_COPY_AND_ASSIGN (do_module_cleanup);
> +

Seeing this makes me a bit nervous, as it easily allows the destructor run on an object
whose fields have not been initialized.  So it will call delete/xfree on uninitialized
pointers.  So I'd rather introduce a constructor before introducing a destructor, or
simultaneously.

The next patches touch that code, so I presume that will change.  But I read patches in
a very linear way, so that is my comment at this intermediary point :).

Simon

Re: [PP?] [PATCH 4/6] Transfer module ownership to do_module_cleanup

[Simon Marchi]

On 2020-08-09 9:52 a.m., Tom Tromey wrote:
> This changes the do_module_cleanup structure to simply hold on to the
> module itself.  This lets us remove most members from
> do_module_cleanup.
> 
> gdb/ChangeLog
> 2020-08-08  Tom Tromey  <tom@tromey.com>
> 
> 	* compile/compile-object-run.c (struct do_module_cleanup): Add
> 	parameters to constructor.  Update destructor.
> 	<source_file, scope, scope_data, out_value_type, out_value_addr,
> 	munmap_list_head, objfile_name_string>: Remove.
> 	<module>: New member.
> 	(do_module_cleanup): Update.
> 	(compile_object_run): Update.
> ---
>  gdb/ChangeLog                    | 10 +++++
>  gdb/compile/compile-object-run.c | 67 ++++++++++++--------------------
>  2 files changed, 35 insertions(+), 42 deletions(-)
> 
> diff --git a/gdb/compile/compile-object-run.c b/gdb/compile/compile-object-run.c
> index 5a680a6723f..ac0a995fee9 100644
> --- a/gdb/compile/compile-object-run.c
> +++ b/gdb/compile/compile-object-run.c
> @@ -32,13 +32,16 @@
>  
>  struct do_module_cleanup
>  {
> -  do_module_cleanup () = default;
> +  do_module_cleanup (int *ptr, compile_module_up &&mod)
> +    : executedp (ptr),
> +      module (std::move (mod))
> +  {
> +  }
>  
>    ~do_module_cleanup ()
>    {
> -    delete munmap_list_head;
> -    xfree (source_file);
> -    xfree (objfile_name_string);
> +    delete module->munmap_list_head;
> +    xfree (module->source_file);
>    }
>  
>    DISABLE_COPY_AND_ASSIGN (do_module_cleanup);
> @@ -47,22 +50,8 @@ struct do_module_cleanup
>       The pointer may be NULL.  */
>    int *executedp;
>  
> -  /* .c file OBJFILE was built from.  It needs to be xfree-d.  */
> -  char *source_file;
> -
> -  /* Copy from struct compile_module.  */
> -  enum compile_i_scope_types scope;
> -  void *scope_data;
> -
> -  /* Copy from struct compile_module.  */
> -  struct type *out_value_type;
> -  CORE_ADDR out_value_addr;
> -
> -  /* Copy from struct compile_module.  */
> -  struct munmap_list *munmap_list_head;
> -
> -  /* objfile_name of our objfile.  */
> -  char *objfile_name_string;
> +  /* The compile module.  */
> +  compile_module_up module;
>  };
>  
>  /* Cleanup everything after the inferior function dummy frame gets
> @@ -80,22 +69,26 @@ do_module_cleanup (void *arg, int registers_valid)
>  
>        /* This code cannot be in compile_object_run as OUT_VALUE_TYPE
>  	 no longer exists there.  */
> -      if (data->scope == COMPILE_I_PRINT_ADDRESS_SCOPE
> -	  || data->scope == COMPILE_I_PRINT_VALUE_SCOPE)
> +      if (data->module->scope == COMPILE_I_PRINT_ADDRESS_SCOPE
> +	  || data->module->scope == COMPILE_I_PRINT_VALUE_SCOPE)
>  	{
>  	  struct value *addr_value;
> -	  struct type *ptr_type = lookup_pointer_type (data->out_value_type);
> +	  struct type *ptr_type
> +	    = lookup_pointer_type (data->module->out_value_type);
>  
> -	  addr_value = value_from_pointer (ptr_type, data->out_value_addr);
> +	  addr_value = value_from_pointer (ptr_type,
> +					   data->module->out_value_addr);
>  
>  	  /* SCOPE_DATA would be stale unless EXECUTEDP != NULL.  */
> -	  compile_print_value (value_ind (addr_value), data->scope_data);
> +	  compile_print_value (value_ind (addr_value),
> +			       data->module->scope_data);
>  	}
>      }
>  
> +  const char *objfile_name_s = objfile_name (data->module->objfile);
>    for (objfile *objfile : current_program_space->objfiles ())
>      if ((objfile->flags & OBJF_USERLOADED) == 0
> -        && (strcmp (objfile_name (objfile), data->objfile_name_string) == 0))
> +        && (strcmp (objfile_name (objfile), objfile_name_s) == 0))
>        {
>  	objfile->unlink ();
>  
> @@ -106,10 +99,10 @@ do_module_cleanup (void *arg, int registers_valid)
>        }
>  
>    /* Delete the .c file.  */
> -  unlink (data->source_file);
> +  unlink (data->module->source_file);
>  
>    /* Delete the .o file.  */
> -  unlink (data->objfile_name_string);
> +  unlink (objfile_name_s);

The objfile gets destroyed when unlinked.  Is it guaranteed that objfile_name_s is still alive at that point?

Simon

Re: [PATCH 1/6] Remove some manual memory management from compile interface

[Tom Tromey]

Simon> You could now remove this `if (num_baseclasses > 0)`, there's no
Simon> reason for it anymore I think.

I looked at this more deeply, and based on the compiler library, I think
the rule is that a 0-length array should be passed as nullptr here:

  status
  marshall (connection *conn, const gcc_vbase_array *a)
  {
    size_t len;

    if (a)
      len = a->n_elements;
    else
      len = (size_t)-1;

    if (!marshall_array_start (conn, 'v', len))
      return FAIL;

    if (!a)
      return OK;
   [...]


So, I changed this code to use your suggestion and then also modified
the final call:

      result = instance->plugin ().start_class_type
	(name.get (), resuld, num_baseclasses > 0 ? &bases : nullptr,
	 filename, line);

This didn't result in any test result changes, but I didn't look to see
if this case is not tested or something like that.

Tom

Re: [PATCH 1/6] Remove some manual memory management from compile interface

[Simon Marchi]

On 2020-09-19 7:45 p.m., Tom Tromey wrote:
> Simon> You could now remove this `if (num_baseclasses > 0)`, there's no
> Simon> reason for it anymore I think.
>
> I looked at this more deeply, and based on the compiler library, I think
> the rule is that a 0-length array should be passed as nullptr here:
>
>   status
>   marshall (connection *conn, const gcc_vbase_array *a)
>   {
>     size_t len;
>
>     if (a)
>       len = a->n_elements;
>     else
>       len = (size_t)-1;
>
>     if (!marshall_array_start (conn, 'v', len))
>       return FAIL;
>
>     if (!a)
>       return OK;
>    [...]
>
>
> So, I changed this code to use your suggestion and then also modified
> the final call:
>
>       result = instance->plugin ().start_class_type
> 	(name.get (), resuld, num_baseclasses > 0 ? &bases : nullptr,
> 	 filename, line);
>
> This didn't result in any test result changes, but I didn't look to see
> if this case is not tested or something like that.

Oh, I didn't realize that the change I proposed would change how we call
the function.  What you ended up doing makes sense, if that's what's the
compiler library expects.

Simon

Re: [PATCH 1/6] Remove some manual memory management from compile interface

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> This didn't result in any test result changes, but I didn't look to see
>> if this case is not tested or something like that.

Simon> Oh, I didn't realize that the change I proposed would change how we call
Simon> the function.  What you ended up doing makes sense, if that's what's the
Simon> compiler library expects.

I dug even further and found out that the distinction doesn't matter on
the remote end:

    static tree
    start_class_def (tree type,
                     const gcc_vbase_array *base_classes)
    {
      tree bases = NULL;
      if (base_classes)
        {
          for (int i = 0; i < base_classes->n_elements; i++)
            {

So, I am going to change it back to your suggested approach and check it
in.

Tom

Re: [PP?] [PATCH 2/6] Use new/delete for do_module_cleanup

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> struct do_module_cleanup
>> {
>> +  do_module_cleanup () = default;
>> +
>> +  ~do_module_cleanup ()
>> +  {
>> +    delete munmap_list_head;
>> +    xfree (source_file);
>> +    xfree (objfile_name_string);
>> +  }
>> +
>> +  DISABLE_COPY_AND_ASSIGN (do_module_cleanup);
>> +

Simon> Seeing this makes me a bit nervous, as it easily allows the destructor run on an object
Simon> whose fields have not been initialized.  So it will call delete/xfree on uninitialized
Simon> pointers.  So I'd rather introduce a constructor before introducing a destructor, or
Simon> simultaneously.

Makes sense.  Because this gets reworked in later patches, I've added
initializers to these fields in this patch.  That will avoid the
possibly-uninitialized problem here.

Tom

Re: [PP?] [PATCH 4/6] Transfer module ownership to do_module_cleanup

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> /* Delete the .o file.  */
>> -  unlink (data->objfile_name_string);
>> +  unlink (objfile_name_s);

Simon> The objfile gets destroyed when unlinked.  Is it guaranteed that
Simon> objfile_name_s is still alive at that point?

Thanks for noticing this.  I've changed it to copy the name locally, and
added a comment explaining why.

Tom