diff mbox

Small clean up of use_displaced_stepping

Message ID 20200227224424.270604-1-simon.marchi@polymtl.ca
State New
Headers show

Commit Message

Simon Marchi Feb. 27, 2020, 10:44 p.m. UTC
This function returns the result of a quite big condition.  I think it
would be more redeable if it was broken up in smaller pieces and
commented.  This is what this patch does.

I also introduced gdbarch_supports_displaced_stepping, since it shows
the intent better than checking for gdbarch_displaced_step_copy_insn_p.
I also used that new function in displaced_step_prepare_throw.

I also updated the comment on top of can_use_displaced_stepping, which
seemed a bit outdated with respect to non-stop.  The comment likely
dates from before it was possible to have targets that always operate
non-stop under the hood, even when the user-visible mode is all-stop.

No functional changes intended.

gdb/ChangeLog:

	* infrun.c (gdbarch_supports_displaced_stepping): New.
	(use_displaced_stepping): Break up conditions in smaller pieces.
	Use gdbarch_supports_displaced_stepping.
	(displaced_step_prepare_throw): Use
	gdbarch_supports_displaced_stepping.
---
 gdb/infrun.c | 52 +++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 13 deletions(-)

Comments

Kevin Buettner March 1, 2020, 2:41 a.m. UTC | #1
On Thu, 27 Feb 2020 17:44:24 -0500
Simon Marchi <simon.marchi@polymtl.ca> wrote:

> This function returns the result of a quite big condition.  I think it
> would be more redeable if it was broken up in smaller pieces and

s/redeable/readable/

> commented.  This is what this patch does.
> 
> I also introduced gdbarch_supports_displaced_stepping, since it shows
> the intent better than checking for gdbarch_displaced_step_copy_insn_p.
> I also used that new function in displaced_step_prepare_throw.
> 
> I also updated the comment on top of can_use_displaced_stepping, which
> seemed a bit outdated with respect to non-stop.  The comment likely
> dates from before it was possible to have targets that always operate
> non-stop under the hood, even when the user-visible mode is all-stop.
> 
> No functional changes intended.
> 
> gdb/ChangeLog:
> 
> 	* infrun.c (gdbarch_supports_displaced_stepping): New.
> 	(use_displaced_stepping): Break up conditions in smaller pieces.
> 	Use gdbarch_supports_displaced_stepping.
> 	(displaced_step_prepare_throw): Use
> 	gdbarch_supports_displaced_stepping.

I read through the patch.  It appears to be equivalent to the terse
conditional that used to be there.  I find it much easier to understand
now.  Thanks for doing this.

Kevin
Simon Marchi March 2, 2020, 8:59 p.m. UTC | #2
On 2020-02-29 9:41 p.m., Kevin Buettner wrote:
> On Thu, 27 Feb 2020 17:44:24 -0500
> Simon Marchi <simon.marchi@polymtl.ca> wrote:
> 
>> This function returns the result of a quite big condition.  I think it
>> would be more redeable if it was broken up in smaller pieces and
> 
> s/redeable/readable/

Fixed.

>> commented.  This is what this patch does.
>>
>> I also introduced gdbarch_supports_displaced_stepping, since it shows
>> the intent better than checking for gdbarch_displaced_step_copy_insn_p.
>> I also used that new function in displaced_step_prepare_throw.
>>
>> I also updated the comment on top of can_use_displaced_stepping, which
>> seemed a bit outdated with respect to non-stop.  The comment likely
>> dates from before it was possible to have targets that always operate
>> non-stop under the hood, even when the user-visible mode is all-stop.
>>
>> No functional changes intended.
>>
>> gdb/ChangeLog:
>>
>> 	* infrun.c (gdbarch_supports_displaced_stepping): New.
>> 	(use_displaced_stepping): Break up conditions in smaller pieces.
>> 	Use gdbarch_supports_displaced_stepping.
>> 	(displaced_step_prepare_throw): Use
>> 	gdbarch_supports_displaced_stepping.
> 
> I read through the patch.  It appears to be equivalent to the terse
> conditional that used to be there.  I find it much easier to understand
> now.  Thanks for doing this.

Thanks for the review, I pushed the patch.

Simon
diff mbox

Patch

diff --git a/gdb/infrun.c b/gdb/infrun.c
index d9a6f7335194..bb1dae638666 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1556,8 +1556,7 @@  infrun_inferior_exit (struct inferior *inf)
    doesn't support it, GDB will instead use the traditional
    hold-and-step approach.  If AUTO (which is the default), GDB will
    decide which technique to use to step over breakpoints depending on
-   which of all-stop or non-stop mode is active --- displaced stepping
-   in non-stop mode; hold-and-step in all-stop mode.  */
+   whether the target works in a non-stop way (see use_displaced_stepping).  */
 
 static enum auto_boolean can_use_displaced_stepping = AUTO_BOOLEAN_AUTO;
 
@@ -1577,23 +1576,50 @@  show_can_use_displaced_stepping (struct ui_file *file, int from_tty,
 			"to step over breakpoints is %s.\n"), value);
 }
 
+/* Return true if the gdbarch implements the required methods to use
+   displaced stepping.  */
+
+static bool
+gdbarch_supports_displaced_stepping (gdbarch *arch)
+{
+  return gdbarch_displaced_step_copy_insn_p (arch);
+}
+
 /* Return non-zero if displaced stepping can/should be used to step
    over breakpoints of thread TP.  */
 
-static int
-use_displaced_stepping (struct thread_info *tp)
+static bool
+use_displaced_stepping (thread_info *tp)
 {
-  struct regcache *regcache = get_thread_regcache (tp);
-  struct gdbarch *gdbarch = regcache->arch ();
+  /* If the user disabled it explicitly, don't use displaced stepping.  */
+  if (can_use_displaced_stepping == AUTO_BOOLEAN_FALSE)
+    return false;
+
+  /* If "auto", only use displaced stepping if the target operates in a non-stop
+     way.  */
+  if (can_use_displaced_stepping == AUTO_BOOLEAN_AUTO
+      && !target_is_non_stop_p ())
+    return false;
+
+  gdbarch *gdbarch = get_thread_regcache (tp)->arch ();
+
+  /* If the architecture doesn't implement displaced stepping, don't use
+     it.  */
+  if (!gdbarch_supports_displaced_stepping (gdbarch))
+    return false;
+
+  /* If recording, don't use displaced stepping.  */
+  if (find_record_target () != nullptr)
+    return false;
+
   displaced_step_inferior_state *displaced_state
     = get_displaced_stepping_state (tp->inf);
 
-  return (((can_use_displaced_stepping == AUTO_BOOLEAN_AUTO
-	    && target_is_non_stop_p ())
-	   || can_use_displaced_stepping == AUTO_BOOLEAN_TRUE)
-	  && gdbarch_displaced_step_copy_insn_p (gdbarch)
-	  && find_record_target () == NULL
-	  && !displaced_state->failed_before);
+  /* If displaced stepping failed before, don't bother trying again.  */
+  if (displaced_state->failed_before)
+    return false;
+
+  return true;
 }
 
 /* Simple function wrapper around displaced_step_inferior_state::reset.  */
@@ -1650,7 +1676,7 @@  displaced_step_prepare_throw (thread_info *tp)
 
   /* We should never reach this function if the architecture does not
      support displaced stepping.  */
-  gdb_assert (gdbarch_displaced_step_copy_insn_p (gdbarch));
+  gdb_assert (gdbarch_supports_displaced_stepping (gdbarch));
 
   /* Nor if the thread isn't meant to step over a breakpoint.  */
   gdb_assert (tp->control.trap_expected);