[PATCH v3 0/3] gdbserver: Fix support for S390 TDB

[PATCH v3 0/3] gdbserver: Fix support for S390 TDB

[Andreas Arnez]

This is a follow-up to patch 2 of the series called "S390: Fixes for
gdbserver on targets with TDB".  The last version of that patch was
here:

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

This new version is split up in three patches and contains the following
changes:

* Invalidate the register cache whenever we are about to fetch the
  register values from the inferior.

* Instead of passing NULL to the regset store function upon ENODATA from
  ptrace, just suppress its invocation, since the registers are now
  "unavailable" by default.  Consequently no longer handle NULL in
  s390_store_tdb or arm_store_wmmxregset.

* Suppress calling the register store function upon other errors from
  ptrace as well.

* Rephrase the while()-loops as for()-loops in regsets_fetch_- and
  -_store_inferior_registers, for improved readability.

Tested on S390 and i386.

Andreas Arnez (3):
  gdbserver: Rephrase loops in regsets_fetch/store_inferior_registers
  gdbserver: Prevent stale/random values in register cache
  S390: Fix gdbserver support for TDB

 gdb/gdbserver/linux-low.c      |   38 +++++++++++++-------------------------
 gdb/gdbserver/linux-s390-low.c |   31 +++++++++++++++++++++----------
 gdb/gdbserver/regcache.c       |    5 +++++
 3 files changed, 39 insertions(+), 35 deletions(-)

-- 
1.7.9.5

[PATCH v3 2/3] 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  |    5 +++++
 2 files changed, 11 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..4ea16a6 100644
--- a/gdb/gdbserver/regcache.c
+++ b/gdb/gdbserver/regcache.c
@@ -52,6 +52,11 @@ get_thread_regcache (struct thread_info *thread, int fetch)
       struct thread_info *saved_thread = current_thread;
 
       current_thread = thread;
+#ifndef IN_PROCESS_AGENT
+      /* Invalidate all registers, to prevent stale left-overs.  */
+      memset (regcache->register_status, REG_UNAVAILABLE,
+	      regcache->tdesc->num_registers);
+#endif
       fetch_inferior_registers (regcache, -1);
       current_thread = saved_thread;
       regcache->registers_valid = 1;
-- 
1.7.9.5

Re: [PATCH v3 2/3] gdbserver: Prevent stale/random values in register cache

[Ulrich Weigand]

Andreas Arnez wrote:

> @@ -52,6 +52,11 @@ get_thread_regcache (struct thread_info *thread, int fetch)
>        struct thread_info *saved_thread = current_thread;
>  
>        current_thread = thread;
> +#ifndef IN_PROCESS_AGENT
> +      /* Invalidate all registers, to prevent stale left-overs.  */
> +      memset (regcache->register_status, REG_UNAVAILABLE,
> +	      regcache->tdesc->num_registers);
> +#endif
>        fetch_inferior_registers (regcache, -1);
>        current_thread = saved_thread;
>        regcache->registers_valid = 1;

The whole get_thread_regcache routine is already under #ifndef IN_PROCESS_AGENT,
so the ifdef seems redundant here.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH v3 2/3] gdbserver: Prevent stale/random values in register cache

[Andreas Arnez]

On Wed, Dec 10 2014, Ulrich Weigand wrote:

> Andreas Arnez wrote:
>
>> @@ -52,6 +52,11 @@ get_thread_regcache (struct thread_info *thread, int fetch)
>>        struct thread_info *saved_thread = current_thread;
>>  
>>        current_thread = thread;
>> +#ifndef IN_PROCESS_AGENT
>> +      /* Invalidate all registers, to prevent stale left-overs.  */
>> +      memset (regcache->register_status, REG_UNAVAILABLE,
>> +	      regcache->tdesc->num_registers);
>> +#endif
>>        fetch_inferior_registers (regcache, -1);
>>        current_thread = saved_thread;
>>        regcache->registers_valid = 1;
>
> The whole get_thread_regcache routine is already under #ifndef IN_PROCESS_AGENT,
> so the ifdef seems redundant here.

Correct ;-)  Will be removed in the next version.

[PATCH v3 1/3] 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.7.9.5

[PATCH v3 3/3] 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 changes in
linux-low.c are needed for treating an unavailable TDB correctly and
without warnings.

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.
	(regsets_store_inferior_registers): Skip regsets without a
	fill_function.
	* linux-s390-low.c (s390_fill_last_break): Remove.
	(s390_store_tdb): New.
	(s390_regsets): Set fill_function to NULL for NT_S390_LAST_BREAK.
	Add regset for NT_S390_TDB.
	(s390_arch_setup): Use regset's size instead of fill_function for
	loop end condition.
---
 gdb/gdbserver/linux-low.c      |    5 +++--
 gdb/gdbserver/linux-s390-low.c |   31 +++++++++++++++++++++----------
 2 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index c1b53ff..6ace4b0 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4256,7 +4256,7 @@ regsets_fetch_inferior_registers (struct regsets_info *regsets_info,
 		 this process mode.  */
 	      disable_regset (regsets_info, regset);
 	    }
-	  else
+	  else if (errno != ENODATA)
 	    {
 	      char s[256];
 	      sprintf (s, "ptrace(regsets_fetch_inferior_registers) PID=%d",
@@ -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..40fb61c 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;
@@ -316,13 +310,30 @@ 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; 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 },
+  /* TDB is read-only.  */
+  { PTRACE_GETREGSET, -1, NT_S390_TDB, 0, EXTENDED_REGS,
+    NULL, s390_store_tdb },
   { 0, 0, 0, -1, -1, NULL, NULL }
 };
 
@@ -485,7 +496,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.7.9.5

Re: [PATCH v3 3/3] S390: Fix gdbserver support for TDB

[Ulrich Weigand]

Andreas Arnez wrote:

> @@ -4256,7 +4256,7 @@ regsets_fetch_inferior_registers (struct regsets_info *regsets_info,
>  		 this process mode.  */
>  	      disable_regset (regsets_info, regset);
>  	    }
> -	  else
> +	  else if (errno != ENODATA)
>  	    {
>  	      char s[256];
>  	      sprintf (s, "ptrace(regsets_fetch_inferior_registers) PID=%d",

It would be better to keep the comment explaining in what situations the kernel
can return ENODATA that you had in a previous iteration of the patch set.

> @@ -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;

This (and the related s390_fill_last_break change) is really an independent
change; maybe do it as a separate patch?  For consistency, we might likewise
want to allow regsets with NULL store_function (in regsets_fetch_inferior_registers).

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH v3 3/3] S390: Fix gdbserver support for TDB

[Andreas Arnez]

On Wed, Dec 10 2014, Ulrich Weigand wrote:

> Andreas Arnez wrote:
>
>> @@ -4256,7 +4256,7 @@ regsets_fetch_inferior_registers (struct regsets_info *regsets_info,
>>  		 this process mode.  */
>>  	      disable_regset (regsets_info, regset);
>>  	    }
>> -	  else
>> +	  else if (errno != ENODATA)
>>  	    {
>>  	      char s[256];
>>  	      sprintf (s, "ptrace(regsets_fetch_inferior_registers) PID=%d",
>
> It would be better to keep the comment explaining in what situations the kernel
> can return ENODATA that you had in a previous iteration of the patch set.

OK, I will add this comment in the next version:

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

>
>> @@ -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;
>
> This (and the related s390_fill_last_break change) is really an independent
> change; maybe do it as a separate patch?  For consistency, we might likewise
> want to allow regsets with NULL store_function (in regsets_fetch_inferior_registers).

OK, I will split the change to allow regsets with a NULL store_function
and the exploitation for last_break out into a separate patch
("gdbserver: Support read-only regsets").

And for consistency I can also allow regsets with a NULL store_function.
In that case I *think* we should suppress the invocation of ptrace with
the regset's get_request in regsets_store_inferior_registers().  In
other words, instead of read-modify-write we would then only do the
write.  Agreed?

In the original patch I omitted support for write-only regsets because I
do not see a use case for this; and if there will be a use case in the
future, I am not sure that the approach described above will really be
appropriate.

Re: [PATCH v3 3/3] S390: Fix gdbserver support for TDB

[Ulrich Weigand]

Andreas Arnez wrote:
> On Wed, Dec 10 2014, Ulrich Weigand wrote:
> > This (and the related s390_fill_last_break change) is really an independent
> > change; maybe do it as a separate patch?  For consistency, we might likewise
> > want to allow regsets with NULL store_function (in regsets_fetch_inferior_registers).
> 
> OK, I will split the change to allow regsets with a NULL store_function
> and the exploitation for last_break out into a separate patch
> ("gdbserver: Support read-only regsets").
> 
> And for consistency I can also allow regsets with a NULL store_function.
> In that case I *think* we should suppress the invocation of ptrace with
> the regset's get_request in regsets_store_inferior_registers().  In
> other words, instead of read-modify-write we would then only do the
> write.  Agreed?

Ah, yes, the situation isn't symmetrical; I didn't think of that.

> In the original patch I omitted support for write-only regsets because I
> do not see a use case for this; and if there will be a use case in the
> future, I am not sure that the approach described above will really be
> appropriate.

Given that, it's probably best to indeed wait until we have a use case
(if ever), so I withdraw my suggestion.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com