Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[Pedro Alves <palves@redhat.com>]

On 05/23/2017 06:49 PM, Alan Hayward wrote:
> 
>> On 22 May 2017, at 18:15, Pedro Alves <palves@redhat.com> wrote:
>> I wonder whether we can get rid of the LONGEST / host integer
>> middleman and simplify things while at it.  For instance, what if we
>> had a version of raw_collect that took the destination buffer length
>> as parameter:
>>
>>      regcache->raw_collect_integer (regnum, (gdb_byte *) addr, len);
>>
>> that would copy bytes over into addr, and if the register is
>> narrower than LEN, then it'd insert the necessary
>> leading zeros (or 0xFFs if signed extension necessary),
>> and if the registers is wider than LEN, then it'd skip
>> copying enough significant bytes so that LEN fits.
>>
>> Likewise for regcache->raw_supply.
> 
> Is it the case that gdb always does a store_integer after a raw_collect
> of a (U)LONGEST?
> And always an extract_integer before a raw_supply of a (U)LONGEST ?
> (Both of these are tricky to grep for, because the code sequence is over
> multiple lines)

The observation here is that we're transferring integer data between
two places that seemingly both use target formatting:

 target integer 1 -> host integer -> target integer 2

when the target integer 1 and 2 have the same sizes, then we
copy data directly without the host integer middle man.
But when they don't have the same size, we do the host integer
conversion steps.  I was questioning having that step in the
first place.  If the target integers have different sizes,
we'll either end up with zero/sign extension, or truncation.
It seems to me offhand that doing those directly in the destination
buffer shouldn't be difficult?

> 
> I was going to mock up a new raw_collect_integer, but then got carried
> away and wrote the full patch changes.
> This version makes the MIPS files look neater.

Hmm, I think you may have misunderstood.  The main point was to
avoid having to have T/LONGEST temporary at all here:

> +template<typename T>
> +typename std::enable_if<(std::is_same<T, LONGEST>::value
> +			 || std::is_same<T, ULONGEST>::value),
> +			void>::type
> +regcache::raw_supply_integer (int regnum, const gdb_byte *addr, int addr_len)
> +{
> +  enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
> +  gdb_byte *regbuf;
> +  size_t regsize;
> +  T val;
> +
> +  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
> +  gdb_assert (!m_readonly_p);
> +
> +  regbuf = register_buffer (regnum);
> +  regsize = m_descr->sizeof_register[regnum];
> +
> +  val = extract_integer<T> (addr, addr_len, byte_order);
> +  store_integer (regbuf, regsize, byte_order, val);
> +  m_register_status[regnum] = REG_VALID;

and maybe the need for all the templating.

I.e., in the cases at hand, both the regcache buffer an
the ADDR/LEN buffer are in target format, with the mismatch
being in integer width (i.e., may need zero/sign extension or
truncation), so it seems to me that we should be able to copy
data to/from the register buffer directly, without having to
convert to host format (the T / LONGEST) as an intermediate step.

So basically if we start with that you have, what we'd need
is a version of store_integer that takes a ADDR/LEN pair
instead of a T val.

Thanks,
Pedro Alves