Re: [PATCH v2 01/10] Aarch64 SVE pseudo register support

[Simon Marchi <simon.marchi@ericsson.com>]

Hi Alan,

I have some more nits/suggestions, feel free to cherry-pick the ones you
want and push the result.

On 2018-06-06 11:16 AM, Alan Hayward wrote:
> @@ -2243,6 +2297,9 @@ aarch64_pseudo_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
>      return group == all_reggroup || group == vector_reggroup;
>    else if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
>      return group == all_reggroup || group == vector_reggroup;
> +  else if (tdep->has_sve () && regnum >= AARCH64_SVE_V0_REGNUM
> +	   && regnum < AARCH64_SVE_V0_REGNUM + 32)

Here you use the magical "32" number but in aarch64_pseudo_register_name you used
AARCH64_V_REGS_NUM to refer (I think) to the same number.  Would it be good to use
AARCH64_V_REGS_NUM everywhere?  It might be good to extract that condition in a
function:

static bool
is_sve_regnum (int regnum)
{
  return (regnum >= AARCH64_SVE_V0_REGNUM
	  && regnum < AARCH64_SVE_V0_REGNUM + AARCH64_V_REGS_NUM);
}

and use it throughout.

>  /* Helper for aarch64_pseudo_write.  */
>  
>  static void
> -aarch64_pseudo_write_1 (struct regcache *regcache, int regnum_offset,
> -			int regsize, const gdb_byte *buf)
> +aarch64_pseudo_write_1 (struct gdbarch *gdbarch, struct regcache *regcache,
> +			int regnum_offset, int regsize, const gdb_byte *buf)

You could use the gdbarch from regcache.

>  {
> -  gdb_byte reg_buf[V_REGISTER_SIZE];
>    unsigned v_regnum = AARCH64_V0_REGNUM + regnum_offset;
>  
> +  /* Enough space for a full vector register.  */
> +  gdb_byte reg_buf[register_size (gdbarch, AARCH64_V0_REGNUM)];
> +  gdb_assert (AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM);

This is checking a static assertion.  You could use static_assert instead, which
produces a compilation error if false.  You can either leave it here or put it
next to the aarch64_regnum definition.  I have seen the same gdb_assert somewhere
else too, with a static_assert you only need one.

Thanks,

Simon