[PATCH,powerpc,v2] gdb-power10-single-step

[PATCH,powerpc,v2] gdb-power10-single-step

[will schmidt]

    Hi,
      This is V2 of the patch that enables power10 single-stepping.
    
    [v1]
    >   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.
    
    [v2]
      Multiple updates, changes and additions based on feedback and
    subsequent work to get breakpoints to behave on prefixed and
    pc-relative instructions.   Including..
    Added #defines to help test for PNOP and prefix instructions.
    Update ppc_displaced_step_copy_insn() to handle pnop and prefixed
    instructions whem R=0 (non-pc-relative).
    
    Update ppc_displaced_step_fixup() to properly handle the offset
    value matching the current instruction size
    
    Update the for-loop within ppc_deal_with_atomic_sequence() to
    count instructions properly in case we have a mix of 4-byte and
    8-byte instructions within the atomic_sequence_length.
    
    Added testcase and harness to exercise pc-relative load/store instructions with R=0.


    YYYY-MM-DD  Will Schmidt  <will_schmidt@vnet.ibm.com>

    gdb/ChangeLog:
	* gdb/rs6000-tdep;c:  Add support for single-stepping of prefixed instructions.

    gdb/testsuite/ChangeLog:
	* gdb/testsuite/gdb.arch/powerpc-plxv-nonrel.s:  Testcase 
	
  using non-relative plxv instructions.
	* gdb/testsuite/gdb.arch/powerpc-plxv-nonrel.exp: Testcase 
	
  harness.



 rs6000-tdep.c                              |   81 ++++++++++++++---
 testsuite/gdb.arch/powerpc-plxv-nonrel.exp |  138 +++++++++++++++++++++++++++++
 testsuite/gdb.arch/powerpc-plxv-nonrel.s   |   30 ++++++
 3 files changed, 237 insertions(+), 12 deletions(-)


diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 1db9663b50..5cd4f978bd 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -839,11 +839,11 @@ constexpr gdb_byte little_breakpoint[] = { 0x08, 0x10, 0x82, 0x7d };
 
 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
 #define BXL_INSN 0x4c000000
 #define BP_INSN 0x7C000008
@@ -867,10 +867,15 @@ typedef BP_MANIPULATION_ENDIAN (little_breakpoint, big_breakpoint)
 #define ADDPCIS_INSN            0x4c000004
 #define ADDPCIS_INSN_MASK       0xfc00003e
 #define ADDPCIS_TARGET_REGISTER 0x03F00000
 #define ADDPCIS_INSN_REGSHIFT   21
 
+#define PNOP_MASK 0xfff3ffff
+#define PNOP_INSN 0x07000000
+#define R_MASK 0x00100000
+#define R_ZERO 0x00000000
+
 /* Check if insn is one of the Load And Reserve instructions used for atomic
    sequences.  */
 #define IS_LOAD_AND_RESERVE_INSN(insn)	((insn & LOAD_AND_RESERVE_MASK) == LWARX_INSTRUCTION \
 					 || (insn & LOAD_AND_RESERVE_MASK) == LDARX_INSTRUCTION \
 					 || (insn & LOAD_AND_RESERVE_MASK) == LBARX_INSTRUCTION \
@@ -897,30 +902,66 @@ ppc_displaced_step_copy_insn (struct gdbarch *gdbarch,
   size_t len = gdbarch_max_insn_length (gdbarch);
   std::unique_ptr<ppc_displaced_step_copy_insn_closure> closure
     (new ppc_displaced_step_copy_insn_closure (len));
   gdb_byte *buf = closure->buf.data ();
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-  int insn;
+  ULONGEST insn;
 
-  read_memory (from, buf, len);
+  len = target_read (current_inferior()->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);
+  displaced_debug_printf ("attempting displacement of instruction as: %16lx, length %ld, ",
+			  insn, len);
+
+  /* If the instruction is prefixed, handle the instruction size properly.  */
+  if ((insn & OP_MASK) == 1 << 26)
+    {
+      /* Handle PNOP.  */
+      /* Handle Prefixed instructions with R=0.  */
+      if (((insn & PNOP_MASK) == PNOP_INSN) ||
+          ((insn & R_MASK) == R_ZERO))
+	 {
+	   insn = extract_signed_integer (buf, 2 * PPC_INSN_SIZE, byte_order);
+	   displaced_debug_printf ("reloaded instruction: %16lx, length %ld",
+				   insn, len);
+	 }
+      else
+	{
+	/* Prefixed with R=1 */
+	/* The prefixed instructions with R=1 will read/write data to/from
+	   locations relative to the current PC.  We would not be able to
+	   fixup after an instruction has written to a displaced location, so
+	   decline to displace those instructions.  */
+	  displaced_debug_printf ("Not displacing prefixed instruction %16lx at %s",
+	  			  insn, paddress (gdbarch, from));
+			return NULL;
+	}
+    } /* prefixed.  */
+  else
+    /* non-prefixed.  */
+    {
+      /* Set the instruction length to 4 to match the actual instruction
+	 length.  */
+      len = 4;
+    }
 
   /* Assume all atomic sequences start with a Load and Reserve instruction.  */
   if (IS_LOAD_AND_RESERVE_INSN (insn))
     {
       displaced_debug_printf ("can't displaced step atomic sequence at %s",
 			      paddress (gdbarch, from));
-
       return NULL;
     }
 
   write_memory (to, buf, len);
 
   displaced_debug_printf ("copy %s->%s: %s",
-			  paddress (gdbarch, from), paddress (gdbarch, to),
-			  displaced_step_dump_bytes (buf, len).c_str ());;
+			paddress (gdbarch, from), paddress (gdbarch, to),
+			displaced_step_dump_bytes (buf, len).c_str ());
 
   /* This is a work around for a problem with g++ 4.8.  */
   return displaced_step_copy_insn_closure_up (closure.release ());
 }
 
@@ -940,11 +981,11 @@ 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;
 
-  opcode = insn & BRANCH_MASK;
+  opcode = insn & OP_MASK;
 
   displaced_debug_printf ("(ppc) fixup (%s, %s)",
 			  paddress (gdbarch, from), paddress (gdbarch, to));
 
   /* Handle the addpcis/lnia instruction.  */
@@ -1028,15 +1069,24 @@ ppc_displaced_step_fixup (struct gdbarch *gdbarch,
 	}
     }
   /* Check for breakpoints in the inferior.  If we've found one, place the PC
      right at the breakpoint instruction.  */
   else if ((insn & BP_MASK) == BP_INSN)
-    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch), from);
+    {
+      regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch), from);
+    }
   else
+    {
   /* Handle any other instructions that do not fit in the categories above.  */
+  /* set offset to 8 if this is an 8-byte (prefixed) instruction. */
+    if ((insn & OP_MASK) == 1 << 26)
+	offset = 2 * PPC_INSN_SIZE;
+    else
+	offset = PPC_INSN_SIZE;
     regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch),
 				    from + offset);
+    }
 }
 
 /* Implementation of gdbarch_displaced_step_prepare.  */
 
 static displaced_step_prepare_status
@@ -1118,17 +1168,24 @@ ppc_deal_with_atomic_sequence (struct regcache *regcache)
 
   /* Assume that no atomic sequence is longer than "atomic_sequence_length" 
      instructions.  */
   for (insn_count = 0; insn_count < atomic_sequence_length; ++insn_count)
     {
-      loc += PPC_INSN_SIZE;
-      insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
+	int cur_insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
+	if ((cur_insn & OP_MASK) == 1 << 26) {
+	  loc += 2*PPC_INSN_SIZE;
+	  insn = read_memory_integer (loc, 2 *  PPC_INSN_SIZE, byte_order);
+	} else {
+	  loc += PPC_INSN_SIZE;
+	  insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
+	}
+    fprintf_unfiltered(gdb_stdlog,"Warning: read_memory_integer: instruction found here. %08x \n",insn);
 
       /* 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;
 
 	  if (bc_insn_count >= 1)
@@ -7099,11 +7156,11 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_displaced_step_prepare (gdbarch, ppc_displaced_step_prepare);
   set_gdbarch_displaced_step_finish (gdbarch, ppc_displaced_step_finish);
   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;
   info.tdesc_data = tdesc_data.get ();
   gdbarch_init_osabi (info, gdbarch);
diff --git a/gdb/testsuite/gdb.arch/powerpc-plxv-nonrel.exp b/gdb/testsuite/gdb.arch/powerpc-plxv-nonrel.exp
new file mode 100644
index 0000000000..c12ff0076c
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc-plxv-nonrel.exp
@@ -0,0 +1,138 @@
+# Copyright 2021 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test to see if gdb is properly single stepping over the
+# displaced plxv instruction.
+# plxv is an extended mnemonic for the addpcis instruction, which
+# stores the $NIA plus an immediate value into a register.
+#
+#		plxv Rx == addpcis Rx,0 == plxv Rx
+#		subcis Rx,value == addpcis Rx,-value
+
+if { ![istarget powerpc*-*] } {
+    verbose "Skipping powerpc plxv test."
+    return
+}
+
+set retval 0
+
+set testfile "powerpc-plxv-nonrel"
+set srcfile ${testfile}.s
+set binfile [standard_output_file ${testfile}]
+
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {}] != "" } {
+    untested "PowerPC plxv test"
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_load ${binfile}
+gdb_test "set radix 0b10000"
+gdb_test "set debug displaced"
+
+if ![runto_main] then {
+      return
+}
+
+gdb_test "set debug displaced on"
+
+proc get_vector_hexadecimal_valueof { exp default {test ""} } {
+	set val "0x0000"
+	global gdb_prompt
+	if {$test == ""} {
+		set test "get vector_hexadecimal valueof \"${exp}\""
+	}
+	gdb_test_multiple "print $${exp}.uint128" $test {
+		-re "\\$\[0-9\]* = (0x\[0-9a-zA-Z\]+).*$gdb_prompt $" {
+			set val $expect_out(1,string)
+				pass "$test"
+		}
+		-re ".*Illegal instruction.*${gdb_prompt} $" {
+			fail "Illegal instruction on print."
+			set val 0xffff
+		}
+	}
+	return ${val}
+}
+
+proc stepi_over_instruction { xyz } {
+	global gdb_prompt
+	gdb_test_multiple "stepi" "${xyz} " {
+		-re ".*Illegal instruction.*${gdb_prompt}" {
+			fail "Illegal instruction on single step."
+		return
+		}
+		-re ".*${gdb_prompt}" {
+		 pass "stepi ${xyz}"
+		}
+	}
+}
+
+set check_pc [get_hexadecimal_valueof "\$pc" "default0"]
+
+# set some breakpoints on the instructions below main().
+gdb_test "disas /r main"
+set bp1 *$check_pc+4
+set bp2 *$check_pc+0d12
+set bp3 *$check_pc+0d20
+set bp4 *$check_pc+0d28
+gdb_breakpoint $bp1
+gdb_breakpoint $bp2
+gdb_breakpoint $bp3
+gdb_breakpoint $bp4
+
+# single-step through the plxv instructions, and retrieve the
+# register values as we proceed.
+
+stepi_over_instruction  "stepi over NOP"
+stepi_over_instruction  "stepi over lnia"
+stepi_over_instruction  "stepi over addi"
+
+stepi_over_instruction  "stepi over vs4 assignment"
+set check_vs4 [get_vector_hexadecimal_valueof "vs4" "default0"]
+
+stepi_over_instruction  "stepi over vs5 assignment"
+set check_vs5 [get_vector_hexadecimal_valueof "vs5" "default0"]
+
+stepi_over_instruction  "stepi over vs6 assignment"
+set check_vs6 [get_vector_hexadecimal_valueof "vs6" "default0"]
+
+stepi_over_instruction  "stepi over vs7 assignment"
+set check_vs7 [get_vector_hexadecimal_valueof "vs7" "default0"]
+
+set vs4_expected 0xa5b5c5d5a4b4c4d4a3b3c3d3a2b2c2d2
+set vs5_expected 0xa7b7c7d7a6b6c6d6a5b5c5d5a4b4c4d4
+set vs6_expected 0xa9b9c9d9a8b8c8d8a7b7c7d7a6b6c6d6
+set vs7_expected 0xabbbcbdbaabacadaa9b9c9d9a8b8c8d8
+
+if [expr  $check_vs4 != $vs4_expected] {
+    fail "unexpected value vs4;  actual:$check_vs4 expected:$vs4_expected"
+}
+if [expr $check_vs5 != $vs5_expected ] {
+    fail "unexpected value vs5;   actual:$check_vs5 expected:$vs5_expected"
+}
+if [expr $check_vs6 != $vs6_expected ] {
+    fail "unexpected value vs6;   actual:$check_vs6 expected:$vs6_expected"
+}
+if [expr $check_vs7 != $vs7_expected ] {
+    fail "unexpected value vs7;   actual:$check_vs7 expected:$vs7_expected"
+}
+
+gdb_test "info break"
+gdb_test "info register vs4 vs5 vs6 vs7 "
+gdb_test "disas main #2"
+
+
diff --git a/gdb/testsuite/gdb.arch/powerpc-plxv-nonrel.s b/gdb/testsuite/gdb.arch/powerpc-plxv-nonrel.s
new file mode 100644
index 0000000000..e40f08a875
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc-plxv-nonrel.s
@@ -0,0 +1,30 @@
+
+# test to verify that the prefixed instructions that
+# load/store non-relative values work OK.
+
+.global main
+.type main,function
+main:
+	nop
+	lnia 4
+	addi 4,4,40
+	plxv 4,4(4),0
+	plxv 5,12(4),0
+	plxv 6,20(4),0
+	plxv 7,28(4),0
+check_here:
+	blr
+mydata:
+	.long 0xa1b1c1d1	# <<-
+	.long 0xa2b2c2d2	# <<- loaded into vs4
+	.long 0xa3b3c3d3	# <<- loaded into vs4
+	.long 0xa4b4c4d4	# <<- loaded into vs4, vs5
+	.long 0xa5b5c5d5	# <<- loaded into vs4, vs5
+	.long 0xa6b6c6d6	# <<- loaded into      vs5, vs6
+	.long 0xa7b7c7d7	# <<- loaded into      vs5, vs6
+	.long 0xa8b8c8d8	# <<- loaded into           vs6, vs7
+	.long 0xa9b9c9d9	# <<- loaded into           vs6, vs7
+	.long 0xaabacada	# <<- loaded into                vs7
+	.long 0xabbbcbdb	# <<- loaded into                vs7
+	.long 0xacbcccdc	# <<-
+

Re: [PATCH,powerpc,v2] gdb-power10-single-step

[Ulrich Weigand]

On Fri, Mar 26, 2021 at 07:25:15PM -0500, will schmidt wrote:

>     gdb/ChangeLog:
> 	* gdb/rs6000-tdep;c:  Add support for single-stepping of prefixed instructions.
> 
>     gdb/testsuite/ChangeLog:
> 	* gdb/testsuite/gdb.arch/powerpc-plxv-nonrel.s:  Testcase 
> 	
>   using non-relative plxv instructions.
> 	* gdb/testsuite/gdb.arch/powerpc-plxv-nonrel.exp: Testcase 
> 	
>   harness.

Again omit the leading "gdb/" or "gdb/testsuite/".  Names should always be
relative to the location of the ChangeLog file that contains the entry.

> @@ -897,30 +902,66 @@ ppc_displaced_step_copy_insn (struct gdbarch *gdbarch,
>    size_t len = gdbarch_max_insn_length (gdbarch);
>    std::unique_ptr<ppc_displaced_step_copy_insn_closure> closure
>      (new ppc_displaced_step_copy_insn_closure (len));
>    gdb_byte *buf = closure->buf.data ();
>    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> -  int insn;
> +  ULONGEST insn;
>  
> -  read_memory (from, buf, len);
> +  len = target_read (current_inferior()->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);
> +  displaced_debug_printf ("attempting displacement of instruction as: %16lx, length %ld, ",
> +			  insn, len);

Again, there's too many debug statements, they will flood the log and make
it not really usable.  We don't need to print every single instruction here.

> +  /* If the instruction is prefixed, handle the instruction size properly.  */
> +  if ((insn & OP_MASK) == 1 << 26)
> +    {
> +      /* Handle PNOP.  */
> +      /* Handle Prefixed instructions with R=0.  */
> +      if (((insn & PNOP_MASK) == PNOP_INSN) ||
> +          ((insn & R_MASK) == R_ZERO))
> +	 {
> +	   insn = extract_signed_integer (buf, 2 * PPC_INSN_SIZE, byte_order);
> +	   displaced_debug_printf ("reloaded instruction: %16lx, length %ld",
> +				   insn, len);

Likewise we don't need to display each instruction here.  This whole block
is probably unnecessary.

> +	 }
> +      else
> +	{
> +	/* Prefixed with R=1 */
> +	/* The prefixed instructions with R=1 will read/write data to/from
> +	   locations relative to the current PC.  We would not be able to
> +	   fixup after an instruction has written to a displaced location, so
> +	   decline to displace those instructions.  */
> +	  displaced_debug_printf ("Not displacing prefixed instruction %16lx at %s",
> +	  			  insn, paddress (gdbarch, from));

This statement is actually useful and should be kept.

> +			return NULL;
> +	}
> +    } /* prefixed.  */
> +  else
> +    /* non-prefixed.  */
> +    {
> +      /* Set the instruction length to 4 to match the actual instruction
> +	 length.  */
> +      len = 4;
> +    }

The formatting is odd throughout -- is this just an issue with the email?
Please verify it complies with the GNU coding style.

>    /* Assume all atomic sequences start with a Load and Reserve instruction.  */
>    if (IS_LOAD_AND_RESERVE_INSN (insn))
>      {
>        displaced_debug_printf ("can't displaced step atomic sequence at %s",
>  			      paddress (gdbarch, from));
> -
>        return NULL;
>      }
>  
>    write_memory (to, buf, len);
>  
>    displaced_debug_printf ("copy %s->%s: %s",
> -			  paddress (gdbarch, from), paddress (gdbarch, to),
> -			  displaced_step_dump_bytes (buf, len).c_str ());;
> +			paddress (gdbarch, from), paddress (gdbarch, to),
> +			displaced_step_dump_bytes (buf, len).c_str ());

Please omit those whitespace changes.

>    /* Check for breakpoints in the inferior.  If we've found one, place the PC
>       right at the breakpoint instruction.  */
>    else if ((insn & BP_MASK) == BP_INSN)
> -    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch), from);
> +    {
> +      regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch), from);
> +    }
>    else
> +    {
>    /* Handle any other instructions that do not fit in the categories above.  */
> +  /* set offset to 8 if this is an 8-byte (prefixed) instruction. */
> +    if ((insn & OP_MASK) == 1 << 26)
> +	offset = 2 * PPC_INSN_SIZE;
> +    else
> +	offset = PPC_INSN_SIZE;
>      regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch),
>  				    from + offset);
> +    }

Check the formatting again.   Also, I think it would be better to move
calculating the correct offset to the top of the function, just after
insn is read initially.


>    for (insn_count = 0; insn_count < atomic_sequence_length; ++insn_count)
>      {
> -      loc += PPC_INSN_SIZE;
> -      insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
> +	int cur_insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
> +	if ((cur_insn & OP_MASK) == 1 << 26) {
> +	  loc += 2*PPC_INSN_SIZE;
> +	  insn = read_memory_integer (loc, 2 *  PPC_INSN_SIZE, byte_order);

This seems wrong to me: subsequent code expects "insn" to be a 4-byte value
(e.g. for the if ((insn & OP_MASK) == BC_INSN) check).

Why do you have the read the full 8 byte instruction in the first place?
Nothing depends on the second half.

> +	} else {
> +	  loc += PPC_INSN_SIZE;
> +	  insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
> +	}

Position of the '{' is definitely wrong.  Also check the indentation.

> +    fprintf_unfiltered(gdb_stdlog,"Warning: read_memory_integer: instruction found here. %08x \n",insn);

This seems left-over from debugging?

Bye,
Ulrich

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

[PATCH,rs6000,v3] gdb-power10-single-step

[will schmidt]

Hi,
  This is V3 of the patch that enables power10 single-stepping.

[v1]
>   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.

[v2]
  Multiple updates, changes and additions based on feedback and
subsequent work to get breakpoints to behave on prefixed and
pc-relative instructions.   Including..
Added #defines to help test for PNOP and prefix instructions.
Update ppc_displaced_step_copy_insn() to handle pnop and prefixed
instructions whem R=0 (non-pc-relative).

Update ppc_displaced_step_fixup() to properly handle the offset
value matching the current instruction size

Update the for-loop within ppc_deal_with_atomic_sequence() to
count instructions properly in case we have a mix of 4-byte and
8-byte instructions within the atomic_sequence_length.

Added testcase and harness to exercise pc-relative load/store
instructions with R=0.
    
[v3]
  Assorted updates per review feedback.  Fixed ChangeLog. Removed
some extraneous debug statements.  Used -wrap in testcase harnesses.

YYYY-MM-DD  Will Schmidt  <will_schmidt@vnet.ibm.com>

	gdb/ChangeLog:
	* rs6000-tdep.c:  Add support for single-stepping of
	prefixed instructions.

	gdb/testsuite/ChangeLog:
	* gdb.arch/powerpc-plxv-nonrel.s:  Testcase using
	non-relative plxv instructions.
	* /gdb.arch/powerpc-plxv-nonrel.exp: Testcase harness.

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index cbb3e013ad9..8861fd5d0f2 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -839,11 +839,11 @@ constexpr gdb_byte little_breakpoint[] = { 0x08, 0x10, 0x82, 0x7d };
 
 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
 #define BXL_INSN 0x4c000000
 #define BP_INSN 0x7C000008
@@ -867,10 +867,15 @@ typedef BP_MANIPULATION_ENDIAN (little_breakpoint, big_breakpoint)
 #define ADDPCIS_INSN            0x4c000004
 #define ADDPCIS_INSN_MASK       0xfc00003e
 #define ADDPCIS_TARGET_REGISTER 0x03F00000
 #define ADDPCIS_INSN_REGSHIFT   21
 
+#define PNOP_MASK 0xfff3ffff
+#define PNOP_INSN 0x07000000
+#define R_MASK 0x00100000
+#define R_ZERO 0x00000000
+
 /* Check if insn is one of the Load And Reserve instructions used for atomic
    sequences.  */
 #define IS_LOAD_AND_RESERVE_INSN(insn)	((insn & LOAD_AND_RESERVE_MASK) == LWARX_INSTRUCTION \
 					 || (insn & LOAD_AND_RESERVE_MASK) == LDARX_INSTRUCTION \
 					 || (insn & LOAD_AND_RESERVE_MASK) == LBARX_INSTRUCTION \
@@ -899,14 +904,40 @@ ppc_displaced_step_copy_insn (struct gdbarch *gdbarch,
     (new ppc_displaced_step_copy_insn_closure (len));
   gdb_byte *buf = closure->buf.data ();
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   int insn;
 
-  read_memory (from, buf, len);
+  len = target_read (current_inferior()->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);
 
+  /* Check for PNOP and for prefixed instructions with R=0.  Those
+     instructions are safe to displace.  Prefixed instructions with R=1
+     will read/write data to/from locations relative to the current PC.
+     We would not be able to fixup after an instruction has written data
+    into a displaced location, so decline to displace those instructions.  */
+  if ((insn & OP_MASK) == 1 << 26)
+    {
+      if (((insn & PNOP_MASK) != PNOP_INSN) &&
+          ((insn & R_MASK) != R_ZERO))
+	{
+	  displaced_debug_printf ("Not displacing prefixed instruction %08x at %s",
+						insn, paddress (gdbarch, from));
+	  return NULL;
+	}
+    }
+  else
+    /* Non-prefixed instructions..  */
+    {
+      /* Set the instruction length to 4 to match the actual instruction
+	 length.  */
+      len = 4;
+    }
+
   /* Assume all atomic sequences start with a Load and Reserve instruction.  */
   if (IS_LOAD_AND_RESERVE_INSN (insn))
     {
       displaced_debug_printf ("can't displaced step atomic sequence at %s",
 			      paddress (gdbarch, from));
@@ -916,11 +947,11 @@ ppc_displaced_step_copy_insn (struct gdbarch *gdbarch,
 
   write_memory (to, buf, len);
 
   displaced_debug_printf ("copy %s->%s: %s",
 			  paddress (gdbarch, from), paddress (gdbarch, to),
-			  displaced_step_dump_bytes (buf, len).c_str ());;
+			  displaced_step_dump_bytes (buf, len).c_str ());
 
   /* This is a work around for a problem with g++ 4.8.  */
   return displaced_step_copy_insn_closure_up (closure.release ());
 }
 
@@ -936,15 +967,21 @@ ppc_displaced_step_fixup (struct gdbarch *gdbarch,
   /* Our closure is a copy of the instruction.  */
   ppc_displaced_step_copy_insn_closure *closure
     = (ppc_displaced_step_copy_insn_closure *) closure_;
   ULONGEST insn  = extract_unsigned_integer (closure->buf.data (),
 					     PPC_INSN_SIZE, byte_order);
-  ULONGEST opcode = 0;
+  ULONGEST opcode;
   /* Offset for non PC-relative instructions.  */
-  LONGEST offset = PPC_INSN_SIZE;
+  LONGEST offset;
 
-  opcode = insn & BRANCH_MASK;
+  opcode = insn & OP_MASK;
+
+  /* set offset to 8 if this is an 8-byte (prefixed) instruction. */
+    if ((opcode) == 1 << 26)
+	offset = 2 * PPC_INSN_SIZE;
+    else
+	offset = PPC_INSN_SIZE;
 
   displaced_debug_printf ("(ppc) fixup (%s, %s)",
 			  paddress (gdbarch, from), paddress (gdbarch, to));
 
   /* Handle the addpcis/lnia instruction.  */
@@ -1112,17 +1149,21 @@ ppc_deal_with_atomic_sequence (struct regcache *regcache)
 
   /* Assume that no atomic sequence is longer than "atomic_sequence_length" 
      instructions.  */
   for (insn_count = 0; insn_count < atomic_sequence_length; ++insn_count)
     {
-      loc += PPC_INSN_SIZE;
+      int cur_insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
+      if ((cur_insn & OP_MASK) == 1 << 26)
+	loc += 2*PPC_INSN_SIZE;
+      else
+	loc += PPC_INSN_SIZE;
       insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
 
       /* 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;
 
 	  if (bc_insn_count >= 1)
@@ -7094,11 +7135,11 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_displaced_step_prepare (gdbarch, ppc_displaced_step_prepare);
   set_gdbarch_displaced_step_finish (gdbarch, ppc_displaced_step_finish);
   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;
   info.tdesc_data = tdesc_data.get ();
   gdbarch_init_osabi (info, gdbarch);
diff --git a/gdb/testsuite/gdb.arch/powerpc-plxv-nonrel.exp b/gdb/testsuite/gdb.arch/powerpc-plxv-nonrel.exp
new file mode 100644
index 00000000000..08f1a379efb
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc-plxv-nonrel.exp
@@ -0,0 +1,131 @@
+# Copyright 2021 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test to see if gdb is properly single stepping over the
+# displaced plxv instruction.
+
+if { ![istarget powerpc*-*] } {
+    verbose "Skipping powerpc plxv test."
+    return
+}
+
+set retval 0
+
+standard_testfile .s
+
+if { [prepare_for_testing "failed to prepare" $testfile "$srcfile" \
+      {debug quiet}] } {
+    return -1
+}
+
+gdb_test "set radix 0b10000"
+gdb_test "set debug displaced"
+
+if ![runto_main] then {
+      return
+}
+
+gdb_test "set debug displaced on"
+
+# Proc to extract the uint128 hex value from the output of
+# a print vector statement.
+proc get_vector_hexadecimal_valueof { exp default {test ""} } {
+	set val "0x0000"
+	global gdb_prompt
+	if {$test == ""} {
+		set test "get vector_hexadecimal valueof \"${exp}\""
+	}
+	gdb_test_multiple "print $${exp}.uint128" $test {
+		-re -wrap "\\$\[0-9\]* = (0x\[0-9a-zA-Z\]+).*" {
+			set val $expect_out(1,string)
+				pass "$test"
+		}
+		-re -wrap ".*Illegal instruction.* $" {
+			fail "Illegal instruction on print."
+			set val 0xffff
+		}
+	}
+	return ${val}
+}
+
+# Proc to do a single-step, and ensure we gently handle
+# an illegal instruction situation.
+proc stepi_over_instruction { xyz } {
+	global gdb_prompt
+	gdb_test_multiple "stepi" "${xyz} " {
+		-re -wrap ".*Illegal instruction.*" {
+			fail "Illegal instruction on single step."
+		return
+		}
+		-re -wrap ".*" {
+		 pass "stepi ${xyz}"
+		}
+	}
+}
+
+set check_pc [get_hexadecimal_valueof "\$pc" "default0"]
+
+# set some breakpoints on the instructions below main().
+gdb_test "disas /r main"
+set bp1 *$check_pc+4
+set bp2 *$check_pc+0d12
+set bp3 *$check_pc+0d20
+set bp4 *$check_pc+0d28
+gdb_breakpoint $bp1
+gdb_breakpoint $bp2
+gdb_breakpoint $bp3
+gdb_breakpoint $bp4
+
+# single-step through the plxv instructions, and retrieve the
+# register values as we proceed.
+
+stepi_over_instruction  "stepi over NOP"
+stepi_over_instruction  "stepi over lnia"
+stepi_over_instruction  "stepi over addi"
+
+stepi_over_instruction  "stepi over vs4 assignment"
+set check_vs4 [get_vector_hexadecimal_valueof "vs4" "default0"]
+
+stepi_over_instruction  "stepi over vs5 assignment"
+set check_vs5 [get_vector_hexadecimal_valueof "vs5" "default0"]
+
+stepi_over_instruction  "stepi over vs6 assignment"
+set check_vs6 [get_vector_hexadecimal_valueof "vs6" "default0"]
+
+stepi_over_instruction  "stepi over vs7 assignment"
+set check_vs7 [get_vector_hexadecimal_valueof "vs7" "default0"]
+
+set vs4_expected 0xa5b5c5d5a4b4c4d4a3b3c3d3a2b2c2d2
+set vs5_expected 0xa7b7c7d7a6b6c6d6a5b5c5d5a4b4c4d4
+set vs6_expected 0xa9b9c9d9a8b8c8d8a7b7c7d7a6b6c6d6
+set vs7_expected 0xabbbcbdbaabacadaa9b9c9d9a8b8c8d8
+
+if [expr  $check_vs4 != $vs4_expected] {
+    fail "unexpected value vs4;  actual:$check_vs4 expected:$vs4_expected"
+}
+if [expr $check_vs5 != $vs5_expected ] {
+    fail "unexpected value vs5;   actual:$check_vs5 expected:$vs5_expected"
+}
+if [expr $check_vs6 != $vs6_expected ] {
+    fail "unexpected value vs6;   actual:$check_vs6 expected:$vs6_expected"
+}
+if [expr $check_vs7 != $vs7_expected ] {
+    fail "unexpected value vs7;   actual:$check_vs7 expected:$vs7_expected"
+}
+
+gdb_test "info break"
+gdb_test "info register vs4 vs5 vs6 vs7 "
+gdb_test "disas main #2"
+
diff --git a/gdb/testsuite/gdb.arch/powerpc-plxv-nonrel.s b/gdb/testsuite/gdb.arch/powerpc-plxv-nonrel.s
new file mode 100644
index 00000000000..4708b214bb0
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc-plxv-nonrel.s
@@ -0,0 +1,45 @@
+# Copyright 2021 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+
+# test to verify that the prefixed instructions that
+# load/store non-relative values work OK.
+
+.global main
+.type main,function
+main:
+	nop
+	lnia 4
+	addi 4,4,40
+	plxv 4,4(4),0
+	plxv 5,12(4),0
+	plxv 6,20(4),0
+	plxv 7,28(4),0
+check_here:
+	blr
+mydata:
+	.long 0xa1b1c1d1	# <<-
+	.long 0xa2b2c2d2	# <<- loaded into vs4
+	.long 0xa3b3c3d3	# <<- loaded into vs4
+	.long 0xa4b4c4d4	# <<- loaded into vs4, vs5
+	.long 0xa5b5c5d5	# <<- loaded into vs4, vs5
+	.long 0xa6b6c6d6	# <<- loaded into      vs5, vs6
+	.long 0xa7b7c7d7	# <<- loaded into      vs5, vs6
+	.long 0xa8b8c8d8	# <<- loaded into           vs6, vs7
+	.long 0xa9b9c9d9	# <<- loaded into           vs6, vs7
+	.long 0xaabacada	# <<- loaded into                vs7
+	.long 0xabbbcbdb	# <<- loaded into                vs7
+	.long 0xacbcccdc	# <<-
+

Re: [PATCH,rs6000,v3] gdb-power10-single-step

[Ulrich Weigand]

On Mon, Mar 29, 2021 at 05:43:53PM -0500, will schmidt wrote:

> YYYY-MM-DD  Will Schmidt  <will_schmidt@vnet.ibm.com>
> 
> 	gdb/ChangeLog:
> 	* rs6000-tdep.c:  Add support for single-stepping of
> 	prefixed instructions.
> 
> 	gdb/testsuite/ChangeLog:
> 	* gdb.arch/powerpc-plxv-nonrel.s:  Testcase using
> 	non-relative plxv instructions.
> 	* /gdb.arch/powerpc-plxv-nonrel.exp: Testcase harness.

Stray '/' at the beginning.

> +  if ((ssize_t) len < PPC_INSN_SIZE)
> +		memory_error (TARGET_XFER_E_IO, from);

Incorrect indentation.

> +  /* Check for PNOP and for prefixed instructions with R=0.  Those
> +     instructions are safe to displace.  Prefixed instructions with R=1
> +     will read/write data to/from locations relative to the current PC.
> +     We would not be able to fixup after an instruction has written data
> +    into a displaced location, so decline to displace those instructions.  */
> +  if ((insn & OP_MASK) == 1 << 26)
> +    {
> +      if (((insn & PNOP_MASK) != PNOP_INSN) &&
> +          ((insn & R_MASK) != R_ZERO))

GDB coding style is to have the && at the start of the second line.

> +	{
> +	  displaced_debug_printf ("Not displacing prefixed instruction %08x at %s",
> +						insn, paddress (gdbarch, from));

Incorrect indentation.

> +  /* set offset to 8 if this is an 8-byte (prefixed) instruction. */

Start with a capital 'S' and end with two spaces after the '.'.

> +    if ((opcode) == 1 << 26)
> +	offset = 2 * PPC_INSN_SIZE;
> +    else
> +	offset = PPC_INSN_SIZE;

Wrong indentation.

>       instructions.  */
>    for (insn_count = 0; insn_count < atomic_sequence_length; ++insn_count)
>      {
> -      loc += PPC_INSN_SIZE;
> +      int cur_insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
> +      if ((cur_insn & OP_MASK) == 1 << 26)
> +	loc += 2*PPC_INSN_SIZE;
> +      else
> +	loc += PPC_INSN_SIZE;
>        insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);

Hmm.  This reads all instruction twice now, which we should avoid for
performance reasons.  But it is not actually necessary: at the start
of this loop, "insn" is always the instruction at "loc", read either
by the code before the loop or the previous iteration of the loop.
So you can just use "insn" for the prefix check.

(Also, indentation seems wrong :-))


Except for this one issue with reading the instructions twice, this
looks now ready to commit (once the coding style issues are fixed).

Thanks,
Ulrich

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