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

Message ID 896b57a450849aa1b45408b74f927d2ebd6833a3.camel@vnet.ibm.com
State New
Headers show
Series
  • [rs6000,v3] gdb-power10-single-step
Related show

Commit Message

Mike Frysinger via Gdb-patches March 29, 2021, 10:43 p.m.
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.

Comments

Mike Frysinger via Gdb-patches March 30, 2021, 7:47 p.m. | #1
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

Patch

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