Re: [PATCH 3/3] tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p

[Stafford Horne <shorne@gmail.com>]

On Thu, Jun 08, 2017 at 10:52:01PM +0200, Simon Marchi wrote:
> Hi Stafford,
> 
> On 2017-06-08 00:15, Stafford Horne wrote:
> > @@ -1081,17 +1080,13 @@ tdesc_remote_register_number (struct gdbarch
> > *gdbarch, int regno)
> >  }
> > 
> >  /* Check whether REGNUM is a member of REGGROUP.  Registers from the
> > -   target description may be classified as general, float, or vector.
> > -   Unlike a gdbarch register_reggroup_p method, this function will
> > -   return -1 if it does not know; the caller should handle registers
> > -   with no specified group.
> > -
> > -   Arbitrary strings (other than "general", "float", and "vector")
> > -   from the description are not used; they cause the register to be
> > -   displayed in "info all-registers" but excluded from "info
> > -   registers" et al.  The names of containing features are also not
> > -   used.  This might be extended to display registers in some more
> > -   useful groupings.
> > +   target description may be classified as general, float, vector or
> > other
> > +   register groups registerd with reggroup_add().  Unlike a gdbarch
> 
> s/registerd/registered/

Doh, I seem to never be able to spell register.

> > @@ -1231,6 +1209,27 @@ tdesc_use_registers (struct gdbarch *gdbarch,
> >  	void **slot = htab_find_slot (reg_hash, reg, INSERT);
> > 
> >  	*slot = reg;
> > +	/* Add reggroup if its new.  */
> > +	if (reg->group != NULL)
> > +	  {
> > +	    struct reggroup *group;
> > +	    bool group_exists = false;
> > +
> > +	    for (group = reggroup_next (gdbarch, NULL);
> > +		 group != NULL;
> > +		 group = reggroup_next (gdbarch, group))
> > +	      {
> > +		if (strcmp (reg->group, reggroup_name (group)) == 0)
> > +		  {
> > +		    group_exists = true;
> > +		    break;
> > +		  }
> > +	      }
> > +
> > +	    if (!group_exists)
> > +	      reggroup_add (gdbarch, reggroup_new (reg->group,
> > +						   USER_REGGROUP));
> 
> Is the memory allocated by reggroup_new ever freed?

It is not, and its a bit tricky.  I looked through gdb, it seems the
reggroup objects are never freed, anywhere.  The list itself and list
elements (reggroup_el) are all allocated on obstack, but not reggroup.

It could get a bit messy to try to do something about it like refcounts or
tracking reggroups per target description.

Any suggestions?

-Stafford