Message ID | 1407835166-827-1-git-send-email-catalin.udma@freescale.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 21991 invoked by alias); 12 Aug 2014 09:23:41 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 21971 invoked by uid 89); 12 Aug 2014 09:23:39 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.1 required=5.0 tests=AWL, BAYES_20, RCVD_IN_DNSWL_LOW, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 X-HELO: na01-bl2-obe.outbound.protection.outlook.com Received: from mail-bl2lp0203.outbound.protection.outlook.com (HELO na01-bl2-obe.outbound.protection.outlook.com) (207.46.163.203) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Tue, 12 Aug 2014 09:23:37 +0000 Received: from BN3PR0301CA0078.namprd03.prod.outlook.com (25.160.152.174) by BL2PR03MB164.namprd03.prod.outlook.com (10.255.230.148) with Microsoft SMTP Server (TLS) id 15.0.1005.10; Tue, 12 Aug 2014 09:23:24 +0000 Received: from BY2FFO11FD012.protection.gbl (2a01:111:f400:7c0c::159) by BN3PR0301CA0078.outlook.office365.com (2a01:111:e400:401e::46) with Microsoft SMTP Server (TLS) id 15.0.1005.10 via Frontend Transport; Tue, 12 Aug 2014 09:23:25 +0000 Received: from az84smr01.freescale.net (192.88.158.2) by BY2FFO11FD012.mail.protection.outlook.com (10.1.14.130) with Microsoft SMTP Server (TLS) id 15.0.1010.11 via Frontend Transport; Tue, 12 Aug 2014 09:23:24 +0000 Received: from udp122517uds.ea.freescale.net (udp122517uds.ea.freescale.net [10.171.74.8]) by az84smr01.freescale.net (8.14.3/8.14.0) with ESMTP id s7C9NMT2001740; Tue, 12 Aug 2014 02:23:23 -0700 From: Catalin Udma <catalin.udma@freescale.com> To: <gdb-patches@sourceware.org> CC: Catalin Udma <catalin.udma@freescale.com> Subject: [PATCH] aarch64/gdbserver: fix floating point registers display Date: Tue, 12 Aug 2014 12:19:26 +0300 Message-ID: <1407835166-827-1-git-send-email-catalin.udma@freescale.com> X-EOPAttributedMessage: 0 X-Forefront-Antispam-Report: CIP:192.88.158.2; CTRY:US; IPV:CAL; IPV:NLI; EFV:NLI; SFV:NSPM; SFS:(6009001)(377424004)(189002)(199003)(102836001)(21056001)(48376002)(50466002)(87936001)(26826002)(84676001)(64706001)(76482001)(79102001)(87286001)(36756003)(4396001)(104166001)(47776003)(20776003)(85852003)(83322001)(77156001)(46102001)(89996001)(88136002)(83072002)(69596002)(97736001)(95666004)(68736004)(19580405001)(62966002)(92726001)(99396002)(92566001)(31966008)(81542001)(33646002)(74662001)(74502001)(2351001)(107046002)(50986999)(105606002)(106466001)(229853001)(104016003)(81156004)(86362001)(85306004)(77982001)(6806004)(44976005)(50226001)(19580395003)(110136001)(93916002)(81342001)(80022001); DIR:OUT; SFP:; SCL:1; SRVR:BL2PR03MB164; H:az84smr01.freescale.net; FPR:; MLV:ovrnspm; PTR:InfoDomainNonexistent; MX:1; A:1; LANG:en; MIME-Version: 1.0 Content-Type: text/plain X-Microsoft-Antispam: BCL:0;PCL:0;RULEID:;UriScan:; X-Forefront-PRVS: 0301360BF5 Received-SPF: Fail (protection.outlook.com: domain of freescale.com does not designate 192.88.158.2 as permitted sender) receiver=protection.outlook.com; client-ip=192.88.158.2; helo=az84smr01.freescale.net; Authentication-Results: spf=fail (sender IP is 192.88.158.2) smtp.mailfrom=catalin.udma@freescale.com; X-OriginatorOrg: freescale.com |
Commit Message
Catalin Udma
Aug. 12, 2014, 9:19 a.m. UTC
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(-)
Comments
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, ®set->vregs[i]); > + collect_register (regcache, AARCH64_FPSR_REGNO, ®set->fpsr); > + collect_register (regcache, AARCH64_FPCR_REGNO, ®set->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, ®set->vregs[i]); > + supply_register (regcache, AARCH64_FPSR_REGNO, ®set->fpsr); > + supply_register (regcache, AARCH64_FPCR_REGNO, ®set->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
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, ®set->vregs[i]); + collect_register (regcache, AARCH64_FPSR_REGNO, ®set->fpsr); + collect_register (regcache, AARCH64_FPCR_REGNO, ®set->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, ®set->vregs[i]); + supply_register (regcache, AARCH64_FPSR_REGNO, ®set->fpsr); + supply_register (regcache, AARCH64_FPCR_REGNO, ®set->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