Message ID | 1441284969-30465-1-git-send-email-yao.qi@linaro.org |
---|---|
State | New, archived |
Headers |
Received: (qmail 35981 invoked by alias); 3 Sep 2015 12:56:30 -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 35958 invoked by uid 89); 3 Sep 2015 12:56:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-pa0-f41.google.com Received: from mail-pa0-f41.google.com (HELO mail-pa0-f41.google.com) (209.85.220.41) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Thu, 03 Sep 2015 12:56:27 +0000 Received: by pacex6 with SMTP id ex6so41437230pac.0 for <gdb-patches@sourceware.org>; Thu, 03 Sep 2015 05:56:25 -0700 (PDT) X-Received: by 10.66.101.39 with SMTP id fd7mr63854172pab.3.1441284985231; Thu, 03 Sep 2015 05:56:25 -0700 (PDT) Received: from E107787-LIN.cambridge.arm.com (gcc1-power7.osuosl.org. [140.211.15.137]) by smtp.gmail.com with ESMTPSA id qc2sm25217710pbc.79.2015.09.03.05.56.23 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 03 Sep 2015 05:56:24 -0700 (PDT) From: Yao Qi <qiyaoltc@gmail.com> X-Google-Original-From: Yao Qi <yao.qi@linaro.org> To: gdb-patches@sourceware.org Cc: pinskia@gmail.com, catalin.udma@freescale.com Subject: [PATCH] aarch64-core.xml: 32-bit cpsr -> 64-bit pstate Date: Thu, 3 Sep 2015 13:56:09 +0100 Message-Id: <1441284969-30465-1-git-send-email-yao.qi@linaro.org> X-IsSubscribed: yes |
Commit Message
Yao Qi
Sept. 3, 2015, 12:56 p.m. UTC
This patch is to change 32-bit cpsr to 64-bit pstate in aarch64 target description. When aarch64 GDB port was added into GDB, cpsr is 32-bit in aarch64-core.xml. Andrew Pinski changed it to 64-bit https://sourceware.org/ml/gdb-patches/2013-12/msg00720.html but forget updating GDBserver side. As a result, GDB thinks cpsr is 64-bit, but GDBserver still thinks it is 32-bit, so contents of registers after cpster, such as floating point registers, are wrong. Catalin Udma gave a patch https://sourceware.org/ml/gdb-patches/2014-08/msg00193.html to change cpsr in GDBserver side to 64-bit. After some discussions, we decided to revert Andrew's patch, and let cpsr be 32-bit. A problem "Remote 'g' packet reply is too long:" (when I connect gdb to kgdb with aarch64 kernel) leads me to the 32-bit vs 64-bit issue again. cpsr is 32-bit, but it only exists aarch32 execution state. As in ARMv8 reference manual, "A1.3.1 Execution state" "The A32 and T32 instruction sets include instructions that operate directly on various PSTATE elements, and instructions that access PSTATE by using the Application Program Status Register (APSR) or the Current Program Status Register (CPSR)". In other words, there is no cpsr at all on aarch64 execution state. On aarch64 execution state, pstate is an abstraction of process state information, so it has no architecturally defined size or enconding. However, the architecture does define how the PSTATE is encoded in the SPSR when taking an exception, and that is the form of PSTATE in ptrace. Currently only bits 31:0 of SPSR are defined, but it is only accessible via MSR/MRS instructions, which always transfer 64-bits, so on ISA level, it should be regarded as 64-bit. This change will break the compatibility for <new gdb, old gdbserver> and <old gdb, new gdbserver>, but current GDB is wrong, we have to fix it. gdb: 2015-09-03 Yao Qi <yao.qi@linaro.org> * aarch64-tdep.c (aarch64_r_register_names): Rename cpsr to pstate. * features/aarch64-core.xmli (cpsr): Rename it to pstate, and change its size to 64-bit. * features/aarch64.c: Regenerate. * regformats/aarch64.dat: Likewise. --- gdb/aarch64-tdep.c | 2 +- gdb/features/aarch64-core.xml | 2 +- gdb/features/aarch64.c | 2 +- gdb/regformats/aarch64.dat | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-)
Comments
On 03/09/15 13:56, Yao Qi wrote: > This patch is to change 32-bit cpsr to 64-bit pstate in aarch64 target > description. > > When aarch64 GDB port was added into GDB, cpsr is 32-bit in > aarch64-core.xml. Andrew Pinski changed it to 64-bit > https://sourceware.org/ml/gdb-patches/2013-12/msg00720.html but forget > updating GDBserver side. As a result, GDB thinks cpsr is 64-bit, but > GDBserver still thinks it is 32-bit, so contents of registers after > cpster, such as floating point registers, are wrong. Catalin Udma > gave a patch https://sourceware.org/ml/gdb-patches/2014-08/msg00193.html > to change cpsr in GDBserver side to 64-bit. After some discussions, > we decided to revert Andrew's patch, and let cpsr be 32-bit. > > A problem "Remote 'g' packet reply is too long:" (when I connect gdb > to kgdb with aarch64 kernel) leads me to the 32-bit vs 64-bit issue > again. > > cpsr is 32-bit, but it only exists aarch32 execution state. As in > ARMv8 reference manual, "A1.3.1 Execution state" > > "The A32 and T32 > instruction sets include instructions that operate directly on various PSTATE elements, and > instructions that access PSTATE by using the Application Program Status Register (APSR) > or the Current Program Status Register (CPSR)". > > In other words, there is no cpsr at all on aarch64 execution state. > On aarch64 execution state, pstate is an abstraction of process state > information, so it has no architecturally defined size or enconding. > > However, the architecture does define how the PSTATE is encoded in the > SPSR when taking an exception, and that is the form of PSTATE in > ptrace. Currently only bits 31:0 of SPSR are defined, but it is only > accessible via MSR/MRS instructions, which always transfer 64-bits, > so on ISA level, it should be regarded as 64-bit. > > This change will break the compatibility for <new gdb, old gdbserver> > and <old gdb, new gdbserver>, but current GDB is wrong, we have to fix it. > Won't this also break any situation where the gdbserver knowledge is embedded in a HW stub? You're assuming that we can just update all those servers and everything will be fine and dandy. I don't think we can't just change this arbitrarily. I think kgdb should do what everyone else has been doing and simply return the bottom 32 bits. R. > gdb: > > 2015-09-03 Yao Qi <yao.qi@linaro.org> > > * aarch64-tdep.c (aarch64_r_register_names): Rename cpsr to > pstate. > * features/aarch64-core.xmli (cpsr): Rename it to pstate, and > change its size to 64-bit. > * features/aarch64.c: Regenerate. > * regformats/aarch64.dat: Likewise. > --- > gdb/aarch64-tdep.c | 2 +- > gdb/features/aarch64-core.xml | 2 +- > gdb/features/aarch64.c | 2 +- > gdb/regformats/aarch64.dat | 2 +- > 4 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c > index 5d8ffd4..04064cb 100644 > --- a/gdb/aarch64-tdep.c > +++ b/gdb/aarch64-tdep.c > @@ -127,7 +127,7 @@ static const char *const aarch64_r_register_names[] = > "x20", "x21", "x22", "x23", > "x24", "x25", "x26", "x27", > "x28", "x29", "x30", "sp", > - "pc", "cpsr" > + "pc", "pstate" > }; > > /* The FP/SIMD 'V' registers. */ > diff --git a/gdb/features/aarch64-core.xml b/gdb/features/aarch64-core.xml > index c95049c..f0f455f 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="32"/> > + <reg name="pstate" bitsize="64"/> > </feature> > diff --git a/gdb/features/aarch64.c b/gdb/features/aarch64.c > index 1e9a99d..643387d 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, 32, "int"); > + tdesc_create_reg (feature, "pstate", 33, 1, NULL, 64, "int"); > > feature = tdesc_create_feature (result, "org.gnu.gdb.aarch64.fpu"); > field_type = tdesc_named_type (feature, "ieee_double"); > diff --git a/gdb/regformats/aarch64.dat b/gdb/regformats/aarch64.dat > index d4cea04..0db2f3a 100644 > --- a/gdb/regformats/aarch64.dat > +++ b/gdb/regformats/aarch64.dat > @@ -36,7 +36,7 @@ expedite:x29,sp,pc > 64:x30 > 64:sp > 64:pc > -32:cpsr > +64:pstate > 128:v0 > 128:v1 > 128:v2 >
Richard Earnshaw <Richard.Earnshaw@foss.arm.com> writes: > Won't this also break any situation where the gdbserver knowledge is > embedded in a HW stub? You're assuming that we can just update all > those servers and everything will be fine and dandy. I don't think we > can't just change this arbitrarily. Yes, it will break any existing debugging stubs, including old gdbserver and jtag probes, which think the register in the target description (cpsr, psate or whatever) is 32-bit. However, I don't assume "we can just update all those servers and everything will be fine and dandy". As I said in the first mail, this change will break the compatibility. AArch64 is still a new architecture, and there isn't much compatibility burden as other old archs have. Under this context, I think correctness is more important than compatibility. If we don't fix it now, we can't fix it forever. So it isn't an arbitrary change. We did change the size of the register in the target description to 64-bit back and forth in 2013 and 2014, but I don't hear any complaints on this from HW jtag probes providers. > > I think kgdb should do what everyone else has been doing and simply > return the bottom 32 bits. I think kgdb is correct. I don't know what "everyone else" is. As far as I can see, openocd doesn't support aarch64 now, but there are some patches to add aarch64 support http://openocd.zylin.com/#/c/2501/14 the register (named CPSR in the openocd patches) is 64-bit, which looks right to me.
On 09/08/2015 09:20 AM, Yao Qi wrote: > > AArch64 is still a new architecture, and there isn't much compatibility > burden as other old archs have. Under the same reasoning, you should be able to fix kgdb as well. > Under this context, I think correctness > is more important than compatibility. If we don't fix it now, we can't fix > it forever. So it isn't an arbitrary change. Simply cpsr->pstate like in your patch has user-visible changes as well. Do users really expect to see "pstate" in "info registers"? If it really OK that "print $cpsr" stops working? But the change is really not OK as is. You can't have a server _not_ send "cpsr" and claim that the target description supports the "org.gnu.gdb.aarch64.core" feature: @node AArch64 Features @subsection AArch64 Features @cindex target descriptions, AArch64 features The @samp{org.gnu.gdb.aarch64.core} feature is required for AArch64 targets. It should contain registers @samp{x0} through @samp{x30}, @samp{sp}, @samp{pc}, and @samp{cpsr}. If we do go the cpsr->pstate direction, we'll have to add a new target feature. Thanks, Pedro Alves
Pedro Alves <palves@redhat.com> writes: > But the change is really not OK as is. You can't have a server _not_ > send "cpsr" and claim that the target description supports > the "org.gnu.gdb.aarch64.core" feature: > > @node AArch64 Features > @subsection AArch64 Features > @cindex target descriptions, AArch64 features > > The @samp{org.gnu.gdb.aarch64.core} feature is required for AArch64 > targets. It should contain registers @samp{x0} through @samp{x30}, > @samp{sp}, @samp{pc}, and @samp{cpsr}. > > If we do go the cpsr->pstate direction, we'll have to add a new > target feature. Yes, I realize that we need to add a new feature, like "org.gnu.gdb.aarch64.core_pstate". I withdraw this patch.
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c index 5d8ffd4..04064cb 100644 --- a/gdb/aarch64-tdep.c +++ b/gdb/aarch64-tdep.c @@ -127,7 +127,7 @@ static const char *const aarch64_r_register_names[] = "x20", "x21", "x22", "x23", "x24", "x25", "x26", "x27", "x28", "x29", "x30", "sp", - "pc", "cpsr" + "pc", "pstate" }; /* The FP/SIMD 'V' registers. */ diff --git a/gdb/features/aarch64-core.xml b/gdb/features/aarch64-core.xml index c95049c..f0f455f 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="32"/> + <reg name="pstate" bitsize="64"/> </feature> diff --git a/gdb/features/aarch64.c b/gdb/features/aarch64.c index 1e9a99d..643387d 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, 32, "int"); + tdesc_create_reg (feature, "pstate", 33, 1, NULL, 64, "int"); feature = tdesc_create_feature (result, "org.gnu.gdb.aarch64.fpu"); field_type = tdesc_named_type (feature, "ieee_double"); diff --git a/gdb/regformats/aarch64.dat b/gdb/regformats/aarch64.dat index d4cea04..0db2f3a 100644 --- a/gdb/regformats/aarch64.dat +++ b/gdb/regformats/aarch64.dat @@ -36,7 +36,7 @@ expedite:x29,sp,pc 64:x30 64:sp 64:pc -32:cpsr +64:pstate 128:v0 128:v1 128:v2