extract_unsigned_integer API (Re: [PATCH] Remove MAX_REGISTER_SIZE from frame.c)

[Pedro Alves <palves@redhat.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