diff mbox

[committed] MIPS/gdbserver: Correctly handle narrow big-endian register transfers

Message ID alpine.DEB.2.00.1805221735230.10896@tp.orcam.me.uk
State Committed
Headers show

Commit Message

Maciej W. Rozycki May 22, 2018, 9:53 p.m. UTC
Fix an issue with `gdbserver' on big-endian n64 MIPS targets, where 
$dspctl is 32-bit while the `ptrace' transfer data type is 64-bit.

Such register data is held in the low order 32 bits of the 64-bit data 
quantity exchanged with the buffer used by `fetch_register' and 
`store_register', however `supply_register' and `collect_register' 
access the same data as a 32-bit quantity.  Consequently the register is 
presented and written as all-zeros held in the most-significant part of 
the big-endian 64-bit data quantity represented in the buffer:

(gdb) info registers
                  zero               at               v0               v1
 R0   0000000000000000 0000000000000001 0000000000000001 0000000000000000
                    a0               a1               a2               a3
 R4   00000001200212b0 0000000000000000 0000000000000021 000000012001a260
                    a4               a5               a6               a7
 R8   000000012001a260 0000000000000004 800000010c60c000 fffffffffffffff8
                    t0               t1               t2               t3
 R12  0000000000000000 000000fff7edab68 0000000000000001 0000000000000000
                    s0               s1               s2               s3
 R16  000000fff7ee2068 0000000120008b80 0000000000000000 0000000000000000
                    s4               s5               s6               s7
 R20  000000000052e5c8 000000000052f008 0000000000000000 0000000000000000
                    t8               t9               k0               k1
 R24  0000000000000000 00000001200027c0 0000000000000000 0000000000000000
                    gp               sp               s8               ra
 R28  00000001200212b0 000000ffffffc850 000000ffffffc850 0000000120005ee8
                status               lo               hi         badvaddr
      0000000000109cf3 0000000000943efe 000000000000000e 000000012001a008
                 cause               pc
      0000000000800024 0000000120005ee8
                  fcsr              fir              hi1              lo1
              0e800000         00f30000 0000000000000000 0101010101010101
                   hi2              lo2              hi3              lo3
      0202020202020202 0303030303030303 0404040404040404 0505050505050505
                dspctl          restart
              00000000 0000000000000000
(gdb) 

Correct this problem then by using the `mips_supply_register' 
`mips_collect_register' accessors for 32-bit registers where the 
`ptrace' data type is 64-bit.  These accessors already operate on 32-bit 
data quantities held in 64-bit containers:

(gdb) info registers
                  zero               at               v0               v1
 R0   0000000000000000 0000000000000001 0000000000000001 0000000000000000
                    a0               a1               a2               a3
 R4   00000001200212b0 0000000000000000 0000000000000021 000000012001a260
                    a4               a5               a6               a7
 R8   000000012001a260 0000000000000004 800000010d82e900 fffffffffffffff8
                    t0               t1               t2               t3
 R12  0000000000000000 000000fff7edab68 0000000000000001 0000000000000000
                    s0               s1               s2               s3
 R16  000000fff7ee2068 0000000120008b80 0000000000000000 0000000000000000
                    s4               s5               s6               s7
 R20  000000000052e5c8 000000000052f008 0000000000000000 0000000000000000
                    t8               t9               k0               k1
 R24  0000000000000000 00000001200027c0 0000000000000000 0000000000000000
                    gp               sp               s8               ra
 R28  00000001200212b0 000000ffffffc850 000000ffffffc850 0000000120005ee8
                status               lo               hi         badvaddr
      0000000000109cf3 0000000000943efe 000000000000000e 000000012001a008
                 cause               pc
      0000000000800024 0000000120005ee8
                  fcsr              fir              hi1              lo1
              0e800000         00f30000 0000000000000000 0101010101010101
                   hi2              lo2              hi3              lo3
      0202020202020202 0303030303030303 0404040404040404 0505050505050505
                dspctl          restart
              55aa33cc 0000000000000000
(gdb) 

	gdb/gdbserver/
	* linux-mips-low.c [HAVE_PTRACE_GETREGS] (mips_collect_register)
	(mips_supply_register): Move outside HAVE_PTRACE_GETREGS.
	(mips_collect_ptrace_register, mips_supply_ptrace_register): New 
	functions.
	(the_low_target): Wire them.
---
On Tue, 22 May 2018, Ulrich Weigand wrote:

> Maciej W. Rozycki wrote:
> 
> >  This fixes the problem for MIPS, however from my understanding of your 
> > commit it will break s390x unless it is further adjusted in a 
> > target-dependent manner.  So my question is: does `ptrace' indeed place 
> > 32-bit quantities transferred in bits 63:32 of the 64-bit integer data 
> > quantity passed?
> 
> Well, my understanding of PTRACE_PEEKUSR and PTRACE_POKEUSR is that they
> are supposed to simulate access to a "user area", i.e. a data structure
> that holds per-process status, in word sized chunks.  Now of course the
> Linux kernel doesn't actually maintain such a "user area", but for the
> purpose of those ptrace calls, it is supposed to pretend it does.

 Now that you write it it makes sense to me and I can only conclude that 
maybe it's MIPS that is an oddball here rather than any other target.

 It could be IRIX legacy, but MIPS USR addresses are effectively indices, 
that is R0 is at 0, R1 is at 1, and so on, and so on, and then finally LO3 
is at 76 and DSPControl is at 77, regardless of their actual width.  
Partial quantities cannot be accessed, hence there's no way to correctly 
read or write say said LO3 with the n32 ABI where the register is 64-bit 
and ptrace(2) can only pass 32-bit quantities.

 We cannot do anything about it as the MIPS/Linux API has been set since 
like 1994.  The way to move forward is to switch to the PTRACE_GETREGSET 
and PTRACE_SETREGSET API, but for DSP I have proposed the required Linux 
kernel part last week only.

> Can't you move the special logic into platform-specific
> the_low_target.collect_ptrace_register and
> the_low_target.supply_ptrace_register calls instead?

 Agreed.  I only discovered the s390x case as I tracked down the commit 
history as I was about to submit the change.  I'm so glad that I asked you 
directly, and thanks for the quick response.

 I guess this also means commit 1d7611244c14 ("PR gdb/22286: 
linux-nat-trad: Support arbitrary register widths") might have to be 
revisited, although the need doesn't seem immediate as it only affects 
MIPS and Alpha I believe, and Alpha does not appear to have been ever used 
big-endian (and by the look of arch/alpha/kernel/ptrace.c in Linux it also 
uses indices rather than offsets, e.g. R0 is at 0, R1 is at 1, and so on, 
hmm...).

 So I actually wonder what the distribution of the targets we support is 
between offset vs index USR addressing.

 Anyway, this replacement change has passed full regression testing with 
o32 and n64 targets, with `gdbserver' strapped to use PTRACE_PEEKUSR and 
PTRACE_POKEUSR requests across all registers.  I have also smoke-tested 
n32, verifying that registers show correctly.  Committed.

 Thank you for your input.

  Maciej
---
 gdb/gdbserver/linux-mips-low.c |   48 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 44 insertions(+), 4 deletions(-)

gdb-mips-gdbserver-fetch-store-register-uneven.diff
diff mbox

Patch

Index: binutils/gdb/gdbserver/linux-mips-low.c
===================================================================
--- binutils.orig/gdb/gdbserver/linux-mips-low.c	2018-05-22 01:00:12.000000000 +0100
+++ binutils/gdb/gdbserver/linux-mips-low.c	2018-05-22 22:10:11.049273576 +0100
@@ -677,8 +677,6 @@  ps_get_thread_area (struct ps_prochandle
   return PS_OK;
 }
 
-#ifdef HAVE_PTRACE_GETREGS
-
 static void
 mips_collect_register (struct regcache *regcache,
 		       int use_64bit, int regno, union mips_register *reg)
@@ -711,6 +709,8 @@  mips_supply_register (struct regcache *r
   supply_register (regcache, regno, reg->buf + offset);
 }
 
+#ifdef HAVE_PTRACE_GETREGS
+
 static void
 mips_collect_register_32bit (struct regcache *regcache,
 			     int use_64bit, int regno, unsigned char *buf)
@@ -846,6 +846,46 @@  mips_store_fpregset (struct regcache *re
 }
 #endif /* HAVE_PTRACE_GETREGS */
 
+/* Take care of 32-bit registers with 64-bit ptrace, POKEUSER side.  */
+
+static void
+mips_collect_ptrace_register (struct regcache *regcache,
+			      int regno, char *buf)
+{
+  const struct target_desc *tdesc = current_process ()->tdesc;
+  int use_64bit = sizeof (PTRACE_XFER_TYPE) == 8;
+
+  if (use_64bit && register_size (regcache->tdesc, regno) == 4)
+    {
+      union mips_register reg;
+
+      mips_collect_register (regcache, 0, regno, &reg);
+      memcpy (buf, &reg, sizeof (reg));
+    }
+  else
+    collect_register (regcache, regno, buf);
+}
+
+/* Take care of 32-bit registers with 64-bit ptrace, PEEKUSER side.  */
+
+static void
+mips_supply_ptrace_register (struct regcache *regcache,
+			     int regno, const char *buf)
+{
+  const struct target_desc *tdesc = current_process ()->tdesc;
+  int use_64bit = sizeof (PTRACE_XFER_TYPE) == 8;
+
+  if (use_64bit && register_size (regcache->tdesc, regno) == 4)
+    {
+      union mips_register reg;
+
+      memcpy (&reg, buf, sizeof (reg));
+      mips_supply_register (regcache, 0, regno, &reg);
+    }
+  else
+    supply_register (regcache, regno, buf);
+}
+
 static struct regset_info mips_regsets[] = {
 #ifdef HAVE_PTRACE_GETREGS
   { PTRACE_GETREGS, PTRACE_SETREGS, 0, 38 * 8, GENERAL_REGS,
@@ -916,8 +956,8 @@  struct linux_target_ops the_low_target =
   mips_remove_point,
   mips_stopped_by_watchpoint,
   mips_stopped_data_address,
-  NULL,
-  NULL,
+  mips_collect_ptrace_register,
+  mips_supply_ptrace_register,
   NULL, /* siginfo_fixup */
   mips_linux_new_process,
   mips_linux_delete_process,