mips-tdep.c: Unification of FPR size detection

mips-tdep.c: Unification of FPR size detection

[Maciej W. Rozycki]

Hello,

 This is a set of changes to put code to determine the width of floating 
point registers into a single place.  This is required for the support for 
MIPS32r2 64-bit FPRs to be added in a sensible way.

 I think the changes are mostly obvious.  If not, then please enquire.  
Modifications to mips_o32_return_value(), mips_o32_push_dummy_call() and 
mips_register_type() change the semantics somewhat, but this is correct as 
for 64-bit processors in the compatibility mode FPRs are seen as 32-bit.  
The rest just shuffles code for consistency.

 One note: the mips2_fp_compat() function is meant to detect the size of 
floating point registers based on the setting of the cp0.Status.FR bit as 
applicable.  However the function currently always returns 0.  So I 
decided to include a change that ifdefs out the entire body.  Enabling the 
body does not trigger any new regressions.  I also added a note on a 
problem with the frame being NULL -- it may have to be dealt with in a 
better way once this code is reenabled.  A possible way is by caching the 
value of cp0.Status.FR elsewhere.

 Tested for using the mipsisa32-sde-elf target, with the 
mips-sim-sde32/-EB, mips-sim-sde32/-EL, mips-sim-sde64/-mips64/-EB and 
mips-sim-sde64/-mips64/-EL boards with no regressions.

2007-11-21  Nigel Stephens  <nigel@mips.com>
            David Ung  <davidu@mips.com>
            Maciej W. Rozycki  <macro@mips.com>

	* mips-tdep.c (mips_float_register_p): New function.
	(mips_float_regsize): Likewise.
	(mips2_fp_compat): Handle a NULL pointer possibly passed.  Ifdef 
	out pointless code.
	(mips_convert_register_p): Use mips_float_register_p().
	(mips_register_type): Likewise.  Use mips_float_regsize()
	instead of mips_isa_regsize() for FP registers.
	(mips_o32_push_dummy_call): Use mips_float_regsize() instead of
	register_size() for FP registers.
	(mips_print_fp_register): Likewise.
	(mips_print_register): Likewise.
	(print_gp_register_row): Likewise.
	(mips_o32_return_value): Use mips_float_regsize() instead of
	hardcoding the expected width of FP registers.

 OK to apply?

  Maciej

14440-2.diff
Index: binutils-quilt/src/gdb/mips-tdep.c
===================================================================
--- binutils-quilt.orig/src/gdb/mips-tdep.c	2007-11-21 11:37:00.000000000 +0000
+++ binutils-quilt/src/gdb/mips-tdep.c	2007-11-21 11:47:35.000000000 +0000
@@ -62,6 +62,8 @@
 
 static struct type *mips_register_type (struct gdbarch *gdbarch, int regnum);
 
+static int mips2_fp_compat (struct frame_info *frame);
+
 /* A useful bit in the CP0 status register (MIPS_PS_REGNUM).  */
 /* This bit is set if we are emulating 32-bit FPRs on a 64-bit chip.  */
 #define ST0_FR (1 << 26)
@@ -211,6 +213,24 @@
   return mips_regnum (gdbarch)->fp0 + 12;
 }
 
+static int
+mips_float_register_p (struct gdbarch *gdbarch, int regnum)
+{
+  return ((regnum % gdbarch_num_regs (gdbarch))
+	  >= mips_regnum (gdbarch)->fp0
+	  && (regnum % gdbarch_num_regs (gdbarch))
+	     < mips_regnum (gdbarch)->fp0 + 32);
+}
+
+static int
+mips_float_regsize (struct gdbarch *gdbarch, struct frame_info *frame)
+{
+  if (mips_isa_regsize (gdbarch) == 8 && ! mips2_fp_compat (frame))
+    /* 64-bit ISA with 32-bit compatibility mode FPU.  */
+    return 8;
+  return 4;
+}
+
 #define MIPS_EABI (gdbarch_tdep (current_gdbarch)->mips_abi == MIPS_ABI_EABI32 \
 		   || gdbarch_tdep (current_gdbarch)->mips_abi == MIPS_ABI_EABI64)
 
@@ -370,13 +390,21 @@
 static int
 mips2_fp_compat (struct frame_info *frame)
 {
-  struct gdbarch *gdbarch = get_frame_arch (frame);
+#if 0
+  struct gdbarch *gdbarch;
+
+  /* FIXME macro 2007-11-15: If we have no frame, then we cannot really
+     get it or gdbarch_data() will be called recursively (through
+     data->post_init()).  Assume the native size.  */
+  if (frame == NULL)
+    return 0;
+  gdbarch = get_frame_arch (frame);
+
   /* MIPS1 and MIPS2 have only 32 bit FPRs, and the FR bit is not
      meaningful.  */
   if (register_size (gdbarch, mips_regnum (gdbarch)->fp0) == 4)
     return 0;
 
-#if 0
   /* FIXME drow 2002-03-10: This is disabled until we can do it consistently,
      in all the places we deal with FP registers.  PR gdb/413.  */
   /* Otherwise check the FR bit in the status register - it controls
@@ -664,10 +692,7 @@
 {
   return (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG
 	  && register_size (gdbarch, regnum) == 4
-	  && (regnum % gdbarch_num_regs (gdbarch))
-		>= mips_regnum (gdbarch)->fp0
-	  && (regnum % gdbarch_num_regs (gdbarch))
-		< mips_regnum (gdbarch)->fp0 + 32
+	  && mips_float_register_p (gdbarch, regnum)
 	  && TYPE_CODE (type) == TYPE_CODE_FLT && TYPE_LENGTH (type) == 8);
 }
 
@@ -694,13 +719,11 @@
 mips_register_type (struct gdbarch *gdbarch, int regnum)
 {
   gdb_assert (regnum >= 0 && regnum < 2 * gdbarch_num_regs (gdbarch));
-  if ((regnum % gdbarch_num_regs (gdbarch)) >= mips_regnum (gdbarch)->fp0
-      && (regnum % gdbarch_num_regs (gdbarch))
-	 < mips_regnum (gdbarch)->fp0 + 32)
+  if (mips_float_register_p (gdbarch, regnum))
     {
       /* The floating-point registers raw, or cooked, always match
          mips_isa_regsize(), and also map 1:1, byte for byte.  */
-      if (mips_isa_regsize (gdbarch) == 4)
+      if (mips_float_regsize (gdbarch, NULL) == 4)
 	return builtin_type_ieee_single;
       else
 	return builtin_type_ieee_double;
@@ -3512,7 +3535,7 @@
       if (fp_register_arg_p (typecode, arg_type)
 	  && float_argreg <= MIPS_LAST_FP_ARG_REGNUM)
 	{
-	  if (register_size (gdbarch, float_argreg) < 8 && len == 8)
+	  if (mips_float_regsize (gdbarch, NULL) < 8 && len == 8)
 	    {
 	      int low_offset = gdbarch_byte_order (gdbarch)
 			       == BFD_ENDIAN_BIG ? 4 : 0;
@@ -3721,7 +3744,8 @@
       || TYPE_CODE (type) == TYPE_CODE_ARRAY)
     return RETURN_VALUE_STRUCT_CONVENTION;
   else if (TYPE_CODE (type) == TYPE_CODE_FLT
-	   && TYPE_LENGTH (type) == 4 && tdep->mips_fpu_type != MIPS_FPU_NONE)
+	   && TYPE_LENGTH (type) <= mips_float_regsize (gdbarch, NULL)
+	   && tdep->mips_fpu_type != MIPS_FPU_NONE)
     {
       /* A single-precision floating-point value.  It fits in the
          least significant part of FP0.  */
@@ -4275,14 +4299,14 @@
   double doub, flt1;	/* doubles extracted from raw hex data */
   int inv1, inv2;
 
-  raw_buffer = alloca (2 * register_size (gdbarch, mips_regnum (gdbarch)->fp0));
+  raw_buffer = alloca (2 * mips_float_regsize (gdbarch, frame));
 
   fprintf_filtered (file, "%s:", gdbarch_register_name (gdbarch, regnum));
   fprintf_filtered (file, "%*s",
 		    4 - (int) strlen (gdbarch_register_name (gdbarch, regnum)),
 		    "");
 
-  if (register_size (gdbarch, regnum) == 4 || mips2_fp_compat (frame))
+  if (mips_float_regsize (gdbarch, frame) == 4)
     {
       /* 4-byte registers: Print hex and floating.  Also print even
          numbered registers as doubles.  */
@@ -4346,7 +4370,7 @@
   gdb_byte raw_buffer[MAX_REGISTER_SIZE];
   int offset;
 
-  if (TYPE_CODE (register_type (gdbarch, regnum)) == TYPE_CODE_FLT)
+  if (mips_float_register_p (gdbarch, regnum))
     {
       mips_print_fp_register (file, frame, regnum);
       return;
@@ -4417,8 +4441,7 @@
     {
       if (*gdbarch_register_name (gdbarch, regnum) == '\0')
 	continue;		/* unused register */
-      if (TYPE_CODE (register_type (gdbarch, regnum)) ==
-	  TYPE_CODE_FLT)
+      if (mips_float_register_p (gdbarch, regnum))
 	break;			/* end the row: reached FP register */
       /* Large registers are handled separately.  */
       if (register_size (gdbarch, regnum) > mips_abi_regsize (gdbarch))
@@ -4457,8 +4480,7 @@
     {
       if (*gdbarch_register_name (gdbarch, regnum) == '\0')
 	continue;		/* unused register */
-      if (TYPE_CODE (register_type (gdbarch, regnum)) ==
-	  TYPE_CODE_FLT)
+      if (mips_float_register_p (gdbarch, regnum))
 	break;			/* end row: reached FP register */
       if (register_size (gdbarch, regnum) > mips_abi_regsize (gdbarch))
 	break;			/* End row: large register.  */
@@ -4513,8 +4535,7 @@
       while (regnum < gdbarch_num_regs (gdbarch)
 		      + gdbarch_num_pseudo_regs (gdbarch))
 	{
-	  if (TYPE_CODE (register_type (gdbarch, regnum)) ==
-	      TYPE_CODE_FLT)
+	  if (mips_float_register_p (gdbarch, regnum))
 	    {
 	      if (all)		/* true for "INFO ALL-REGISTERS" command */
 		regnum = print_fp_register_row (file, frame, regnum);

Re: mips-tdep.c: Unification of FPR size detection

[Daniel Jacobowitz]

On Wed, Nov 21, 2007 at 05:40:28PM +0000, Maciej W. Rozycki wrote:
>  I think the changes are mostly obvious.  If not, then please enquire.  
> Modifications to mips_o32_return_value(), mips_o32_push_dummy_call() and 
> mips_register_type() change the semantics somewhat, but this is correct as 
> for 64-bit processors in the compatibility mode FPRs are seen as 32-bit.  
> The rest just shuffles code for consistency.

I'll take your word for this part... I find the various FP modes confusing.

>  One note: the mips2_fp_compat() function is meant to detect the size of 
> floating point registers based on the setting of the cp0.Status.FR bit as 
> applicable.  However the function currently always returns 0.  So I 
> decided to include a change that ifdefs out the entire body.  Enabling the 
> body does not trigger any new regressions.  I also added a note on a 
> problem with the frame being NULL -- it may have to be dealt with in a 
> better way once this code is reenabled.  A possible way is by caching the 
> value of cp0.Status.FR elsewhere.

This part does not make sense to me and it suggests that the patch is
wrong.  In general, if you have a function that wants something to do
with a frame and there isn't a frame available you have a design
problem.

The target's registers are a certain size.  That's independent of the
FR bit, which should affect only what we put into the registers and
how we interpret the results.  We can not change the type of registers
during execution; the regcache caches the types and sizes.  Anything
dependent on the FR bit can change during execution, so it can not be
a property of the gdbarch (which the return value of
mips_register_type is).  It should be possible to make the types of
the pseudo-register view change based on FR, though it might upset
other bits of GDB; I'm not sure without trying it.  But the raw
register has to stay fixed.

So, which is mips_float_regsize supposed to be?  The hardware
register size, in which case it should not call mips2_fp_compat,
or the software register size?

Maybe we should rip mips2_fp_compat out entirely.  We'd need yet
a third layer of registers since we already have one for the ABI.


-- 
Daniel Jacobowitz
CodeSourcery

Re: mips-tdep.c: Unification of FPR size detection

[Maciej W. Rozycki]

On Sun, 16 Dec 2007, Daniel Jacobowitz wrote:

> I'll take your word for this part... I find the various FP modes confusing.

 Well, no surprise here and the addition of the 64-bit mode to MIPS32r2 
has not made the view any clearer...

> This part does not make sense to me and it suggests that the patch is
> wrong.  In general, if you have a function that wants something to do
> with a frame and there isn't a frame available you have a design
> problem.

 Well, I do not insist on getting through to the cp0.Status register 
through the frame.  It is just that with the recent removal of the 
globally accessible "current_regcache" variable, you cannot get at the 
register file otherwise than through the frame.  I am looking forward to 
suggestions as to how to access the register bypassing the frame 
infrastructure.

 Actually my long-term idea was to read the register when available and 
cache the value of the flag in the target code for cases when it is not 
accessible through the regular register cache.

> The target's registers are a certain size.  That's independent of the
> FR bit, which should affect only what we put into the registers and
> how we interpret the results.  We can not change the type of registers
> during execution; the regcache caches the types and sizes.  Anything
> dependent on the FR bit can change during execution, so it can not be
> a property of the gdbarch (which the return value of
> mips_register_type is).  It should be possible to make the types of
> the pseudo-register view change based on FR, though it might upset
> other bits of GDB; I'm not sure without trying it.  But the raw
> register has to stay fixed.

 That is actually incorrect.  We do not have to know how it is implemented 
internally in a given CPU and I am told it may indeed vary, but in 
principle cp0.Status.FR does actually change the width of raw floating 
point registers.

 With the bit set all the FPU registers fp0 through to fp31 are 64-bit 
wide.  All the double FP operations are performed on single registers.  
All the single FP operations are performed on single registers.

 With the bit clear all the FPU registers fp0 through to fp31 are 32-bit 
wide.  All the double FP operations are performed on consecutive odd/even 
pairs of registers.  All the single FP operations are performed on single 
registers.  Note that there is a difference here between MIPS Architecture 
and legacy MIPS processors.  For the formers all the registers may be used 
in this mode with single FP operations.  For the latters only 
even-numbered registers are permitted.

> So, which is mips_float_regsize supposed to be?  The hardware
> register size, in which case it should not call mips2_fp_compat,
> or the software register size?

 It is the raw hardware register size and as explained above we do want to 
check mips2_fp_compat().  I have a patch in the queue that makes cooked FP 
registers always be 64-bit though.

> Maybe we should rip mips2_fp_compat out entirely.  We'd need yet
> a third layer of registers since we already have one for the ABI.

 There may be no ABI.  GDB may just be connected to a running target, 
through EJTAG for example, with no executable selected.  In this case the 
only source of information of the FPU configuration is hardware.  I used 
it this way in the past.  And in particular, when debugging the Linux 
kernel, it may change the setting of cp0.Status.FR under your feet, so 
whenever execution stops, the width of the FP register stack has to be 
updated following the state of the bit.

  Maciej

Re: mips-tdep.c: Unification of FPR size detection

[Daniel Jacobowitz]

On Wed, Dec 19, 2007 at 05:07:42PM +0000, Maciej W. Rozycki wrote:
> > This part does not make sense to me and it suggests that the patch is
> > wrong.  In general, if you have a function that wants something to do
> > with a frame and there isn't a frame available you have a design
> > problem.
> 
>  Well, I do not insist on getting through to the cp0.Status register 
> through the frame.  It is just that with the recent removal of the 
> globally accessible "current_regcache" variable, you cannot get at the 
> register file otherwise than through the frame.  I am looking forward to 
> suggestions as to how to access the register bypassing the frame 
> infrastructure.

No, that's the exact opposite of what I meant.  If you don't have a
frame available, then you have to assume the target essentially has no
registers (yet, or any longer, or...).  So we can not make any
decisions based on their values.  If you're caching the value,
it seems suspiciously to me like guessing an arbitrary wrong value,
maybe even from a previous session.

> > The target's registers are a certain size.  That's independent of the
> > FR bit, which should affect only what we put into the registers and
> > how we interpret the results.  We can not change the type of registers
> > during execution; the regcache caches the types and sizes.  Anything
> > dependent on the FR bit can change during execution, so it can not be
> > a property of the gdbarch (which the return value of
> > mips_register_type is).  It should be possible to make the types of
> > the pseudo-register view change based on FR, though it might upset
> > other bits of GDB; I'm not sure without trying it.  But the raw
> > register has to stay fixed.
> 
>  That is actually incorrect.  We do not have to know how it is implemented 
> internally in a given CPU and I am told it may indeed vary, but in 
> principle cp0.Status.FR does actually change the width of raw floating 
> point registers.

Right.  But it doesn't change the size of the registers as presented
by a remote stub to GDB.  So, for our purposes I think the best model
is that the size of the raw FP registers does not change.  How can we
change the size of the raw registers while the stub is sending the
same bit patterns to GDB?

Something needs to change, sure, but I don't think this is it.

-- 
Daniel Jacobowitz
CodeSourcery

Re: mips-tdep.c: Unification of FPR size detection

[Maciej W. Rozycki]

On Wed, 19 Dec 2007, Daniel Jacobowitz wrote:

> No, that's the exact opposite of what I meant.  If you don't have a
> frame available, then you have to assume the target essentially has no
> registers (yet, or any longer, or...).  So we can not make any
> decisions based on their values.  If you're caching the value,
> it seems suspiciously to me like guessing an arbitrary wrong value,
> maybe even from a previous session.

 Well, for whatever reason a frame is commonly not available during a 
debugging session -- I think it gets invalidated at some point and the 
problem is in the process of being reestablished the size of the FP 
registers is being determined.  I can see if I can get a backtrace leading 
to this scenario.

 This patch in its current form assumes if no frame is available, then the 
width of the registers is irrelevant and because some value has to be 
provided it uses the native one.

> Right.  But it doesn't change the size of the registers as presented
> by a remote stub to GDB.  So, for our purposes I think the best model
> is that the size of the raw FP registers does not change.  How can we
> change the size of the raw registers while the stub is sending the
> same bit patterns to GDB?

 This sounds like a bug in the stub then -- probably an outcome from the 
semantics of the "ldc1/sdc1" instructions which it may use rather than 
exposing the actual topology of the register file.  It may have worked 
reasonably well for legacy MIPS processors, because odd-numbered FP 
registers were only usable as a high-part of double values, so the FPU 
could have been effectively treated as one having 16 64-bit FPRs split in 
an original way.

 It is misleading for current (MIPS rev.2) processors though as in the 
32-bit FPR mode all the registers are full-featured single FPRs and also 
the paired-single and fixed long type are not supported with pairs of 
registers even though the double type is.

 I am not sure that pretending the architecture of the FPU is different to 
what it actually is is going to gain us anything.

> Something needs to change, sure, but I don't think this is it.

 Any proposals then?  Either way cp0.Status.FR has to be checked -- either 
for the width of FPRs (64-bit vs 32-bit) as in the patch proposed or for 
the number of them (32 vs 16) if the width is pretended to be always 
64-bit.

  Maciej

Re: mips-tdep.c: Unification of FPR size detection

[Daniel Jacobowitz]

On Thu, Dec 20, 2007 at 04:57:11PM +0000, Maciej W. Rozycki wrote:
>  This patch in its current form assumes if no frame is available, then the 
> width of the registers is irrelevant and because some value has to be 
> provided it uses the native one.

The value you provide then (probably during regcache creation) will
stick around longer than you expect.

> > Right.  But it doesn't change the size of the registers as presented
> > by a remote stub to GDB.  So, for our purposes I think the best model
> > is that the size of the raw FP registers does not change.  How can we
> > change the size of the raw registers while the stub is sending the
> > same bit patterns to GDB?
> 
>  This sounds like a bug in the stub then -- probably an outcome from the 
> semantics of the "ldc1/sdc1" instructions which it may use rather than 
> exposing the actual topology of the register file.  It may have worked 
> reasonably well for legacy MIPS processors, because odd-numbered FP 
> registers were only usable as a high-part of double values, so the FPU 
> could have been effectively treated as one having 16 64-bit FPRs split in 
> an original way.

I'm not talking about the implementation of a stub; I'm talking about
the remote serial protocol and GDB<->stub communications.  One thing
we simply can not do is change the size of the registers in the middle
of a debugging session.  If we do, GDB will have no way to parse the
resulting 'g' packet and locate the status register to check FR.

Some background to be sure we're using the same terminology:

The MIPS port is currently organized with two sets of registers.  One
is the raw registers, those provided by hardware.  The other is the
ABI registers, those defined by the current ABI.  The raw registers
are always used to communicate with the target.  The ABI registers are
always used to communicate with the user.  $f0 is always an ABI
register and its size is always determined by the ABI.

Ideally, the target would define the raw registers and the gdbarch
would define the ABI registers.  In practice it's messier than that,
but that's how it's supposed to work.

You are trying to distinguish the raw registers, used for
communication, from the hardware registers, which effectively change
under software control.  The port is not prepared for that.  The
only mechanism we have to report different sizes for registers
is the creation of a new gdbarch; it seems like the two settings
of the FR bit should correspond to two different gdbarches.  But
selecting a gdbarch based on register values will be tricky.
Possible, though.

How do you detect, or plan to, when the register has changed?  Poll it
every time we stop the target?  Once we know how to detect changes,
that should let us determine when to select an appropriate
architecture.  I'd rather do it in some way that doesn't penalize
debugging performance too badly for existing targets.

Then there's the question of how to present this to the user.  Maybe
we only need to do it for some setting of "set mips abi".  Anything
else is going to involve changing the raw / ABI distinction that the
port is currently designed around.

-- 
Daniel Jacobowitz
CodeSourcery