Re: [PATCH 10/11] Add XTENSA_MAX_REGISTER_SIZE

[Alan Hayward <Alan dot Hayward at arm dot com>]

> On 25 Apr 2017, at 16:39, Yao Qi <qiyaoltc@gmail.com> wrote:
> 
> Alan Hayward <Alan.Hayward@arm.com> writes:
> 
>> @@ -559,16 +562,15 @@ xtensa_pseudo_register_read (struct gdbarch *gdbarch,
>>       && (regnum >= gdbarch_tdep (gdbarch)->a0_base)
>>       && (regnum <= gdbarch_tdep (gdbarch)->a0_base + 15))
>>     {
>> -      gdb_byte *buf = (gdb_byte *) alloca (MAX_REGISTER_SIZE);
>> +      ULONGEST value;
>>       enum register_status status;
>> 
>> -      status = regcache_raw_read (regcache,
>> -				  gdbarch_tdep (gdbarch)->wb_regnum,
>> -				  buf);
>> +      status = regcache_raw_read_unsigned (regcache,
>> +					   gdbarch_tdep (gdbarch)->wb_regnum,
>> +					   &value);
>>       if (status != REG_VALID)
>> 	return status;
>> -      regnum = arreg_number (gdbarch, regnum,
>> -			     extract_unsigned_integer (buf, 4, byte_order));
>> +      regnum = arreg_number (gdbarch, regnum, value);
>>     }
>> 
>>   /* We can always read non-pseudo registers.  */
>> @@ -656,12 +658,10 @@ xtensa_pseudo_register_write (struct gdbarch *gdbarch,
>>       && (regnum >= gdbarch_tdep (gdbarch)->a0_base)
>>       && (regnum <= gdbarch_tdep (gdbarch)->a0_base + 15))
>>     {
>> -      gdb_byte *buf = (gdb_byte *) alloca (MAX_REGISTER_SIZE);
>> -
>> -      regcache_raw_read (regcache,
>> -			 gdbarch_tdep (gdbarch)->wb_regnum, buf);
>> -      regnum = arreg_number (gdbarch, regnum,
>> -			     extract_unsigned_integer (buf, 4, byte_order));
>> +      ULONGEST value;
>> +      regcache_raw_read_unsigned (regcache,
>> +				  gdbarch_tdep (gdbarch)->wb_regnum, &value);
>> +      regnum = arreg_number (gdbarch, regnum, value);
> 
> This part of patch is OK, but the part using XTENSA_MAX_REGISTER_SIZE
> still needs some review.
> 
> -- 
> Yao (齐尧)


Ok, Above committed with:

2017-04-26  Alan Hayward  <alan.hayward@arm.com>

	* xtensa-tdep.c (xtensa_pseudo_register_read): Use
	regcache_raw_read_unsigned.
	(xtensa_pseudo_register_write): Likewise.


This leaves the following left to review (below).

Looking at the code, I'm still a little unsure exactly how "value" is
being used. "value" is declared as max register size, rounded up to the
next int. Looking at the use of value, I think the code requires max
size of a register multiplied by the value in reg->mask + 1 - and checking
xtensa-config.c, this never goes above one. The comment also states
the size of a masked register is never greater than 32bits.
Maybe the value should be given a size of 32bits * (mask->count + 1) ?


2017-04-06  Alan Hayward  <alan.hayward@arm.com>

	* xtensa-tdep.c (XTENSA_MAX_REGISTER_SIZE): Add.
        (xtensa_register_write_masked): Use XTENSA_MAX_REGISTER_SIZE.
	(xtensa_register_read_masked): Likewise.


diff --git a/gdb/xtensa-tdep.c b/gdb/xtensa-tdep.c
index 0a4ed37cf9582a7aa43ec601fc013f1f93624a39..e79c925b345a8bd03e3a674c731c00e86f1ea39a 100644
--- a/gdb/xtensa-tdep.c
+++ b/gdb/xtensa-tdep.c
@@ -120,6 +120,9 @@ static unsigned int xtensa_debug_level = 0;
 #define PS_WOE			(1<<18)
 #define PS_EXC			(1<<4)

+/* Big enough to hold the size of the largest register in bytes.  */
+#define XTENSA_MAX_REGISTER_SIZE	16
+
 static int
 windowing_enabled (struct gdbarch *gdbarch, unsigned int ps)
 {
@@ -370,7 +373,7 @@ static void
 xtensa_register_write_masked (struct regcache *regcache,
 			      xtensa_register_t *reg, const gdb_byte *buffer)
 {
-  unsigned int value[(MAX_REGISTER_SIZE + 3) / 4];
+  unsigned int value[(XTENSA_MAX_REGISTER_SIZE + 3) / 4];
   const xtensa_mask_t *mask = reg->mask;

   int shift = 0;		/* Shift for next mask (mod 32).  */
@@ -454,7 +457,7 @@ static enum register_status
 xtensa_register_read_masked (struct regcache *regcache,
 			     xtensa_register_t *reg, gdb_byte *buffer)
 {
-  unsigned int value[(MAX_REGISTER_SIZE + 3) / 4];
+  unsigned int value[(XTENSA_MAX_REGISTER_SIZE + 3) / 4];
   const xtensa_mask_t *mask = reg->mask;

   int shift = 0;