Re: [PATCH 3/4] Use std::vector in solib-target lm_info

[Simon Marchi <simon dot marchi at polymtl dot ca>]

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