[rfc] Fix PR 2250 - multithreaded single-step problems in all-stop mode

[rfc] Fix PR 2250 - multithreaded single-step problems in all-stop mode

[Ulrich Weigand]

Hello,

a while ago I posted a patch to fix problem PR 2250:
http://sourceware.org/ml/gdb-patches/2008-05/msg00089.html

This problem is about the behaviour of GDB when single-stepping
a thread of a multi-threaded application.  If during a single-step
operation, some other event happens in another thread, GDB may get
into an inconsistent state, where some internal data structures
still think a single-step operation is going on, while this is
in fact no longer true.

The patch I originally proposed, however, conflicted with the
new non-stop mode (where single-step operations can be outstanding
in multiple threads at the same time), so it was never applied.
Now that all the non-stop code is in mainline, the original bug
is still present in the all-stop mode.  I've reworked the patch
to fix the problem in all-stop mode, while keeping the new behaviour
in non-stop mode unchanges.

As discussed in the message refered to above, I'm implementing the
the following two changes:

- If the step operation is interrupted by an *internal* breakpoint
  that is handled transparently, the operation continues in a 
  transparent and consistent manner after the breakpoint was handled.
  (This is the handle_inferior_event change in the patch below.)

- If the step operation is interrupted by an *explicit* breakpoint
  that breaks to a user prompt, it is completely cancelled.  It is
  then up to the user how to continue from the prompt.
  (This is the clear_proceed_status change in the patch below.)

Regression tested on powerpc64-linux.  This patch fixes the problem
originally reported as PR 2250 (thanks to Emi Suzuki for verifying!).

I didn't test anything with all-stop mode, but by construction the
patch should not modify the behaviour at all in this case.

Does this look right?   If there are no objections, I'm planning
on committing this in a couple of days.

Bye,
Ulrich


ChangeLog:

	PR gdb/2250
	* infrun.c (clear_proceed_status_thread): New function.
	(clear_proceed_status_callback): New function.
	(clear_proceed_status): In all-stop mode, clear per-thread
	proceed status of *all* threads, not only the current.
	(handle_inferior_event): In all-stop mode, if we're stepping
	one thread, but got some inferior event in another thread
	that does not cause GDB to break to the user interface,
	ensure the interrupted stepping operation continues in the
	original thread.
	(currently_stepping): Move thread-related tests to ...
	(currently_stepping_thread): ... this new function.
	(currently_stepping_callback): New function.

Index: gdb/infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.336
diff -u -p -r1.336 infrun.c
--- gdb/infrun.c	5 Nov 2008 20:23:07 -0000	1.336
+++ gdb/infrun.c	13 Nov 2008 19:54:06 -0000
@@ -75,6 +75,8 @@ static void set_schedlock_func (char *ar
 
 static int currently_stepping (struct thread_info *tp);
 
+static int currently_stepping_callback (struct thread_info *tp, void *data);
+
 static void xdb_handle_command (char *args, int from_tty);
 
 static int prepare_to_proceed (int);
@@ -1161,31 +1163,59 @@ a command like `return' or `jump' to con
 /* Clear out all variables saying what to do when inferior is continued.
    First do this, then set the ones you want, then call `proceed'.  */
 
+static void
+clear_proceed_status_thread (struct thread_info *tp)
+{
+  if (debug_infrun)
+    fprintf_unfiltered (gdb_stdlog,
+			"infrun: clear_proceed_status_thread (%s)\n",
+			target_pid_to_str (tp->ptid));
+
+  tp->trap_expected = 0;
+  tp->step_range_start = 0;
+  tp->step_range_end = 0;
+  tp->step_frame_id = null_frame_id;
+  tp->step_over_calls = STEP_OVER_UNDEBUGGABLE;
+  tp->stop_requested = 0;
+
+  tp->stop_step = 0;
+
+  tp->proceed_to_finish = 0;
+
+  /* Discard any remaining commands or status from previous stop.  */
+  bpstat_clear (&tp->stop_bpstat);
+}
+
+static int
+clear_proceed_status_callback (struct thread_info *tp, void *data)
+{
+  if (is_exited (tp->ptid))
+    return 0;
+
+  clear_proceed_status_thread (tp);
+  return 0;
+}
+
 void
 clear_proceed_status (void)
 {
   if (!ptid_equal (inferior_ptid, null_ptid))
     {
-      struct thread_info *tp;
       struct inferior *inferior;
 
-      tp = inferior_thread ();
-
-      tp->trap_expected = 0;
-      tp->step_range_start = 0;
-      tp->step_range_end = 0;
-      tp->step_frame_id = null_frame_id;
-      tp->step_over_calls = STEP_OVER_UNDEBUGGABLE;
-      tp->stop_requested = 0;
-
-      tp->stop_step = 0;
-
-      tp->proceed_to_finish = 0;
-
-      /* Discard any remaining commands or status from previous
-	 stop.  */
-      bpstat_clear (&tp->stop_bpstat);
-
+      if (non_stop)
+	{
+	  /* If in non-stop mode, only delete the per-thread status
+	     of the current thread.  */
+	  clear_proceed_status_thread (inferior_thread ());
+	}
+      else
+	{
+	  /* In all-stop mode, delete the per-thread status of
+	     *all* threads.  */
+	  iterate_over_threads (clear_proceed_status_callback, NULL);
+	}
+  
       inferior = current_inferior ();
       inferior->stop_soon = NO_STOP_QUIETLY;
     }
@@ -3222,6 +3252,43 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
      test for stepping.  But, if not stepping,
      do not stop.  */
 
+  /* In all-stop mode, if we're currently stepping but have stopped in
+     some other thread, we need to switch back to the stepped thread.  */
+  if (!non_stop)
+    {
+      struct thread_info *tp;
+      tp = iterate_over_threads (currently_stepping_callback,
+				 ecs->event_thread);
+      if (tp)
+	{
+	  /* However, if the current thread is blocked on some internal
+	     breakpoint, and we simply need to step over that breakpoint
+	     to get it going again, do that first.  */
+	  if ((ecs->event_thread->trap_expected
+	       && ecs->event_thread->stop_signal != TARGET_SIGNAL_TRAP)
+	      || ecs->event_thread->stepping_over_breakpoint)
+	    {
+	      keep_going (ecs);
+	      return;
+	    }
+
+	  /* Otherwise, we no longer expect a trap in the current thread.
+	     Clear the trap_expected flag before switching back -- this is
+	     what keep_going would do as well, if we called it.  */
+	  ecs->event_thread->trap_expected = 0;
+
+	  if (debug_infrun)
+	    fprintf_unfiltered (gdb_stdlog,
+				"infrun: switching back to stepped thread\n");
+
+	  ecs->event_thread = tp;
+	  ecs->ptid = tp->ptid;
+	  context_switch (ecs->ptid);
+	  keep_going (ecs);
+	  return;
+	}
+    }
+
   /* Are we stepping to get the inferior out of the dynamic linker's
      hook (and possibly the dld itself) after catching a shlib
      event?  */
@@ -3641,12 +3708,25 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
 /* Are we in the middle of stepping?  */
 
 static int
+currently_stepping_thread (struct thread_info *tp)
+{
+  return (tp->step_range_end && tp->step_resume_breakpoint == NULL)
+	 || tp->trap_expected
+	 || tp->stepping_through_solib_after_catch;
+}
+
+static int
+currently_stepping_callback (struct thread_info *tp, void *data)
+{
+  /* Return true if any thread *but* the one passed in "data" is
+     in the middle of stepping.  */
+  return tp != data && currently_stepping_thread (tp);
+}
+
+static int
 currently_stepping (struct thread_info *tp)
 {
-  return (((tp->step_range_end && tp->step_resume_breakpoint == NULL)
-	   || tp->trap_expected)
-	  || tp->stepping_through_solib_after_catch
-	  || bpstat_should_step ());
+  return currently_stepping_thread (tp) || bpstat_should_step ();
 }
 
 /* Inferior has stepped into a subroutine call with source code that
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Fix PR 2250 - multithreaded single-step problems in all-stop mode

[Pedro Alves]

On Friday 14 November 2008 21:04:18, Ulrich Weigand wrote:

> a while ago I posted a patch to fix problem PR 2250:
> http://sourceware.org/ml/gdb-patches/2008-05/msg00089.html
> 
> This problem is about the behaviour of GDB when single-stepping
> a thread of a multi-threaded application.  If during a single-step
> operation, some other event happens in another thread, GDB may get
> into an inconsistent state, where some internal data structures
> still think a single-step operation is going on, while this is
> in fact no longer true.
> 
> The patch I originally proposed, however, conflicted with the
> new non-stop mode (where single-step operations can be outstanding
> in multiple threads at the same time), so it was never applied.
> Now that all the non-stop code is in mainline, the original bug
> is still present in the all-stop mode.  I've reworked the patch
> to fix the problem in all-stop mode, while keeping the new behaviour
> in non-stop mode unchanges.

Thank you very much for your patience and for refitting the patch.

I do find this version much cleaner than the last (for not
relying on global variables, and due to context-switching
being a thing of the past).

> 
> As discussed in the message refered to above, I'm implementing the
> the following two changes:
> 
> - If the step operation is interrupted by an *internal* breakpoint
>   that is handled transparently, the operation continues in a 
>   transparent and consistent manner after the breakpoint was handled.
>   (This is the handle_inferior_event change in the patch below.)
> 

I still agree with this behaviour.  I've made it so that GDB removes
step-resume breakpoints of all threads (in all-stop) on a normal_stop
for the same reasoning (delete_step_thread_step_resume_breakpoint).

> - If the step operation is interrupted by an *explicit* breakpoint
>   that breaks to a user prompt, it is completely cancelled.  It is
>   then up to the user how to continue from the prompt.
>   (This is the clear_proceed_status change in the patch below.)

Yay!  I had something similar here to address this bullet (updated
from the last time I showed it, but unfinished and a bit hacky) that
I ended up never submitting.  Yours is better.

> Regression tested on powerpc64-linux.  This patch fixes the problem
> originally reported as PR 2250 (thanks to Emi Suzuki for verifying!).
> 
> I didn't test anything with all-stop mode, but by construction the
> patch should not modify the behaviour at all in this case.

Yeah, should be fine (obviously talking about non-stop mode here).

> Does this look right?   If there are no objections, I'm planning
> on committing this in a couple of days.

I like this a lot.


Hmm, speaking of things that aren't cleared properly in all
threads when we are going to start a proceed, shouldn't the
things cleared in init_thread_stepping_state be cleared
in clear_proceed_status_thread instead?  E.g.,
step_after_step_resume_breakpoint, etc.

-- 
Pedro Alves

Re: [rfc] Fix PR 2250 - multithreaded single-step problems in all-stop mode

[Ulrich Weigand]

Pedro Alves wrote:

> On Friday 14 November 2008 21:04:18, Ulrich Weigand wrote:
> > Does this look right?   If there are no objections, I'm planning
> > on committing this in a couple of days.
> 
> I like this a lot.

Thanks for looking over the patch!

I've checked this in now.

> Hmm, speaking of things that aren't cleared properly in all
> threads when we are going to start a proceed, shouldn't the
> things cleared in init_thread_stepping_state be cleared
> in clear_proceed_status_thread instead?  E.g.,
> step_after_step_resume_breakpoint, etc.

Yes, it would appear everything except setting current_line
and current_symtab (which depend on prev_pc) should really
be cleared in clear_proceed_status_thread ...  I'll come 
up with an add-on patch.

B.t.w. what about init_execution_control_state?  This seems to
be nowhere called ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Fix PR 2250 - multithreaded single-step problems in all-stop mode

[Pedro Alves]

On Monday 17 November 2008 18:56:22, Ulrich Weigand wrote:

> B.t.w. what about init_execution_control_state?  This seems to
> be nowhere called ...

Hmm, good catch.  It seems that got left behind when I pulled
all that was context-switched out of struct execution_control_state
(which is now a bit mis-named).

There used to be a call to init_execution_control_state in
wait_for_inferior around where now there's a memset, so we're
not missing anything (clearing ecs->random_signal).

I don't think there's any reason to keep random_signal
untouched across handle_inferior_event invocations in the while (1)
loop in wait_for_inferior ? --- handle_inferior_event seems
to set or clear it itself before relying on its value, but I wouldn't
be that surprised there's a weird twisted code path where that isn't
happening, and we could be reading a random_signal from a previous stop.
Maybe moving the memset to inside the while (1) loop in
wait_for_inferior would make things a bit clearer (if my reasoning
about random_signal not being needed across events is correct).

-- 
Pedro Alves

Re: [rfc] Fix PR 2250 - multithreaded single-step problems in all-stop mode

[Ulrich Weigand]

Pedro Alves wrote:

> I don't think there's any reason to keep random_signal
> untouched across handle_inferior_event invocations in the while (1)
> loop in wait_for_inferior ? --- handle_inferior_event seems
> to set or clear it itself before relying on its value, but I wouldn't
> be that surprised there's a weird twisted code path where that isn't
> happening, and we could be reading a random_signal from a previous stop.
> Maybe moving the memset to inside the while (1) loop in
> wait_for_inferior would make things a bit clearer (if my reasoning
> about random_signal not being needed across events is correct).

I agree that random_signal need not survive beyond a handling of a
single event.  In fact, I think random_signal should probably simply
be a local variable of handle_inferior_event; it's not accessed
anywhere else.

In fact other members of the ecs struct should probably be
local variables, maybe some of them passed explicitly to
subroutines.  I think this would help simplify understanding
the data-flow along handle_inferior_event and its subroutines ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Fix PR 2250 - multithreaded single-step problems in all-stop mode

[Pedro Alves]

On Monday 17 November 2008 20:30:05, Ulrich Weigand wrote:
> In fact other members of the ecs struct should probably be
> local variables, maybe some of them passed explicitly to
> subroutines.  I think this would help simplify understanding
> the data-flow along handle_inferior_event and its subroutines ...

Agreed, probably, maybe.  I few months ago I started doing something like
that and got rid of ecs completely, but then I looked at the result and
noticed that cutting handle_inferior_event into smaller pieces first
(or at the same time) would probably have had better immediate clarity
gains, but I didn't try it.  That colides a bit (and possibly goes in the
opposite direction) with just plain getting rid of ecs, as by doing the latter,
you find yourself adjusting callers of callers to pass new flags around (as opposed
to having everything related to an event handy in a single struct).  That's a
similar argument to the recent struct value_print_options or replacing
current_language with passing a struct around or similars.

Anyway, I don't have that much strong feelings in either direction, just
telling the world my (possibly bogus) war story.  Patches do speak
much louder than words.  :-)

-- 
Pedro Alves

[rfc] [0/7] infrun cleanup

[Ulrich Weigand]

Pedro Alves wote:
> On Monday 17 November 2008 20:30:05, Ulrich Weigand wrote:
> > In fact other members of the ecs struct should probably be
> > local variables, maybe some of them passed explicitly to
> > subroutines.  I think this would help simplify understanding
> > the data-flow along handle_inferior_event and its subroutines ...
> 
> Agreed, probably, maybe.  I few months ago I started doing something like
> that and got rid of ecs completely, but then I looked at the result and
> noticed that cutting handle_inferior_event into smaller pieces first
> (or at the same time) would probably have had better immediate clarity
> gains, but I didn't try it.  That colides a bit (and possibly goes in the
> opposite direction) with just plain getting rid of ecs, as by doing the
> latter, you find yourself adjusting callers of callers to pass new flags
> around (as opposed to having everything related to an event handy in a
> single struct).  That's a > similar argument to the recent struct
> value_print_options or replacing > current_language with passing a
> struct around or similars.
> 
> Anyway, I don't have that much strong feelings in either direction, just
> telling the world my (possibly bogus) war story.  Patches do speak
> much louder than words.  :-)

OK, here's a couple of patches :-)  These will completely eliminate
struct execution_control_state, while at the same time making the
overall flow of control though handle_inferior_event clearer, IMO.

I'd appreciate any feedback on this approach (in particular if 
you've done things differently in your attempt) ...

Overall patch set tested on amd64-linux with no regressions.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] [0/7] infrun cleanup

[Pedro Alves]

On Sunday 07 December 2008 00:15:20, Ulrich Weigand wrote:

> I'd appreciate any feedback on this approach (in particular if 
> you've done things differently in your attempt) ...

Eh, no, a lot of dejavu here.  :-)

> Overall patch set tested on amd64-linux with no regressions.

Thanks much for doing this.  I've commented on one.  I'll
look at the rest tomorrow.

-- 
Pedro Alves

Re: [rfc] [0/7] infrun cleanup

[Pedro Alves]

On Sunday 07 December 2008 01:28:38, Pedro Alves wrote:
> On Sunday 07 December 2008 00:15:20, Ulrich Weigand wrote:
> 
> > I'd appreciate any feedback on this approach (in particular if 
> > you've done things differently in your attempt) ...
> 
> Eh, no, a lot of dejavu here.  :-)
> 
> > Overall patch set tested on amd64-linux with no regressions.
> 
> Thanks much for doing this.  I've commented on one.  I'll
> look at the rest tomorrow.
> 

It all looks good to me.

The only bit that concerns me, is:

> Within handle_inferior_event (and its subroutines), every path that
> ends in
> 
>    stop_stepping (ecs);
>    return;
> 
> is replaced by
> 
>    return 0;
> 
> and likewise every path that ends in
> 
>    prepare_to_wait (ecs);
>    return
> 
> is replaced by 
> 
>    return 1;

I'm not so sure this makes things clearer than what's there
currently.  One now has to remember what "return 0" or "return 1" means,
while previously, calls to prepare_to_wait/stop_stepping made
it quite explicit.

We also lost the debug message that hinted us that we're going
to need to wait for another target event ("infrun: prepare_to_wait"), or
that we're done ("infrun: stop_stepping").  Perhaps leave the
stop_stopping/prepare_to_wait functions, for the debug output, and
for clarity?

Say, you could make it like so:

-    prepare_to_wait (ecs);
-    return;
+    return prepare_to_wait ();

-    stop_stepping (ecs);
-    return;
+    return stop_stepping ();

And something like:

-static void
-stop_stepping (struct execution_control_state *ecs)
+static int
+stop_stepping (void)
{
  if (debug_infrun)
    fprintf_unfiltered (gdb_stdlog, "infrun: stop_stepping\n");

  /* Let callers know we don't want to wait for the inferior anymore.  */
- ecs->wait_some_more = 0;
+ return 0;
}

Maybe it's just me, though, it's not a strong opinion.

There are a couple of comments left behind that should be cleaned up,
if we remove those functions, e.g.,

/* Print why the inferior has stopped. We always print something when
   the inferior exits, or receives a signal. The rest of the cases are
   dealt with later on in normal_stop() and print_it_typical().  Ideally
   there should be a call to this function from handle_inferior_event()
   each time stop_stepping() is called.*/
static void
print_stop_reason (enum inferior_stop_reason stop_reason, int stop_info)

Or,

  /* Refresh prev_pc value just prior to resuming.  This used to be
     done in stop_stepping, however, setting prev_pc there did not handle
     scenarios such as inferior function calls or returning from
     a function via the return command.  In those cases, the prev_pc
     ...

-- 
Pedro Alves

Re: [rfc] [0/7] infrun cleanup

[Ulrich Weigand]

Pedro Alves wrote:
> On Sunday 07 December 2008 01:28:38, Pedro Alves wrote:
> > On Sunday 07 December 2008 00:15:20, Ulrich Weigand wrote:
> > 
> > > I'd appreciate any feedback on this approach (in particular if 
> > > you've done things differently in your attempt) ...
> > 
> > Eh, no, a lot of dejavu here.  :-)
> > 
> > > Overall patch set tested on amd64-linux with no regressions.
> > 
> > Thanks much for doing this.  I've commented on one.  I'll
> > look at the rest tomorrow.
> > 
> 
> It all looks good to me.

Thanks for your feedback!

> The only bit that concerns me, is:
> 
> > Within handle_inferior_event (and its subroutines), every path that
> > ends in
> > 
> >    stop_stepping (ecs);
> >    return;
> > 
> > is replaced by
> > 
> >    return 0;
> > 
> > and likewise every path that ends in
> > 
> >    prepare_to_wait (ecs);
> >    return
> > 
> > is replaced by 
> > 
> >    return 1;
> 
> I'm not so sure this makes things clearer than what's there
> currently.  One now has to remember what "return 0" or "return 1" means,
> while previously, calls to prepare_to_wait/stop_stepping made
> it quite explicit.
> 
> We also lost the debug message that hinted us that we're going
> to need to wait for another target event ("infrun: prepare_to_wait"), or
> that we're done ("infrun: stop_stepping").  Perhaps leave the
> stop_stopping/prepare_to_wait functions, for the debug output, and
> for clarity?

I agree that the return 0/1 is not quite optimal.  On the other hand,
I feel it would be nice to get rid of those nearly-empty functions
(the debug messages seem to me mostly redundant, as the callers of
stop_stepping / prepare_to_wait typically already have their own,
more specific debug message ...).

In the context of some further cleanup and splitting handle_inferior_event
into multiple more independent parts, I had been wondering whether it
might be a good idea to use a enum (like enum inferior_stop_reason) 
instead of the boolean: handle_inferior_event (and its hypothetical
subroutines) would return enum values to indicate *why* the inferior
stopped, including a new STILL_RUNNING value to indicate that it
in fact hasn't yet stopped.

In this set-up you'd have statements like

   return STILL_RUNNING;

or

   return END_STEPPING_RANGE;

or (another potential new value)

   return HIT_BREAKPOINT;

within handle_inferior_event; and its caller would be rewritten like

  do
    {
      ptid = target_wait (...);
      stop_reason = handle_inferior_event (ptid, ...);
    }
  while (stop_reason == STILL_RUNNING);

It might be feasible to use the stop_reason in the future to merge
some of the print_stop_reason stuff into normal_stop and reduce the
amount of duplicate checks; or even to replace some of the "global"
output variables like stopped_by_random_signal or tp->stop_step.

What do you think?


> There are a couple of comments left behind that should be cleaned up,
> if we remove those functions, e.g.,
> 
> /* Print why the inferior has stopped. We always print something when
>    the inferior exits, or receives a signal. The rest of the cases are
>    dealt with later on in normal_stop() and print_it_typical().  Ideally
>    there should be a call to this function from handle_inferior_event()
>    each time stop_stepping() is called.*/

I thought I had updated that one?

>   /* Refresh prev_pc value just prior to resuming.  This used to be
>      done in stop_stepping, however, setting prev_pc there did not handle
>      scenarios such as inferior function calls or returning from
>      a function via the return command.  In those cases, the prev_pc
>      ...

I left that because it specifically refers to how things were done
in the past, when we still had that function ...  Maybe it could be
marked as "the former stop_stepping" or so.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] [0/7] infrun cleanup

[Pedro Alves]

On Sunday 07 December 2008 18:19:49, Ulrich Weigand wrote:

> In the context of some further cleanup and splitting handle_inferior_event
> into multiple more independent parts, I had been wondering whether it
> might be a good idea to use a enum (like enum inferior_stop_reason) 
> instead of the boolean: handle_inferior_event (and its hypothetical
> subroutines) would return enum values to indicate *why* the inferior
> stopped, including a new STILL_RUNNING value to indicate that it
> in fact hasn't yet stopped.
> 
> In this set-up you'd have statements like
> 
>    return STILL_RUNNING;

'return WAIT_SOME_MORE;' would be historically more appropriate.  :-)

> 
> or
> 
>    return END_STEPPING_RANGE;
> 
> or (another potential new value)
> 
>    return HIT_BREAKPOINT;
> 
> within handle_inferior_event; and its caller would be rewritten like
> 
>   do
>     {
>       ptid = target_wait (...);
>       stop_reason = handle_inferior_event (ptid, ...);
>     }
>   while (stop_reason == STILL_RUNNING);
> 
> It might be feasible to use the stop_reason in the future to merge
> some of the print_stop_reason stuff into normal_stop and reduce the
> amount of duplicate checks; or even to replace some of the "global"
> output variables like stopped_by_random_signal or tp->stop_step.

Sounds good to me.


> I thought I had updated that one?

Oh, you did.  I missed it, sorry.

> 
> >   /* Refresh prev_pc value just prior to resuming.  This used to be
> >      done in stop_stepping, however, setting prev_pc there did not handle
> >      scenarios such as inferior function calls or returning from
> >      a function via the return command.  In those cases, the prev_pc
> >      ...
> 
> I left that because it specifically refers to how things were done
> in the past, when we still had that function ...  Maybe it could be
> marked as "the former stop_stepping" or so.

Well, that whole comment becomes mostly useless after these cleanups.
It refers to stop_stepping, ecs, init_execution_control_state, all things
that are gone, which seems to leave more confusion than clarify things.
I wonder if we should just remove most of it, and just comment why we
set prev_pc there...

-- 
Pedro Alves