Re: [PATCH v3] Support large registers in regcache transfer_regset

[Alan Hayward <Alan.Hayward@arm.com>]

> On 22 Jun 2018, at 15:25, Simon Marchi <simon.marchi@ericsson.com> wrote:
> 
> Hi Alan,
> 
> The patch LGTM.  I noted two small comments you might want to address before
> pushing.  Sorry again for the extra work I made you do!

No probs!

> 
> On 2018-06-22 09:08 AM, Alan Hayward wrote:
>> @@ -868,6 +900,39 @@ regcache::write_part (int regnum, int offset, int len,
>> 
>> /* See regcache.h.  */
>> 
>> +void
>> +reg_buffer::raw_supply_part (int regnum, int offset, int len,
>> +			     const gdb_byte *in)
>> +{
>> +  int reg_size = register_size (arch (), regnum);
>> +
>> +  gdb_assert (in != nullptr);
>> +  gdb_assert (offset >= 0 && offset <= reg_size);
>> +  gdb_assert (len >= 0 && offset + len <= reg_size);
>> +
>> +  if (offset == 0 && len == 0)
>> +    {
>> +      /* Nothing to do.  */
>> +      return;
>> +    }
>> +
>> +  if (offset == 0 && len == reg_size)
>> +    {
>> +      /* Supply the full register.  */
>> +      return raw_supply (regnum, in);
>> +    }
>> +
>> +  gdb_byte *reg = (gdb_byte *) alloca (reg_size);
>> +
>> +  /* Read when needed.  */
>> +  if (offset > 0 || offset + len < reg_size)
> 
> I think this if is unneeded now, with the earlier check?  If we reach
> this point, it will always be for a partial register.

Yes. I had removed the same check from raw_collect_part, but didn’t
spot this.

> 
>> diff --git a/gdb/regcache.h b/gdb/regcache.h
>> index 983137f6ad..e706d87b54 100644
>> --- a/gdb/regcache.h
>> +++ b/gdb/regcache.h
>> @@ -166,6 +166,10 @@ public:
>>   void raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,
>> 			    bool is_signed) const;
>> 
>> +  /* Collect register REGNUM from REGCACHE, starting at offset in REGCACHE,
>> +     reading only LEN.  */
> 
> I think "starting at offset in REGCACHE" is wrong, you probably meant
> "starting at offset in the register" or something like that?

Done.

> 
>> +  void raw_collect_part (int regnum, int offset, int len, gdb_byte *out) const;
>> +
>>   /* See common/common-regcache.h.  */
>>   void raw_supply (int regnum, const void *buf) override;
>> 
>> @@ -187,6 +191,10 @@ public:
>>      unavailable).  */
>>   void raw_supply_zeroed (int regnum);
>> 
>> +  /* Supply register REGNUM to REGCACHE, starting at offset in REGCACHE, writing
>> +     only LEN, without editing the rest of the register.  */
> 
> Here too.
> 

Done.


Pushed with changes as suggested.


Thanks,
Alan.