[v6,1/5] tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p

  From: Franck Jullien <franck.jullien@gmail.com>

tdesc_register_in_reggroup_p in now able to handle arbitrary
groups. This is useful when groups are created while the
target descriptor file is received from the remote.

This can be the case of a soft core target processor where
registers/groups can change.

gdb/ChangeLog:

2013-02-15  Franck Jullien  <franck.jullien@gmail.com>

	* target-descriptions.c (tdesc_register_in_reggroup_p): Support
	arbitrary strings.
---
 gdb/target-descriptions.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
  

Stafford Horne <shorne@gmail.com> writes:

> tdesc_register_in_reggroup_p in now able to handle arbitrary
> groups. This is useful when groups are created while the
> target descriptor file is received from the remote.
>
> This can be the case of a soft core target processor where
> registers/groups can change.
>
> gdb/ChangeLog:
>
> 2013-02-15  Franck Jullien  <franck.jullien@gmail.com>
>
> 	* target-descriptions.c (tdesc_register_in_reggroup_p): Support
> 	arbitrary strings.

I am not sure what do you want to change and why do you need this
change?  I didn't see it in the previous version.

If you want to control what registers are displayed, you can follow the
way nds32 used, see nds32-tdep.c:nds32_register_reggroup_p.
  

On Tue, May 02, 2017 at 03:40:20PM +0100, Yao Qi wrote:
> Stafford Horne <shorne@gmail.com> writes:
> 
> > tdesc_register_in_reggroup_p in now able to handle arbitrary
> > groups. This is useful when groups are created while the
> > target descriptor file is received from the remote.
> >
> > This can be the case of a soft core target processor where
> > registers/groups can change.
> >
> > gdb/ChangeLog:
> >
> > 2013-02-15  Franck Jullien  <franck.jullien@gmail.com>
> >
> > 	* target-descriptions.c (tdesc_register_in_reggroup_p): Support
> > 	arbitrary strings.
> 
> I am not sure what do you want to change and why do you need this
> change?  I didn't see it in the previous version.
> 
> If you want to control what registers are displayed, you can follow the
> way nds32 used, see nds32-tdep.c:nds32_register_reggroup_p.

Hi Yao,

Thanks for reviewing.  Sorry, this was in previous versions but dropped on
accident then added back, I forgot to mention.

Currenly tdesc_register_in_reggroup_p() only returns true if the register
group number is one of the hard coded reggroups: float, vector, general,
save or restor.

This change is to also allow returning true in the case that the register
group was registered with gdb with reggroup_add().  This seems like
something that will be generally required so it was added to the
target-descriptions definition.  If you think not I can make it for
openrisc only, but I think it makes more sense in tdesc.

This allows the command like, the below to return the registers for the
named reggroup.

  info reg system

-Stafford
  

Stafford Horne <shorne@gmail.com> writes:

> This change is to also allow returning true in the case that the register
> group was registered with gdb with reggroup_add().  This seems like
> something that will be generally required so it was added to the
> target-descriptions definition.  If you think not I can make it for
> openrisc only, but I think it makes more sense in tdesc.

Thanks for the explanation.

>
> This allows the command like, the below to return the registers for the
> named reggroup.
>
>   info reg system

Some bits are missing,

 - A test case, in which we can define some reggroups "foo" and test
   expected "foo" is shown in the output of "maintenance print
   reggroups".  You can add reggroups in gdb.xml/extra-regs.xml.

 - A test case with live inferior, get a list of support reggroups from
   the output of "maint print reggroups", pass each reggroup to "info
   reg $group", and test there is no error.

 - Document "info registers".
   https://sourceware.org/gdb/current/onlinedocs/gdb/Registers.html
   doesn't document the usage "info reg GROUP".  We need to add it.

 - Add a news entry, because your patch adds a user visible change.
  

Stafford Horne <shorne@gmail.com> writes:
>  
>        if (reggroup == general_reggroup)
>  	return general_p;
> +
> +      if (strcmp (reg->group, reggroup_name (reggroup)) == 0)
> +	return 1;
> +
>      }

I read the patch again, and find that we can replace the whole block in
"if (reg != NULL && reg->group != NULL)" with the code this patch adds,
like this,

  if (reg != NULL && reg->group != NULL
      && (strcmp (reg->group, reggroup_name (reggroup)) == 0))
     return 1;
  

On Tue, May 09, 2017 at 03:21:43PM +0100, Yao Qi wrote:
> Stafford Horne <shorne@gmail.com> writes:
> >  
> >        if (reggroup == general_reggroup)
> >  	return general_p;
> > +
> > +      if (strcmp (reg->group, reggroup_name (reggroup)) == 0)
> > +	return 1;
> > +
> >      }
> 
> I read the patch again, and find that we can replace the whole block in
> "if (reg != NULL && reg->group != NULL)" with the code this patch adds,
> like this,
> 
>   if (reg != NULL && reg->group != NULL
>       && (strcmp (reg->group, reggroup_name (reggroup)) == 0))
>      return 1;

Thanks,  I was thinking to simply to this as well,  but I left the patch I
inheherited as is.  I will also update the commit log and comment to make
it more clear what is going on.

> -- 
> Yao (齐尧)
  

On Tue, May 09, 2017 at 02:54:01PM +0100, Yao Qi wrote:
> Stafford Horne <shorne@gmail.com> writes:
> 
> > This change is to also allow returning true in the case that the register
> > group was registered with gdb with reggroup_add().  This seems like
> > something that will be generally required so it was added to the
> > target-descriptions definition.  If you think not I can make it for
> > openrisc only, but I think it makes more sense in tdesc.
> 
> Thanks for the explanation.
> 
> >
> > This allows the command like, the below to return the registers for the
> > named reggroup.
> >
> >   info reg system
> 
> Some bits are missing,
> 
>  - A test case, in which we can define some reggroups "foo" and test
>    expected "foo" is shown in the output of "maintenance print
>    reggroups".  You can add reggroups in gdb.xml/extra-regs.xml.
> 
>  - A test case with live inferior, get a list of support reggroups from
>    the output of "maint print reggroups", pass each reggroup to "info
>    reg $group", and test there is no error.
> 
>  - Document "info registers".
>    https://sourceware.org/gdb/current/onlinedocs/gdb/Registers.html
>    doesn't document the usage "info reg GROUP".  We need to add it.
> 
>  - Add a news entry, because your patch adds a user visible change.

Right,

I am working on this, I have most of it done.  However, I am thinking to
submit this separately from the OpenRISC port.

There are more changes required to get this to work as you mention in the
(1st) test case above.  Currently tdesc supplied reggroups are not added by
default, they need to be manually supplied by calling reggroup_add().  I
have made a patch to make this automatic.

As mentioned I will split this out separate from the OpenRISC patch and do
all that is mentioned above.

-Stafford