[2/6] Use new/delete for do_module_cleanup

Message ID 20200809135258.8207-3-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 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(-)

-- 
2.17.2

Comments

Simon Marchi Aug. 9, 2020, 10:45 p.m. | #1
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
Tom Tromey Sept. 23, 2020, 1:36 p.m. | #2
>>>>> "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

Patch

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;