Re: [PATCH 2/3 v2] Process record support for PowerPC

Ulrich Weigand
2014-12-08T19:12:07Z

Wei-cheng Wang wrote:

This is looking very good now; just a couple of minor issues left.

> * configure.tgt (powerpc*-*-linux): A linux-record.o to
Typo "Add"
> gdb_target_obs.
> (ppc_linux_record_tdep, ppc64_linux_record_tdep) New for linux syscal
> record.
Missing ":" after ")". Typo "syscall"
> (ppc_canonicalize_syscall, ppc_linux_syscall_record,
> ppc_linux_record_signal, ppc_init_linux_record_tdep): New functions.
> (ppc_linux_init_abi): Set process_record, process_record_signal.
> * ppc-tdep.h: Add ppc_syscall_record and ppc_linux_record_tdep to
> gdbarch_tdep.
Should be: "* ppc-tdep.h (struct gdbarch_tdep): Add ..."

> (ppc_process_record): New declaration.
> * rs6000-tdep.c: (ppc_record_vsr, ppc_process_record_op4,
No ":" since we start with a "(".
> ppc_process_record_op19, ppc_process_record_op31,
> ppc_process_record_op59, ppc_process_record_op60,
> ppc_process_record_op63, ppc_process_record): New functions.
>
> Changelog for testsuite
>
> 2014-12-06 Wei-cheng Wang <cole945@gmail.com>
>
> * lib/gdb.exp: supports_process_record): Return true for
> powerpc*-*-linux* (supports_reverse): Likewise.

* lib/gdb.exp: supports_process_record): Return true for
powerpc*-*-linux*.
(supports_reverse): Likewise.

> diff --git a/gdb/ppc-tdep.h b/gdb/ppc-tdep.h
> index 08554ff..81e11a4 100644
> --- a/gdb/ppc-tdep.h
> +++ b/gdb/ppc-tdep.h
> @@ -25,6 +25,7 @@ struct frame_info;
> struct value;
> struct regcache;
> struct type;
> +struct linux_record_tdep;
>
> /* From ppc-sysv-tdep.c ... */
> enum return_value_convention ppc_sysv_abi_return_value (struct gdbarch *gdbarch,
> @@ -259,6 +260,9 @@ struct gdbarch_tdep
> /* ISA-specific types. */
> struct type *ppc_builtin_type_vec64;
> struct type *ppc_builtin_type_vec128;
> +
> + int (*ppc_syscall_record) (struct regcache *regcache);
> + struct linux_record_tdep *ppc_linux_record_tdep;
> };

The generic tdep file should not use any linux-specific data types.
It would be better remove the ppc_linux_record_tdep field and handle
this fully in ppc-linux-tdep.c, either by using two ppc_syscall_record
routines or (better) by having the routine do a tdep->wordsize check.

> +/* PPC process record-replay */
> +
> +struct linux_record_tdep ppc_linux_record_tdep;
> +struct linux_record_tdep ppc64_linux_record_tdep;

Should be "static".

> +ppc_linux_record_signal (struct gdbarch *gdbarch, struct regcache *regcache,
> + enum gdb_signal signal)
[snip]
> + for (i = 3; i <= 6; i++)
> + {
> + if (record_full_arch_list_add_reg (regcache, tdep->ppc_gp0_regnum + i))
> + return -1;
> + }
> + if (tdep->wordsize == 8
> + && record_full_arch_list_add_reg (regcache, tdep->ppc_gp0_regnum + 12))
> + return -1;
Probably better to just clobber all registers 3 .. 12, just in case at some
point the kernel changes to pass more arguments to signal handlers. (No
need really to check for tdep->wordsize, cannot hurt to record r12 also
on 32-bit.)

> +ppc_init_linux_record_tdep (struct gdbarch *gdbarch,
> + struct linux_record_tdep *record_tdep)
[snip]
> + record_tdep->size_pointer = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
> + record_tdep->size_int = gdbarch_int_bit (gdbarch) / TARGET_CHAR_BIT;
> + record_tdep->size_long = gdbarch_long_bit (gdbarch) / TARGET_CHAR_BIT;
> + record_tdep->size_ulong = gdbarch_long_bit (gdbarch) / TARGET_CHAR_BIT;
Might as well use hard-coded values here, we use hard-coded values for
everything else ...

> +ppc_process_record_op4 (struct gdbarch *gdbarch, struct regcache *regcache,
> + CORE_ADDR addr, uint32_t insn)
[snip]
> + case 32: /* Vector Multiply-High-Add Signed Halfword Saturate */
> + case 33: /* Vector Multiply-High-Round-Add Signed Halfword Saturate */
> + case 39: /* Vector Multiply-Sum Unsigned Halfword Saturate */
> + case 41: /* Vector Multiply-Sum Signed Halfword Saturate */
> + case 42: /* Vector Select */
> + case 43: /* Vector Permute */
> + case 44: /* Vector Shift Left Double by Octet Immediate */
> + case 60: /* Vector Add Extended Unsigned Quadword Modulo */
> + case 61: /* Vector Add Extended & write Carry Unsigned Quadword */
> + case 62: /* Vector Subtract Extended Unsigned Quadword Modulo */
> + case 63: /* Vector Subtract Extended & write Carry Unsigned Quadword */
> + case 34: /* Vector Multiply-Low-Add Unsigned Halfword Modulo */
> + case 36: /* Vector Multiply-Sum Unsigned Byte Modulo */
> + case 37: /* Vector Multiply-Sum Mixed Byte Modulo */
> + case 38: /* Vector Multiply-Sum Unsigned Halfword Modulo */
> + case 40: /* Vector Multiply-Sum Signed Halfword Modulo */
> + case 46: /* Vector Multiply-Add Single-Precision */
> + case 47: /* Vector Negative Multiply-Subtract Single-Precision */
These still do not handle the case of VRC != 0 correctly; see my reply
to the 0/3 email.

> +ppc_process_record_op60 (struct gdbarch *gdbarch, struct regcache *regcache,
> + CORE_ADDR addr, uint32_t insn)
[snip]
> + case 61: /* VSX Scalar Test for software Divide Double-Precision */
Should move down to 125/93

> + case 240: /* VSX Vector Copy Sign Double-Precision */
> + case 208: /* VSX Vector Copy Sign Single-Precision */
These two do not modify fpscr, should move down.

> +ppc_process_record_op63 (struct gdbarch *gdbarch, struct regcache *regcache,
> + CORE_ADDR addr, uint32_t insn)
[snip]
> + case 130: /* DFP Compare Ordered Quad */
> + case 162: /* DFP Test Exponent Quad */
> + case 194: /* DFP Test Data Class Quad */
> + case 226: /* DFP Test Data Group Quad */
> + case 642: /* DFP Compare Unordered Quad */
> + case 674: /* DFP Test Significance Quad */
None of these actually have an RC bit, but they unconditionally set CR.
(Sorry, didn't notice that on the initial review.)
> + if (PPC_RC (insn))
> + record_full_arch_list_add_reg (regcache, tdep->ppc_cr_regnum);
> + record_full_arch_list_add_reg (regcache, tdep->ppc_fpscr_regnum);
> + return 0;

> + case 23: /* Floating Select */
> + case 583: /* Move From FPSCR */
> + record_full_arch_list_add_reg (regcache, tdep->ppc_cr_regnum);
But these two do have an RC bit. (Likewise.)

> +ppc_process_record (struct gdbarch *gdbarch, struct regcache *regcache,
> + CORE_ADDR addr)
[snip]
> + case 56: /* Load Quadword */
> + if (PPC_FIELD (insn, 30, 2) != 0)
> + goto UNKNOWN_OP;
As discussed in the other email, this test is actually wrong here;
sorry. The test should only be with opcode 57.

Thanks,
Ulrich

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