Remove MAX_REGISTER_SIZE from frame.c

  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?

> @@ -1460,11 +1473,15 @@ put_frame_register_bytes (struct frame_info *frame, int regnum,
>  	}
>        else
>  	{
> -	  gdb_byte buf[MAX_REGISTER_SIZE];
> -
> -	  deprecated_frame_register_read (frame, regnum, buf);
> -	  memcpy (buf + offset, myaddr, curr_len);
> -	  put_frame_register (frame, regnum, buf);
> +	  struct value *value = frame_unwind_register_value (frame->next,
> +							     regnum);
> +	  gdb_assert (value != NULL);
> +
> +	  memcpy ((char *) value_contents_all (value) + offset, myaddr,
> +		  curr_len);

Use value_contents_writeable,

> +	  put_frame_register (frame, regnum, value_contents_all (value));

s/value_contents_all/value_contents_raw/

because value_contents_all requires value is available but
deprecated_frame_register_read doesn't.

> +	  release_value (value);
> +	  value_free (value);
  

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
  

Pedro Alves <palves@redhat.com> writes:

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

My lambda-based solution does validate the boundary before reading
contents to buffer,

+ULONGEST
+extract_unsigned_integer (gdb::function_view<void (gdb_byte *, size_t size)> content_provider,
+                         int len, enum bfd_endian byte_order)
+{
+  if (len > (int) sizeof (ULONGEST))
+    error (_("\
+That operation is not available on integers of more than %d bytes."),
+          (int) sizeof (ULONGEST));
+
+  gdb_byte buf[sizeof (ULONGEST)];
+
+  content_provider (buf, len);
+  return extract_unsigned_integer_1 (buf, len, byte_order);
+}

>
>  extractor extr;
>  frame_unwind_register (frame, regnum, ext.buffer ());

We may overflow ext.buffer (), because the boundary checking is done in
.extract below,

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

On 03/28/2017 05:13 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> 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.
> 
> My lambda-based solution does validate the boundary before reading
> contents to buffer,
> 
> +ULONGEST
> +extract_unsigned_integer (gdb::function_view<void (gdb_byte *, size_t size)> content_provider,
> +                         int len, enum bfd_endian byte_order)
> +{
> +  if (len > (int) sizeof (ULONGEST))
> +    error (_("\
> +That operation is not available on integers of more than %d bytes."),
> +          (int) sizeof (ULONGEST));
> +
> +  gdb_byte buf[sizeof (ULONGEST)];
> +
> +  content_provider (buf, len);
> +  return extract_unsigned_integer_1 (buf, len, byte_order);
> +}
> 

The upfront check doesn't protect entirely.  It still up to the lambda,
to ultimately check for overflow.  Here:

  return extract_unsigned_integer ([&] (gdb_byte *buf, size_t size)
				   {
				     frame_unwind_register (frame, regnum, buf);
				   }, size, byte_order);

... frame_unwind_register can overflow the buffer, since it ignores "size".
And if we require adding some check inside the lambda, then we've really
not gained that much by doing the upfront check.

>>
>>  extractor extr;
>>  frame_unwind_register (frame, regnum, ext.buffer ());
> 
> We may overflow ext.buffer (), because the boundary checking is done in
> .extract below,
> 
>>  return extr.extract (size, byte_order);

Yes, and that can sorted by e.g., passing the size to the buffer()
method, as I mentioned in the comment.  Like:

  extractor extr;
  frame_unwind_register (frame, regnum, ext.buffer (size));
  return extr.extract (size, byte_order);

extractor::buffer(size_t) would throw error on overflow.

Or pass it to the ctor (which would likewise throw error on overflow):

  extractor extr (size);
  frame_unwind_register (frame, regnum, ext.buffer ());
  return extr.extract (size, byte_order);

Could even store the size and byte order inside the extractor
object, and avoid writing the size more than once:

  extractor extr (size, byte_order);
  frame_unwind_register (frame, regnum, ext.buffer ());
  return extr.extract ();

Or make "extrator::buffer" remember the last size, so extractors
can be reused.  Or even support both, ctor with and without size,
buffer() with and without size.  extractror::extract would always
used the last remembered size.

So I still don't see any advantage in a callback-based interface.

Thanks,
Pedro Alves
  

On 03/28/2017 05:57 PM, Pedro Alves wrote:

> Yes, and that can sorted by e.g., passing the size to the buffer()
> method, as I mentioned in the comment.  Like:
> 
>   extractor extr;
>   frame_unwind_register (frame, regnum, ext.buffer (size));
>   return extr.extract (size, byte_order);
> 
> extractor::buffer(size_t) would throw error on overflow.
> 
> Or pass it to the ctor (which would likewise throw error on overflow):
> 
>   extractor extr (size);
>   frame_unwind_register (frame, regnum, ext.buffer ());
>   return extr.extract (size, byte_order);
> 
> Could even store the size and byte order inside the extractor
> object, and avoid writing the size more than once:
> 
>   extractor extr (size, byte_order);
>   frame_unwind_register (frame, regnum, ext.buffer ());
>   return extr.extract ();
> 
> Or make "extrator::buffer" remember the last size, so extractors
> can be reused.  Or even support both, ctor with and without size,
> buffer() with and without size.  extractror::extract would always
> used the last remembered size.
> 
> So I still don't see any advantage in a callback-based interface.

Thinking about this a bit more, if we went this direction, I think the
simplest would be to add an extract::size() method that returned the
size of the buffer, and use that for bounds when filling in the
data, like:

   extractor extr;
   frame_unwind_register (frame, regnum, ext.buffer (), ext.size ());
   return extr.extract (type_len, byte_order);

If type_len is larger than the buffer size, then we'll still get an
error either inside extractor::extract, and maybe earlier
inside frame_unwind_register (if it had that size parameter).

Though for the particular case of frame_unwind_register, since the
frame machinery works with struct value's, inside frame_unwind_register
there's going to be a value created already, and that has a contents
buffer we could access directly.  So e.g.,
inside frame_unwind_register_signed, we should be able to use
frame_unwind_register_value directly thus avoid the need for the local
buffer and copying data.

Thanks,
Pedro Alves