Re: [RFA] mips: Fix "info registers" output

Re: [RFA] mips: Fix "info registers" output

[Andrew Cagney]

Just FYI,  this one needs an assignment.  Once the assignment is on 
place can you repost?

	Andrew

Re: [RFA] mips: Fix "info registers" output

[Eli Zaretskii]

On Tue, 19 Jun 2001, Daniel Jacobowitz wrote:

> There were some pretty nasty problems in the FP register printing code, and
> a potential formatting problem in the GP register printing code.

What problems?  Could you please elaborate and maybe even post a short
session script to demonstrate the problems?

It is very hard to comment on such changes without knowing what are
you trying to fix.

Re: [RFA] mips: Fix "info registers" output

[Daniel Jacobowitz]

On Thu, Jun 21, 2001 at 10:47:06AM +0300, Eli Zaretskii wrote:
> 
> On Tue, 19 Jun 2001, Daniel Jacobowitz wrote:
> 
> > There were some pretty nasty problems in the FP register printing code, and
> > a potential formatting problem in the GP register printing code.
> 
> What problems?  Could you please elaborate and maybe even post a short
> session script to demonstrate the problems?
> 
> It is very hard to comment on such changes without knowing what are
> you trying to fix.

Sorry.  To be more precise:

 - the decoding of double-precision arguments was completely broken. 
In addition to copying unallocated memory, and getting the byte order
wrong, we tried to decode two four-byte pointers to data instead of the
data itself.  Thus the doubles were generally 'nan'.

 - If the number of GPRs fit precisely on an integer number of rows,
blank space would be printed without even a trailing newline, and the
prompt would be mysteriously indented.


-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] mips: Fix "info registers" output

[Eli Zaretskii]

On Thu, 21 Jun 2001, Daniel Jacobowitz wrote:

>  - the decoding of double-precision arguments was completely broken. 
> In addition to copying unallocated memory, and getting the byte order
> wrong, we tried to decode two four-byte pointers to data instead of the
> data itself.  Thus the doubles were generally 'nan'.
> 
>  - If the number of GPRs fit precisely on an integer number of rows,
> blank space would be printed without even a trailing newline, and the
> prompt would be mysteriously indented.

I cannot reproduce this on SGI Irix 6.5, which also uses mips-tdeps.c.  I 
used a small throw-away FP program and all the FP registers printed 
perfectly okay.  No NaNs at all.  info registers seems also okay.

Perhaps I didn't try exactly what you did.  Any further hints?

Re: [RFA] mips: Fix "info registers" output

[Daniel Jacobowitz]

On Thu, Jun 21, 2001 at 07:31:15PM +0300, Eli Zaretskii wrote:
> 
> On Thu, 21 Jun 2001, Daniel Jacobowitz wrote:
> 
> >  - the decoding of double-precision arguments was completely broken. 
> > In addition to copying unallocated memory, and getting the byte order
> > wrong, we tried to decode two four-byte pointers to data instead of the
> > data itself.  Thus the doubles were generally 'nan'.
> > 
> >  - If the number of GPRs fit precisely on an integer number of rows,
> > blank space would be printed without even a trailing newline, and the
> > prompt would be mysteriously indented.
> 
> I cannot reproduce this on SGI Irix 6.5, which also uses mips-tdeps.c.  I 
> used a small throw-away FP program and all the FP registers printed 
> perfectly okay.  No NaNs at all.  info registers seems also okay.
> 
> Perhaps I didn't try exactly what you did.  Any further hints?

Really?  Hmm.  I looked over the code and it seemed that both branches
were wrong; however, I could be mistaken...

Aha!  Let me guess - Irix 6.5 is using the FP registers in 8 byte mode,
right?  There was indeed a bug on that path of the code, but it doesn't
actually affect the output.  We have this:

  raw_buffer[0] = (char *) alloca (REGISTER_RAW_SIZE (FP0_REGNUM));
  raw_buffer[1] = (char *) alloca (REGISTER_RAW_SIZE (FP0_REGNUM));
  dbl_buffer = (char *) alloca (2 * REGISTER_RAW_SIZE (FP0_REGNUM));

  /* Get the data in raw format.  */
  if (read_relative_register_raw_bytes (regnum, raw_buffer[HI]))
    error ("can't read register %d (%s)", regnum, REGISTER_NAME (regnum));

[snip]

      memcpy (dbl_buffer, raw_buffer[HI], 2 * REGISTER_RAW_SIZE (FP0_REGNUM));
      flt1 = unpack_double (builtin_type_float,
                            &raw_buffer[HI][offset], &inv1);
      doub = unpack_double (builtin_type_double, dbl_buffer, &inv3);

So we're copying 2 * 8 bytes out of an 8 byte buffer.  But float is
still 4 bytes and double still 8 bytes, so as long as we don't clobber
the stack we shouldn't see a real problem.


The real killer is on the other branch, size == 4:
      memcpy (dbl_buffer, raw_buffer, 2 * REGISTER_RAW_SIZE (FP0_REGNUM));

raw_buffer points to 8 bytes, sure enough - but they're both pointers
to four byte buffers before my patch.  That won't decode.


-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] mips: Fix "info registers" output

[Eli Zaretskii]

> Date: Thu, 21 Jun 2001 09:44:18 -0700
> From: Daniel Jacobowitz <dmj+@andrew.cmu.edu>
> 
> Aha!  Let me guess - Irix 6.5 is using the FP registers in 8 byte mode,
> right?

Yes.

> There was indeed a bug on that path of the code, but it doesn't
> actually affect the output.  We have this:
> 
>   raw_buffer[0] = (char *) alloca (REGISTER_RAW_SIZE (FP0_REGNUM));
>   raw_buffer[1] = (char *) alloca (REGISTER_RAW_SIZE (FP0_REGNUM));
>   dbl_buffer = (char *) alloca (2 * REGISTER_RAW_SIZE (FP0_REGNUM));
> 
>   /* Get the data in raw format.  */
>   if (read_relative_register_raw_bytes (regnum, raw_buffer[HI]))
>     error ("can't read register %d (%s)", regnum, REGISTER_NAME (regnum));
> 
> [snip]
> 
>       memcpy (dbl_buffer, raw_buffer[HI], 2 * REGISTER_RAW_SIZE (FP0_REGNUM));
>       flt1 = unpack_double (builtin_type_float,
>                             &raw_buffer[HI][offset], &inv1);
>       doub = unpack_double (builtin_type_double, dbl_buffer, &inv3);
> 
> So we're copying 2 * 8 bytes out of an 8 byte buffer.

I don't think so.  The memcpy part uses raw_buffer[HI] as its address,
but the two buffers whose addresses are in raw_buffer[0] and
raw_buffer[1] are layed out on the stack one after the other.  So you
have enough space there, and memcpy can copy up to 2*8 bytes without
fear.  It's nasty code, but it works.  (No, I didn't write that code ;-)

> The real killer is on the other branch, size == 4:
>       memcpy (dbl_buffer, raw_buffer, 2 * REGISTER_RAW_SIZE (FP0_REGNUM));
> 
> raw_buffer points to 8 bytes, sure enough - but they're both pointers
> to four byte buffers before my patch.  That won't decode.

So why replacing raw_buffer with raw_buffer[HI] in the call to memcpy
isn't all that is needed to fix this?

Re: [RFA] mips: Fix "info registers" output

[Daniel Jacobowitz]

On Thu, Jun 21, 2001 at 09:57:18PM +0300, Eli Zaretskii wrote:
> > There was indeed a bug on that path of the code, but it doesn't
> > actually affect the output.  We have this:
> > 
> >   raw_buffer[0] = (char *) alloca (REGISTER_RAW_SIZE (FP0_REGNUM));
> >   raw_buffer[1] = (char *) alloca (REGISTER_RAW_SIZE (FP0_REGNUM));
> >   dbl_buffer = (char *) alloca (2 * REGISTER_RAW_SIZE (FP0_REGNUM));
> > 
> >   /* Get the data in raw format.  */
> >   if (read_relative_register_raw_bytes (regnum, raw_buffer[HI]))
> >     error ("can't read register %d (%s)", regnum, REGISTER_NAME (regnum));
> > 
> > [snip]
> > 
> >       memcpy (dbl_buffer, raw_buffer[HI], 2 * REGISTER_RAW_SIZE (FP0_REGNUM));
> >       flt1 = unpack_double (builtin_type_float,
> >                             &raw_buffer[HI][offset], &inv1);
> >       doub = unpack_double (builtin_type_double, dbl_buffer, &inv3);
> > 
> > So we're copying 2 * 8 bytes out of an 8 byte buffer.
> 
> I don't think so.  The memcpy part uses raw_buffer[HI] as its address,
> but the two buffers whose addresses are in raw_buffer[0] and
> raw_buffer[1] are layed out on the stack one after the other.  So you
> have enough space there, and memcpy can copy up to 2*8 bytes without
> fear.  It's nasty code, but it works.  (No, I didn't write that code ;-)

Well, there's still issues.  We only initialized one register's worth
of data, for one thing.  And we only need one register's worth.

> > The real killer is on the other branch, size == 4:
> >       memcpy (dbl_buffer, raw_buffer, 2 * REGISTER_RAW_SIZE (FP0_REGNUM));
> > 
> > raw_buffer points to 8 bytes, sure enough - but they're both pointers
> > to four byte buffers before my patch.  That won't decode.
> 
> So why replacing raw_buffer with raw_buffer[HI] in the call to memcpy
> isn't all that is needed to fix this?

HI could be 0 or 1.  It's not clear what order we want to end up with,
or when we want to byteswap.

There's slightly less invasive corrections for this, but we have the
handy REGISTER_CONVERT_TO_TYPE macro.  We use it in the code for 'info
register f0', so I made us use the same code in 'info registers'.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] mips: Fix "info registers" output

[Don Howard]

I submitted a patch for this problem a while ago. Here is an explaination of
the problem as I understood it:

   Background: MIPS1 & 2 fp registers are 32 bits wide.  To support
   64bit operations, these early MIPS cpus treat fp register pairs
   (f0,f1) as a single register (d0).  Later MIPS cpu's have 64 bit fp
   registers and offer a compatibility mode that emulates the MIPS2 fp
   model.  When operating in MIPS2 fp compat mode, later cpu's split
   double precision floats into two 32-bit chunks and store them in
   consecutive fp regs.  To display 64-bit floats stored in this
   fashion, we have to combine 32 bits from f0 and 32 bits from f1.
   Throw in user-configurable endianness and you have a real mess.

I'll repost my patch shortly.

-- 
-Don
dhoward@redhat.com
gdb engineering

Re: [RFA] mips: Fix "info registers" output

[Daniel Jacobowitz]

On Tue, Jun 19, 2001 at 10:50:07PM -0700, Daniel Jacobowitz wrote:
> There were some pretty nasty problems in the FP register printing code, and
> a potential formatting problem in the GP register printing code.  How
> does this look to correct them?
> 
> 2001-06-19  Daniel Jacobowitz  <drow@mvista.com>
> 	* mips-tdep.c (do_fp_register_row): Convert to use
> 	REGISTER_CONVERT_TO_TYPE.
> 	(do_gp_register_row): Only add whitespace if registers
> 	were printed.

This patch got bogged down in details.  Rather than reposting it (there
are still several memory corruption bugs in do_fp_register_row) I'm
just submitting an obvious fix for the one that was biting me.  I think
the function should be killed in favor of something involving gdbarch,
but this fixes the immediate problem and I do not see an appropriate
gdbarch method.

To recap:
  On o32 MIPS systems, $f0 and $f1 are 32-bit values.  $d0 (well, $f0
  accessed by a .d instruction, generally) is a 64-bit value involving
  both of them.  If you want to read $f0 as a double, you want $d0.

GDB only supports two ways to see this: info registers f0 or info
all-registers.  We don't actually have a name for this value.  The best
fix would probably be to introduce $d0 as a pseudo register, but I
can't think how to do that without running into other problems on
systems with 64-bit FP registers.

Meanwhile, Andrew, is this trivial fix OK?  It just makes us decode the
actual register values instead of two host pointers in our call to
unpack_double.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

2002-03-07  Daniel Jacobowitz  <drow@mvista.com>

	* mips-tdep.c (do_fp_register_row): Print composite
	double-precision registers correctly.

Index: mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.66
diff -u -p -r1.66 mips-tdep.c
--- mips-tdep.c	2002/02/20 22:51:41	1.66
+++ mips-tdep.c	2002/03/07 21:48:44
@@ -2777,9 +2777,11 @@ do_fp_register_row (int regnum)
 	       regnum + 1, REGISTER_NAME (regnum + 1));
 
       /* copy the two floats into one double, and unpack both */
-      memcpy (dbl_buffer, raw_buffer, 2 * REGISTER_RAW_SIZE (FP0_REGNUM));
       flt1 = unpack_double (builtin_type_float, raw_buffer[HI], &inv1);
       flt2 = unpack_double (builtin_type_float, raw_buffer[LO], &inv2);
+      memcpy (dbl_buffer, raw_buffer[HI], REGISTER_RAW_SIZE (FP0_REGNUM));
+      memcpy (dbl_buffer + REGISTER_RAW_SIZE (FP0_REGNUM),
+	      raw_buffer[LO], REGISTER_RAW_SIZE (FP0_REGNUM));
       doub = unpack_double (builtin_type_double, dbl_buffer, &inv3);
 
       printf_filtered (" %-5s", REGISTER_NAME (regnum));

Re: [RFA] mips: Fix "info registers" output

[Andrew Cagney]

> Meanwhile, Andrew, is this trivial fix OK?  It just makes us decode the
> actual register values instead of two host pointers in our call to
> unpack_double.

I suspect the code was more broken then you thought!

If my memory is correct, the value is always stored LE in the register 
pair.  The code manages to do a double swap and consequently doesn't 
correctly re-order the registers in the dbl_buffer.

It is also really broken when a 32 bit ABI is on a 64 bit ISA.  (It very 
nearly corrupts the stack).

>  /* copy the two floats into one double, and unpack both */
> -      memcpy (dbl_buffer, raw_buffer, 2 * REGISTER_RAW_SIZE (FP0_REGNUM));
>        flt1 = unpack_double (builtin_type_float, raw_buffer[HI], &inv1);
>        flt2 = unpack_double (builtin_type_float, raw_buffer[LO], &inv2);
> +      memcpy (dbl_buffer, raw_buffer[HI], REGISTER_RAW_SIZE (FP0_REGNUM));
> +      memcpy (dbl_buffer + REGISTER_RAW_SIZE (FP0_REGNUM),
> +	      raw_buffer[LO], REGISTER_RAW_SIZE (FP0_REGNUM));
>        doub = unpack_double (builtin_type_double, dbl_buffer, &inv3);
>  

I suspect a bigger improvement would look something like (I'm still not 
100% sure of what goes where with BE/LE though :-)

if (!FP_REGISTER_DOUBLE)
   char *raw_buffer[2];
   char *dbl_buffer;
   raw_buffer[0] = alloca; [1] = alloca; dbl_buffer = alloca;
   read_relative_register(regnum+0, raw_buffer[0])
   read_relative_register(regnum+1, raw_buffer[1])
   if (BE)
     reg_offset = (REGISTER_RAW_SIZE() == 8) ? 4 : 0;
     type_float = builtin_type_ieee_single_big
     type_double = builtin_type_ieee_double_big
     memcpy dbl_buffer+4, raw_buffer[0] + reg_offset, 4);
     memcpy dbl_buffer+0, raw_buffer[1] + reg_offset, 4);
   else
     reg_offset = 0
     type_float = builtin_type_ieee_single_little
     type_double = builtin_type_ieee_double_little
     memcpy dbl_buffer+0, raw_buffer[0] + reg_offset, 4);
     memcpy dbl_buffer+4, raw_buffer[1] + reg_offset, 4);
   flt0 = unpack_double (type_float, raw_buffer[0] + reg_offset, &inv)
   flt1 = unpack_double (type_float, raw_buffer[1] + reg_offset, &inv)
   doub = unpack_double (type_double, dbl_buffer, &inv3)


Andrew

Re: [RFA] mips: Fix "info registers" output

[Daniel Jacobowitz]

On Sat, Mar 09, 2002 at 09:05:13PM -0500, Andrew Cagney wrote:
> >Meanwhile, Andrew, is this trivial fix OK?  It just makes us decode the
> >actual register values instead of two host pointers in our call to
> >unpack_double.
> 
> I suspect the code was more broken then you thought!

I don't know about that.  I pretty much rewrote it the first time I
tried to fix this, last June.

> If my memory is correct, the value is always stored LE in the register 
> pair.  The code manages to do a double swap and consequently doesn't 
> correctly re-order the registers in the dbl_buffer.
> 
> It is also really broken when a 32 bit ABI is on a 64 bit ISA.  (It very 
> nearly corrupts the stack).

I suppose...

> > /* copy the two floats into one double, and unpack both */
> >-      memcpy (dbl_buffer, raw_buffer, 2 * REGISTER_RAW_SIZE (FP0_REGNUM));
> >       flt1 = unpack_double (builtin_type_float, raw_buffer[HI], &inv1);
> >       flt2 = unpack_double (builtin_type_float, raw_buffer[LO], &inv2);
> >+      memcpy (dbl_buffer, raw_buffer[HI], REGISTER_RAW_SIZE (FP0_REGNUM));
> >+      memcpy (dbl_buffer + REGISTER_RAW_SIZE (FP0_REGNUM),
> >+	      raw_buffer[LO], REGISTER_RAW_SIZE (FP0_REGNUM));
> >       doub = unpack_double (builtin_type_double, dbl_buffer, &inv3);
> > 
> 
> I suspect a bigger improvement would look something like (I'm still not 
> 100% sure of what goes where with BE/LE though :-)
> 
> if (!FP_REGISTER_DOUBLE)

FP_REGISTER_DOUBLE describes a property of the ABI.  I don't really
think it's the appropriate check when printing floating-point
registers; we always take care to print the single-precision value even
if FP_REGISTER_DOUBLE, because they might be used in single-precision
anyway.

I sat down with an old patch from Don Howard (approved, but never
committed) and my old patches and thought about this for a bit.  I
fixed all the mentioned nits with both patches, and rethought some of
the design, and here's what I came up with.  The comments in the code
speak for themselves fairly well:

+/* Floating point register management.
+
+   Background: MIPS1 & 2 fp registers are 32 bits wide.  To support
+   64bit operations, these early MIPS cpus treat fp register pairs
+   (f0,f1) as a single register (d0).  Later MIPS cpu's have 64 bit fp
+   registers and offer a compatibility mode that emulates the MIPS2 fp
+   model.  When operating in MIPS2 fp compat mode, later cpu's split
+   double precision floats into two 32-bit chunks and store them in
+   consecutive fp regs.  To display 64-bit floats stored in this
+   fashion, we have to combine 32 bits from f0 and 32 bits from f1.
+   Throw in user-configurable endianness and you have a real mess.
+
+   The way this works is:
+     - If we are in 32-bit mode or on a 32-bit processor, then a 64-bit
+       double-precision value will be split across two logical registers.
+       The lower-numbered logical register will hold the low-order bits,
+       regardless of the processor's endianness.
+     - If we are on a 64-bit processor, and we are looking for a
+       single-precision value, it will be in the low ordered bits
+       of a 64-bit GPR (after mfc1, for example) or a 64-bit register
+       save slot in memory.
+     - If we are in 64-bit mode, everything is straightforward.

I add two functions, mips_read_fp_register_single and
mips_read_fp_register_double, that understand this.  In the process, I
eliminated the next-to-last use of REGISTER_CONVERT_TO_TYPE, and
simplified a lot of code.

Behaves correctly on the only target I have handy, which is a true
32-bit machine.  Andrew, do you like the look of this?

(Don's in the changelog because I cribbed one function and a lot of
comments from him.)

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

2002-03-10  Daniel Jacobowitz  <drow@mvista.com>
	    Don Howard <dhoward@redhat.com>

	* mips-tdep.c (ST0_FR): Define.
	(mips2_fp_compat): New function.
	(mips_read_fp_register_single): New function.
	(mips_read_fp_register_double): New function.
	(mips_print_register): Use them.
	(do_fp_register_row): Likewise.

Index: mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.66
diff -u -r1.66 mips-tdep.c
--- mips-tdep.c	2002/02/20 22:51:41	1.66
+++ mips-tdep.c	2002/03/10 06:43:35
@@ -44,6 +44,10 @@
 #include "elf-bfd.h"
 #include "symcat.h"
 
+/* A useful bit in the CP0 status register (PS_REGNUM).  */
+/* This bit is set if we are emulating 32-bit FPRs on a 64-bit chip.  */
+#define ST0_FR (1 << 26)
+
 /* The sizes of floating point registers.  */
 
 enum
@@ -174,6 +178,27 @@
     return 4;
 }
 
+/* Determine if a MIPS3 or later cpu is operating in MIPS{1,2} FPU
+   compatiblity mode.  A return value of 1 means that we have
+   physical 64-bit registers, but should treat them as 32-bit registers.  */
+
+static int
+mips2_fp_compat (void)
+{
+  /* MIPS1 and MIPS2 have only 32 bit FPRs, and the FR bit is not
+     meaningful.  */
+  if (REGISTER_RAW_SIZE (FP0_REGNUM) == 4)
+    return 0;
+
+  /* Otherwise check the FR bit in the status register - it controls
+     the FP compatiblity mode.  If it is clear we are in compatibility
+     mode.  */
+  if ((read_register (PS_REGNUM) & ST0_FR) == 0)
+    return 1;
+  
+  return 0;
+}
+
 /* Indicate that the ABI makes use of double-precision registers
    provided by the FPU (rather than combining pairs of registers to
    form double-precision values).  Do not use "TARGET_IS_MIPS64" to
@@ -257,6 +282,9 @@
 static CORE_ADDR after_prologue (CORE_ADDR pc,
 				 mips_extra_func_info_t proc_desc);
 
+static void mips_read_fp_register_single (int regno, char *rare_buffer);
+static void mips_read_fp_register_double (int regno, char *rare_buffer);
+
 /* This value is the model of MIPS in use.  It is derived from the value
    of the PrID register.  */
 
@@ -2676,7 +2704,103 @@
     }
 }
 
+/* Floating point register management.
+
+   Background: MIPS1 & 2 fp registers are 32 bits wide.  To support
+   64bit operations, these early MIPS cpus treat fp register pairs
+   (f0,f1) as a single register (d0).  Later MIPS cpu's have 64 bit fp
+   registers and offer a compatibility mode that emulates the MIPS2 fp
+   model.  When operating in MIPS2 fp compat mode, later cpu's split
+   double precision floats into two 32-bit chunks and store them in
+   consecutive fp regs.  To display 64-bit floats stored in this
+   fashion, we have to combine 32 bits from f0 and 32 bits from f1.
+   Throw in user-configurable endianness and you have a real mess.
+
+   The way this works is:
+     - If we are in 32-bit mode or on a 32-bit processor, then a 64-bit
+       double-precision value will be split across two logical registers.
+       The lower-numbered logical register will hold the low-order bits,
+       regardless of the processor's endianness.
+     - If we are on a 64-bit processor, and we are looking for a
+       single-precision value, it will be in the low ordered bits
+       of a 64-bit GPR (after mfc1, for example) or a 64-bit register
+       save slot in memory.
+     - If we are in 64-bit mode, everything is straightforward.
+
+   Note that this code only deals with "live" registers at the top of the
+   stack.  We will attempt to deal with saved registers later, when
+   the raw/cooked register interface is in place. (We need a general
+   interface that can deal with dynamic saved register sizes -- fp
+   regs could be 32 bits wide in one frame and 64 on the frame above
+   and below).  */
+
+/* Copy a 32-bit single-precision value from the current frame
+   into rare_buffer.  */
+
 static void
+mips_read_fp_register_single (int regno, char *rare_buffer)
+{
+  int raw_size = REGISTER_RAW_SIZE (regno);
+  char *raw_buffer = alloca (raw_size);
+
+  if (read_relative_register_raw_bytes (regno, raw_buffer))
+    error ("can't read register %d (%s)", regno, REGISTER_NAME (regno));
+  if (raw_size == 8)
+    {
+      /* We have a 64-bit value for this register.  Find the low-order
+	 32 bits.  */
+      int offset;
+
+      if (TARGET_BYTE_ORDER == BFD_ENDIAN_BIG)
+	offset = 4;
+      else
+	offset = 0;
+
+      memcpy (rare_buffer, raw_buffer + offset, 4);
+    }
+  else
+    {
+      memcpy (rare_buffer, raw_buffer, 4);
+    }
+}
+
+/* Copy a 64-bit double-precision value from the current frame into
+   rare_buffer.  This may include getting half of it from the next
+   register.  */
+
+static void
+mips_read_fp_register_double (int regno, char *rare_buffer)
+{
+  int raw_size = REGISTER_RAW_SIZE (regno);
+
+  if (raw_size == 8 && !mips2_fp_compat ())
+    {
+      /* We have a 64-bit value for this register, and we should use
+	 all 64 bits.  */
+      if (read_relative_register_raw_bytes (regno, rare_buffer))
+	error ("can't read register %d (%s)", regno, REGISTER_NAME (regno));
+    }
+  else
+    {
+      if ((regno - FP0_REGNUM) & 1)
+	internal_error (__FILE__, __LINE__,
+			"mips_read_fp_register_double: bad access to "
+			"odd-numbered FP register");
+
+      if (TARGET_BYTE_ORDER == BFD_ENDIAN_BIG)
+	{
+	  mips_read_fp_register_single (regno, rare_buffer + 4);
+	  mips_read_fp_register_single (regno + 1, rare_buffer);
+	}
+      else      
+	{
+	  mips_read_fp_register_single (regno, rare_buffer);
+	  mips_read_fp_register_single (regno + 1, rare_buffer + 4);
+	}
+    }
+}
+
+static void
 mips_print_register (int regnum, int all)
 {
   char raw_buffer[MAX_REGISTER_RAW_SIZE];
@@ -2688,22 +2812,23 @@
       return;
     }
 
-  /* If an even floating point register, also print as double. */
+  /* If we have a actual 32-bit floating point register (or we are in
+     32-bit compatibility mode), and the register is even-numbered,
+     also print it as a double (spanning two registers).  */
   if (TYPE_CODE (REGISTER_VIRTUAL_TYPE (regnum)) == TYPE_CODE_FLT
+      && (REGISTER_RAW_SIZE (regnum) == 4
+	  || mips2_fp_compat ())
       && !((regnum - FP0_REGNUM) & 1))
-    if (REGISTER_RAW_SIZE (regnum) == 4)	/* this would be silly on MIPS64 or N32 (Irix 6) */
-      {
-	char dbuffer[2 * MAX_REGISTER_RAW_SIZE];
+    {
+      char dbuffer[2 * MAX_REGISTER_RAW_SIZE];
 
-	read_relative_register_raw_bytes (regnum, dbuffer);
-	read_relative_register_raw_bytes (regnum + 1, dbuffer + MIPS_REGSIZE);
-	REGISTER_CONVERT_TO_TYPE (regnum, builtin_type_double, dbuffer);
+      mips_read_fp_register_double (regnum, dbuffer);
 
-	printf_filtered ("(d%d: ", regnum - FP0_REGNUM);
-	val_print (builtin_type_double, dbuffer, 0, 0,
-		   gdb_stdout, 0, 1, 0, Val_pretty_default);
-	printf_filtered ("); ");
-      }
+      printf_filtered ("(d%d: ", regnum - FP0_REGNUM);
+      val_print (builtin_type_double, dbuffer, 0, 0,
+		 gdb_stdout, 0, 1, 0, Val_pretty_default);
+      printf_filtered ("); ");
+    }
   fputs_filtered (REGISTER_NAME (regnum), gdb_stdout);
 
   /* The problem with printing numeric register names (r26, etc.) is that
@@ -2717,8 +2842,10 @@
 
   /* If virtual format is floating, print it that way.  */
   if (TYPE_CODE (REGISTER_VIRTUAL_TYPE (regnum)) == TYPE_CODE_FLT)
-    if (FP_REGISTER_DOUBLE)
-      {				/* show 8-byte floats as float AND double: */
+    if (REGISTER_RAW_SIZE (regnum) == 8 && !mips2_fp_compat ())
+      {
+	/* We have a meaningful 64-bit value in this register.  Show
+	   it as a 32-bit float and a 64-bit double.  */
 	int offset = 4 * (TARGET_BYTE_ORDER == BFD_ENDIAN_BIG);
 
 	printf_filtered (" (float) ");
@@ -2753,35 +2880,25 @@
 static int
 do_fp_register_row (int regnum)
 {				/* do values for FP (float) regs */
-  char *raw_buffer[2];
-  char *dbl_buffer;
-  /* use HI and LO to control the order of combining two flt regs */
-  int HI = (TARGET_BYTE_ORDER == BFD_ENDIAN_BIG);
-  int LO = (TARGET_BYTE_ORDER != BFD_ENDIAN_BIG);
+  char *raw_buffer;
   double doub, flt1, flt2;	/* doubles extracted from raw hex data */
   int inv1, inv2, inv3;
 
-  raw_buffer[0] = (char *) alloca (REGISTER_RAW_SIZE (FP0_REGNUM));
-  raw_buffer[1] = (char *) alloca (REGISTER_RAW_SIZE (FP0_REGNUM));
-  dbl_buffer = (char *) alloca (2 * REGISTER_RAW_SIZE (FP0_REGNUM));
+  raw_buffer = (char *) alloca (2 * REGISTER_RAW_SIZE (FP0_REGNUM));
 
-  /* Get the data in raw format.  */
-  if (read_relative_register_raw_bytes (regnum, raw_buffer[HI]))
-    error ("can't read register %d (%s)", regnum, REGISTER_NAME (regnum));
-  if (REGISTER_RAW_SIZE (regnum) == 4)
-    {
-      /* 4-byte registers: we can fit two registers per row. */
-      /* Also print every pair of 4-byte regs as an 8-byte double. */
-      if (read_relative_register_raw_bytes (regnum + 1, raw_buffer[LO]))
-	error ("can't read register %d (%s)",
-	       regnum + 1, REGISTER_NAME (regnum + 1));
-
-      /* copy the two floats into one double, and unpack both */
-      memcpy (dbl_buffer, raw_buffer, 2 * REGISTER_RAW_SIZE (FP0_REGNUM));
-      flt1 = unpack_double (builtin_type_float, raw_buffer[HI], &inv1);
-      flt2 = unpack_double (builtin_type_float, raw_buffer[LO], &inv2);
-      doub = unpack_double (builtin_type_double, dbl_buffer, &inv3);
+  if (REGISTER_RAW_SIZE (regnum) == 4 || mips2_fp_compat ())
+    {
+      /* 4-byte registers: we can fit two registers per row.  */
+      /* Also print every pair of 4-byte regs as an 8-byte double.  */
+      mips_read_fp_register_single (regnum, raw_buffer);
+      flt1 = unpack_double (builtin_type_float, raw_buffer, &inv1);
+
+      mips_read_fp_register_single (regnum + 1, raw_buffer);
+      flt2 = unpack_double (builtin_type_float, raw_buffer, &inv2);
 
+      mips_read_fp_register_double (regnum, raw_buffer);
+      doub = unpack_double (builtin_type_double, raw_buffer, &inv3);
+      
       printf_filtered (" %-5s", REGISTER_NAME (regnum));
       if (inv1)
 	printf_filtered (": <invalid float>");
@@ -2805,14 +2922,14 @@
       regnum += 2;
     }
   else
-    {				/* eight byte registers: print each one as float AND as double. */
-      int offset = 4 * (TARGET_BYTE_ORDER == BFD_ENDIAN_BIG);
-
-      memcpy (dbl_buffer, raw_buffer[HI], 2 * REGISTER_RAW_SIZE (FP0_REGNUM));
-      flt1 = unpack_double (builtin_type_float,
-			    &raw_buffer[HI][offset], &inv1);
-      doub = unpack_double (builtin_type_double, dbl_buffer, &inv3);
-
+    {
+      /* Eight byte registers: print each one as float AND as double.  */
+      mips_read_fp_register_single (regnum, raw_buffer);
+      flt1 = unpack_double (builtin_type_double, raw_buffer, &inv1);
+
+      mips_read_fp_register_double (regnum, raw_buffer);
+      doub = unpack_double (builtin_type_double, raw_buffer, &inv3);
+      
       printf_filtered (" %-5s: ", REGISTER_NAME (regnum));
       if (inv1)
 	printf_filtered ("<invalid float>");

Re: [RFA] mips: Fix "info registers" output

[Andrew Cagney]

> 
> FP_REGISTER_DOUBLE describes a property of the ABI.  I don't really
> think it's the appropriate check when printing floating-point
> registers; we always take care to print the single-precision value even
> if FP_REGISTER_DOUBLE, because they might be used in single-precision
> anyway.

True, sort of.  The decision is a function of that FP bit, 
FP_REGISTER_DOUBLE and the user typing ``(gdb) set mips 
fp-register-double on, damit!'' (the user is always right :-).

If the FP register bit is used by just this code, other parts of GDB are 
going to be inconsistent since they are still using FP_REGISTER_DOUBLE 
when [un]packing FP registers.  Can I suggest using FP_REGISTER_DOUBLE 
initially (#if 0 #else #endif the code in mips2_fp_compat()) and bug 
report the need to change everyting to use mips2_fp_compat() as a 
separate change.

Apart from that, I think the code is brilliant.  Just suggest a few 
comment tweaks before the commit.

+      if (TARGET_BYTE_ORDER == BFD_ENDIAN_BIG)
+ 
{
+ 
   mips_read_fp_register_single (regno, rare_buffer + 4);
+ 
   mips_read_fp_register_single (regno + 1, rare_buffer);
+ 
}
+      else
+ 
{
+ 
   mips_read_fp_register_single (regno, rare_buffer);
+ 
   mips_read_fp_register_single (regno + 1, rare_buffer + 4);
+ 
}

Suggest mentioning that mips_read_fp_register_single() handles the 
problem of extracting the correct four bytes from from each register.

> -  /* use HI and LO to control the order of combining two flt regs */
> -  int HI = (TARGET_BYTE_ORDER == BFD_ENDIAN_BIG);
> -  int LO = (TARGET_BYTE_ORDER != BFD_ENDIAN_BIG);

Yes! In 20:20 hindsight that was a very confusing idea.

> +      /* 4-byte registers: we can fit two registers per row.  */
> +      /* Also print every pair of 4-byte regs as an 8-byte double.  */
> +      mips_read_fp_register_single (regnum, raw_buffer);
> +      flt1 = unpack_double (builtin_type_float, raw_buffer, &inv1);
> +
> +      mips_read_fp_register_single (regnum + 1, raw_buffer);
> +      flt2 = unpack_double (builtin_type_float, raw_buffer, &inv2);
>  
> +      mips_read_fp_register_double (regnum, raw_buffer);
> +      doub = unpack_double (builtin_type_double, raw_buffer, &inv3);
> +      
>        printf_filtered (" %-5s", REGISTER_NAME (regnum));

Suggest a FIXME and bug report here.  It isn't safe to assume things 
like builtin_type_double is 64 bit.  The code should use the ABI 
independant builtin_type_ieee_BLAH.  But this is a separate bug and not 
your problem :-)

enjoy,
Andrew

Re: [RFA] mips: Fix "info registers" output

[Daniel Jacobowitz]

On Sun, Mar 10, 2002 at 11:30:09AM -0500, Andrew Cagney wrote:
> >
> >FP_REGISTER_DOUBLE describes a property of the ABI.  I don't really
> >think it's the appropriate check when printing floating-point
> >registers; we always take care to print the single-precision value even
> >if FP_REGISTER_DOUBLE, because they might be used in single-precision
> >anyway.
> 
> True, sort of.  The decision is a function of that FP bit, 
> FP_REGISTER_DOUBLE and the user typing ``(gdb) set mips 
> fp-register-double on, damit!'' (the user is always right :-).

(said `set' does not exist, of course)

> If the FP register bit is used by just this code, other parts of GDB are 
> going to be inconsistent since they are still using FP_REGISTER_DOUBLE 
> when [un]packing FP registers.  Can I suggest using FP_REGISTER_DOUBLE 
> initially (#if 0 #else #endif the code in mips2_fp_compat()) and bug 
> report the need to change everyting to use mips2_fp_compat() as a 
> separate change.

But there is nowhere else that we really unpack FP registers... well, I
suppose there is actually.  A fixme and PR it is.  The PR will include
a question about what to do with the displayed types of these
registers.

I have a few ideas, involving gdbarch, on how to solve this properly. 
I'll get back to it in a few months, thus the PR :)

> Apart from that, I think the code is brilliant.  Just suggest a few 
> comment tweaks before the commit.
> 
> +      if (TARGET_BYTE_ORDER == BFD_ENDIAN_BIG)
> + 
> {
> + 
>   mips_read_fp_register_single (regno, rare_buffer + 4);
> + 
>   mips_read_fp_register_single (regno + 1, rare_buffer);
> + 
> }
> +      else
> + 
> {
> + 
>   mips_read_fp_register_single (regno, rare_buffer);
> + 
>   mips_read_fp_register_single (regno + 1, rare_buffer + 4);
> + 
> }
> 
> Suggest mentioning that mips_read_fp_register_single() handles the 
> problem of extracting the correct four bytes from from each register.

OK, will do.

> >-  /* use HI and LO to control the order of combining two flt regs */
> >-  int HI = (TARGET_BYTE_ORDER == BFD_ENDIAN_BIG);
> >-  int LO = (TARGET_BYTE_ORDER != BFD_ENDIAN_BIG);
> 
> Yes! In 20:20 hindsight that was a very confusing idea.
> 
> >+      /* 4-byte registers: we can fit two registers per row.  */
> >+      /* Also print every pair of 4-byte regs as an 8-byte double.  */
> >+      mips_read_fp_register_single (regnum, raw_buffer);
> >+      flt1 = unpack_double (builtin_type_float, raw_buffer, &inv1);
> >+
> >+      mips_read_fp_register_single (regnum + 1, raw_buffer);
> >+      flt2 = unpack_double (builtin_type_float, raw_buffer, &inv2);
> > 
> >+      mips_read_fp_register_double (regnum, raw_buffer);
> >+      doub = unpack_double (builtin_type_double, raw_buffer, &inv3);
> >+      
> >       printf_filtered (" %-5s", REGISTER_NAME (regnum));
> 
> Suggest a FIXME and bug report here.  It isn't safe to assume things 
> like builtin_type_double is 64 bit.  The code should use the ABI 
> independant builtin_type_ieee_BLAH.  But this is a separate bug and not 
> your problem :-)

  builtin_type_double =
    init_type (TYPE_CODE_FLT, TARGET_DOUBLE_BIT / TARGET_CHAR_BIT,
               0,
               "double", (struct objfile *) NULL);


  set_gdbarch_double_bit (gdbarch, 64);


Why isn't it safe to assume that a double is 64-bit when we explicitly
set it that way?  I assume that the builtin types get swapped out when
we change gdbarch... yes, they do.  Besides, is MIPS FP actually IEEE? 
Oh, I suppose the values probably are and only some of the math isn't.

Committed without that last FIXME; I'll add it if it's really
necessary.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] mips: Fix "info registers" output

[Andrew Cagney]

> On Sun, Mar 10, 2002 at 11:30:09AM -0500, Andrew Cagney wrote:

>> Suggest a FIXME and bug report here.  It isn't safe to assume things 
>> like builtin_type_double is 64 bit.  The code should use the ABI 
>> independant builtin_type_ieee_BLAH.  But this is a separate bug and not 
>> your problem :-)
> 
> 
> builtin_type_double =
>     init_type (TYPE_CODE_FLT, TARGET_DOUBLE_BIT / TARGET_CHAR_BIT,
>                0,
>                "double", (struct objfile *) NULL);
> 
> 
>   set_gdbarch_double_bit (gdbarch, 64);
> 
> 
> Why isn't it safe to assume that a double is 64-bit when we explicitly
> set it that way?  I assume that the builtin types get swapped out when
> we change gdbarch... yes, they do.  Besides, is MIPS FP actually IEEE? 
> Oh, I suppose the values probably are and only some of the math isn't.

Have a look at GCC's -fshort-double option.  I'm not sure how MIPS would 
respond to it but I suspect it would make everyones head hurt. :-)

The ``info registers'' code is both ISA and ABI dependant.  ISA since 
that determines the raw format of registers (ieee_double_{big,little} 
not double) - the ABI shouldn't change a MIPS DOUBLE.  ABI since that 
determins where bits of registers end up being saved on the stack.

> Committed without that last FIXME; I'll add it if it's really
> necessary
Er, where's the fire?  With all respect, it is always better to give the 
other party the chance to respond.

Andrew

Re: [RFA] mips: Fix "info registers" output

[Daniel Jacobowitz]

On Sun, Mar 10, 2002 at 12:46:46PM -0500, Andrew Cagney wrote:
> >On Sun, Mar 10, 2002 at 11:30:09AM -0500, Andrew Cagney wrote:
> 
> >>Suggest a FIXME and bug report here.  It isn't safe to assume things 
> >>like builtin_type_double is 64 bit.  The code should use the ABI 
> >>independant builtin_type_ieee_BLAH.  But this is a separate bug and not 
> >>your problem :-)
> >
> >
> >builtin_type_double =
> >    init_type (TYPE_CODE_FLT, TARGET_DOUBLE_BIT / TARGET_CHAR_BIT,
> >               0,
> >               "double", (struct objfile *) NULL);
> >
> >
> >  set_gdbarch_double_bit (gdbarch, 64);
> >
> >
> >Why isn't it safe to assume that a double is 64-bit when we explicitly
> >set it that way?  I assume that the builtin types get swapped out when
> >we change gdbarch... yes, they do.  Besides, is MIPS FP actually IEEE? 
> >Oh, I suppose the values probably are and only some of the math isn't.
> 
> Have a look at GCC's -fshort-double option.  I'm not sure how MIPS would 
> respond to it but I suspect it would make everyones head hurt. :-)

I suspect the stabs reader would go insane :)  You'd have two floating
point types with different names but the same size... all sorts of
assumptions would probably get confused.

That wouldn't affect builtin_type_double, though, which we define in
terms of the 64 bits.  Are you saying that should be updated based on
the type of 'double' in the objfile?  I don't really think so, since
'the type associated with the word "double"' isn't necessarily tied to
"what this architecture would normally call a double" and
builtin_type_double is the latter.  MIPS would normally call a 64-bit
FP quantity a double, whatever GCC is up to.

> The ``info registers'' code is both ISA and ABI dependant.  ISA since 
> that determines the raw format of registers (ieee_double_{big,little} 
> not double) - the ABI shouldn't change a MIPS DOUBLE.  ABI since that 
> determins where bits of registers end up being saved on the stack.

Right; the way registers are acquired is both ISA and ABI dependent.  I
don't know that the way they are interpreted afterwards is ABI
dependent...

> >Committed without that last FIXME; I'll add it if it's really
> >necessary
> Er, where's the fire?  With all respect, it is always better to give the 
> other party the chance to respond.

Sorry, no fire.  Perhaps I'm just unconscionably impatient, but the
more almost-entirely-approved patches I have lying around from nine
months ago, the more confused I become.  I'll try to curb myself.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] mips: Fix "info registers" output

[Andrew Cagney]

> Have a look at GCC's -fshort-double option.  I'm not sure how MIPS would 
>> respond to it but I suspect it would make everyones head hurt. :-)
> 
> 
> I suspect the stabs reader would go insane :)  You'd have two floating
> point types with different names but the same size... all sorts of
> assumptions would probably get confused.

It works for the d10v!  That has:

	float 32
	double 32
	long double 64

by default.  The poor d10v gets confused when ``double 64'' is specifed 
but that is only because the corresponding gdb code pre-dates multi-arch 
and so code to handle this is simply missing.

> That wouldn't affect builtin_type_double, though, which we define in
> terms of the 64 bits.  Are you saying that should be updated based on
> the type of 'double' in the objfile?  I don't really think so, since
> 'the type associated with the word "double"' isn't necessarily tied to
> "what this architecture would normally call a double" and
> builtin_type_double is the latter.  MIPS would normally call a 64-bit
> FP quantity a double, whatever GCC is up to.

Now I'm confused :-).  The type builtin_type_double is determined by the 
ABI, its value/behavour is influenced by TARGET_DOUBLE_BIT, 
TARGET_BYTE_ORDER and TARGET_DOUBLE_FORMAT.  They are all attributes of 
the ABI and their values can be determined from the object file.

Separate to this is the real register format and that is determined by 
the ISA.

Consider the PPC which has strictly 64 bit FP registers but supports the 
32 bit ``float'' type.  If you print the register it is 64 bit, if you 
print a 32 bit float stored in the register than the 64 bit value is 
first converted to 32 bit.

Other examples are Arm, m68k and i386.  As soon as those targets stopped 
trying to use type_double et.al. bugs mysteriously disappeared and the 
code became simpler :-)  The mips hasn't yet done this.

> 
> Right; the way registers are acquired is both ISA and ABI dependent.  I
> don't know that the way they are interpreted afterwards is ABI
> dependent...

Their interpretation is ISA dependant so they should be using things 
like type_ieee_double et.al.

enjoy,
Andrew

Re: [RFA] mips: Fix "info registers" output

[Daniel Jacobowitz]

On Sun, Mar 10, 2002 at 03:45:15PM -0500, Andrew Cagney wrote:
> >That wouldn't affect builtin_type_double, though, which we define in
> >terms of the 64 bits.  Are you saying that should be updated based on
> >the type of 'double' in the objfile?  I don't really think so, since
> >'the type associated with the word "double"' isn't necessarily tied to
> >"what this architecture would normally call a double" and
> >builtin_type_double is the latter.  MIPS would normally call a 64-bit
> >FP quantity a double, whatever GCC is up to.
> 
> Now I'm confused :-).  The type builtin_type_double is determined by the 
> ABI, its value/behavour is influenced by TARGET_DOUBLE_BIT, 
> TARGET_BYTE_ORDER and TARGET_DOUBLE_FORMAT.  They are all attributes of 
> the ABI and their values can be determined from the object file.

OK... but in all MIPS ABIs that I'm aware of, ever, double is 64-bit. 
Still, I see that I've misunderstood the meaning of the builtin types.

> Separate to this is the real register format and that is determined by 
> the ISA.
> 
> Consider the PPC which has strictly 64 bit FP registers but supports the 
> 32 bit ``float'' type.  If you print the register it is 64 bit, if you 
> print a 32 bit float stored in the register than the 64 bit value is 
> first converted to 32 bit.
> 
> Other examples are Arm, m68k and i386.  As soon as those targets stopped 
> trying to use type_double et.al. bugs mysteriously disappeared and the 
> code became simpler :-)  The mips hasn't yet done this.
> 
> >
> >Right; the way registers are acquired is both ISA and ABI dependent.  I
> >don't know that the way they are interpreted afterwards is ABI
> >dependent...
> 
> Their interpretation is ISA dependant so they should be using things 
> like type_ieee_double et.al.

Right now, for MIPS, builtin_type_double always means "whichever of
builtin_type_ieee_double_{big,little} is appropriate" and likewise
for builtin_type_float.  (I note in passing that the type_ieee
variables are not referenced.  Anywhere.  Only the floatformats are.)

All I could see changed in mips-tdep.c would be to explicitly state
that, and I don't really see the point, given the MIPS FP models we
support.  That's why I didn't see the need for the FIXME in question.
Perhaps I'm not understanding you...?

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] mips: Fix "info registers" output

[Andrew Cagney]

> Right now, for MIPS, builtin_type_double always means "whichever of
> builtin_type_ieee_double_{big,little} is appropriate" and likewise
> for builtin_type_float.  (I note in passing that the type_ieee
> variables are not referenced.  Anywhere.  Only the floatformats are.)

Yes, it is a bug.  MIPS integer registers have been (mostly?) changed to 
the correct uint32 et.al.  That fixed problems when debugging -mlong64 
-mlong32 etc code.

> All I could see changed in mips-tdep.c would be to explicitly state
> that, and I don't really see the point, given the MIPS FP models we
> support.  That's why I didn't see the need for the FIXME in question.
> Perhaps I'm not understanding you...?

It is one of those things where the ``right thing'' may not appear to 
have an immediate return.

enjoy,
Andrew