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

[Luis Machado <lgustavo@codesourcery.com>]

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

Only a few nits.

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

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

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

Pedantically, "... fetches REGNUM".

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

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

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

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

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

If not, then the change looks sane.

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

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

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

Same thing as above, about higher-numbered registers.

Otherwise i have no further comments.