[rfc][3/3] gdbserver bi-arch support: fix s390x partial register access

[rfc][3/3] gdbserver bi-arch support: fix s390x partial register access

[Ulrich Weigand]

Hello,

this is the final patch required to enable bi-arch support for gdbserver on
s390x-linux.  It fixes problems caused by accessing 32-bit register values
from a 64-bit program using the 64-bit ptrace interface and regsets.

When dealing with 32-bit partial register values, for some registers these
need to be placed in the high 32-bit of a 64-bit register, while for others
they need to be placed in the low 32-bit.  There already is a field
left_pad_xfer in the low target structure that allows the low target to
select this -- but this field currently has only a single boolean setting
which is applied to all registers.

This patch changes that field to a function depending on the register
number, and adapts the sole current user (linux-ppc64-low.c).

The second problem is that when *setting* a partial register value, current
code always pads the remainder of the register with zero.  This is a problem
with the PSW mask register on s390x, where one of the bits in the second
half needs to be and remain set; otherwise the PSW is invalid and the
inferior crashes.

The patch fixes this by changing usr_store_inferior_registers by using a
read-modify-write cycle when partially updating a register so that the
bits in the part of the register that is not changed keep their old
values.

Finally, the patch fixes s390_fill_gregset to also handle partial register
fills correctly.

With these changes, and on top of the two preceding patches, I've run a
full GDB test suite run (using gdbserver natively) on s390x-ibm-linux
using both 64-bit and 31-bit target executables.  Native s390-ibm-linux
runs also showed no regressions.

Bye,
Ulrich


ChangeLog:

	* linux-low.h (struct linux_target_ops): Change left_pad_xfer from
	boolean to function returning boolean.
	* linux-low.c (fetch_register): Pass register number into
	the_low_target.left_pad_xfer call.
	(usr_store_inferior_registers): Likewise.  Also, when changing
	partial register contents, do not pad with zero but leave old
	register contents unchanged.

	* linux-ppc64-low.c (ppc_left_pad_xfer): New function.
	(the_low_target): Use it as the_low_target.left_pad_xfer.

	* linux-s390-low.c (s390_left_pad_xfer): New function.
	(the_low_target): Use it as the_low_target.left_pad_xfer.
	(s390_fill_gregset): Handle partial register fills.


diff -urNp gdb-orig/gdb/gdbserver/linux-low.c gdb-head/gdb/gdbserver/linux-low.c
--- gdb-orig/gdb/gdbserver/linux-low.c	2008-01-18 01:00:50.000000000 +0100
+++ gdb-head/gdb/gdbserver/linux-low.c	2008-01-18 01:00:55.000000000 +0100
@@ -1384,7 +1384,7 @@ fetch_register (int regno)
 	  goto error_exit;
 	}
     }
-  if (the_low_target.left_pad_xfer
+  if (the_low_target.left_pad_xfer && the_low_target.left_pad_xfer (regno)
       && register_size (regno) < sizeof (PTRACE_XFER_TYPE))
     supply_register (regno, (buf + sizeof (PTRACE_XFER_TYPE)
 			     - register_size (regno)));
@@ -1431,7 +1431,10 @@ usr_store_inferior_registers (int regno)
 	     & - sizeof (PTRACE_XFER_TYPE);
       buf = alloca (size);
       memset (buf, 0, size);
-      if (the_low_target.left_pad_xfer
+      if (register_size (regno) < sizeof (PTRACE_XFER_TYPE))
+	*(PTRACE_XFER_TYPE *) buf = ptrace (PTRACE_PEEKUSER, inferior_pid,
+					    (PTRACE_ARG3_TYPE) regaddr, 0);
+      if (the_low_target.left_pad_xfer && the_low_target.left_pad_xfer (regno)
 	  && register_size (regno) < sizeof (PTRACE_XFER_TYPE))
 	collect_register (regno, (buf + sizeof (PTRACE_XFER_TYPE)
 				  - register_size (regno)));
diff -urNp gdb-orig/gdb/gdbserver/linux-low.h gdb-head/gdb/gdbserver/linux-low.h
--- gdb-orig/gdb/gdbserver/linux-low.h	2008-01-17 22:28:29.000000000 +0100
+++ gdb-head/gdb/gdbserver/linux-low.h	2008-01-18 01:00:55.000000000 +0100
@@ -71,7 +71,7 @@ struct linux_target_ops
 
   /* Whether to left-pad registers for PEEKUSR/POKEUSR if they are smaller
      than an xfer unit.  */
-  int left_pad_xfer;
+  int (*left_pad_xfer) (int regno);
 
   /* What string to report to GDB when it asks for the architecture,
      or NULL not to answer.  */
diff -urNp gdb-orig/gdb/gdbserver/linux-ppc64-low.c gdb-head/gdb/gdbserver/linux-ppc64-low.c
--- gdb-orig/gdb/gdbserver/linux-ppc64-low.c	2008-01-17 22:28:29.000000000 +0100
+++ gdb-head/gdb/gdbserver/linux-ppc64-low.c	2008-01-18 01:00:55.000000000 +0100
@@ -81,6 +81,12 @@ ppc_set_pc (CORE_ADDR pc)
   supply_register_by_name ("pc", &newpc);
 }
 
+static int
+ppc_left_pad_xfer (int regno)
+{
+  return 1;
+}
+
 /* Correct in either endianness.
    This instruction is "twge r2, r2", which GDB uses as a software
    breakpoint.  */
@@ -187,5 +193,5 @@ struct linux_target_ops the_low_target =
   NULL,
   NULL,
   NULL,
-  1
+  ppc_left_pad_xfer,
 };
diff -urNp gdb-orig/gdb/gdbserver/linux-s390-low.c gdb-head/gdb/gdbserver/linux-s390-low.c
--- gdb-orig/gdb/gdbserver/linux-s390-low.c	2008-01-18 01:00:50.000000000 +0100
+++ gdb-head/gdb/gdbserver/linux-s390-low.c	2008-01-18 01:00:55.000000000 +0100
@@ -79,6 +79,13 @@ s390_cannot_store_register (int regno)
   return 0;
 }
 
+static int
+s390_left_pad_xfer (int regno)
+{
+  return regno == find_regno ("pswa")
+         || (regno >= find_regno ("r0") && regno <= find_regno ("r15"));
+}
+
 /* Provide only a fill function for the general register set.  ps_lgetregs
    will use this for NPTL support.  */
 
@@ -87,7 +94,17 @@ static void s390_fill_gregset (void *buf
   int i;
 
   for (i = 0; i < 34; i++)
-    collect_register (i, (char *) buf + s390_regmap[i]);
+    {
+      int size = register_size (i);
+      int offset = s390_regmap[i];
+
+      memset ((char *) buf + offset, 0, sizeof (long));
+	
+      if (size < sizeof (long) && s390_left_pad_xfer (i))
+	offset += sizeof (long) - size;
+
+      collect_register (i, (char *) buf + offset);
+    }
 }
 
 struct regset_info target_regsets[] = {
@@ -179,5 +196,10 @@ struct linux_target_ops the_low_target =
   NULL,
   s390_breakpoint_len,
   s390_breakpoint_at,
+  NULL,
+  NULL,
+  NULL,
+  NULL,
+  s390_left_pad_xfer,
 };
 
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc][3/3] gdbserver bi-arch support: fix s390x partial register access

[Daniel Jacobowitz]

On Mon, Jan 21, 2008 at 06:46:26PM +0100, Ulrich Weigand wrote:
> The second problem is that when *setting* a partial register value, current
> code always pads the remainder of the register with zero.  This is a problem
> with the PSW mask register on s390x, where one of the bits in the second
> half needs to be and remain set; otherwise the PSW is invalid and the
> inferior crashes.
> 
> The patch fixes this by changing usr_store_inferior_registers by using a
> read-modify-write cycle when partially updating a register so that the
> bits in the part of the register that is not changed keep their old
> values.

What do you think about passing data to the low target in this case?
For MIPS the right bits will be value-dependent - registers must be
sign extended.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc][3/3] gdbserver bi-arch support: fix s390x partial register access

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> What do you think about passing data to the low target in this case?
> For MIPS the right bits will be value-dependent - registers must be
> sign extended.

Hmm, what about the version below?  This should allow the low target
whatever flexibility it requires ...

Bye,
Ulrich


ChangeLog:

	* linux-low.h (struct linux_target_ops): Replace left_pad_xfer field
	by collect_ptrace_register and supply_ptrace_register hooks.
	* linux-low.c (fetch_register): Use supply_ptrace_register callback
	instead of checking for the_low_target.left_pad_xfer.
	(usr_store_inferior_registers): Use collect_ptrace_register callback
	instead of checking for the_low_target.left_pad_xfer.

	* linux-s390-low.c (s390_collect_ptrace_register): New function.
	(s390_supply_ptrace_register): Likewise.
	(s390_fill_gregset): Call s390_collect_ptrace_register.
	(the_low_target): Update.

	* linux-ppc64-low.c (ppc_collect_ptrace_register): New function.
	(ppc_supply_ptrace_register): Likewise.
	(the_low_target): Update.

	* linux-i386-low.c (the_low_target): Update.
	* linux-x86-64-low.c (the_low_target): Update.


diff -urNp gdb-orig/gdb/gdbserver/linux-i386-low.c gdb-head/gdb/gdbserver/linux-i386-low.c
--- gdb-orig/gdb/gdbserver/linux-i386-low.c	2008-01-29 23:21:45.000000000 +0100
+++ gdb-head/gdb/gdbserver/linux-i386-low.c	2008-01-30 00:18:49.000000000 +0100
@@ -205,6 +205,7 @@ struct linux_target_ops the_low_target =
   NULL,
   NULL,
   NULL,
-  0,
+  NULL,
+  NULL,
   "i386"
 };
diff -urNp gdb-orig/gdb/gdbserver/linux-low.c gdb-head/gdb/gdbserver/linux-low.c
--- gdb-orig/gdb/gdbserver/linux-low.c	2008-01-29 23:34:20.000000000 +0100
+++ gdb-head/gdb/gdbserver/linux-low.c	2008-01-30 00:12:02.000000000 +0100
@@ -1391,10 +1391,9 @@ fetch_register (int regno)
 	  goto error_exit;
 	}
     }
-  if (the_low_target.left_pad_xfer
-      && register_size (regno) < sizeof (PTRACE_XFER_TYPE))
-    supply_register (regno, (buf + sizeof (PTRACE_XFER_TYPE)
-			     - register_size (regno)));
+
+  if (the_low_target.supply_ptrace_register)
+    the_low_target.supply_ptrace_register (regno, buf);
   else
     supply_register (regno, buf);
 
@@ -1438,12 +1437,12 @@ usr_store_inferior_registers (int regno)
 	     & - sizeof (PTRACE_XFER_TYPE);
       buf = alloca (size);
       memset (buf, 0, size);
-      if (the_low_target.left_pad_xfer
-	  && register_size (regno) < sizeof (PTRACE_XFER_TYPE))
-	collect_register (regno, (buf + sizeof (PTRACE_XFER_TYPE)
-				  - register_size (regno)));
+
+      if (the_low_target.collect_ptrace_register)
+	the_low_target.collect_ptrace_register (regno, buf);
       else
 	collect_register (regno, buf);
+
       for (i = 0; i < size; i += sizeof (PTRACE_XFER_TYPE))
 	{
 	  errno = 0;
diff -urNp gdb-orig/gdb/gdbserver/linux-low.h gdb-head/gdb/gdbserver/linux-low.h
--- gdb-orig/gdb/gdbserver/linux-low.h	2008-01-29 23:21:45.000000000 +0100
+++ gdb-head/gdb/gdbserver/linux-low.h	2008-01-29 23:55:41.000000000 +0100
@@ -69,9 +69,10 @@ struct linux_target_ops
   int (*stopped_by_watchpoint) (void);
   CORE_ADDR (*stopped_data_address) (void);
 
-  /* Whether to left-pad registers for PEEKUSR/POKEUSR if they are smaller
-     than an xfer unit.  */
-  int left_pad_xfer;
+  /* Hooks to reformat register data for PEEKUSR/POKEUSR (in particular
+     for registers smaller than an xfer unit).  */
+  void (*collect_ptrace_register) (int regno, char *buf);
+  void (*supply_ptrace_register) (int regno, const char *buf);
 
   /* What string to report to GDB when it asks for the architecture,
      or NULL not to answer.  */
diff -urNp gdb-orig/gdb/gdbserver/linux-ppc64-low.c gdb-head/gdb/gdbserver/linux-ppc64-low.c
--- gdb-orig/gdb/gdbserver/linux-ppc64-low.c	2008-01-29 23:21:45.000000000 +0100
+++ gdb-head/gdb/gdbserver/linux-ppc64-low.c	2008-01-30 00:11:13.000000000 +0100
@@ -64,6 +64,26 @@ ppc_cannot_fetch_register (int regno)
   return 0;
 }
 
+static void
+ppc_collect_ptrace_register (int regno, char *buf)
+{
+  int size = register_size (regno);
+  if (size < sizeof (long))
+    collect_register (regno, buf + sizeof (long) - size);
+  else
+    collect_register (regno, buf);
+}
+
+static void
+ppc_supply_ptrace_register (int regno, const char *buf)
+{
+  int size = register_size (regno);
+  if (size < sizeof (long))
+    supply_register (regno, buf + sizeof (long) - size);
+  else
+    supply_register (regno, buf);
+}
+
 static CORE_ADDR
 ppc_get_pc (void)
 {
@@ -187,5 +207,6 @@ struct linux_target_ops the_low_target =
   NULL,
   NULL,
   NULL,
-  1
+  ppc_collect_ptrace_register,
+  ppc_supply_ptrace_register,
 };
diff -urNp gdb-orig/gdb/gdbserver/linux-s390-low.c gdb-head/gdb/gdbserver/linux-s390-low.c
--- gdb-orig/gdb/gdbserver/linux-s390-low.c	2008-01-29 23:34:20.000000000 +0100
+++ gdb-head/gdb/gdbserver/linux-s390-low.c	2008-01-30 00:13:19.000000000 +0100
@@ -79,6 +79,45 @@ s390_cannot_store_register (int regno)
   return 0;
 }
 
+static void
+s390_collect_ptrace_register (int regno, char *buf)
+{
+  int size = register_size (regno);
+  if (size < sizeof (long))
+    {
+      memset (buf, 0, sizeof (long));
+
+      if (regno == find_regno ("pswa")
+	  || (regno >= find_regno ("r0") && regno <= find_regno ("r15")))
+	collect_register (regno, buf + sizeof (long) - size);
+      else
+	collect_register (regno, buf);
+
+      /* When debugging a 32-bit inferior on a 64-bit host, make sure
+	 the 31-bit addressing mode bit is set in the PSW mask.  */
+      if (regno == find_regno ("pswm"))
+	buf[size] |= 0x80;
+    }
+  else
+    collect_register (regno, buf);
+}
+
+static void
+s390_supply_ptrace_register (int regno, const char *buf)
+{
+  int size = register_size (regno);
+  if (size < sizeof (long))
+    {
+      if (regno == find_regno ("pswa")
+	  || (regno >= find_regno ("r0") && regno <= find_regno ("r15")))
+	supply_register (regno, buf + sizeof (long) - size);
+      else
+	supply_register (regno, buf);
+    }
+  else
+    supply_register (regno, buf);
+}
+
 /* Provide only a fill function for the general register set.  ps_lgetregs
    will use this for NPTL support.  */
 
@@ -87,7 +126,7 @@ static void s390_fill_gregset (void *buf
   int i;
 
   for (i = 0; i < 34; i++)
-    collect_register (i, (char *) buf + s390_regmap[i]);
+    s390_collect_ptrace_register (i, (char *) buf + s390_regmap[i]);
 }
 
 struct regset_info target_regsets[] = {
@@ -179,5 +218,11 @@ struct linux_target_ops the_low_target =
   NULL,
   s390_breakpoint_len,
   s390_breakpoint_at,
+  NULL,
+  NULL,
+  NULL,
+  NULL,
+  s390_collect_ptrace_register,
+  s390_supply_ptrace_register,
 };
 
diff -urNp gdb-orig/gdb/gdbserver/linux-x86-64-low.c gdb-head/gdb/gdbserver/linux-x86-64-low.c
--- gdb-orig/gdb/gdbserver/linux-x86-64-low.c	2008-01-29 23:21:45.000000000 +0100
+++ gdb-head/gdb/gdbserver/linux-x86-64-low.c	2008-01-30 00:19:33.000000000 +0100
@@ -179,6 +179,7 @@ struct linux_target_ops the_low_target =
   NULL,
   NULL,
   NULL,
-  0,
+  NULL,
+  NULL,
   "i386:x86-64",
 };


-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc][3/3] gdbserver bi-arch support: fix s390x partial register access

[Daniel Jacobowitz]

On Wed, Jan 30, 2008 at 12:35:17AM +0100, Ulrich Weigand wrote:
> Daniel Jacobowitz wrote:
> 
> > What do you think about passing data to the low target in this case?
> > For MIPS the right bits will be value-dependent - registers must be
> > sign extended.
> 
> Hmm, what about the version below?  This should allow the low target
> whatever flexibility it requires ...

Looks good to me.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc][3/3] gdbserver bi-arch support: fix s390x partial register access

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Wed, Jan 30, 2008 at 12:35:17AM +0100, Ulrich Weigand wrote:
> > Daniel Jacobowitz wrote:
> > 
> > > What do you think about passing data to the low target in this case?
> > > For MIPS the right bits will be value-dependent - registers must be
> > > sign extended.
> > 
> > Hmm, what about the version below?  This should allow the low target
> > whatever flexibility it requires ...
> 
> Looks good to me.

Checked in as well, thanks.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com