[2/2] GDB: S12Z: new function s12z_extract_return_value

  Make gdb aware of the return values of functions which
return in registers.

gdb/ChangeLog:
* s12z-tdep.c (s12z_extract_return_value): New function.
  (inv_reg_perm) New array.
  (s12z_return_value): Populate readbuf if non-null.
---
 gdb/s12z-tdep.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 61 insertions(+), 1 deletion(-)
  

Hi John,

See my comments inline with your patch, below.

On Mon, 12 Nov 2018 10:17:21 +0100
John Darrington <john@darrington.wattle.id.au> wrote:

> Make gdb aware of the return values of functions which
> return in registers.
> 
> gdb/ChangeLog:
> * s12z-tdep.c (s12z_extract_return_value): New function.
>   (inv_reg_perm) New array.
>   (s12z_return_value): Populate readbuf if non-null.

Make sure that this is indented correctly when it eventually goes
in the ChangeLog file.

> diff --git a/gdb/s12z-tdep.c b/gdb/s12z-tdep.c
> index bd0bd7c001..3da88523f7 100644
> --- a/gdb/s12z-tdep.c
> +++ b/gdb/s12z-tdep.c
> @@ -37,7 +37,9 @@
>  #define N_PHYSICAL_REGISTERS (S12Z_N_REGISTERS - 2)
>  
>  
> -/*  A permutation of all the physical registers.  */
> +/*  A permutation of all the physical registers.   Indexing this array
> +    with an integer from gdb's internal representation will return the
> +    register enum.  */
>  static const int reg_perm[N_PHYSICAL_REGISTERS] =
>    {
>     REG_D0,
> @@ -55,6 +57,16 @@ static const int reg_perm[N_PHYSICAL_REGISTERS] =
>     REG_CCW
>    };
>  
> +/*  The inverse of the above permutation.  Indexing this
> +    array with a register enum (e.g. REG_D2) will return the register
> +    number in gdb's internal representation.  */
> +static const int inv_reg_perm[N_PHYSICAL_REGISTERS] =
> +  {
> +   2, 3, 4, 5,      /* d2, d3, d4, d5 */
> +   0, 1,            /* d0, d1 */
> +   6, 7,            /* d6, d7 */
> +   8, 9, 10, 11, 12 /* x, y, s, p, ccw */
> +  };

My two cents on all of the above...

I think you'll have a lot less grief with this architecture port if
you don't try to use the numbering defined in include/opcode/s12z.h. 
Create a new numbering with new constants for GDB's purposes ordered
as shown in the reg_perm array.  Then use these constants in place of
the various REG_ constants that are currently in s12z-tdep.c.  If you
still want to be able to access registers[], it may make sense to
have an array which maps GDB's constants to those in include/opcode.

Also, if you want CCH and CCL to be show in "info registers" and/or
allow the user to display and set them, these can be implemented via
the use of pseudo-registers.

>  /*  Return the name of the register REGNUM.  */
>  static const char *
> @@ -467,11 +479,59 @@ s12z_print_registers_info (struct gdbarch *gdbarch,
>  
>  
>  
> +
> +static void
> +s12z_extract_return_value (struct type *type, struct regcache *regcache,
> +                              void *valbuf)
> +{
> +  int reg = -1;
> +
> +  gdb_byte buf[4];
> +
> +  switch (TYPE_LENGTH (type))
> +    {
> +    case 0:   /* Nothing to do */
> +      return;
> +
> +    case 1:
> +      reg = REG_D0;
> +      break;
> +
> +    case 2:
> +      reg = REG_D2;
> +      break;
> +
> +    case 3:
> +      reg = REG_X;
> +      break;
> +
> +    case 4:
> +      reg = REG_D6;
> +      break;
> +
> +    default:
> +      error (_("bad size for return value"));
> +      return;
> +    }
> +
> +  regcache->cooked_read (inv_reg_perm[reg], buf);
> +  memcpy (valbuf, buf, TYPE_LENGTH (type));

Is there any reason not to just pass valbuf in place of buf to
cooked_read?  Doing so will get rid of the memcpy.

Kevin
  

Hi Kevin,

Thanks for the review.

On Sat, Nov 17, 2018 at 11:35:50PM -0700, Kevin Buettner wrote:
     > 
     > gdb/ChangeLog:
     > * s12z-tdep.c (s12z_extract_return_value): New function.
     >   (inv_reg_perm) New array.
     >   (s12z_return_value): Populate readbuf if non-null.
     
     Make sure that this is indented correctly when it eventually goes
     in the ChangeLog file.

Is it documented anywhere what "correctly" means?
     
     My two cents on all of the above...
     
     I think you'll have a lot less grief with this architecture port if
     you don't try to use the numbering defined in include/opcode/s12z.h. 
     Create a new numbering with new constants for GDB's purposes ordered
     as shown in the reg_perm array.  Then use these constants in place of
     the various REG_ constants that are currently in s12z-tdep.c.  If you
     still want to be able to access registers[], it may make sense to
     have an array which maps GDB's constants to those in include/opcode.

I'm beginning to think that you're right here.  I may change it in the
way you propose in a future patch.
     
     Also, if you want CCH and CCL to be show in "info registers" and/or
     allow the user to display and set them, these can be implemented via
     the use of pseudo-registers.

A the moment, I don't think it's worth bothering about.  CCH and CCL are
merely the high and low bytes of a 16bit register CCW.   The names CCH
and CCL only exist because of two (rarely used) instructions in the ISA.
     
     >  /*  Return the name of the register REGNUM.  */
     >  static const char *
     > @@ -467,11 +479,59 @@ s12z_print_registers_info (struct gdbarch *gdbarch,
     >  
     >  
     >  
     > +
     > +static void
     > +s12z_extract_return_value (struct type *type, struct regcache *regcache,
     > +                              void *valbuf)
     > +{
     > +  int reg = -1;
     > +
     > +  gdb_byte buf[4];
     > +
     > +  switch (TYPE_LENGTH (type))
     > +    {
     > +    case 0:   /* Nothing to do */
     > +      return;
     > +
     > +    case 1:
     > +      reg = REG_D0;
     > +      break;
     > +
     > +    case 2:
     > +      reg = REG_D2;
     > +      break;
     > +
     > +    case 3:
     > +      reg = REG_X;
     > +      break;
     > +
     > +    case 4:
     > +      reg = REG_D6;
     > +      break;
     > +
     > +    default:
     > +      error (_("bad size for return value"));
     > +      return;
     > +    }
     > +
     > +  regcache->cooked_read (inv_reg_perm[reg], buf);
     > +  memcpy (valbuf, buf, TYPE_LENGTH (type));
     
     Is there any reason not to just pass valbuf in place of buf to
     cooked_read?  Doing so will get rid of the memcpy.

Probably not.  Thanks for noticing this.
     
J'
  

On Sun, 18 Nov 2018 10:13:37 +0100
John Darrington <john@darrington.wattle.id.au> wrote:

> Hi Kevin,
> 
> Thanks for the review.
> 
> On Sat, Nov 17, 2018 at 11:35:50PM -0700, Kevin Buettner wrote:
>      > 
>      > gdb/ChangeLog:
>      > * s12z-tdep.c (s12z_extract_return_value): New function.
>      >   (inv_reg_perm) New array.
>      >   (s12z_return_value): Populate readbuf if non-null.  
>      
>      Make sure that this is indented correctly when it eventually goes
>      in the ChangeLog file.
> 
> Is it documented anywhere what "correctly" means?

https://www.gnu.org/prep/standards/standards.html#Change-Logs
https://www.gnu.org/software/emacs/manual/html_node/emacs/Format-of-ChangeLog.html

Though with regards to indentation, neither of those links adequately
describe the ChangeLog entries that we've been using for many years
now.

When you do the commit / push, your ChangeLog entry should look something
like this:

YYYY-MM-DD  John Darrington  <john@darrington.wattle.id.au>

	* s12z-tdep.c (s12z_extract_return_value): New function.
	(inv_reg_perm) New array.
	(s12z_return_value): Populate readbuf if non-null.  

In particular, a single tab precedes the non-header (and non-empty)
portions of the ChangeLog entry.

Kevin