This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.

Index Nav:	[Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav:	[Date Prev] [Date Next]	[Thread Prev] [Thread Next]
Other format:	[Raw text]

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

From: Wei-cheng Wang <cole945 at gmail dot com>
To: uweigand at de dot ibm dot com, gdb-patches at sourceware dot org
Date: Sun, 07 Dec 2014 01:57:07 +0800
Subject: Re: [PATCH 0/3 v2] Process record support for PowerPC
Authentication-results: sourceware.org; auth=none
References: <201411281215 dot sASCFh2d003491 at d03av02 dot boulder dot ibm dot 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

Follow-Ups:
- Re: [PATCH 0/3 v2] Process record support for PowerPC
  - From: Ulrich Weigand

Index Nav:	[Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav:	[Date Prev] [Date Next]	[Thread Prev] [Thread Next]