* [PATCH 1/4] Cleanups to FreeBSD/mips native register operations.
2017-04-12 18:37 [PATCH 0/4] Cleanups for MIPS registers (mostly FreeBSD-specific) John Baldwin
@ 2017-04-12 18:37 ` John Baldwin
2017-04-13 15:48 ` Luis Machado
2017-04-12 18:37 ` [PATCH 2/4] Use mips_regnum instead of constants for FreeBSD/mips " John Baldwin
` (2 subsequent siblings)
3 siblings, 1 reply; 30+ messages in thread
From: John Baldwin @ 2017-04-12 18:37 UTC (permalink / raw)
To: gdb-patches
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, ®s, 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) ®s, 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/4] Cleanups to FreeBSD/mips native register operations.
2017-04-12 18:37 ` [PATCH 1/4] Cleanups to FreeBSD/mips native register operations John Baldwin
@ 2017-04-13 15:48 ` Luis Machado
2017-04-14 18:02 ` John Baldwin
0 siblings, 1 reply; 30+ messages in thread
From: Luis Machado @ 2017-04-13 15:48 UTC (permalink / raw)
To: John Baldwin, gdb-patches
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, ®s, 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) ®s, 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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/4] Cleanups to FreeBSD/mips native register operations.
2017-04-13 15:48 ` Luis Machado
@ 2017-04-14 18:02 ` John Baldwin
0 siblings, 0 replies; 30+ messages in thread
From: John Baldwin @ 2017-04-14 18:02 UTC (permalink / raw)
To: Luis Machado; +Cc: gdb-patches
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, ®s, 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 2/4] Use mips_regnum instead of constants for FreeBSD/mips register operations.
2017-04-12 18:37 [PATCH 0/4] Cleanups for MIPS registers (mostly FreeBSD-specific) John Baldwin
2017-04-12 18:37 ` [PATCH 1/4] Cleanups to FreeBSD/mips native register operations John Baldwin
@ 2017-04-12 18:37 ` John Baldwin
2017-04-13 16:31 ` Luis Machado
2017-04-12 18:37 ` [PATCH 3/4] Consistently use fprintf_filtered when displaying MIPS registers John Baldwin
2017-04-12 18:45 ` [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers John Baldwin
3 siblings, 1 reply; 30+ messages in thread
From: John Baldwin @ 2017-04-12 18:37 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/4] Use mips_regnum instead of constants for FreeBSD/mips register operations.
2017-04-12 18:37 ` [PATCH 2/4] Use mips_regnum instead of constants for FreeBSD/mips " John Baldwin
@ 2017-04-13 16:31 ` Luis Machado
0 siblings, 0 replies; 30+ messages in thread
From: Luis Machado @ 2017-04-13 16:31 UTC (permalink / raw)
To: John Baldwin, gdb-patches
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 3/4] Consistently use fprintf_filtered when displaying MIPS registers.
2017-04-12 18:37 [PATCH 0/4] Cleanups for MIPS registers (mostly FreeBSD-specific) John Baldwin
2017-04-12 18:37 ` [PATCH 1/4] Cleanups to FreeBSD/mips native register operations John Baldwin
2017-04-12 18:37 ` [PATCH 2/4] Use mips_regnum instead of constants for FreeBSD/mips " John Baldwin
@ 2017-04-12 18:37 ` John Baldwin
2017-04-13 16:29 ` Luis Machado
2017-04-12 18:45 ` [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers John Baldwin
3 siblings, 1 reply; 30+ messages in thread
From: John Baldwin @ 2017-04-12 18:37 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] Consistently use fprintf_filtered when displaying MIPS registers.
2017-04-12 18:37 ` [PATCH 3/4] Consistently use fprintf_filtered when displaying MIPS registers John Baldwin
@ 2017-04-13 16:29 ` Luis Machado
2017-04-27 16:05 ` Pedro Alves
0 siblings, 1 reply; 30+ messages in thread
From: Luis Machado @ 2017-04-13 16:29 UTC (permalink / raw)
To: John Baldwin, gdb-patches
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] Consistently use fprintf_filtered when displaying MIPS registers.
2017-04-13 16:29 ` Luis Machado
@ 2017-04-27 16:05 ` Pedro Alves
2017-04-28 16:52 ` John Baldwin
0 siblings, 1 reply; 30+ messages in thread
From: Pedro Alves @ 2017-04-27 16:05 UTC (permalink / raw)
To: Luis Machado, John Baldwin, gdb-patches
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] Consistently use fprintf_filtered when displaying MIPS registers.
2017-04-27 16:05 ` Pedro Alves
@ 2017-04-28 16:52 ` John Baldwin
0 siblings, 0 replies; 30+ messages in thread
From: John Baldwin @ 2017-04-28 16:52 UTC (permalink / raw)
To: Pedro Alves; +Cc: Luis Machado, gdb-patches
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.
2017-04-12 18:37 [PATCH 0/4] Cleanups for MIPS registers (mostly FreeBSD-specific) John Baldwin
` (2 preceding siblings ...)
2017-04-12 18:37 ` [PATCH 3/4] Consistently use fprintf_filtered when displaying MIPS registers John Baldwin
@ 2017-04-12 18:45 ` John Baldwin
2017-04-13 16:37 ` Luis Machado
3 siblings, 1 reply; 30+ messages in thread
From: John Baldwin @ 2017-04-12 18:45 UTC (permalink / raw)
To: gdb-patches
'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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.
2017-04-12 18:45 ` [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers John Baldwin
@ 2017-04-13 16:37 ` Luis Machado
2017-04-14 18:02 ` John Baldwin
0 siblings, 1 reply; 30+ messages in thread
From: Luis Machado @ 2017-04-13 16:37 UTC (permalink / raw)
To: John Baldwin, gdb-patches
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?
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.
2017-04-13 16:37 ` Luis Machado
@ 2017-04-14 18:02 ` John Baldwin
2017-04-15 16:02 ` Maciej W. Rozycki
0 siblings, 1 reply; 30+ messages in thread
From: John Baldwin @ 2017-04-14 18:02 UTC (permalink / raw)
To: Luis Machado; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.
2017-04-14 18:02 ` John Baldwin
@ 2017-04-15 16:02 ` Maciej W. Rozycki
2017-04-15 17:36 ` John Baldwin
0 siblings, 1 reply; 30+ messages in thread
From: Maciej W. Rozycki @ 2017-04-15 16:02 UTC (permalink / raw)
To: John Baldwin; +Cc: Luis Machado, gdb-patches
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.
2017-04-15 16:02 ` Maciej W. Rozycki
@ 2017-04-15 17:36 ` John Baldwin
2017-04-15 22:07 ` Maciej W. Rozycki
0 siblings, 1 reply; 30+ messages in thread
From: John Baldwin @ 2017-04-15 17:36 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: Luis Machado, gdb-patches
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.
2017-04-15 17:36 ` John Baldwin
@ 2017-04-15 22:07 ` Maciej W. Rozycki
2017-04-17 18:27 ` John Baldwin
0 siblings, 1 reply; 30+ messages in thread
From: Maciej W. Rozycki @ 2017-04-15 22:07 UTC (permalink / raw)
To: John Baldwin; +Cc: Luis Machado, gdb-patches
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.
2017-04-15 22:07 ` Maciej W. Rozycki
@ 2017-04-17 18:27 ` John Baldwin
2017-04-18 21:33 ` Maciej W. Rozycki
0 siblings, 1 reply; 30+ messages in thread
From: John Baldwin @ 2017-04-17 18:27 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: Luis Machado, gdb-patches
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.
2017-04-17 18:27 ` John Baldwin
@ 2017-04-18 21:33 ` Maciej W. Rozycki
2017-04-18 22:19 ` John Baldwin
2017-04-27 15:50 ` Pedro Alves
0 siblings, 2 replies; 30+ messages in thread
From: Maciej W. Rozycki @ 2017-04-18 21:33 UTC (permalink / raw)
To: John Baldwin; +Cc: Luis Machado, gdb-patches
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.
2017-04-18 21:33 ` Maciej W. Rozycki
@ 2017-04-18 22:19 ` John Baldwin
2017-04-25 21:02 ` John Baldwin
2017-04-27 0:49 ` Maciej W. Rozycki
2017-04-27 15:50 ` Pedro Alves
1 sibling, 2 replies; 30+ messages in thread
From: John Baldwin @ 2017-04-18 22:19 UTC (permalink / raw)
To: gdb-patches; +Cc: Maciej W. Rozycki, Luis Machado
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.
2017-04-18 22:19 ` John Baldwin
@ 2017-04-25 21:02 ` John Baldwin
2017-04-27 0:49 ` Maciej W. Rozycki
1 sibling, 0 replies; 30+ messages in thread
From: John Baldwin @ 2017-04-25 21:02 UTC (permalink / raw)
To: gdb-patches; +Cc: Maciej W. Rozycki, Luis Machado
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.
2017-04-18 22:19 ` John Baldwin
2017-04-25 21:02 ` John Baldwin
@ 2017-04-27 0:49 ` Maciej W. Rozycki
1 sibling, 0 replies; 30+ messages in thread
From: Maciej W. Rozycki @ 2017-04-27 0:49 UTC (permalink / raw)
To: John Baldwin; +Cc: gdb-patches, Luis Machado
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.
2017-04-18 21:33 ` Maciej W. Rozycki
2017-04-18 22:19 ` John Baldwin
@ 2017-04-27 15:50 ` Pedro Alves
2017-04-27 19:38 ` Maciej W. Rozycki
1 sibling, 1 reply; 30+ messages in thread
From: Pedro Alves @ 2017-04-27 15:50 UTC (permalink / raw)
To: Maciej W. Rozycki, John Baldwin; +Cc: Luis Machado, gdb-patches
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.
2017-04-27 15:50 ` Pedro Alves
@ 2017-04-27 19:38 ` Maciej W. Rozycki
2017-04-28 13:51 ` Pedro Alves
0 siblings, 1 reply; 30+ messages in thread
From: Maciej W. Rozycki @ 2017-04-27 19:38 UTC (permalink / raw)
To: Pedro Alves; +Cc: John Baldwin, Luis Machado, gdb-patches
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.
2017-04-27 19:38 ` Maciej W. Rozycki
@ 2017-04-28 13:51 ` Pedro Alves
2017-04-28 16:52 ` John Baldwin
0 siblings, 1 reply; 30+ messages in thread
From: Pedro Alves @ 2017-04-28 13:51 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: John Baldwin, Luis Machado, gdb-patches
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.
2017-04-28 13:51 ` Pedro Alves
@ 2017-04-28 16:52 ` John Baldwin
2017-05-05 19:51 ` John Baldwin
0 siblings, 1 reply; 30+ messages in thread
From: John Baldwin @ 2017-04-28 16:52 UTC (permalink / raw)
To: gdb-patches; +Cc: Pedro Alves, Maciej W. Rozycki, Luis Machado
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.
2017-04-28 16:52 ` John Baldwin
@ 2017-05-05 19:51 ` John Baldwin
2017-05-05 20:08 ` Maciej W. Rozycki
0 siblings, 1 reply; 30+ messages in thread
From: John Baldwin @ 2017-05-05 19:51 UTC (permalink / raw)
To: gdb-patches; +Cc: Pedro Alves, Maciej W. Rozycki, Luis Machado
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.
2017-05-05 19:51 ` John Baldwin
@ 2017-05-05 20:08 ` Maciej W. Rozycki
2017-06-12 18:47 ` John Baldwin
0 siblings, 1 reply; 30+ messages in thread
From: Maciej W. Rozycki @ 2017-05-05 20:08 UTC (permalink / raw)
To: John Baldwin; +Cc: gdb-patches, Pedro Alves, Luis Machado
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.
2017-05-05 20:08 ` Maciej W. Rozycki
@ 2017-06-12 18:47 ` John Baldwin
2017-06-15 23:50 ` Maciej W. Rozycki
0 siblings, 1 reply; 30+ messages in thread
From: John Baldwin @ 2017-06-12 18:47 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: gdb-patches, Pedro Alves, Luis Machado
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.
2017-06-12 18:47 ` John Baldwin
@ 2017-06-15 23:50 ` Maciej W. Rozycki
2017-06-16 16:17 ` John Baldwin
0 siblings, 1 reply; 30+ messages in thread
From: Maciej W. Rozycki @ 2017-06-15 23:50 UTC (permalink / raw)
To: John Baldwin; +Cc: gdb-patches, Pedro Alves, Luis Machado
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers.
2017-06-15 23:50 ` Maciej W. Rozycki
@ 2017-06-16 16:17 ` John Baldwin
0 siblings, 0 replies; 30+ messages in thread
From: John Baldwin @ 2017-06-16 16:17 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: gdb-patches, Pedro Alves, Luis Machado
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
^ permalink raw reply [flat|nested] 30+ messages in thread