[PATCH 8/11] Add FRV_MAX_REGISTER_SIZE

[PATCH 8/11] Add FRV_MAX_REGISTER_SIZE

[Alan Hayward]

Max size set to 64bits, which I determined using frv_register_type()

Tested on a --enable-targets=all build using make check with board files
unix and native-gdbserver.

I do not have a FRV machine to test on.

Ok to commit?

Alan.

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

	* frv-linux-tdep.c (frv_linux_supply_gregset): Use FRV_MAX_REGISTER_SIZE.
	* frv-tdep.h (FRV_MAX_REGISTER_SIZE): Add.


diff --git a/gdb/frv-linux-tdep.c b/gdb/frv-linux-tdep.c
index eb87f93058b0287e8f05c585d1b6aa1ff2bffb78..6f5585e774c86afe7e271f90df1bf3db93340421 100644
--- a/gdb/frv-linux-tdep.c
+++ b/gdb/frv-linux-tdep.c
@@ -413,9 +413,9 @@ frv_linux_supply_gregset (const struct regset *regset,
 			  int regnum, const void *gregs, size_t len)
 {
   int regi;
-  char zerobuf[MAX_REGISTER_SIZE];
+  char zerobuf[FRV_MAX_REGISTER_SIZE];

-  memset (zerobuf, 0, MAX_REGISTER_SIZE);
+  memset (zerobuf, 0, FRV_MAX_REGISTER_SIZE);

   /* gr0 always contains 0.  Also, the kernel passes the TBR value in
      this slot.  */
diff --git a/gdb/frv-tdep.h b/gdb/frv-tdep.h
index c1a0f35456c879c655198820f5319bf0ab42b9f7..9c8c32eb08b6e4b4d87139b201008c1ee419f6a9 100644
--- a/gdb/frv-tdep.h
+++ b/gdb/frv-tdep.h
@@ -92,6 +92,9 @@ enum {
   frv_num_pseudo_regs = last_pseudo_regnum - first_pseudo_regnum + 1,
 };

+/* Big enough to hold the size of the largest register in bytes.  */
+#define FRV_MAX_REGISTER_SIZE	8
+
 /* Return the FR-V ABI associated with GDBARCH.  */
 enum frv_abi frv_abi (struct gdbarch *gdbarch);

Re: [PATCH 8/11] Add FRV_MAX_REGISTER_SIZE

[Yao Qi]

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

> -  char zerobuf[MAX_REGISTER_SIZE];
> +  char zerobuf[FRV_MAX_REGISTER_SIZE];
>
> -  memset (zerobuf, 0, MAX_REGISTER_SIZE);
> +  memset (zerobuf, 0, FRV_MAX_REGISTER_SIZE);
>
>    /* gr0 always contains 0.  Also, the kernel passes the TBR value in
>       this slot.  */

The code here fills some gr registers with zeros,

  /* gr0 always contains 0.  Also, the kernel passes the TBR value in
     this slot.  */
  regcache_raw_supply (regcache, first_gpr_regnum, zerobuf);

  /* Fill gr32, ..., gr63 with zeros. */
  for (regi = first_gpr_regnum + 32; regi <= last_gpr_regnum; regi++)
    regcache_raw_supply (regcache, regi, zerobuf);

the size of these gr registers are know, 8 bytes.  It won't be changed.
We can do,

 gdb_byte zerobuf[8] = { 0 };

the code is still easy to read.  If you really dislike magic number (IMO, 8
is not a magic number in this context), you can define FRR_GR_REGISTER_SIZE.

Alternatively, you can add a new regache api, regcache_raw_supply_zero.
Many places can use this api, and some uses of MAX_REGISTER_SIZE can be
removed too, for example,

  regcache_raw_supply (regcache, MIPS_ZERO_REGNUM, zerobuf);

-- 
Yao (齐尧)

Re: [PATCH 8/11] Add FRV_MAX_REGISTER_SIZE

[Alan Hayward]

> On 11 Apr 2017, at 11:02, Yao Qi <qiyaoltc@gmail.com> wrote:
> 
> Alan Hayward <Alan.Hayward@arm.com> writes:
> 
>> -  char zerobuf[MAX_REGISTER_SIZE];
>> +  char zerobuf[FRV_MAX_REGISTER_SIZE];
>> 
>> -  memset (zerobuf, 0, MAX_REGISTER_SIZE);
>> +  memset (zerobuf, 0, FRV_MAX_REGISTER_SIZE);
>> 
>>   /* gr0 always contains 0.  Also, the kernel passes the TBR value in
>>      this slot.  */
> 
> The code here fills some gr registers with zeros,
> 
>  /* gr0 always contains 0.  Also, the kernel passes the TBR value in
>     this slot.  */
>  regcache_raw_supply (regcache, first_gpr_regnum, zerobuf);
> 
>  /* Fill gr32, ..., gr63 with zeros. */
>  for (regi = first_gpr_regnum + 32; regi <= last_gpr_regnum; regi++)
>    regcache_raw_supply (regcache, regi, zerobuf);
> 
> the size of these gr registers are know, 8 bytes.  It won't be changed.
> We can do,
> 
> gdb_byte zerobuf[8] = { 0 };
> 
> the code is still easy to read.  If you really dislike magic number (IMO, 8
> is not a magic number in this context), you can define FRR_GR_REGISTER_SIZE.
> 
> Alternatively, you can add a new regache api, regcache_raw_supply_zero.
> Many places can use this api, and some uses of MAX_REGISTER_SIZE can be
> removed too, for example,
> 
>  regcache_raw_supply (regcache, MIPS_ZERO_REGNUM, zerobuf);
> 
> -- 
> Yao (齐尧)

Went with the simpler solution.

I don't have a FRV machine to test on.
Tested on a --enable-targets=all build using make check with board files
unix and native-gdbserver.

Ok to commit?

Alan.

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

	* gdb/frv-linux-tdep.c (frv_linux_supply_gregset): Remove MAX_REGISTER_SIZE.


diff --git a/gdb/frv-linux-tdep.c b/gdb/frv-linux-tdep.c
index eb87f93058b0287e8f05c585d1b6aa1ff2bffb78..3f91bf0ee81a5d5ca915c88337c6a4b4ed53d344 100644
--- a/gdb/frv-linux-tdep.c
+++ b/gdb/frv-linux-tdep.c
@@ -413,9 +413,7 @@ frv_linux_supply_gregset (const struct regset *regset,
 			  int regnum, const void *gregs, size_t len)
 {
   int regi;
-  char zerobuf[MAX_REGISTER_SIZE];
-
-  memset (zerobuf, 0, MAX_REGISTER_SIZE);
+  gdb_byte zerobuf[8] = { 0 };

   /* gr0 always contains 0.  Also, the kernel passes the TBR value in
      this slot.  */

Re: [PATCH 8/11] Add FRV_MAX_REGISTER_SIZE

[Alan Hayward]

> On 12 Apr 2017, at 09:27, Alan Hayward <Alan.Hayward@arm.com> wrote:
> 
> 
>> On 11 Apr 2017, at 11:02, Yao Qi <qiyaoltc@gmail.com> wrote:
>> 
>> Alan Hayward <Alan.Hayward@arm.com> writes:
>> 
>>> -  char zerobuf[MAX_REGISTER_SIZE];
>>> +  char zerobuf[FRV_MAX_REGISTER_SIZE];
>>> 
>>> -  memset (zerobuf, 0, MAX_REGISTER_SIZE);
>>> +  memset (zerobuf, 0, FRV_MAX_REGISTER_SIZE);
>>> 
>>>  /* gr0 always contains 0.  Also, the kernel passes the TBR value in
>>>     this slot.  */
>> 
>> The code here fills some gr registers with zeros,
>> 
>> /* gr0 always contains 0.  Also, the kernel passes the TBR value in
>>    this slot.  */
>> regcache_raw_supply (regcache, first_gpr_regnum, zerobuf);
>> 
>> /* Fill gr32, ..., gr63 with zeros. */
>> for (regi = first_gpr_regnum + 32; regi <= last_gpr_regnum; regi++)
>>   regcache_raw_supply (regcache, regi, zerobuf);
>> 
>> the size of these gr registers are know, 8 bytes.  It won't be changed.
>> We can do,
>> 
>> gdb_byte zerobuf[8] = { 0 };
>> 
>> the code is still easy to read.  If you really dislike magic number (IMO, 8
>> is not a magic number in this context), you can define FRR_GR_REGISTER_SIZE.
>> 
>> Alternatively, you can add a new regache api, regcache_raw_supply_zero.
>> Many places can use this api, and some uses of MAX_REGISTER_SIZE can be
>> removed too, for example,
>> 
>> regcache_raw_supply (regcache, MIPS_ZERO_REGNUM, zerobuf);
>> 
>> -- 
>> Yao (齐尧)
> 
> Went with the simpler solution.
> 
> I don't have a FRV machine to test on.
> Tested on a --enable-targets=all build using make check with board files
> unix and native-gdbserver.
> 

Looking again at my later patches, I agree that regcache_raw_supply_zero
would be useful in other places too.

I considered making regcache_raw_supply_zero call regcache_raw_supply, but
in the end it made more sense to make it completely separate.

I don't have a FRV machine to test on.
Tested on a --enable-targets=all build using make check with board files
unix and native-gdbserver.

Ok to commit?

Alan.

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

	* gdb/frv-linux-tdep.c (frv_linux_supply_gregset): Call
	regcache_raw_supply_zero
	* gdb/regcache.c (regcache_raw_supply_zero): New.
	* gdb/regcache.h (regcache_raw_supply_zero): New declaration.


diff --git a/gdb/frv-linux-tdep.c b/gdb/frv-linux-tdep.c
index eb87f93058b0287e8f05c585d1b6aa1ff2bffb78..d52eb2163437e32029c989c2caecce8533f11a65 100644
--- a/gdb/frv-linux-tdep.c
+++ b/gdb/frv-linux-tdep.c
@@ -413,17 +413,14 @@ frv_linux_supply_gregset (const struct regset *regset,
 			  int regnum, const void *gregs, size_t len)
 {
   int regi;
-  char zerobuf[MAX_REGISTER_SIZE];
-
-  memset (zerobuf, 0, MAX_REGISTER_SIZE);

   /* gr0 always contains 0.  Also, the kernel passes the TBR value in
      this slot.  */
-  regcache_raw_supply (regcache, first_gpr_regnum, zerobuf);
+  regcache_raw_supply_zero (regcache, first_gpr_regnum);

   /* Fill gr32, ..., gr63 with zeros. */
   for (regi = first_gpr_regnum + 32; regi <= last_gpr_regnum; regi++)
-    regcache_raw_supply (regcache, regi, zerobuf);
+    regcache_raw_supply_zero (regcache, regi);

   regcache_supply_regset (regset, regcache, regnum, gregs, len);
 }
diff --git a/gdb/regcache.h b/gdb/regcache.h
index f201f0c084f40ccf276a7e1ba19050cbc11208ad..ff6e583a48f9200f4a46ea5fcca69bfdd2862dac 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -152,6 +152,12 @@ extern void regcache_raw_supply (struct regcache *regcache,
 extern void regcache_raw_collect (const struct regcache *regcache,
 				  int regnum, void *buf);

+/* Supply register REGNUM with zeroed value to REGCACHE.  This is not the same
+   as calling regcache_raw_supply with NULL (which will set the state to
+   unavailable).  */
+
+extern void regcache_raw_supply_zero (struct regcache *regcache, int regnum);
+
 /* Collect register REGNUM from SOURCE_REGCACHE and store its contents to
    DEST_REGCACHE.  */
 extern void regcache_raw_copy (const struct regcache *dest_regcache,
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 20e43f486ff02f15000bebee2a4eb03db53111d4..03f7d216f7d2fb8dd2e5b1189c9743ed37facb30 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -1125,6 +1125,27 @@ regcache_raw_collect (const struct regcache *regcache, int regnum, void *buf)
   memcpy (buf, regbuf, size);
 }

+/* Supply register REGNUM with zeroed value to REGCACHE.  This is not the same
+   as calling regcache_raw_supply with NULL (which will set the state to
+   unavailable).  */
+
+void
+regcache_raw_supply_zero (struct regcache *regcache, int regnum)
+{
+  void *regbuf;
+  size_t size;
+
+  gdb_assert (regcache != NULL);
+  gdb_assert (regnum >= 0 && regnum < regcache->descr->nr_raw_registers);
+  gdb_assert (!regcache->readonly_p);
+
+  regbuf = register_buffer (regcache, regnum);
+  size = regcache->descr->sizeof_register[regnum];
+
+  memset (regbuf, 0, size);
+  regcache->register_status[regnum] = REG_VALID;
+}
+
 void
 regcache_raw_copy (const struct regcache *dest_regcache, int regnum,
 		   struct regcache *src_regcache)

Re: [PATCH 8/11] Add FRV_MAX_REGISTER_SIZE

[Yao Qi]

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

Hi Alan,
regcache.c is updated, so please update your patch.

> I considered making regcache_raw_supply_zero call regcache_raw_supply, but
> in the end it made more sense to make it completely separate.

You can call raw_supply (regnum, NULL) and then set the status to REG_VALID.

-- 
Yao (齐尧)

Re: [PATCH 8/11] Add FRV_MAX_REGISTER_SIZE

[Pedro Alves]

On 05/03/2017 09:44 AM, Yao Qi wrote:
> Alan Hayward <Alan.Hayward@arm.com> writes:
> 
> Hi Alan,
> regcache.c is updated, so please update your patch.
> 
>> I considered making regcache_raw_supply_zero call regcache_raw_supply, but
>> in the end it made more sense to make it completely separate.
> 
> You can call raw_supply (regnum, NULL) and then set the status to REG_VALID.
> 

I think I agree with Alan -- if we defer to raw_supply, then I'd still prefer
that the memset is still done in regcache_raw_supply_zero, because whether
unavailable registers actually have a contents buffer at all is
implementation detail.  We currently zero REG_UNVAILABLE registers in raw_supply,
but that could change.  (And if we reuse raw_supply as is, then memset, we'll
memset twice.)

BTW, note that gdbserver has an equivalent function, called
"supply_register_zeroed".

Thanks,
Pedro Alves

Re: [PATCH 8/11] Add FRV_MAX_REGISTER_SIZE

[Alan Hayward]

> On 3 May 2017, at 10:27, Pedro Alves <palves@redhat.com> wrote:
> 
> On 05/03/2017 09:44 AM, Yao Qi wrote:
>> Alan Hayward <Alan.Hayward@arm.com> writes:
>> 
>> Hi Alan,
>> regcache.c is updated, so please update your patch.
>> 
>>> I considered making regcache_raw_supply_zero call regcache_raw_supply, but
>>> in the end it made more sense to make it completely separate.
>> 
>> You can call raw_supply (regnum, NULL) and then set the status to REG_VALID.
>> 
> 
> I think I agree with Alan -- if we defer to raw_supply, then I'd still prefer
> that the memset is still done in regcache_raw_supply_zero, because whether
> unavailable registers actually have a contents buffer at all is
> implementation detail.  We currently zero REG_UNVAILABLE registers in raw_supply,
> but that could change.  (And if we reuse raw_supply as is, then memset, we'll
> memset twice.)
> 
> BTW, note that gdbserver has an equivalent function, called
> "supply_register_zeroed".
> 

Agreed. I didn't want somebody in the future to change raw_supply and cause
raw_supply_zero to break.

Patch updated to head and raw_supply_zero moved into regcache class.

Tested on a --enable-targets=all using make check with board files
unix and native-gdbserver.

Ok to commit?

Alan.

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

	* frv-linux-tdep.c (frv_linux_supply_gregset): Use raw_supply_zero.
	* regcache.c (regcache::raw_supply_zero): New function.
	* regcache.h (regcache::raw_supply_zero: New declaration.


diff --git a/gdb/frv-linux-tdep.c b/gdb/frv-linux-tdep.c
index eb87f93058b0287e8f05c585d1b6aa1ff2bffb78..30e5bf00c199a0193ea6cac3a14070445492d3fe 100644
--- a/gdb/frv-linux-tdep.c
+++ b/gdb/frv-linux-tdep.c
@@ -413,17 +413,14 @@ frv_linux_supply_gregset (const struct regset *regset,
 			  int regnum, const void *gregs, size_t len)
 {
   int regi;
-  char zerobuf[MAX_REGISTER_SIZE];
-
-  memset (zerobuf, 0, MAX_REGISTER_SIZE);

   /* gr0 always contains 0.  Also, the kernel passes the TBR value in
      this slot.  */
-  regcache_raw_supply (regcache, first_gpr_regnum, zerobuf);
+  regcache->raw_supply_zero (first_gpr_regnum);

   /* Fill gr32, ..., gr63 with zeros. */
   for (regi = first_gpr_regnum + 32; regi <= last_gpr_regnum; regi++)
-    regcache_raw_supply (regcache, regi, zerobuf);
+    regcache->raw_supply_zero (regi);

   regcache_supply_regset (regset, regcache, regnum, gregs, len);
 }
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 346d290b752d8809f78b72c104ccbc2f5574d83c..ee949597d5691b692b1162a92bc328266fdc160a 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -296,6 +296,8 @@ public:

   void raw_supply (int regnum, const void *buf);

+  void raw_supply_zero (int regnum);
+
   void raw_copy (int regnum, struct regcache *src_regcache);

   enum register_status get_register_status (int regnum) const;
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 748c30e2d4c5f572e9741f73a39c4bc555d1d663..89c2eb55e29af9a806a684382dc928c307ff934b 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -1208,6 +1208,26 @@ regcache::raw_supply (int regnum, const void *buf)
     }
 }

+/* Supply register REGNUM with zeroed value to REGCACHE.  This is not the same
+   as calling raw_supply with NULL (which will set the state to
+   unavailable).  */
+
+void
+regcache::raw_supply_zero (int regnum)
+{
+  void *regbuf;
+  size_t size;
+
+  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
+  gdb_assert (!m_readonly_p);
+
+  regbuf = register_buffer (regnum);
+  size = m_descr->sizeof_register[regnum];
+
+  memset (regbuf, 0, size);
+  m_register_status[regnum] = REG_VALID;
+}
+
 /* Collect register REGNUM from REGCACHE and store its contents in BUF.  */

 void

Re: [PATCH 8/11] Add FRV_MAX_REGISTER_SIZE

[Pedro Alves]

On 05/03/2017 11:56 AM, Alan Hayward wrote:

>> BTW, note that gdbserver has an equivalent function, called
>> "supply_register_zeroed".

> +/* Supply register REGNUM with zeroed value to REGCACHE.  This is not the same
> +   as calling raw_supply with NULL (which will set the state to
> +   unavailable).  */
> +
> +void
> +regcache::raw_supply_zero (int regnum)

A very minor detail, but I'd prefer it if gdb and gdbserver agreed 
on terminology.  Should we call this one "zeroed" too, or
rename gdbserver's to "zero"?

Thanks,
Pedro Alves

Re: [PATCH 8/11] Add FRV_MAX_REGISTER_SIZE

[Alan Hayward]

> On 3 May 2017, at 12:23, Pedro Alves <palves@redhat.com> wrote:
> 
> On 05/03/2017 11:56 AM, Alan Hayward wrote:
> 
>>> BTW, note that gdbserver has an equivalent function, called
>>> "supply_register_zeroed".
> 
>> +/* Supply register REGNUM with zeroed value to REGCACHE.  This is not the same
>> +   as calling raw_supply with NULL (which will set the state to
>> +   unavailable).  */
>> +
>> +void
>> +regcache::raw_supply_zero (int regnum)
> 
> A very minor detail, but I'd prefer it if gdb and gdbserver agreed 
> on terminology.  Should we call this one "zeroed" too, or
> rename gdbserver's to "zero"?
> 
> Thanks,
> Pedro Alves
> 

I’m happy to update raw_supply to raw_supply_zeroed.
I think that sounds better too.


Alan.

Re: [PATCH 8/11] Add FRV_MAX_REGISTER_SIZE

[Yao Qi]

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

Hi Alan,

> +/* Supply register REGNUM with zeroed value to REGCACHE.  This is not the same

REGCACHE don't have to be capitalized.  This line is too long.

> +   as calling raw_supply with NULL (which will set the state to
> +   unavailable).  */
> +
> +void
> +regcache::raw_supply_zero (int regnum)

s/raw_supply_zero/raw_supply_zeroed/

OK with these changes.

-- 
Yao (齐尧)