[PATCH] Remove MAX_REGISTER_SIZE from frame.c

[PATCH] Remove MAX_REGISTER_SIZE from frame.c

[Alan Hayward]

In addition, remove as usage of deprecated_frame_register_read

Tested using make check with board files unix and native-gdbserver.

Ok to commit?

Alan.


2017-02-21  Alan Hayward  <alan.hayward@arm.com>

	* gdb/frame.c (frame_unwind_register_signed): Use LONGEST for max size.
	(frame_unwind_register_unsigned): Use ULONGEST for max size.
	(get_frame_register_bytes): Unwind using value.
	(put_frame_register_bytes): Likewise.



diff --git a/gdb/frame.c b/gdb/frame.c
index d98003dee7c34a63bd25356e6674721664a4b2f3..22cfdea4bcb20582229ffc360ead060c43d7cd81 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -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));

   frame_unwind_register (frame, regnum, buf);
   return extract_signed_integer (buf, size, byte_order);
@@ -1270,7 +1274,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];
+  gdb_byte buf[sizeof (ULONGEST)];
+
+  if (size > (int) sizeof (ULONGEST))
+    error (_("Cannot unwind integers more than %d bytes."),
+	   (int) sizeof (ULONGEST));

   frame_unwind_register (frame, regnum, buf);
   return extract_unsigned_integer (buf, size, byte_order);
@@ -1410,16 +1418,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 +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);
+	  put_frame_register (frame, regnum, value_contents_all (value));
+	  release_value (value);
+	  value_free (value);
 	}

       myaddr += curr_len;

Re: [PATCH] Remove MAX_REGISTER_SIZE from frame.c

[Yao Qi]

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

Re: [PATCH] Remove MAX_REGISTER_SIZE from frame.c

[Alan Hayward]

[-- 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¬

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

[Pedro Alves]

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

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

[Yao Qi]

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

-- 
Yao (齐尧)

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

[Pedro Alves]

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

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

[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

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

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

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

Yes, that is the simplest way.

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

How is the patch below?

-- 
Yao (齐尧)

From d8f91babfcd6810ff4942aa8275d3d447ae1a83a Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Wed, 29 Mar 2017 16:49:11 +0100
Subject: [PATCH] Use frame_unwind_register_value in
 frame_unwind_register_unsigned

gdb:

2017-03-29  Yao Qi  <yao.qi@linaro.org>

	* frame.c (frame_unwind_register_unsigned): Call
	frame_unwind_register_value.
---
 gdb/frame.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index d98003d..a10f3e5 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1270,10 +1270,27 @@ 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];
+  struct value *value = frame_unwind_register_value (frame, regnum);
 
-  frame_unwind_register (frame, regnum, buf);
-  return extract_unsigned_integer (buf, size, byte_order);
+  gdb_assert (value != NULL);
+
+  if (value_optimized_out (value))
+    {
+      throw_error (OPTIMIZED_OUT_ERROR,
+		   _("Register %d was not saved"), regnum);
+    }
+  if (!value_entirely_available (value))
+    {
+      throw_error (NOT_AVAILABLE_ERROR,
+		   _("Register %d is not available"), regnum);
+    }
+
+  ULONGEST r = extract_unsigned_integer (value_contents_all (value), size,
+					 byte_order);
+
+  release_value (value);
+  value_free (value);
+  return r;
 }
 
 ULONGEST
-- 
1.9.1

Re: [PATCH] Remove MAX_REGISTER_SIZE from frame.c

[Yao Qi]

Alan Hayward <Alan.Hayward@arm.com> writes:

> 2017-03-24  Alan Hayward  <alan.hayward@arm.com>
>
> 	* frame.c (get_frame_register_bytes): Unwind using value.
> 	(put_frame_register_bytes): Likewise.

Patch is good to me.

-- 
Yao (齐尧)

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

[Pedro Alves]

On 04/03/2017 02:58 PM, Yao Qi wrote:

>>
>> 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.
> 
> How is the patch below?
> 

Fine with me.

Thanks,
Pedro Alves

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

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Fine with me.

Patch is pushed in.

-- 
Yao (齐尧)