This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/2] gdb: Add process record and replay support for s390.
- From: "Ulrich Weigand" <uweigand at de dot ibm dot com>
- To: koriakin at 0x04 dot net (Marcin KoÅcielnicki)
- Cc: gdb-patches at sourceware dot org, koriakin at 0x04 dot net (Marcin KoÅcielnicki)
- Date: Mon, 2 Nov 2015 21:35:22 +0100 (CET)
- Subject: Re: [PATCH 1/2] gdb: Add process record and replay support for s390.
- Authentication-results: sourceware.org; auth=none
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