[PATCH 0/4] Cleanups for MIPS registers (mostly FreeBSD-specific)

[PATCH 0/4] Cleanups for MIPS registers (mostly FreeBSD-specific)

[John Baldwin]

Various small fixes to working with registers on FreeBSD/mips as well as
one OS-agnostic fix to handle unavailable registers more gracefully in
'info registers'.  Note that for the 32-bit info registers there isn't
room in the table layout for the full "<unavailable>" string, so a
truncated string "<unavail" is output instead.  I originally tried to
use "<unavail>" but it is one character too wide for 32-bit (8 hex
characters per register).

John Baldwin (4):
  Cleanups to FreeBSD/mips native register operations.
  Use mips_regnum instead of constants for FreeBSD/mips register
    operations.
  Consistently use fprintf_filtered when displaying MIPS registers.
  Don't throw an error in 'info registers' for unavailable MIPS GP
    registers.

 gdb/ChangeLog        | 28 ++++++++++++++++++++++++++++
 gdb/mips-fbsd-nat.c  | 26 ++++++++++++++------------
 gdb/mips-fbsd-tdep.c | 29 ++++++++++++++++-------------
 gdb/mips-tdep.c      | 12 +++++++++---
 4 files changed, 67 insertions(+), 28 deletions(-)

-- 
2.11.0

[PATCH 1/4] Cleanups to FreeBSD/mips native register operations.

[John Baldwin]

Compare against the "raw" PC register number instead of the cooked
register number when determining if a register was handled by
PT_GETREGS.  Previously the register fetch/store operations only tried
PT_GETREGS to fetch any individual register.  The result was that
fetching or storing an individual register not covered by PT_GETREGS
(such as floating point registers) did not work.

While here, remove an early exit to simplify the code flow from the
PT_GETREGS / PT_SETREGS case, and add a getfpregs_supplies similar to
getregs_supplies to describe the registers supplied by PT_GETFPREGS
and PT_SETFPREGS.

gdb/ChangeLog:

	* mips-fbsd-nat.c (getregs_supplies): Fix upper bound comparison.
	(getpfpregs_supplies): New function.
	(mips_fbsd_fetch_inferior_registers): Remove early exit and use
	getfpregs_supplies.
	(mips_fbsd_store_inferior_registers): Likewise.
---
 gdb/ChangeLog       |  8 ++++++++
 gdb/mips-fbsd-nat.c | 26 ++++++++++++++------------
 2 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7fcfebbeb7..69c3efa317 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2017-04-11  John Baldwin  <jhb@FreeBSD.org>
+
+	* mips-fbsd-nat.c (getregs_supplies): Fix upper bound comparison.
+	(getpfpregs_supplies): New function.
+	(mips_fbsd_fetch_inferior_registers): Remove early exit and use
+	getfpregs_supplies.
+	(mips_fbsd_store_inferior_registers): Likewise.
+
 2017-04-04  John Baldwin  <jhb@FreeBSD.org>
 
 	* amd64-fbsd-tdep.c: Remove "bsd-uthread.h" include.
diff --git a/gdb/mips-fbsd-nat.c b/gdb/mips-fbsd-nat.c
index 078df52db6..e2ed63e829 100644
--- a/gdb/mips-fbsd-nat.c
+++ b/gdb/mips-fbsd-nat.c
@@ -37,7 +37,16 @@ static bool
 getregs_supplies (struct gdbarch *gdbarch, int regnum)
 {
   return (regnum >= MIPS_ZERO_REGNUM
-	  && regnum <= gdbarch_pc_regnum (gdbarch));
+	  && regnum <= mips_regnum (gdbarch)->pc);
+}
+
+/* Determine if PT_GETFPREGS fetches this register.  */
+
+static bool
+getfpregs_supplies (struct gdbarch *gdbarch, int regnum)
+{
+  return (regnum >= mips_regnum (gdbarch)->fp0
+	  && regnum < mips_regnum (gdbarch)->fp_implementation_revision);
 }
 
 /* Fetch register REGNUM from the inferior.  If REGNUM is -1, do this
@@ -47,9 +56,9 @@ static void
 mips_fbsd_fetch_inferior_registers (struct target_ops *ops,
 				    struct regcache *regcache, int regnum)
 {
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
   pid_t pid = get_ptrace_pid (regcache_get_ptid (regcache));
 
-  struct gdbarch *gdbarch = get_regcache_arch (regcache);
   if (regnum == -1 || getregs_supplies (gdbarch, regnum))
     {
       struct reg regs;
@@ -58,12 +67,9 @@ mips_fbsd_fetch_inferior_registers (struct target_ops *ops,
 	perror_with_name (_("Couldn't get registers"));
 
       mips_fbsd_supply_gregs (regcache, regnum, &regs, sizeof (register_t));
-      if (regnum != -1)
-	return;
     }
 
-  if (regnum == -1
-      || regnum >= gdbarch_fp0_regnum (get_regcache_arch (regcache)))
+  if (regnum == -1 || getfpregs_supplies (gdbarch, regnum))
     {
       struct fpreg fpregs;
 
@@ -82,9 +88,9 @@ static void
 mips_fbsd_store_inferior_registers (struct target_ops *ops,
 				    struct regcache *regcache, int regnum)
 {
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
   pid_t pid = get_ptrace_pid (regcache_get_ptid (regcache));
 
-  struct gdbarch *gdbarch = get_regcache_arch (regcache);
   if (regnum == -1 || getregs_supplies (gdbarch, regnum))
     {
       struct reg regs;
@@ -97,13 +103,9 @@ mips_fbsd_store_inferior_registers (struct target_ops *ops,
 
       if (ptrace (PT_SETREGS, pid, (PTRACE_TYPE_ARG3) &regs, 0) == -1)
 	perror_with_name (_("Couldn't write registers"));
-
-      if (regnum != -1)
-	return;
     }
 
-  if (regnum == -1
-      || regnum >= gdbarch_fp0_regnum (get_regcache_arch (regcache)))
+  if (regnum == -1 || getfpregs_supplies (gdbarch, regnum))
     {
       struct fpreg fpregs;
 
-- 
2.11.0

Re: [PATCH 1/4] Cleanups to FreeBSD/mips native register operations.

[Luis Machado]

On 04/12/2017 01:37 PM, John Baldwin wrote:
> Compare against the "raw" PC register number instead of the cooked
> register number when determining if a register was handled by
> PT_GETREGS.  Previously the register fetch/store operations only tried
> PT_GETREGS to fetch any individual register.  The result was that
> fetching or storing an individual register not covered by PT_GETREGS
> (such as floating point registers) did not work.
>
> While here, remove an early exit to simplify the code flow from the
> PT_GETREGS / PT_SETREGS case, and add a getfpregs_supplies similar to
> getregs_supplies to describe the registers supplied by PT_GETFPREGS
> and PT_SETFPREGS.
>
> gdb/ChangeLog:
>
> 	* mips-fbsd-nat.c (getregs_supplies): Fix upper bound comparison.
> 	(getpfpregs_supplies): New function.
> 	(mips_fbsd_fetch_inferior_registers): Remove early exit and use
> 	getfpregs_supplies.
> 	(mips_fbsd_store_inferior_registers): Likewise.
> ---
>  gdb/ChangeLog       |  8 ++++++++
>  gdb/mips-fbsd-nat.c | 26 ++++++++++++++------------
>  2 files changed, 22 insertions(+), 12 deletions(-)

Only a few nits.

> diff --git a/gdb/mips-fbsd-nat.c b/gdb/mips-fbsd-nat.c
> index 078df52db6..e2ed63e829 100644
> --- a/gdb/mips-fbsd-nat.c
> +++ b/gdb/mips-fbsd-nat.c
> @@ -37,7 +37,16 @@ static bool
>  getregs_supplies (struct gdbarch *gdbarch, int regnum)
>  {
>    return (regnum >= MIPS_ZERO_REGNUM
> -	  && regnum <= gdbarch_pc_regnum (gdbarch));
> +	  && regnum <= mips_regnum (gdbarch)->pc);
> +}

Can the BSD backend override the pc value in gdbarch_pc_regnum (...) so 
it fits what is expected? Or is this a case where the cooked pc register 
number is still useful and we need to handle things differently for the 
raw pc register number?

> +
> +/* Determine if PT_GETFPREGS fetches this register.  */

Pedantically, "... fetches REGNUM".

> @@ -47,9 +56,9 @@ static void
>  mips_fbsd_fetch_inferior_registers (struct target_ops *ops,
>  				    struct regcache *regcache, int regnum)
>  {
> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);
>    pid_t pid = get_ptrace_pid (regcache_get_ptid (regcache));
>
> -  struct gdbarch *gdbarch = get_regcache_arch (regcache);
>    if (regnum == -1 || getregs_supplies (gdbarch, regnum))

With C++ we can leave the declaration closer to its use. Same in the 
other case below.

> @@ -58,12 +67,9 @@ mips_fbsd_fetch_inferior_registers (struct target_ops *ops,
>  	perror_with_name (_("Couldn't get registers"));
>
>        mips_fbsd_supply_gregs (regcache, regnum, &regs, sizeof (register_t));
> -      if (regnum != -1)
> -	return;
>      }
>
> -  if (regnum == -1
> -      || regnum >= gdbarch_fp0_regnum (get_regcache_arch (regcache)))
> +  if (regnum == -1 || getfpregs_supplies (gdbarch, regnum))

Does MIPS on fsbd handle vector registers? I ask this because regnum >= 
"fp0 regnum" may mean anything other than general purpose registers.

If there are vector (or higher-numbered registers), the new conditional 
block means something different compared to the old one.

If not, then the change looks sane.

> @@ -82,9 +88,9 @@ static void
>  mips_fbsd_store_inferior_registers (struct target_ops *ops,
>  				    struct regcache *regcache, int regnum)
>  {
> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);
>    pid_t pid = get_ptrace_pid (regcache_get_ptid (regcache));
>
> -  struct gdbarch *gdbarch = get_regcache_arch (regcache);
>    if (regnum == -1 || getregs_supplies (gdbarch, regnum))
>      {
>        struct reg regs;

Same as above about declaring something closer to where it is used.

> @@ -97,13 +103,9 @@ mips_fbsd_store_inferior_registers (struct target_ops *ops,
>
>        if (ptrace (PT_SETREGS, pid, (PTRACE_TYPE_ARG3) &regs, 0) == -1)
>  	perror_with_name (_("Couldn't write registers"));
> -
> -      if (regnum != -1)
> -	return;
>      }
>
> -  if (regnum == -1
> -      || regnum >= gdbarch_fp0_regnum (get_regcache_arch (regcache)))
> +  if (regnum == -1 || getfpregs_supplies (gdbarch, regnum))

Same thing as above, about higher-numbered registers.

Otherwise i have no further comments.

Re: [PATCH 1/4] Cleanups to FreeBSD/mips native register operations.

[John Baldwin]

On Thursday, April 13, 2017 10:48:48 AM Luis Machado wrote:
> On 04/12/2017 01:37 PM, John Baldwin wrote:
> > Compare against the "raw" PC register number instead of the cooked
> > register number when determining if a register was handled by
> > PT_GETREGS.  Previously the register fetch/store operations only tried
> > PT_GETREGS to fetch any individual register.  The result was that
> > fetching or storing an individual register not covered by PT_GETREGS
> > (such as floating point registers) did not work.
> >
> > While here, remove an early exit to simplify the code flow from the
> > PT_GETREGS / PT_SETREGS case, and add a getfpregs_supplies similar to
> > getregs_supplies to describe the registers supplied by PT_GETFPREGS
> > and PT_SETFPREGS.
> >
> > gdb/ChangeLog:
> >
> > 	* mips-fbsd-nat.c (getregs_supplies): Fix upper bound comparison.
> > 	(getpfpregs_supplies): New function.
> > 	(mips_fbsd_fetch_inferior_registers): Remove early exit and use
> > 	getfpregs_supplies.
> > 	(mips_fbsd_store_inferior_registers): Likewise.
> > ---
> >  gdb/ChangeLog       |  8 ++++++++
> >  gdb/mips-fbsd-nat.c | 26 ++++++++++++++------------
> >  2 files changed, 22 insertions(+), 12 deletions(-)
> 
> Only a few nits.
> 
> > diff --git a/gdb/mips-fbsd-nat.c b/gdb/mips-fbsd-nat.c
> > index 078df52db6..e2ed63e829 100644
> > --- a/gdb/mips-fbsd-nat.c
> > +++ b/gdb/mips-fbsd-nat.c
> > @@ -37,7 +37,16 @@ static bool
> >  getregs_supplies (struct gdbarch *gdbarch, int regnum)
> >  {
> >    return (regnum >= MIPS_ZERO_REGNUM
> > -	  && regnum <= gdbarch_pc_regnum (gdbarch));
> > +	  && regnum <= mips_regnum (gdbarch)->pc);
> > +}
> 
> Can the BSD backend override the pc value in gdbarch_pc_regnum (...) so 
> it fits what is expected? Or is this a case where the cooked pc register 
> number is still useful and we need to handle things differently for the 
> raw pc register number?

The cooked value is too large.  In particular, the existing range
right now goes from "0" to "cooked PC" which includes
"raw GP + raw FP + cooked GP".  This means that a request for a raw
FP register will see 'getregs_supplies() return true and will try
to satisfy it via PT_GETREGS (which doesn't work).  The end result is that
the register just isn't found.

> > +
> > +/* Determine if PT_GETFPREGS fetches this register.  */
> 
> Pedantically, "... fetches REGNUM".
> 
> > @@ -47,9 +56,9 @@ static void
> >  mips_fbsd_fetch_inferior_registers (struct target_ops *ops,
> >  				    struct regcache *regcache, int regnum)
> >  {
> > +  struct gdbarch *gdbarch = get_regcache_arch (regcache);
> >    pid_t pid = get_ptrace_pid (regcache_get_ptid (regcache));
> >
> > -  struct gdbarch *gdbarch = get_regcache_arch (regcache);
> >    if (regnum == -1 || getregs_supplies (gdbarch, regnum))
> 
> With C++ we can leave the declaration closer to its use. Same in the 
> other case below.

Yes, though in this case, gdbarch is actually used sooner than 'pid'.  I
can revert these though.

> > @@ -58,12 +67,9 @@ mips_fbsd_fetch_inferior_registers (struct target_ops *ops,
> >  	perror_with_name (_("Couldn't get registers"));
> >
> >        mips_fbsd_supply_gregs (regcache, regnum, &regs, sizeof (register_t));
> > -      if (regnum != -1)
> > -	return;
> >      }
> >
> > -  if (regnum == -1
> > -      || regnum >= gdbarch_fp0_regnum (get_regcache_arch (regcache)))
> > +  if (regnum == -1 || getfpregs_supplies (gdbarch, regnum))
> 
> Does MIPS on fsbd handle vector registers? I ask this because regnum >= 
> "fp0 regnum" may mean anything other than general purpose registers.

It does not, but I'm working on a research CPU that is an extension to MIPS
(CHERI) and it uses a third bank of registers that live above 'fp' with
a separate ptrace interface, etc.  I don't know that the CHERI bits will ever
be upstreamed since it is a research design, but this newer check seems
strictly more correct as it only uses PT_GETFPREGS to fetch registers that
are actually supported (e.g. it won't try to use it to fetch fir which
FreeBSD doesn't export via PT_GETFPREGS).

> If there are vector (or higher-numbered registers), the new conditional 
> block means something different compared to the old one.

I think that the new conditional is more correct in the case of higher
numbered registers as it means we don't try to use PT_GETFPREGS to
fetch a register it doesn't support.

> If not, then the change looks sane.

-- 
John Baldwin

[PATCH 2/4] Use mips_regnum instead of constants for FreeBSD/mips register operations.

[John Baldwin]

gdb/ChangeLog:

	* mips-fbsd-tdep.c (MIPS_PC_REGNUM): Remove.
	(MIPS_FP0_REGNUM): Remove.
	(MIPS_FSR_REGNUM): Remove.
	(mips_fbsd_supply_fpregs): Use mips_regnum.
	(mips_fbsd_supply_gregs): Likewise.
	(mips_fbsd_collect_fpregs): Likewise.
	(mips_fbsd_collect_gregs): Likewise.
---
 gdb/ChangeLog        | 10 ++++++++++
 gdb/mips-fbsd-tdep.c | 29 ++++++++++++++++-------------
 2 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 69c3efa317..73d24d2c9d 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,15 @@
 2017-04-11  John Baldwin  <jhb@FreeBSD.org>
 
+	* mips-fbsd-tdep.c (MIPS_PC_REGNUM): Remove.
+	(MIPS_FP0_REGNUM): Remove.
+	(MIPS_FSR_REGNUM): Remove.
+	(mips_fbsd_supply_fpregs): Use mips_regnum.
+	(mips_fbsd_supply_gregs): Likewise.
+	(mips_fbsd_collect_fpregs): Likewise.
+	(mips_fbsd_collect_gregs): Likewise.
+
+2017-04-11  John Baldwin  <jhb@FreeBSD.org>
+
 	* mips-fbsd-nat.c (getregs_supplies): Fix upper bound comparison.
 	(getpfpregs_supplies): New function.
 	(mips_fbsd_fetch_inferior_registers): Remove early exit and use
diff --git a/gdb/mips-fbsd-tdep.c b/gdb/mips-fbsd-tdep.c
index 00fae0ec60..d5bec3c157 100644
--- a/gdb/mips-fbsd-tdep.c
+++ b/gdb/mips-fbsd-tdep.c
@@ -29,11 +29,6 @@
 
 #include "solib-svr4.h"
 
-/* Shorthand for some register numbers used below.  */
-#define MIPS_PC_REGNUM  MIPS_EMBED_PC_REGNUM
-#define MIPS_FP0_REGNUM MIPS_EMBED_FP0_REGNUM
-#define MIPS_FSR_REGNUM MIPS_EMBED_FP0_REGNUM + 32
-
 /* Core file support. */
 
 /* Number of registers in `struct reg' from <machine/reg.h>.  The
@@ -108,13 +103,16 @@ void
 mips_fbsd_supply_fpregs (struct regcache *regcache, int regnum,
 			 const void *fpregs, size_t regsize)
 {
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
   const gdb_byte *regs = (const gdb_byte *) fpregs;
-  int i;
+  int i, fp0num, fsrnum;
 
-  for (i = MIPS_FP0_REGNUM; i <= MIPS_FSR_REGNUM; i++)
+  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 - MIPS_FP0_REGNUM) * regsize, regsize);
+			    regs + (i - fp0num) * regsize, regsize);
 }
 
 /* Supply the general-purpose registers stored in GREGS to REGCACHE.
@@ -125,10 +123,11 @@ void
 mips_fbsd_supply_gregs (struct regcache *regcache, int regnum,
 			const void *gregs, size_t regsize)
 {
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
   const gdb_byte *regs = (const gdb_byte *) gregs;
   int i;
 
-  for (i = 0; i <= MIPS_PC_REGNUM; i++)
+  for (i = 0; i <= mips_regnum (gdbarch)->pc; i++)
     if (regnum == i || regnum == -1)
       mips_fbsd_supply_reg (regcache, i, regs + i * regsize, regsize);
 }
@@ -141,13 +140,16 @@ void
 mips_fbsd_collect_fpregs (const struct regcache *regcache, int regnum,
 			  void *fpregs, size_t regsize)
 {
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
   gdb_byte *regs = (gdb_byte *) fpregs;
-  int i;
+  int i, fp0num, fsrnum;
 
-  for (i = MIPS_FP0_REGNUM; i <= MIPS_FSR_REGNUM; i++)
+  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 - MIPS_FP0_REGNUM) * regsize, regsize);
+			     regs + (i - fp0num) * regsize, regsize);
 }
 
 /* Collect the general-purpose registers from REGCACHE and store them
@@ -158,10 +160,11 @@ void
 mips_fbsd_collect_gregs (const struct regcache *regcache, int regnum,
 			 void *gregs, size_t regsize)
 {
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
   gdb_byte *regs = (gdb_byte *) gregs;
   int i;
 
-  for (i = 0; i <= MIPS_PC_REGNUM; i++)
+  for (i = 0; i <= mips_regnum (gdbarch)->pc; i++)
     if (regnum == i || regnum == -1)
       mips_fbsd_collect_reg (regcache, i, regs + i * regsize, regsize);
 }
-- 
2.11.0

Re: [PATCH 2/4] Use mips_regnum instead of constants for FreeBSD/mips register operations.

[Luis Machado]

On 04/12/2017 01:37 PM, John Baldwin wrote:
> gdb/ChangeLog:
>
> 	* mips-fbsd-tdep.c (MIPS_PC_REGNUM): Remove.
> 	(MIPS_FP0_REGNUM): Remove.
> 	(MIPS_FSR_REGNUM): Remove.
> 	(mips_fbsd_supply_fpregs): Use mips_regnum.
> 	(mips_fbsd_supply_gregs): Likewise.
> 	(mips_fbsd_collect_fpregs): Likewise.
> 	(mips_fbsd_collect_gregs): Likewise.
> ---
>  gdb/ChangeLog        | 10 ++++++++++
>  gdb/mips-fbsd-tdep.c | 29 ++++++++++++++++-------------
>  2 files changed, 26 insertions(+), 13 deletions(-)

This one looks correct to me. I have no comments.

[PATCH 3/4] Consistently use fprintf_filtered when displaying MIPS registers.

[John Baldwin]

One line was using printf_filtered instead of fprintf_filtered
to the requested file.

gdb/ChangeLog:

	* mips-tdep.c (print_gp_register_row): Replace printf_filtered
	with fprintf_filtered.
---
 gdb/ChangeLog   | 5 +++++
 gdb/mips-tdep.c | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 73d24d2c9d..f1ac925fec 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2017-04-11  John Baldwin  <jhb@FreeBSD.org>
 
+	* mips-tdep.c (print_gp_register_row): Replace printf_filtered
+	with fprintf_filtered.
+
+2017-04-11  John Baldwin  <jhb@FreeBSD.org>
+
 	* mips-fbsd-tdep.c (MIPS_PC_REGNUM): Remove.
 	(MIPS_FP0_REGNUM): Remove.
 	(MIPS_FSR_REGNUM): Remove.
diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 41cb9d82c6..674b5098b0 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -6539,7 +6539,7 @@ print_gp_register_row (struct ui_file *file, struct frame_info *frame,
       for (byte = 0;
 	   byte < (mips_abi_regsize (gdbarch)
 		   - register_size (gdbarch, regnum)); byte++)
-	printf_filtered ("  ");
+	fprintf_filtered (file, "  ");
       /* Now print the register value in hex, endian order.  */
       if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
 	for (byte =
-- 
2.11.0

Re: [PATCH 3/4] Consistently use fprintf_filtered when displaying MIPS registers.

[Luis Machado]

On 04/12/2017 01:37 PM, John Baldwin wrote:
> One line was using printf_filtered instead of fprintf_filtered
> to the requested file.
>
> gdb/ChangeLog:
>
> 	* mips-tdep.c (print_gp_register_row): Replace printf_filtered
> 	with fprintf_filtered.
> ---
>  gdb/ChangeLog   | 5 +++++
>  gdb/mips-tdep.c | 2 +-
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 73d24d2c9d..f1ac925fec 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,5 +1,10 @@
>  2017-04-11  John Baldwin  <jhb@FreeBSD.org>
>
> +	* mips-tdep.c (print_gp_register_row): Replace printf_filtered
> +	with fprintf_filtered.
> +
> +2017-04-11  John Baldwin  <jhb@FreeBSD.org>
> +
>  	* mips-fbsd-tdep.c (MIPS_PC_REGNUM): Remove.
>  	(MIPS_FP0_REGNUM): Remove.
>  	(MIPS_FSR_REGNUM): Remove.
> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
> index 41cb9d82c6..674b5098b0 100644
> --- a/gdb/mips-tdep.c
> +++ b/gdb/mips-tdep.c
> @@ -6539,7 +6539,7 @@ print_gp_register_row (struct ui_file *file, struct frame_info *frame,
>        for (byte = 0;
>  	   byte < (mips_abi_regsize (gdbarch)
>  		   - register_size (gdbarch, regnum)); byte++)
> -	printf_filtered ("  ");
> +	fprintf_filtered (file, "  ");
>        /* Now print the register value in hex, endian order.  */
>        if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
>  	for (byte =
>

This one seems trivial enough. I have no comments.

Re: [PATCH 3/4] Consistently use fprintf_filtered when displaying MIPS registers.

[Pedro Alves]

On 04/13/2017 05:29 PM, Luis Machado wrote:
> On 04/12/2017 01:37 PM, John Baldwin wrote:
>> One line was using printf_filtered instead of fprintf_filtered
>> to the requested file.
>>
>> gdb/ChangeLog:
>>
>>     * mips-tdep.c (print_gp_register_row): Replace printf_filtered
>>     with fprintf_filtered.
>> ---
>>  gdb/ChangeLog   | 5 +++++
>>  gdb/mips-tdep.c | 2 +-
>>  2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index 73d24d2c9d..f1ac925fec 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,5 +1,10 @@
>>  2017-04-11  John Baldwin  <jhb@FreeBSD.org>
>>
>> +    * mips-tdep.c (print_gp_register_row): Replace printf_filtered
>> +    with fprintf_filtered.
>> +
>> +2017-04-11  John Baldwin  <jhb@FreeBSD.org>
>> +
>>      * mips-fbsd-tdep.c (MIPS_PC_REGNUM): Remove.
>>      (MIPS_FP0_REGNUM): Remove.
>>      (MIPS_FSR_REGNUM): Remove.
>> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
>> index 41cb9d82c6..674b5098b0 100644
>> --- a/gdb/mips-tdep.c
>> +++ b/gdb/mips-tdep.c
>> @@ -6539,7 +6539,7 @@ print_gp_register_row (struct ui_file *file,
>> struct frame_info *frame,
>>        for (byte = 0;
>>         byte < (mips_abi_regsize (gdbarch)
>>             - register_size (gdbarch, regnum)); byte++)
>> -    printf_filtered ("  ");
>> +    fprintf_filtered (file, "  ");
>>        /* Now print the register value in hex, endian order.  */
>>        if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
>>      for (byte =
>>
> 
> This one seems trivial enough. I have no comments.

Agreed.  This one's obviously correct.  Please push it in.

Thanks,
Pedro Alves

Re: [PATCH 3/4] Consistently use fprintf_filtered when displaying MIPS registers.

[John Baldwin]

On Thursday, April 27, 2017 05:05:33 PM Pedro Alves wrote:
> On 04/13/2017 05:29 PM, Luis Machado wrote:
> > On 04/12/2017 01:37 PM, John Baldwin wrote:
> >> One line was using printf_filtered instead of fprintf_filtered
> >> to the requested file.
> >>
> >> gdb/ChangeLog:
> >>
> >>     * mips-tdep.c (print_gp_register_row): Replace printf_filtered
> >>     with fprintf_filtered.
> >> ---
> >>  gdb/ChangeLog   | 5 +++++
> >>  gdb/mips-tdep.c | 2 +-
> >>  2 files changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> >> index 73d24d2c9d..f1ac925fec 100644
> >> --- a/gdb/ChangeLog
> >> +++ b/gdb/ChangeLog
> >> @@ -1,5 +1,10 @@
> >>  2017-04-11  John Baldwin  <jhb@FreeBSD.org>
> >>
> >> +    * mips-tdep.c (print_gp_register_row): Replace printf_filtered
> >> +    with fprintf_filtered.
> >> +
> >> +2017-04-11  John Baldwin  <jhb@FreeBSD.org>
> >> +
> >>      * mips-fbsd-tdep.c (MIPS_PC_REGNUM): Remove.
> >>      (MIPS_FP0_REGNUM): Remove.
> >>      (MIPS_FSR_REGNUM): Remove.
> >> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
> >> index 41cb9d82c6..674b5098b0 100644
> >> --- a/gdb/mips-tdep.c
> >> +++ b/gdb/mips-tdep.c
> >> @@ -6539,7 +6539,7 @@ print_gp_register_row (struct ui_file *file,
> >> struct frame_info *frame,
> >>        for (byte = 0;
> >>         byte < (mips_abi_regsize (gdbarch)
> >>             - register_size (gdbarch, regnum)); byte++)
> >> -    printf_filtered ("  ");
> >> +    fprintf_filtered (file, "  ");
> >>        /* Now print the register value in hex, endian order.  */
> >>        if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
> >>      for (byte =
> >>
> > 
> > This one seems trivial enough. I have no comments.
> 
> Agreed.  This one's obviously correct.  Please push it in.

Pushed.  I know we are still discussing patch 4, but are patches 1 and
2 in the V2 series ok to go in?  I believe I addressed Luis' earlier
feedback on those two patches.

-- 
John Baldwin

[PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.

[John Baldwin]

'info registers' for MIPS throws an error and when it first encounters
an unavailable register.  This does not match other architectures
which annotate unavailable registers and continue to print out the
values of subsequent registers.  Replace the error by displaying an
aligned "<unavailable>".  This string is truncated when displaying a
32-bit register.

gdb/ChangeLog:

	* mips-tdep.c (print_gp_register_row): Don't error for unavailable
	registers.
---
 gdb/ChangeLog   |  5 +++++
 gdb/mips-tdep.c | 10 ++++++++--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index f1ac925fec..96a187667d 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2017-04-11  John Baldwin  <jhb@FreeBSD.org>
 
+	* mips-tdep.c (print_gp_register_row): Don't error for unavailable
+	registers.
+
+2017-04-11  John Baldwin  <jhb@FreeBSD.org>
+
 	* mips-tdep.c (print_gp_register_row): Replace printf_filtered
 	with fprintf_filtered.
 
diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 674b5098b0..4ec0c31341 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -6533,8 +6533,14 @@ print_gp_register_row (struct ui_file *file, struct frame_info *frame,
 
       /* OK: get the data in raw format.  */
       if (!deprecated_frame_register_read (frame, regnum, raw_buffer))
-	error (_("can't read register %d (%s)"),
-	       regnum, gdbarch_register_name (gdbarch, regnum));
+	{
+	  fprintf_filtered (file, "%*.*s ",
+			    (int) mips_abi_regsize (gdbarch) * 2,
+			    (int) mips_abi_regsize (gdbarch) * 2,
+			    "<unavailable>");
+	  col++;
+	  continue;
+	}
       /* pad small registers */
       for (byte = 0;
 	   byte < (mips_abi_regsize (gdbarch)
-- 
2.11.0

Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.

[Luis Machado]

On 04/12/2017 01:37 PM, John Baldwin wrote:
> index 674b5098b0..4ec0c31341 100644
> --- a/gdb/mips-tdep.c
> +++ b/gdb/mips-tdep.c
> @@ -6533,8 +6533,14 @@ print_gp_register_row (struct ui_file *file, struct frame_info *frame,
>
>        /* OK: get the data in raw format.  */
>        if (!deprecated_frame_register_read (frame, regnum, raw_buffer))
> -	error (_("can't read register %d (%s)"),
> -	       regnum, gdbarch_register_name (gdbarch, regnum));
> +	{
> +	  fprintf_filtered (file, "%*.*s ",
> +			    (int) mips_abi_regsize (gdbarch) * 2,
> +			    (int) mips_abi_regsize (gdbarch) * 2,
> +			    "<unavailable>");
> +	  col++;
> +	  continue;
> +	}

What is the output you're getting in this case?

Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.

[John Baldwin]

On Thursday, April 13, 2017 11:37:15 AM Luis Machado wrote:
> On 04/12/2017 01:37 PM, John Baldwin wrote:
> > index 674b5098b0..4ec0c31341 100644
> > --- a/gdb/mips-tdep.c
> > +++ b/gdb/mips-tdep.c
> > @@ -6533,8 +6533,14 @@ print_gp_register_row (struct ui_file *file, struct frame_info *frame,
> >
> >        /* OK: get the data in raw format.  */
> >        if (!deprecated_frame_register_read (frame, regnum, raw_buffer))
> > -	error (_("can't read register %d (%s)"),
> > -	       regnum, gdbarch_register_name (gdbarch, regnum));
> > +	{
> > +	  fprintf_filtered (file, "%*.*s ",
> > +			    (int) mips_abi_regsize (gdbarch) * 2,
> > +			    (int) mips_abi_regsize (gdbarch) * 2,
> > +			    "<unavailable>");
> > +	  col++;
> > +	  continue;
> > +	}
> 
> What is the output you're getting in this case?

On FreeBSD (which doesn't support fir) I now get this:

O32:

(gdb) info registers 
          zero       at       v0       v1       a0       a1       a2       a3
 R0   00000000 00000000 00000000 00000000 00000001 7fffecac 7fffecb4 004182c0 
            t0       t1       t2       t3       t4       t5       t6       t7
 R8   00000000 00000000 00000061 2e9a1b76 00124f9b 00000001 00001000 00401964 
            s0       s1       s2       s3       s4       s5       s6       s7
 R16  7fffecb4 00000001 7fffecac 00000000 00000000 00000000 00000000 00000000 
            t8       t9       k0       k1       gp       sp       s8       ra
 R24  0000008d 004033d0 00000000 00000000 00420320 7ffeec60 00000000 00401980 
            sr       lo       hi      bad    cause       pc
      00008413 0000d0d7 000002d1 004033d0 00000024 004033d0 
           fsr      fir
      00000000 <unavail 


N64:

(gdb) info registers 
                  zero               at               v0               v1
 R0   0000000000000000 0000000000000000 0000000000000000 0000000000000000 
                    a0               a1               a2               a3
 R4   0000000000000001 0000007fffffeb40 0000007fffffeb50 0000000120018030 
                    a4               a5               a6               a7
 R8   0000000000000000 0000000000000001 0000000000000000 0000000000000001 
                    t0               t1               t2               t3
 R12  0000000000000000 0000000000000000 0000000000000000 0000000160051940 
                    s0               s1               s2               s3
 R16  0000007fffffeb50 0000000000000001 0000007fffffeb40 0000000000000000 
                    s4               s5               s6               s7
 R20  0000000000000000 0000000000000000 0000000000000000 0000000000000000 
                    t8               t9               k0               k1
 R24  000000000000008d 00000001200035e8 0000000000000000 0000000000000000 
                    gp               sp               s8               ra
 R28  0000000120020090 0000007fffffead8 0000000000000000 00000001200020b4 
                    sr               lo               hi              bad
      00000000008084b3 000000000000d0d7 00000000000002d1 0000007fffffeaa8 
                 cause               pc
      0000000000000024 00000001200035e8 
                   fsr              fir
              00000000    <unavailable> 

It was more dire on a target that doesn't supply all registers.  For example,
I have an out of tree target for FreeBSD kernels and stopped threads in
FreeBSD's kernel only supply a subset of GPRs.  Without the patch examining
registers for a stopped thread looks like this:

(kgdb) info registers 
                  zero               at               v0               v1
 R0   0000000000000000 can't read register 91 (at)

with the patch it looks like this:

(kgdb) info registers 
                  zero               at               v0               v1
 R0   0000000000000000    <unavailable>    <unavailable>    <unavailable> 
                    a0               a1               a2               a3
 R4      <unavailable>    <unavailable>    <unavailable>    <unavailable> 
                    a4               a5               a6               a7
 R8      <unavailable>    <unavailable>    <unavailable>    <unavailable> 
                    t0               t1               t2               t3
 R12     <unavailable>    <unavailable>    <unavailable>    <unavailable> 
                    s0               s1               s2               s3
 R16  00000000ffff00fa 0000000000000000 0000000000000000 0000010000000000 
                    s4               s5               s6               s7
 R20  00000000ffff00fa 0000000000000000 0000000000000000 0000010000000000 
                    t8               t9               k0               k1
 R24  0000007fffffefa0    <unavailable>    <unavailable>    <unavailable> 
                    gp               sp               s8               ra
 R28  0000000000000102 00000000fffd002e    <unavailable> 0000007fffffefa0 
                    sr               lo               hi              bad
      0000000000000010    <unavailable>    <unavailable>    <unavailable> 
                 cause               pc
         <unavailable> 0000000000000000 
                   fsr              fir
         <unavailable>    <unavailable> 

-- 
John Baldwin

Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.

[Maciej W. Rozycki]

On Fri, 14 Apr 2017, John Baldwin wrote:

> > What is the output you're getting in this case?
> 
> On FreeBSD (which doesn't support fir) I now get this:

 If the register is not ever supplied, then you need a target description 
that does not include it.  The rest of code will then handle it correctly.

> It was more dire on a target that doesn't supply all registers.  For example,
> I have an out of tree target for FreeBSD kernels and stopped threads in
> FreeBSD's kernel only supply a subset of GPRs.  Without the patch examining
> registers for a stopped thread looks like this:

 Why can't the remaining general registers be read or written -- is that a 
bug in the kernel?

 That sort of defeats the point of debugging, where you'd expect to be 
able to poke at any register that is at debuggee's disposal (so not 
supplying FIR can be considered a bug too).  A program's variable could 
live in such an inaccessible register for example.

 I'll see if there's anything else I want to comment on in this series 
next week.

  Maciej

Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.

[John Baldwin]

On Saturday, April 15, 2017 05:02:23 PM Maciej W. Rozycki wrote:
> On Fri, 14 Apr 2017, John Baldwin wrote:
> 
> > > What is the output you're getting in this case?
> > 
> > On FreeBSD (which doesn't support fir) I now get this:
> 
>  If the register is not ever supplied, then you need a target description 
> that does not include it.  The rest of code will then handle it correctly.

No, mips-tdep.c requires fir to be included in the description:

static struct gdbarch *
mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
{
  ...
  /* Check any target description for validity.  */
  if (tdesc_has_registers (info.target_desc))
    {
      ...
      valid_p
        &= tdesc_numbered_register (feature, tdesc_data,
                                    mips_regnum.fp_implementation_revision,
                                    "fir");

      if (!valid_p)
        {
          tdesc_data_cleanup (tdesc_data);
          return NULL;
        }

      ...
    }
  ...
}

Thus, any target description that doesn't include fir is rejected.  I will
change FreeBSD to export fir via ptrace() and core dumps at some point,
but it doesn't currently.

Note that Linux doesn't supply a valid fir from core dumps either (it just
hardcodes it as zero):

linux-mips-tdep.c:

void
mips_supply_fpregset (struct regcache *regcache,
                      const mips_elf_fpregset_t *fpregsetp)
{
  ...
  char zerobuf[MAX_REGISTER_SIZE];

  memset (zerobuf, 0, MAX_REGISTER_SIZE);
  ...

  /* FIXME: how can we supply FCRIR?  The ABI doesn't tell us.  */
  regcache_raw_supply (regcache,
                       mips_regnum (gdbarch)->fp_implementation_revision,
                       zerobuf);
}

> > It was more dire on a target that doesn't supply all registers.  For example,
> > I have an out of tree target for FreeBSD kernels and stopped threads in
> > FreeBSD's kernel only supply a subset of GPRs.  Without the patch examining
> > registers for a stopped thread looks like this:
> 
>  Why can't the remaining general registers be read or written -- is that a 
> bug in the kernel?
> 
>  That sort of defeats the point of debugging, where you'd expect to be 
> able to poke at any register that is at debuggee's disposal (so not 
> supplying FIR can be considered a bug too).  A program's variable could 
> live in such an inaccessible register for example.

This isn't about the user thread state.  When a user thread enters the kernel
due to an exception, system call, etc. then all registers are saved and are
available to the debugger.  This is about debugging kernel threads in the kernel
itself.  During a context switch, only a subset of registers are explicitly
saved in the thread's control block on FreeBSD (generally callee-save registers).
Caller-save registers can be found by unwinding the stack.

-- 
John Baldwin

Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.

[Maciej W. Rozycki]

On Sat, 15 Apr 2017, John Baldwin wrote:

> >  If the register is not ever supplied, then you need a target description 
> > that does not include it.  The rest of code will then handle it correctly.
> 
> No, mips-tdep.c requires fir to be included in the description:
> 
> static struct gdbarch *
> mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> {
>   ...
>   /* Check any target description for validity.  */
>   if (tdesc_has_registers (info.target_desc))
>     {
>       ...
>       valid_p
>         &= tdesc_numbered_register (feature, tdesc_data,
>                                     mips_regnum.fp_implementation_revision,
>                                     "fir");
> 
>       if (!valid_p)
>         {
>           tdesc_data_cleanup (tdesc_data);
>           return NULL;
>         }
> 
>       ...
>     }
>   ...
> }

 That needs to be fixed then.  Previously there was no need to handle FIR 
specially.  Overall we need to handle configurations without FPU as well.

> Note that Linux doesn't supply a valid fir from core dumps either (it just
> hardcodes it as zero):
> 
> linux-mips-tdep.c:
> 
> void
> mips_supply_fpregset (struct regcache *regcache,
>                       const mips_elf_fpregset_t *fpregsetp)
> {
>   ...
>   char zerobuf[MAX_REGISTER_SIZE];
> 
>   memset (zerobuf, 0, MAX_REGISTER_SIZE);
>   ...
> 
>   /* FIXME: how can we supply FCRIR?  The ABI doesn't tell us.  */
>   regcache_raw_supply (regcache,
>                        mips_regnum (gdbarch)->fp_implementation_revision,
>                        zerobuf);
> }

 Ack.

> >  Why can't the remaining general registers be read or written -- is that a 
> > bug in the kernel?
> > 
> >  That sort of defeats the point of debugging, where you'd expect to be 
> > able to poke at any register that is at debuggee's disposal (so not 
> > supplying FIR can be considered a bug too).  A program's variable could 
> > live in such an inaccessible register for example.
> 
> This isn't about the user thread state.  When a user thread enters the kernel
> due to an exception, system call, etc. then all registers are saved and are
> available to the debugger.  This is about debugging kernel threads in the kernel
> itself.  During a context switch, only a subset of registers are explicitly
> saved in the thread's control block on FreeBSD (generally callee-save registers).
> Caller-save registers can be found by unwinding the stack.

 Fair enough.  Such details have to be given in the patch description 
itself though.

 I'm somewhat put off by the truncated message in the 32-bit case though 
-- unless a better proposition comes up, then how about using `xxxxxxxx'
and `xxxxxxxxxxxxxxxx' for the 32-bit and 64-bit case respectively as with 
some previous effort?  What do other target backends do?

  Maciej

Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.

[John Baldwin]

On Saturday, April 15, 2017 11:06:35 PM Maciej W. Rozycki wrote:
> On Sat, 15 Apr 2017, John Baldwin wrote:
> 
> > >  If the register is not ever supplied, then you need a target description 
> > > that does not include it.  The rest of code will then handle it correctly.
> > 
> > No, mips-tdep.c requires fir to be included in the description:
> > 
> > static struct gdbarch *
> > mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> > {
> >   ...
> >   /* Check any target description for validity.  */
> >   if (tdesc_has_registers (info.target_desc))
> >     {
> >       ...
> >       valid_p
> >         &= tdesc_numbered_register (feature, tdesc_data,
> >                                     mips_regnum.fp_implementation_revision,
> >                                     "fir");
> > 
> >       if (!valid_p)
> >         {
> >           tdesc_data_cleanup (tdesc_data);
> >           return NULL;
> >         }
> > 
> >       ...
> >     }
> >   ...
> > }
> 
>  That needs to be fixed then.  Previously there was no need to handle FIR 
> specially.  Overall we need to handle configurations without FPU as well.

That might be true, but that is a larger patch (and it doesn't help with my
kernel target case where only a subset of GPRs are valid).

> > >  Why can't the remaining general registers be read or written -- is that a 
> > > bug in the kernel?
> > > 
> > >  That sort of defeats the point of debugging, where you'd expect to be 
> > > able to poke at any register that is at debuggee's disposal (so not 
> > > supplying FIR can be considered a bug too).  A program's variable could 
> > > live in such an inaccessible register for example.
> > 
> > This isn't about the user thread state.  When a user thread enters the kernel
> > due to an exception, system call, etc. then all registers are saved and are
> > available to the debugger.  This is about debugging kernel threads in the kernel
> > itself.  During a context switch, only a subset of registers are explicitly
> > saved in the thread's control block on FreeBSD (generally callee-save registers).
> > Caller-save registers can be found by unwinding the stack.
> 
>  Fair enough.  Such details have to be given in the patch description 
> itself though.

Ok.  I can give this as an example in the commit log.

>  I'm somewhat put off by the truncated message in the 32-bit case though 
> -- unless a better proposition comes up, then how about using `xxxxxxxx'
> and `xxxxxxxxxxxxxxxx' for the 32-bit and 64-bit case respectively as with 
> some previous effort?  What do other target backends do?

I don't disagree with the 32-bit format and am certainly open to other
options.  Other architectures don't generally use a table and use the full
"<unavailable>" string, e.g. on a FreeBSD 64-bit x86 kernel (which uses the
"stock" method to print registers rather than a gdbarch-specific one):

(kgdb) info registers 
rax            <unavailable>
rbx            0xfffff800085cb500       -8795952728832
rcx            <unavailable>
rdx            <unavailable>
rsi            <unavailable>
rdi            <unavailable>
rbp            0xfffffe04674819c0       0xfffffe04674819c0
rsp            0xfffffe0467481968       0xfffffe0467481968
r8             <unavailable>
r9             <unavailable>
r10            <unavailable>
r11            <unavailable>
r12            0xffffffff80d43b00       -2133574912
r13            0xfffff801fb2f3000       -8787583881216
r14            0x0      0
r15            0xffffffff80d43b00       -2133574912
rip            0xffffffff8058bb12       0xffffffff8058bb12 <sched_switch+1218>
eflags         <unavailable>
cs             0x20     32
ss             0x28     40
ds             <unavailable>
es             <unavailable>
fs             <unavailable>
gs             <unavailable>


-- 
John Baldwin

Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.

[Maciej W. Rozycki]

On Mon, 17 Apr 2017, John Baldwin wrote:

> >  That needs to be fixed then.  Previously there was no need to handle FIR 
> > specially.  Overall we need to handle configurations without FPU as well.
> 
> That might be true, but that is a larger patch (and it doesn't help with my
> kernel target case where only a subset of GPRs are valid).

 Minimising changes is not our goal though, unlike making them correct.  
And I think we need to tell apart a situation where a register (FIR) is 
invalid according to the OS ABI and where a subset of registers may not 
always be accessible.

> >  I'm somewhat put off by the truncated message in the 32-bit case though 
> > -- unless a better proposition comes up, then how about using `xxxxxxxx'
> > and `xxxxxxxxxxxxxxxx' for the 32-bit and 64-bit case respectively as with 
> > some previous effort?  What do other target backends do?
> 
> I don't disagree with the 32-bit format and am certainly open to other
> options.  Other architectures don't generally use a table and use the full
> "<unavailable>" string, e.g. on a FreeBSD 64-bit x86 kernel (which uses the
> "stock" method to print registers rather than a gdbarch-specific one):
> 
> (kgdb) info registers 
> rax            <unavailable>
> rbx            0xfffff800085cb500       -8795952728832
> rcx            <unavailable>
> rdx            <unavailable>
> rsi            <unavailable>
> rdi            <unavailable>
> rbp            0xfffffe04674819c0       0xfffffe04674819c0
> rsp            0xfffffe0467481968       0xfffffe0467481968
> r8             <unavailable>
> r9             <unavailable>
> r10            <unavailable>
> r11            <unavailable>
> r12            0xffffffff80d43b00       -2133574912
> r13            0xfffff801fb2f3000       -8787583881216
> r14            0x0      0
> r15            0xffffffff80d43b00       -2133574912
> rip            0xffffffff8058bb12       0xffffffff8058bb12 <sched_switch+1218>
> eflags         <unavailable>
> cs             0x20     32
> ss             0x28     40
> ds             <unavailable>
> es             <unavailable>
> fs             <unavailable>
> gs             <unavailable>

 Thanks for checking that.  NB I find output above quite messy, especially 
the lack of column alignment, e.g. `r14' vs `r15'.  It makes it hard to 
read for me.

 I've given your proposal some thought and I'm of mixed minds between two 
possibilites I have come up with for the condensed format, specifically 
`xxxxxxxx' vs `<absent>', which I think are both suitable without being 
messy:

(gdb) info registers
          zero       at       v0       v1       a0       a1       a2       a3
 R0   00000000 fffffff8 77fde8cc 00000000 77ff0000 77fec458 7ffffbd4 7ffffbdc
            t0       t1       t2       t3       t4       t5       t6       t7
 R8   c34e4bff ea5f612e 81010100 2f2f2f2f 7ffffce1 ffffffff f0000000 7ffff4e8
            s0       s1       s2       s3       s4       s5       s6       s7
 R16  78007010 00401360 xxxxxxxx xxxxxxxx xxxxxxxx 004f0000 00000063 0000006c
            t8       t9       k0       k1       gp       sp       s8       ra
 R24  77fdf64c 00401360 7ffff67c 00000000 78007010 7ffffbd0 00000061 77fcc97c
        status       lo       hi badvaddr    cause       pc
      0100a413 008cbcee 00000006 77f83e90 00800024 00401360
          fcsr      fir  restart
      00000000 00f30000 00000000
(gdb)

vs:

(gdb) info registers
          zero       at       v0       v1       a0       a1       a2       a3
 R0   00000000 fffffff8 77fde8cc 00000000 77ff0000 77fec458 7ffffbd4 7ffffbdc
            t0       t1       t2       t3       t4       t5       t6       t7
 R8   c34e4bff ea5f612e 81010100 2f2f2f2f 7ffffce1 ffffffff f0000000 7ffff4e8
            s0       s1       s2       s3       s4       s5       s6       s7
 R16  78007010 00401360 <absent> <absent> <absent> 004f0000 00000063 0000006c
            t8       t9       k0       k1       gp       sp       s8       ra
 R24  77fdf64c 00401360 7ffff67c 00000000 78007010 7ffffbd0 00000061 77fcc97c
        status       lo       hi badvaddr    cause       pc
      0100a413 008cbcee 00000006 77f83e90 00800024 00401360
          fcsr      fir  restart
      00000000 00f30000 00000000
(gdb)

The use of angle brackets with the latter variant is consistent with other 
targets, so I might have just a slight preference for it, but I'll be 
happy to accept input from other people.

 Where space is at disposal full `<unavailable>' could be printed, e.g:

(gdb) info registers s3 s4 s5
s3: <unavailable>
s4: <unavailable>
s5: 0x4f0000
(gdb)

or:

(gdb) info registers
                  zero               at               v0               v1
 R0   0000000000000000 fffffffffffffff0   <unavailable>  ffffffffc0000000
                    a0               a1               a2               a3
 R4     <unavailable>  0000000000000001 000000fffffffbb8 000000fffffffbc8
                    a4               a5               a6               a7
 R8     <unavailable>  0000000000000000 8101010101010100 2f2f2f2f2f2f2f2f
                    t0               t1               t2               t3
 R12  0000000000000000 0000000000000001 ffffffff8122f5d8   <unavailable>
                    s0               s1               s2               s3
 R16  000000fff8006000 0000000120000970 000000fff8006000 0000000000500000
                    s4               s5               s6               s7
 R20  0000000000521ec8 0000000000522608 0000000000000000 0000000000000000
                    t8               t9               k0               k1
 R24  000000000000161a 0000000120000970 0000000000000000 0000000000000000
                    gp               sp               s8               ra
 R28  000000fff8006000 000000fffffffbb0 0000000000000000   <unavailable>
                status               lo               hi         badvaddr
      0000000000109cf3 000000000d35ec75 0000000000000000   <unavailable>
                 cause               pc
      0000000000800024 0000000120000970
                  fcsr              fir          restart
              00000000         00f30000 0000000000000000
(gdb)

 Comments?  Thoughts?

  Maciej

Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.

[John Baldwin]

On Tuesday, April 18, 2017 10:33:28 PM Maciej W. Rozycki wrote:
> On Mon, 17 Apr 2017, John Baldwin wrote:
> 
> > >  That needs to be fixed then.  Previously there was no need to handle FIR 
> > > specially.  Overall we need to handle configurations without FPU as well.
> > 
> > That might be true, but that is a larger patch (and it doesn't help with my
> > kernel target case where only a subset of GPRs are valid).
> 
>  Minimising changes is not our goal though, unlike making them correct.  
> And I think we need to tell apart a situation where a register (FIR) is 
> invalid according to the OS ABI and where a subset of registers may not 
> always be accessible.

For FreeBSD/mips in particular I will probably fix the FIR issue by fixing
FreeBSD to export FIR.  That said, 'info registers' on other architectures
is consistent in that they do not throw an error for an unavailable register
but annotate it as such.  The goal of this patch was to align MIPS with
other architectures in terms of that behavior.  Other architectures also
permit registers to be unavailable without requiring a custom target
description FWIW (e.g. the segment registers on x86 are effectively
"optional" and not always supplied by a target).

> > >  I'm somewhat put off by the truncated message in the 32-bit case though 
> > > -- unless a better proposition comes up, then how about using `xxxxxxxx'
> > > and `xxxxxxxxxxxxxxxx' for the 32-bit and 64-bit case respectively as with 
> > > some previous effort?  What do other target backends do?
> > 
> > I don't disagree with the 32-bit format and am certainly open to other
> > options.  Other architectures don't generally use a table and use the full
> > "<unavailable>" string, e.g. on a FreeBSD 64-bit x86 kernel (which uses the
> > "stock" method to print registers rather than a gdbarch-specific one):
> > 
> > (kgdb) info registers 
> > rax            <unavailable>
> > rbx            0xfffff800085cb500       -8795952728832
> > rcx            <unavailable>
> > rdx            <unavailable>
> > rsi            <unavailable>
> > rdi            <unavailable>
> > rbp            0xfffffe04674819c0       0xfffffe04674819c0
> > rsp            0xfffffe0467481968       0xfffffe0467481968
> > r8             <unavailable>
> > r9             <unavailable>
> > r10            <unavailable>
> > r11            <unavailable>
> > r12            0xffffffff80d43b00       -2133574912
> > r13            0xfffff801fb2f3000       -8787583881216
> > r14            0x0      0
> > r15            0xffffffff80d43b00       -2133574912
> > rip            0xffffffff8058bb12       0xffffffff8058bb12 <sched_switch+1218>
> > eflags         <unavailable>
> > cs             0x20     32
> > ss             0x28     40
> > ds             <unavailable>
> > es             <unavailable>
> > fs             <unavailable>
> > gs             <unavailable>
> 
>  Thanks for checking that.  NB I find output above quite messy, especially 
> the lack of column alignment, e.g. `r14' vs `r15'.  It makes it hard to 
> read for me.

I don't disagree with the note on alignment.  That is probably worth fixing
in a separate change.

>  I've given your proposal some thought and I'm of mixed minds between two 
> possibilites I have come up with for the condensed format, specifically 
> `xxxxxxxx' vs `<absent>', which I think are both suitable without being 
> messy:
> 
> [snip]
> 
> The use of angle brackets with the latter variant is consistent with other 
> targets, so I might have just a slight preference for it, but I'll be 
> happy to accept input from other people.

I like <absent> and prefer it for similar reasons (consistency).

>  Where space is at disposal full `<unavailable>' could be printed, e.g:
> 
> (gdb) info registers s3 s4 s5
> s3: <unavailable>
> s4: <unavailable>
> s5: 0x4f0000
> (gdb)
> 
> or:
> 
> (gdb) info registers
>                   zero               at               v0               v1
>  R0   0000000000000000 fffffffffffffff0   <unavailable>  ffffffffc0000000
>                     a0               a1               a2               a3
>  R4     <unavailable>  0000000000000001 000000fffffffbb8 000000fffffffbc8
>                     a4               a5               a6               a7
>  R8     <unavailable>  0000000000000000 8101010101010100 2f2f2f2f2f2f2f2f
>                     t0               t1               t2               t3
>  R12  0000000000000000 0000000000000001 ffffffff8122f5d8   <unavailable>
>                     s0               s1               s2               s3
>  R16  000000fff8006000 0000000120000970 000000fff8006000 0000000000500000
>                     s4               s5               s6               s7
>  R20  0000000000521ec8 0000000000522608 0000000000000000 0000000000000000
>                     t8               t9               k0               k1
>  R24  000000000000161a 0000000120000970 0000000000000000 0000000000000000
>                     gp               sp               s8               ra
>  R28  000000fff8006000 000000fffffffbb0 0000000000000000   <unavailable>
>                 status               lo               hi         badvaddr
>       0000000000109cf3 000000000d35ec75 0000000000000000   <unavailable>
>                  cause               pc
>       0000000000800024 0000000120000970
>                   fcsr              fir          restart
>               00000000         00f30000 0000000000000000
> (gdb)
> 
>  Comments?  Thoughts?

I'm generally fine with using <absent> in the 32-bit table and <unavailable>
otherwise.  I can rework the patch to do that and come back with a V2.

-- 
John Baldwin

Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.

[John Baldwin]

On Tuesday, April 18, 2017 03:19:31 PM John Baldwin wrote:
> On Tuesday, April 18, 2017 10:33:28 PM Maciej W. Rozycki wrote:
> > The use of angle brackets with the latter variant is consistent with other 
> > targets, so I might have just a slight preference for it, but I'll be 
> > happy to accept input from other people.
> 
> I like <absent> and prefer it for similar reasons (consistency).

FYI, I've posted a V2 which uses <absent>.

> >  Where space is at disposal full `<unavailable>' could be printed, e.g:
> > 
> > (gdb) info registers s3 s4 s5
> > s3: <unavailable>
> > s4: <unavailable>
> > s5: 0x4f0000

This already works this way since it uses the generic routines to print
register values.  Only the table format is affected by the patch.

-- 
John Baldwin

Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.

[Maciej W. Rozycki]

On Tue, 18 Apr 2017, John Baldwin wrote:

> >  Minimising changes is not our goal though, unlike making them correct.  
> > And I think we need to tell apart a situation where a register (FIR) is 
> > invalid according to the OS ABI and where a subset of registers may not 
> > always be accessible.
> 
> For FreeBSD/mips in particular I will probably fix the FIR issue by fixing
> FreeBSD to export FIR.

 We'll still have to handle old kernel versions that do not have the fix.

>  That said, 'info registers' on other architectures
> is consistent in that they do not throw an error for an unavailable register
> but annotate it as such.  The goal of this patch was to align MIPS with
> other architectures in terms of that behavior.  Other architectures also
> permit registers to be unavailable without requiring a custom target
> description FWIW (e.g. the segment registers on x86 are effectively
> "optional" and not always supplied by a target).

 Agreed, however you need to be clear in your patch description which of 
the two bugs present here it is intended to fix.

> >  Thanks for checking that.  NB I find output above quite messy, especially 
> > the lack of column alignment, e.g. `r14' vs `r15'.  It makes it hard to 
> > read for me.
> 
> I don't disagree with the note on alignment.  That is probably worth fixing
> in a separate change.

 Sure, I haven't asked you or indeed anybody to rush fixing it (although 
obviously I won't mind either).

 I'll look through v2 of your changes and see if I have any further
concerns.

  Maciej

Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.

[Pedro Alves]

On 04/18/2017 10:33 PM, Maciej W. Rozycki wrote:
> 
> The use of angle brackets with the latter variant is consistent with other 
> targets, so I might have just a slight preference for it, but I'll be 
> happy to accept input from other people.

FWIW, I'd find using anything but <unavailable> when that's what
GDB means to be making the port be gratuitously different.
"<unavailable>" always means the same thing in gdb -- gdb knows
the value materially exists, but it can't get at it for some
reason (e.g., ptrace does not expose it, a core dump is trimmed, value
not collect in a trace frame, etc.).

Also, I'm not absolutely sure I'm following the code correctly, but ISTM
that the current mips table printing code can already print strings larger
than unavailable, like "<invalid float>" in mips_print_fp_register?

I also noticed that there's code in print_gp_register_row that handles
printing large registers separately:

      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))
	{
...
	  /* Print this register on a row by itself.  */

supposedly to avoid column misalignment.  Maybe <unavailable> registers could
be printed on a row by themselves too?

(Or the column width could extended if there's an unavailable register, but I'm
sort of trying to avoid suggesting a deeper change.)

Note that while the mips table format is more compact, the generic
format has the advantage that is prints symbol information, like:

>> rip            0xffffffff8058bb12       0xffffffff8058bb12 <sched_switch+1218>

I definitely agree that it'd be nice to make the generic code
always print the columns neatly aligned.

(Maybe the ideal would be if the generic code learned about MIPS compact
format and users could pick the format they want with a format switch
like "info register /c" or some such.  But again, that'd be a larger
change which I'd not like to require.)

Thanks,
Pedro Alves

Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.

[Maciej W. Rozycki]

On Thu, 27 Apr 2017, Pedro Alves wrote:

> > The use of angle brackets with the latter variant is consistent with other 
> > targets, so I might have just a slight preference for it, but I'll be 
> > happy to accept input from other people.
> 
> FWIW, I'd find using anything but <unavailable> when that's what
> GDB means to be making the port be gratuitously different.
> "<unavailable>" always means the same thing in gdb -- gdb knows
> the value materially exists, but it can't get at it for some
> reason (e.g., ptrace does not expose it, a core dump is trimmed, value
> not collect in a trace frame, etc.).

 If you think keeping the word the same across ports is essential, then 
`<unavl>' would be my second choice, at some aesthetical cost.

> Also, I'm not absolutely sure I'm following the code correctly, but ISTM
> that the current mips table printing code can already print strings larger
> than unavailable, like "<invalid float>" in mips_print_fp_register?

 For obvious reasons FP registers are formatted differently:

(gdb) info all-registers
          zero       at       v0       v1       a0       a1       a2       a3
 R0   00000000 fffffff8 77fde8cc 00000000 77ff0000 77fec458 7ffffbd4 7ffffbdc
            t0       t1       t2       t3       t4       t5       t6       t7
 R8   c34e4bff ea5f612e 81010100 2f2f2f2f 7ffffce1 ffffffff f0000000 7ffff4e8
            s0       s1       s2       s3       s4       s5       s6       s7
 R16  78007010 00401360 0051e008 00516188 005161c8 004f0000 00000063 0000006c
            t8       t9       k0       k1       gp       sp       s8       ra
 R24  77fdf64c 00401360 7ffff67c 00000000 78007010 7ffffbd0 00000061 77fcc97c
        status       lo       hi badvaddr    cause       pc
      0100a413 008cbcee 00000006 77f83e90 00800024 00401360
 f0:  0x7ff80000 flt: nan               dbl: nan
 f1:  0x7ff80000 flt: nan
 f2:  0x7ff80000 flt: nan               dbl: nan
 f3:  0x7ff80000 flt: nan
 f4:  0x7ff80000 flt: nan               dbl: nan
 f5:  0x7ff80000 flt: nan
 f6:  0x7ff80000 flt: nan               dbl: nan
 f7:  0x7ff80000 flt: nan
 f8:  0x7ff80000 flt: nan               dbl: nan
 f9:  0x7ff80000 flt: nan
 f10: 0x7ff80000 flt: nan               dbl: nan
 f11: 0x7ff80000 flt: nan
 f12: 0x7ff80000 flt: nan               dbl: nan
 f13: 0x7ff80000 flt: nan
 f14: 0x7ff80000 flt: nan               dbl: nan
 f15: 0x7ff80000 flt: nan
 f16: 0x7ff80000 flt: nan               dbl: nan
 f17: 0x7ff80000 flt: nan
 f18: 0x7ff80000 flt: nan               dbl: nan
 f19: 0x7ff80000 flt: nan
 f20: 0x7ff80000 flt: nan               dbl: nan
 f21: 0x7ff80000 flt: nan
 f22: 0x7ff80000 flt: nan               dbl: nan
 f23: 0x7ff80000 flt: nan
 f24: 0x7ff80000 flt: nan               dbl: nan
 f25: 0x7ff80000 flt: nan
 f26: 0x7ff80000 flt: nan               dbl: nan
 f27: 0x7ff80000 flt: nan
 f28: 0x7ff80000 flt: nan               dbl: nan
 f29: 0x7ff80000 flt: nan
 f30: 0x7ff80000 flt: nan               dbl: nan
 f31: 0x7ff80000 flt: nan
          fcsr      fir  restart
      00000000 00f30000 00000000
(gdb) 

so there's enough space for unusual interpretations.

> I also noticed that there's code in print_gp_register_row that handles
> printing large registers separately:
> 
>       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))
> 	{
> ...
> 	  /* Print this register on a row by itself.  */
> 
> supposedly to avoid column misalignment.  Maybe <unavailable> registers could
> be printed on a row by themselves too?

 Hmm, I've investigated the origin of this piece and it has turned out to 
be quite recent in MIPS port's history, introduced as a hack with commit 
0cc93a06b3ed ("Avoid MIPS port breakage on large registers"), 
<https://sourceware.org/ml/gdb-patches/2007-05/msg00334.html>, to handle 
the artificial $restart register, which is always printed last (and as 
such doesn't break output formatting in a considerable way).

 See the justification downthread and TBH I wouldn't let this change in if 
I were asked to approve it today.  Instead I would request to implement it 
like I did for $fcsr and $fir with commit 78b86327b530 ("mips-tdep: Make 
FCRs always 32-bit") -- though there's something fishy going on there 
anyway as we (still) do not have proper support in place for a debug 
scenario where the ptrace(2) caller has been built for an ABI different 
from the debuggee's ABI -- and the problem is on the kernel side.  Though 
by choosing the ptrace(2) syscall number matching the debuggee's ABI 
(there's one for each) I think it would be avoided; but then there's no 
libc support for doing that.

 So this is not a path I want to follow; I'd rather drop this piece of 
code and handle the case of $restart correctly.

> Note that while the mips table format is more compact, the generic
> format has the advantage that is prints symbol information, like:
> 
> >> rip            0xffffffff8058bb12       0xffffffff8058bb12 <sched_switch+1218>

 With 32 GPRs + HI/LO + PC (which are the minimum subset of architectural 
registers available to user mode software) that does not fit in a standard 
80x24 terminal and IMHO overwhelms the reader with a flood of information 
-- which I believe is also the reason why FPRs are not shown by default.

> (Maybe the ideal would be if the generic code learned about MIPS compact
> format and users could pick the format they want with a format switch
> like "info register /c" or some such.  But again, that'd be a larger
> change which I'd not like to require.)

 That sounds to me like an idea worth considering, assuming that the 
current target-specific defaults are retained.

  Maciej

Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.

[Pedro Alves]

On 04/27/2017 08:37 PM, Maciej W. Rozycki wrote:
> On Thu, 27 Apr 2017, Pedro Alves wrote:
> 
>>> The use of angle brackets with the latter variant is consistent with other 
>>> targets, so I might have just a slight preference for it, but I'll be 
>>> happy to accept input from other people.
>>
>> FWIW, I'd find using anything but <unavailable> when that's what
>> GDB means to be making the port be gratuitously different.
>> "<unavailable>" always means the same thing in gdb -- gdb knows
>> the value materially exists, but it can't get at it for some
>> reason (e.g., ptrace does not expose it, a core dump is trimmed, value
>> not collect in a trace frame, etc.).
> 
>  If you think keeping the word the same across ports is essential, then 
> `<unavl>' would be my second choice, at some aesthetical cost.

I'm not sure that trying to come up with short
variants of these special values manually is the best option.

Another output you could see here is "<not saved>".
GDB prints that if GDB figures out the register is not saved
in a frame (or if the DWARF indicates that, via DW_CFA_undefined).

E.g., with the gdb.mi/mi-reg-undefined.exp testcase binary:

(gdb) b stop_frame
Breakpoint 1 at 0x40059a: file mi-reg-undefined.c, line 21.
(gdb) r
Breakpoint 1, stop_frame () at mi-reg-undefined.c:21
(gdb) up
#1  0x00000000004005aa in first_frame () at mi-reg-undefined.c:26
26      in mi-reg-undefined.c
(gdb) info registers
rax            <not saved>
...

We don't have any arch-independent test for this unfortunately.

Maybe a two-pass approach where the first pass prints
each register to a buffer to find the largest width, and then
use that for column width would be more future proof.  It's also
work better if these magic values are ever translated (i18n).

But TBC, I won't object whatever output you decide is best for MIPS,
and I certainly don't want to impose extra work on John -- getting rid
of that error call is the most important here.
I'm just trying to expose a concern so you can have a more
informed decision.

> 
>> Also, I'm not absolutely sure I'm following the code correctly, but ISTM
>> that the current mips table printing code can already print strings larger
>> than unavailable, like "<invalid float>" in mips_print_fp_register?
> 
>  For obvious reasons FP registers are formatted differently:

OK.  It wasn't actually obvious to me that each FP register was getting
its own column.

>>       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))
>> 	{
>> ...
>> 	  /* Print this register on a row by itself.  */
>>
>> supposedly to avoid column misalignment.  Maybe <unavailable> registers could
>> be printed on a row by themselves too?
> 
>  Hmm, I've investigated the origin of this piece and it has turned out to 
> be quite recent in MIPS port's history, introduced as a hack with commit 

I like how 10 years ago is considered recent.  :-)

Thanks,
Pedro Alves

Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.

[John Baldwin]

On Friday, April 28, 2017 02:51:41 PM Pedro Alves wrote:
> On 04/27/2017 08:37 PM, Maciej W. Rozycki wrote:
> > On Thu, 27 Apr 2017, Pedro Alves wrote:
> > 
> >>> The use of angle brackets with the latter variant is consistent with other 
> >>> targets, so I might have just a slight preference for it, but I'll be 
> >>> happy to accept input from other people.
> >>
> >> FWIW, I'd find using anything but <unavailable> when that's what
> >> GDB means to be making the port be gratuitously different.
> >> "<unavailable>" always means the same thing in gdb -- gdb knows
> >> the value materially exists, but it can't get at it for some
> >> reason (e.g., ptrace does not expose it, a core dump is trimmed, value
> >> not collect in a trace frame, etc.).
> > 
> >  If you think keeping the word the same across ports is essential, then 
> > `<unavl>' would be my second choice, at some aesthetical cost.
> 
> I'm not sure that trying to come up with short
> variants of these special values manually is the best option.
> 
> Another output you could see here is "<not saved>".
> GDB prints that if GDB figures out the register is not saved
> in a frame (or if the DWARF indicates that, via DW_CFA_undefined).

Unfortunately, even "<not saved>" is too long for the 8 character field
for a 32-bit register value.  One perhaps simple option would be to always
use the same table layout for 32-bit vs 64-bit (4 columns x 11 rows).  It
would mean doubling the lines that 'info registers' shows on 32-bit MIPS,
but it would still fit in 24 lines and would probably simplify the
implementation.

However, as you note, the primary goal is removing the error(), and I can
live with any format that is readable.

-- 
John Baldwin

Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.

[John Baldwin]

On Friday, April 28, 2017 09:48:50 AM John Baldwin wrote:
> On Friday, April 28, 2017 02:51:41 PM Pedro Alves wrote:
> > On 04/27/2017 08:37 PM, Maciej W. Rozycki wrote:
> > > On Thu, 27 Apr 2017, Pedro Alves wrote:
> > > 
> > >>> The use of angle brackets with the latter variant is consistent with other 
> > >>> targets, so I might have just a slight preference for it, but I'll be 
> > >>> happy to accept input from other people.
> > >>
> > >> FWIW, I'd find using anything but <unavailable> when that's what
> > >> GDB means to be making the port be gratuitously different.
> > >> "<unavailable>" always means the same thing in gdb -- gdb knows
> > >> the value materially exists, but it can't get at it for some
> > >> reason (e.g., ptrace does not expose it, a core dump is trimmed, value
> > >> not collect in a trace frame, etc.).
> > > 
> > >  If you think keeping the word the same across ports is essential, then 
> > > `<unavl>' would be my second choice, at some aesthetical cost.
> > 
> > I'm not sure that trying to come up with short
> > variants of these special values manually is the best option.
> > 
> > Another output you could see here is "<not saved>".
> > GDB prints that if GDB figures out the register is not saved
> > in a frame (or if the DWARF indicates that, via DW_CFA_undefined).
> 
> Unfortunately, even "<not saved>" is too long for the 8 character field
> for a 32-bit register value.  One perhaps simple option would be to always
> use the same table layout for 32-bit vs 64-bit (4 columns x 11 rows).  It
> would mean doubling the lines that 'info registers' shows on 32-bit MIPS,
> but it would still fit in 24 lines and would probably simplify the
> implementation.
> 
> However, as you note, the primary goal is removing the error(), and I can
> live with any format that is readable.

Any other thoughts on "<absent>" vs "<unavl>" vs always using the 64-bit
table layout vs <insert other option here>?

-- 
John Baldwin

Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.

[Maciej W. Rozycki]

On Fri, 5 May 2017, John Baldwin wrote:

> > However, as you note, the primary goal is removing the error(), and I can
> > live with any format that is readable.
> 
> Any other thoughts on "<absent>" vs "<unavl>" vs always using the 64-bit
> table layout vs <insert other option here>?

 I've been busy with binutils stuff recently, sorry.  I'll try to have a 
look again next week.

  Maciej

Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.

[John Baldwin]

On Friday, May 05, 2017 09:08:01 PM Maciej W. Rozycki wrote:
> On Fri, 5 May 2017, John Baldwin wrote:
> 
> > > However, as you note, the primary goal is removing the error(), and I can
> > > live with any format that is readable.
> > 
> > Any other thoughts on "<absent>" vs "<unavl>" vs always using the 64-bit
> > table layout vs <insert other option here>?
> 
>  I've been busy with binutils stuff recently, sorry.  I'll try to have a 
> look again next week.

Ping?

-- 
John Baldwin

Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.

[Maciej W. Rozycki]

On Mon, 12 Jun 2017, John Baldwin wrote:

> > > > However, as you note, the primary goal is removing the error(), and I can
> > > > live with any format that is readable.
> > > 
> > > Any other thoughts on "<absent>" vs "<unavl>" vs always using the 64-bit
> > > table layout vs <insert other option here>?
> > 
> >  I've been busy with binutils stuff recently, sorry.  I'll try to have a 
> > look again next week.
> 
> Ping?

 Given the lack of consensus otherwise I think ` <unavl>' is the only way 
to go.  I hope there is consensus about this variant being better than 
`<unavail'.

 AFAIK output from `info registers' is not meant for machine consumption 
and as such does not constitute a protocol.  Therefore we can freely 
change it again anytime if we find a better alternative, as long as it is 
comprehensible to a human.

 Thank you for patience and apologies for the long RTT.

  Maciej

Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.

[John Baldwin]

On 6/15/17 7:50 PM, Maciej W. Rozycki wrote:
> On Mon, 12 Jun 2017, John Baldwin wrote:
> 
>>>>> However, as you note, the primary goal is removing the error(), and I can
>>>>> live with any format that is readable.
>>>>
>>>> Any other thoughts on "<absent>" vs "<unavl>" vs always using the 64-bit
>>>> table layout vs <insert other option here>?
>>>
>>>  I've been busy with binutils stuff recently, sorry.  I'll try to have a 
>>> look again next week.
>>
>> Ping?
> 
>  Given the lack of consensus otherwise I think ` <unavl>' is the only way 
> to go.  I hope there is consensus about this variant being better than 
> `<unavail'.

Yes, I think this is better certainly. 
>  Thank you for patience and apologies for the long RTT.

No problem.  I'm on vacation-ish so it might be a while before I test and
post the updated patch.

-- 
John Baldwin