* [PATCH 1/6] Remove some manual memory management from compile interface
2020-08-09 13:52 [PATCH 0/6] Avoid manual memory management in gdb/compile/ Tom Tromey
@ 2020-08-09 13:52 ` Tom Tromey
2020-08-09 22:34 ` Simon Marchi
2020-08-09 13:52 ` [PATCH 2/6] Use new/delete for do_module_cleanup Tom Tromey
` (4 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Tom Tromey @ 2020-08-09 13:52 UTC (permalink / raw)
To: gdb-patches; +Cc: 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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/6] Remove some manual memory management from compile interface
2020-08-09 13:52 ` [PATCH 1/6] Remove some manual memory management from compile interface Tom Tromey
@ 2020-08-09 22:34 ` Simon Marchi
2020-09-19 23:45 ` Tom Tromey
0 siblings, 1 reply; 15+ messages in thread
From: Simon Marchi @ 2020-08-09 22:34 UTC (permalink / raw)
To: Tom Tromey, gdb-patches
> 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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/6] Remove some manual memory management from compile interface
2020-08-09 22:34 ` Simon Marchi
@ 2020-09-19 23:45 ` Tom Tromey
2020-09-19 23:52 ` Simon Marchi
0 siblings, 1 reply; 15+ messages in thread
From: Tom Tromey @ 2020-09-19 23:45 UTC (permalink / raw)
To: Simon Marchi; +Cc: Tom Tromey, gdb-patches
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/6] Remove some manual memory management from compile interface
2020-09-19 23:45 ` Tom Tromey
@ 2020-09-19 23:52 ` Simon Marchi
2020-09-23 12:56 ` Tom Tromey
0 siblings, 1 reply; 15+ messages in thread
From: Simon Marchi @ 2020-09-19 23:52 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/6] Remove some manual memory management from compile interface
2020-09-19 23:52 ` Simon Marchi
@ 2020-09-23 12:56 ` Tom Tromey
0 siblings, 0 replies; 15+ messages in thread
From: Tom Tromey @ 2020-09-23 12:56 UTC (permalink / raw)
To: Simon Marchi; +Cc: Tom Tromey, gdb-patches
>>>>> "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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/6] Use new/delete for do_module_cleanup
2020-08-09 13:52 [PATCH 0/6] Avoid manual memory management in gdb/compile/ Tom Tromey
2020-08-09 13:52 ` [PATCH 1/6] Remove some manual memory management from compile interface Tom Tromey
@ 2020-08-09 13:52 ` Tom Tromey
2020-08-09 22:45 ` [PP?] " Simon Marchi
2020-08-09 13:52 ` [PATCH 3/6] Introduce and use compile_module_up Tom Tromey
` (3 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Tom Tromey @ 2020-08-09 13:52 UTC (permalink / raw)
To: gdb-patches; +Cc: 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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PP?] [PATCH 2/6] Use new/delete for do_module_cleanup
2020-08-09 13:52 ` [PATCH 2/6] Use new/delete for do_module_cleanup Tom Tromey
@ 2020-08-09 22:45 ` Simon Marchi
2020-09-23 13:36 ` Tom Tromey
0 siblings, 1 reply; 15+ messages in thread
From: Simon Marchi @ 2020-08-09 22:45 UTC (permalink / raw)
To: Tom Tromey, gdb-patches
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PP?] [PATCH 2/6] Use new/delete for do_module_cleanup
2020-08-09 22:45 ` [PP?] " Simon Marchi
@ 2020-09-23 13:36 ` Tom Tromey
0 siblings, 0 replies; 15+ messages in thread
From: Tom Tromey @ 2020-09-23 13:36 UTC (permalink / raw)
To: Simon Marchi; +Cc: Tom Tromey, gdb-patches
>>>>> "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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/6] Introduce and use compile_module_up
2020-08-09 13:52 [PATCH 0/6] Avoid manual memory management in gdb/compile/ Tom Tromey
2020-08-09 13:52 ` [PATCH 1/6] Remove some manual memory management from compile interface Tom Tromey
2020-08-09 13:52 ` [PATCH 2/6] Use new/delete for do_module_cleanup Tom Tromey
@ 2020-08-09 13:52 ` Tom Tromey
2020-08-09 13:52 ` [PATCH 4/6] Transfer module ownership to do_module_cleanup Tom Tromey
` (2 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: Tom Tromey @ 2020-08-09 13:52 UTC (permalink / raw)
To: gdb-patches; +Cc: 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
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 4/6] Transfer module ownership to do_module_cleanup
2020-08-09 13:52 [PATCH 0/6] Avoid manual memory management in gdb/compile/ Tom Tromey
` (2 preceding siblings ...)
2020-08-09 13:52 ` [PATCH 3/6] Introduce and use compile_module_up Tom Tromey
@ 2020-08-09 13:52 ` Tom Tromey
2020-08-10 0:48 ` [PP?] " Simon Marchi
2020-08-09 13:52 ` [PATCH 5/6] Simplify compile_module cleanup Tom Tromey
2020-08-09 13:52 ` [PATCH 6/6] Avoid manual memory management of argv arrays in gdb/compile Tom Tromey
5 siblings, 1 reply; 15+ messages in thread
From: Tom Tromey @ 2020-08-09 13:52 UTC (permalink / raw)
To: gdb-patches; +Cc: 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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PP?] [PATCH 4/6] Transfer module ownership to do_module_cleanup
2020-08-09 13:52 ` [PATCH 4/6] Transfer module ownership to do_module_cleanup Tom Tromey
@ 2020-08-10 0:48 ` Simon Marchi
2020-09-23 14:55 ` Tom Tromey
0 siblings, 1 reply; 15+ messages in thread
From: Simon Marchi @ 2020-08-10 0:48 UTC (permalink / raw)
To: Tom Tromey, gdb-patches
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PP?] [PATCH 4/6] Transfer module ownership to do_module_cleanup
2020-08-10 0:48 ` [PP?] " Simon Marchi
@ 2020-09-23 14:55 ` Tom Tromey
0 siblings, 0 replies; 15+ messages in thread
From: Tom Tromey @ 2020-09-23 14:55 UTC (permalink / raw)
To: Simon Marchi; +Cc: Tom Tromey, gdb-patches
>>>>> "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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 5/6] Simplify compile_module cleanup
2020-08-09 13:52 [PATCH 0/6] Avoid manual memory management in gdb/compile/ Tom Tromey
` (3 preceding siblings ...)
2020-08-09 13:52 ` [PATCH 4/6] Transfer module ownership to do_module_cleanup Tom Tromey
@ 2020-08-09 13:52 ` Tom Tromey
2020-08-09 13:52 ` [PATCH 6/6] Avoid manual memory management of argv arrays in gdb/compile Tom Tromey
5 siblings, 0 replies; 15+ messages in thread
From: Tom Tromey @ 2020-08-09 13:52 UTC (permalink / raw)
To: gdb-patches; +Cc: 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
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 6/6] Avoid manual memory management of argv arrays in gdb/compile
2020-08-09 13:52 [PATCH 0/6] Avoid manual memory management in gdb/compile/ Tom Tromey
` (4 preceding siblings ...)
2020-08-09 13:52 ` [PATCH 5/6] Simplify compile_module cleanup Tom Tromey
@ 2020-08-09 13:52 ` Tom Tromey
5 siblings, 0 replies; 15+ messages in thread
From: Tom Tromey @ 2020-08-09 13:52 UTC (permalink / raw)
To: gdb-patches; +Cc: 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
^ permalink raw reply [flat|nested] 15+ messages in thread