[3/4] Use std::vector in solib-target lm_info
Commit Message
Replace the two VEC fields with std::vector.
I found only one place where these lm_infos were allocated, but two
where they are freed. It looks like solib_target_free_so missed freeing
section_bases before.
More c++ification is obviously possible, but my goal right now is to get
rid of VEC (CORE_ADDR).
I wasn't really able to test this, since the list of remote targets that use
this method of fetching solibs is quite limited (windows, dicos and
arm-symbian, from what I can see).
gdb/ChangeLog:
* solib-target.c (struct lm_info): Add default constructor,
delete copy constructor and operator=.
<segment_bases, section_bases>: Change type to
std::vector<CORE_ADDR>.
(library_list_start_segment, library_list_start_section,
library_list_start_library, library_list_end_library,
solib_target_free_library_list, solib_target_free_so,
solib_target_relocate_section_addresses): Adjust.
---
gdb/solib-target.c | 49 ++++++++++++++++++++++---------------------------
1 file changed, 22 insertions(+), 27 deletions(-)
Comments
On 04/16/2017 03:14 PM, Simon Marchi wrote:
> Replace the two VEC fields with std::vector.
>
> I found only one place where these lm_infos were allocated, but two
> where they are freed. It looks like solib_target_free_so missed freeing
> section_bases before.
>
> More c++ification is obviously possible, but my goal right now is to get
> rid of VEC (CORE_ADDR).
>
> I wasn't really able to test this, since the list of remote targets that use
> this method of fetching solibs is quite limited (windows, dicos and
> arm-symbian, from what I can see).
Other random "bare metal" / RTOSs that GDB doesn't need to know about
use solib-target too. It's the default solib implementation exactly
to allow for GDB to remain agnostic about them. That doesn't help
you with testing, it's just a FYI.
> /* Handle the start of a <library> element. */
> @@ -119,7 +124,7 @@ library_list_start_library (struct gdb_xml_parser *parser,
> void *user_data, VEC(gdb_xml_value_s) *attributes)
> {
> VEC(lm_info_p) **list = (VEC(lm_info_p) **) user_data;
> - struct lm_info *item = XCNEW (struct lm_info);
Note this was an XCNEW, which means that it zeroed all
memory. Did you check whether all fields are initialized
after the patch?
> + struct lm_info *item = new lm_info;
> const char *name
> = (const char *) xml_find_attribute (attributes, "name")->value;
>
> @@ -135,10 +140,8 @@ library_list_end_library (struct gdb_xml_parser *parser,
> VEC(lm_info_p) **list = (VEC(lm_info_p) **) user_data;
> struct lm_info *lm_info = VEC_last (lm_info_p, *list);
>
> - if (lm_info->segment_bases == NULL
> - && lm_info->section_bases == NULL)
> - gdb_xml_error (parser,
> - _("No segment or section bases defined"));
> + if (lm_info->segment_bases.empty () && lm_info->section_bases.empty ())
> + gdb_xml_error (parser, _("No segment or section bases defined"));
> }
>
>
> @@ -175,9 +178,7 @@ solib_target_free_library_list (void *p)
> for (ix = 0; VEC_iterate (lm_info_p, *result, ix, info); ix++)
> {
> xfree (info->name);
> - VEC_free (CORE_ADDR, info->segment_bases);
> - VEC_free (CORE_ADDR, info->section_bases);
> - xfree (info);
> + delete info;
As a general principle, I'd rather move all destruction bits to
the destructor at the same time when we C++fy a struct.
It doesn't really complicate the patch, while not doing it
makes it easier to leave these bits missed behind in a
random follow up patch that adds a dtor.
I.e., above, I'd prefer to move the xfree to a dtor in
the same patch.
> }
> VEC_free (lm_info_p, *result);
> *result = NULL;
> @@ -326,8 +327,7 @@ solib_target_free_so (struct so_list *so)
> {
> gdb_assert (so->lm_info->name == NULL);
> xfree (so->lm_info->offsets);
> - VEC_free (CORE_ADDR, so->lm_info->segment_bases);
> - xfree (so->lm_info);
> + delete so->lm_info;
Ditto.
Thanks,
Pedro Alves
On 2017-04-18 16:18, Pedro Alves wrote:
> On 04/16/2017 03:14 PM, Simon Marchi wrote:
>> Replace the two VEC fields with std::vector.
>>
>> I found only one place where these lm_infos were allocated, but two
>> where they are freed. It looks like solib_target_free_so missed
>> freeing
>> section_bases before.
>>
>> More c++ification is obviously possible, but my goal right now is to
>> get
>> rid of VEC (CORE_ADDR).
>>
>> I wasn't really able to test this, since the list of remote targets
>> that use
>> this method of fetching solibs is quite limited (windows, dicos and
>> arm-symbian, from what I can see).
>
> Other random "bare metal" / RTOSs that GDB doesn't need to know about
> use solib-target too. It's the default solib implementation exactly
> to allow for GDB to remain agnostic about them. That doesn't help
> you with testing, it's just a FYI.
Ok, thanks for the info.
>> /* Handle the start of a <library> element. */
>> @@ -119,7 +124,7 @@ library_list_start_library (struct gdb_xml_parser
>> *parser,
>> void *user_data, VEC(gdb_xml_value_s) *attributes)
>> {
>> VEC(lm_info_p) **list = (VEC(lm_info_p) **) user_data;
>> - struct lm_info *item = XCNEW (struct lm_info);
>
> Note this was an XCNEW, which means that it zeroed all
> memory. Did you check whether all fields are initialized
> after the patch?
Err you're right. I'll add default initializers in the class, e.g.:
char *name {};
>> + struct lm_info *item = new lm_info;
>> const char *name
>> = (const char *) xml_find_attribute (attributes, "name")->value;
>>
>> @@ -135,10 +140,8 @@ library_list_end_library (struct gdb_xml_parser
>> *parser,
>> VEC(lm_info_p) **list = (VEC(lm_info_p) **) user_data;
>> struct lm_info *lm_info = VEC_last (lm_info_p, *list);
>>
>> - if (lm_info->segment_bases == NULL
>> - && lm_info->section_bases == NULL)
>> - gdb_xml_error (parser,
>> - _("No segment or section bases defined"));
>> + if (lm_info->segment_bases.empty () && lm_info->section_bases.empty
>> ())
>> + gdb_xml_error (parser, _("No segment or section bases defined"));
>> }
>>
>>
>> @@ -175,9 +178,7 @@ solib_target_free_library_list (void *p)
>> for (ix = 0; VEC_iterate (lm_info_p, *result, ix, info); ix++)
>> {
>> xfree (info->name);
>> - VEC_free (CORE_ADDR, info->segment_bases);
>> - VEC_free (CORE_ADDR, info->section_bases);
>> - xfree (info);
>> + delete info;
>
> As a general principle, I'd rather move all destruction bits to
> the destructor at the same time when we C++fy a struct.
> It doesn't really complicate the patch, while not doing it
> makes it easier to leave these bits missed behind in a
> random follow up patch that adds a dtor.
>
> I.e., above, I'd prefer to move the xfree to a dtor in
> the same patch.
It makes sense.
>> }
>> VEC_free (lm_info_p, *result);
>> *result = NULL;
>> @@ -326,8 +327,7 @@ solib_target_free_so (struct so_list *so)
>> {
>> gdb_assert (so->lm_info->name == NULL);
>> xfree (so->lm_info->offsets);
>> - VEC_free (CORE_ADDR, so->lm_info->segment_bases);
>> - xfree (so->lm_info);
>> + delete so->lm_info;
>
> Ditto.
Ok.
I'll send a v2 in not too long.
Thanks,
Simon
Something else I realized is that these lm_info structures
are all defined differently in the different solib-*.c files.
It was OK in C, but in C++, that's an ODR violation
("-flto -Wodr" will tell you about them).
I think that as long as these types are PODs, it isn't a
problem in practice, exactly due to C compatibility.
But as soon as we add non-trivial ctors/dtors (with external linkage),
then I think we're likely to run into problems as the linker picks
just one of the (different) types' ctor/dtor definitions assuming
they're all the same.
#1 - One way around it would be to make "struct lm_info" an
empty struct
struct lm_info {};
in solist.h, instead of an opaque struct:
struct lm_info;
and then make all lm_info implementations inherit from that
instead of redefining it. You then need to add casts from
lm_info * to the appropriate type within each of the
solib-*.c implementations.
The downside is that this requires changing every port at the
same time, otherwise the solib-*.c files won't compile due to
lm_info redefinitions.
#2 - An alternative way, is to instead keep struct lm_info opaque,
and rename each solib-*.c lm_info's name. E.g., do:
-struct lm_info
+struct solib_target_lm_info
And then add the casts between "solib_target_lm_info *"
and "lm_info *" in each solib-*.c as we convert them.
So you'd just do it in solib-target.c for now. This is more
casts than the other approach (likely just one or two where
the lm_info object is constructed), because you now need to
cast in the solib_target_lm_info -> lm_info direction too.
However, if we changed so_list::lm_info to "void *" [1], we
would avoid those casts and only need the
lm_info -> solib_target_lm_info casts, as in the other approach.
[1] - the type is already opaque to common code so we'd not
really be adding any type hole at all.
#3 - Yet another approach is to get rid of the lm_info type and
replace it with inheriting the so_list type directly:
struct solib_target_lib : so_list
{
// old lm_info fields go here.
};
This is I suspect, the cleanest. It is already the responsibility
of the solib-*.c backend to create/destroy so_list objects.
It could also be done incrementally, I think. We'd have to start by
touching all solib-*.c files to use new/delete instead of malloc/free
to allocate so_list objects, but after that, I think we could
progress one backend at a time as we find a need...
Thanks,
Pedro Alves
@@ -25,10 +25,15 @@
#include "target.h"
#include "vec.h"
#include "solib-target.h"
+#include <vector>
/* Private data for each loaded library. */
struct lm_info
{
+ lm_info () = default;
+ lm_info (const lm_info &) = delete;
+ lm_info &operator= (const lm_info &) = delete;
+
/* The library's name. The name is normally kept in the struct
so_list; it is only here during XML parsing. */
char *name;
@@ -38,11 +43,11 @@ struct lm_info
/* The base addresses for each independently relocatable segment of
this shared library. */
- VEC(CORE_ADDR) *segment_bases;
+ std::vector<CORE_ADDR> segment_bases;
/* The base addresses for each independently allocatable,
relocatable section of this shared library. */
- VEC(CORE_ADDR) *section_bases;
+ std::vector<CORE_ADDR> section_bases;
/* The cached offsets for each section of this shared library,
determined from SEGMENT_BASES, or SECTION_BASES. */
@@ -86,11 +91,11 @@ library_list_start_segment (struct gdb_xml_parser *parser,
= (ULONGEST *) xml_find_attribute (attributes, "address")->value;
CORE_ADDR address = (CORE_ADDR) *address_p;
- if (last->section_bases != NULL)
+ if (!last->section_bases.empty ())
gdb_xml_error (parser,
_("Library list with both segments and sections"));
- VEC_safe_push (CORE_ADDR, last->segment_bases, address);
+ last->segment_bases.push_back (address);
}
static void
@@ -104,11 +109,11 @@ library_list_start_section (struct gdb_xml_parser *parser,
= (ULONGEST *) xml_find_attribute (attributes, "address")->value;
CORE_ADDR address = (CORE_ADDR) *address_p;
- if (last->segment_bases != NULL)
+ if (!last->segment_bases.empty ())
gdb_xml_error (parser,
_("Library list with both segments and sections"));
- VEC_safe_push (CORE_ADDR, last->section_bases, address);
+ last->section_bases.push_back (address);
}
/* Handle the start of a <library> element. */
@@ -119,7 +124,7 @@ library_list_start_library (struct gdb_xml_parser *parser,
void *user_data, VEC(gdb_xml_value_s) *attributes)
{
VEC(lm_info_p) **list = (VEC(lm_info_p) **) user_data;
- struct lm_info *item = XCNEW (struct lm_info);
+ struct lm_info *item = new lm_info;
const char *name
= (const char *) xml_find_attribute (attributes, "name")->value;
@@ -135,10 +140,8 @@ library_list_end_library (struct gdb_xml_parser *parser,
VEC(lm_info_p) **list = (VEC(lm_info_p) **) user_data;
struct lm_info *lm_info = VEC_last (lm_info_p, *list);
- if (lm_info->segment_bases == NULL
- && lm_info->section_bases == NULL)
- gdb_xml_error (parser,
- _("No segment or section bases defined"));
+ if (lm_info->segment_bases.empty () && lm_info->section_bases.empty ())
+ gdb_xml_error (parser, _("No segment or section bases defined"));
}
@@ -175,9 +178,7 @@ solib_target_free_library_list (void *p)
for (ix = 0; VEC_iterate (lm_info_p, *result, ix, info); ix++)
{
xfree (info->name);
- VEC_free (CORE_ADDR, info->segment_bases);
- VEC_free (CORE_ADDR, info->section_bases);
- xfree (info);
+ delete info;
}
VEC_free (lm_info_p, *result);
*result = NULL;
@@ -326,8 +327,7 @@ solib_target_free_so (struct so_list *so)
{
gdb_assert (so->lm_info->name == NULL);
xfree (so->lm_info->offsets);
- VEC_free (CORE_ADDR, so->lm_info->segment_bases);
- xfree (so->lm_info);
+ delete so->lm_info;
}
static void
@@ -346,12 +346,11 @@ solib_target_relocate_section_addresses (struct so_list *so,
= ((struct section_offsets *)
xzalloc (SIZEOF_N_SECTION_OFFSETS (num_sections)));
- if (so->lm_info->section_bases)
+ if (!so->lm_info->section_bases.empty ())
{
int i;
asection *sect;
- int num_section_bases
- = VEC_length (CORE_ADDR, so->lm_info->section_bases);
+ int num_section_bases = so->lm_info->section_bases.size ();
int num_alloc_sections = 0;
for (i = 0, sect = so->abfd->sections;
@@ -368,10 +367,7 @@ Could not relocate shared library \"%s\": wrong number of ALLOC sections"),
{
int bases_index = 0;
int found_range = 0;
- CORE_ADDR *section_bases;
-
- section_bases = VEC_address (CORE_ADDR,
- so->lm_info->section_bases);
+ CORE_ADDR *section_bases = so->lm_info->section_bases.data ();
so->addr_low = ~(CORE_ADDR) 0;
so->addr_high = 0;
@@ -404,7 +400,7 @@ Could not relocate shared library \"%s\": wrong number of ALLOC sections"),
gdb_assert (so->addr_low <= so->addr_high);
}
}
- else if (so->lm_info->segment_bases)
+ else if (!so->lm_info->segment_bases.empty ())
{
struct symfile_segment_data *data;
@@ -419,9 +415,8 @@ Could not relocate shared library \"%s\": no segments"), so->so_name);
int num_bases;
CORE_ADDR *segment_bases;
- num_bases = VEC_length (CORE_ADDR, so->lm_info->segment_bases);
- segment_bases = VEC_address (CORE_ADDR,
- so->lm_info->segment_bases);
+ num_bases = so->lm_info->segment_bases.size ();
+ segment_bases = so->lm_info->segment_bases.data ();
if (!symfile_map_offsets_to_segments (so->abfd, data,
so->lm_info->offsets,