Message ID | 20170412183727.22483-2-jhb@FreeBSD.org |
---|---|
State | New |
Headers | show |
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.
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;