diff mbox

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

Message ID 1432822816-32327-5-git-send-email-yao.qi@linaro.org
State New
Headers show

Commit Message

Yao Qi May 28, 2015, 2:20 p.m. UTC
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(-)

Comments

Doug Evans May 28, 2015, 6:50 p.m. UTC | #1
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
>
Yao Qi May 29, 2015, 1:11 p.m. UTC | #2
Doug Evans <dje@google.com> writes:

> 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).

Yes, this variable have three states, uninitialised (-1), true (1)
and false (0) and that is reason I check "have_ptrace_getregset == 1"
instead of "have_ptrace_getregset".

>
> 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.

I am OK to remove "== 1" from patches #4, #5 and #6.
diff mbox

Patch

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)
+    {
+      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)
     {