diff mbox

gdb/riscv: Add target description support

Message ID 20190223203958.GA15942@embecosm.com
State New
Headers show

Commit Message

Andrew Burgess Feb. 23, 2019, 8:39 p.m. UTC
* Tom Tromey <tom@tromey.com> [2019-02-22 10:42:50 -0700]:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
> 
> Andrew> This is a slightly revised version of the RISC-V target descriptions
> Andrew> patch.
> 
> I'm seeing a difference that I think was introduced by this patch and I
> am wondering whether it is intentional and whether something ought to be
> done about it.  I'm really not sure, this is my first foray into RISC-V
> and into target descriptions.
> 
> With an older gdb (8.2), with remote debugging enabled:
> 
>     (gdb) p $fflags
>     Sending packet: $p42#d6...Ack
>     Packet received: 0000000000000000
>     $1 = 0
> 
> Here you can see that gdb thinks the register number for fflags is 0x42.
> And, that is the value of RISCV_CSR_FFLAGS_REGNUM, even in today's gdb
> master:
> 
>     (top-gdb) p/x RISCV_CSR_FFLAGS_REGNUM
>     $1 = 0x42
> 
> However with a newer gdb, with an older qemu, I get a failure:
> 
>     Sending packet: $p41#d5...Ack
>     Packet received: E14
>     Could not fetch register "fflags"; remote failure reply 'E14'
> 
> Here you can see gdb is sending 0x41.
> 
> RISCV_CSR_FFLAGS_REGNUM is computed by:
> 
>     RISCV_LAST_FP_REGNUM = 64,	/* Last Floating Point Register */
>     RISCV_FIRST_CSR_REGNUM = 65,  /* First CSR */
>     [...]
>     #define DECLARE_CSR(name, num) \
>       RISCV_ ## num ## _REGNUM = RISCV_FIRST_CSR_REGNUM + num,
> 
> Then from riscv-opc.h:
> 
>     #define CSR_FFLAGS 0x1
>     [...]
>     DECLARE_CSR(fflags, CSR_FFLAGS)
> 
> So, in effect it is RISCV_LAST_FP_REGNUM + 2.
> 
> 
> But then, e.g., in the 32-bit FPU description:
> 
> Andrew> +static int
> Andrew> +create_feature_riscv_32bit_fpu (struct target_desc *result, long regnum)
> Andrew> +{
> [...]
> Andrew> +  tdesc_create_reg (feature, "ft11", regnum++, 1, NULL, 32, "ieee_single");
> Andrew> +  tdesc_create_reg (feature, "fflags", regnum++, 1, NULL, 32, "int");
> 
> I think this is where the discrepancy lies.
> 
> 
> I'm not really sure what ought to be done here.  Do you have any ideas?

So you are correct.  When I added the target description support I
failed to consider backward compatibility with existing tools enough.

So, when GDB connect to a target that doesn't provide a target
description a default description is used.  This default has features
created by calls to the functions like create_feature_riscv_32bit_fpu
and friends, and it is these functions that are picking the default
register numbering.

In an ideal world, with no legacy baggage, these create_feature_*
functions shouldn't really care about register numbering, which is how
I have them setup right now.  However, it doesn't have to be that way,
we can force the numbering in the create_feature_* functions.

The patch below tries to restore the legacy numbering for fcsr, frm,
and fflags.  Does this restore the behaviour you are expecting?

One other thing to note about the change to target descriptions was
that the default target descriptions don't include any CSRs.  At the
time I figured that guessing about what CSRs a target supports wasn't
a good thing to do, and instead we should be encouraging targets to
move to proper target description support in order to support CSRs.
The consequence is that when you connect to your legacy QEMU the only
CSRs you'll have access too are the fcsr, frm, and fflags (as these
get grouped in with FP registers as they should be available if the FP
registers are available).

One possibility as an alternative to this patch is writing a target
description file that describes you legacy QEMU and loading this into
GDB with 'set tdesc filename FILENAME', with this approach you could
force the register numbering to match your expectations, and you'd get
access to your CSRs back.

Let me know if you think the patch below is an improvement, though
it's possibly not as pure (with hard-coded register numbering) than
before, I'd be happy to have this merged if the compatibility was
important.

Thanks,
Andrew

--

[PATCH] gdb/riscv: Use legacy register numbers in default target description

When the target description support was added to RISC-V, the register
numbers assigned to the fflags, frm, and fcsr control registers in the
default target descriptions didn't match the register numbers used by
GDB before the target description support was added.

What this means is that if a tools exists in the wild that is using
hard-coded register number, setup to match GDB's old behaviour, then
this will have been broken (for fflags, frm, and fcsr) by the move to
target descriptions.  QEMU is such a tool.

There are a couple of solutions that could be used to work around this
issue:

 - The user can create their own xml description file with the
   register numbers setup to match their old tool, then load this by
   telling GDB 'set tdesc filename FILENAME'.

 - Update their old tool to use the newer default numbering scheme, or
   better yet add proper target description support to their tool.

 - We could have RISC-V GDB change to maintain the old defaults.

This patch implements the last of these ideas, changing the default
numbering to match the old behaviour.

This change is only visible to targets that don't supply their own xml
description file and instead rely on GDB's default numbering.

gdb/ChangeLog:

	* features/riscv/32bit-cpu.xml: Add register numbers.
	* features/riscv/32bit-fpu.c: Regenerate.
	* features/riscv/32bit-fpu.xml: Add register numbers.
	* features/riscv/64bit-cpu.xml: Add register numbers.
	* features/riscv/64bit-fpu.c: Regenerate.
	* features/riscv/64bit-fpu.xml: Add register numbers.
---
 gdb/ChangeLog                    |  9 +++++++++
 gdb/features/riscv/32bit-cpu.xml |  6 +++++-
 gdb/features/riscv/32bit-fpu.c   |  2 ++
 gdb/features/riscv/32bit-fpu.xml | 12 ++++++++----
 gdb/features/riscv/64bit-cpu.xml |  6 +++++-
 gdb/features/riscv/64bit-fpu.c   |  2 ++
 gdb/features/riscv/64bit-fpu.xml | 12 ++++++++----
 7 files changed, 39 insertions(+), 10 deletions(-)

Comments

Joel Brobecker Feb. 26, 2019, 11:55 a.m. UTC | #1
Hi Andrew,

> [PATCH] gdb/riscv: Use legacy register numbers in default target description
> 
> When the target description support was added to RISC-V, the register
> numbers assigned to the fflags, frm, and fcsr control registers in the
> default target descriptions didn't match the register numbers used by
> GDB before the target description support was added.
> 
> What this means is that if a tools exists in the wild that is using
> hard-coded register number, setup to match GDB's old behaviour, then
> this will have been broken (for fflags, frm, and fcsr) by the move to
> target descriptions.  QEMU is such a tool.
> 
> There are a couple of solutions that could be used to work around this
> issue:
> 
>  - The user can create their own xml description file with the
>    register numbers setup to match their old tool, then load this by
>    telling GDB 'set tdesc filename FILENAME'.
> 
>  - Update their old tool to use the newer default numbering scheme, or
>    better yet add proper target description support to their tool.
> 
>  - We could have RISC-V GDB change to maintain the old defaults.
> 
> This patch implements the last of these ideas, changing the default
> numbering to match the old behaviour.
> 
> This change is only visible to targets that don't supply their own xml
> description file and instead rely on GDB's default numbering.
> 
> gdb/ChangeLog:
> 
> 	* features/riscv/32bit-cpu.xml: Add register numbers.
> 	* features/riscv/32bit-fpu.c: Regenerate.
> 	* features/riscv/32bit-fpu.xml: Add register numbers.
> 	* features/riscv/64bit-cpu.xml: Add register numbers.
> 	* features/riscv/64bit-fpu.c: Regenerate.
> 	* features/riscv/64bit-fpu.xml: Add register numbers.

I've had a chance to look at the patch, and fwiw, it looks good to me.
If others agree that it is OK, I think it would be nice if we pushed
the patch before I create the gdb-8.3-branch, and then create the first
pre-release (8.2.90).
Tom Tromey March 4, 2019, 4:18 p.m. UTC | #2
>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Tom> I'm not really sure what ought to be done here.  Do you have any
Tom> ideas?

Andrew> So you are correct.  When I added the target description support I
Andrew> failed to consider backward compatibility with existing tools enough.
[...]

I just wanted to say thanks for doing this.
I can confirm it helped.

Tom
diff mbox

Patch

diff --git a/gdb/features/riscv/32bit-cpu.xml b/gdb/features/riscv/32bit-cpu.xml
index 466f2c0648..0d07aaec85 100644
--- a/gdb/features/riscv/32bit-cpu.xml
+++ b/gdb/features/riscv/32bit-cpu.xml
@@ -5,9 +5,13 @@ 
      are permitted in any medium without royalty provided the copyright
      notice and this notice are preserved.  -->
 
+<!-- Register numbers are hard-coded in order to maintain backward
+     compatibility with older versions of tools that didn't use xml
+     register descriptions.  -->
+
 <!DOCTYPE feature SYSTEM "gdb-target.dtd">
 <feature name="org.gnu.gdb.riscv.cpu">
-  <reg name="zero" bitsize="32" type="int"/>
+  <reg name="zero" bitsize="32" type="int" regnum="0"/>
   <reg name="ra" bitsize="32" type="code_ptr"/>
   <reg name="sp" bitsize="32" type="data_ptr"/>
   <reg name="gp" bitsize="32" type="data_ptr"/>
diff --git a/gdb/features/riscv/32bit-fpu.c b/gdb/features/riscv/32bit-fpu.c
index 22e80d640e..a19780aab0 100644
--- a/gdb/features/riscv/32bit-fpu.c
+++ b/gdb/features/riscv/32bit-fpu.c
@@ -9,6 +9,7 @@  create_feature_riscv_32bit_fpu (struct target_desc *result, long regnum)
   struct tdesc_feature *feature;
 
   feature = tdesc_create_feature (result, "org.gnu.gdb.riscv.fpu");
+  regnum = 33;
   tdesc_create_reg (feature, "ft0", regnum++, 1, NULL, 32, "ieee_single");
   tdesc_create_reg (feature, "ft1", regnum++, 1, NULL, 32, "ieee_single");
   tdesc_create_reg (feature, "ft2", regnum++, 1, NULL, 32, "ieee_single");
@@ -41,6 +42,7 @@  create_feature_riscv_32bit_fpu (struct target_desc *result, long regnum)
   tdesc_create_reg (feature, "ft9", regnum++, 1, NULL, 32, "ieee_single");
   tdesc_create_reg (feature, "ft10", regnum++, 1, NULL, 32, "ieee_single");
   tdesc_create_reg (feature, "ft11", regnum++, 1, NULL, 32, "ieee_single");
+  regnum = 66;
   tdesc_create_reg (feature, "fflags", regnum++, 1, NULL, 32, "int");
   tdesc_create_reg (feature, "frm", regnum++, 1, NULL, 32, "int");
   tdesc_create_reg (feature, "fcsr", regnum++, 1, NULL, 32, "int");
diff --git a/gdb/features/riscv/32bit-fpu.xml b/gdb/features/riscv/32bit-fpu.xml
index 6a44b842b8..1eaae9119e 100644
--- a/gdb/features/riscv/32bit-fpu.xml
+++ b/gdb/features/riscv/32bit-fpu.xml
@@ -5,9 +5,13 @@ 
      are permitted in any medium without royalty provided the copyright
      notice and this notice are preserved.  -->
 
+<!-- Register numbers are hard-coded in order to maintain backward
+     compatibility with older versions of tools that didn't use xml
+     register descriptions.  -->
+
 <!DOCTYPE feature SYSTEM "gdb-target.dtd">
 <feature name="org.gnu.gdb.riscv.fpu">
-  <reg name="ft0" bitsize="32" type="ieee_single"/>
+  <reg name="ft0" bitsize="32" type="ieee_single" regnum="33"/>
   <reg name="ft1" bitsize="32" type="ieee_single"/>
   <reg name="ft2" bitsize="32" type="ieee_single"/>
   <reg name="ft3" bitsize="32" type="ieee_single"/>
@@ -40,7 +44,7 @@ 
   <reg name="ft10" bitsize="32" type="ieee_single"/>
   <reg name="ft11" bitsize="32" type="ieee_single"/>
 
-  <reg name="fflags" bitsize="32" type="int"/>
-  <reg name="frm" bitsize="32" type="int"/>
-  <reg name="fcsr" bitsize="32" type="int"/>
+  <reg name="fflags" bitsize="32" type="int" regnum="66"/>
+  <reg name="frm" bitsize="32" type="int" regnum="67"/>
+  <reg name="fcsr" bitsize="32" type="int" regnum="68"/>
 </feature>
diff --git a/gdb/features/riscv/64bit-cpu.xml b/gdb/features/riscv/64bit-cpu.xml
index c4d83de09b..b8aa424ae4 100644
--- a/gdb/features/riscv/64bit-cpu.xml
+++ b/gdb/features/riscv/64bit-cpu.xml
@@ -5,9 +5,13 @@ 
      are permitted in any medium without royalty provided the copyright
      notice and this notice are preserved.  -->
 
+<!-- Register numbers are hard-coded in order to maintain backward
+     compatibility with older versions of tools that didn't use xml
+     register descriptions.  -->
+
 <!DOCTYPE feature SYSTEM "gdb-target.dtd">
 <feature name="org.gnu.gdb.riscv.cpu">
-  <reg name="zero" bitsize="64" type="int"/>
+  <reg name="zero" bitsize="64" type="int" regnum="0"/>
   <reg name="ra" bitsize="64" type="code_ptr"/>
   <reg name="sp" bitsize="64" type="data_ptr"/>
   <reg name="gp" bitsize="64" type="data_ptr"/>
diff --git a/gdb/features/riscv/64bit-fpu.c b/gdb/features/riscv/64bit-fpu.c
index 8cbd7484ab..b93cb4ec03 100644
--- a/gdb/features/riscv/64bit-fpu.c
+++ b/gdb/features/riscv/64bit-fpu.c
@@ -17,6 +17,7 @@  create_feature_riscv_64bit_fpu (struct target_desc *result, long regnum)
   field_type = tdesc_named_type (feature, "ieee_double");
   tdesc_add_field (type_with_fields, "double", field_type);
 
+  regnum = 33;
   tdesc_create_reg (feature, "ft0", regnum++, 1, NULL, 64, "riscv_double");
   tdesc_create_reg (feature, "ft1", regnum++, 1, NULL, 64, "riscv_double");
   tdesc_create_reg (feature, "ft2", regnum++, 1, NULL, 64, "riscv_double");
@@ -49,6 +50,7 @@  create_feature_riscv_64bit_fpu (struct target_desc *result, long regnum)
   tdesc_create_reg (feature, "ft9", regnum++, 1, NULL, 64, "riscv_double");
   tdesc_create_reg (feature, "ft10", regnum++, 1, NULL, 64, "riscv_double");
   tdesc_create_reg (feature, "ft11", regnum++, 1, NULL, 64, "riscv_double");
+  regnum = 66;
   tdesc_create_reg (feature, "fflags", regnum++, 1, NULL, 32, "int");
   tdesc_create_reg (feature, "frm", regnum++, 1, NULL, 32, "int");
   tdesc_create_reg (feature, "fcsr", regnum++, 1, NULL, 32, "int");
diff --git a/gdb/features/riscv/64bit-fpu.xml b/gdb/features/riscv/64bit-fpu.xml
index fd14ebced8..794854cc01 100644
--- a/gdb/features/riscv/64bit-fpu.xml
+++ b/gdb/features/riscv/64bit-fpu.xml
@@ -5,6 +5,10 @@ 
      are permitted in any medium without royalty provided the copyright
      notice and this notice are preserved.  -->
 
+<!-- Register numbers are hard-coded in order to maintain backward
+     compatibility with older versions of tools that didn't use xml
+     register descriptions.  -->
+
 <!DOCTYPE feature SYSTEM "gdb-target.dtd">
 <feature name="org.gnu.gdb.riscv.fpu">
 
@@ -13,7 +17,7 @@ 
     <field name="double" type="ieee_double"/>
   </union>
 
-  <reg name="ft0" bitsize="64" type="riscv_double"/>
+  <reg name="ft0" bitsize="64" type="riscv_double" regnum="33"/>
   <reg name="ft1" bitsize="64" type="riscv_double"/>
   <reg name="ft2" bitsize="64" type="riscv_double"/>
   <reg name="ft3" bitsize="64" type="riscv_double"/>
@@ -46,7 +50,7 @@ 
   <reg name="ft10" bitsize="64" type="riscv_double"/>
   <reg name="ft11" bitsize="64" type="riscv_double"/>
 
-  <reg name="fflags" bitsize="32" type="int"/>
-  <reg name="frm" bitsize="32" type="int"/>
-  <reg name="fcsr" bitsize="32" type="int"/>
+  <reg name="fflags" bitsize="32" type="int" regnum="66"/>
+  <reg name="frm" bitsize="32" type="int" regnum="67"/>
+  <reg name="fcsr" bitsize="32" type="int" regnum="68"/>
 </feature>