[1/4] Cleanups to FreeBSD/mips native register operations.
Commit Message
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(-)
Comments
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.
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.
@@ -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.
@@ -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;