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

Alan Hayward
2017-05-24T09:08:02Z

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 5754 bytes --]

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

\x16º&Öéj×!zÊÞ¶êç×\x7fvçÙb²Ö«r\x18\x1dn–­r\x17¬