[PATCH] gdb-power10-single-step

[PATCH] gdb-power10-single-step

[will schmidt]

gdb-power10-single-step

Hi,
  This is a patch written by Alan Modra.  With his permission
I'm submitting this for review and helping get this upstream.

Powerpc / Power10 ISA 3.1 adds prefixed instructions, which
are 8 bytes in length.  This is in contrast to powerpc previously
always having 4 byte instruction length.  This patch implements
changes to allow GDB to better detect prefixed instructions, and
handle single stepping across the 8 byte instructions.


gdb/ChangeLog
2021-03-05  Alan Modra  <amodra@gmail.com>

	* ppc-tdep.h (ppc_software_single_step): Rename from
	ppc_deal_with_atomic_sequence.
	* rs6000-aix-tdep.c (rs6000_software_single_step): Adjust.
	* rs6000-tdep.c (OP_MASK): Rename from BRANCH_MASK, adjust throughout.
	(ppc_displaced_step_copy_insn): Handle reads of less than two
	insns.
	(ppc_displaced_step_fixup): Handle power10 insns.
	(ppc_software_single_step): Likewise.
	(rs6000_gdbarch_init): Adjust for rename.  Set max_insn_length
	to two words.

diff --git a/gdb/ppc-tdep.h b/gdb/ppc-tdep.h
index 77b6ab154f..8591e2cdbb 100644
--- a/gdb/ppc-tdep.h
+++ b/gdb/ppc-tdep.h
@@ -75,7 +75,7 @@ int ppc_altivec_support_p (struct gdbarch *gdbarch);
 /* Return non-zero if the architecture described by GDBARCH has
    VSX registers (vsr0 --- vsr63).  */
 int vsx_support_p (struct gdbarch *gdbarch);
-std::vector<CORE_ADDR> ppc_deal_with_atomic_sequence
+std::vector<CORE_ADDR> ppc_software_single_step
   (struct regcache *regcache);


diff --git a/gdb/rs6000-aix-tdep.c b/gdb/rs6000-aix-tdep.c
index 02d492b337..9d4c79fdf0 100644
--- a/gdb/rs6000-aix-tdep.c
+++ b/gdb/rs6000-aix-tdep.c
@@ -785,7 +785,7 @@ rs6000_software_single_step (struct regcache *regcache)

   insn = read_memory_integer (loc, 4, byte_order);

-  std::vector<CORE_ADDR> next_pcs = ppc_deal_with_atomic_sequence (regcache);
+  std::vector<CORE_ADDR> next_pcs = ppc_software_single_step (regcache);
   if (!next_pcs.empty ())
     return next_pcs;

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index b09f63137d..bc981036a8 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -841,7 +841,7 @@ typedef BP_MANIPULATION_ENDIAN (little_breakpoint, big_breakpoint)
   rs6000_breakpoint;

 /* Instruction masks for displaced stepping.  */
-#define BRANCH_MASK 0xfc000000
+#define OP_MASK 0xfc000000
 #define BP_MASK 0xFC0007FE
 #define B_INSN 0x48000000
 #define BC_INSN 0x40000000
@@ -895,7 +895,10 @@ ppc_displaced_step_copy_insn (struct gdbarch *gdbarch,
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   int insn;

-  read_memory (from, buf, len);
+  len = target_read (current_top_target (), TARGET_OBJECT_MEMORY, NULL,
+		     buf, from, len);
+  if ((ssize_t) len < PPC_INSN_SIZE)
+    memory_error (TARGET_XFER_E_IO, from);

   insn = extract_signed_integer (buf, PPC_INSN_SIZE, byte_order);

@@ -934,9 +937,9 @@ ppc_displaced_step_fixup (struct gdbarch *gdbarch,
 					     PPC_INSN_SIZE, byte_order);
   ULONGEST opcode = 0;
   /* Offset for non PC-relative instructions.  */
-  LONGEST offset = PPC_INSN_SIZE;
+  LONGEST offset;

-  opcode = insn & BRANCH_MASK;
+  opcode = insn & OP_MASK;

   displaced_debug_printf ("(ppc) fixup (%s, %s)",
 			  paddress (gdbarch, from), paddress (gdbarch, to));
@@ -1001,9 +1004,14 @@ ppc_displaced_step_fixup (struct gdbarch *gdbarch,
   else if ((insn & BP_MASK) == BP_INSN)
     regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch), from);
   else
-  /* Handle any other instructions that do not fit in the categories above.  */
-    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch),
-				    from + offset);
+    {
+      offset = PPC_INSN_SIZE;
+      /* Power10 prefix instructions are two words in length.  */
+      if (opcode == 1 << 26)
+	offset = 2 * PPC_INSN_SIZE;
+      regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+				      from + offset);
+    }
 }

 /* Implementation of gdbarch_displaced_step_prepare.  */
@@ -1061,12 +1069,13 @@ ppc_displaced_step_hw_singlestep (struct gdbarch *gdbarch)
   return true;
 }

-/* Checks for an atomic sequence of instructions beginning with a
-   Load And Reserve instruction and ending with a Store Conditional
-   instruction.  If such a sequence is found, attempt to step through it.
-   A breakpoint is placed at the end of the sequence.  */
+/* Step over Power10 prefix instructions and atomic sequence of
+   instructions beginning with a Load And Reserve instruction and
+   ending with a Store Conditional instruction.  If such a sequence is
+   found, attempt to step through it.  A breakpoint is placed at the
+   end of the sequence.  */
 std::vector<CORE_ADDR>
-ppc_deal_with_atomic_sequence (struct regcache *regcache)
+ppc_software_single_step (struct regcache *regcache)
 {
   struct gdbarch *gdbarch = regcache->arch ();
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
@@ -1081,6 +1090,10 @@ ppc_deal_with_atomic_sequence (struct regcache *regcache)
   const int atomic_sequence_length = 16; /* Instruction sequence length.  */
   int bc_insn_count = 0; /* Conditional branch instruction count.  */

+  /* Power10 prefix instructions are two words in length.  */
+  if ((insn & OP_MASK) == 1 << 26)
+    return { pc + 8 };
+
   /* Assume all atomic sequences start with a Load And Reserve instruction.  */
   if (!IS_LOAD_AND_RESERVE_INSN (insn))
     return {};
@@ -1095,7 +1108,7 @@ ppc_deal_with_atomic_sequence (struct regcache *regcache)
       /* Assume that there is at most one conditional branch in the atomic
 	 sequence.  If a conditional branch is found, put a breakpoint in 
 	 its destination address.  */
-      if ((insn & BRANCH_MASK) == BC_INSN)
+      if ((insn & OP_MASK) == BC_INSN)
 	{
 	  int immediate = ((insn & 0xfffc) ^ 0x8000) - 0x8000;
 	  int absolute = insn & 2;
@@ -7038,8 +7051,8 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
      it shouldn't be.  */
   set_gdbarch_sofun_address_maybe_missing (gdbarch, 1);

-  /* Handles single stepping of atomic sequences.  */
-  set_gdbarch_software_single_step (gdbarch, ppc_deal_with_atomic_sequence);
+  /* Handles single stepping of power10 prefix insns and atomic sequences.  */
+  set_gdbarch_software_single_step (gdbarch, ppc_software_single_step);

   /* Not sure on this.  FIXMEmgo */
   set_gdbarch_frame_args_skip (gdbarch, 8);
@@ -7070,7 +7083,7 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_displaced_step_restore_all_in_ptid
     (gdbarch, ppc_displaced_step_restore_all_in_ptid);

-  set_gdbarch_max_insn_length (gdbarch, PPC_INSN_SIZE);
+  set_gdbarch_max_insn_length (gdbarch, 2 * PPC_INSN_SIZE);

   /* Hook in ABI-specific overrides, if they have been registered.  */
   info.target_desc = tdesc;

Re: [PATCH] gdb-power10-single-step

[Ulrich Weigand]

Will Schmidt wrote:

>   This is a patch written by Alan Modra.  With his permission
> I'm submitting this for review and helping get this upstream.
> 
> Powerpc / Power10 ISA 3.1 adds prefixed instructions, which
> are 8 bytes in length.  This is in contrast to powerpc previously
> always having 4 byte instruction length.  This patch implements
> changes to allow GDB to better detect prefixed instructions, and
> handle single stepping across the 8 byte instructions.

There's a few issues I see here:

- The patch now *forces* software single-stepping for all 8-byte
  instructions.  I'm not sure why this is necessary; I thought
  that hardware single-stepping was supported for 8-byte instructions
  as well?  That would certainly be preferable.

- However, the inner loop of ppc_deal_with_atomic_sequence should
  probably be updated to correctly skip 8-byte instructions; e.g.
  to avoid mistakenly recognizing the second word of an 8-byte
  instructions for a branch or store conditional.  (Also, the
  count of up to "16 instructions" is wrong if 8-byte instructions
  are not handled specifically.)

- In ppc_displaced_step_fixup 8-byte instructions are now recognized,
  and this is good as far as it goes.  However, there are some 8-byte
  instruction that have PC-relative semantics, and those will need
  an additional fixup.  (Note that this same fixup is already missing
  for the Power9 addpcis instruction, see bug 27525).

Bye,
Ulrich

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

Re: [PATCH] gdb-power10-single-step

[will schmidt]

On Wed, 2021-03-10 at 18:50 +0100, Ulrich Weigand wrote:
> Will Schmidt wrote:
> 
> >   This is a patch written by Alan Modra.  With his permission
> > I'm submitting this for review and helping get this upstream.
> > 
> > Powerpc / Power10 ISA 3.1 adds prefixed instructions, which
> > are 8 bytes in length.  This is in contrast to powerpc previously
> > always having 4 byte instruction length.  This patch implements
> > changes to allow GDB to better detect prefixed instructions, and
> > handle single stepping across the 8 byte instructions.
> 
> There's a few issues I see here:
> 
> - The patch now *forces* software single-stepping for all 8-byte
>   instructions.  I'm not sure why this is necessary; I thought
>   that hardware single-stepping was supported for 8-byte instructions
>   as well?  That would certainly be preferable.

Sounds reasonable.  I shall add to the queue to investigate.

> 
> - However, the inner loop of ppc_deal_with_atomic_sequence should
>   probably be updated to correctly skip 8-byte instructions; e.g.
>   to avoid mistakenly recognizing the second word of an 8-byte
>   instructions for a branch or store conditional.  (Also, the
>   count of up to "16 instructions" is wrong if 8-byte instructions
>   are not handled specifically.)

Noted.

> 
> - In ppc_displaced_step_fixup 8-byte instructions are now recognized,
>   and this is good as far as it goes.  However, there are some 8-byte
>   instruction that have PC-relative semantics, and those will need
>   an additional fixup.  (Note that this same fixup is already missing
>   for the Power9 addpcis instruction, see bug 27525).

I have a separate patch in the works to address 27525, specifically for
the addpcis/lnia instructions.  It does not handle 8-byte instructions
as-is, but I've tried to write it with the idea that i'll be updating
or supplementing it with subsequent patches to support 8-byte
instructions.
I'll be sending that along shortly to get some feedback.

Thanks for the review. 
-Will

> 
> Bye,
> Ulrich
> 

Re: [PATCH] gdb-power10-single-step

[will schmidt]

On Wed, 2021-03-10 at 18:50 +0100, Ulrich Weigand wrote:
> Will Schmidt wrote:
> 
> >   This is a patch written by Alan Modra.  With his permission
> > I'm submitting this for review and helping get this upstream.
> > 
> > Powerpc / Power10 ISA 3.1 adds prefixed instructions, which
> > are 8 bytes in length.  This is in contrast to powerpc previously
> > always having 4 byte instruction length.  This patch implements
> > changes to allow GDB to better detect prefixed instructions, and
> > handle single stepping across the 8 byte instructions.
> 
> There's a few issues I see here:

I've dug in a bit more,.. have a few questions related to the patch and
the comments here.  I've got a refreshed version of this patch in my
queue, with a nit or two that i'm  still trying to understand and
squash before I post it.

> 
> - The patch now *forces* software single-stepping for all 8-byte
>   instructions.  I'm not sure why this is necessary; I thought
>   that hardware single-stepping was supported for 8-byte instructions
>   as well?  That would certainly be preferable.


Does software single-stepping go hand-in-hand with executing the
instructions from a displaced location?
I only see one clear return-if-prefixed change in the patch, so I am
assuming the above refers to the patch chunk seen as :
> @@ -1081,6 +1090,10 @@ ppc_deal_with_atomic_sequence (struct regcache *regcache)
>    const int atomic_sequence_length = 16; /* Instruction sequence length.  */
>    int bc_insn_count = 0; /* Conditional branch instruction count.  */
> 
> +  /* Power10 prefix instructions are two words in length.  */
> +  if ((insn & OP_MASK) == 1 << 26)
> +    return { pc + 8 };


I've got a local change to eliminate that return.   Per the poking I've
done so far, none of the prefix instructions I've run against so far
allow us past the is_load_and_reserve instruction check. 
>   if (!IS_LOAD_AND_RESERVE_INSN (insn))
>     return {};

statement, so not a significant code flow behavior change.



> - However, the inner loop of ppc_deal_with_atomic_sequence should
>   probably be updated to correctly skip 8-byte instructions; e.g.
>   to avoid mistakenly recognizing the second word of an 8-byte
>   instructions for a branch or store conditional.  (Also, the
>   count of up to "16 instructions" is wrong if 8-byte instructions
>   are not handled specifically.)

I've got a local change to inspect the instruction and determine if it
is prefixed, so I think i've got this handled.  I'm generally assuming
we will never start halfway through an 8-byte prefixed instruction.


> - In ppc_displaced_step_fixup 8-byte instructions are now recognized,
>   and this is good as far as it goes.  However, there are some 8-byte
>   instruction that have PC-relative semantics, and those will need
>   an additional fixup.  (Note that this same fixup is already missing
>   for the Power9 addpcis instruction, see bug 27525).

There should be a patch on-list to start to address this.

Thanks,
-Will


> 
> Bye,
> Ulrich
>