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

[Wei-cheng Wang <cole945@gmail.com>]

On 2014/11/28 下午 08:15, Ulrich Weigand wrote:
> Wei-cheng Wang wrote:
> Excellent!   Some suggestions for additional testing:
>    - 32-bit mode (test using -m32) -- it seems some of your Linux-specific
>      code may not work correctly in 32-bit mode
>    - Different ISA levels (test using -mcpu=power8/power7/...) to exercise
>      different instructions

   Tested with -m32 and -mcpu=power8/power7
   * All pass for -mcpu=power8/power7
   * 12 fails due to skip-plt-stub.  Fixed in this patch.

> The only one of those that would be of interest (on current server
> platforms) would be VSX.  I see that you're actually already supporting
> many VSX instructions, in particular loads and stores.  The one missing
> piece is opcode 60.  It would be great if you could add that to complete
> VSX support ... but this is not a requirement to get this patch approved,
> we can always add it later.
   VSX instructions in opcode 60 are added now.

> What is this (infrun.h) needed for?
   For execution_direction == EXEC_REVERSE.  They are declared in infrun.h.

>> +#define SUBSTRCMP(sym, stub)  (memcmp (sym + 8, stub, sizeof (stub) - 1) == 0)
>> +      if (SUBSTRCMP (MSYMBOL_LINKAGE_NAME (sym.minsym), ".plt_call."))
>> +	return 1;
>> +      if (SUBSTRCMP (MSYMBOL_LINKAGE_NAME (sym.minsym), ".plt_branch."))
>> +	return 1;
>> +      if (SUBSTRCMP (MSYMBOL_LINKAGE_NAME (sym.minsym), ".plt_branch_r2off."))
>> +	return 1;
>> +#undef SUBSTRCMP
> Huh.  The "sym + 8" seems odd.  What if symbol name is shorter than 8 bytes?
   I was trying to match such symbols,
     00000017.plt_call.foo
     ^^^^^^^^
     8 is used to skip the hex prefix.
> Also, those stub symbols are not guaranteed to be present.
> Can't we generalize the skip_trampoline_code so that it also accepts
> a PC in the middle of the sequence?  That would seem a more general
> solution.
   Ok, I changed the implementation above in in_dynsym_resolve_code to use
   skip_trampoline_code to match the sequence backward.  Is this ok?

>> @@ -1345,6 +1524,15 @@ ppc_linux_init_abi (struct gdbarch_info info,
>> +      if (powerpc_so_ops.in_dynsym_resolve_code == NULL)
>> +	{
>> +	  powerpc_so_ops = svr4_so_ops;
>> +	  /* Override dynamic resolve function.  */
>> +	  powerpc_so_ops.in_dynsym_resolve_code =
>> +	    powerpc_linux_in_dynsym_resolve_code;
>> +	}
>> +      set_solib_ops (gdbarch, &powerpc_so_ops);
>> +
> If we actually need this, we shouldn't copy this part in both the
> wordsize == 4 and wordsize == 8 paths, but just have it once.
> Also, this part should probably be broken out into a separate patch.
>> +  if (syscall_gdb == gdb_sys_sigreturn
>> +      || syscall_gdb == gdb_sys_rt_sigreturn)
>> +   {
>> +     if (ppc_all_but_pc_registers_record (regcache))
>> +       return -1;
>
> For example, here, we might actually have to record *all* registers, since
> a sigreturn syscall will in fact reload not just the GPRs, but also float
> and vector registers.
   Changed to record GPRs, FPRs and Vector registers as you suggested.

>> +static int
>> +ppc64_linux_record_signal (struct gdbarch *gdbarch,
>> +                           struct regcache *regcache,
>> +                           enum gdb_signal signal)
>> +{
>> +  /* See arch/powerpc/kernel/signal_64.c
>> +	 arch/powerpc/include/asm/ptrace.h
>> +     for details.  */
>> +  const int SIGNAL_FRAMESIZE = 128;
>> +  const int sizeof_rt_sigframe = 1440 * 2 + 8 * 2 + 4 * 6 + 8 + 8 + 128 + 512;
> This is too big for 32-bit, but that may not matter.  (See also below.)
>> +  ULONGEST sp;
>> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>> +
>> +  if (ppc_all_but_pc_registers_record (regcache))
>> +    return -1;
> I'm not sure why we need this here; signal delivery does not actually change
> all registers, it only changes those affected by a function call (possibly
> 0, 11, 12, some argument registers, LR, CTR, SP).
   I checked signal_64.c/signal_32.c in kernel code.
   If I didn't get it wrong, 3, 4 ,5 ,and 6, (and 12 for 64-bit only),
   LR, CTR and SP should be saved, and I record these registers in the new patch.
   However, I didn't find any document describes about this.

>> +  if (record_full_arch_list_add_mem (sp, SIGNAL_FRAMESIZE + sizeof_rt_sigframe))
>> +    return -1;
> I see x86 does this too, but I wonder why.  All this memory is in fact
> uninitialized before the signal, so what's the point of tracking its original
> contents?
   I thought users may want to record exactly what happened?
   It shouldn't matter if recording too much memory for 32-bit.
   Or should I just removed this?  As you said, the are uninitialized before the signal.

>> +  /* Support reverse debugging.  */
>> +  set_gdbarch_process_record (gdbarch, ppc64_process_record);
>> +  set_gdbarch_process_record_signal (gdbarch, ppc64_linux_record_signal);
>> +  tdep->syscall_record = ppc_linux_syscall_record;
>
> At this place in ppc_linux_init_abi, we are handling *both* 32-bit and
> 64-bit code.  This means the callbacks installed here should handle
> both bitsizes.  That seems OK for ppc64_process_record, but probably
> not for ppc64_linux_record_signal and ppc_linux_syscall_record (in
> their current implementation).
   32/64 use the same syscall numbers, and they clobbered the same registers,
   so ppc_linux_syscall_record can be shared for both 32- and 64-bit, right?
   ppc_linux_record_signal is revised by checking tdep->wordsize to save extra r12
   for 64-bit.

>> +  /* Initialize the ppc_linux_record_tdep.  */
>> +  /* These values are the size of the type that will be used in a system
>> +     call.  They are obtained from Linux Kernel source.
>> +
>> +     See arch/powerpc/include/uapi/asm/ioctls.h.  */
>> +  ppc_linux_record_tdep.size_pointer
>> +    = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
>
> This is problematic.  There is only one single instance of
> ppc_linux_record_tdep, so when this code is called for 32-bit
> and then for 64-bit, that single instance gets overwritten.
> I think we really need two copies of the structure, and
> install the one appropriate for the current bitsize.
>
> Note that many of the hard-coded "size" values in your
> patch are only correct for 64-bit, so the 32-bit copy
> of the struct would have to use different values.
> (The non-"size" values should be OK for both.)

   ppc_linux_record_tdep and ppc64_linux_record_tdep are declared,
   and tdep->pcc_linux_record_tdep will point to the right one.

 >> +  ppc_linux_record_tdep.size_PAGE_SIZE = 4096;
 > This is a bit of a problem.  The PowerPC kernel can be compiled
 > with either 4k or 64k page size.  We could either try to detect
 > at runtime (from auxv) or always use the conservative 64k.
   Always 64K in this patch.
   If we want to handle both 4k/64k,
   * 4 ppc_linux_records are need for (32 + 64) * (4K + 64K).
   * Add field of pagesize in ppc_tdep.
   * Revise "Find a candidate among extant architectures." in
     rs6000_gdbarch_init to check pagesize.

>> +static const struct frame_unwind rs6000_epilogue_frame_unwind =
>> +{
>> +  NORMAL_FRAME,
>> +  default_frame_unwind_stop_reason,
>> +  rs6000_epilogue_frame_this_id, rs6000_epilogue_frame_prev_register,
>> +  NULL,
>> +  rs6000_epilogue_frame_sniffer
>> +};
>
> Is this actually still necessary with a current compiler that tracks
> epilogues in DWARF?   In any case, it would be better to provide this
> in a separate patch.
   This is necessary.
   Some cases test "reverse-next over undebuggable solib functions."

>> +    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 */
> All of those use a VRC field, and therefore need to be treated like the case 45
> at the beginning of this function!
    I am confused.  I checked PowerISA_V2.07_PUBLIC.pdf again,
    and these only _use_ VRC, but not write VRC, so does case 45.

>> +			/* 5.16 Decimal Integer Arithmetic Instructions */
> Headline superfluous at this point.
   Section headlines are all removed.

>> +    case 339:          /* Move From Special Purpose Register */
> Maybe add case 371 Move From Time Base here.  The instruction is
> phased out, but potentially still used in some code.
   Ok, added.

>> +    /* These write VSR of size 8.  */
>> +    case 588:		/* Load VSX Scalar Doubleword Indexed */
>> +      ppc_record_vsr (regcache, tdep, PPC_XT (insn), 8);
> Note that the contents of the other doubleword element are undefined
> after this instruction, so you need to save all of it, i.e. size == 16.
> In fact, we probably don't even need the size argument then.
   "SIZE" argument is removed as you suggest.

>> +static int
>> +ppc64_process_record_op60 (struct gdbarch *gdbarch, struct regcache *regcache,
>> +                          CORE_ADDR addr, uint32_t insn)
>> +{
>> +  int ext = PPC_EXTOP (insn);
>> +
>> +  fprintf_unfiltered (gdb_stdlog, "Warning: Don't know how to record "
>> +                     "%08x at %08lx, 60-%d.\n", insn, addr, ext);
> Since those may actually occur (frequently) in user code, we should probably
> reword the message to something along the lines of "VSX instructions not
> yet supported".  (Or simply implement them :-))
   Opcode 60 are all handled in the new patch :)

>> +    case 56:		/* Load Quadword */
> Should check that the last two bits are zero.
   I checked ISA, it doesn't specify the last two bits.  Bit-28x-31 is "///".
   However, I add the check as you suggest in the new patch.

>> +    case 47:		/* Store Multiple Word */
>> +	{
>> +	  ULONGEST addr = 0;
>> +	  int size;
> Unused?
>> +
>> +	  if (PPC_RA (insn) != 0)
>> +	    regcache_raw_read_unsigned (regcache,
>> +					tdep->ppc_gp0_regnum + PPC_RA (insn),
>> +					&addr);
>> +
>> +	  addr += PPC_D (insn);
>> +
>> +	  for (i = PPC_RS (insn); i < 32; i++)
>> +	    record_full_arch_list_add_mem (addr + i * 4, 4);
> The offset is wrong; the first register is stored at addr.
> Is there a reason why this can't be just a single record call?
   Changed to just a single record as you suggested.

> Bye,
> Ulrich

Thanks for the review,
Wei-cheng