From: Alan Hayward <Alan.Hayward@arm.com>
To: Pedro Alves <palves@redhat.com>
Cc: Yao Qi <qiyaoltc@gmail.com>,
"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>,
nd <nd@arm.com>
Subject: Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)
Date: Wed, 24 May 2017 09:08:00 -0000 [thread overview]
Message-ID: <B0DE7D70-F835-40F1-A7BE-8AC978C00092@arm.com> (raw)
In-Reply-To: <e12a3bb2-c70f-0148-999d-d83c39077e50@redhat.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
>
next prev parent reply other threads:[~2017-05-24 9:08 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-04 10:12 [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE Alan Hayward
2017-04-04 17:19 ` John Baldwin
2017-04-05 10:27 ` Alan Hayward
2017-04-11 15:37 ` Yao Qi
2017-05-05 8:03 ` [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (1/4) Alan Hayward
2017-05-05 21:44 ` Yao Qi
2017-05-05 8:04 ` [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (4/4) Alan Hayward
2017-06-07 8:31 ` Alan Hayward
2017-06-08 20:27 ` Maciej W. Rozycki
2017-06-09 10:31 ` Alan Hayward
2017-06-09 11:03 ` Pedro Alves
2017-06-09 11:48 ` Maciej W. Rozycki
2017-06-09 12:05 ` Pedro Alves
2017-06-09 13:23 ` Maciej W. Rozycki
2017-06-09 14:29 ` Pedro Alves
2017-06-12 9:09 ` Alan Hayward
2017-06-12 18:11 ` Pedro Alves
2017-06-12 14:29 ` Maciej W. Rozycki
2017-05-05 8:04 ` [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4) Alan Hayward
2017-05-05 19:51 ` John Baldwin
2017-05-12 8:53 ` Yao Qi
2017-05-16 11:16 ` Alan Hayward
2017-05-22 12:07 ` Yao Qi
2017-05-22 16:05 ` Alan Hayward
2017-05-22 17:15 ` Pedro Alves
2017-05-23 17:49 ` Alan Hayward
2017-05-23 18:30 ` Pedro Alves
2017-05-24 9:08 ` Alan Hayward [this message]
2017-05-24 9:20 ` Pedro Alves
2017-05-24 10:20 ` Alan Hayward
2017-05-24 11:07 ` Pedro Alves
2017-05-24 19:45 ` Alan Hayward
2017-05-25 10:46 ` Pedro Alves
2017-05-25 11:43 ` Yao Qi
2017-05-25 11:48 ` Pedro Alves
2017-05-26 8:54 ` Alan Hayward
2017-05-26 10:26 ` Pedro Alves
2017-05-26 15:30 ` Alan Hayward
2017-05-26 15:49 ` Pedro Alves
2017-05-26 16:18 ` Alan Hayward
2017-05-26 16:00 ` John Baldwin
2017-05-24 9:29 ` Pedro Alves
2017-05-05 8:04 ` [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (3/4) Alan Hayward
2017-05-05 21:54 ` Yao Qi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=B0DE7D70-F835-40F1-A7BE-8AC978C00092@arm.com \
--to=alan.hayward@arm.com \
--cc=gdb-patches@sourceware.org \
--cc=nd@arm.com \
--cc=palves@redhat.com \
--cc=qiyaoltc@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).