Re: [PATCH 1/2] gdb: Add process record and replay support for s390.

["Ulrich Weigand" <uweigand@de.ibm.com>]

Marcin KoÅcielnicki <koriakin at 0x04 dot net> wrote:

>gdb/ChangeLog:
>
>	* gdb/configure.tgt: Add linux-record.o to s390*-linux.
>	* gdb/s390-linux-tdep.c (s390_all_but_pc_registers_record): New function.
>	(s390_canonicalize_syscall): New function.
>	(s390_linux_syscall_record): New function.
>	(s390_linux_record_signal): New function.
>	(s390_record_calc_disp_common): New function.
>	(s390_record_calc_disp): New function.
>	(s390_record_calc_disp_vsce): New function.
>	(s390_record_calc_rl): New function.
>	(s390_record_gpr_g): New function.
>	(s390_record_gpr_h): New function.
>	(s390_record_vr): New function.
>	(s390_process_record): New function.
>	(s390_init_linux_record_tdep): New function.
>	(s390_gdbarch_init): Fill record function slots.
>
>gdb/testsuite/ChangeLog:
>
>	* gdb.reverse/s390-mvcle.c: New test.
>	* gdb.reverse/s390-mvcle.exp: New file.
>	* lib/gdb.exp: Enable reverse tests on s390*-linux.

This is all looking really good!  Just a small number of issues --
mostly cosmetic, except for the A(L)GSI bug.


>+#include "linux-record.h"
>+#include "record-full.h"

Adding those includes should be mentioned in the ChangeLog.

>+static struct linux_record_tdep s390_linux_record_tdep;
>+static struct linux_record_tdep s390x_linux_record_tdep;

Likewise these new global variables.

>+static enum gdb_syscall
>+s390_canonicalize_syscall (int syscall, enum s390_abi_kind abi)
>+{
>+  switch (syscall)
>+    {
>+    /* s390 syscall numbers < 222 are mostly the same as x86, so just list the exceptions. */

Line too long.  Two spaces after ".".  (The same coding style issue
appears in a number of other comments.)

>+    case 223:
>+      return gdb_sys_sendfile64;
This is 32-bit only.


>+static int
>+s390_linux_record_signal (struct gdbarch *gdbarch, struct regcache *regcache,
>+                          enum gdb_signal signal)
>+{
>+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>+  /* There are two kinds of signal frames on s390. rt_sigframe is always
>+     the larger one, so don't even bother with sigframe. */
>+  const int sizeof_rt_sigframe = (tdep->abi == ABI_LINUX_ZSERIES ? 160 + 8 + 128 + 1024 : 96 + 8 + 128 + 1000);

Line too long.

 
>+static CORE_ADDR
>+s390_record_calc_disp_common (struct gdbarch *gdbarch, struct regcache *regcache,
>+                             ULONGEST x, uint16_t bd, int8_t dh)
>+{
>+  uint8_t rb = bd >> 12 & 0xf;
>+  int32_t d = (bd & 0xfff) | ((int32_t)dh << 12);
>+  ULONGEST b;
>+  CORE_ADDR res = d + x;
>+  int am;
Unused.


>+static int
>+s390_process_record (struct gdbarch *gdbarch, struct regcache *regcache,
>+                    CORE_ADDR addr)
>+{
>+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>+  uint16_t insn[3] = {0};
>+  /* Instruction as bytes. */
>+  uint8_t ibyte[6];
>+  /* Instruction as nibbles. */
>+  uint8_t inib[12];
>+  /* Instruction vector registers. */
>+  uint8_t ivec[4];
>+  CORE_ADDR oaddr, oaddr2, oaddr3;
>+  ULONGEST tmp;
>+  int i, n;
>+  /* if EX/EXRL instruction used, here's the reg parameter */
>+  int ex = -1;
>+
>+  /* Attempting to use EX or EXRL jumps back here */
>+ex:
>+
>+  /* Read instruction. */
>+  insn[0] = read_memory_unsigned_integer (addr, 2, BFD_ENDIAN_BIG);

While s390 is indeed always big-endian, I'd still prefer to avoid
hard-coding this here; just use gdbarch_byte_order.

>+    case 0x4e: /* CVD - convert to binary */
"convert to decimal"

>+    /* 0x80 priviledged: SSM - set system mask */
"privileged" (also various other places)

>+        case 0xb255: /* MVST - move string [partial] */
>+          {
>+            /* XXX - partial instruction with long memory write */
>+            uint8_t end;
>+            gdb_byte cur;
>+            ULONGEST num = 0;
>+            /* Read ending byte.  */
>+            regcache_raw_read_unsigned (regcache, S390_R0_REGNUM, &tmp);
>+            end = tmp & 0xff;
>+            /* Get address of second operand.  */
>+            regcache_raw_read_unsigned (regcache, S390_R0_REGNUM + inib[7], &tmp);
>+            oaddr = s390_record_address_mask (gdbarch, regcache, tmp);
>+            /* Search for ending byte and compute length.  */
>+            do {
>+              num++;
>+              if (target_read_memory (oaddr, &cur, 1))
>+                return -1;
>+              oaddr++;
>+            } while (cur != end);

It would be more efficient to read larger chunks at a time ...
But I guess that can be fixed if/when it turns out to be a problem.

>+        case 0xeb7a: /* AGSI - add immediate */
>+        case 0xeb7e: /* ALGSI - add immediate */
>+          oaddr = s390_record_calc_disp (gdbarch, regcache, 0, insn[1], ibyte[4]);
>+          if (record_full_arch_list_add_mem (oaddr, 4))
This needs to record 8 bytes.


>+/* Initialize linux_record_tdep if not initialized yet.  */
>+
>+static void
>+s390_init_linux_record_tdep (struct linux_record_tdep *record_tdep,
>+                             enum s390_abi_kind abi)
>+{

>+  if (abi == ABI_LINUX_ZSERIES)
>+      record_tdep->size_msqid_ds = 120;
This seems to be 104 on my kernel
>+      record_tdep->size_shmid_ds = 112;
and this 82.

>+  else if (abi == ABI_LINUX_S390)
>+      record_tdep->size_msqid_ds = 88;
and 56 on s390
>+      record_tdep->size_shmid_ds = 84;
and this 48.


>+  record_tdep->arg6 = S390_R7_REGNUM;
R7 is never used as argument register; all s390 syscalls use
at most 5 GPR arguments (r2 .. r6).

>+  record_tdep->ioctl_TIOCGPTN = 0x40045430;
Looks like this should be 0x80045430 ...
>+  record_tdep->ioctl_TIOCSPTLCK = 0x80045431;
... and this 0x40045431


Bye,
Ulrich

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