This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
extract_unsigned_integer API (Re: [PATCH] Remove MAX_REGISTER_SIZE from frame.c)
- From: Pedro Alves <palves at redhat dot com>
- To: Yao Qi <qiyaoltc at gmail dot com>, Alan Hayward <Alan dot Hayward at arm dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>, nd <nd at arm dot com>
- Date: Tue, 28 Mar 2017 15:08:51 +0100
- Subject: extract_unsigned_integer API (Re: [PATCH] Remove MAX_REGISTER_SIZE from frame.c)
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves at redhat dot com
- Dkim-filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 39DC97AEB8
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 39DC97AEB8
- References: <E80FFABA-2912-4223-AC55-2F4DE6055F47@arm.com> <86lgspqisk.fsf@gmail.com>
Hi Yao,
I didn't notice your patch/question until now. See below.
On 03/01/2017 12:32 PM, Yao Qi wrote:
> Alan Hayward <Alan.Hayward@arm.com> writes:
>
>> @@ -1252,7 +1252,11 @@ frame_unwind_register_signed (struct frame_info *frame, int regnum)
>> struct gdbarch *gdbarch = frame_unwind_arch (frame);
>> enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>> int size = register_size (gdbarch, regnum);
>> - gdb_byte buf[MAX_REGISTER_SIZE];
>> + gdb_byte buf[sizeof (LONGEST)];
>> +
>> + if (size > (int) sizeof (LONGEST))
>> + error (_("Cannot unwind integers more than %d bytes."),
>> + (int) sizeof (LONGEST));
>>
>
> We apply the restriction of extract_signed_integer to its caller here.
> People will wonder why do we have such check until he/she digs into
> extract_signed_integer. My first reaction is to add some comments to
> explain why do we do so, but the recent gdb::function_view reminds me
> that we can do something differently (and better, IMO).
>
> Current pattern of using extract_unsigned_integer is
>
> 1) allocate an array on stack,
> 2) read data from regcache or frame into the array,
> 3) pass the array to extract_unsigned_integer
>
> we can pass a callable function_view as a content provider to
> extract_unsigned_integer, so that we don't need step 1). The code
> becomes,
>
> return extract_unsigned_integer ([&] (gdb_byte *buf, size_t size)
> {
> frame_unwind_register (frame, regnum, buf);
> }, size, byte_order);
>
> We can remove some uses of MAX_REGISTER_SIZE in this way. Do you (Alan
> and others) like the patch below?
This looks a bit over engineered to me.
If extract_unsigned_integer always creates a local buffer inside,
and it's always going to be a buffer the size of a LONGEST, because
that's the type that extract_unsigned_integer returns, then,
I'd think that hiding the buffer size and the extract_unsigned_integer
call in a class instead would do. Like:
class extractor
{
public:
extractor () = default;
// Get buffer. Could take a "size" parameter too,
// for pre-validation instead of passing "size" to "extract".
// Or make that a separate size() method. Or add a "size" parameter
// to the ctor and validate there. Whatever. The lambda-based
// solution isn't validating upfront either.
gdb_byte *buffer () { return m_buffer; }
// Do extraction.
LONGEST extract (size_t size, bfd_endian byte_order);
private:
gdb_byte m_buffer[sizeof (LONGEST)];
};
LONGEST
extractor::extract (size_t size, bfd_endian byte_order)
{
if (size > sizeof (LONGEST))
error (_("\
That operation is not available on integers larger than %d bytes."),
sizeof (LONGEST));
return extract_unsigned_integer (m_buffer, size, byte_order);
}
And then used like:
extractor extr;
frame_unwind_register (frame, regnum, ext.buffer ());
return extr.extract (size, byte_order);
Instead of:
return extract_unsigned_integer ([&] (gdb_byte *buf, size_t size)
{
frame_unwind_register (frame, regnum, buf);
}, size, byte_order);
Thanks,
Pedro Alves