Re: [PATCH 2/2] Remove gdbarch method displaced_step_hw_singlestep

[Yao Qi <qiyaoltc at gmail dot com>]

Pedro Alves <palves@redhat.com> writes:

> I wasn't sure whether this is safe, but I convinced myself it is.
> I'd have been nice if there had been a note in the log about the below:
>
> Currently, when stepping past an instruction in the scratch pad, these
> archs won't ever reach the software_single_step method, always forcing a
> hardware single-step, even if the software_single_step method would
> insert some breakpoint.  The question is: is it safe now for them to
> analyse the instruction copied to the scratch pad, and potentially insert
> a software single-step?
>
> I think it is safe, because we won't ever use displaced stepping
> for the cases where the software_single_step method would return
> something non-NULL.  software_single_step returns non-NULL _only_

except displaced stepping on arm linux target.  However, GDB won't use
software single step for displaced stepping, because it either
single-step the instructions the scratch pad or resume the program from
the instructions in the scratch pad but these instructions end with a
breakpoint instruction (see arm_displaced_init_closure).

> for atomic regions, while OTOH, displaced_step_copy_insn always returns
> NULL for atomic regions.  E.g., notice how ppc_displaced_step_copy_insn
> vs ppc_deal_with_atomic_sequence.

I'd like to add some comments, see the patch below,

-- 
Yao (éå)
From c758e0080db93b26c88ab2d6dddb5451470c1a7f Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Fri, 18 Mar 2016 16:25:38 +0000
Subject: [PATCH] Remove gdbarch method displaced_step_hw_singlestep

displaced_step_hw_singlestep was added for some targets, which can do
hardware single step, but need software single step for some special
instructions.  After we change gdbarch method software_single_step,
displaced_step_hw_singlestep is no longer necessary, because we can
get the same information from software_single_step.  This patch is
to remove displaced_step_hw_singlestep.

gdb:

2016-05-09  Yao Qi  <yao.qi@linaro.org>

	* aarch64-linux-tdep.c (aarch64_linux_init_abi): Don't call
	set_gdbarch_displaced_step_hw_singlestep.
	* aarch64-tdep.c (aarch64_displaced_step_hw_singlestep): Remove.
	* arch-utils.c (default_displaced_step_hw_singlestep): Remove.
	* arch-utils.h (default_displaced_step_hw_singlestep): Remove.
	* gdbarch.sh (displaced_step_hw_singlestep): Remove.
	* gdbarch.c, gdbarch.h: Regenerated.
	* infrun.c (resume): Don't call
	gdbarch_displaced_step_hw_singlestep.  Call
	gdbarch_software_single_step instead.
	* rs6000-tdep.c (ppc_displaced_step_hw_singlestep): Remove.
	(rs6000_gdbarch_init): Don't call
	set_gdbarch_displaced_step_hw_singlestep.
	* s390-linux-tdep.c (s390_displaced_step_hw_singlestep): Remove.
	(s390_gdbarch_init): Don't call
	set_gdbarch_displaced_step_hw_singlestep.

diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index 651a0f0..8344d06 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -1208,8 +1208,6 @@ aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   set_gdbarch_displaced_step_free_closure (gdbarch,
 					   simple_displaced_step_free_closure);
   set_gdbarch_displaced_step_location (gdbarch, linux_displaced_step_location);
-  set_gdbarch_displaced_step_hw_singlestep (gdbarch,
-					    aarch64_displaced_step_hw_singlestep);
 }
 
 /* Provide a prototype to silence -Wmissing-prototypes.  */
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 52c9ea6..460eade 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -2637,15 +2637,6 @@ aarch64_displaced_step_fixup (struct gdbarch *gdbarch,
     }
 }
 
-/* Implement the "displaced_step_hw_singlestep" gdbarch method.  */
-
-int
-aarch64_displaced_step_hw_singlestep (struct gdbarch *gdbarch,
-				      struct displaced_step_closure *closure)
-{
-  return 1;
-}
-
 /* 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.
diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index c3d7802..3049252 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -67,13 +67,6 @@ simple_displaced_step_free_closure (struct gdbarch *gdbarch,
   xfree (closure);
 }
 
-int
-default_displaced_step_hw_singlestep (struct gdbarch *gdbarch,
-				      struct displaced_step_closure *closure)
-{
-  return !gdbarch_software_single_step_p (gdbarch);
-}
-
 CORE_ADDR
 displaced_step_at_entry_point (struct gdbarch *gdbarch)
 {
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index 9e1e70e..318e227 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -45,11 +45,6 @@ extern void
   simple_displaced_step_free_closure (struct gdbarch *gdbarch,
                                       struct displaced_step_closure *closure);
 
-/* Default implementation of gdbarch_displaced_hw_singlestep.  */
-extern int
-  default_displaced_step_hw_singlestep (struct gdbarch *,
-					struct displaced_step_closure *);
-
 /* Possible value for gdbarch_displaced_step_location:
    Place displaced instructions at the program's entry point,
    leaving space for inferior function call return breakpoints.  */
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index e59a93d..d469dc9 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -277,7 +277,6 @@ struct gdbarch
   gdbarch_skip_permanent_breakpoint_ftype *skip_permanent_breakpoint;
   ULONGEST max_insn_length;
   gdbarch_displaced_step_copy_insn_ftype *displaced_step_copy_insn;
-  gdbarch_displaced_step_hw_singlestep_ftype *displaced_step_hw_singlestep;
   gdbarch_displaced_step_fixup_ftype *displaced_step_fixup;
   gdbarch_displaced_step_free_closure_ftype *displaced_step_free_closure;
   gdbarch_displaced_step_location_ftype *displaced_step_location;
@@ -414,7 +413,6 @@ gdbarch_alloc (const struct gdbarch_info *info,
   gdbarch->adjust_dwarf2_line = default_adjust_dwarf2_line;
   gdbarch->register_reggroup_p = default_register_reggroup_p;
   gdbarch->skip_permanent_breakpoint = default_skip_permanent_breakpoint;
-  gdbarch->displaced_step_hw_singlestep = default_displaced_step_hw_singlestep;
   gdbarch->displaced_step_fixup = NULL;
   gdbarch->displaced_step_free_closure = NULL;
   gdbarch->displaced_step_location = NULL;
@@ -624,7 +622,6 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of skip_permanent_breakpoint, invalid_p == 0 */
   /* Skip verify of max_insn_length, has predicate.  */
   /* Skip verify of displaced_step_copy_insn, has predicate.  */
-  /* Skip verify of displaced_step_hw_singlestep, invalid_p == 0 */
   /* Skip verify of displaced_step_fixup, has predicate.  */
   if ((! gdbarch->displaced_step_free_closure) != (! gdbarch->displaced_step_copy_insn))
     fprintf_unfiltered (log, "\n\tdisplaced_step_free_closure");
@@ -873,9 +870,6 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
                       "gdbarch_dump: displaced_step_free_closure = <%s>\n",
                       host_address_to_string (gdbarch->displaced_step_free_closure));
   fprintf_unfiltered (file,
-                      "gdbarch_dump: displaced_step_hw_singlestep = <%s>\n",
-                      host_address_to_string (gdbarch->displaced_step_hw_singlestep));
-  fprintf_unfiltered (file,
                       "gdbarch_dump: displaced_step_location = <%s>\n",
                       host_address_to_string (gdbarch->displaced_step_location));
   fprintf_unfiltered (file,
@@ -3749,23 +3743,6 @@ set_gdbarch_displaced_step_copy_insn (struct gdbarch *gdbarch,
 }
 
 int
-gdbarch_displaced_step_hw_singlestep (struct gdbarch *gdbarch, struct displaced_step_closure *closure)
-{
-  gdb_assert (gdbarch != NULL);
-  gdb_assert (gdbarch->displaced_step_hw_singlestep != NULL);
-  if (gdbarch_debug >= 2)
-    fprintf_unfiltered (gdb_stdlog, "gdbarch_displaced_step_hw_singlestep called\n");
-  return gdbarch->displaced_step_hw_singlestep (gdbarch, closure);
-}
-
-void
-set_gdbarch_displaced_step_hw_singlestep (struct gdbarch *gdbarch,
-                                          gdbarch_displaced_step_hw_singlestep_ftype displaced_step_hw_singlestep)
-{
-  gdbarch->displaced_step_hw_singlestep = displaced_step_hw_singlestep;
-}
-
-int
 gdbarch_displaced_step_fixup_p (struct gdbarch *gdbarch)
 {
   gdb_assert (gdbarch != NULL);
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index e026c0e..da0edcc 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -957,20 +957,6 @@ typedef struct displaced_step_closure * (gdbarch_displaced_step_copy_insn_ftype)
 extern struct displaced_step_closure * gdbarch_displaced_step_copy_insn (struct gdbarch *gdbarch, CORE_ADDR from, CORE_ADDR to, struct regcache *regs);
 extern void set_gdbarch_displaced_step_copy_insn (struct gdbarch *gdbarch, gdbarch_displaced_step_copy_insn_ftype *displaced_step_copy_insn);
 
-/* Return true if GDB should use hardware single-stepping to execute
-   the displaced instruction identified by CLOSURE.  If false,
-   GDB will simply restart execution at the displaced instruction
-   location, and it is up to the target to ensure GDB will receive
-   control again (e.g. by placing a software breakpoint instruction
-   into the displaced instruction buffer).
-  
-   The default implementation returns false on all targets that
-   provide a gdbarch_software_single_step routine, and true otherwise. */
-
-typedef int (gdbarch_displaced_step_hw_singlestep_ftype) (struct gdbarch *gdbarch, struct displaced_step_closure *closure);
-extern int gdbarch_displaced_step_hw_singlestep (struct gdbarch *gdbarch, struct displaced_step_closure *closure);
-extern void set_gdbarch_displaced_step_hw_singlestep (struct gdbarch *gdbarch, gdbarch_displaced_step_hw_singlestep_ftype *displaced_step_hw_singlestep);
-
 /* Fix up the state resulting from successfully single-stepping a
    displaced instruction, to give the result we would have gotten from
    stepping the instruction in its original location.
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index b144c04..8b8710f 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -780,17 +780,6 @@ V:ULONGEST:max_insn_length:::0:0
 # that case.
 M:struct displaced_step_closure *:displaced_step_copy_insn:CORE_ADDR from, CORE_ADDR to, struct regcache *regs:from, to, regs
 
-# Return true if GDB should use hardware single-stepping to execute
-# the displaced instruction identified by CLOSURE.  If false,
-# GDB will simply restart execution at the displaced instruction
-# location, and it is up to the target to ensure GDB will receive
-# control again (e.g. by placing a software breakpoint instruction
-# into the displaced instruction buffer).
-#
-# The default implementation returns false on all targets that
-# provide a gdbarch_software_single_step routine, and true otherwise.
-m:int:displaced_step_hw_singlestep:struct displaced_step_closure *closure:closure::default_displaced_step_hw_singlestep::0
-
 # Fix up the state resulting from successfully single-stepping a
 # displaced instruction, to give the result we would have gotten from
 # stepping the instruction in its original location.
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 3ffec86..464e62a 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2626,8 +2626,33 @@ resume (enum gdb_signal sig)
 	  pc = regcache_read_pc (get_thread_regcache (inferior_ptid));
 
 	  displaced = get_displaced_stepping_state (ptid_get_pid (inferior_ptid));
-	  step = gdbarch_displaced_step_hw_singlestep (gdbarch,
-						       displaced->step_closure);
+
+	  /* Although GDB won't use software single step for displaced
+	     stepping, we call software_single_step here to determine
+	     whether to
+	     - single step the instructions in the scratch pad, (like
+	     x86, ppc and aarch64),
+	     - or resume the program from the instructions in the scratch
+	     pad.  These instructions must end with a breakpoint
+	     instruction.  */
+	  if (gdbarch_software_single_step_p (gdbarch))
+	    {
+	      VEC (CORE_ADDR) * next_pcs;
+
+	      next_pcs = gdbarch_software_single_step (gdbarch,
+						       get_current_frame ());
+
+	      if (next_pcs != NULL)
+		{
+		  step = 0;
+		  VEC_free (CORE_ADDR, next_pcs);
+		}
+	      else
+		step = 1;
+	    }
+	  else
+	    step = 1;
+
 	}
     }
 
diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 26c8ed9..5f0ede2 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -1136,15 +1136,6 @@ ppc_displaced_step_fixup (struct gdbarch *gdbarch,
 				    from + offset);
 }
 
-/* Always use hardware single-stepping to execute the
-   displaced instruction.  */
-static int
-ppc_displaced_step_hw_singlestep (struct gdbarch *gdbarch,
-				  struct displaced_step_closure *closure)
-{
-  return 1;
-}
-
 /* Checks for an atomic sequence of instructions beginning with a LWARX/LDARX
    instruction and ending with a STWCX/STDCX instruction.  If such a sequence
    is found, attempt to step through it.  A breakpoint is placed at the end of 
@@ -6071,8 +6062,7 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   /* Setup displaced stepping.  */
   set_gdbarch_displaced_step_copy_insn (gdbarch,
 					ppc_displaced_step_copy_insn);
-  set_gdbarch_displaced_step_hw_singlestep (gdbarch,
-					    ppc_displaced_step_hw_singlestep);
+
   set_gdbarch_displaced_step_fixup (gdbarch, ppc_displaced_step_fixup);
   set_gdbarch_displaced_step_free_closure (gdbarch,
 					   simple_displaced_step_free_closure);
diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c
index dd26ba6..77c7b33 100644
--- a/gdb/s390-linux-tdep.c
+++ b/gdb/s390-linux-tdep.c
@@ -759,14 +759,6 @@ s390_software_single_step (struct frame_info *frame)
   return next_pcs;
 }
 
-static int
-s390_displaced_step_hw_singlestep (struct gdbarch *gdbarch,
-				   struct displaced_step_closure *closure)
-{
-  return 1;
-}
-
-
 /* Maps for register sets.  */
 
 static const struct regcache_map_entry s390_gregmap[] =
@@ -7990,7 +7982,6 @@ s390_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
   set_gdbarch_breakpoint_from_pc (gdbarch, s390_breakpoint_from_pc);
   set_gdbarch_software_single_step (gdbarch, s390_software_single_step);
-  set_gdbarch_displaced_step_hw_singlestep (gdbarch, s390_displaced_step_hw_singlestep);
   set_gdbarch_skip_prologue (gdbarch, s390_skip_prologue);
   set_gdbarch_stack_frame_destroyed_p (gdbarch, s390_stack_frame_destroyed_p);