[RFC/mips] Problem with fetching/setting 32 bit registers

[RFC/mips] Problem with fetching/setting 32 bit registers

[Joel Brobecker]

Hello,

I think the perfect person to ask this would have been Andrew, but
he's been away for a while now (good for him, though). Anyway, I think
others have a good knowledge of this port, so hopefully I can get some
help in solving my issue. I know what happens, and I have a patch that
gives me very results, but I think I did not fix it the proper way.

This is on mips-irix.

A customer of ours was debugging his program that was misfunctioning.
He was suspecting a float overflow. So he tried changing a flag in
the FSR register so that divisions leading to an infinity would trigger
a signal rather than generate an Inf value. That's when he discovered
that the FSR value read by GDB was incorrect, and that he was also
unable to change it.

To reproduce the problem, we have a small testcase. Sources are pasted
at the end of this message. For those who don't understand Ada, all it
does is basically do a float division by a very small number, leading
to an Inf result. What we're trying to do is check the FSR register
mask, modify it before the division is made, and verify that the
division now causes a SIGFPE signal to be raised.

This is what GDB does, using a recent version from CVS head:

        (gdb) start
        (gdb) p /x $fsr
 !!! -> $1 = 0x0
        (gdb) set $fsr := 0x01000e00
        (gdb) p /x $fsr
 !!! -> $2 = 0x0
        (gdb) c
        Continuing.
 !!! -> overflow did not crash..result:  +Inf****************
        
        Program exited normally.

For $1, the actual value is: 0x1000000. And for $2, we expect to
see the value we set (0x01000e00), not 0x0. And as a confirmation
that the value was incorrectly set we see the program running past
the division without any SIGFPE being raised.

I think this regression has been introduced when the mips port
was converted to the new register cache mechanism, where the
distinction between raw and cooked registers is made. It used
to work with 5.1.1 for instance (although not perfectly, GDB was
printing this register as a 64 bits value, which is not true).

What I found was that the problem was related to the fact that
the FSR register is 32 bits. So the cooked register type was set
to a 32 bits type, while underneath, the value provided by the
kernel is located in a 64 bits buffer. And looking close, I found
that we're reading and writing th wrong end of the buffer :-(.

Here is the relevant code using when reading a pseudo register
whose size is smaller than the size of the cooked register:

  else if (register_size (gdbarch, rawnum) >
           register_size (gdbarch, cookednum))
    {
      if (gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p
          || TARGET_BYTE_ORDER == BFD_ENDIAN_LITTLE)
        regcache_raw_read_part (regcache, rawnum, 0, 4, buf);
      else
        regcache_raw_read_part (regcache, rawnum, 4, 4, buf);

In our case (mips-irix), we have a big-endian byte order, and
therefore read the register value from the last 4 bytes, while
I found out that they are actually in the first 4.

The gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p check
gave me the idea of trying to set remote-mips64-transfers-32bit-regs,
and this gave me the expected result:

        (gdb) set remote-mips64-transfers-32bit-regs
        (gdb) start
        (gdb) info reg fsr
        fsr: 0x1000000
        (gdb) set $fsr := 0x01000e00
        (gdb) info reg fsr
        fsr: 0x1000e00
        (gdb) cont
        Continuing.

        Program received signal SIGFPE, Arithmetic exception.
        0x10004188 in hello () at hello.adb:7
        7         f := 1.0e200/value;

So this confirmed that we were reading the register value from
the wrong end.

But this is not the right fix, since this flag actually tells
GDB that all registers are 32 bit, which is wrong for mips-irix.
All general registers are 64 bits.

I think I understand the logic behind having deciding to use
a zero offset in the little-endian case while using the second
half for big-endian targets. And that suggests that mips-irix
is an exception to the usual rule.

I have tried the following change, just as an experiment:

diff -u -p -r1.5 mips-tdep.c
--- mips-tdep.c 17 Mar 2005 04:43:28 -0000      1.5
+++ mips-tdep.c 23 Mar 2005 00:30:38 -0000
@@ -606,7 +606,8 @@ mips_pseudo_register_read (struct gdbarc
           register_size (gdbarch, cookednum))
     {
       if (gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p
-         || TARGET_BYTE_ORDER == BFD_ENDIAN_LITTLE)
+         || TARGET_BYTE_ORDER == BFD_ENDIAN_LITTLE
+          || gdbarch_osabi (current_gdbarch) == GDB_OSABI_IRIX)
        regcache_raw_read_part (regcache, rawnum, 0, 4, buf);
       else
        regcache_raw_read_part (regcache, rawnum, 4, 4, buf);
@@ -628,7 +629,8 @@ mips_pseudo_register_write (struct gdbar
           register_size (gdbarch, cookednum))
     {
       if (gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p
-         || TARGET_BYTE_ORDER == BFD_ENDIAN_LITTLE)
+         || TARGET_BYTE_ORDER == BFD_ENDIAN_LITTLE
+          || gdbarch_osabi (current_gdbarch) == GDB_OSABI_IRIX)
        regcache_raw_write_part (regcache, rawnum, 0, 4, buf);
       else
        regcache_raw_write_part (regcache, rawnum, 4, 4, buf);

And it works very well. In fact, not only does it fix the problem
exposed here, but it does help a lot in the testsuite as well
(with function calls, and signal testcases).

But I am hesitating a bit. It looks a bit like a hack to be specifying
specifically IRIX here. I am afraid that there might be another way of
fixing this more cleanly. But I don't see anything. One idea I had was
to add yet another flag in the tdep part to say force the side where
to look. But that wouldn't very practical either. So I'm left with
the change above.

Any comments?

Thanks,
-- 
Joel

-- hello.adb
with text_io; use text_io;
with data; use data;
procedure hello is

begin
  f := 1.0e200/value;
  put_line ("overflow did not crash..result: " & long_float'image(f));
end;

--- data.ads
package data is
 f : long_float;
 value : long_float := 1.0e-300;
end;
---

Re: [RFC/mips] Problem with fetching/setting 32 bit registers

[Daniel Jacobowitz]

On Wed, Mar 23, 2005 at 08:14:07PM -0800, Joel Brobecker wrote:
> What I found was that the problem was related to the fact that
> the FSR register is 32 bits. So the cooked register type was set
> to a 32 bits type, while underneath, the value provided by the
> kernel is located in a 64 bits buffer. And looking close, I found
> that we're reading and writing th wrong end of the buffer :-(.
> 
> Here is the relevant code using when reading a pseudo register
> whose size is smaller than the size of the cooked register:
> 
>   else if (register_size (gdbarch, rawnum) >
>            register_size (gdbarch, cookednum))
>     {
>       if (gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p
>           || TARGET_BYTE_ORDER == BFD_ENDIAN_LITTLE)
>         regcache_raw_read_part (regcache, rawnum, 0, 4, buf);
>       else
>         regcache_raw_read_part (regcache, rawnum, 4, 4, buf);
> 
> In our case (mips-irix), we have a big-endian byte order, and
> therefore read the register value from the last 4 bytes, while
> I found out that they are actually in the first 4.

This appears to be a bug in your native support, at least relative to
the rest of the MIPS port.

> -         || TARGET_BYTE_ORDER == BFD_ENDIAN_LITTLE)
> +         || TARGET_BYTE_ORDER == BFD_ENDIAN_LITTLE
> +          || gdbarch_osabi (current_gdbarch) == GDB_OSABI_IRIX)

> And it works very well. In fact, not only does it fix the problem
> exposed here, but it does help a lot in the testsuite as well
> (with function calls, and signal testcases).
> 
> But I am hesitating a bit. It looks a bit like a hack to be specifying
> specifically IRIX here. I am afraid that there might be another way of
> fixing this more cleanly. But I don't see anything. One idea I had was
> to add yet another flag in the tdep part to say force the side where
> to look. But that wouldn't very practical either. So I'm left with
> the change above.

??? What I'm not getting from your explanation is what besides the FSR
is affected.  It looks to me like you want to take this (irix5-nat.c):

  /* FIXME, this is wrong for the N32 ABI which has 64 bit FP regs. */

  for (regi = 0; regi < 32; regi++)
    regcache_raw_supply (current_regcache, FP0_REGNUM + regi,
                         (char *) &fpregsetp->fp_r.fp_regs[regi]);

  regcache_raw_supply (current_regcache,
                       mips_regnum (current_gdbarch)->fp_control_status,
                       (char *) &fpregsetp->fp_csr);

... and fix it.

The real funny bit of the whole story appears to be (mips-tdep.c):

  else if (regnum < NUM_REGS)
    {
      /* The raw or ISA registers.  These are all sized according to
         the ISA regsize.  */
      if (mips_isa_regsize (gdbarch) == 4)
        return builtin_type_int32;
      else
        return builtin_type_int64;
    }

I don't know why the raw view of this register is 64-bit.  That's
probably affecting the mips-linux port too.  However, changing that
messes with the remote protocol, so we're probably stuck.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFC/mips] Problem with fetching/setting 32 bit registers

[Joel Brobecker]

Daniel,

> This appears to be a bug in your native support, at least relative to
> the rest of the MIPS port.

Thanks to your help, I think I actually better what happens, now. It's a
size issue: fpregsetp->fp_csr is 32bits long, while the value stored int
the raw part of the regcache is 64 bits. So when we do the memcpy inside
regcache_raw_supply(), we end up copying the FSR register value at the
first 4 bytes of the regcache buffer, and then some other junk. When
comes the time to transform this raw register into a cooked register, we
naturally look at the last 4 bytes...

I am currently testing the attached patch. Doesn't it look better?

Thanks,
-- 
Joel


Index: irix5-nat.c
===================================================================
RCS file: /gnat.dev/cvs/Dev/gdb/gdb-6.3/gdb/irix5-nat.c,v
retrieving revision 1.2
diff -u -p -r1.2 irix5-nat.c
--- irix5-nat.c	18 Nov 2004 10:49:32 -0000	1.2
+++ irix5-nat.c	25 Mar 2005 05:46:56 -0000
@@ -157,6 +157,7 @@ supply_fpregset (fpregset_t *fpregsetp)
 {
   int regi;
   static char zerobuf[32] = {0};
+  char fsrbuf[64];
 
   /* FIXME, this is wrong for the N32 ABI which has 64 bit FP regs. */
 
@@ -164,9 +165,17 @@ supply_fpregset (fpregset_t *fpregsetp)
     regcache_raw_supply (current_regcache, FP0_REGNUM + regi,
 			 (char *) &fpregsetp->fp_r.fp_regs[regi]);
 
+  /* We can't supply the FSR register directly to the regcache,
+     because there is a size issue: On one hand, fpregsetp->fp_csr
+     is 32bits long, while the regcache expects a 64bits long value.
+     So we use a buffer of the correct size and copy into it the register
+     value at the proper location.  */
+  memset (fsrbuf, 0, 4);
+  memcpy (fsrbuf + 4, &fpregsetp->fp_csr, 4);
+
   regcache_raw_supply (current_regcache,
 		       mips_regnum (current_gdbarch)->fp_control_status,
-		       (char *) &fpregsetp->fp_csr);
+		       fsrbuf);
 
   /* FIXME: how can we supply FCRIR?  SGI doesn't tell us. */
   regcache_raw_supply (current_regcache,
@@ -194,7 +203,20 @@ fill_fpregset (fpregset_t *fpregsetp, in
 
   if ((regno == -1)
       || (regno == mips_regnum (current_gdbarch)->fp_control_status))
-    fpregsetp->fp_csr = *(unsigned *) &deprecated_registers[DEPRECATED_REGISTER_BYTE (mips_regnum (current_gdbarch)->fp_control_status)];
+    {
+      char fsrbuf[64];
+
+    /* We can't fill the FSR register directly from the regcache,
+       because there is a size issue: On one hand, fpregsetp->fp_csr
+       is 32bits long, while the regcache expects a 64bits long buffer.
+       So we use a buffer of the correct size and copy the register
+       value from that buffer.  */
+      regcache_raw_read (current_regcache,
+                         mips_regnum (current_gdbarch)->fp_control_status,
+                         fsrbuf);
+
+      memcpy (&fpregsetp->fp_csr, fsrbuf + 4, 4);
+    }
 }
 
 

Re: [RFC/mips] Problem with fetching/setting 32 bit registers

[Mark Kettenis]

   Date: Thu, 24 Mar 2005 21:49:17 -0800
   From: Joel Brobecker <brobecker@adacore.com>

   Daniel,

   > This appears to be a bug in your native support, at least relative to
   > the rest of the MIPS port.

   Thanks to your help, I think I actually better what happens, now. It's a
   size issue: fpregsetp->fp_csr is 32bits long, while the value stored int
   the raw part of the regcache is 64 bits. So when we do the memcpy inside
   regcache_raw_supply(), we end up copying the FSR register value at the
   first 4 bytes of the regcache buffer, and then some other junk. When
   comes the time to transform this raw register into a cooked register, we
   naturally look at the last 4 bytes...

   I am currently testing the attached patch. Doesn't it look better?

Looks better but:

   Index: irix5-nat.c
   ===================================================================
   RCS file: /gnat.dev/cvs/Dev/gdb/gdb-6.3/gdb/irix5-nat.c,v
   retrieving revision 1.2
   diff -u -p -r1.2 irix5-nat.c
   --- irix5-nat.c	18 Nov 2004 10:49:32 -0000	1.2
   +++ irix5-nat.c	25 Mar 2005 05:46:56 -0000
   @@ -157,6 +157,7 @@ supply_fpregset (fpregset_t *fpregsetp)
    {
      int regi;
      static char zerobuf[32] = {0};
   +  char fsrbuf[64];

Looks like you're confused about bits and bytes.  Guess 8 is large
enough here ;-).

      /* FIXME, this is wrong for the N32 ABI which has 64 bit FP regs. */

Could you remove the redundant parenthesis here while you're at it:

   @@ -194,7 +203,20 @@ fill_fpregset (fpregset_t *fpregsetp, in

      if ((regno == -1)
	  || (regno == mips_regnum (current_gdbarch)->fp_control_status))
   -    fpregsetp->fp_csr = *(unsigned *) &deprecated_registers[DEPRECATED_REGISTER_BYTE (mips_regnum (current_gdbarch)->fp_control_status)];
   +    {
   +      char fsrbuf[64];
   +
   +    /* We can't fill the FSR register directly from the regcache,
   +       because there is a size issue: On one hand, fpregsetp->fp_csr
   +       is 32bits long, while the regcache expects a 64bits long buffer.
   +       So we use a buffer of the correct size and copy the register
   +       value from that buffer.  */
   +      regcache_raw_read (current_regcache,
   +                         mips_regnum (current_gdbarch)->fp_control_status,
   +                         fsrbuf);
   +
   +      memcpy (&fpregsetp->fp_csr, fsrbuf + 4, 4);
   +    }
    }

Re: [RFC/mips] Problem with fetching/setting 32 bit registers

[Joel Brobecker]

Mark,

> Looks better but:

Thanks for all your comments. I have tested the patch with your
comments included. I'll send an RFA shortly.

-- 
Joel