[1/2] Remove munmap_listp_free_cleanup
Commit Message
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(-)
Comments
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
>>>>> "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
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
>>>>> "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
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
@@ -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;
}
@@ -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
{
@@ -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);