[PATCH 1/2] Remove munmap_listp_free_cleanup

[PATCH 1/2] Remove munmap_listp_free_cleanup

[Tom Tromey]

This removes munmap_listp_free_cleanup, replacing it with a
std::unique_ptr at one spot and an explicit delete in another.  It
seemed simplest to completely change this data structure.

gdb/ChangeLog
2018-09-15  Tom Tromey  <tom@tromey.com>

	* compile/compile-object-run.c (do_module_cleanup): Use delete.
	* compile/compile-object-load.c (struct munmap_list): Move to
	header file.
	(munmap_list::add): Rename from munmap_list_add; rewrite.
	(munmap_list::~munmap_list): Rename from munmap_list_free.
	(munmap_listp_free_cleanup): Remove.
	(compile_object_load): Update.
	* compile/compile-object-load.h (struct munmap_list): Move from
	compile-object-load.c.  Rewrite.n
---
 gdb/ChangeLog                     | 12 +++++
 gdb/compile/compile-object-load.c | 88 ++++++++-----------------------
 gdb/compile/compile-object-load.h | 25 ++++++++-
 gdb/compile/compile-object-run.c  |  2 +-
 4 files changed, 59 insertions(+), 68 deletions(-)

diff --git a/gdb/compile/compile-object-load.c b/gdb/compile/compile-object-load.c
index 40053d281a1..db792792327 100644
--- a/gdb/compile/compile-object-load.c
+++ b/gdb/compile/compile-object-load.c
@@ -34,56 +34,22 @@
 #include "arch-utils.h"
 #include <algorithm>
 
-/* Track inferior memory reserved by inferior mmap.  */
-
-struct munmap_list
-{
-  struct munmap_list *next;
-  CORE_ADDR addr, size;
-};
-
-/* Add inferior mmap memory range ADDR..ADDR+SIZE (exclusive) to list
-   HEADP.  *HEADP needs to be initialized to NULL.  */
-
-static void
-munmap_list_add (struct munmap_list **headp, CORE_ADDR addr, CORE_ADDR size)
-{
-  struct munmap_list *head_new = XNEW (struct munmap_list);
-
-  head_new->next = *headp;
-  *headp = head_new;
-  head_new->addr = addr;
-  head_new->size = size;
-}
-
-/* Free list of inferior mmap memory ranges HEAD.  HEAD is the first
-   element of the list, it can be NULL.  After calling this function
-   HEAD pointer is invalid and the possible list needs to be
-   reinitialized by caller to NULL.  */
+/* Add inferior mmap memory range ADDR..ADDR+SIZE (exclusive) to the
+   list.  */
 
 void
-munmap_list_free (struct munmap_list *head)
+munmap_list::add (CORE_ADDR addr, CORE_ADDR size)
 {
-  while (head)
-    {
-      struct munmap_list *todo = head;
-
-      head = todo->next;
-      gdbarch_infcall_munmap (target_gdbarch (), todo->addr, todo->size);
-      xfree (todo);
-    }
+  struct munmap_item item = { addr, size };
+  items.push_front (item);
 }
 
-/* Stub for munmap_list_free suitable for make_cleanup.  Contrary to
-   munmap_list_free this function's parameter is a pointer to the first
-   list element pointer.  */
+/* Destroy an munmap_list.  */
 
-static void
-munmap_listp_free_cleanup (void *headp_voidp)
+munmap_list::~munmap_list ()
 {
-  struct munmap_list **headp = (struct munmap_list **) headp_voidp;
-
-  munmap_list_free (*headp);
+  for (auto &item : items)
+    gdbarch_infcall_munmap (target_gdbarch (), item.addr, item.size);
 }
 
 /* Helper data for setup_sections.  */
@@ -105,7 +71,7 @@ struct setup_sections_data
 
   /* List of inferior mmap ranges where setup_sections should add its
      next range.  */
-  struct munmap_list **munmap_list_headp;
+  std::unique_ptr<struct munmap_list> munmap_list;
 };
 
 /* Place all ABFD sections next to each other obeying all constraints.  */
@@ -155,7 +121,7 @@ setup_sections (bfd *abfd, asection *sect, void *data_voidp)
 	{
 	  addr = gdbarch_infcall_mmap (target_gdbarch (), data->last_size,
 				       data->last_prot);
-	  munmap_list_add (data->munmap_list_headp, addr, data->last_size);
+	  data->munmap_list->add (addr, data->last_size);
 	  if (compile_debug)
 	    fprintf_unfiltered (gdb_stdlog,
 				"allocated %s bytes at %s prot %u\n",
@@ -611,7 +577,6 @@ struct compile_module *
 compile_object_load (const compile_file_names &file_names,
 		     enum compile_i_scope_types scope, void *scope_data)
 {
-  struct cleanup *cleanups;
   struct setup_sections_data setup_sections_data;
   CORE_ADDR regs_addr, out_value_addr = 0;
   struct symbol *func_sym;
@@ -626,7 +591,6 @@ compile_object_load (const compile_file_names &file_names,
   struct objfile *objfile;
   int expect_parameters;
   struct type *expect_return_type;
-  struct munmap_list *munmap_list_head = NULL;
 
   gdb::unique_xmalloc_ptr<char> filename
     (tilde_expand (file_names.object_file ()));
@@ -648,15 +612,15 @@ compile_object_load (const compile_file_names &file_names,
   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_headp = &munmap_list_head;
-  cleanups = make_cleanup (munmap_listp_free_cleanup, &munmap_list_head);
+  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);
 
   storage_needed = bfd_get_symtab_upper_bound (abfd.get ());
   if (storage_needed < 0)
     error (_("Cannot read symbols of compiled module \"%s\": %s"),
-          filename.get (), bfd_errmsg (bfd_get_error ()));
+	   filename.get (), bfd_errmsg (bfd_get_error ()));
 
   /* SYMFILE_VERBOSE is not passed even if FROM_TTY, user is not interested in
      "Reading symbols from ..." message for automatically generated file.  */
@@ -703,8 +667,8 @@ compile_object_load (const compile_file_names &file_names,
 	   objfile_name (objfile));
   if (!types_deeply_equal (expect_return_type, TYPE_TARGET_TYPE (func_type)))
     error (_("Invalid return type of function \"%s\" in compiled "
-	    "module \"%s\"."),
-	  GCC_FE_WRAPPER_FUNCTION, objfile_name (objfile));
+	     "module \"%s\"."),
+	   GCC_FE_WRAPPER_FUNCTION, objfile_name (objfile));
 
   /* The memory may be later needed
      by bfd_generic_get_relocated_section_contents
@@ -714,7 +678,7 @@ compile_object_load (const compile_file_names &file_names,
   number_of_symbols = bfd_canonicalize_symtab (abfd.get (), symbol_table);
   if (number_of_symbols < 0)
     error (_("Cannot parse symbols of compiled module \"%s\": %s"),
-          filename.get (), bfd_errmsg (bfd_get_error ()));
+	   filename.get (), bfd_errmsg (bfd_get_error ()));
 
   missing_symbols = 0;
   for (symp = symbol_table; symp < symbol_table + number_of_symbols; symp++)
@@ -784,7 +748,7 @@ compile_object_load (const compile_file_names &file_names,
 					TYPE_LENGTH (regs_type),
 					GDB_MMAP_PROT_READ);
       gdb_assert (regs_addr != 0);
-      munmap_list_add (&munmap_list_head, 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",
@@ -799,18 +763,15 @@ compile_object_load (const compile_file_names &file_names,
     {
       out_value_type = get_out_value_type (func_sym, objfile, scope);
       if (out_value_type == NULL)
-	{
-	  do_cleanups (cleanups);
-	  return NULL;
-	}
+	return NULL;
       check_typedef (out_value_type);
       out_value_addr = gdbarch_infcall_mmap (target_gdbarch (),
 					     TYPE_LENGTH (out_value_type),
 					     (GDB_MMAP_PROT_READ
 					      | GDB_MMAP_PROT_WRITE));
       gdb_assert (out_value_addr != 0);
-      munmap_list_add (&munmap_list_head, 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",
@@ -828,12 +789,7 @@ compile_object_load (const compile_file_names &file_names,
   retval->scope_data = scope_data;
   retval->out_value_type = out_value_type;
   retval->out_value_addr = out_value_addr;
-
-  /* CLEANUPS will free MUNMAP_LIST_HEAD.  */
-  retval->munmap_list_head = munmap_list_head;
-  munmap_list_head = NULL;
-
-  do_cleanups (cleanups);
+  retval->munmap_list_head = setup_sections_data.munmap_list.release ();
 
   return retval;
 }
diff --git a/gdb/compile/compile-object-load.h b/gdb/compile/compile-object-load.h
index 6f62535878a..7569c42bf5c 100644
--- a/gdb/compile/compile-object-load.h
+++ b/gdb/compile/compile-object-load.h
@@ -18,8 +18,31 @@
 #define GDB_COMPILE_OBJECT_LOAD_H
 
 #include "compile-internal.h"
+#include <list>
 
-struct munmap_list;
+struct munmap_list
+{
+public:
+
+  munmap_list () = default;
+  ~munmap_list ();
+
+  DISABLE_COPY_AND_ASSIGN (munmap_list);
+
+  /* Add a region to the list.  */
+  void add (CORE_ADDR addr, CORE_ADDR size);
+
+private:
+
+  /* Track inferior memory reserved by inferior mmap.  */
+
+  struct munmap_item
+  {
+    CORE_ADDR addr, size;
+  };
+
+  std::list<munmap_item> items;
+};
 
 struct compile_module
 {
diff --git a/gdb/compile/compile-object-run.c b/gdb/compile/compile-object-run.c
index 0846c735fe2..f3ec932365e 100644
--- a/gdb/compile/compile-object-run.c
+++ b/gdb/compile/compile-object-run.c
@@ -99,7 +99,7 @@ do_module_cleanup (void *arg, int registers_valid)
   unlink (data->source_file);
   xfree (data->source_file);
 
-  munmap_list_free (data->munmap_list_head);
+  delete data->munmap_list_head;
 
   /* Delete the .o file.  */
   unlink (data->objfile_name_string);
-- 
2.17.1

[PATCH 0/2] remove the remaining cleanups from gdb/compile

[Tom Tromey]

This series removes the remaining cleanups from gdb/compile.

Tested by the buildbot.

Tom

[PATCH 2/2] Remove remaining cleanups from compile-object-load.c

[Tom Tromey]

This removes the remaining cleanups from compile-object-load.c.

gdb/ChangeLog
2018-09-15  Tom Tromey  <tom@tromey.com>

	* compile/compile-object-load.c (struct
	link_hash_table_cleanup_data): Add constructor and destructor.
	Use DISABLE_COPY_AND_ASSIGN.
	(~link_hash_table_cleanup_data): Rename from
	link_hash_table_free.  Now a destructor.
	(copy_sections): Use gdb::unique_xmalloc_ptr.  Remove cleanups.
---
 gdb/ChangeLog                     |  9 ++++++
 gdb/compile/compile-object-load.c | 51 +++++++++++++++----------------
 2 files changed, 34 insertions(+), 26 deletions(-)

diff --git a/gdb/compile/compile-object-load.c b/gdb/compile/compile-object-load.c
index db792792327..d1982cf51d4 100644
--- a/gdb/compile/compile-object-load.c
+++ b/gdb/compile/compile-object-load.c
@@ -282,22 +282,26 @@ static const struct bfd_link_callbacks link_callbacks =
 
 struct link_hash_table_cleanup_data
 {
-  bfd *abfd;
-  bfd *link_next;
-};
+  explicit link_hash_table_cleanup_data (bfd *abfd_)
+    : abfd (abfd_),
+      link_next (abfd->link.next)
+  {
+  }
 
-/* Cleanup callback for struct bfd_link_info.  */
+  ~link_hash_table_cleanup_data ()
+  {
+    if (abfd->is_linker_output)
+      (*abfd->link.hash->hash_table_free) (abfd);
+    abfd->link.next = link_next;
+  }
 
-static void
-link_hash_table_free (void *d)
-{
-  struct link_hash_table_cleanup_data *data
-    = (struct link_hash_table_cleanup_data *) d;
+  DISABLE_COPY_AND_ASSIGN (link_hash_table_cleanup_data);
 
-  if (data->abfd->is_linker_output)
-    (*data->abfd->link.hash->hash_table_free) (data->abfd);
-  data->abfd->link.next = data->link_next;
-}
+private:
+
+  bfd *abfd;
+  bfd *link_next;
+};
 
 /* Relocate and store into inferior memory each section SECT of ABFD.  */
 
@@ -305,12 +309,10 @@ static void
 copy_sections (bfd *abfd, asection *sect, void *data)
 {
   asymbol **symbol_table = (asymbol **) data;
-  bfd_byte *sect_data, *sect_data_got;
-  struct cleanup *cleanups;
+  bfd_byte *sect_data_got;
   struct bfd_link_info link_info;
   struct bfd_link_order link_order;
   CORE_ADDR inferior_addr;
-  struct link_hash_table_cleanup_data cleanup_data;
 
   if ((bfd_get_section_flags (abfd, sect) & (SEC_ALLOC | SEC_LOAD))
       != (SEC_ALLOC | SEC_LOAD))
@@ -326,13 +328,11 @@ copy_sections (bfd *abfd, asection *sect, void *data)
   link_info.input_bfds = abfd;
   link_info.input_bfds_tail = &abfd->link.next;
 
-  cleanup_data.abfd = abfd;
-  cleanup_data.link_next = abfd->link.next;
+  struct link_hash_table_cleanup_data cleanup_data (abfd);
 
   abfd->link.next = NULL;
   link_info.hash = bfd_link_hash_table_create (abfd);
 
-  cleanups = make_cleanup (link_hash_table_free, &cleanup_data);
   link_info.callbacks = &link_callbacks;
 
   memset (&link_order, 0, sizeof (link_order));
@@ -342,21 +342,22 @@ copy_sections (bfd *abfd, asection *sect, void *data)
   link_order.size = bfd_get_section_size (sect);
   link_order.u.indirect.section = sect;
 
-  sect_data = (bfd_byte *) xmalloc (bfd_get_section_size (sect));
-  make_cleanup (xfree, sect_data);
+  gdb::unique_xmalloc_ptr<gdb_byte> sect_data
+    ((bfd_byte *) xmalloc (bfd_get_section_size (sect)));
 
   sect_data_got = bfd_get_relocated_section_contents (abfd, &link_info,
-						      &link_order, sect_data,
+						      &link_order,
+						      sect_data.get (),
 						      FALSE, symbol_table);
 
   if (sect_data_got == NULL)
     error (_("Cannot map compiled module \"%s\" section \"%s\": %s"),
 	   bfd_get_filename (abfd), bfd_get_section_name (abfd, sect),
 	   bfd_errmsg (bfd_get_error ()));
-  gdb_assert (sect_data_got == sect_data);
+  gdb_assert (sect_data_got == sect_data.get ());
 
   inferior_addr = bfd_get_section_vma (abfd, sect);
-  if (0 != target_write_memory (inferior_addr, sect_data,
+  if (0 != target_write_memory (inferior_addr, sect_data.get (),
 				bfd_get_section_size (sect)))
     error (_("Cannot write compiled module \"%s\" section \"%s\" "
 	     "to inferior memory range %s-%s."),
@@ -364,8 +365,6 @@ copy_sections (bfd *abfd, asection *sect, void *data)
 	   paddress (target_gdbarch (), inferior_addr),
 	   paddress (target_gdbarch (),
 		     inferior_addr + bfd_get_section_size (sect)));
-
-  do_cleanups (cleanups);
 }
 
 /* Fetch the type of COMPILE_I_EXPR_PTR_TYPE and COMPILE_I_EXPR_VAL
-- 
2.17.1

Re: [PATCH 1/2] Remove munmap_listp_free_cleanup

[Simon Marchi]

On 2018-09-15 6:24 p.m., Tom Tromey wrote:
> diff --git a/gdb/compile/compile-object-load.h b/gdb/compile/compile-object-load.h
> index 6f62535878a..7569c42bf5c 100644
> --- a/gdb/compile/compile-object-load.h
> +++ b/gdb/compile/compile-object-load.h
> @@ -18,8 +18,31 @@
>  #define GDB_COMPILE_OBJECT_LOAD_H
>  
>  #include "compile-internal.h"
> +#include <list>
>  
> -struct munmap_list;
> +struct munmap_list
> +{
> +public:
> +
> +  munmap_list () = default;
> +  ~munmap_list ();
> +
> +  DISABLE_COPY_AND_ASSIGN (munmap_list);
> +
> +  /* Add a region to the list.  */
> +  void add (CORE_ADDR addr, CORE_ADDR size);
> +
> +private:
> +
> +  /* Track inferior memory reserved by inferior mmap.  */
> +
> +  struct munmap_item
> +  {
> +    CORE_ADDR addr, size;
> +  };
> +
> +  std::list<munmap_item> items;

Not a big deal, but we might as well use a vector.  The objects are cheap to copy, so there's probably less overhead overall (memory and time) to use a vector than a doubly linked list.

I was going to say:

1. Why not use munmap_list as directly as field of setup_sections_data, instead
   of a unique_ptr, and
2. std::move it to the compile_module object.

But that would require C++ifying compile_module and probably do_module_cleanup too,
and that's perhaps a too big of a step for this patch.

Simon

Re: [PATCH 2/2] Remove remaining cleanups from compile-object-load.c

[Simon Marchi]

On 2018-09-15 6:24 p.m., Tom Tromey wrote:
> This removes the remaining cleanups from compile-object-load.c.

LGTM.

Simon

Re: [PATCH 1/2] Remove munmap_listp_free_cleanup

[Tom Tromey]

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

Simon> Not a big deal, but we might as well use a vector.  The objects
Simon> are cheap to copy, so there's probably less overhead overall
Simon> (memory and time) to use a vector than a doubly linked list.

I made this change.

Simon> I was going to say:

Simon> 1. Why not use munmap_list as directly as field of setup_sections_data, instead
Simon>    of a unique_ptr, and
Simon> 2. std::move it to the compile_module object.

Simon> But that would require C++ifying compile_module and probably
Simon> do_module_cleanup too, and that's perhaps a too big of a step for
Simon> this patch.

Yeah, that's why I didn't do it.

Tom

Re: [PATCH 1/2] Remove munmap_listp_free_cleanup

[Pedro Alves]

On 09/15/2018 11:24 PM, Tom Tromey wrote:
> -static void
> -munmap_listp_free_cleanup (void *headp_voidp)
> +munmap_list::~munmap_list ()
>  {
> -  struct munmap_list **headp = (struct munmap_list **) headp_voidp;
> -
> -  munmap_list_free (*headp);
> +  for (auto &item : items)
> +    gdbarch_infcall_munmap (target_gdbarch (), item.addr, item.size);
>  }

I think we should wrap that in try/catch, because an infcall can be
aborted/throw, and we're in a destructor.

Thanks,
Pedro Alves

Re: [PATCH 1/2] Remove munmap_listp_free_cleanup

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> I think we should wrap that in try/catch, because an infcall can be
Pedro> aborted/throw, and we're in a destructor.

My latest version just ignores the exception.  It seemed to me that
there isn't much the user can do about this, so a warning would be
un-helpful.  If you agree, I'll push that.  Or, I could have it emit a
warning if you think that's worthwhile.

Tom

Re: [PATCH 1/2] Remove munmap_listp_free_cleanup

[Pedro Alves]

On 09/17/2018 10:21 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> I think we should wrap that in try/catch, because an infcall can be
> Pedro> aborted/throw, and we're in a destructor.
> 
> My latest version just ignores the exception.  It seemed to me that
> there isn't much the user can do about this, so a warning would be
> un-helpful.  If you agree, I'll push that.  Or, I could have it emit a
> warning if you think that's worthwhile.

I agree.

Thanks,
Pedro Alves