Re: [PATCH 4/6] Fetch and store GP registers by PTRACE_{G,S}ETREGSET

[Doug Evans <dje at google dot com>]

On Thu, May 28, 2015 at 7:20 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
> If kernel supports PTRACE_GETREGSET, GDB uses PTRACE_{G,S}ETREGSET
> to fetch and store GP registers.
>
> gdb:
>
> 2015-05-28  Yao Qi  <yao.qi@linaro.org>
>
>         * arm-linux-nat.c (fetch_register): Use PTRACE_GETREGSET.
>         (fetch_regs): Likewise.
>         (store_regs): Use PTRACE_SETREGSET.
>         (store_register): Likewise.
> ---
>  gdb/arm-linux-nat.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 72 insertions(+), 7 deletions(-)
>
> diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c
> index 877559e..0a86ed6 100644
> --- a/gdb/arm-linux-nat.c
> +++ b/gdb/arm-linux-nat.c
> @@ -225,7 +225,18 @@ fetch_register (struct regcache *regcache, int regno)
>    /* Get the thread id for the ptrace call.  */
>    tid = GET_THREAD_ID (inferior_ptid);
>
> -  ret = ptrace (PTRACE_GETREGS, tid, 0, &regs);
> +  if (have_ptrace_getregset == 1)

Hi.

The == 1 in this test hinders readability (to me anyway).
[It occurs here and in 5/6, 6/6.]
The name suggests the variable is a boolean, so I'm
left wondering "Can it have values other than 0/1,
and is the else clause correct for those other values?"

Digging deeper the reader would find the variable is tri-state,
but the initial -1 value should never be seen here (at least
that's the intuitive choice).

If one wanted to add an assert that the value is not -1 here
that'd be ok, though one could also argue it's overkill.
I don't have a preference either way.

But I suggest removing the "== 1" in the test.

> +    {
> +      struct iovec iov;
> +
> +      iov.iov_base = &regs;
> +      iov.iov_len = sizeof (regs);
> +
> +      ret = ptrace (PTRACE_GETREGSET, tid, NT_PRSTATUS, &iov);
> +    }
> +  else
> +    ret = ptrace (PTRACE_GETREGS, tid, 0, &regs);
> +
>    if (ret < 0)
>      {
>        warning (_("Unable to fetch general register."));
> @@ -266,8 +277,19 @@ fetch_regs (struct regcache *regcache)
>
>    /* Get the thread id for the ptrace call.  */
>    tid = GET_THREAD_ID (inferior_ptid);
> -
> -  ret = ptrace (PTRACE_GETREGS, tid, 0, &regs);
> +
> +  if (have_ptrace_getregset == 1)
> +    {
> +      struct iovec iov;
> +
> +      iov.iov_base = &regs;
> +      iov.iov_len = sizeof (regs);
> +
> +      ret = ptrace (PTRACE_GETREGSET, tid, NT_PRSTATUS, &iov);
> +    }
> +  else
> +    ret = ptrace (PTRACE_GETREGS, tid, 0, &regs);
> +
>    if (ret < 0)
>      {
>        warning (_("Unable to fetch general registers."));
> @@ -306,7 +328,18 @@ store_register (const struct regcache *regcache, int regno)
>    tid = GET_THREAD_ID (inferior_ptid);
>
>    /* Get the general registers from the process.  */
> -  ret = ptrace (PTRACE_GETREGS, tid, 0, &regs);
> +  if (have_ptrace_getregset == 1)
> +    {
> +      struct iovec iov;
> +
> +      iov.iov_base = &regs;
> +      iov.iov_len = sizeof (regs);
> +
> +      ret = ptrace (PTRACE_GETREGSET, tid, NT_PRSTATUS, &iov);
> +    }
> +  else
> +    ret = ptrace (PTRACE_GETREGS, tid, 0, &regs);
> +
>    if (ret < 0)
>      {
>        warning (_("Unable to fetch general registers."));
> @@ -322,7 +355,18 @@ store_register (const struct regcache *regcache, int regno)
>      regcache_raw_collect (regcache, ARM_PC_REGNUM,
>                          (char *) &regs[ARM_PC_REGNUM]);
>
> -  ret = ptrace (PTRACE_SETREGS, tid, 0, &regs);
> +  if (have_ptrace_getregset == 1)
> +    {
> +      struct iovec iov;
> +
> +      iov.iov_base = &regs;
> +      iov.iov_len = sizeof (regs);
> +
> +      ret = ptrace (PTRACE_SETREGSET, tid, NT_PRSTATUS, &iov);
> +    }
> +  else
> +    ret = ptrace (PTRACE_SETREGS, tid, 0, &regs);
> +
>    if (ret < 0)
>      {
>        warning (_("Unable to store general register."));
> @@ -340,7 +384,18 @@ store_regs (const struct regcache *regcache)
>    tid = GET_THREAD_ID (inferior_ptid);
>
>    /* Fetch the general registers.  */
> -  ret = ptrace (PTRACE_GETREGS, tid, 0, &regs);
> +  if (have_ptrace_getregset == 1)
> +    {
> +      struct iovec iov;
> +
> +      iov.iov_base = &regs;
> +      iov.iov_len = sizeof (regs);
> +
> +      ret = ptrace (PTRACE_GETREGSET, tid, NT_PRSTATUS, &iov);
> +    }
> +  else
> +    ret = ptrace (PTRACE_GETREGS, tid, 0, &regs);
> +
>    if (ret < 0)
>      {
>        warning (_("Unable to fetch general registers."));
> @@ -357,7 +412,17 @@ store_regs (const struct regcache *regcache)
>      regcache_raw_collect (regcache, ARM_PS_REGNUM,
>                          (char *) &regs[ARM_CPSR_GREGNUM]);
>
> -  ret = ptrace (PTRACE_SETREGS, tid, 0, &regs);
> +  if (have_ptrace_getregset == 1)
> +    {
> +      struct iovec iov;
> +
> +      iov.iov_base = &regs;
> +      iov.iov_len = sizeof (regs);
> +
> +      ret = ptrace (PTRACE_SETREGSET, tid, NT_PRSTATUS, &iov);
> +    }
> +  else
> +    ret = ptrace (PTRACE_SETREGS, tid, 0, &regs);
>
>    if (ret < 0)
>      {
> --
> 1.9.1
>