[4/6] Transfer module ownership to do_module_cleanup

Message ID 20200809135258.8207-5-tom@tromey.com
State New
Headers show
Series
  • Avoid manual memory management in gdb/compile/
Related show

Commit Message

Tom Tromey Aug. 9, 2020, 1:52 p.m.
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(-)

-- 
2.17.2

Comments

Simon Marchi Aug. 10, 2020, 12:48 a.m. | #1
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
Tom Tromey Sept. 23, 2020, 2:55 p.m. | #2
>>>>> "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

Patch

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 ());