[PATCH v4 0/4] gdbserver: Fix support for S390 TDB

[PATCH v4 0/4] gdbserver: Fix support for S390 TDB

[Andreas Arnez]

Last version here:

  https://sourceware.org/ml/gdb-patches/2014-12/msg00193.html

New in v4:

* Removed the #ifndef IN_PROCESS_AGENT in get_thread_regcache, since
  the whole routine is already under the same #ifndef.

* Added a comment to the handling of ENODATA from ptrace.

* Split out support for read-only regsets into a separate patch (now
  patch #3).


Andreas Arnez (4):
  gdbserver: Rephrase loops in regsets_fetch/store_inferior_registers
  gdbserver: Prevent stale/random values in register cache
  gdbserver: Support read-only regsets in linux-low.c
  S390: Fix gdbserver support for TDB

 gdb/gdbserver/linux-low.c      | 42 ++++++++++++++++++------------------------
 gdb/gdbserver/linux-s390-low.c | 31 +++++++++++++++++++++----------
 gdb/gdbserver/regcache.c       |  3 +++
 3 files changed, 42 insertions(+), 34 deletions(-)

-- 
1.8.4.2

[PATCH v4 1/4] gdbserver: Rephrase loops in regsets_fetch/store_inferior_registers

[Andreas Arnez]

Replace the while-loops in linux-low.c that iterate over regsets by
for-loops.  This makes it clearer what is iterated over.  Also, since
"continue" now moves on to the next iteration without having to
increment the regset pointer first, the code is slightly reduced.

In case of EIO the old code did not increment the regset pointer, but
iterated over the same (now disabled) regset again.  This extra
iteration is now avoided.

gdb/gdbserver/ChangeLog:

	* linux-low.c (regsets_fetch_inferior_registers): Rephrase
	while-loop as for-loop.
	(regsets_store_inferior_registers): Likewise.
---
 gdb/gdbserver/linux-low.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 01f11b7..164b0f6 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4221,19 +4221,14 @@ regsets_fetch_inferior_registers (struct regsets_info *regsets_info,
   int pid;
   struct iovec iov;
 
-  regset = regsets_info->regsets;
-
   pid = lwpid_of (current_thread);
-  while (regset->size >= 0)
+  for (regset = regsets_info->regsets; regset->size >= 0; regset++)
     {
       void *buf, *data;
       int nt_type, res;
 
       if (regset->size == 0 || regset_disabled (regsets_info, regset))
-	{
-	  regset ++;
-	  continue;
-	}
+	continue;
 
       buf = xmalloc (regset->size);
 
@@ -4274,7 +4269,6 @@ regsets_fetch_inferior_registers (struct regsets_info *regsets_info,
       else if (regset->type == GENERAL_REGS)
 	saw_general_regs = 1;
       regset->store_function (regcache, buf);
-      regset ++;
       free (buf);
     }
   if (saw_general_regs)
@@ -4292,19 +4286,14 @@ regsets_store_inferior_registers (struct regsets_info *regsets_info,
   int pid;
   struct iovec iov;
 
-  regset = regsets_info->regsets;
-
   pid = lwpid_of (current_thread);
-  while (regset->size >= 0)
+  for (regset = regsets_info->regsets; regset->size >= 0; regset++)
     {
       void *buf, *data;
       int nt_type, res;
 
       if (regset->size == 0 || regset_disabled (regsets_info, regset))
-	{
-	  regset ++;
-	  continue;
-	}
+	continue;
 
       buf = xmalloc (regset->size);
 
@@ -4350,8 +4339,6 @@ regsets_store_inferior_registers (struct regsets_info *regsets_info,
 	      /* If we get EIO on a regset, do not try it again for
 		 this process mode.  */
 	      disable_regset (regsets_info, regset);
-	      free (buf);
-	      continue;
 	    }
 	  else if (errno == ESRCH)
 	    {
@@ -4369,7 +4356,6 @@ regsets_store_inferior_registers (struct regsets_info *regsets_info,
 	}
       else if (regset->type == GENERAL_REGS)
 	saw_general_regs = 1;
-      regset ++;
       free (buf);
     }
   if (saw_general_regs)
-- 
1.8.4.2

Re: [PATCH v4 1/4] gdbserver: Rephrase loops in regsets_fetch/store_inferior_registers

[Pedro Alves]

On 12/12/2014 10:16 AM, Andreas Arnez wrote:
> Replace the while-loops in linux-low.c that iterate over regsets by
> for-loops.  This makes it clearer what is iterated over.  Also, since
> "continue" now moves on to the next iteration without having to
> increment the regset pointer first, the code is slightly reduced.
> 
> In case of EIO the old code did not increment the regset pointer, but
> iterated over the same (now disabled) regset again.  This extra
> iteration is now avoided.

Nice.  Looks good to me.

Thanks,
Pedro Alves

[PATCH v4 3/4] gdbserver: Support read-only regsets in linux-low.c

[Andreas Arnez]

For GNU/Linux targets using the regsets interface, this change
supports regsets that can be read but not written.  The S390 "last
break" regset is an example.  So far it had been defined with
regset->set_request == PTRACE_GETREGSET, such that the respective
ptrace call does not cause any harm.  Now we just skip the whole
read/modify/write sequence for regsets that do not define a
fill_function.

gdb/gdbserver/ChangeLog:

	* linux-low.c (regsets_store_inferior_registers): Skip regsets
	without a fill_function.
	* linux-s390-low.c (s390_fill_last_break): Remove.
	(s390_regsets): Set fill_function to NULL for NT_S390_LAST_BREAK.
	(s390_arch_setup): Use regset's size instead of fill_function for
	loop end condition.
---
 gdb/gdbserver/linux-low.c      |  3 ++-
 gdb/gdbserver/linux-s390-low.c | 14 ++++----------
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index c1b53ff..5f62010 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4293,7 +4293,8 @@ regsets_store_inferior_registers (struct regsets_info *regsets_info,
       void *buf, *data;
       int nt_type, res;
 
-      if (regset->size == 0 || regset_disabled (regsets_info, regset))
+      if (regset->size == 0 || regset_disabled (regsets_info, regset)
+	  || regset->fill_function == NULL)
 	continue;
 
       buf = xmalloc (regset->size);
diff --git a/gdb/gdbserver/linux-s390-low.c b/gdb/gdbserver/linux-s390-low.c
index 79fa6c0..9f77f30 100644
--- a/gdb/gdbserver/linux-s390-low.c
+++ b/gdb/gdbserver/linux-s390-low.c
@@ -290,12 +290,6 @@ s390_fill_gregset (struct regcache *regcache, void *buf)
 /* Fill and store functions for extended register sets.  */
 
 static void
-s390_fill_last_break (struct regcache *regcache, void *buf)
-{
-  /* Last break address is read-only.  */
-}
-
-static void
 s390_store_last_break (struct regcache *regcache, const void *buf)
 {
   const char *p;
@@ -318,9 +312,9 @@ s390_store_system_call (struct regcache *regcache, const void *buf)
 
 static struct regset_info s390_regsets[] = {
   { 0, 0, 0, 0, GENERAL_REGS, s390_fill_gregset, NULL },
-  /* Last break address is read-only; do not attempt PTRACE_SETREGSET.  */
-  { PTRACE_GETREGSET, PTRACE_GETREGSET, NT_S390_LAST_BREAK, 0,
-    EXTENDED_REGS, s390_fill_last_break, s390_store_last_break },
+  /* Last break address is read-only; no fill function.  */
+  { PTRACE_GETREGSET, -1, NT_S390_LAST_BREAK, 0, EXTENDED_REGS,
+    NULL, s390_store_last_break },
   { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_S390_SYSTEM_CALL, 0,
     EXTENDED_REGS, s390_fill_system_call, s390_store_system_call },
   { 0, 0, 0, -1, -1, NULL, NULL }
@@ -485,7 +479,7 @@ s390_arch_setup (void)
 #endif
 
   /* Update target_regsets according to available register sets.  */
-  for (regset = s390_regsets; regset->fill_function != NULL; regset++)
+  for (regset = s390_regsets; regset->size >= 0; regset++)
     if (regset->get_request == PTRACE_GETREGSET)
       switch (regset->nt_type)
 	{
-- 
1.8.4.2

Re: [PATCH v4 3/4] gdbserver: Support read-only regsets in linux-low.c

[Pedro Alves]

On 12/12/2014 10:16 AM, Andreas Arnez wrote:
> For GNU/Linux targets using the regsets interface, this change
> supports regsets that can be read but not written.  The S390 "last
> break" regset is an example.  So far it had been defined with
> regset->set_request == PTRACE_GETREGSET, such that the respective
> ptrace call does not cause any harm.  Now we just skip the whole
> read/modify/write sequence for regsets that do not define a
> fill_function.

Looks good to me.

Thanks,
Pedro Alves

[PATCH v4 2/4] gdbserver: Prevent stale/random values in register cache

[Andreas Arnez]

When fetch_inferior_registers does not update all registers, this
patch assures that no stale register values remain in the register
cache.  On Linux platforms using the regsets interface, when one of
the ptrace calls used for fetching the register values returns an
error, this patch also avoids copying the random data returned from
ptrace into the register cache.  All unfetched registers are marked
"unavailable" instead.

gdb/gdbserver/ChangeLog:

	* linux-low.c (regsets_fetch_inferior_registers): Do not invoke
	the regset's store function when ptrace returned an error.
	* regcache.c (get_thread_regcache): Invalidate register cache
	before fetching inferior's registers.
---
 gdb/gdbserver/linux-low.c | 11 ++++++-----
 gdb/gdbserver/regcache.c  |  3 +++
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 164b0f6..c1b53ff 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4255,8 +4255,6 @@ regsets_fetch_inferior_registers (struct regsets_info *regsets_info,
 	      /* If we get EIO on a regset, do not try it again for
 		 this process mode.  */
 	      disable_regset (regsets_info, regset);
-	      free (buf);
-	      continue;
 	    }
 	  else
 	    {
@@ -4266,9 +4264,12 @@ regsets_fetch_inferior_registers (struct regsets_info *regsets_info,
 	      perror (s);
 	    }
 	}
-      else if (regset->type == GENERAL_REGS)
-	saw_general_regs = 1;
-      regset->store_function (regcache, buf);
+      else
+	{
+	  if (regset->type == GENERAL_REGS)
+	    saw_general_regs = 1;
+	  regset->store_function (regcache, buf);
+	}
       free (buf);
     }
   if (saw_general_regs)
diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
index 718ae8c..8c874f0 100644
--- a/gdb/gdbserver/regcache.c
+++ b/gdb/gdbserver/regcache.c
@@ -52,6 +52,9 @@ get_thread_regcache (struct thread_info *thread, int fetch)
       struct thread_info *saved_thread = current_thread;
 
       current_thread = thread;
+      /* Invalidate all registers, to prevent stale left-overs.  */
+      memset (regcache->register_status, REG_UNAVAILABLE,
+	      regcache->tdesc->num_registers);
       fetch_inferior_registers (regcache, -1);
       current_thread = saved_thread;
       regcache->registers_valid = 1;
-- 
1.8.4.2

Re: [PATCH v4 2/4] gdbserver: Prevent stale/random values in register cache

[Pedro Alves]

On 12/12/2014 10:16 AM, Andreas Arnez wrote:
> When fetch_inferior_registers does not update all registers, this
> patch assures that no stale register values remain in the register
> cache.  On Linux platforms using the regsets interface, when one of
> the ptrace calls used for fetching the register values returns an
> error, this patch also avoids copying the random data returned from
> ptrace into the register cache.  All unfetched registers are marked
> "unavailable" instead.

Looks good to me.

Thanks,
Pedro Alves

[PATCH v4 4/4] S390: Fix gdbserver support for TDB

[Andreas Arnez]

This makes gdbserver actually provide values for the TDB registers
when the inferior was stopped in a transaction.  The change in
linux-low.c is needed to suppress the warning for an unavailable TDB.

The test case 's390-tdbregs.exp' passes with this patch and fails
without.

gdb/gdbserver/ChangeLog:

	* linux-low.c (regsets_fetch_inferior_registers): Suppress the
	warning upon ENODATA from ptrace.
	* linux-s390-low.c (s390_store_tdb): New.
	(s390_regsets): Add regset for NT_S390_TDB.
---
 gdb/gdbserver/linux-low.c      |  6 ++++++
 gdb/gdbserver/linux-s390-low.c | 17 +++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 5f62010..5ea9200 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4256,6 +4256,12 @@ regsets_fetch_inferior_registers (struct regsets_info *regsets_info,
 		 this process mode.  */
 	      disable_regset (regsets_info, regset);
 	    }
+	  else if (errno == ENODATA)
+	    {
+	      /* ENODATA may be returned if the regset is currently
+		 not "active".  This can happen in normal operation,
+		 so suppress the warning in this case.  */
+	    }
 	  else
 	    {
 	      char s[256];
diff --git a/gdb/gdbserver/linux-s390-low.c b/gdb/gdbserver/linux-s390-low.c
index 9f77f30..40fb61c 100644
--- a/gdb/gdbserver/linux-s390-low.c
+++ b/gdb/gdbserver/linux-s390-low.c
@@ -310,6 +310,20 @@ s390_store_system_call (struct regcache *regcache, const void *buf)
   supply_register_by_name (regcache, "system_call", buf);
 }
 
+static void
+s390_store_tdb (struct regcache *regcache, const void *buf)
+{
+  int tdb0 = find_regno (regcache->tdesc, "tdb0");
+  int tr0 = find_regno (regcache->tdesc, "tr0");
+  int i;
+
+  for (i = 0; i < 4; i++)
+    supply_register (regcache, tdb0 + i, (const char *) buf + 8 * i);
+
+  for (i = 0; i < 16; i++)
+    supply_register (regcache, tr0 + i, (const char *) buf + 8 * (16 + i));
+}
+
 static struct regset_info s390_regsets[] = {
   { 0, 0, 0, 0, GENERAL_REGS, s390_fill_gregset, NULL },
   /* Last break address is read-only; no fill function.  */
@@ -317,6 +331,9 @@ static struct regset_info s390_regsets[] = {
     NULL, s390_store_last_break },
   { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_S390_SYSTEM_CALL, 0,
     EXTENDED_REGS, s390_fill_system_call, s390_store_system_call },
+  /* TDB is read-only.  */
+  { PTRACE_GETREGSET, -1, NT_S390_TDB, 0, EXTENDED_REGS,
+    NULL, s390_store_tdb },
   { 0, 0, 0, -1, -1, NULL, NULL }
 };
 
-- 
1.8.4.2

Re: [PATCH v4 4/4] S390: Fix gdbserver support for TDB

[Pedro Alves]

On 12/12/2014 10:16 AM, Andreas Arnez wrote:
> This makes gdbserver actually provide values for the TDB registers
> when the inferior was stopped in a transaction.  The change in
> linux-low.c is needed to suppress the warning for an unavailable TDB.
> 
> The test case 's390-tdbregs.exp' passes with this patch and fails
> without.

Looks good to me.

Thanks,
Pedro Alves