Re: [PATCH] Remove MAX_REGISTER_SIZE from frame.c

[Alan Hayward <Alan.Hayward@arm.com>]

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

Patch update as per comments below.
Tested using make check with board files unix and native-gdbserver.
Ok to commit?

I'm happy with Yao's patch below too.

Alan.


2017-03-24  Alan Hayward  <alan.hayward@arm.com>

	* frame.c (get_frame_register_bytes): Unwind using value.
	(put_frame_register_bytes): Likewise.



diff --git a/gdb/frame.c b/gdb/frame.c
index d98003dee7c34a63bd25356e6674721664a4b2f3..05a3be400c35ada0de48fd529e9cc83e0eaa941d 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1410,16 +1410,21 @@ get_frame_register_bytes (struct frame_info *frame, int regnum,
 	}
       else
 	{
-	  gdb_byte buf[MAX_REGISTER_SIZE];
-	  enum lval_type lval;
-	  CORE_ADDR addr;
-	  int realnum;
+	  struct value *value = frame_unwind_register_value (frame->next,
+							     regnum);
+	  gdb_assert (value != NULL);
+	  *optimizedp = value_optimized_out (value);
+	  *unavailablep = !value_entirely_available (value);

-	  frame_register (frame, regnum, optimizedp, unavailablep,
-			  &lval, &addr, &realnum, buf);
 	  if (*optimizedp || *unavailablep)
-	    return 0;
-	  memcpy (myaddr, buf + offset, curr_len);
+	    {
+	      release_value (value);
+	      value_free (value);
+	      return 0;
+	    }
+	  memcpy (myaddr, value_contents_all (value) + offset, curr_len);
+	  release_value (value);
+	  value_free (value);
 	}

       myaddr += curr_len;
@@ -1460,11 +1465,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_writeable (value) + offset, myaddr,
+		  curr_len);
+	  put_frame_register (frame, regnum, value_contents_raw (value));
+	  release_value (value);
+	  value_free (value);
 	}

       myaddr += curr_len;



> On 1 Mar 2017, at 12:32, Yao Qi <qiyaoltc@gmail.com> 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?
> 
>> @@ -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);
> 
> -- 
> Yao (齐尧)
> From 2a76b39ffa87ed015f3637ee4a9d083f682863a0 Mon Sep 17 00:00:00 2001
> From: Yao Qi <yao.qi@linaro.org>
> Date: Wed, 1 Mar 2017 10:43:29 +0000
> Subject: [PATCH] Use content provider in extract_unsigned_integer
> 
> ---
> gdb/ada-lang.c  |  9 ++++++---
> gdb/defs.h      |  4 ++++
> gdb/findvar.c   | 38 ++++++++++++++++++++++++++++++--------
> gdb/frame.c     |  7 ++++---
> gdb/ia64-tdep.c | 19 +++++++++++++++----
> 5 files changed, 59 insertions(+), 18 deletions(-)
> 
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index 753409c..07ce04a 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -4555,12 +4555,15 @@ value_pointer (struct value *value, struct type *type)
> {
>   struct gdbarch *gdbarch = get_type_arch (type);
>   unsigned len = TYPE_LENGTH (type);
> -  gdb_byte *buf = (gdb_byte *) alloca (len);
>   CORE_ADDR addr;
> 
>   addr = value_address (value);
> -  gdbarch_address_to_pointer (gdbarch, type, buf, addr);
> -  addr = extract_unsigned_integer (buf, len, gdbarch_byte_order (gdbarch));
> +  addr = extract_unsigned_integer ([&] (gdb_byte *buf, size_t size)
> +				   {
> +				     gdbarch_address_to_pointer (gdbarch,
> +								 type, buf,
> +								 addr);
> +				   }, len, gdbarch_byte_order (gdbarch));
>   return addr;
> }
> 
> diff --git a/gdb/defs.h b/gdb/defs.h
> index aa58605..a2f8fce 100644
> --- a/gdb/defs.h
> +++ b/gdb/defs.h
> @@ -628,6 +628,10 @@ extern LONGEST extract_signed_integer (const gdb_byte *, int,
> extern ULONGEST extract_unsigned_integer (const gdb_byte *, int,
> 					  enum bfd_endian);
> 
> +extern ULONGEST
> +  extract_unsigned_integer (gdb::function_view<void (gdb_byte *, size_t size)> content_provider,
> +			    int len, enum bfd_endian byte_order);
> +
> extern int extract_long_unsigned_integer (const gdb_byte *, int,
> 					  enum bfd_endian, LONGEST *);
> 
> diff --git a/gdb/findvar.c b/gdb/findvar.c
> index 80c709a..6d7d0de 100644
> --- a/gdb/findvar.c
> +++ b/gdb/findvar.c
> @@ -81,20 +81,15 @@ That operation is not available on integers of more than %d bytes."),
>   return retval;
> }
> 
> -ULONGEST
> -extract_unsigned_integer (const gdb_byte *addr, int len,
> -			  enum bfd_endian byte_order)
> +static ULONGEST
> +extract_unsigned_integer_1 (const gdb_byte *addr, int len,
> +			    enum bfd_endian byte_order)
> {
>   ULONGEST retval;
>   const unsigned char *p;
>   const unsigned char *startaddr = addr;
>   const unsigned char *endaddr = startaddr + len;
> 
> -  if (len > (int) sizeof (ULONGEST))
> -    error (_("\
> -That operation is not available on integers of more than %d bytes."),
> -	   (int) sizeof (ULONGEST));
> -
>   /* Start at the most significant end of the integer, and work towards
>      the least significant.  */
>   retval = 0;
> @@ -111,6 +106,33 @@ That operation is not available on integers of more than %d bytes."),
>   return retval;
> }
> 
> +ULONGEST
> +extract_unsigned_integer (const gdb_byte *addr, 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));
> +
> +  return extract_unsigned_integer_1 (addr, len, byte_order);
> +}
> +
> +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);
> +}
> +
> /* Sometimes a long long unsigned integer can be extracted as a
>    LONGEST value.  This is done so that we can print these values
>    better.  If this integer can be converted to a LONGEST, this
> diff --git a/gdb/frame.c b/gdb/frame.c
> index d98003d..57f82f1 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -1270,10 +1270,11 @@ frame_unwind_register_unsigned (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];
> 
> -  frame_unwind_register (frame, regnum, buf);
> -  return extract_unsigned_integer (buf, size, byte_order);
> +  return extract_unsigned_integer ([&] (gdb_byte *buf, size_t size)
> +				   {
> +				     frame_unwind_register (frame, regnum, buf);
> +				   }, size, byte_order);
> }
> 
> ULONGEST
> diff --git a/gdb/ia64-tdep.c b/gdb/ia64-tdep.c
> index 4c53bc6..2748dac 100644
> --- a/gdb/ia64-tdep.c
> +++ b/gdb/ia64-tdep.c
> @@ -1516,7 +1516,6 @@ examine_prologue (CORE_ADDR pc, CORE_ADDR lim_pc,
> 	  else if (qp == 0 && rN == 2 
> 	        && ((rM == fp_reg && fp_reg != 0) || rM == 12))
> 	    {
> -	      gdb_byte buf[MAX_REGISTER_SIZE];
> 	      CORE_ADDR saved_sp = 0;
> 	      /* adds r2, spilloffset, rFramePointer 
> 	           or
> @@ -1534,8 +1533,15 @@ examine_prologue (CORE_ADDR pc, CORE_ADDR lim_pc,
> 		{
> 		  struct gdbarch *gdbarch = get_frame_arch (this_frame);
> 		  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> -		  get_frame_register (this_frame, sp_regnum, buf);
> -		  saved_sp = extract_unsigned_integer (buf, 8, byte_order);
> +
> +		  saved_sp
> +		    = extract_unsigned_integer ([&] (gdb_byte * buf,
> +						     size_t size)
> +						{
> +						  get_frame_register (this_frame,
> +								      sp_regnum,
> +								      buf);
> +						}, 8, byte_order);
> 		}
> 	      spill_addr  = saved_sp
> 	                  + (rM == 12 ? 0 : mem_stack_frame_size) 
> @@ -1782,7 +1788,12 @@ examine_prologue (CORE_ADDR pc, CORE_ADDR lim_pc,
>       else if (cfm_reg != 0)
> 	{
> 	  get_frame_register (this_frame, cfm_reg, buf);
> -	  cfm = extract_unsigned_integer (buf, 8, byte_order);
> +	  cfm = extract_unsigned_integer ([&] (gdb_byte * buf, size_t size)
> +					  {
> +					    get_frame_register (this_frame,
> +								cfm_reg,
> +								buf);
> +					  }, 8, byte_order);
> 	}
>       cache->prev_cfm = cfm;
> 
> -- 
> 1.9.1

\x16º&Öéj×!zÊÞ¶êç×~û÷Éb²Ö«r\x18\x1dn–­r\x17¬