Re: [PATCH] Remove MAX_REGISTER_SIZE from frame.c

[Yao Qi <qiyaoltc@gmail.com>]

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