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


Wei-cheng Wang wrote:
> On 2014/11/28 ä¸?å?? 08:15, Ulrich Weigand 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.

Very good!  Thanks for the additional testing.

> > 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.

Many thanks for completing VSX support!


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

I see, thanks.

> > 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?

It seems odd to have in_dynsym_resolve_code call into
skip_trampoline_code.  Is there a reason why the skip_trampoline_code
implementation cannot accept a PC in the middle of the sequence?


> >> +  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.

Well, which argument registers are clobbered really depends on which
arguments the kernel passes to a signal handler routine.  The kernel
really doesn't change any registers except those needed to effect an
orderly function call to the signal handler, according to the ABI.

However, I think it would be safest to assume all argument registers
can be clobbered, just in case of future kernel changes.


> >> +  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.

Well, given that x86 does it, I guess I'm fine with leaving it in.

> > 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?

Yes, that looks right to me.


> > 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.

OK, sounds good.

> >> +  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.

Agreed that always 64k is good for now.


> > 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."

Huh, OK.  Patch 1/3 looks OK to me then.


> >> +    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.

The problem is not that they write to VRC, but that the insn wouldn't
match the case statement any more for any VRC != 0.   You have:

+  int ext = PPC_FIELD (insn, 21, 11);
[snip]
+  switch (ext)
+    {
[snip]
+    case 32:           /* Vector Multiply-High-Add Signed Halfword Saturate */

But the last 11 bits of this instruction also hold the VRC field, so if
VRC != 0, then ext would be != 32 for this instruction.

That's why I said the check needs to be handled like Vector Permute:

+  if ((ext & 0x3f) == 45)
+    {

That is, you should have a separate

  switch (ext & 0x3f)
    {
    case 45:
       ...
    case 32:
       ...
    ...
    }

at the beginning to handle all instructions with VRC field.


> >> +    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.

Oops, you are right: for opcode 56, that check should not be there.
(In fact, adding the check is wrong.)   Sorry for the confusion.

I was confusing this with opcode 57, where the check is actually
necessary.

I'll also reply to the patches separately.

Thanks again,
Ulrich


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


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