[7/11] Add BFIN_MAX_REGISTER_SIZE

  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.
  

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);
  

> 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 (齐尧)
  

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.
  

> 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.
  

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.
  

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
  

> 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.
  

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).
  

> 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.
  

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