Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
* [PATCH] aarch64/gdbserver: fix floating point registers display
@ 2014-08-12  9:23 Catalin Udma
  2014-08-12  9:43 ` Richard Earnshaw
  0 siblings, 1 reply; 14+ messages in thread
From: Catalin Udma @ 2014-08-12  9:23 UTC (permalink / raw)
  To: gdb-patches; +Cc: Catalin Udma

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

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] aarch64/gdbserver: fix floating point registers display
  2014-08-12  9:23 [PATCH] aarch64/gdbserver: fix floating point registers display Catalin Udma
@ 2014-08-12  9:43 ` Richard Earnshaw
  2014-08-12 10:29   ` Yao Qi
  0 siblings, 1 reply; 14+ messages in thread
From: Richard Earnshaw @ 2014-08-12  9:43 UTC (permalink / raw)
  To: Catalin Udma; +Cc: gdb-patches

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
> 



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] aarch64/gdbserver: fix floating point registers display
  2014-08-12  9:43 ` Richard Earnshaw
@ 2014-08-12 10:29   ` Yao Qi
  2014-08-13 12:24     ` Philippe Waroquiers
  0 siblings, 1 reply; 14+ messages in thread
From: Yao Qi @ 2014-08-12 10:29 UTC (permalink / raw)
  To: Richard Earnshaw, Catalin Udma; +Cc: gdb-patches

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.

-- 
Yao (齐尧)


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] aarch64/gdbserver: fix floating point registers display
  2014-08-12 10:29   ` Yao Qi
@ 2014-08-13 12:24     ` Philippe Waroquiers
  2014-08-13 12:43       ` catalin.udma
       [not found]       ` <53EB5C86.4030307@codesourcery.com>
  0 siblings, 2 replies; 14+ messages in thread
From: Philippe Waroquiers @ 2014-08-13 12:24 UTC (permalink / raw)
  To: Yao Qi; +Cc: Richard Earnshaw, Catalin Udma, gdb-patches

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)


^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH] aarch64/gdbserver: fix floating point registers display
  2014-08-13 12:24     ` Philippe Waroquiers
@ 2014-08-13 12:43       ` catalin.udma
       [not found]       ` <53EB5C86.4030307@codesourcery.com>
  1 sibling, 0 replies; 14+ messages in thread
From: catalin.udma @ 2014-08-13 12:43 UTC (permalink / raw)
  To: Philippe Waroquiers, Yao Qi; +Cc: Richard Earnshaw, gdb-patches

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)


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] aarch64/gdbserver: fix floating point registers display
       [not found]       ` <53EB5C86.4030307@codesourcery.com>
@ 2014-08-13 14:42         ` Richard Earnshaw
  2014-08-20 17:36           ` Pedro Alves
  0 siblings, 1 reply; 14+ messages in thread
From: Richard Earnshaw @ 2014-08-13 14:42 UTC (permalink / raw)
  To: Yao Qi; +Cc: Philippe Waroquiers, Catalin Udma, gdb-patches

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.



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] aarch64/gdbserver: fix floating point registers display
  2014-08-13 14:42         ` Richard Earnshaw
@ 2014-08-20 17:36           ` Pedro Alves
  2014-08-21  6:54             ` catalin.udma
  0 siblings, 1 reply; 14+ messages in thread
From: Pedro Alves @ 2014-08-20 17:36 UTC (permalink / raw)
  To: Richard Earnshaw, Yao Qi; +Cc: Philippe Waroquiers, Catalin Udma, gdb-patches

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH] aarch64/gdbserver: fix floating point registers display
  2014-08-20 17:36           ` Pedro Alves
@ 2014-08-21  6:54             ` catalin.udma
  2014-08-21  6:56               ` Andrew Pinski
  0 siblings, 1 reply; 14+ messages in thread
From: catalin.udma @ 2014-08-21  6:54 UTC (permalink / raw)
  To: Pedro Alves, Richard Earnshaw, Yao Qi; +Cc: Philippe Waroquiers, gdb-patches

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] aarch64/gdbserver: fix floating point registers display
  2014-08-21  6:54             ` catalin.udma
@ 2014-08-21  6:56               ` Andrew Pinski
  2014-08-21  7:30                 ` Yao Qi
  0 siblings, 1 reply; 14+ messages in thread
From: Andrew Pinski @ 2014-08-21  6:56 UTC (permalink / raw)
  To: catalin.udma
  Cc: Pedro Alves, Richard Earnshaw, Yao Qi, Philippe Waroquiers, gdb-patches

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
>


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] aarch64/gdbserver: fix floating point registers display
  2014-08-21  6:56               ` Andrew Pinski
@ 2014-08-21  7:30                 ` Yao Qi
  2014-08-21 16:05                   ` Pedro Alves
  0 siblings, 1 reply; 14+ messages in thread
From: Yao Qi @ 2014-08-21  7:30 UTC (permalink / raw)
  To: Andrew Pinski, catalin.udma
  Cc: Pedro Alves, Richard Earnshaw, Philippe Waroquiers, gdb-patches

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.

-- 
Yao (齐尧)


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] aarch64/gdbserver: fix floating point registers display
  2014-08-21  7:30                 ` Yao Qi
@ 2014-08-21 16:05                   ` Pedro Alves
  2014-10-01  9:51                     ` Pedro Alves
  0 siblings, 1 reply; 14+ messages in thread
From: Pedro Alves @ 2014-08-21 16:05 UTC (permalink / raw)
  To: Yao Qi, Andrew Pinski, catalin.udma
  Cc: Richard Earnshaw, Philippe Waroquiers, gdb-patches

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] aarch64/gdbserver: fix floating point registers display
  2014-08-21 16:05                   ` Pedro Alves
@ 2014-10-01  9:51                     ` Pedro Alves
  2014-10-06  9:06                       ` catalin.udma
  0 siblings, 1 reply; 14+ messages in thread
From: Pedro Alves @ 2014-10-01  9:51 UTC (permalink / raw)
  To: Yao Qi, Andrew Pinski, catalin.udma
  Cc: Richard Earnshaw, Philippe Waroquiers, gdb-patches

On 08/21/2014 05:05 PM, Pedro Alves wrote:
> 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?

I'm giving the gdb/regformats/ dir some TLC today, and while
regenerating the .dat files stumbled on this.

I've now reverted the patch from mainline and the 7.8 branch, with
the patch below.

Catalin, can I get you to file a PR for the floating point issue,
and add it to:

 https://sourceware.org/gdb/wiki/GDB_7.8_Release

?

(that PR list is what goes into the 7.8.1 release announcement.)

Thanks.

--------
From 63fcc8bcd98a8cd9672dd8672f663662f8caa811 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 1 Oct 2014 10:44:08 +0100
Subject: [PATCH] Aarch64: Make CPSR a 32-bit register again in the target
 description

This reverts commit a4d9ba85 - 'AARCH64: Change cpsr type to be
64bit.'.

Even though Linux's ptrace exposes CPSR as 64-bit, CPSR is really
32-bit, and basing GDB's fundamentals on a particular OS's ptrace(2)
implementation is a bad idea.

In addition, while that commit intended to fix big endian Aarch64, it
ended up breaking floating point debugging against GDBserver, for both
big and little endian, because it changed the CPSR to be 64-bit in the
features/aarch64-core.xml file, but missed regenerating the
regformats/aarch64.dat file.  If we generate it now, we see this:

  diff --git c/gdb/regformats/aarch64.dat w/gdb/regformats/aarch64.dat
  index afe1028..0d32183 100644
  --- c/gdb/regformats/aarch64.dat
  +++ w/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

IOW, that commit left regformats/aarch64.dat still considering CPSR as
32-bits.  regformats/aarch64.dat is used by GDBserver for its internal
regcache layout, and for the g/G packet register block.  See the
generated aarch64.c file in GDBserver's build dir.

So the target description xml file that GDBserver reports to GDB is
now claiming that CPSR is 64-bit, but what GDBserver actually puts in
the g/G register packets is 32-bits.  Because GDB thinks CPSR is
64-bit (because that's what the XML description says), GDB will be
reading the remaining 32-bit bits of CPSR out of v0 (the register
immediately afterwards), and then all the registers that follow CPSR
in the register packet end up wrong in GDB, because they're being read
from the wrong offsets...

gdb/
2014-10-01  Pedro Alves  <palves@redhat.com>

	* features/aarch64-core.xml (cpsr): Change back to 32-bit.
	* features/aarch64.c: Regenerate.
---
 gdb/ChangeLog                 | 5 +++++
 gdb/features/aarch64-core.xml | 2 +-
 gdb/features/aarch64.c        | 2 +-
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 242fe26..f16db06 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2014-10-01  Pedro Alves  <palves@redhat.com>
+
+	* features/aarch64-core.xml (cpsr): Change back to 32-bit.
+	* features/aarch64.c: Regenerate.
+
 2014-09-11  Pedro Alves  <palves@redhat.com>
 
 	PR gdb/17347
diff --git a/gdb/features/aarch64-core.xml b/gdb/features/aarch64-core.xml
index dbec6dc..9b45a22 100644
--- a/gdb/features/aarch64-core.xml
+++ b/gdb/features/aarch64-core.xml
@@ -42,5 +42,5 @@
   <reg name="sp" bitsize="64" type="data_ptr"/>
 
   <reg name="pc" bitsize="64" type="code_ptr"/>
-  <reg name="cpsr" bitsize="64"/>
+  <reg name="cpsr" bitsize="32"/>
 </feature>
diff --git a/gdb/features/aarch64.c b/gdb/features/aarch64.c
index 31a148e..1e9a99d 100644
--- a/gdb/features/aarch64.c
+++ b/gdb/features/aarch64.c
@@ -50,7 +50,7 @@ initialize_tdesc_aarch64 (void)
   tdesc_create_reg (feature, "x30", 30, 1, NULL, 64, "int");
   tdesc_create_reg (feature, "sp", 31, 1, NULL, 64, "data_ptr");
   tdesc_create_reg (feature, "pc", 32, 1, NULL, 64, "code_ptr");
-  tdesc_create_reg (feature, "cpsr", 33, 1, NULL, 64, "int");
+  tdesc_create_reg (feature, "cpsr", 33, 1, NULL, 32, "int");
 
   feature = tdesc_create_feature (result, "org.gnu.gdb.aarch64.fpu");
   field_type = tdesc_named_type (feature, "ieee_double");
-- 
1.9.3



^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH] aarch64/gdbserver: fix floating point registers display
  2014-10-01  9:51                     ` Pedro Alves
@ 2014-10-06  9:06                       ` catalin.udma
  2014-10-06 12:28                         ` Pedro Alves
  0 siblings, 1 reply; 14+ messages in thread
From: catalin.udma @ 2014-10-06  9:06 UTC (permalink / raw)
  To: Pedro Alves, Yao Qi, Andrew Pinski
  Cc: Richard Earnshaw, Philippe Waroquiers, gdb-patches

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 612 bytes --]

> I'm giving the gdb/regformats/ dir some TLC today, and while
> regenerating the .dat files stumbled on this.
> 
> I've now reverted the patch from mainline and the 7.8 branch, with
> the patch below.
> 
> Catalin, can I get you to file a PR for the floating point issue,
> and add it to:
> 
>  https://sourceware.org/gdb/wiki/GDB_7.8_Release
> 
> ?
> 
> (that PR list is what goes into the 7.8.1 release announcement.)
> 
> Thanks.
[Catalin Udma] 
Hi,
I filled a PR for this issue:
https://sourceware.org/bugzilla/show_bug.cgi?id=17457

Regards,
Catalin
\x16º&Öéj×!zÊÞ¶êç×^¸ÛIb²Ö«r\x18\x1dn–­r\x17¬

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] aarch64/gdbserver: fix floating point registers display
  2014-10-06  9:06                       ` catalin.udma
@ 2014-10-06 12:28                         ` Pedro Alves
  0 siblings, 0 replies; 14+ messages in thread
From: Pedro Alves @ 2014-10-06 12:28 UTC (permalink / raw)
  To: catalin.udma, Yao Qi, Andrew Pinski
  Cc: Richard Earnshaw, Philippe Waroquiers, gdb-patches

On 10/06/2014 10:06 AM, catalin.udma@freescale.com wrote:
>> I've now reverted the patch from mainline and the 7.8 branch, with
>> the patch below.
>>
>> Catalin, can I get you to file a PR for the floating point issue,
>> and add it to:
>>
>>  https://sourceware.org/gdb/wiki/GDB_7.8_Release
>>
>> ?
>>
>> (that PR list is what goes into the 7.8.1 release announcement.)
>>
>> Thanks.
> [Catalin Udma] 
> Hi,
> I filled a PR for this issue:
> https://sourceware.org/bugzilla/show_bug.cgi?id=17457

Thank you.  I've added it to the wiki page.

Thanks,
Pedro Alves


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2014-10-06 12:28 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-12  9:23 [PATCH] aarch64/gdbserver: fix floating point registers display Catalin Udma
2014-08-12  9:43 ` Richard Earnshaw
2014-08-12 10:29   ` Yao Qi
2014-08-13 12:24     ` Philippe Waroquiers
2014-08-13 12:43       ` catalin.udma
     [not found]       ` <53EB5C86.4030307@codesourcery.com>
2014-08-13 14:42         ` Richard Earnshaw
2014-08-20 17:36           ` Pedro Alves
2014-08-21  6:54             ` catalin.udma
2014-08-21  6:56               ` Andrew Pinski
2014-08-21  7:30                 ` Yao Qi
2014-08-21 16:05                   ` Pedro Alves
2014-10-01  9:51                     ` Pedro Alves
2014-10-06  9:06                       ` catalin.udma
2014-10-06 12:28                         ` Pedro Alves

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox