aarch64/gdbserver: fix floating point registers display

  When using aarch64 gdb with gdbserver, floating point registers are
not correctly displayed, as below:
  (gdb) info registers fpsr fpcr
  fpsr           <unavailable>
  fpcr           <unavailable>
Also, the offset for floating point v0-v31 registers in gdbserver
is wrong because it is computed based on 32-bit size of CPSR register
as defined in the  regformat/aarch64.dat file

To fix these problems, the missing fpsr and fpcr registers are added
when floating point registers are read/write and the aarch64.dat file
is updated to use the correct CPSR size of 64-bits accordingly to the
definition in aarch64-core.xml

gdb/
2014-08-12  Catalin Udma  <catalin.udma@freescale.com>

        * regformats/aarch64.dat (cpsr): Change to be 64bit.

gdb/gdbserver/
2014-08-12  Catalin Udma  <catalin.udma@freescale.com>

        * linux-aarch64-low.c (AARCH64_FPSR_REGNO): New define.
        (AARCH64_FPCR_REGNO): Likewise.
        (AARCH64_NUM_REGS): Update to include fpsr/fpcr registers.
        (aarch64_fill_fpregset): Add missing fpsp/fpcr registers.
        (aarch64_store_fpregset): Likewise.
---
 gdb/gdbserver/linux-aarch64-low.c |    8 +++++++-
 gdb/regformats/aarch64.dat        |    2 +-
 2 files changed, 8 insertions(+), 2 deletions(-)
  

On 12/08/14 10:19, Catalin Udma wrote:
> When using aarch64 gdb with gdbserver, floating point registers are
> not correctly displayed, as below:
>   (gdb) info registers fpsr fpcr
>   fpsr           <unavailable>
>   fpcr           <unavailable>
> Also, the offset for floating point v0-v31 registers in gdbserver
> is wrong because it is computed based on 32-bit size of CPSR register
> as defined in the  regformat/aarch64.dat file
> 
> To fix these problems, the missing fpsr and fpcr registers are added
> when floating point registers are read/write and the aarch64.dat file
> is updated to use the correct CPSR size of 64-bits accordingly to the
> definition in aarch64-core.xml

This doesn't seem right to me.  The CPSR is a 32-bit register, not a
64-bit one.

R.

> 
> gdb/
> 2014-08-12  Catalin Udma  <catalin.udma@freescale.com>
> 
>         * regformats/aarch64.dat (cpsr): Change to be 64bit.
> 
> gdb/gdbserver/
> 2014-08-12  Catalin Udma  <catalin.udma@freescale.com>
> 
>         * linux-aarch64-low.c (AARCH64_FPSR_REGNO): New define.
>         (AARCH64_FPCR_REGNO): Likewise.
>         (AARCH64_NUM_REGS): Update to include fpsr/fpcr registers.
>         (aarch64_fill_fpregset): Add missing fpsp/fpcr registers.
>         (aarch64_store_fpregset): Likewise.
> ---
>  gdb/gdbserver/linux-aarch64-low.c |    8 +++++++-
>  gdb/regformats/aarch64.dat        |    2 +-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarch64-low.c
> index 6066e15..3453b2e 100644
> --- a/gdb/gdbserver/linux-aarch64-low.c
> +++ b/gdb/gdbserver/linux-aarch64-low.c
> @@ -46,8 +46,10 @@ extern const struct target_desc *tdesc_aarch64;
>  #define AARCH64_PC_REGNO   32
>  #define AARCH64_CPSR_REGNO 33
>  #define AARCH64_V0_REGNO   34
> +#define AARCH64_FPSR_REGNO (AARCH64_V0_REGNO + AARCH64_V_REGS_NUM)
> +#define AARCH64_FPCR_REGNO (AARCH64_V0_REGNO + AARCH64_V_REGS_NUM + 1)
>  
> -#define AARCH64_NUM_REGS (AARCH64_V0_REGNO + AARCH64_V_REGS_NUM)
> +#define AARCH64_NUM_REGS (AARCH64_V0_REGNO + AARCH64_V_REGS_NUM + 2)
>  
>  static int
>  aarch64_regmap [] =
> @@ -255,6 +257,8 @@ aarch64_fill_fpregset (struct regcache *regcache, void *buf)
>  
>    for (i = 0; i < AARCH64_V_REGS_NUM; i++)
>      collect_register (regcache, AARCH64_V0_REGNO + i, &regset->vregs[i]);
> +  collect_register (regcache, AARCH64_FPSR_REGNO, &regset->fpsr);
> +  collect_register (regcache, AARCH64_FPCR_REGNO, &regset->fpcr);
>  }
>  
>  static void
> @@ -265,6 +269,8 @@ aarch64_store_fpregset (struct regcache *regcache, const void *buf)
>  
>    for (i = 0; i < AARCH64_V_REGS_NUM; i++)
>      supply_register (regcache, AARCH64_V0_REGNO + i, &regset->vregs[i]);
> +  supply_register (regcache, AARCH64_FPSR_REGNO, &regset->fpsr);
> +  supply_register (regcache, AARCH64_FPCR_REGNO, &regset->fpcr);
>  }
>  
>  /* Debugging of hardware breakpoint/watchpoint support.  */
> diff --git a/gdb/regformats/aarch64.dat b/gdb/regformats/aarch64.dat
> index afe1028..0d32183 100644
> --- a/gdb/regformats/aarch64.dat
> +++ b/gdb/regformats/aarch64.dat
> @@ -35,7 +35,7 @@ expedite:x29,sp,pc
>  64:x30
>  64:sp
>  64:pc
> -32:cpsr
> +64:cpsr
>  128:v0
>  128:v1
>  128:v2
>
  

On 08/12/2014 05:43 PM, Richard Earnshaw wrote:
> This doesn't seem right to me.  The CPSR is a 32-bit register, not a
> 64-bit one.

cpsr is 64-bit in the target description, see
gdb/features/aarch64-core.xml,

  <reg name="cpsr" bitsize="64"/>

We need to fix it.
  

On Tue, 2014-08-12 at 18:25 +0800, Yao Qi wrote:
> On 08/12/2014 05:43 PM, Richard Earnshaw wrote:
> > This doesn't seem right to me.  The CPSR is a 32-bit register, not a
> > 64-bit one.
> 
> cpsr is 64-bit in the target description, see
> gdb/features/aarch64-core.xml,
> 
>   <reg name="cpsr" bitsize="64"/>
> 
> We need to fix it.
The 'it' in 'fix it' is ambiguous to me.
Does the 'it' mean:
     fix aarch64-core.xml to change cpsr to 32 bits ?
or does that confirm the initial proposal i.e.
     fix e.g. aarch64.dat to change cpsr to 64 bits ?

Philippe
(following the discussion, to update Valgrind aarch64 gdbserver
accordingly)
  

On 08/13/2014 08:25 PM, Philippe Waroquiers wrote:
> The 'it' in 'fix it' is ambiguous to me.
> Does the 'it' mean:
>      fix aarch64-core.xml to change cpsr to 32 bits ?

That was what I meant, however ....

> or does that confirm the initial proposal i.e.
>      fix e.g. aarch64.dat to change cpsr to 64 bits ?

... I find a patch changed cpsr to 64 bit in last Dec.

 [PATCH] AARCH64: Change cpsr type to be 64bit.
 https://sourceware.org/ml/gdb-patches/2013-12/msg00720.html

and looks aarch64.dat was not updated together in this patch.

I am sure that aarch64.dat and aarch64-core.xml are not in sync,
but I don't know which way to go, sorry.
  

Hi, all,

I'll add some details, just to have a better view about the cpsr size changes:

- CPSR register size was initially 32 bits (features/aarch64-core.xml), but the
 register size has been changed in commit a4d9ba85e (2013-12-18): 
   features/aarch64-core.xml (cpsr): Changed to be 64 bits

- When Changing .xml file, there should be regenerated accordingly aarch64.c and 
regformats/aarch64.dat, to match the new size definition in the XML file.
- features/aarch64.c has been regenerated in the above mentioned commit, while the
file regformats/aarch64.dat wasn't regenerated, thus using the previous cpsr size of 32 bits


The regformats/aarch64.dat update in my commit fixes the de-synchronization problem
where gdb is seeing the cpsr size 64-bits while gdbserver 32-bits.

Regards,
Catalin


> -----Original Message-----

> From: Philippe Waroquiers [mailto:philippe.waroquiers@skynet.be]

> Sent: Wednesday, August 13, 2014 3:26 PM

> To: Yao Qi

> Cc: Richard Earnshaw; Udma Catalin-Dan-B32721; gdb-patches@sourceware.org

> Subject: Re: [PATCH] aarch64/gdbserver: fix floating point registers

> display

> 

> On Tue, 2014-08-12 at 18:25 +0800, Yao Qi wrote:

> > On 08/12/2014 05:43 PM, Richard Earnshaw wrote:

> > > This doesn't seem right to me.  The CPSR is a 32-bit register, not a

> > > 64-bit one.

> >

> > cpsr is 64-bit in the target description, see

> > gdb/features/aarch64-core.xml,

> >

> >   <reg name="cpsr" bitsize="64"/>

> >

> > We need to fix it.

> The 'it' in 'fix it' is ambiguous to me.

> Does the 'it' mean:

>      fix aarch64-core.xml to change cpsr to 32 bits ?

> or does that confirm the initial proposal i.e.

>      fix e.g. aarch64.dat to change cpsr to 64 bits ?

> 

> Philippe

> (following the discussion, to update Valgrind aarch64 gdbserver

> accordingly)
  

On 13/08/14 13:39, Yao Qi wrote:
> On 08/13/2014 08:25 PM, Philippe Waroquiers wrote:
>> The 'it' in 'fix it' is ambiguous to me.
>> Does the 'it' mean:
>>      fix aarch64-core.xml to change cpsr to 32 bits ?
> 
> That was what I meant, however ....
> 
>> or does that confirm the initial proposal i.e.
>>      fix e.g. aarch64.dat to change cpsr to 64 bits ?
> 
> ... I find a patch changed cpsr to 64 bit in last Dec.
> 
>  [PATCH] AARCH64: Change cpsr type to be 64bit.
>  https://sourceware.org/ml/gdb-patches/2013-12/msg00720.html
> 
> and looks aarch64.dat was not updated together in this patch.
> 
> I am sure that aarch64.dat and aarch64-core.xml are not in sync,
> but I don't know which way to go, sorry.
> 

Changing the XML doesn't sound like the right way forward, the XML can
be embedded into other components as part of the register description
interface.

Hmm, I can't see where anyone ever formally approved that change.  In
fact, Mark K commented at the time:

"Basing GDB's fundamentals on a particular OS's ptrace(2)
implementation is a bad idea."

So it seems to me that that change was indeed incorrect and should
probably be reverted (at least in its current incarnation).

R.
  

On 08/13/2014 03:42 PM, Richard Earnshaw wrote:
> On 13/08/14 13:39, Yao Qi wrote:
>> On 08/13/2014 08:25 PM, Philippe Waroquiers wrote:
>>> The 'it' in 'fix it' is ambiguous to me.
>>> Does the 'it' mean:
>>>      fix aarch64-core.xml to change cpsr to 32 bits ?
>>
>> That was what I meant, however ....
>>
>>> or does that confirm the initial proposal i.e.
>>>      fix e.g. aarch64.dat to change cpsr to 64 bits ?
>>
>> ... I find a patch changed cpsr to 64 bit in last Dec.
>>
>>  [PATCH] AARCH64: Change cpsr type to be 64bit.
>>  https://sourceware.org/ml/gdb-patches/2013-12/msg00720.html
>>
>> and looks aarch64.dat was not updated together in this patch.
>>
>> I am sure that aarch64.dat and aarch64-core.xml are not in sync,
>> but I don't know which way to go, sorry.
>>
> 
> Changing the XML doesn't sound like the right way forward, the XML can
> be embedded into other components as part of the register description
> interface.
> 
> Hmm, I can't see where anyone ever formally approved that change.  In
> fact, Mark K commented at the time:
> 
> "Basing GDB's fundamentals on a particular OS's ptrace(2)
> implementation is a bad idea."
> 
> So it seems to me that that change was indeed incorrect and should
> probably be reverted (at least in its current incarnation).

I agree, and I'm surprised to learn the patch went in.  :-/

Thanks,
Pedro Alves
  

Thank you all for your comments.
As a follow-up, should I re-submit my patch without changing 
cpsr size in regformats/aarch64.dat? ... While  the current cpsr
size de-synchronization would be fixed by reverting the patch 
we are discussing about?

Regards,
Catalin

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Wednesday, August 20, 2014 8:36 PM
> To: Richard Earnshaw; Yao Qi
> Cc: Philippe Waroquiers; Udma Catalin-Dan-B32721; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH] aarch64/gdbserver: fix floating point registers
> display
> 
> On 08/13/2014 03:42 PM, Richard Earnshaw wrote:
> > On 13/08/14 13:39, Yao Qi wrote:
> >> On 08/13/2014 08:25 PM, Philippe Waroquiers wrote:
> >>> The 'it' in 'fix it' is ambiguous to me.
> >>> Does the 'it' mean:
> >>>      fix aarch64-core.xml to change cpsr to 32 bits ?
> >>
> >> That was what I meant, however ....
> >>
> >>> or does that confirm the initial proposal i.e.
> >>>      fix e.g. aarch64.dat to change cpsr to 64 bits ?
> >>
> >> ... I find a patch changed cpsr to 64 bit in last Dec.
> >>
> >>  [PATCH] AARCH64: Change cpsr type to be 64bit.
> >>  https://sourceware.org/ml/gdb-patches/2013-12/msg00720.html
> >>
> >> and looks aarch64.dat was not updated together in this patch.
> >>
> >> I am sure that aarch64.dat and aarch64-core.xml are not in sync,
> >> but I don't know which way to go, sorry.
> >>
> >
> > Changing the XML doesn't sound like the right way forward, the XML can
> > be embedded into other components as part of the register description
> > interface.
> >
> > Hmm, I can't see where anyone ever formally approved that change.  In
> > fact, Mark K commented at the time:
> >
> > "Basing GDB's fundamentals on a particular OS's ptrace(2)
> > implementation is a bad idea."
> >
> > So it seems to me that that change was indeed incorrect and should
> > probably be reverted (at least in its current incarnation).
> 
> I agree, and I'm surprised to learn the patch went in.  :-/
> 
> Thanks,
> Pedro Alves
  

On Wed, Aug 20, 2014 at 11:54 PM, catalin.udma@freescale.com
<catalin.udma@freescale.com> wrote:
> Thank you all for your comments.
> As a follow-up, should I re-submit my patch without changing
> cpsr size in regformats/aarch64.dat? ... While  the current cpsr
> size de-synchronization would be fixed by reverting the patch
> we are discussing about?

If we revert the patch is someone going to fix big-endian support then
since that is the original why it was requested.

Thanks,
Andrew

>
> Regards,
> Catalin
>
>> -----Original Message-----
>> From: Pedro Alves [mailto:palves@redhat.com]
>> Sent: Wednesday, August 20, 2014 8:36 PM
>> To: Richard Earnshaw; Yao Qi
>> Cc: Philippe Waroquiers; Udma Catalin-Dan-B32721; gdb-
>> patches@sourceware.org
>> Subject: Re: [PATCH] aarch64/gdbserver: fix floating point registers
>> display
>>
>> On 08/13/2014 03:42 PM, Richard Earnshaw wrote:
>> > On 13/08/14 13:39, Yao Qi wrote:
>> >> On 08/13/2014 08:25 PM, Philippe Waroquiers wrote:
>> >>> The 'it' in 'fix it' is ambiguous to me.
>> >>> Does the 'it' mean:
>> >>>      fix aarch64-core.xml to change cpsr to 32 bits ?
>> >>
>> >> That was what I meant, however ....
>> >>
>> >>> or does that confirm the initial proposal i.e.
>> >>>      fix e.g. aarch64.dat to change cpsr to 64 bits ?
>> >>
>> >> ... I find a patch changed cpsr to 64 bit in last Dec.
>> >>
>> >>  [PATCH] AARCH64: Change cpsr type to be 64bit.
>> >>  https://sourceware.org/ml/gdb-patches/2013-12/msg00720.html
>> >>
>> >> and looks aarch64.dat was not updated together in this patch.
>> >>
>> >> I am sure that aarch64.dat and aarch64-core.xml are not in sync,
>> >> but I don't know which way to go, sorry.
>> >>
>> >
>> > Changing the XML doesn't sound like the right way forward, the XML can
>> > be embedded into other components as part of the register description
>> > interface.
>> >
>> > Hmm, I can't see where anyone ever formally approved that change.  In
>> > fact, Mark K commented at the time:
>> >
>> > "Basing GDB's fundamentals on a particular OS's ptrace(2)
>> > implementation is a bad idea."
>> >
>> > So it seems to me that that change was indeed incorrect and should
>> > probably be reverted (at least in its current incarnation).
>>
>> I agree, and I'm surprised to learn the patch went in.  :-/
>>
>> Thanks,
>> Pedro Alves
>
  

On 08/21/2014 02:56 PM, Andrew Pinski wrote:
> If we revert the patch is someone going to fix big-endian support then
> since that is the original why it was requested.

I prefer to revert the patch, because it has problem.  How to fix
big-endian support is another issue.
  

On 08/21/2014 08:26 AM, Yao Qi wrote:
> On 08/21/2014 02:56 PM, Andrew Pinski wrote:
>> If we revert the patch is someone going to fix big-endian support then
>> since that is the original why it was requested.
> 
> I prefer to revert the patch, because it has problem.  How to fix
> big-endian support is another issue.

Agreed.  It just seems to me that gdbserver needs to extract
the right 32-bits out of the ptrace buffer?

Thanks,
Pedro Alves