diff mbox

Remove MAX_REGISTER_SIZE from frame.c

Message ID BFF6340B-8563-49A6-8635-0229ACA9306A@arm.com
State New
Headers show

Commit Message

Alan Hayward March 24, 2017, 2:49 p.m. UTC 
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.






> 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

Comments

Yao Qi April 3, 2017, 8:41 p.m. UTC | #1 
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.
diff mbox

Patch

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;