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

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

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