[PATCH 7/11] Add BFIN_MAX_REGISTER_SIZE

[PATCH 7/11] Add BFIN_MAX_REGISTER_SIZE

[Alan Hayward]

Max size set to 32bits, which I determined using regformats/reg-bfin.dat

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

I do not have an BFIN machine to test on.

Ok to commit?

Alan.


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

	* bfin-tdep.c (bfin_pseudo_register_read): Use BFIN_MAX_REGISTER_SIZE.
	(bfin_pseudo_register_write): Likewise.
	* bfin-tdep.h (BFIN_MAX_REGISTER_SIZE): Add.


diff --git a/gdb/bfin-tdep.h b/gdb/bfin-tdep.h
index 164466c2d6d1bd2dc0bc47ca3729d0e42e2d7ccb..e89e5c3138eb8245b10ccda98f0672e8e3fd728b 100644
--- a/gdb/bfin-tdep.h
+++ b/gdb/bfin-tdep.h
@@ -84,6 +84,9 @@ enum gdb_regnum {
 #define BFIN_NUM_REGS (BFIN_PC_REGNUM + 1)
 #define BFIN_NUM_PSEUDO_REGS (1)

+/* Big enough to hold the size of the largest register in bytes.  */
+#define BFIN_MAX_REGISTER_SIZE	4
+
 /* The ABIs for Blackfin.  */
 enum bfin_abi
 {
diff --git a/gdb/bfin-tdep.c b/gdb/bfin-tdep.c
index 3df1ba387a323dc6827b1189432f8877d1833184..9b45633cab15b8e0adb0d51a2fa650dc2bc6339b 100644
--- a/gdb/bfin-tdep.c
+++ b/gdb/bfin-tdep.c
@@ -689,7 +689,7 @@ static enum register_status
 bfin_pseudo_register_read (struct gdbarch *gdbarch, struct regcache *regcache,
 			   int regnum, gdb_byte *buffer)
 {
-  gdb_byte *buf = (gdb_byte *) alloca (MAX_REGISTER_SIZE);
+  gdb_byte *buf = (gdb_byte *) alloca (BFIN_MAX_REGISTER_SIZE);
   enum register_status status;

   if (regnum != BFIN_CC_REGNUM)
@@ -710,7 +710,7 @@ static void
 bfin_pseudo_register_write (struct gdbarch *gdbarch, struct regcache *regcache,
 			    int regnum, const gdb_byte *buffer)
 {
-  gdb_byte *buf = (gdb_byte *) alloca (MAX_REGISTER_SIZE);
+  gdb_byte *buf = (gdb_byte *) alloca (BFIN_MAX_REGISTER_SIZE);

   if (regnum != BFIN_CC_REGNUM)
     internal_error (__FILE__, __LINE__,

Re: [PATCH 7/11] Add BFIN_MAX_REGISTER_SIZE

[Yao Qi]

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

> diff --git a/gdb/bfin-tdep.c b/gdb/bfin-tdep.c
> index 3df1ba387a323dc6827b1189432f8877d1833184..9b45633cab15b8e0adb0d51a2fa650dc2bc6339b 100644
> --- a/gdb/bfin-tdep.c
> +++ b/gdb/bfin-tdep.c
> @@ -689,7 +689,7 @@ static enum register_status
>  bfin_pseudo_register_read (struct gdbarch *gdbarch, struct regcache *regcache,
>  			   int regnum, gdb_byte *buffer)
>  {
> -  gdb_byte *buf = (gdb_byte *) alloca (MAX_REGISTER_SIZE);
> +  gdb_byte *buf = (gdb_byte *) alloca (BFIN_MAX_REGISTER_SIZE);

Why don't you do "gdb_byte buf[4];"?  It is only for CC register which
is 32-bit.

>    enum register_status status;
>
>    if (regnum != BFIN_CC_REGNUM)
> @@ -710,7 +710,7 @@ static void
>  bfin_pseudo_register_write (struct gdbarch *gdbarch, struct regcache *regcache,
>  			    int regnum, const gdb_byte *buffer)
>  {
> -  gdb_byte *buf = (gdb_byte *) alloca (MAX_REGISTER_SIZE);
> +  gdb_byte *buf = (gdb_byte *) alloca (BFIN_MAX_REGISTER_SIZE);

-- 
Yao (齐尧)

Re: [PATCH 7/11] Add BFIN_MAX_REGISTER_SIZE

[Alan Hayward]

> On 5 Apr 2017, at 11:28, Yao Qi <qiyaoltc@gmail.com> wrote:
> 
> Alan Hayward <Alan.Hayward@arm.com> writes:
> 
>> diff --git a/gdb/bfin-tdep.c b/gdb/bfin-tdep.c
>> index 3df1ba387a323dc6827b1189432f8877d1833184..9b45633cab15b8e0adb0d51a2fa650dc2bc6339b 100644
>> --- a/gdb/bfin-tdep.c
>> +++ b/gdb/bfin-tdep.c
>> @@ -689,7 +689,7 @@ static enum register_status
>> bfin_pseudo_register_read (struct gdbarch *gdbarch, struct regcache *regcache,
>> 			   int regnum, gdb_byte *buffer)
>> {
>> -  gdb_byte *buf = (gdb_byte *) alloca (MAX_REGISTER_SIZE);
>> +  gdb_byte *buf = (gdb_byte *) alloca (BFIN_MAX_REGISTER_SIZE);
> 
> Why don't you do "gdb_byte buf[4];"?  It is only for CC register which
> is 32-bit.
> 

Is it not clearer code to add and use a macro rather than a magic number ?

It’s also not obvious anywhere that the astat register is 32bits. I had to go
digging inside regformats/reg-bfin.dat before I found it out.

Given that BFIN_MAX_REGISTER_SIZE is also 4, it compiles to the same size anyway.


>>   enum register_status status;
>> 
>>   if (regnum != BFIN_CC_REGNUM)
>> @@ -710,7 +710,7 @@ static void
>> bfin_pseudo_register_write (struct gdbarch *gdbarch, struct regcache *regcache,
>> 			    int regnum, const gdb_byte *buffer)
>> {
>> -  gdb_byte *buf = (gdb_byte *) alloca (MAX_REGISTER_SIZE);
>> +  gdb_byte *buf = (gdb_byte *) alloca (BFIN_MAX_REGISTER_SIZE);
> 
> -- 
> Yao (齐尧)

Re: [PATCH 7/11] Add BFIN_MAX_REGISTER_SIZE

[Andreas Schwab]

On Apr 05 2017, Alan Hayward <Alan.Hayward@arm.com> wrote:

>> On 5 Apr 2017, at 11:28, Yao Qi <qiyaoltc@gmail.com> wrote:
>> 
>> Alan Hayward <Alan.Hayward@arm.com> writes:
>> 
>>> diff --git a/gdb/bfin-tdep.c b/gdb/bfin-tdep.c
>>> index 3df1ba387a323dc6827b1189432f8877d1833184..9b45633cab15b8e0adb0d51a2fa650dc2bc6339b 100644
>>> --- a/gdb/bfin-tdep.c
>>> +++ b/gdb/bfin-tdep.c
>>> @@ -689,7 +689,7 @@ static enum register_status
>>> bfin_pseudo_register_read (struct gdbarch *gdbarch, struct regcache *regcache,
>>> 			   int regnum, gdb_byte *buffer)
>>> {
>>> -  gdb_byte *buf = (gdb_byte *) alloca (MAX_REGISTER_SIZE);
>>> +  gdb_byte *buf = (gdb_byte *) alloca (BFIN_MAX_REGISTER_SIZE);
>> 
>> Why don't you do "gdb_byte buf[4];"?  It is only for CC register which
>> is 32-bit.
>> 
>
> Is it not clearer code to add and use a macro rather than a magic number ?
>
> It’s also not obvious anywhere that the astat register is 32bits. I had to go
> digging inside regformats/reg-bfin.dat before I found it out.
>
> Given that BFIN_MAX_REGISTER_SIZE is also 4, it compiles to the same size anyway.

Since BFIN_MAX_REGISTER_SIZE is a constant you don't need alloca either
way.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH 7/11] Add BFIN_MAX_REGISTER_SIZE

[Alan Hayward]

> On 5 Apr 2017, at 14:50, Andreas Schwab <schwab@suse.de> wrote:
> 
> On Apr 05 2017, Alan Hayward <Alan.Hayward@arm.com> wrote:
> 
>>> On 5 Apr 2017, at 11:28, Yao Qi <qiyaoltc@gmail.com> wrote:
>>> 
>>> Alan Hayward <Alan.Hayward@arm.com> writes:
>>> 
>>>> diff --git a/gdb/bfin-tdep.c b/gdb/bfin-tdep.c
>>>> index 3df1ba387a323dc6827b1189432f8877d1833184..9b45633cab15b8e0adb0d51a2fa650dc2bc6339b 100644
>>>> --- a/gdb/bfin-tdep.c
>>>> +++ b/gdb/bfin-tdep.c
>>>> @@ -689,7 +689,7 @@ static enum register_status
>>>> bfin_pseudo_register_read (struct gdbarch *gdbarch, struct regcache *regcache,
>>>> 			   int regnum, gdb_byte *buffer)
>>>> {
>>>> -  gdb_byte *buf = (gdb_byte *) alloca (MAX_REGISTER_SIZE);
>>>> +  gdb_byte *buf = (gdb_byte *) alloca (BFIN_MAX_REGISTER_SIZE);
>>> 
>>> Why don't you do "gdb_byte buf[4];"?  It is only for CC register which
>>> is 32-bit.
>>> 
>> 
>> Is it not clearer code to add and use a macro rather than a magic number ?
>> 
>> It’s also not obvious anywhere that the astat register is 32bits. I had to go
>> digging inside regformats/reg-bfin.dat before I found it out.
>> 
>> Given that BFIN_MAX_REGISTER_SIZE is also 4, it compiles to the same size anyway.
> 
> Since BFIN_MAX_REGISTER_SIZE is a constant you don't need alloca either
> way.
> 

Sorry, yes, I meant to say:

Why not use:
gdb_byte buf[BFIN_MAX_REGISTER_SIZE];

Rather than
gdb_byte buf[4];

Given that a macro is clearer than a magic number.

Alan.

Re: [PATCH 7/11] Add BFIN_MAX_REGISTER_SIZE

[Yao Qi]

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

> Is it not clearer code to add and use a macro rather than a magic number ?
>

Macro is better, but "buf[4]" is not that magic, because it is only used
within bfin_pseudo_register_read, or we can define ASTAT_REGISTER_SIZE.

> It’s also not obvious anywhere that the astat register is 32bits. I had to go
> digging inside regformats/reg-bfin.dat before I found it out.

It is easier to figure out the size of a specific register than the max
size of a set of registers.

>
> Given that BFIN_MAX_REGISTER_SIZE is also 4, it compiles to the same
> size anyway.

"gdb_byte buf[BFIN_MAX_REGISTER_SIZE]" is fine to me.  Could you define
BFIN_MAX_REGISTER_SIZE in bfin-tdep.c instead of .h?  It is not used
elsewhere.

-- 
Yao (齐尧)

Re: [PATCH 7/11] Add BFIN_MAX_REGISTER_SIZE

[Alan Hayward]

> On 5 Apr 2017, at 15:10, Yao Qi <qiyaoltc@gmail.com> wrote:
> 
> Alan Hayward <Alan.Hayward@arm.com> writes:
> 
>> Is it not clearer code to add and use a macro rather than a magic number ?
>> 
> 
> Macro is better, but "buf[4]" is not that magic, because it is only used
> within bfin_pseudo_register_read, or we can define ASTAT_REGISTER_SIZE.
> 
>> It’s also not obvious anywhere that the astat register is 32bits. I had to go
>> digging inside regformats/reg-bfin.dat before I found it out.
> 
> It is easier to figure out the size of a specific register than the max
> size of a set of registers.
> 
>> 
>> Given that BFIN_MAX_REGISTER_SIZE is also 4, it compiles to the same
>> size anyway.
> 
> "gdb_byte buf[BFIN_MAX_REGISTER_SIZE]" is fine to me.  Could you define
> BFIN_MAX_REGISTER_SIZE in bfin-tdep.c instead of .h?  It is not used
> elsewhere.
> 

Ok, pushed the following:

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

	* bfin-tdep.c (BFIN_MAX_REGISTER_SIZE): Add.
	(bfin_pseudo_register_read): Use BFIN_MAX_REGISTER_SIZE.
	(bfin_pseudo_register_write): Likewise


diff --git a/gdb/bfin-tdep.c b/gdb/bfin-tdep.c
index 3df1ba387a323dc6827b1189432f8877d1833184..c590a9132c00f539b5c6e57b8f156845ff50acad 100644
--- a/gdb/bfin-tdep.c
+++ b/gdb/bfin-tdep.c
@@ -241,6 +241,8 @@ static const int map_gcc_gdb[] =
   BFIN_LB1_REGNUM
 };

+/* Big enough to hold the size of the largest register in bytes.  */
+#define BFIN_MAX_REGISTER_SIZE	4

 struct bfin_frame_cache
 {
@@ -689,7 +691,7 @@ static enum register_status
 bfin_pseudo_register_read (struct gdbarch *gdbarch, struct regcache *regcache,
 			   int regnum, gdb_byte *buffer)
 {
-  gdb_byte *buf = (gdb_byte *) alloca (MAX_REGISTER_SIZE);
+  gdb_byte buf[BFIN_MAX_REGISTER_SIZE];
   enum register_status status;

   if (regnum != BFIN_CC_REGNUM)
@@ -710,7 +712,7 @@ static void
 bfin_pseudo_register_write (struct gdbarch *gdbarch, struct regcache *regcache,
 			    int regnum, const gdb_byte *buffer)
 {
-  gdb_byte *buf = (gdb_byte *) alloca (MAX_REGISTER_SIZE);
+  gdb_byte buf[BFIN_MAX_REGISTER_SIZE];

   if (regnum != BFIN_CC_REGNUM)
     internal_error (__FILE__, __LINE__,

Re: [PATCH 7/11] Add BFIN_MAX_REGISTER_SIZE

[Pedro Alves]

On 04/04/2017 11:14 AM, Alan Hayward wrote:
> Max size set to 32bits, which I determined using regformats/reg-bfin.dat

Makes me wonder sth (and in general, not for this patch in particular):

Is it possible that any of these code paths that hardcode an arch specific
max register size end up seeing a larger register size because the reported
xml target description includes such a larger register?

E.g., say arch A normally only has 32-bit registers, for as much GDB knows.
And then some stub for some variant of A includes a register
in the description like:

  <reg name="foo" bitsize="64" type="uint64"/>

It kinds of sounds like the max register size is capped by what target
descriptions can describe for that architecture, not exactly by the size
of the registers that GDB considers "core" registers.  That may
already have been taken into account and it may well be that the paths
that use the FOO_MAX_REGISTER_SIZE macros only ever work with registers
that GDB does know about (haven't checked carefully), rendering the concern
moot, but I wanted to put the thought out there anyway.

Thanks,
Pedro Alves

Re: [PATCH 7/11] Add BFIN_MAX_REGISTER_SIZE

[Alan Hayward]

> On 5 Apr 2017, at 16:03, Pedro Alves <palves@redhat.com> wrote:
> 
> On 04/04/2017 11:14 AM, Alan Hayward wrote:
>> Max size set to 32bits, which I determined using regformats/reg-bfin.dat
> 
> Makes me wonder sth (and in general, not for this patch in particular):
> 
> Is it possible that any of these code paths that hardcode an arch specific
> max register size end up seeing a larger register size because the reported
> xml target description includes such a larger register?
> 
> E.g., say arch A normally only has 32-bit registers, for as much GDB knows.
> And then some stub for some variant of A includes a register
> in the description like:
> 
>  <reg name="foo" bitsize="64" type="uint64"/>
> 
> It kinds of sounds like the max register size is capped by what target
> descriptions can describe for that architecture, not exactly by the size
> of the registers that GDB considers "core" registers.  That may
> already have been taken into account and it may well be that the paths
> that use the FOO_MAX_REGISTER_SIZE macros only ever work with registers
> that GDB does know about (haven't checked carefully), rendering the concern
> moot, but I wanted to put the thought out there anyway.
> 
> Thanks,
> Pedro Alves
> 

There is some existing code in regcache.c that checks that no register in the
target descriptor is greater than MAX_REGISTER_SIZE.
Obviously, this code will vanish when MAX_REGISTER_SIZE disappears.

If people think that this is an important check to have, then maybe there needs
to be an additional patch set. For each target with a FOO_MAX_REGISTER_SIZE,
in the init code for that target add:
  gdb_assert (FOO_MAX_REGISTER_SIZE >= max_register_size (gdbarch));
?

(To keep this simple I’d do it after adding these patches which add
 FOO_MAX_REGISTER_SIZE).


Alan.

Re: [PATCH 7/11] Add BFIN_MAX_REGISTER_SIZE

[Yao Qi]

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

> There is some existing code in regcache.c that checks that no register in the
> target descriptor is greater than MAX_REGISTER_SIZE.
> Obviously, this code will vanish when MAX_REGISTER_SIZE disappears.
>
> If people think that this is an important check to have, then maybe there needs
> to be an additional patch set. For each target with a FOO_MAX_REGISTER_SIZE,
> in the init code for that target add:
>   gdb_assert (FOO_MAX_REGISTER_SIZE >= max_register_size (gdbarch));
> ?

It is not just about checking.  What if the assert fail?
FOO_MAX_REGISTER_SIZE is a strong claim for arch FOO.  That is why I
suggested "or we can define ASTAT_REGISTER_SIZE".

>
> (To keep this simple I’d do it after adding these patches which add
>  FOO_MAX_REGISTER_SIZE).

-- 
Yao (齐尧)

Re: [PATCH 7/11] Add BFIN_MAX_REGISTER_SIZE

[Alan Hayward]

> On 7 Apr 2017, at 17:04, Yao Qi <qiyaoltc@gmail.com> wrote:
> 
> Alan Hayward <Alan.Hayward@arm.com> writes:
> 
>> There is some existing code in regcache.c that checks that no register in the
>> target descriptor is greater than MAX_REGISTER_SIZE.
>> Obviously, this code will vanish when MAX_REGISTER_SIZE disappears.
>> 
>> If people think that this is an important check to have, then maybe there needs
>> to be an additional patch set. For each target with a FOO_MAX_REGISTER_SIZE,
>> in the init code for that target add:
>>  gdb_assert (FOO_MAX_REGISTER_SIZE >= max_register_size (gdbarch));
>> ?
> 
> It is not just about checking.  What if the assert fail?
> FOO_MAX_REGISTER_SIZE is a strong claim for arch FOO.  That is why I
> suggested "or we can define ASTAT_REGISTER_SIZE".
> 

The checks would be added as a replacement for what is already in
init_regcache_descr() code in regcache.c:

    for (i = 0; i < descr->nr_raw_registers; i++)
      {
	<snip>
	gdb_assert (MAX_REGISTER_SIZE >= descr->sizeof_register[i]);
        <snip>
      }

If the assert fails then the its not safe for gdb to run - the define
should always be big enough to hold any register on the current architecture


Alan.

Re: [PATCH 7/11] Add BFIN_MAX_REGISTER_SIZE

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 437 bytes --]

On 04 Apr 2017 10:14, Alan Hayward wrote:
> Max size set to 32bits, which I determined using regformats/reg-bfin.dat
> 
> Tested on a --enable-targets=all build using make check with board files
> unix and native-gdbserver.
> 
> I do not have an BFIN machine to test on.

yes, Blackfin only has 32-bit registers.  it has two 40-bit accumulators
(A0 & A1), but they're read/written as 32-bit (A0.W) & 8-bit (A0.X) pairs.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]