Re: Flip the interpreter to synchronously wait for commands finishing, in command lists and similars

[Pedro Alves <pedro@codesourcery.com>]

On Saturday 03 September 2011 00:28:15, Matt Rice wrote:
> On Fri, Sep 2, 2011 at 10:07 AM, Pedro Alves <pedro@codesourcery.com> wrote:
> >
> > With this patch, synchronous execution commands (the regular step, next,
> > etc., that is, those with an & suffixed) with a target running
> > in async mode work correctly (AFAICT) with all current use cases.    We
> > can consider flipping on async on by default, and then incrementally
> > convert specific paths to state machines, for new use cases, rather than
> > delaying flipping on async on by default until _everything_ is converted
> > into a state-machine.
> 
> I was hoping this might fix, (but doesn't seem to) the following case:
> 
> ./gdb/gdb -ex 'set target-async on' -ex 'attach 7625' -ex 'continue'
> ./gdb/gdb -ex 'set target-async on' -ex 'attach 7625' -ex 'continue&'
> 
> both these commands exhibit:
> Continuing.
> 
> Program received signal SIGSTOP, Stopped (signal).
> .....
> (gdb)

Thanks.  An attach ends with a TARGET_SIGNAL_STOP/SIGSTOP, that is
normally suppressed by STOP_QUIETLY_NO_SIGSTOP, but we weren't entering
the new loop in execute_command because the current thread isn't marked
running at that point yet.  So, "continue" ran before the attach was
complete, and "continue"ing calls clear_proceed_status
which wipes inferior->stop_soon.  By the time we process the TARGET_SIGNAL_STOP,
we can no longer explain it, and so report it as a regular SIGSTOP.

I had added the "is_running()" check for the hook-stop-continue.exp test,
as otherwise, when we went to go execute the first command in the hook-stop
command list, being that a command that doesn't start the target 
running, we'd enter the loop in execute_command:

 if (!interpreter_async && sync_execution)
	{
	  while (gdb_do_one_event () >= 0)
	    if (!sync_execution)
	      break;
	}

but we'd never leave it, hanging in gdb_do_one_event, because the
previous command had already finished, and the target was stopped
already, but sync_execution was still set.

Here's a better fix for that issue, which fixes your example
too.  Clear sync_execution before running the hook-stop, as
soon as the previous sync execution command finishes.

This make a hell lot more sense.  I don't know why I didn't
think of it before :-).

Tested it on x86_64-linux (async) and applied.

> thus changing the behavior of '-ex continue' should we turn
> target-async on by default.

ECANTPARSE?

-- 
Pedro Alves

2011-09-05  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* inf-loop.c (execute_command): Don't check if the current thread
	if running before synchronously waiting for command completion.
	* infrun.c (fetch_inferior_event): Handle "set exec-done-display"
	here.
	(normal_stop): Call async_enable_stdin here.
	* inf-loop.c (inferior_event_handler): Don't call
	async_enable_stdin, nor handle "set exec-done-display" here.

---
 gdb/inf-loop.c |   14 --------------
 gdb/infrun.c   |   17 +++++++++++++++--
 gdb/top.c      |    2 +-
 3 files changed, 16 insertions(+), 17 deletions(-)

Index: src/gdb/top.c
===================================================================
--- src.orig/gdb/top.c	2011-09-05 15:11:13.063963929 +0100
+++ src/gdb/top.c	2011-09-05 15:11:54.333963936 +0100
@@ -447,7 +447,7 @@ execute_command (char *p, int from_tty)
 	 command's list, running command hooks or similars), and we
 	 just ran a synchronous command that started the target, wait
 	 for that command to end.  */
-      if (!interpreter_async && sync_execution && is_running (inferior_ptid))
+      if (!interpreter_async && sync_execution)
 	{
 	  while (gdb_do_one_event () >= 0)
 	    if (!sync_execution)
Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c	2011-09-05 15:11:12.353963929 +0100
+++ src/gdb/infrun.c	2011-09-05 15:11:54.333963936 +0100
@@ -2713,6 +2713,7 @@ fetch_inferior_event (void *client_data)
   struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
   struct cleanup *ts_old_chain;
   int was_sync = sync_execution;
+  int cmd_done = 0;
 
   memset (ecs, 0, sizeof (*ecs));
 
@@ -2804,7 +2805,10 @@ fetch_inferior_event (void *client_data)
 	  && ecs->event_thread->control.stop_step)
 	inferior_event_handler (INF_EXEC_CONTINUE, NULL);
       else
-	inferior_event_handler (INF_EXEC_COMPLETE, NULL);
+	{
+	  inferior_event_handler (INF_EXEC_COMPLETE, NULL);
+	  cmd_done = 1;
+	}
     }
 
   /* No error, don't finish the thread states yet.  */
@@ -2814,9 +2818,17 @@ fetch_inferior_event (void *client_data)
   do_cleanups (old_chain);
 
   /* If the inferior was in sync execution mode, and now isn't,
-     restore the prompt.  */
+     restore the prompt (a synchronous execution command has finished,
+     and we're ready for input).  */
   if (interpreter_async && was_sync && !sync_execution)
     display_gdb_prompt (0);
+
+  if (cmd_done
+      && !was_sync
+      && exec_done_display_p
+      && (ptid_equal (inferior_ptid, null_ptid)
+	  || !is_running (inferior_ptid)))
+    printf_unfiltered (_("completed.\n"));
 }
 
 /* Record the frame and location we're currently stepping through.  */
@@ -5814,6 +5826,7 @@ normal_stop (void)
     goto done;
 
   target_terminal_ours ();
+  async_enable_stdin ();
 
   /* Set the current source location.  This will also happen if we
      display the frame below, but the current SAL will be incorrect
Index: src/gdb/inf-loop.c
===================================================================
--- src.orig/gdb/inf-loop.c	2011-09-05 15:11:12.353963929 +0100
+++ src/gdb/inf-loop.c	2011-09-05 15:11:54.333963936 +0100
@@ -41,7 +41,6 @@ void
 inferior_event_handler (enum inferior_event_type event_type, 
 			gdb_client_data client_data)
 {
-  int was_sync = 0;
   struct cleanup *cleanup_if_error = make_bpstat_clear_actions_cleanup ();
 
   switch (event_type)
@@ -63,7 +62,6 @@ inferior_event_handler (enum inferior_ev
       break;
 
     case INF_EXEC_COMPLETE:
-
       if (!non_stop)
 	{
 	  /* Unregister the inferior from the event loop.  This is done
@@ -73,12 +71,6 @@ inferior_event_handler (enum inferior_ev
 	    target_async (NULL, 0);
 	}
 
-      /* The call to async_enable_stdin below resets 'sync_execution'.
-	 However, if sync_execution is 1 now, we also need to show the
-	 prompt below, so save the current value.  */
-      was_sync = sync_execution;
-      async_enable_stdin ();
-
       /* Do all continuations associated with the whole inferior (not
 	 a particular thread).  */
       if (!ptid_equal (inferior_ptid, null_ptid))
@@ -129,12 +121,6 @@ inferior_event_handler (enum inferior_ev
 	    }
 	  exception_print (gdb_stderr, e);
 	}
-
-      if (!was_sync
-	  && exec_done_display_p
-	  && (ptid_equal (inferior_ptid, null_ptid)
-	      || !is_running (inferior_ptid)))
-	printf_unfiltered (_("completed.\n"));
       break;
 
     case INF_EXEC_CONTINUE: