[RFC 1/4] [gdb/tdep] Add dummy amd64_software_single_step

[RFC 1/4] [gdb/tdep] Add dummy amd64_software_single_step

[Tom de Vries]

The documentation of gdbarch_software_single_step states:
...
Return a vector of addresses on which the software single step
breakpoints should be inserted.  NULL means software single step is
not used.
...

Add a dummy amd64_software_single_step, returning an empty vector.

As per the documentation, this shouldn't have any effect.  However, there are
regressions.

The reason for these regressions is the use of gdbarch_software_single_step_p:
...
bool
gdbarch_software_single_step_p (struct gdbarch *gdbarch)
{
  gdb_assert (gdbarch != NULL);
  return gdbarch->software_single_step != NULL;
}
...
which simply tests for the presence of the hook, and which returns true
instead of false after adding amd64_software_single_step.

Fix this by introducing a software_single_step_p which follows the logic of
insert_single_step_breakpoints, and using this instead of
gdbarch_software_single_step_p.

This leaves gdbarch_software_single_step_p used only in:
- software_single_step_p, and
- of gdbarch_displaced_step_hw_singlestep.

In the latter case, software_single_step_p doesn't work well, so instead
use a amd64_displaced_step_hw_singlestep always returning true.

Tested on x86_64-linux.
---
 gdb/amd64-tdep.c  | 15 +++++++++++++++
 gdb/breakpoint.c  | 15 +++++++++++++++
 gdb/breakpoint.h  |  2 ++
 gdb/infrun.c      |  2 +-
 gdb/linux-nat.c   |  4 ++--
 gdb/record-full.c |  4 ++--
 6 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index e6feee677b3..85721bbadf4 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -3158,6 +3158,18 @@ amd64_in_indirect_branch_thunk (struct gdbarch *gdbarch, CORE_ADDR pc)
 				       AMD64_RIP_REGNUM);
 }
 
+static std::vector<CORE_ADDR>
+amd64_software_single_step (struct regcache *regcache)
+{
+  return {};
+}
+
+static bool
+amd64_displaced_step_hw_singlestep (gdbarch *)
+{
+  return true;
+}
+
 void
 amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch,
 		const target_desc *default_tdesc)
@@ -3329,6 +3341,9 @@ amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch,
 					amd64_in_indirect_branch_thunk);
 
   register_amd64_ravenscar_ops (gdbarch);
+
+  set_gdbarch_software_single_step (gdbarch, amd64_software_single_step);
+  set_gdbarch_displaced_step_hw_singlestep (gdbarch, amd64_displaced_step_hw_singlestep);
 }
 
 /* Initialize ARCH for x86-64, no osabi.  */
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 699919e32b3..60e795e1849 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -13945,6 +13945,21 @@ insert_single_step_breakpoint (struct gdbarch *gdbarch,
   update_global_location_list (UGLL_INSERT);
 }
 
+bool
+software_single_step_p (struct gdbarch *gdbarch)
+{
+  if (!gdbarch_software_single_step_p (gdbarch))
+    return false;
+
+  regcache *regcache = get_thread_regcache (inferior_thread ());
+  std::vector<CORE_ADDR> next_pcs;
+
+  next_pcs = gdbarch_software_single_step (gdbarch, regcache);
+
+  return !next_pcs.empty ();
+}
+
+
 /* Insert single step breakpoints according to the current state.  */
 
 int
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index feb798224c0..20b1eb23b83 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1815,6 +1815,8 @@ extern void insert_single_step_breakpoint (struct gdbarch *,
 					   const address_space *,
 					   CORE_ADDR);
 
+extern bool software_single_step_p (struct gdbarch *gdbarch);
+
 /* Insert all software single step breakpoints for the current frame.
    Return true if any software single step breakpoints are inserted,
    otherwise, return false.  */
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 45c1b4a79bb..f7f76737c40 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2366,7 +2366,7 @@ maybe_software_singlestep (struct gdbarch *gdbarch)
   bool hw_step = true;
 
   if (execution_direction == EXEC_FORWARD
-      && gdbarch_software_single_step_p (gdbarch))
+      && software_single_step_p (gdbarch))
     hw_step = !insert_single_step_breakpoints (gdbarch);
 
   return hw_step;
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 737e0f7bda6..9954750fc84 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -490,8 +490,8 @@ linux_nat_target::follow_fork (inferior *child_inf, ptid_t child_ptid,
 	  /* Note that we consult the parent's architecture instead of
 	     the child's because there's no inferior for the child at
 	     this point.  */
-	  if (!gdbarch_software_single_step_p (target_thread_architecture
-					       (parent_ptid)))
+	  if (!software_single_step_p (target_thread_architecture
+				       (parent_ptid)))
 	    {
 	      int status;
 
diff --git a/gdb/record-full.c b/gdb/record-full.c
index 2fc9e433ca8..023e9cb7b26 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -1074,7 +1074,7 @@ record_full_target::resume (ptid_t ptid, int step, enum gdb_signal signal)
       if (!step)
 	{
 	  /* This is not hard single step.  */
-	  if (!gdbarch_software_single_step_p (gdbarch))
+	  if (!software_single_step_p (gdbarch))
 	    {
 	      /* This is a normal continue.  */
 	      step = 1;
@@ -1249,7 +1249,7 @@ record_full_wait_1 (struct target_ops *ops,
 		      process_stratum_target *proc_target
 			= current_inferior ()->process_target ();
 
-		      if (gdbarch_software_single_step_p (gdbarch))
+		      if (software_single_step_p (gdbarch))
 			{
 			  /* Try to insert the software single step breakpoint.
 			     If insert success, set step to 0.  */

base-commit: 35efddd5a12bbe514ff3870ec67a0357774fbe04
-- 
2.35.3

[RFC 2/4] [gdb] Introduce maint set prefer-software-single-stepping on/off

[Tom de Vries]

Add a maintenance command that enables using software single-stepping on
targets that support this:
...
(gdb) maint set prefer-software-single-stepping on
...

By default, prefer-software-single-stepping is off, and behaviour is
unchanged.

Also, there are no users yet, the following patch will add one for amd64.

This is an RFC for the moment, so no docs yet.

Tested on x86_64-linux.
---
 gdb/breakpoint.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 60e795e1849..3d90e2a2159 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -13945,6 +13945,9 @@ insert_single_step_breakpoint (struct gdbarch *gdbarch,
   update_global_location_list (UGLL_INSERT);
 }
 
+extern bool prefer_software_single_stepping;
+bool prefer_software_single_stepping;
+
 bool
 software_single_step_p (struct gdbarch *gdbarch)
 {
@@ -15168,6 +15171,16 @@ When on, breakpoint location specific debugging is enabled."),
 			   show_debug_breakpoint,
 			   &setdebuglist, &showdebuglist);
 
+  add_setshow_boolean_cmd ("prefer-software-single-stepping", class_maintenance,
+			   &prefer_software_single_stepping, _("\
+Set whether software single-stepping is preferred."), _("\
+Show whether software single-stepping is preferred."), _("\
+When on, if both are available, software single-stepping is preferred over hardware single-stepping."),
+			   NULL,
+			   NULL,
+			   &maintenance_set_cmdlist,
+			   &maintenance_show_cmdlist);
+
   add_setshow_enum_cmd ("condition-evaluation", class_breakpoint,
 			condition_evaluation_enums,
 			&condition_evaluation_mode_1, _("\
-- 
2.35.3

[RFC 3/4] [gdb/tdep] Enable prefer-software-single-stepping on amd64

[Tom de Vries]

Add amd64 support for prefer-software-single-stepping.

The support is not complete (only non control-flow insns are supported), but
it doesn't need to be since hardware single-stepping is used as fallback.

Tested on x86_64-linux.
---
 gdb/amd64-tdep.c | 68 ++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 63 insertions(+), 5 deletions(-)

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 85721bbadf4..841b072c714 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -1194,7 +1194,7 @@ static const unsigned char twobyte_has_modrm[256] = {
   /*	   0 1 2 3 4 5 6 7 8 9 a b c d e f	  */
 };
 
-static int amd64_syscall_p (const struct amd64_insn *insn, int *lengthp);
+static int amd64_syscall_with_length_p (const struct amd64_insn *insn, int *lengthp);
 
 static int
 rex_prefix_p (gdb_byte pfx)
@@ -1514,7 +1514,7 @@ amd64_displaced_step_copy_insn (struct gdbarch *gdbarch,
   {
     int syscall_length;
 
-    if (amd64_syscall_p (details, &syscall_length))
+    if (amd64_syscall_with_length_p (details, &syscall_length))
       buf[details->opcode_offset + syscall_length] = NOP_OPCODE;
   }
 
@@ -1623,11 +1623,28 @@ amd64_call_p (const struct amd64_insn *details)
   return 0;
 }
 
+static int
+amd64_cond_jmp_p (const struct amd64_insn *details)
+{
+  const gdb_byte *insn = &details->raw_insn[details->opcode_offset];
+
+  if (insn[0] >= 0x70 && insn[0] <= 0x7f)
+    return 1;
+
+  if (insn[0] == 0xe3)
+    return 1;
+
+  if (insn[0] == 0x0f && insn[1] >= 0x80 && insn[1] <= 0x8f)
+    return 1;
+
+  return 0;
+}
+
 /* Return non-zero if INSN is a system call, and set *LENGTHP to its
    length in bytes.  Otherwise, return zero.  */
 
 static int
-amd64_syscall_p (const struct amd64_insn *details, int *lengthp)
+amd64_syscall_with_length_p (const struct amd64_insn *details, int *lengthp)
 {
   const gdb_byte *insn = &details->raw_insn[details->opcode_offset];
 
@@ -1640,6 +1657,15 @@ amd64_syscall_p (const struct amd64_insn *details, int *lengthp)
   return 0;
 }
 
+/* As amd64_syscall_with_length_p, but without the lengthp argument.  */
+
+static int
+amd64_syscall_p (const struct amd64_insn *details)
+{
+  int dummy;
+  return amd64_syscall_with_length_p (details, &dummy);
+}
+
 /* Classify the instruction at ADDR using PRED.
    Throw an error if the memory can't be read.  */
 
@@ -1683,6 +1709,18 @@ amd64_insn_is_jump (struct gdbarch *gdbarch, CORE_ADDR addr)
   return amd64_classify_insn_at (gdbarch, addr, amd64_jmp_p);
 }
 
+static int
+amd64_insn_is_cond_jump (struct gdbarch *gdbarch, CORE_ADDR addr)
+{
+  return amd64_classify_insn_at (gdbarch, addr, amd64_cond_jmp_p);
+}
+
+static int
+amd64_insn_is_syscall (struct gdbarch *gdbarch, CORE_ADDR addr)
+{
+  return amd64_classify_insn_at (gdbarch, addr, amd64_syscall_p);
+}
+
 /* Fix up the state of registers and memory after having single-stepped
    a displaced instruction.  */
 
@@ -1748,7 +1786,7 @@ amd64_displaced_step_fixup (struct gdbarch *gdbarch,
 	 the instruction has put control where it belongs, and leave
 	 it unrelocated.  Goodness help us if there are PC-relative
 	 system calls.	*/
-      if (amd64_syscall_p (insn_details, &insn_len)
+      if (amd64_syscall_with_length_p (insn_details, &insn_len)
 	  /* GDB can get control back after the insn after the syscall.
 	     Presumably this is a kernel bug.  Fixup ensures it's a nop, we
 	     add one to the length for it.  */
@@ -3158,10 +3196,30 @@ amd64_in_indirect_branch_thunk (struct gdbarch *gdbarch, CORE_ADDR pc)
 				       AMD64_RIP_REGNUM);
 }
 
+extern bool prefer_software_single_stepping;
+
 static std::vector<CORE_ADDR>
 amd64_software_single_step (struct regcache *regcache)
 {
-  return {};
+  if (!prefer_software_single_stepping)
+    return {};
+
+  struct gdbarch *gdbarch = regcache->arch ();
+  CORE_ADDR pc = regcache_read_pc (regcache);
+
+  if (amd64_insn_is_call (gdbarch, pc))
+    return {};
+  else if (amd64_insn_is_ret (gdbarch, pc))
+    return {};
+  else if (amd64_insn_is_jump (gdbarch, pc))
+    return {};
+  else if (amd64_insn_is_cond_jump (gdbarch, pc))
+    return {};
+  else if (amd64_insn_is_syscall (gdbarch, pc))
+    return {};
+
+  size_t len = gdb_insn_length (gdbarch, pc);
+  return { pc + len };
 }
 
 static bool
-- 
2.35.3

[RFC 4/4] [gdb/testsuite] Add test-case gdb.base/gdb-sigterm-3.exp

[Tom de Vries]

Reproduce PR gdb/31061 (originally reported on arm, a software single-step
architecture) on amd64, a hardware single-step architecture, using the new
maintenance command "maint set prefer-software-single-stepping on".

The test-case is based on gdb.base/gdb-sigterm.exp, so leave the 2013-2023
copyright years in place.

Tested on x86_64-linux.
---
 gdb/testsuite/gdb.base/gdb-sigterm-3.c   |  52 +++++++++++
 gdb/testsuite/gdb.base/gdb-sigterm-3.exp | 105 +++++++++++++++++++++++
 2 files changed, 157 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/gdb-sigterm-3.c
 create mode 100644 gdb/testsuite/gdb.base/gdb-sigterm-3.exp

diff --git a/gdb/testsuite/gdb.base/gdb-sigterm-3.c b/gdb/testsuite/gdb.base/gdb-sigterm-3.c
new file mode 100644
index 00000000000..3e359f009ee
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdb-sigterm-3.c
@@ -0,0 +1,52 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013-2023 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/>.  */
+
+#include <unistd.h>
+
+#if defined(__s390__) || defined(__s390x__)
+#define NOP asm("nopr 0")
+#elif defined(__or1k__)
+#define NOP asm("l.nop")
+#else
+#define NOP asm("nop")
+#endif
+
+#define NOP10 NOP; NOP; NOP; NOP; NOP; NOP; NOP; NOP; NOP; NOP
+
+int
+main (void)
+{
+  /* Allow for as much timeout as DejaGnu wants, plus a bit of
+     slack.  */
+  unsigned int seconds = TIMEOUT + 20;
+
+  alarm (seconds);
+
+  for (;;) /* loop-line */
+    {
+      NOP10;
+      NOP10;
+      NOP10;
+      NOP10;
+      NOP10;
+      NOP10;
+      NOP10;
+      NOP10;
+      NOP10;
+      NOP10;
+    }
+}
diff --git a/gdb/testsuite/gdb.base/gdb-sigterm-3.exp b/gdb/testsuite/gdb.base/gdb-sigterm-3.exp
new file mode 100644
index 00000000000..44ce20b4f26
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdb-sigterm-3.exp
@@ -0,0 +1,105 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2013-2023 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 relies on checking gdb debug output. Do not run if gdb debug is
+# enabled as any debug will be redirected to the log.
+require !gdb_debug_enabled
+
+standard_testfile
+
+# The test program exits after a while, in case GDB crashes.  Make it
+# wait at least as long as we may wait before declaring a time out
+# failure.
+set options { "additional_flags=-DTIMEOUT=$timeout" debug }
+if { [build_executable ${testfile}.exp ${testfile} $srcfile $options] == -1 } {
+    return -1
+}
+
+# Return 0 on success, non-zero otherwise.
+
+proc do_test { pass } {
+    global testfile gdb_prompt binfile pf_prefix
+
+    if ![runto_main] {
+	return -1
+    }
+
+    gdb_breakpoint "${testfile}.c:[gdb_get_line_number "loop-line" ${testfile}.c]" \
+		   temporary
+
+    gdb_test "continue" "Temporary breakpoint .* NOP10;.*"
+
+    gdb_test_no_output "set range-stepping off"
+    gdb_test_no_output "set debug infrun 1"
+
+    set abort 1
+    gdb_test_multiple "stepi-loop" "run a bit" {
+	-re {\[infrun\] process_event_stop_test: stepi/nexti} {
+	    pass $gdb_test_name
+	    set abort 0
+	}
+    }
+    if $abort {
+	verbose -log "$pf_prefix $test: did not run"
+	return $abort
+    }
+
+    set gdb_pid [exp_pid -i [board_info host fileid]]
+    remote_exec host "kill -TERM ${gdb_pid}"
+
+    set test "expect eof"
+    set abort 1
+    set stepping 0
+    # If GDB mishandles the SIGTERM and doesn't exit, this should FAIL
+    # with timeout.  We don't expect a GDB prompt, so we see one,
+    # we'll FAIL too.
+    gdb_test_multiple "" "expect eof" {
+	eof {
+	    pass "$gdb_test_name (got eof)"
+	    set abort 0
+	}
+	-re {\[infrun\] process_event_stop_test: stepping inside range} {
+	    incr stepping
+	    exp_continue
+	}
+    }
+    verbose -log "$pf_prefix $test: stepped $stepping times"
+    return $abort
+}
+
+# Testcase was FAILing approx. on 10th pass with unpatched GDB.
+# 50 runs should be approx. a safe number to be sure it is fixed now.
+set passes 50
+
+for {set pass 0} {$pass < $passes} {incr pass} {
+    with_test_prefix "pass=$pass" {
+	clean_restart ${testfile}
+	send_gdb "define stepi-loop\n"
+	send_gdb "while 1\n"
+	send_gdb "stepi\n"
+	send_gdb "end\n"
+	send_gdb "end\n"
+	gdb_test "" ""
+	gdb_test_no_output "maint set prefer-software-single-stepping on"
+
+	if { [do_test $pass] != 0 } {
+	    break
+	}
+    }
+}
+
+gdb_assert {$pass == $passes} "$passes SIGTERM passes"
-- 
2.35.3