RISC-V: Fix xlen to flen typo in FP reg handling.

Message ID 20181103032033.21653-1-jimw@sifive.com
State New, archived
Headers

Commit Message

Jim Wilson Nov. 3, 2018, 3:20 a.m. UTC
  This fixes a bug in FP register handling for targets where xlen != flen.
Tested against riscv-test/debug where it fixes a few failures.  Also tested
on RV64GC linux with the gdb testsuite where it has no effect.

	gdb/
	* riscv-tdep.c (riscv_register_type): Use riscv_isa_flen for FP regs
	not riscv_isa_xlen.
---
 gdb/riscv-tdep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

Comments

Andrew Burgess Nov. 3, 2018, 8:34 a.m. UTC | #1
* Jim Wilson <jimw@sifive.com> [2018-11-02 20:20:33 -0700]:

> This fixes a bug in FP register handling for targets where xlen != flen.
> Tested against riscv-test/debug where it fixes a few failures.  Also tested
> on RV64GC linux with the gdb testsuite where it has no effect.
> 
> 	gdb/
> 	* riscv-tdep.c (riscv_register_type): Use riscv_isa_flen for FP regs
> 	not riscv_isa_xlen.

Can you hold off merging this patch please.  There's a non-obvious
reason why this uses xlen here (even though it's the wrong thing to
do).

The reason is something to do with remote targets, when the remote
target reads a register then it initialises a cache that contains the
types of all registers (for the sizes I think), for FP regs right now,
that results in a call to read MISA.  This read requires the remote to
initialise a cache of all the register types, which results in a read
of MISA.  Which requires the remote to initialise a cache .....

So we get stuck in infinite recursion :-(

I'm pretty sure the above is basically correct, but I'd have to do
some additional tests to get an exact explanation for you.

However, don't worry.  I'm currently working on proper target
description support.  I had hoped to get it posted this week, but it
now looks like it will be early next week.  I just need final testing
and a bit of cleanup / ChangeLogs writing etc.  The target description
patch removes the MISA reading stuff completely right now, and fixes
this xreg instead of freg hack.

So could I ask, please don't push this until say the end of next
week.  If I haven't posted the target description patch by then, then
go ahead and I'll accept that my remote tests will break.

Thanks,
Andrew




> ---
>  gdb/riscv-tdep.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index db372e2163..b94802aa97 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -630,7 +630,7 @@ riscv_register_type (struct gdbarch *gdbarch, int regnum)
>      }
>    else if (regnum <= RISCV_LAST_FP_REGNUM)
>      {
> -      regsize = riscv_isa_xlen (gdbarch);
> +      regsize = riscv_isa_flen (gdbarch);
>        switch (regsize)
>  	{
>  	case 4:
> -- 
> 2.17.1
>
  
Jim Wilson Nov. 3, 2018, 8:13 p.m. UTC | #2
On Sat, Nov 3, 2018 at 1:34 AM Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
> However, don't worry.  I'm currently working on proper target
> description support.

Do you perhaps mean XML register set support?  I started working on a
patch for that.  This is needed for our OpenOCD and QEMU support.

Meanwhile, I can hold off on this patch.

Jim
  

Patch

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index db372e2163..b94802aa97 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -630,7 +630,7 @@  riscv_register_type (struct gdbarch *gdbarch, int regnum)
     }
   else if (regnum <= RISCV_LAST_FP_REGNUM)
     {
-      regsize = riscv_isa_xlen (gdbarch);
+      regsize = riscv_isa_flen (gdbarch);
       switch (regsize)
 	{
 	case 4: