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

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

> On 23 May 2017, at 19:30, Pedro Alves <palves@redhat.com> wrote:
> 
> 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.
> 

Would still need to have eater signed/unsigned versions of the functions,
or maybe have “bool signed” parameter.


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

Are you suggesting something like this:
(Warning - this snippet may not even compile, and I’m not sure on the endian
logic)


void
regcache::raw_supply_integer (int regnum, const gdb_byte *addr, int addr_len,
			      bool is_signed)
{
  enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
  void *regbuf;
  size_t regsize;

  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
  gdb_assert (!m_readonly_p);
  gdb_assert (addr != 0);
  gdb_assert (addr_len <= regsize);

  regbuf = register_buffer (regnum);
  regsize = m_descr->sizeof_register[regnum];

  copy_and_fill_to_size (regbuf, addr, addr_len, regsize, is_signed, byte_order);
  m_register_status[regnum] = REG_VALID;
}

/* Copy COPY_LEN bytes from SOURCE to DEST, then sign extend or zero extend
   to FILL_LEN bytes.  */
void copy_and_fill_to_size (const gdb_byte *dest, const gdb_byte *source,
			    int copy_len, int fill_len, bool is_signed,
			    enum bfd_endian byte_order)
{
  signed int len_diff = fill_len - copy_len;
  gdb_assert (len_diff >= 0);

  if (byte_order == BFD_ENDIAN_BIG)
    memcpy (dest+len_diff, source, copy_len);
  else
    memcpy (dest, source, copy_len);

  if (len_diff > 0)
    {
      if (signed)
      	// TODO: sign extend from copy_len to fill_len
      else
	{
	  if (byte_order == BFD_ENDIAN_BIG)
      	    memset (dest, 0, len_diff);
      	  else
      	    memset (dest+copy_len, 0, len_diff);
	}
    }
}


Meanwhile raw_collect_integer doesn’t need a signed parameter:

void
regcache::raw_collect_integer (int regnum, gdb_byte *addr, int addr_len) const
{
  enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
  const gdb_byte *regbuf;
  size_t regsize;

  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
  gdb_assert (addr_len <= regsize);

  regbuf = register_buffer (regnum);
  regsize = m_descr->sizeof_register[regnum];

  if (byte_order == BFD_ENDIAN_BIG)
    regbuf += regsize - addr_len;

  memcpy (addr, regbuf, addr_len);
}



> Thanks,
> Pedro Alves
>