diff mbox

Look for FIR in the last FreeBSD/mips floating-point register.

Message ID 20170531165803.50633-1-jhb@FreeBSD.org
State New
Headers show

Commit Message

John Baldwin May 31, 2017, 4:58 p.m. UTC
FreeBSD/mips kernels were recently changed to include the floating
point implementation revision register in the floating point register
set exported in process cores and via ptrace() (r318067).  This change
will first ship in FreeBSD 12.0 when it is eventually released.  The
space used to hold FIR was previously reserved in 'struct fpreg' as a
zero-filled dummy for padding, so 'struct fpreg' has not changed in
size.  On the one hand this means that there is not an easy way to detect
if if the FIR register is the zero-filled dummy or the true FIR value.
However, it also means that there is no need to deal with multiple
layouts of 'struct fpreg'.  I've chosen to always treat the last register
in 'struct fpreg' as the FIR.  This means that process cores and
ptrace() on older kernels will report a FIR value of 0.  However, FreeBSD
doesn't currently ship a release image for FreeBSD/mips, and releases
prior to 12.0 assume soft-float, so I think this is a reasonable tradeoff
for simplicity.

gdb/ChangeLog:

	* mips-fbsd-nat.c (getfpregs_supplies): Return true for FIR.
	* mips-fbsd-tdep.c (mips_fbsd_supply_fpregs): Split supply of FSR
	out of loop and add supply of FIR.
	(mips_fbsd_collect_fpregs): Split collect of FSR out of loop and
	add collect of FIR.
---
 gdb/ChangeLog        |  8 ++++++++
 gdb/mips-fbsd-nat.c  |  2 +-
 gdb/mips-fbsd-tdep.c | 41 ++++++++++++++++++++++++++++-------------
 3 files changed, 37 insertions(+), 14 deletions(-)

Comments

Simon Marchi June 5, 2017, 8:27 p.m. UTC | #1
On 2017-05-31 18:58, John Baldwin wrote:
> FreeBSD/mips kernels were recently changed to include the floating
> point implementation revision register in the floating point register
> set exported in process cores and via ptrace() (r318067).  This change
> will first ship in FreeBSD 12.0 when it is eventually released.  The
> space used to hold FIR was previously reserved in 'struct fpreg' as a
> zero-filled dummy for padding, so 'struct fpreg' has not changed in
> size.  On the one hand this means that there is not an easy way to 
> detect
> if if the FIR register is the zero-filled dummy or the true FIR value.
> However, it also means that there is no need to deal with multiple
> layouts of 'struct fpreg'.  I've chosen to always treat the last 
> register
> in 'struct fpreg' as the FIR.  This means that process cores and
> ptrace() on older kernels will report a FIR value of 0.  However, 
> FreeBSD
> doesn't currently ship a release image for FreeBSD/mips, and releases
> prior to 12.0 assume soft-float, so I think this is a reasonable 
> tradeoff
> for simplicity.

The patch LGTM, I just have one question down there, really out of 
curiosity.

> gdb/ChangeLog:
> 
> 	* mips-fbsd-nat.c (getfpregs_supplies): Return true for FIR.
> 	* mips-fbsd-tdep.c (mips_fbsd_supply_fpregs): Split supply of FSR
> 	out of loop and add supply of FIR.
> 	(mips_fbsd_collect_fpregs): Split collect of FSR out of loop and
> 	add collect of FIR.
> ---
>  gdb/ChangeLog        |  8 ++++++++
>  gdb/mips-fbsd-nat.c  |  2 +-
>  gdb/mips-fbsd-tdep.c | 41 ++++++++++++++++++++++++++++-------------
>  3 files changed, 37 insertions(+), 14 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 263d57ff66..0e2a7b5833 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,11 @@
> +2017-05-31  John Baldwin  <jhb@FreeBSD.org>
> +
> +	* mips-fbsd-nat.c (getfpregs_supplies): Return true for FIR.
> +	* mips-fbsd-tdep.c (mips_fbsd_supply_fpregs): Split supply of FSR
> +	out of loop and add supply of FIR.
> +	(mips_fbsd_collect_fpregs): Split collect of FSR out of loop and
> +	add collect of FIR.
> +
>  2017-05-31  Simon Marchi  <simon.marchi@ericsson.com>
> 
>  	* memattr.c (mem_info_command): Rename to ...
> diff --git a/gdb/mips-fbsd-nat.c b/gdb/mips-fbsd-nat.c
> index 53817d7cd7..c381186535 100644
> --- a/gdb/mips-fbsd-nat.c
> +++ b/gdb/mips-fbsd-nat.c
> @@ -46,7 +46,7 @@ static bool
>  getfpregs_supplies (struct gdbarch *gdbarch, int regnum)
>  {
>    return (regnum >= mips_regnum (gdbarch)->fp0
> -	  && regnum < mips_regnum (gdbarch)->fp_implementation_revision);
> +	  && regnum <= mips_regnum (gdbarch)->fp_implementation_revision);
>  }
> 
>  /* Fetch register REGNUM from the inferior.  If REGNUM is -1, do this
> diff --git a/gdb/mips-fbsd-tdep.c b/gdb/mips-fbsd-tdep.c
> index 44b960d4d2..34a19b0ead 100644
> --- a/gdb/mips-fbsd-tdep.c
> +++ b/gdb/mips-fbsd-tdep.c
> @@ -39,7 +39,8 @@
> 
>  /* Number of registers in `struct fpreg' from <machine/reg.h>.  The
>     first 32 hold floating point registers.  33 holds the FSR.  The
> -   34th is a dummy for padding.  */
> +   34th holds FIR on FreeBSD 12.0 and newer kernels.  On older kernels
> +   it was a zero-filled dummy for padding.  */
>  #define MIPS_FBSD_NUM_FPREGS	34
> 
>  /* Supply a single register.  The register size might not match, so 
> use
> @@ -72,14 +73,21 @@ mips_fbsd_supply_fpregs (struct regcache
> *regcache, int regnum,
>  {
>    struct gdbarch *gdbarch = get_regcache_arch (regcache);
>    const gdb_byte *regs = (const gdb_byte *) fpregs;
> -  int i, fp0num, fsrnum;
> +  int i, fp0num;
> 
>    fp0num = mips_regnum (gdbarch)->fp0;
> -  fsrnum = mips_regnum (gdbarch)->fp_control_status;
> -  for (i = fp0num; i <= fsrnum; i++)
> -    if (regnum == i || regnum == -1)
> -      mips_fbsd_supply_reg (regcache, i,
> -			    regs + (i - fp0num) * regsize, regsize);
> +  for (i = 0; i <= 32; i++)
> +    if (regnum == fp0num + i || regnum == -1)
> +      mips_fbsd_supply_reg (regcache, fp0num + i,
> +			    regs + i * regsize, regsize);
> +  if (regnum == mips_regnum (gdbarch)->fp_control_status || regnum == 
> -1)
> +    mips_fbsd_supply_reg (regcache, mips_regnum 
> (gdbarch)->fp_control_status,
> +			  regs + 32 * regsize, regsize);
> +  if (regnum == mips_regnum (gdbarch)->fp_implementation_revision
> +      || regnum == -1)
> +    mips_fbsd_supply_reg (regcache,
> +			  mips_regnum (gdbarch)->fp_implementation_revision,
> +			  regs + 33 * regsize, regsize);
>  }
> 
>  /* Supply the general-purpose registers stored in GREGS to REGCACHE.
> @@ -109,14 +117,21 @@ mips_fbsd_collect_fpregs (const struct regcache
> *regcache, int regnum,
>  {
>    struct gdbarch *gdbarch = get_regcache_arch (regcache);
>    gdb_byte *regs = (gdb_byte *) fpregs;
> -  int i, fp0num, fsrnum;
> +  int i, fp0num;
> 
>    fp0num = mips_regnum (gdbarch)->fp0;
> -  fsrnum = mips_regnum (gdbarch)->fp_control_status;
> -  for (i = fp0num; i <= fsrnum; i++)
> -    if (regnum == i || regnum == -1)
> -      mips_fbsd_collect_reg (regcache, i,
> -			     regs + (i - fp0num) * regsize, regsize);
> +  for (i = 0; i < 32; i++)
> +    if (regnum == fp0num + i || regnum == -1)
> +      mips_fbsd_collect_reg (regcache, fp0num + i,
> +			     regs + i * regsize, regsize);
> +  if (regnum == mips_regnum (gdbarch)->fp_control_status || regnum == 
> -1)
> +    mips_fbsd_collect_reg (regcache, mips_regnum 
> (gdbarch)->fp_control_status,
> +			   regs + 32 * regsize, regsize);
> +  if (regnum == mips_regnum (gdbarch)->fp_implementation_revision
> +      || regnum == -1)
> +    mips_fbsd_collect_reg (regcache,
> +			   mips_regnum (gdbarch)->fp_implementation_revision,
> +			   regs + 33 * regsize, regsize);

I'm just curious to know why you changed this.  They previous code 
assumed that the 32 floating-point registers and the control register 
were contiguous their numbering.  Is this no longer true with the new 
register, or is it just for clarity that you separately handle the 
different kinds of registers?

>  }
> 
>  /* Collect the general-purpose registers from REGCACHE and store them

Thanks,

Simon
John Baldwin June 6, 2017, 5:46 p.m. UTC | #2
On Monday, June 05, 2017 10:27:48 PM Simon Marchi wrote:
> On 2017-05-31 18:58, John Baldwin wrote:
> > +  for (i = 0; i < 32; i++)
> > +    if (regnum == fp0num + i || regnum == -1)
> > +      mips_fbsd_collect_reg (regcache, fp0num + i,
> > +			     regs + i * regsize, regsize);
> > +  if (regnum == mips_regnum (gdbarch)->fp_control_status || regnum == 
> > -1)
> > +    mips_fbsd_collect_reg (regcache, mips_regnum 
> > (gdbarch)->fp_control_status,
> > +			   regs + 32 * regsize, regsize);
> > +  if (regnum == mips_regnum (gdbarch)->fp_implementation_revision
> > +      || regnum == -1)
> > +    mips_fbsd_collect_reg (regcache,
> > +			   mips_regnum (gdbarch)->fp_implementation_revision,
> > +			   regs + 33 * regsize, regsize);
> 
> I'm just curious to know why you changed this.  They previous code 
> assumed that the 32 floating-point registers and the control register 
> were contiguous their numbering.  Is this no longer true with the new 
> register, or is it just for clarity that you separately handle the 
> different kinds of registers?

Just for clarity.  mips-tdep.c does happen to layout the registers such
that fsr and fir are consecutive after fp31, but fsr and fir do have
separate entries in the mips_regnum structure.
Maciej W. Rozycki June 8, 2017, 7:15 a.m. UTC | #3
On Wed, 31 May 2017, John Baldwin wrote:

> FreeBSD/mips kernels were recently changed to include the floating
> point implementation revision register in the floating point register
> set exported in process cores and via ptrace() (r318067).  This change
> will first ship in FreeBSD 12.0 when it is eventually released.  The
> space used to hold FIR was previously reserved in 'struct fpreg' as a
> zero-filled dummy for padding, so 'struct fpreg' has not changed in
> size.  On the one hand this means that there is not an easy way to detect
> if if the FIR register is the zero-filled dummy or the true FIR value.

 Well, CP1.FIR is generally expected to hold non zero; in particular in 
legacy MIPS processors (before CP0.Config1.FP was defined) checking for a 
non-zero value in CP1.FIR (bits 15:8 specifically) was the recommended way 
to detect the presence of FPU hardware[1].  And from MIPSr1 on there have 
to be floating-point formats supported reported in CP1.FIR, with D and S 
being mandatory, so you'll see non-zero bits at least in their positions 
(the W bit was only added with MIPSr2).

 You'll only ever see zero in CP1.FIR with some broken simulators, which 
therefore cannot be relied on being a correct architecture implementation 
anyway.

> However, it also means that there is no need to deal with multiple
> layouts of 'struct fpreg'.  I've chosen to always treat the last register
> in 'struct fpreg' as the FIR.  This means that process cores and
> ptrace() on older kernels will report a FIR value of 0.  However, FreeBSD
> doesn't currently ship a release image for FreeBSD/mips, and releases
> prior to 12.0 assume soft-float, so I think this is a reasonable tradeoff
> for simplicity.

 You could also exclude a zero CP1.FIR from the view at the GDB side, 
pretty much how e.g. support for the optional DSP registers has been 
implemented.  As noted above the setting of zero bears no value anyway.

References:

[1] "IDT MIPS Microprocessor Family Software Reference Manual", Version 
    2.0, Integrated Device Technology, Inc., October 1996, Chapter 8 
    "Floating Point Co-Processor", p. 8-9

  Maciej
John Baldwin June 12, 2017, 6:47 p.m. UTC | #4
On Thursday, June 08, 2017 08:15:39 AM Maciej W. Rozycki wrote:
> On Wed, 31 May 2017, John Baldwin wrote:
> 
> > FreeBSD/mips kernels were recently changed to include the floating
> > point implementation revision register in the floating point register
> > set exported in process cores and via ptrace() (r318067).  This change
> > will first ship in FreeBSD 12.0 when it is eventually released.  The
> > space used to hold FIR was previously reserved in 'struct fpreg' as a
> > zero-filled dummy for padding, so 'struct fpreg' has not changed in
> > size.  On the one hand this means that there is not an easy way to detect
> > if if the FIR register is the zero-filled dummy or the true FIR value.
> 
>  Well, CP1.FIR is generally expected to hold non zero; in particular in 
> legacy MIPS processors (before CP0.Config1.FP was defined) checking for a 
> non-zero value in CP1.FIR (bits 15:8 specifically) was the recommended way 
> to detect the presence of FPU hardware[1].  And from MIPSr1 on there have 
> to be floating-point formats supported reported in CP1.FIR, with D and S 
> being mandatory, so you'll see non-zero bits at least in their positions 
> (the W bit was only added with MIPSr2).

Ah, I had been going off of my (probably stale) copy of See Mips Run which
only talks about comparing FIR with 0.  FreeBSD requires MIPSr3, so it should
always see a non-zero FIR then.

> > However, it also means that there is no need to deal with multiple
> > layouts of 'struct fpreg'.  I've chosen to always treat the last register
> > in 'struct fpreg' as the FIR.  This means that process cores and
> > ptrace() on older kernels will report a FIR value of 0.  However, FreeBSD
> > doesn't currently ship a release image for FreeBSD/mips, and releases
> > prior to 12.0 assume soft-float, so I think this is a reasonable tradeoff
> > for simplicity.
> 
>  You could also exclude a zero CP1.FIR from the view at the GDB side, 
> pretty much how e.g. support for the optional DSP registers has been 
> implemented.  As noted above the setting of zero bears no value anyway.

It ends up being a one-line change to just exclude a zero FIR, so I can do that.
Maciej W. Rozycki June 12, 2017, 8:23 p.m. UTC | #5
On Mon, 12 Jun 2017, John Baldwin wrote:

> >  Well, CP1.FIR is generally expected to hold non zero; in particular in 
> > legacy MIPS processors (before CP0.Config1.FP was defined) checking for a 
> > non-zero value in CP1.FIR (bits 15:8 specifically) was the recommended way 
> > to detect the presence of FPU hardware[1].  And from MIPSr1 on there have 
> > to be floating-point formats supported reported in CP1.FIR, with D and S 
> > being mandatory, so you'll see non-zero bits at least in their positions 
> > (the W bit was only added with MIPSr2).
> 
> Ah, I had been going off of my (probably stale) copy of See Mips Run which
> only talks about comparing FIR with 0.  FreeBSD requires MIPSr3, so it should
> always see a non-zero FIR then.

 FAOD MIPSr3 (as in MIPS32r3/MIPS64r3) or MIPS III?

 I'm asking as I find it odd for anything to require MIPSr3 and not to 
handle MIPSr2 given that the main difference between the two is microMIPS 
and 2008-NaN support, both optional.

  Maciej
John Baldwin June 13, 2017, 4:37 p.m. UTC | #6
On Monday, June 12, 2017 09:23:22 PM Maciej W. Rozycki wrote:
> On Mon, 12 Jun 2017, John Baldwin wrote:
> 
> > >  Well, CP1.FIR is generally expected to hold non zero; in particular in 
> > > legacy MIPS processors (before CP0.Config1.FP was defined) checking for a 
> > > non-zero value in CP1.FIR (bits 15:8 specifically) was the recommended way 
> > > to detect the presence of FPU hardware[1].  And from MIPSr1 on there have 
> > > to be floating-point formats supported reported in CP1.FIR, with D and S 
> > > being mandatory, so you'll see non-zero bits at least in their positions 
> > > (the W bit was only added with MIPSr2).
> > 
> > Ah, I had been going off of my (probably stale) copy of See Mips Run which
> > only talks about comparing FIR with 0.  FreeBSD requires MIPSr3, so it should
> > always see a non-zero FIR then.
> 
>  FAOD MIPSr3 (as in MIPS32r3/MIPS64r3) or MIPS III?

Sorry, MIPS III only (so now I'm not certain if a non-zero FIR is guaranteed).
The only mips bits I have worked with myself are either qemu's malta targets
and a mips4000-ish 64-bit research CPU (all of which have FPUs).
diff mbox

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 263d57ff66..0e2a7b5833 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@ 
+2017-05-31  John Baldwin  <jhb@FreeBSD.org>
+
+	* mips-fbsd-nat.c (getfpregs_supplies): Return true for FIR.
+	* mips-fbsd-tdep.c (mips_fbsd_supply_fpregs): Split supply of FSR
+	out of loop and add supply of FIR.
+	(mips_fbsd_collect_fpregs): Split collect of FSR out of loop and
+	add collect of FIR.
+
 2017-05-31  Simon Marchi  <simon.marchi@ericsson.com>
 
 	* memattr.c (mem_info_command): Rename to ...
diff --git a/gdb/mips-fbsd-nat.c b/gdb/mips-fbsd-nat.c
index 53817d7cd7..c381186535 100644
--- a/gdb/mips-fbsd-nat.c
+++ b/gdb/mips-fbsd-nat.c
@@ -46,7 +46,7 @@  static bool
 getfpregs_supplies (struct gdbarch *gdbarch, int regnum)
 {
   return (regnum >= mips_regnum (gdbarch)->fp0
-	  && regnum < mips_regnum (gdbarch)->fp_implementation_revision);
+	  && regnum <= mips_regnum (gdbarch)->fp_implementation_revision);
 }
 
 /* Fetch register REGNUM from the inferior.  If REGNUM is -1, do this
diff --git a/gdb/mips-fbsd-tdep.c b/gdb/mips-fbsd-tdep.c
index 44b960d4d2..34a19b0ead 100644
--- a/gdb/mips-fbsd-tdep.c
+++ b/gdb/mips-fbsd-tdep.c
@@ -39,7 +39,8 @@ 
 
 /* Number of registers in `struct fpreg' from <machine/reg.h>.  The
    first 32 hold floating point registers.  33 holds the FSR.  The
-   34th is a dummy for padding.  */
+   34th holds FIR on FreeBSD 12.0 and newer kernels.  On older kernels
+   it was a zero-filled dummy for padding.  */
 #define MIPS_FBSD_NUM_FPREGS	34
 
 /* Supply a single register.  The register size might not match, so use
@@ -72,14 +73,21 @@  mips_fbsd_supply_fpregs (struct regcache *regcache, int regnum,
 {
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
   const gdb_byte *regs = (const gdb_byte *) fpregs;
-  int i, fp0num, fsrnum;
+  int i, fp0num;
 
   fp0num = mips_regnum (gdbarch)->fp0;
-  fsrnum = mips_regnum (gdbarch)->fp_control_status;
-  for (i = fp0num; i <= fsrnum; i++)
-    if (regnum == i || regnum == -1)
-      mips_fbsd_supply_reg (regcache, i,
-			    regs + (i - fp0num) * regsize, regsize);
+  for (i = 0; i <= 32; i++)
+    if (regnum == fp0num + i || regnum == -1)
+      mips_fbsd_supply_reg (regcache, fp0num + i,
+			    regs + i * regsize, regsize);
+  if (regnum == mips_regnum (gdbarch)->fp_control_status || regnum == -1)
+    mips_fbsd_supply_reg (regcache, mips_regnum (gdbarch)->fp_control_status,
+			  regs + 32 * regsize, regsize);
+  if (regnum == mips_regnum (gdbarch)->fp_implementation_revision
+      || regnum == -1)
+    mips_fbsd_supply_reg (regcache,
+			  mips_regnum (gdbarch)->fp_implementation_revision,
+			  regs + 33 * regsize, regsize);
 }
 
 /* Supply the general-purpose registers stored in GREGS to REGCACHE.
@@ -109,14 +117,21 @@  mips_fbsd_collect_fpregs (const struct regcache *regcache, int regnum,
 {
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
   gdb_byte *regs = (gdb_byte *) fpregs;
-  int i, fp0num, fsrnum;
+  int i, fp0num;
 
   fp0num = mips_regnum (gdbarch)->fp0;
-  fsrnum = mips_regnum (gdbarch)->fp_control_status;
-  for (i = fp0num; i <= fsrnum; i++)
-    if (regnum == i || regnum == -1)
-      mips_fbsd_collect_reg (regcache, i,
-			     regs + (i - fp0num) * regsize, regsize);
+  for (i = 0; i < 32; i++)
+    if (regnum == fp0num + i || regnum == -1)
+      mips_fbsd_collect_reg (regcache, fp0num + i,
+			     regs + i * regsize, regsize);
+  if (regnum == mips_regnum (gdbarch)->fp_control_status || regnum == -1)
+    mips_fbsd_collect_reg (regcache, mips_regnum (gdbarch)->fp_control_status,
+			   regs + 32 * regsize, regsize);
+  if (regnum == mips_regnum (gdbarch)->fp_implementation_revision
+      || regnum == -1)
+    mips_fbsd_collect_reg (regcache,
+			   mips_regnum (gdbarch)->fp_implementation_revision,
+			   regs + 33 * regsize, regsize);
 }
 
 /* Collect the general-purpose registers from REGCACHE and store them