[PATCH 1/2] ppc: use 'trap' ('tw, 31, 0, 0', 0x7fe00008) as breakpoint instruction

[PATCH 1/2] ppc: use 'trap' ('tw, 31, 0, 0', 0x7fe00008) as breakpoint instruction

[Jan Vrany]

GDB used to use "tw 12, r2, r2" as a breakpoint instruction. While it
works, the PowerPC specifies 'tw, 31, 0, 0' (0x7fe00008) as the
canonical unconditional trap.
---
 gdb/rs6000-tdep.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 87a494e0bb8..43880fa4426 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -824,8 +824,8 @@ rs6000_fetch_pointer_argument (struct frame_info *frame, int argi,
 
 /* Sequence of bytes for breakpoint instruction.  */
 
-constexpr gdb_byte big_breakpoint[] = { 0x7d, 0x82, 0x10, 0x08 };
-constexpr gdb_byte little_breakpoint[] = { 0x08, 0x10, 0x82, 0x7d };
+constexpr gdb_byte big_breakpoint[] = { 0x7f, 0xe0, 0x00, 0x08 };
+constexpr gdb_byte little_breakpoint[] = { 0x08, 0x00, 0xe0, 0x7f };
 
 typedef BP_MANIPULATION_ENDIAN (little_breakpoint, big_breakpoint)
   rs6000_breakpoint;
-- 
2.30.2

[PATCH 2/2] ppc: recognize all program traps

[Jan Vrany]

Permanent program breakpoints (ones inserted into the code) other than
the one GDB uses for POWER (0x7fe00008) did not result in stop but
caused GDB to loop infinitely.

This was because GDB did not recognize trap instructions other than
"trap". For example, "tw 12, 4, 4" was not be recognized, causing GDB
to loop forever.

This commit fixes this by providing POWER specific hook
(gdbarch_program_breakpoint_here_p) recognizing all tw, twi, td and tdi
instructions.

Tested on Linux on PowerPC e500 and on QEMU PPC64le.
---
 gdb/rs6000-tdep.c                       | 66 +++++++++++++++++++++++
 gdb/testsuite/gdb.arch/powerpc-trap.exp | 72 +++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/powerpc-trap.s   | 30 +++++++++++
 gdb/testsuite/gdb.arch/ppc64-trap.exp   | 72 +++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/ppc64-trap.s     | 32 +++++++++++
 5 files changed, 272 insertions(+)
 create mode 100644 gdb/testsuite/gdb.arch/powerpc-trap.exp
 create mode 100644 gdb/testsuite/gdb.arch/powerpc-trap.s
 create mode 100644 gdb/testsuite/gdb.arch/ppc64-trap.exp
 create mode 100644 gdb/testsuite/gdb.arch/ppc64-trap.s

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 43880fa4426..eeafca1ff99 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -6247,6 +6247,70 @@ ppc_process_record (struct gdbarch *gdbarch, struct regcache *regcache,
   return 0;
 }
 
+/* Used for matching tw, twi, td and tdi instructions for POWER.  */
+
+static constexpr uint32_t TX_INSN_MASK = 0xFC0007FF;
+static constexpr uint32_t TW_INSN = 0x7C000008;
+static constexpr uint32_t TD_INSN = 0x7C000088;
+
+static constexpr uint32_t TXI_INSN_MASK = 0xFC000000;
+static constexpr uint32_t TWI_INSN = 0x0C000000;
+static constexpr uint32_t TDI_INSN = 0x08000000;
+
+static inline bool
+is_tw_insn (uint32_t insn)
+{
+  return (insn & TX_INSN_MASK) == TW_INSN;
+}
+
+static inline bool
+is_twi_insn (uint32_t insn)
+{
+  return (insn & TXI_INSN_MASK) == TWI_INSN;
+}
+
+static inline bool
+is_td_insn (uint32_t insn)
+{
+  return (insn & TX_INSN_MASK) == TD_INSN;
+}
+
+static inline bool
+is_tdi_insn (uint32_t insn)
+{
+  return (insn & TXI_INSN_MASK) == TDI_INSN;
+}
+
+/* Implementation of gdbarch_program_breakpoint_here_p for POWER.  */
+
+static bool
+rs6000_program_breakpoint_here_p (gdbarch *gdbarch, CORE_ADDR address)
+{
+  const uint32_t insn_len = 4;
+  gdb_byte target_mem[4];
+
+  /* Enable the automatic memory restoration from breakpoints while
+     we read the memory.  Otherwise we may find temporary breakpoints, ones
+     inserted by GDB, and flag them as permanent breakpoints.  */
+  scoped_restore restore_memory
+      = make_scoped_restore_show_memory_breakpoints (0);
+
+  if (target_read_memory (address, target_mem, insn_len) == 0)
+    {
+      uint32_t insn = (uint32_t)extract_unsigned_integer (
+          target_mem, insn_len, gdbarch_byte_order_for_code (gdbarch));
+
+      /* Check if INSN is a TW, TWI, TD or TDI instruction.  There
+         are multiple choices of such instructions with different registers
+         and / or immediate values but they all cause a break. */
+      if (is_tw_insn (insn) || is_twi_insn (insn) || is_td_insn (insn)
+          || is_tdi_insn (insn))
+        return true;
+    }
+
+  return false;
+}
+
 /* Initialize the current architecture based on INFO.  If possible, re-use an
    architecture from ARCHES, which is a list of architectures already created
    during this debugging session.
@@ -7109,6 +7173,8 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 				       rs6000_breakpoint::kind_from_pc);
   set_gdbarch_sw_breakpoint_from_kind (gdbarch,
 				       rs6000_breakpoint::bp_from_kind);
+  set_gdbarch_program_breakpoint_here_p (gdbarch,
+                                         rs6000_program_breakpoint_here_p);
 
   /* The value of symbols of type N_SO and N_FUN maybe null when
      it shouldn't be.  */
diff --git a/gdb/testsuite/gdb.arch/powerpc-trap.exp b/gdb/testsuite/gdb.arch/powerpc-trap.exp
new file mode 100644
index 00000000000..4e86bd4b9a1
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc-trap.exp
@@ -0,0 +1,72 @@
+# Copyright 2020-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/>.
+#
+# This file is part of the gdb testsuite.
+
+# Test if GDB stops at various trap instructions inserted into
+# the code.
+
+if { ![istarget powerpc-*] } {
+    verbose "Skipping ${gdb_test_file_name}."
+    return
+}
+
+standard_testfile .s
+if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile}]} {
+    return -1
+}
+
+if {![runto_main]} {
+    untested "could not run to main"
+    return -1
+}
+
+# Number of expected SIGTRAP's to get.  This needs to be kept in sync
+# with the source file.
+set expected_traps 3
+set keep_going 1
+set count 0
+
+# Make sure we have a lower timeout in case GDB doesn't support a particular
+# instruction.  Such instruction will cause GDB to loop infinitely.
+while {$keep_going} {
+    # Continue to next program breakpoint instruction.
+    gdb_test_multiple "continue" "trap instruction $count causes SIGTRAP" {
+	-re "Program received signal SIGTRAP, Trace/breakpoint trap.*$gdb_prompt $" {
+	    pass $gdb_test_name
+
+	    # Advance PC to nex instruction
+	    gdb_test "set \$pc = \$pc + 4" "" "advance past trap instruction $count"
+
+	 #    # Insert a breakpoint at the program breakpoint instruction so
+	 #    # GDB can step over it.
+	 #    gdb_test "break" \
+		# "Breakpoint $decimal at $hex: file .*$srcfile, line $decimal.*" \
+		# "insert breakpoint at trap instruction $count"
+	    incr count
+	}
+	# We've reached the end of the test.
+	-re "exited with code 01.*$gdb_prompt $" {
+	    set keep_going 0
+	}
+	timeout {
+	    fail $gdb_test_name
+	    set keep_going 0
+	}
+    }
+}
+
+# Verify we stopped at the expected number of SIGTRAP's.
+gdb_assert {$count == $expected_traps} "all trap instructions triggered"
diff --git a/gdb/testsuite/gdb.arch/powerpc-trap.s b/gdb/testsuite/gdb.arch/powerpc-trap.s
new file mode 100644
index 00000000000..c503cb1e33e
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc-trap.s
@@ -0,0 +1,30 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   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/>. */
+
+/* To test if GDB stops at various trap instructions inserted into
+   the code.  */
+
+.global main
+.type main,function
+main:
+  ori 0, 0, 0
+  trap
+  tw  12, 2, 2
+  twi 31, 3, 3
+  ori 0, 0, 0
+  blr
+
diff --git a/gdb/testsuite/gdb.arch/ppc64-trap.exp b/gdb/testsuite/gdb.arch/ppc64-trap.exp
new file mode 100644
index 00000000000..1341808b9fd
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/ppc64-trap.exp
@@ -0,0 +1,72 @@
+# Copyright 2020-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/>.
+#
+# This file is part of the gdb testsuite.
+
+# Test if GDB stops at various trap instructions inserted into
+# the code.
+
+if { ![istarget powerpc64-*] } {
+    verbose "Skipping ${gdb_test_file_name}."
+    return
+}
+
+standard_testfile .s
+if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile}]} {
+    return -1
+}
+
+if {![runto_main]} {
+    untested "could not run to main"
+    return -1
+}
+
+# Number of expected SIGTRAP's to get.  This needs to be kept in sync
+# with the source file.
+set expected_traps 5
+set keep_going 1
+set count 0
+
+# Make sure we have a lower timeout in case GDB doesn't support a particular
+# instruction.  Such instruction will cause GDB to loop infinitely.
+while {$keep_going} {
+    # Continue to next program breakpoint instruction.
+    gdb_test_multiple "continue" "trap instruction $count causes SIGTRAP" {
+	-re "Program received signal SIGTRAP, Trace/breakpoint trap.*$gdb_prompt $" {
+	    pass $gdb_test_name
+
+	    # Advance PC to nex instruction
+	    gdb_test "set \$pc = \$pc + 4" "" "advance past trap instruction $count"
+
+	 #    # Insert a breakpoint at the program breakpoint instruction so
+	 #    # GDB can step over it.
+	 #    gdb_test "break" \
+		# "Breakpoint $decimal at $hex: file .*$srcfile, line $decimal.*" \
+		# "insert breakpoint at trap instruction $count"
+	    incr count
+	}
+	# We've reached the end of the test.
+	-re "exited with code 01.*$gdb_prompt $" {
+	    set keep_going 0
+	}
+	timeout {
+	    fail $gdb_test_name
+	    set keep_going 0
+	}
+    }
+}
+
+# Verify we stopped at the expected number of SIGTRAP's.
+gdb_assert {$count == $expected_traps} "all trap instructions triggered"
diff --git a/gdb/testsuite/gdb.arch/ppc64-trap.s b/gdb/testsuite/gdb.arch/ppc64-trap.s
new file mode 100644
index 00000000000..b307018f3ec
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/ppc64-trap.s
@@ -0,0 +1,32 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   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/>. */
+
+/* To test if GDB stops at various trap instructions inserted into
+   the code.  */
+
+.global main
+.type main,function
+main:
+  ori 0, 0, 0
+  trap
+  tw  12, 2, 2
+  twi 31, 3, 3
+  td  12, 2, 2
+  tdi 31, 3, 3
+  ori 0, 0, 0
+  blr
+
-- 
2.30.2

Re: [PATCH 2/2] ppc: recognize all program traps

[Lancelot SIX]

Hi,

I do not know PPC so I will not comment on what actually done, but I
have minor remarks noted below.

On Tue, Nov 23, 2021 at 03:42:37PM +0000, Jan Vrany via Gdb-patches wrote:
> Permanent program breakpoints (ones inserted into the code) other than
> the one GDB uses for POWER (0x7fe00008) did not result in stop but
> caused GDB to loop infinitely.
> 
> This was because GDB did not recognize trap instructions other than
> "trap". For example, "tw 12, 4, 4" was not be recognized, causing GDB
> to loop forever.
> 
> This commit fixes this by providing POWER specific hook
> (gdbarch_program_breakpoint_here_p) recognizing all tw, twi, td and tdi
> instructions.
> 
> Tested on Linux on PowerPC e500 and on QEMU PPC64le.
> ---
>  gdb/rs6000-tdep.c                       | 66 +++++++++++++++++++++++
>  gdb/testsuite/gdb.arch/powerpc-trap.exp | 72 +++++++++++++++++++++++++
>  gdb/testsuite/gdb.arch/powerpc-trap.s   | 30 +++++++++++
>  gdb/testsuite/gdb.arch/ppc64-trap.exp   | 72 +++++++++++++++++++++++++
>  gdb/testsuite/gdb.arch/ppc64-trap.s     | 32 +++++++++++
>  5 files changed, 272 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.arch/powerpc-trap.exp
>  create mode 100644 gdb/testsuite/gdb.arch/powerpc-trap.s
>  create mode 100644 gdb/testsuite/gdb.arch/ppc64-trap.exp
>  create mode 100644 gdb/testsuite/gdb.arch/ppc64-trap.s
> 
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index 43880fa4426..eeafca1ff99 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -6247,6 +6247,70 @@ ppc_process_record (struct gdbarch *gdbarch, struct regcache *regcache,
>    return 0;
>  }
>  
> +/* Used for matching tw, twi, td and tdi instructions for POWER.  */
> +
> +static constexpr uint32_t TX_INSN_MASK = 0xFC0007FF;
> +static constexpr uint32_t TW_INSN = 0x7C000008;
> +static constexpr uint32_t TD_INSN = 0x7C000088;
> +
> +static constexpr uint32_t TXI_INSN_MASK = 0xFC000000;
> +static constexpr uint32_t TWI_INSN = 0x0C000000;
> +static constexpr uint32_t TDI_INSN = 0x08000000;
> +
> +static inline bool
> +is_tw_insn (uint32_t insn)
> +{
> +  return (insn & TX_INSN_MASK) == TW_INSN;
> +}
> +
> +static inline bool
> +is_twi_insn (uint32_t insn)
> +{
> +  return (insn & TXI_INSN_MASK) == TWI_INSN;
> +}
> +
> +static inline bool
> +is_td_insn (uint32_t insn)
> +{
> +  return (insn & TX_INSN_MASK) == TD_INSN;
> +}
> +
> +static inline bool
> +is_tdi_insn (uint32_t insn)
> +{
> +  return (insn & TXI_INSN_MASK) == TDI_INSN;
> +}
> +
> +/* Implementation of gdbarch_program_breakpoint_here_p for POWER.  */
> +
> +static bool
> +rs6000_program_breakpoint_here_p (gdbarch *gdbarch, CORE_ADDR address)
> +{
> +  const uint32_t insn_len = 4;
> +  gdb_byte target_mem[4];

You could probably use 'gdb_byte target_mem[insn_len]' here.

> +
> +  /* Enable the automatic memory restoration from breakpoints while
> +     we read the memory.  Otherwise we may find temporary breakpoints, ones
> +     inserted by GDB, and flag them as permanent breakpoints.  */
> +  scoped_restore restore_memory
> +      = make_scoped_restore_show_memory_breakpoints (0);
> +
> +  if (target_read_memory (address, target_mem, insn_len) == 0)
> +    {
> +      uint32_t insn = (uint32_t)extract_unsigned_integer (
> +          target_mem, insn_len, gdbarch_byte_order_for_code (gdbarch));
> +
> +      /* Check if INSN is a TW, TWI, TD or TDI instruction.  There
> +         are multiple choices of such instructions with different registers
> +         and / or immediate values but they all cause a break. */
> +      if (is_tw_insn (insn) || is_twi_insn (insn) || is_td_insn (insn)
> +          || is_tdi_insn (insn))
> +        return true;
> +    }
> +
> +  return false;
> +}
> +
>  /* Initialize the current architecture based on INFO.  If possible, re-use an
>     architecture from ARCHES, which is a list of architectures already created
>     during this debugging session.
> @@ -7109,6 +7173,8 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>  				       rs6000_breakpoint::kind_from_pc);
>    set_gdbarch_sw_breakpoint_from_kind (gdbarch,
>  				       rs6000_breakpoint::bp_from_kind);
> +  set_gdbarch_program_breakpoint_here_p (gdbarch,
> +                                         rs6000_program_breakpoint_here_p);
>  
>    /* The value of symbols of type N_SO and N_FUN maybe null when
>       it shouldn't be.  */
> diff --git a/gdb/testsuite/gdb.arch/powerpc-trap.exp b/gdb/testsuite/gdb.arch/powerpc-trap.exp
> new file mode 100644
> index 00000000000..4e86bd4b9a1
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/powerpc-trap.exp
> @@ -0,0 +1,72 @@
> +# Copyright 2020-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/>.
> +#
> +# This file is part of the gdb testsuite.
> +
> +# Test if GDB stops at various trap instructions inserted into
> +# the code.
> +
> +if { ![istarget powerpc-*] } {
> +    verbose "Skipping ${gdb_test_file_name}."
> +    return
> +}
> +
> +standard_testfile .s
> +if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile}]} {
> +    return -1
> +}
> +
> +if {![runto_main]} {
> +    untested "could not run to main"
> +    return -1
> +}
> +
> +# Number of expected SIGTRAP's to get.  This needs to be kept in sync
> +# with the source file.
> +set expected_traps 3
> +set keep_going 1
> +set count 0
> +
> +# Make sure we have a lower timeout in case GDB doesn't support a particular
> +# instruction.  Such instruction will cause GDB to loop infinitely.
> +while {$keep_going} {
> +    # Continue to next program breakpoint instruction.
> +    gdb_test_multiple "continue" "trap instruction $count causes SIGTRAP" {
> +	-re "Program received signal SIGTRAP, Trace/breakpoint trap.*$gdb_prompt $" {
> +	    pass $gdb_test_name
> +
> +	    # Advance PC to nex instruction
> +	    gdb_test "set \$pc = \$pc + 4" "" "advance past trap instruction $count"
> +
> +	 #    # Insert a breakpoint at the program breakpoint instruction so
> +	 #    # GDB can step over it.
> +	 #    gdb_test "break" \
> +		# "Breakpoint $decimal at $hex: file .*$srcfile, line $decimal.*" \
> +		# "insert breakpoint at trap instruction $count"

The part above is commented.  Is this something you forgot to remove
before creating your patch?

> +	    incr count
> +	}
> +	# We've reached the end of the test.
> +	-re "exited with code 01.*$gdb_prompt $" {
> +	    set keep_going 0
> +	}
> +	timeout {
> +	    fail $gdb_test_name
> +	    set keep_going 0
> +	}
> +    }
> +}
> +
> +# Verify we stopped at the expected number of SIGTRAP's.
> +gdb_assert {$count == $expected_traps} "all trap instructions triggered"
> diff --git a/gdb/testsuite/gdb.arch/powerpc-trap.s b/gdb/testsuite/gdb.arch/powerpc-trap.s
> new file mode 100644
> index 00000000000..c503cb1e33e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/powerpc-trap.s
> @@ -0,0 +1,30 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   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/>. */
> +
> +/* To test if GDB stops at various trap instructions inserted into
> +   the code.  */
> +
> +.global main
> +.type main,function
> +main:
> +  ori 0, 0, 0
> +  trap
> +  tw  12, 2, 2
> +  twi 31, 3, 3
> +  ori 0, 0, 0
> +  blr
> +
> diff --git a/gdb/testsuite/gdb.arch/ppc64-trap.exp b/gdb/testsuite/gdb.arch/ppc64-trap.exp
> new file mode 100644
> index 00000000000..1341808b9fd
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/ppc64-trap.exp
> @@ -0,0 +1,72 @@
> +# Copyright 2020-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/>.
> +#
> +# This file is part of the gdb testsuite.
> +
> +# Test if GDB stops at various trap instructions inserted into
> +# the code.
> +
> +if { ![istarget powerpc64-*] } {
> +    verbose "Skipping ${gdb_test_file_name}."
> +    return
> +}
> +
> +standard_testfile .s
> +if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile}]} {
> +    return -1
> +}
> +
> +if {![runto_main]} {
> +    untested "could not run to main"
> +    return -1
> +}
> +
> +# Number of expected SIGTRAP's to get.  This needs to be kept in sync
> +# with the source file.
> +set expected_traps 5
> +set keep_going 1
> +set count 0
> +
> +# Make sure we have a lower timeout in case GDB doesn't support a particular
> +# instruction.  Such instruction will cause GDB to loop infinitely.
> +while {$keep_going} {
> +    # Continue to next program breakpoint instruction.
> +    gdb_test_multiple "continue" "trap instruction $count causes SIGTRAP" {
> +	-re "Program received signal SIGTRAP, Trace/breakpoint trap.*$gdb_prompt $" {
> +	    pass $gdb_test_name
> +
> +	    # Advance PC to nex instruction
> +	    gdb_test "set \$pc = \$pc + 4" "" "advance past trap instruction $count"
> +
> +	 #    # Insert a breakpoint at the program breakpoint instruction so
> +	 #    # GDB can step over it.
> +	 #    gdb_test "break" \
> +		# "Breakpoint $decimal at $hex: file .*$srcfile, line $decimal.*" \
> +		# "insert breakpoint at trap instruction $count"

Similar remark here, did you forget to remove this?

Best,
Lancelot.

> +	    incr count
> +	}
> +	# We've reached the end of the test.
> +	-re "exited with code 01.*$gdb_prompt $" {
> +	    set keep_going 0
> +	}
> +	timeout {
> +	    fail $gdb_test_name
> +	    set keep_going 0
> +	}
> +    }
> +}
> +
> +# Verify we stopped at the expected number of SIGTRAP's.
> +gdb_assert {$count == $expected_traps} "all trap instructions triggered"
> diff --git a/gdb/testsuite/gdb.arch/ppc64-trap.s b/gdb/testsuite/gdb.arch/ppc64-trap.s
> new file mode 100644
> index 00000000000..b307018f3ec
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/ppc64-trap.s
> @@ -0,0 +1,32 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   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/>. */
> +
> +/* To test if GDB stops at various trap instructions inserted into
> +   the code.  */
> +
> +.global main
> +.type main,function
> +main:
> +  ori 0, 0, 0
> +  trap
> +  tw  12, 2, 2
> +  twi 31, 3, 3
> +  td  12, 2, 2
> +  tdi 31, 3, 3
> +  ori 0, 0, 0
> +  blr
> +
> -- 
> 2.30.2
> 

Re: [PATCH 2/2] ppc: recognize all program traps

[Lancelot SIX]

> > +
> > +/* Implementation of gdbarch_program_breakpoint_here_p for POWER.  */
> > +
> > +static bool
> > +rs6000_program_breakpoint_here_p (gdbarch *gdbarch, CORE_ADDR address)
> > +{
> > +  const uint32_t insn_len = 4;
> > +  gdb_byte target_mem[4];
> 
> You could probably use 'gdb_byte target_mem[insn_len]' here.

Hi again,

Using insn_len here makes most sense if insn_len is also declared
constexpr.  So this could become:

    constexpr uint32_t insn_len = 4;
    gdb_byte target_mem[insn_len];

What do you think?

Best Lancelot.

[PATCH v2 0/2] ppc: recognize all program traps

[Jan Vrany]

This new mini-series addresses Lancelot's comments.

Changes since v1:

* Use already #defined PPC_INSN_SIZE in rs6000_program_breakpoint_here_p().
  I decided to use PPC_INSN_SIZE rather than (local) constexpr as suggested
  to make it consistent with the rest of the code (which uses PPC_INSN_SIZE
  a lot).

* Remove leftover comments from testcases.

Jan Vrany (2):
  ppc: use 'trap' ('tw, 31, 0, 0', 0x7fe00008) as breakpoint instruction
  ppc: recognize all program traps

 gdb/rs6000-tdep.c                       | 69 ++++++++++++++++++++++++-
 gdb/testsuite/gdb.arch/powerpc-trap.exp | 67 ++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/powerpc-trap.s   | 30 +++++++++++
 gdb/testsuite/gdb.arch/ppc64-trap.exp   | 67 ++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/ppc64-trap.s     | 32 ++++++++++++
 5 files changed, 263 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.arch/powerpc-trap.exp
 create mode 100644 gdb/testsuite/gdb.arch/powerpc-trap.s
 create mode 100644 gdb/testsuite/gdb.arch/ppc64-trap.exp
 create mode 100644 gdb/testsuite/gdb.arch/ppc64-trap.s

-- 
2.30.2

[PATCH v2 1/2] ppc: use 'trap' ('tw, 31, 0, 0', 0x7fe00008) as breakpoint instruction

[Jan Vrany]

GDB used to use "tw 12, r2, r2" as a breakpoint instruction. While it
works, the PowerPC specifies 'tw, 31, 0, 0' (0x7fe00008) as the
canonical unconditional trap.
---
 gdb/rs6000-tdep.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 87a494e0bb8..43880fa4426 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -824,8 +824,8 @@ rs6000_fetch_pointer_argument (struct frame_info *frame, int argi,
 
 /* Sequence of bytes for breakpoint instruction.  */
 
-constexpr gdb_byte big_breakpoint[] = { 0x7d, 0x82, 0x10, 0x08 };
-constexpr gdb_byte little_breakpoint[] = { 0x08, 0x10, 0x82, 0x7d };
+constexpr gdb_byte big_breakpoint[] = { 0x7f, 0xe0, 0x00, 0x08 };
+constexpr gdb_byte little_breakpoint[] = { 0x08, 0x00, 0xe0, 0x7f };
 
 typedef BP_MANIPULATION_ENDIAN (little_breakpoint, big_breakpoint)
   rs6000_breakpoint;
-- 
2.30.2

[PATCH v2 2/2] ppc: recognize all program traps

[Jan Vrany]

Permanent program breakpoints (ones inserted into the code) other than
the one GDB uses for POWER (0x7fe00008) did not result in stop but
caused GDB to loop infinitely.

This was because GDB did not recognize trap instructions other than
"trap". For example, "tw 12, 4, 4" was not be recognized, causing GDB
to loop forever.

This commit fixes this by providing POWER specific hook
(gdbarch_program_breakpoint_here_p) recognizing all tw, twi, td and tdi
instructions.

Tested on Linux on PowerPC e500 and on QEMU PPC64le.
---
 gdb/rs6000-tdep.c                       | 65 ++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/powerpc-trap.exp | 67 +++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/powerpc-trap.s   | 30 +++++++++++
 gdb/testsuite/gdb.arch/ppc64-trap.exp   | 67 +++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/ppc64-trap.s     | 32 ++++++++++++
 5 files changed, 261 insertions(+)
 create mode 100644 gdb/testsuite/gdb.arch/powerpc-trap.exp
 create mode 100644 gdb/testsuite/gdb.arch/powerpc-trap.s
 create mode 100644 gdb/testsuite/gdb.arch/ppc64-trap.exp
 create mode 100644 gdb/testsuite/gdb.arch/ppc64-trap.s

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 43880fa4426..ed6838b9e16 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -6247,6 +6247,69 @@ ppc_process_record (struct gdbarch *gdbarch, struct regcache *regcache,
   return 0;
 }
 
+/* Used for matching tw, twi, td and tdi instructions for POWER.  */
+
+static constexpr uint32_t TX_INSN_MASK = 0xFC0007FF;
+static constexpr uint32_t TW_INSN = 0x7C000008;
+static constexpr uint32_t TD_INSN = 0x7C000088;
+
+static constexpr uint32_t TXI_INSN_MASK = 0xFC000000;
+static constexpr uint32_t TWI_INSN = 0x0C000000;
+static constexpr uint32_t TDI_INSN = 0x08000000;
+
+static inline bool
+is_tw_insn (uint32_t insn)
+{
+  return (insn & TX_INSN_MASK) == TW_INSN;
+}
+
+static inline bool
+is_twi_insn (uint32_t insn)
+{
+  return (insn & TXI_INSN_MASK) == TWI_INSN;
+}
+
+static inline bool
+is_td_insn (uint32_t insn)
+{
+  return (insn & TX_INSN_MASK) == TD_INSN;
+}
+
+static inline bool
+is_tdi_insn (uint32_t insn)
+{
+  return (insn & TXI_INSN_MASK) == TDI_INSN;
+}
+
+/* Implementation of gdbarch_program_breakpoint_here_p for POWER.  */
+
+static bool
+rs6000_program_breakpoint_here_p (gdbarch *gdbarch, CORE_ADDR address)
+{
+  gdb_byte target_mem[PPC_INSN_SIZE];
+
+  /* Enable the automatic memory restoration from breakpoints while
+     we read the memory.  Otherwise we may find temporary breakpoints, ones
+     inserted by GDB, and flag them as permanent breakpoints.  */
+  scoped_restore restore_memory
+      = make_scoped_restore_show_memory_breakpoints (0);
+
+  if (target_read_memory (address, target_mem, PPC_INSN_SIZE) == 0)
+    {
+      uint32_t insn = (uint32_t)extract_unsigned_integer (
+          target_mem, PPC_INSN_SIZE, gdbarch_byte_order_for_code (gdbarch));
+
+      /* Check if INSN is a TW, TWI, TD or TDI instruction.  There
+         are multiple choices of such instructions with different registers
+         and / or immediate values but they all cause a break. */
+      if (is_tw_insn (insn) || is_twi_insn (insn) || is_td_insn (insn)
+          || is_tdi_insn (insn))
+        return true;
+    }
+
+  return false;
+}
+
 /* Initialize the current architecture based on INFO.  If possible, re-use an
    architecture from ARCHES, which is a list of architectures already created
    during this debugging session.
@@ -7109,6 +7172,8 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 				       rs6000_breakpoint::kind_from_pc);
   set_gdbarch_sw_breakpoint_from_kind (gdbarch,
 				       rs6000_breakpoint::bp_from_kind);
+  set_gdbarch_program_breakpoint_here_p (gdbarch,
+                                         rs6000_program_breakpoint_here_p);
 
   /* The value of symbols of type N_SO and N_FUN maybe null when
      it shouldn't be.  */
diff --git a/gdb/testsuite/gdb.arch/powerpc-trap.exp b/gdb/testsuite/gdb.arch/powerpc-trap.exp
new file mode 100644
index 00000000000..86f8e61b89b
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc-trap.exp
@@ -0,0 +1,67 @@
+# Copyright 2020-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/>.
+#
+# This file is part of the gdb testsuite.
+
+# Test if GDB stops at various trap instructions inserted into
+# the code.
+
+if { ![istarget powerpc-*] } {
+    verbose "Skipping ${gdb_test_file_name}."
+    return
+}
+
+standard_testfile .s
+if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile}]} {
+    return -1
+}
+
+if {![runto_main]} {
+    untested "could not run to main"
+    return -1
+}
+
+# Number of expected SIGTRAP's to get.  This needs to be kept in sync
+# with the source file.
+set expected_traps 3
+set keep_going 1
+set count 0
+
+# Make sure we have a lower timeout in case GDB doesn't support a particular
+# instruction.  Such instruction will cause GDB to loop infinitely.
+while {$keep_going} {
+    # Continue to next program breakpoint instruction.
+    gdb_test_multiple "continue" "trap instruction $count causes SIGTRAP" {
+	-re "Program received signal SIGTRAP, Trace/breakpoint trap.*$gdb_prompt $" {
+	    pass $gdb_test_name
+
+	    # Advance PC to next instruction
+	    gdb_test "set \$pc = \$pc + 4" "" "advance past trap instruction $count"
+
+	    incr count
+	}
+	# We've reached the end of the test.
+	-re "exited with code 01.*$gdb_prompt $" {
+	    set keep_going 0
+	}
+	timeout {
+	    fail $gdb_test_name
+	    set keep_going 0
+	}
+    }
+}
+
+# Verify we stopped at the expected number of SIGTRAP's.
+gdb_assert {$count == $expected_traps} "all trap instructions triggered"
diff --git a/gdb/testsuite/gdb.arch/powerpc-trap.s b/gdb/testsuite/gdb.arch/powerpc-trap.s
new file mode 100644
index 00000000000..c503cb1e33e
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc-trap.s
@@ -0,0 +1,30 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   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/>. */
+
+/* To test if GDB stops at various trap instructions inserted into
+   the code.  */
+
+.global main
+.type main,function
+main:
+  ori 0, 0, 0
+  trap
+  tw  12, 2, 2
+  twi 31, 3, 3
+  ori 0, 0, 0
+  blr
+
diff --git a/gdb/testsuite/gdb.arch/ppc64-trap.exp b/gdb/testsuite/gdb.arch/ppc64-trap.exp
new file mode 100644
index 00000000000..ffd640c681d
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/ppc64-trap.exp
@@ -0,0 +1,67 @@
+# Copyright 2020-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/>.
+#
+# This file is part of the gdb testsuite.
+
+# Test if GDB stops at various trap instructions inserted into
+# the code.
+
+if { ![istarget powerpc64-*] } {
+    verbose "Skipping ${gdb_test_file_name}."
+    return
+}
+
+standard_testfile .s
+if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile}]} {
+    return -1
+}
+
+if {![runto_main]} {
+    untested "could not run to main"
+    return -1
+}
+
+# Number of expected SIGTRAP's to get.  This needs to be kept in sync
+# with the source file.
+set expected_traps 5
+set keep_going 1
+set count 0
+
+# Make sure we have a lower timeout in case GDB doesn't support a particular
+# instruction.  Such instruction will cause GDB to loop infinitely.
+while {$keep_going} {
+    # Continue to next program breakpoint instruction.
+    gdb_test_multiple "continue" "trap instruction $count causes SIGTRAP" {
+	-re "Program received signal SIGTRAP, Trace/breakpoint trap.*$gdb_prompt $" {
+	    pass $gdb_test_name
+
+	    # Advance PC to next instruction
+	    gdb_test "set \$pc = \$pc + 4" "" "advance past trap instruction $count"
+
+	    incr count
+	}
+	# We've reached the end of the test.
+	-re "exited with code 01.*$gdb_prompt $" {
+	    set keep_going 0
+	}
+	timeout {
+	    fail $gdb_test_name
+	    set keep_going 0
+	}
+    }
+}
+
+# Verify we stopped at the expected number of SIGTRAP's.
+gdb_assert {$count == $expected_traps} "all trap instructions triggered"
diff --git a/gdb/testsuite/gdb.arch/ppc64-trap.s b/gdb/testsuite/gdb.arch/ppc64-trap.s
new file mode 100644
index 00000000000..b307018f3ec
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/ppc64-trap.s
@@ -0,0 +1,32 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   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/>. */
+
+/* To test if GDB stops at various trap instructions inserted into
+   the code.  */
+
+.global main
+.type main,function
+main:
+  ori 0, 0, 0
+  trap
+  tw  12, 2, 2
+  twi 31, 3, 3
+  td  12, 2, 2
+  tdi 31, 3, 3
+  ori 0, 0, 0
+  blr
+
-- 
2.30.2

Re: [PATCH v2 1/2] ppc: use 'trap' ('tw, 31, 0, 0', 0x7fe00008) as breakpoint instruction

[Joel Brobecker]

Hi Jan,

On Wed, Nov 24, 2021 at 01:09:25PM +0000, Jan Vrany via Gdb-patches wrote:
> GDB used to use "tw 12, r2, r2" as a breakpoint instruction. While it
> works, the PowerPC specifies 'tw, 31, 0, 0' (0x7fe00008) as the
> canonical unconditional trap.
> ---
>  gdb/rs6000-tdep.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index 87a494e0bb8..43880fa4426 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -824,8 +824,8 @@ rs6000_fetch_pointer_argument (struct frame_info *frame, int argi,
>  
>  /* Sequence of bytes for breakpoint instruction.  */
>  
> -constexpr gdb_byte big_breakpoint[] = { 0x7d, 0x82, 0x10, 0x08 };
> -constexpr gdb_byte little_breakpoint[] = { 0x08, 0x10, 0x82, 0x7d };
> +constexpr gdb_byte big_breakpoint[] = { 0x7f, 0xe0, 0x00, 0x08 };
> +constexpr gdb_byte little_breakpoint[] = { 0x08, 0x00, 0xe0, 0x7f };
>  
>  typedef BP_MANIPULATION_ENDIAN (little_breakpoint, big_breakpoint)
>    rs6000_breakpoint;

I'm going to say OK, but I'd like to know what your source is
for this, so we can document that change. Part of me wonders
why we're making a change like this one on some code which
works, which is always a risk, no matter how small, unless we have
enough evidence that this is going to be helpful in practice.

Thank you,
-- 
Joel

Re: [PATCH v2 2/2] ppc: recognize all program traps

[Joel Brobecker]

On Wed, Nov 24, 2021 at 01:09:26PM +0000, Jan Vrany via Gdb-patches wrote:
> Permanent program breakpoints (ones inserted into the code) other than
> the one GDB uses for POWER (0x7fe00008) did not result in stop but
> caused GDB to loop infinitely.
> 
> This was because GDB did not recognize trap instructions other than
> "trap". For example, "tw 12, 4, 4" was not be recognized, causing GDB
> to loop forever.
> 
> This commit fixes this by providing POWER specific hook
> (gdbarch_program_breakpoint_here_p) recognizing all tw, twi, td and tdi
> instructions.
> 
> Tested on Linux on PowerPC e500 and on QEMU PPC64le.
> ---
>  gdb/rs6000-tdep.c                       | 65 ++++++++++++++++++++++++
>  gdb/testsuite/gdb.arch/powerpc-trap.exp | 67 +++++++++++++++++++++++++
>  gdb/testsuite/gdb.arch/powerpc-trap.s   | 30 +++++++++++
>  gdb/testsuite/gdb.arch/ppc64-trap.exp   | 67 +++++++++++++++++++++++++
>  gdb/testsuite/gdb.arch/ppc64-trap.s     | 32 ++++++++++++
>  5 files changed, 261 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.arch/powerpc-trap.exp
>  create mode 100644 gdb/testsuite/gdb.arch/powerpc-trap.s
>  create mode 100644 gdb/testsuite/gdb.arch/ppc64-trap.exp
>  create mode 100644 gdb/testsuite/gdb.arch/ppc64-trap.s
> 
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index 43880fa4426..ed6838b9e16 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -6247,6 +6247,69 @@ ppc_process_record (struct gdbarch *gdbarch, struct regcache *regcache,
>    return 0;
>  }
>  
> +/* Used for matching tw, twi, td and tdi instructions for POWER.  */
> +
> +static constexpr uint32_t TX_INSN_MASK = 0xFC0007FF;
> +static constexpr uint32_t TW_INSN = 0x7C000008;
> +static constexpr uint32_t TD_INSN = 0x7C000088;
> +
> +static constexpr uint32_t TXI_INSN_MASK = 0xFC000000;
> +static constexpr uint32_t TWI_INSN = 0x0C000000;
> +static constexpr uint32_t TDI_INSN = 0x08000000;
> +
> +static inline bool
> +is_tw_insn (uint32_t insn)
> +{
> +  return (insn & TX_INSN_MASK) == TW_INSN;
> +}
> +
> +static inline bool
> +is_twi_insn (uint32_t insn)
> +{
> +  return (insn & TXI_INSN_MASK) == TWI_INSN;
> +}
> +
> +static inline bool
> +is_td_insn (uint32_t insn)
> +{
> +  return (insn & TX_INSN_MASK) == TD_INSN;
> +}
> +
> +static inline bool
> +is_tdi_insn (uint32_t insn)
> +{
> +  return (insn & TXI_INSN_MASK) == TDI_INSN;
> +}
> +
> +/* Implementation of gdbarch_program_breakpoint_here_p for POWER.  */
> +
> +static bool
> +rs6000_program_breakpoint_here_p (gdbarch *gdbarch, CORE_ADDR address)
> +{
> +  gdb_byte target_mem[PPC_INSN_SIZE];
> +
> +  /* Enable the automatic memory restoration from breakpoints while
> +     we read the memory.  Otherwise we may find temporary breakpoints, ones
> +     inserted by GDB, and flag them as permanent breakpoints.  */
> +  scoped_restore restore_memory
> +      = make_scoped_restore_show_memory_breakpoints (0);

Small coding standard violation: Indentation in this case should
remain 2 characters.

> +
> +  if (target_read_memory (address, target_mem, PPC_INSN_SIZE) == 0)
> +    {
> +      uint32_t insn = (uint32_t)extract_unsigned_integer (
> +          target_mem, PPC_INSN_SIZE, gdbarch_byte_order_for_code (gdbarch));

Small coding standard violations:
  - You need a space after the "(uint32_t)" cast
  - For the opening parenthesis in the call to extract_unsigned_integer,
    it should go on the next line, indented by 2 characters:

    uint32_t insn = (uint32_t) extract_unsigned_integer
      (target_mem, PPC_INSN_SIZE, gdbarch_byte_order_for_code (gdbarch));

Other than that, the patch looks good to me.

> +      /* Check if INSN is a TW, TWI, TD or TDI instruction.  There
> +         are multiple choices of such instructions with different registers
> +         and / or immediate values but they all cause a break. */
> +      if (is_tw_insn (insn) || is_twi_insn (insn) || is_td_insn (insn)
> +          || is_tdi_insn (insn))
> +        return true;
> +    }
> +
> +  return false;
> +}
> +
>  /* Initialize the current architecture based on INFO.  If possible, re-use an
>     architecture from ARCHES, which is a list of architectures already created
>     during this debugging session.
> @@ -7109,6 +7172,8 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>  				       rs6000_breakpoint::kind_from_pc);
>    set_gdbarch_sw_breakpoint_from_kind (gdbarch,
>  				       rs6000_breakpoint::bp_from_kind);
> +  set_gdbarch_program_breakpoint_here_p (gdbarch,
> +                                         rs6000_program_breakpoint_here_p);
>  
>    /* The value of symbols of type N_SO and N_FUN maybe null when
>       it shouldn't be.  */
> diff --git a/gdb/testsuite/gdb.arch/powerpc-trap.exp b/gdb/testsuite/gdb.arch/powerpc-trap.exp
> new file mode 100644
> index 00000000000..86f8e61b89b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/powerpc-trap.exp
> @@ -0,0 +1,67 @@
> +# Copyright 2020-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/>.
> +#
> +# This file is part of the gdb testsuite.
> +
> +# Test if GDB stops at various trap instructions inserted into
> +# the code.
> +
> +if { ![istarget powerpc-*] } {
> +    verbose "Skipping ${gdb_test_file_name}."
> +    return
> +}
> +
> +standard_testfile .s
> +if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile}]} {
> +    return -1
> +}
> +
> +if {![runto_main]} {
> +    untested "could not run to main"
> +    return -1
> +}
> +
> +# Number of expected SIGTRAP's to get.  This needs to be kept in sync
> +# with the source file.
> +set expected_traps 3
> +set keep_going 1
> +set count 0
> +
> +# Make sure we have a lower timeout in case GDB doesn't support a particular
> +# instruction.  Such instruction will cause GDB to loop infinitely.
> +while {$keep_going} {
> +    # Continue to next program breakpoint instruction.
> +    gdb_test_multiple "continue" "trap instruction $count causes SIGTRAP" {
> +	-re "Program received signal SIGTRAP, Trace/breakpoint trap.*$gdb_prompt $" {
> +	    pass $gdb_test_name
> +
> +	    # Advance PC to next instruction
> +	    gdb_test "set \$pc = \$pc + 4" "" "advance past trap instruction $count"
> +
> +	    incr count
> +	}
> +	# We've reached the end of the test.
> +	-re "exited with code 01.*$gdb_prompt $" {
> +	    set keep_going 0
> +	}
> +	timeout {
> +	    fail $gdb_test_name
> +	    set keep_going 0
> +	}
> +    }
> +}
> +
> +# Verify we stopped at the expected number of SIGTRAP's.
> +gdb_assert {$count == $expected_traps} "all trap instructions triggered"
> diff --git a/gdb/testsuite/gdb.arch/powerpc-trap.s b/gdb/testsuite/gdb.arch/powerpc-trap.s
> new file mode 100644
> index 00000000000..c503cb1e33e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/powerpc-trap.s
> @@ -0,0 +1,30 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   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/>. */
> +
> +/* To test if GDB stops at various trap instructions inserted into
> +   the code.  */
> +
> +.global main
> +.type main,function
> +main:
> +  ori 0, 0, 0
> +  trap
> +  tw  12, 2, 2
> +  twi 31, 3, 3
> +  ori 0, 0, 0
> +  blr
> +
> diff --git a/gdb/testsuite/gdb.arch/ppc64-trap.exp b/gdb/testsuite/gdb.arch/ppc64-trap.exp
> new file mode 100644
> index 00000000000..ffd640c681d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/ppc64-trap.exp
> @@ -0,0 +1,67 @@
> +# Copyright 2020-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/>.
> +#
> +# This file is part of the gdb testsuite.
> +
> +# Test if GDB stops at various trap instructions inserted into
> +# the code.
> +
> +if { ![istarget powerpc64-*] } {
> +    verbose "Skipping ${gdb_test_file_name}."
> +    return
> +}
> +
> +standard_testfile .s
> +if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile}]} {
> +    return -1
> +}
> +
> +if {![runto_main]} {
> +    untested "could not run to main"
> +    return -1
> +}
> +
> +# Number of expected SIGTRAP's to get.  This needs to be kept in sync
> +# with the source file.
> +set expected_traps 5
> +set keep_going 1
> +set count 0
> +
> +# Make sure we have a lower timeout in case GDB doesn't support a particular
> +# instruction.  Such instruction will cause GDB to loop infinitely.
> +while {$keep_going} {
> +    # Continue to next program breakpoint instruction.
> +    gdb_test_multiple "continue" "trap instruction $count causes SIGTRAP" {
> +	-re "Program received signal SIGTRAP, Trace/breakpoint trap.*$gdb_prompt $" {
> +	    pass $gdb_test_name
> +
> +	    # Advance PC to next instruction
> +	    gdb_test "set \$pc = \$pc + 4" "" "advance past trap instruction $count"
> +
> +	    incr count
> +	}
> +	# We've reached the end of the test.
> +	-re "exited with code 01.*$gdb_prompt $" {
> +	    set keep_going 0
> +	}
> +	timeout {
> +	    fail $gdb_test_name
> +	    set keep_going 0
> +	}
> +    }
> +}
> +
> +# Verify we stopped at the expected number of SIGTRAP's.
> +gdb_assert {$count == $expected_traps} "all trap instructions triggered"
> diff --git a/gdb/testsuite/gdb.arch/ppc64-trap.s b/gdb/testsuite/gdb.arch/ppc64-trap.s
> new file mode 100644
> index 00000000000..b307018f3ec
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/ppc64-trap.s
> @@ -0,0 +1,32 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   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/>. */
> +
> +/* To test if GDB stops at various trap instructions inserted into
> +   the code.  */
> +
> +.global main
> +.type main,function
> +main:
> +  ori 0, 0, 0
> +  trap
> +  tw  12, 2, 2
> +  twi 31, 3, 3
> +  td  12, 2, 2
> +  tdi 31, 3, 3
> +  ori 0, 0, 0
> +  blr
> +
> -- 
> 2.30.2
> 

-- 
Joel

Re: [PATCH v2 1/2] ppc: use 'trap' ('tw, 31, 0, 0', 0x7fe00008) as breakpoint instruction

[Jan Vrany]

Hi Joel

On Sat, 2021-11-27 at 10:45 +0400, Joel Brobecker wrote:
> Hi Jan,
> 
> On Wed, Nov 24, 2021 at 01:09:25PM +0000, Jan Vrany via Gdb-patches
> wrote:
> > GDB used to use "tw 12, r2, r2" as a breakpoint instruction. While
> > it
> > works, the PowerPC specifies 'tw, 31, 0, 0' (0x7fe00008) as the
> > canonical unconditional trap.
> > ---
> >  gdb/rs6000-tdep.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> > index 87a494e0bb8..43880fa4426 100644
> > --- a/gdb/rs6000-tdep.c
> > +++ b/gdb/rs6000-tdep.c
> > @@ -824,8 +824,8 @@ rs6000_fetch_pointer_argument (struct
> > frame_info *frame, int argi,
> >  
> >  /* Sequence of bytes for breakpoint instruction.  */
> >  
> > -constexpr gdb_byte big_breakpoint[] = { 0x7d, 0x82, 0x10, 0x08 };
> > -constexpr gdb_byte little_breakpoint[] = { 0x08, 0x10, 0x82, 0x7d
> > };
> > +constexpr gdb_byte big_breakpoint[] = { 0x7f, 0xe0, 0x00, 0x08 };
> > +constexpr gdb_byte little_breakpoint[] = { 0x08, 0x00, 0xe0, 0x7f
> > };
> >  
> >  typedef BP_MANIPULATION_ENDIAN (little_breakpoint, big_breakpoint)
> >    rs6000_breakpoint;
> 
> I'm going to say OK, but I'd like to know what your source is
> for this, so we can document that change. Part of me wonders
> why we're making a change like this one on some code which
> works, which is always a risk, no matter how small, unless we have
> enough evidence that this is going to be helpful in practice.

According to Power ISA 3.0 B spec [1], section 3.3.11 Fixed-Point Trap
Instructions (also section C.6 Trap Mnemonics), unconditional trap 
is encoded as `tw 31, 0, 0`. While debugging a problem on POWER
(gdb not stopping on a `trap` instruction) I found that GDB is using
`tdnl r2, r2` (trap if not less than) which works but it is different
than unconditional trap as specified. I could not find out why 
`tdnl` was used instead of unconditional `trap`. I thought it is 
more "natural" to use canonical unconditional trap. So this is merely
a "cosmetic" change. 

I agree that I should have included reference to the spec in the 
commit message and maybe also add comment to the code. Will do in 
next version (should we decide to keep this patch).

Now, I do understand the risk of changing something that works. I'm
happy to drop this patch since the next one in this miniseries 
fixes the problem we were experiencing properly (that is, GDB not
stopping on software-planted breakpoint instructions).

What do you think? 

Thanks! 

Jan

[1]:
https://openpowerfoundation.org/?resource_lib=power-isa-version-3-0

> 
> Thank you,

Re: [PATCH v2 2/2] ppc: recognize all program traps

[Pedro Alves]

On 2021-11-24 13:09, Jan Vrany via Gdb-patches wrote:
> +# Number of expected SIGTRAP's to get.  This needs to be kept in sync
> +# with the source file.
> +set expected_traps 3
> +set keep_going 1
> +set count 0
> +
> +# Make sure we have a lower timeout in case GDB doesn't support a particular
> +# instruction.  Such instruction will cause GDB to loop infinitely.
> +while {$keep_going} {
> +    # Continue to next program breakpoint instruction.
> +    gdb_test_multiple "continue" "trap instruction $count causes SIGTRAP" {
> +	-re "Program received signal SIGTRAP, Trace/breakpoint trap.*$gdb_prompt $" {
> +	    pass $gdb_test_name
> +
> +	    # Advance PC to next instruction
> +	    gdb_test "set \$pc = \$pc + 4" "" "advance past trap instruction $count"
> +
> +	    incr count
> +	}
> +	# We've reached the end of the test.
> +	-re "exited with code 01.*$gdb_prompt $" {
> +	    set keep_going 0
> +	}
> +	timeout {
> +	    fail $gdb_test_name
> +	    set keep_going 0
> +	}
> +    }
> +}
> +


This while loop will iterate forever if gdb_test_multiple trips on some internal match, like
some unexpected text out of gdb that ends with a prompt, or an internal error.  The logic of
"keep_going" should be reversed so that the loop breaks if anything goes wrong.   Also, the
last continue to "exited with code 01" doesn't itself issue a pass unlike the other continues,
which seems inconsistent.  BTW, if we flip the logic, we no longer need the timeout check.

Something like this (completely untested):

~~~~~~~~~~~~
# Number of expected SIGTRAP's to get.  This needs to be kept in sync
# with the source file.
set expected_traps 3
set count 0
set keep_going 1

# Make sure we have a lower timeout in case GDB doesn't support a particular
# instruction.  Such instruction will cause GDB to loop infinitely.
while {$keep_going} {

    set keep_going 0

    # Continue to next program breakpoint instruction.
    gdb_test_multiple "continue" "trap instruction $count causes SIGTRAP" {
	-re "Program received signal SIGTRAP, Trace/breakpoint trap.*$gdb_prompt $" {
	    pass $gdb_test_name

	    # Advance PC to next instruction
	    gdb_test "set \$pc = \$pc + 4" "" "advance past trap instruction $count"

	    incr count
	    if {$count < $expected_traps} {
	       set keep_going 1
	    }
	}
    }

# Verify we stopped at the expected number of SIGTRAP's.
gdb_assert {$count == $expected_traps} "all trap instructions triggered"

# One last continue to reach the end of the test, to make sure we don't get
# another SIGTRAP.
gdb_test "continue" "exited with code 01.*" "continue to end"
~~~~~~~~~~~~


Some thing in the other file.  Or you could merge the files, and make the exp file
set a different .S filename and different "expected_traps" depending on arch, to
avoid duplication.


BTW, I don't understand what the "Make sure we have a lower timeout" comment is referring
to -- I don't see anything changing the timeout.

BTW², seems strange to expect that the program exits with exit code 1 instead of 0 on a success run.
I can't write PPC assembly to save my life, but I don't see any "1", or writing to r3 in in
the assembly, so I guess that is assuming the value is already 1 on entry.  I wonder whether that's a
good assumption in all environments.

Re: [PATCH v2 1/2] ppc: use 'trap' ('tw, 31, 0, 0', 0x7fe00008) as breakpoint instruction

[Joel Brobecker]

Hi Jan,

> > >  typedef BP_MANIPULATION_ENDIAN (little_breakpoint, big_breakpoint)
> > >    rs6000_breakpoint;
> > 
> > I'm going to say OK, but I'd like to know what your source is
> > for this, so we can document that change. Part of me wonders
> > why we're making a change like this one on some code which
> > works, which is always a risk, no matter how small, unless we have
> > enough evidence that this is going to be helpful in practice.
> 
> According to Power ISA 3.0 B spec [1], section 3.3.11 Fixed-Point Trap
> Instructions (also section C.6 Trap Mnemonics), unconditional trap 
> is encoded as `tw 31, 0, 0`. While debugging a problem on POWER
> (gdb not stopping on a `trap` instruction) I found that GDB is using
> `tdnl r2, r2` (trap if not less than) which works but it is different
> than unconditional trap as specified. I could not find out why 
> `tdnl` was used instead of unconditional `trap`. I thought it is 
> more "natural" to use canonical unconditional trap. So this is merely
> a "cosmetic" change. 
> 
> I agree that I should have included reference to the spec in the 
> commit message and maybe also add comment to the code. Will do in 
> next version (should we decide to keep this patch).
> 
> Now, I do understand the risk of changing something that works. I'm
> happy to drop this patch since the next one in this miniseries 
> fixes the problem we were experiencing properly (that is, GDB not
> stopping on software-planted breakpoint instructions).
> 
> What do you think? 

With the addition of the reference in the commit message, I'd say
let's make the change. I don't think the risk is very high, and
I don't want an issue that's most likely non-existant to cause
enough FUD that we'd ignore a patch that looks correct.
Worse case scenario, we revert!

-- 
Joel

Re: [PATCH v2 2/2] ppc: recognize all program traps

[Jan Vrany]

On Tue, 2021-11-30 at 12:17 +0000, Pedro Alves wrote:
> On 2021-11-24 13:09, Jan Vrany via Gdb-patches wrote:
> > +# Number of expected SIGTRAP's to get.  This needs to be kept in
> > sync
> > +# with the source file.
> > +set expected_traps 3
> > +set keep_going 1
> > +set count 0
> > +
> > +# Make sure we have a lower timeout in case GDB doesn't support a
> > particular
> > +# instruction.  Such instruction will cause GDB to loop
> > infinitely.
> > +while {$keep_going} {
> > +    # Continue to next program breakpoint instruction.
> > +    gdb_test_multiple "continue" "trap instruction $count causes
> > SIGTRAP" {
> > +       -re "Program received signal SIGTRAP, Trace/breakpoint
> > trap.*$gdb_prompt $" {
> > +           pass $gdb_test_name
> > +
> > +           # Advance PC to next instruction
> > +           gdb_test "set \$pc = \$pc + 4" "" "advance past trap
> > instruction $count"
> > +
> > +           incr count
> > +       }
> > +       # We've reached the end of the test.
> > +       -re "exited with code 01.*$gdb_prompt $" {
> > +           set keep_going 0
> > +       }
> > +       timeout {
> > +           fail $gdb_test_name
> > +           set keep_going 0
> > +       }
> > +    }
> > +}
> > +
> 
> 
> This while loop will iterate forever if gdb_test_multiple trips on
> some internal match, like
> some unexpected text out of gdb that ends with a prompt, or an
> internal error.  The logic of
> "keep_going" should be reversed so that the loop breaks if anything
> goes wrong.   Also, the
> last continue to "exited with code 01" doesn't itself issue a pass
> unlike the other continues,
> which seems inconsistent.  BTW, if we flip the logic, we no longer
> need the timeout check.
> 
> Something like this (completely untested):
> 
> ~~~~~~~~~~~~
> # Number of expected SIGTRAP's to get.  This needs to be kept in sync
> # with the source file.
> set expected_traps 3
> set count 0
> set keep_going 1
> 
> # Make sure we have a lower timeout in case GDB doesn't support a
> particular
> # instruction.  Such instruction will cause GDB to loop infinitely.
> while {$keep_going} {
> 
>     set keep_going 0
> 
>     # Continue to next program breakpoint instruction.
>     gdb_test_multiple "continue" "trap instruction $count causes
> SIGTRAP" {
>         -re "Program received signal SIGTRAP, Trace/breakpoint
> trap.*$gdb_prompt $" {
>             pass $gdb_test_name
> 
>             # Advance PC to next instruction
>             gdb_test "set \$pc = \$pc + 4" "" "advance past trap
> instruction $count"
> 
>             incr count
>             if {$count < $expected_traps} {
>                set keep_going 1
>             }
>         }
>     }
> 
> # Verify we stopped at the expected number of SIGTRAP's.
> gdb_assert {$count == $expected_traps} "all trap instructions
> triggered"
> 
> # One last continue to reach the end of the test, to make sure we
> don't get
> # another SIGTRAP.
> gdb_test "continue" "exited with code 01.*" "continue to end"
> ~~~~~~~~~~~~
> 
> 
> Some thing in the other file.  Or you could merge the files, and make
> the exp file
> set a different .S filename and different "expected_traps" depending
> on arch, to
> avoid duplication.


Good point. I'll do as suggested in next version. 

> 
> 
> BTW, I don't understand what the "Make sure we have a lower timeout"
> comment is referring
> to -- I don't see anything changing the timeout.

Neither I do, to be honest. These tests are shameless hacked copies of
existing gdb.arch/aarch64-brk-patterns.exp test so this and the 
comment above also apply to it as well. 

I prefer not to change aarch64 test now since I do not have aarch64 
test environment set up yet.

> 
> BTW², seems strange to expect that the program exits with exit code 1
> instead of 0 on a success run.
> I can't write PPC assembly to save my life, but I don't see any "1",
> or writing to r3 in in
> the assembly, so I guess that is assuming the value is already 1 on
> entry.  I wonder whether that's a
> good assumption in all environments.

This works because main gets "int argc" as first parameter and test
runs program with no aguments so argc is always 1. 
AFAIK, in all calling conventions used on PPC first argument is passed
in r3 which is also a return register. 

But I agree this is little to hacky. I'll change it. 

Thanks! Jan

[PATCH v3 0/2] ppc: recognize all program traps

[Jan Vrany]

This new mini-series addresses Joel's and Pedro's comments.

Changes since v2:

  * add reference to PowerPC ISA spec in commit message
  * merged PowerPC trap tests to one file handling
    32bit and 64bit PowerPCs.
  * simplified test logic as suggested by Pedro.
  * explicitly set test program status code 0 in
    powerpc-trap.s and powerpc64-trap.s


Jan Vrany (2):
  ppc: use "trap" ("tw, 31, 0, 0") as breakpoint instruction
  ppc: recognize all program traps

 gdb/rs6000-tdep.c                       | 69 +++++++++++++++++++++++-
 gdb/testsuite/gdb.arch/powerpc-trap.exp | 72 +++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/powerpc-trap.s   | 31 +++++++++++
 gdb/testsuite/gdb.arch/powerpc64-trap.s | 33 ++++++++++++
 4 files changed, 203 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.arch/powerpc-trap.exp
 create mode 100644 gdb/testsuite/gdb.arch/powerpc-trap.s
 create mode 100644 gdb/testsuite/gdb.arch/powerpc64-trap.s

-- 
2.30.2

[PATCH v3 1/2] ppc: use "trap" ("tw, 31, 0, 0") as breakpoint instruction

[Jan Vrany]

Power ISA 3.0 B spec [1], sections 3.3.11 "Fixed-Point Trap Instructions"
and section C.6 "Trap Mnemonics" specify "tw, 31, 0, 0" (encoded as
0x7fe00008) as canonical unconditional trap instruction.

This commit changes the breakpoint instruction used by GDB from
"tw 12, r2, r2" to unconditional "trap".

[1]: https://openpowerfoundation.org/?resource_lib=power-isa-version-3-0
---
 gdb/rs6000-tdep.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 87a494e0bb8..43880fa4426 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -824,8 +824,8 @@ rs6000_fetch_pointer_argument (struct frame_info *frame, int argi,
 
 /* Sequence of bytes for breakpoint instruction.  */
 
-constexpr gdb_byte big_breakpoint[] = { 0x7d, 0x82, 0x10, 0x08 };
-constexpr gdb_byte little_breakpoint[] = { 0x08, 0x10, 0x82, 0x7d };
+constexpr gdb_byte big_breakpoint[] = { 0x7f, 0xe0, 0x00, 0x08 };
+constexpr gdb_byte little_breakpoint[] = { 0x08, 0x00, 0xe0, 0x7f };
 
 typedef BP_MANIPULATION_ENDIAN (little_breakpoint, big_breakpoint)
   rs6000_breakpoint;
-- 
2.30.2

[PATCH v3 2/2] ppc: recognize all program traps

[Jan Vrany]

Permanent program breakpoints (ones inserted into the code) other than
the one GDB uses for POWER (0x7fe00008) did not result in stop but
caused GDB to loop infinitely.

This was because GDB did not recognize trap instructions other than
"trap". For example, "tw 12, 4, 4" was not be recognized, causing GDB
to loop forever.

This commit fixes this by providing POWER specific hook
(gdbarch_program_breakpoint_here_p) recognizing all tw, twi, td and tdi
instructions.

Tested on Linux on PowerPC e500 and on QEMU PPC64le.
---
 gdb/rs6000-tdep.c                       | 65 ++++++++++++++++++++++
 gdb/testsuite/gdb.arch/powerpc-trap.exp | 72 +++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/powerpc-trap.s   | 31 +++++++++++
 gdb/testsuite/gdb.arch/powerpc64-trap.s | 33 ++++++++++++
 4 files changed, 201 insertions(+)
 create mode 100644 gdb/testsuite/gdb.arch/powerpc-trap.exp
 create mode 100644 gdb/testsuite/gdb.arch/powerpc-trap.s
 create mode 100644 gdb/testsuite/gdb.arch/powerpc64-trap.s

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 43880fa4426..ce98dc2f884 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -6247,6 +6247,69 @@ ppc_process_record (struct gdbarch *gdbarch, struct regcache *regcache,
   return 0;
 }
 
+/* Used for matching tw, twi, td and tdi instructions for POWER.  */
+
+static constexpr uint32_t TX_INSN_MASK = 0xFC0007FF;
+static constexpr uint32_t TW_INSN = 0x7C000008;
+static constexpr uint32_t TD_INSN = 0x7C000088;
+
+static constexpr uint32_t TXI_INSN_MASK = 0xFC000000;
+static constexpr uint32_t TWI_INSN = 0x0C000000;
+static constexpr uint32_t TDI_INSN = 0x08000000;
+
+static inline bool
+is_tw_insn (uint32_t insn)
+{
+  return (insn & TX_INSN_MASK) == TW_INSN;
+}
+
+static inline bool
+is_twi_insn (uint32_t insn)
+{
+  return (insn & TXI_INSN_MASK) == TWI_INSN;
+}
+
+static inline bool
+is_td_insn (uint32_t insn)
+{
+  return (insn & TX_INSN_MASK) == TD_INSN;
+}
+
+static inline bool
+is_tdi_insn (uint32_t insn)
+{
+  return (insn & TXI_INSN_MASK) == TDI_INSN;
+}
+
+/* Implementation of gdbarch_program_breakpoint_here_p for POWER.  */
+
+static bool
+rs6000_program_breakpoint_here_p (gdbarch *gdbarch, CORE_ADDR address)
+{
+  gdb_byte target_mem[PPC_INSN_SIZE];
+
+  /* Enable the automatic memory restoration from breakpoints while
+     we read the memory.  Otherwise we may find temporary breakpoints, ones
+     inserted by GDB, and flag them as permanent breakpoints.  */
+  scoped_restore restore_memory
+    = make_scoped_restore_show_memory_breakpoints (0);
+
+  if (target_read_memory (address, target_mem, PPC_INSN_SIZE) == 0)
+    {
+      uint32_t insn = (uint32_t) extract_unsigned_integer
+        (target_mem, PPC_INSN_SIZE, gdbarch_byte_order_for_code (gdbarch));
+
+      /* Check if INSN is a TW, TWI, TD or TDI instruction.  There
+         are multiple choices of such instructions with different registers
+         and / or immediate values but they all cause a break. */
+      if (is_tw_insn (insn) || is_twi_insn (insn) || is_td_insn (insn)
+          || is_tdi_insn (insn))
+        return true;
+    }
+
+  return false;
+}
+
 /* Initialize the current architecture based on INFO.  If possible, re-use an
    architecture from ARCHES, which is a list of architectures already created
    during this debugging session.
@@ -7109,6 +7172,8 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 				       rs6000_breakpoint::kind_from_pc);
   set_gdbarch_sw_breakpoint_from_kind (gdbarch,
 				       rs6000_breakpoint::bp_from_kind);
+  set_gdbarch_program_breakpoint_here_p (gdbarch,
+                                         rs6000_program_breakpoint_here_p);
 
   /* The value of symbols of type N_SO and N_FUN maybe null when
      it shouldn't be.  */
diff --git a/gdb/testsuite/gdb.arch/powerpc-trap.exp b/gdb/testsuite/gdb.arch/powerpc-trap.exp
new file mode 100644
index 00000000000..9cdbbf0cc30
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc-trap.exp
@@ -0,0 +1,72 @@
+# 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/>.
+#
+# This file is part of the gdb testsuite.
+
+# Test if GDB stops at various trap instructions inserted into
+# the code.
+
+if { [istarget powerpc-*] } {
+   standard_testfile powerpc-trap.s
+   # Number of expected SIGTRAP's to get.  This needs to be kept in sync
+   # with the source file powerpc-trap.s
+   set expected_traps 3
+} elseif {[istarget powerpc64*] } {
+   standard_testfile powerpc64-trap.s
+   # Number of expected SIGTRAP's to get.  This needs to be kept in sync
+   # with the source file powerpc64-trap.s
+   set expected_traps 5
+} else {
+    verbose "Skipping ${gdb_test_file_name}."
+    return
+}
+
+if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile}]} {
+    return -1
+}
+
+if {![runto_main]} {
+    untested "could not run to main"
+    return -1
+}
+
+set keep_going 1
+set count 0
+
+while {$keep_going} {
+    set keep_going 0
+
+    # Continue to next program breakpoint instruction.
+    gdb_test_multiple "continue" "trap instruction $count causes SIGTRAP" {
+	-re "Program received signal SIGTRAP, Trace/breakpoint trap.*$gdb_prompt $" {
+	    pass $gdb_test_name
+
+	    # Advance PC to next instruction
+	    gdb_test "set \$pc = \$pc + 4" "" "advance past trap instruction $count"
+
+	    incr count
+	    if {$count < $expected_traps} {
+	    	set keep_going 1
+	    }
+	}
+    }
+}
+
+# Verify we stopped at the expected number of SIGTRAP's.
+gdb_assert {$count == $expected_traps} "all trap instructions triggered"
+
+# One last continue to reach the end of the test, to make sure we don't get
+# another SIGTRAP.
+gdb_test "continue" "exited normally.*" "continue to end"
diff --git a/gdb/testsuite/gdb.arch/powerpc-trap.s b/gdb/testsuite/gdb.arch/powerpc-trap.s
new file mode 100644
index 00000000000..b03176f747e
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc-trap.s
@@ -0,0 +1,31 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   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/>. */
+
+/* To test if GDB stops at various trap instructions inserted into
+   the code.  */
+
+.global main
+.type main,function
+main:
+  ori 0, 0, 0
+  trap
+  tw  12, 2, 2
+  twi 31, 3, 3
+  ori 0, 0, 0
+  li  3, 0
+  blr
+
diff --git a/gdb/testsuite/gdb.arch/powerpc64-trap.s b/gdb/testsuite/gdb.arch/powerpc64-trap.s
new file mode 100644
index 00000000000..2272b42edc2
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc64-trap.s
@@ -0,0 +1,33 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   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/>. */
+
+/* To test if GDB stops at various trap instructions inserted into
+   the code.  */
+
+.global main
+.type main,function
+main:
+  ori 0, 0, 0
+  trap
+  tw  12, 2, 2
+  twi 31, 3, 3
+  td  12, 2, 2
+  tdi 31, 3, 3
+  ori 0, 0, 0
+  li  3, 0
+  blr
+
-- 
2.30.2

Re: [PATCH v3 1/2] ppc: use "trap" ("tw, 31, 0, 0") as breakpoint instruction

[Joel Brobecker]

Hi Jan,

On Wed, Dec 01, 2021 at 02:30:04PM +0000, Jan Vrany wrote:
> Power ISA 3.0 B spec [1], sections 3.3.11 "Fixed-Point Trap Instructions"
> and section C.6 "Trap Mnemonics" specify "tw, 31, 0, 0" (encoded as
> 0x7fe00008) as canonical unconditional trap instruction.
> 
> This commit changes the breakpoint instruction used by GDB from
> "tw 12, r2, r2" to unconditional "trap".
> 
> [1]: https://openpowerfoundation.org/?resource_lib=power-isa-version-3-0

Thanks for the additional reference. This looks good to me, and
I think it's can be pushed independently of patch #2.

> ---
>  gdb/rs6000-tdep.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index 87a494e0bb8..43880fa4426 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -824,8 +824,8 @@ rs6000_fetch_pointer_argument (struct frame_info *frame, int argi,
>  
>  /* Sequence of bytes for breakpoint instruction.  */
>  
> -constexpr gdb_byte big_breakpoint[] = { 0x7d, 0x82, 0x10, 0x08 };
> -constexpr gdb_byte little_breakpoint[] = { 0x08, 0x10, 0x82, 0x7d };
> +constexpr gdb_byte big_breakpoint[] = { 0x7f, 0xe0, 0x00, 0x08 };
> +constexpr gdb_byte little_breakpoint[] = { 0x08, 0x00, 0xe0, 0x7f };
>  
>  typedef BP_MANIPULATION_ENDIAN (little_breakpoint, big_breakpoint)
>    rs6000_breakpoint;
> -- 
> 2.30.2
> 

-- 
Joel

Re: [PATCH v3 2/2] ppc: recognize all program traps

[Joel Brobecker]

> Permanent program breakpoints (ones inserted into the code) other than
> the one GDB uses for POWER (0x7fe00008) did not result in stop but
> caused GDB to loop infinitely.
> 
> This was because GDB did not recognize trap instructions other than
> "trap". For example, "tw 12, 4, 4" was not be recognized, causing GDB
> to loop forever.
> 
> This commit fixes this by providing POWER specific hook
> (gdbarch_program_breakpoint_here_p) recognizing all tw, twi, td and tdi
> instructions.
> 
> Tested on Linux on PowerPC e500 and on QEMU PPC64le.

I quickly scanned the patch and it looked good to me. But I'll let
Pedro give the final seal of approval, as he had some good remarks
regarding the testing part.

> ---
>  gdb/rs6000-tdep.c                       | 65 ++++++++++++++++++++++
>  gdb/testsuite/gdb.arch/powerpc-trap.exp | 72 +++++++++++++++++++++++++
>  gdb/testsuite/gdb.arch/powerpc-trap.s   | 31 +++++++++++
>  gdb/testsuite/gdb.arch/powerpc64-trap.s | 33 ++++++++++++
>  4 files changed, 201 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.arch/powerpc-trap.exp
>  create mode 100644 gdb/testsuite/gdb.arch/powerpc-trap.s
>  create mode 100644 gdb/testsuite/gdb.arch/powerpc64-trap.s
> 
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index 43880fa4426..ce98dc2f884 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -6247,6 +6247,69 @@ ppc_process_record (struct gdbarch *gdbarch, struct regcache *regcache,
>    return 0;
>  }
>  
> +/* Used for matching tw, twi, td and tdi instructions for POWER.  */
> +
> +static constexpr uint32_t TX_INSN_MASK = 0xFC0007FF;
> +static constexpr uint32_t TW_INSN = 0x7C000008;
> +static constexpr uint32_t TD_INSN = 0x7C000088;
> +
> +static constexpr uint32_t TXI_INSN_MASK = 0xFC000000;
> +static constexpr uint32_t TWI_INSN = 0x0C000000;
> +static constexpr uint32_t TDI_INSN = 0x08000000;
> +
> +static inline bool
> +is_tw_insn (uint32_t insn)
> +{
> +  return (insn & TX_INSN_MASK) == TW_INSN;
> +}
> +
> +static inline bool
> +is_twi_insn (uint32_t insn)
> +{
> +  return (insn & TXI_INSN_MASK) == TWI_INSN;
> +}
> +
> +static inline bool
> +is_td_insn (uint32_t insn)
> +{
> +  return (insn & TX_INSN_MASK) == TD_INSN;
> +}
> +
> +static inline bool
> +is_tdi_insn (uint32_t insn)
> +{
> +  return (insn & TXI_INSN_MASK) == TDI_INSN;
> +}
> +
> +/* Implementation of gdbarch_program_breakpoint_here_p for POWER.  */
> +
> +static bool
> +rs6000_program_breakpoint_here_p (gdbarch *gdbarch, CORE_ADDR address)
> +{
> +  gdb_byte target_mem[PPC_INSN_SIZE];
> +
> +  /* Enable the automatic memory restoration from breakpoints while
> +     we read the memory.  Otherwise we may find temporary breakpoints, ones
> +     inserted by GDB, and flag them as permanent breakpoints.  */
> +  scoped_restore restore_memory
> +    = make_scoped_restore_show_memory_breakpoints (0);
> +
> +  if (target_read_memory (address, target_mem, PPC_INSN_SIZE) == 0)
> +    {
> +      uint32_t insn = (uint32_t) extract_unsigned_integer
> +        (target_mem, PPC_INSN_SIZE, gdbarch_byte_order_for_code (gdbarch));
> +
> +      /* Check if INSN is a TW, TWI, TD or TDI instruction.  There
> +         are multiple choices of such instructions with different registers
> +         and / or immediate values but they all cause a break. */
> +      if (is_tw_insn (insn) || is_twi_insn (insn) || is_td_insn (insn)
> +          || is_tdi_insn (insn))
> +        return true;
> +    }
> +
> +  return false;
> +}
> +
>  /* Initialize the current architecture based on INFO.  If possible, re-use an
>     architecture from ARCHES, which is a list of architectures already created
>     during this debugging session.
> @@ -7109,6 +7172,8 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>  				       rs6000_breakpoint::kind_from_pc);
>    set_gdbarch_sw_breakpoint_from_kind (gdbarch,
>  				       rs6000_breakpoint::bp_from_kind);
> +  set_gdbarch_program_breakpoint_here_p (gdbarch,
> +                                         rs6000_program_breakpoint_here_p);
>  
>    /* The value of symbols of type N_SO and N_FUN maybe null when
>       it shouldn't be.  */
> diff --git a/gdb/testsuite/gdb.arch/powerpc-trap.exp b/gdb/testsuite/gdb.arch/powerpc-trap.exp
> new file mode 100644
> index 00000000000..9cdbbf0cc30
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/powerpc-trap.exp
> @@ -0,0 +1,72 @@
> +# 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/>.
> +#
> +# This file is part of the gdb testsuite.
> +
> +# Test if GDB stops at various trap instructions inserted into
> +# the code.
> +
> +if { [istarget powerpc-*] } {
> +   standard_testfile powerpc-trap.s
> +   # Number of expected SIGTRAP's to get.  This needs to be kept in sync
> +   # with the source file powerpc-trap.s
> +   set expected_traps 3
> +} elseif {[istarget powerpc64*] } {
> +   standard_testfile powerpc64-trap.s
> +   # Number of expected SIGTRAP's to get.  This needs to be kept in sync
> +   # with the source file powerpc64-trap.s
> +   set expected_traps 5
> +} else {
> +    verbose "Skipping ${gdb_test_file_name}."
> +    return
> +}
> +
> +if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile}]} {
> +    return -1
> +}
> +
> +if {![runto_main]} {
> +    untested "could not run to main"
> +    return -1
> +}
> +
> +set keep_going 1
> +set count 0
> +
> +while {$keep_going} {
> +    set keep_going 0
> +
> +    # Continue to next program breakpoint instruction.
> +    gdb_test_multiple "continue" "trap instruction $count causes SIGTRAP" {
> +	-re "Program received signal SIGTRAP, Trace/breakpoint trap.*$gdb_prompt $" {
> +	    pass $gdb_test_name
> +
> +	    # Advance PC to next instruction
> +	    gdb_test "set \$pc = \$pc + 4" "" "advance past trap instruction $count"
> +
> +	    incr count
> +	    if {$count < $expected_traps} {
> +	    	set keep_going 1
> +	    }
> +	}
> +    }
> +}
> +
> +# Verify we stopped at the expected number of SIGTRAP's.
> +gdb_assert {$count == $expected_traps} "all trap instructions triggered"
> +
> +# One last continue to reach the end of the test, to make sure we don't get
> +# another SIGTRAP.
> +gdb_test "continue" "exited normally.*" "continue to end"
> diff --git a/gdb/testsuite/gdb.arch/powerpc-trap.s b/gdb/testsuite/gdb.arch/powerpc-trap.s
> new file mode 100644
> index 00000000000..b03176f747e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/powerpc-trap.s
> @@ -0,0 +1,31 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   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/>. */
> +
> +/* To test if GDB stops at various trap instructions inserted into
> +   the code.  */
> +
> +.global main
> +.type main,function
> +main:
> +  ori 0, 0, 0
> +  trap
> +  tw  12, 2, 2
> +  twi 31, 3, 3
> +  ori 0, 0, 0
> +  li  3, 0
> +  blr
> +
> diff --git a/gdb/testsuite/gdb.arch/powerpc64-trap.s b/gdb/testsuite/gdb.arch/powerpc64-trap.s
> new file mode 100644
> index 00000000000..2272b42edc2
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/powerpc64-trap.s
> @@ -0,0 +1,33 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   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/>. */
> +
> +/* To test if GDB stops at various trap instructions inserted into
> +   the code.  */
> +
> +.global main
> +.type main,function
> +main:
> +  ori 0, 0, 0
> +  trap
> +  tw  12, 2, 2
> +  twi 31, 3, 3
> +  td  12, 2, 2
> +  tdi 31, 3, 3
> +  ori 0, 0, 0
> +  li  3, 0
> +  blr
> +
> -- 
> 2.30.2
> 

-- 
Joel

Re: [PATCH v3 2/2] ppc: recognize all program traps

[Pedro Alves]

Thanks!  This LGTM.  Just a super tiny nit to pick.  No need to repost for this.

> +if { [istarget powerpc-*] } {
> +   standard_testfile powerpc-trap.s
> +   # Number of expected SIGTRAP's to get.  This needs to be kept in sync
> +   # with the source file powerpc-trap.s

SIGTRAP's -> SIGTRAPs  (i.e., plural, not possessive.)

Add missing period at the end, after the filename:

   ... file powerpc-trap.s.

> +   set expected_traps 3
> +} elseif {[istarget powerpc64*] } {
> +   standard_testfile powerpc64-trap.s
> +   # Number of expected SIGTRAP's to get.  This needs to be kept in sync
> +   # with the source file powerpc64-trap.s

Ditto.