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

[Alan Hayward <Alan dot Hayward at arm dot com>]

Thanks for the review. Updated as below and pushed.


> On 6 Jun 2018, at 23:17, Simon Marchi <simon.marchi@ericsson.com> wrote:
> 
> 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.

Updated to use AARCH64_V_REGS_NUM.

I originally used 32 because I was matching the style of the code above which
uses 32. Agreed in this case makes more sense to use AARCH64_V_REGS_NUM.

Didn’t add the func, as here we’re checking against AARCH64_SVE_V0_REGNUM
and not AARCH64_V0_REGNUM.

> 
>> /* 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.

Not changed.

Make sense, but regcache is already being passed into aarch64_pseudo_write (which
the function type is defined in gdbarch). Easier to just pass it straight down
given the compiler would have already have put gdbarch in a register.

> 
>> {
>> -  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.
> 

Updated to gdb_static_assert. Left both of them in the code at the same place (there’s
one in the read func and one in the write func).

I wanted something explicit in this function because these are (I think) the only
two functions that rely on the defines being the same.


Thanks,
Alan.