Re: [RFC v5 6/8] fix py-finish-breakpoint.exp with always-async

[Pedro Alves <palves@redhat.com>]

Hi,

I somehow neglected to respond to your response to:
 https://sourceware.org/ml/gdb-patches/2013-11/msg00298.html
at:
 https://sourceware.org/ml/gdb-patches/2013-12/msg00349.html

I investigated this further in the direction of my comments,
and extended the commit log with the resulting details.
Here's what I pushed.

Thanks.

-----------------
From: Tom Tromey <tromey@redhat.com>
Fix py-finish-breakpoint.exp with target async.

With target async enabled, py-finish-breakpoint.exp triggers an
assertion failure.

The failure occurs because execute_command re-enters the event loop in
some circumstances, and in this case resets the sync_execution flag.
Then later GDB reaches this assertion in normal_stop:

      gdb_assert (sync_execution || !target_can_async_p ());

In detail:

#1 - A synchronous execution command is run.  sync_execution is set.

#2 - A python breakpoint is hit (TARGET_WAITKIND_STOPPED), and the
     corresponding Python breakpoint's stop method is executed.  When
     and while python commands are executed, interpreter_async is
     forced to 0.

#3 - The Python stop method happens to execute a not-execution-related
     gdb command.  In this case, "where 1".

#4 - Seeing that sync_execution is set, execute_command nests a new
     event loop (although that wasn't necessary; this is the problem).

#5 - The linux-nat target's pipe in the event loop happens to be
     marked.  That's normal, due to this in linux_nat_wait:

     /* If we requested any event, and something came out, assume there
        may be more.  If we requested a specific lwp or process, also
        assume there may be more.  */

     The nested event loop thus immediately wakes up and calls
     target_wait.  No thread is actually executing in the inferior, so
     the target returns TARGET_WAITKIND_NO_RESUMED.

#6 - normal_stop is reached.  GDB prints "No unwaited-for children
     left.", and resets the sync_execution flag (IOW, there are no
     resumed threads left, so the synchronous command is considered
     completed.)  This is already bogus.  We were handling a
     breakpoint!

#7 - the nested event loop unwinds/ends.  GDB is now back to handling
     the python stop method (TARGET_WAITKIND_STOPPED), which decides
     the breakpoint should stop.  normal_stop is called for this
     event.  However, normal_stop actually works with the _last_
     reported target status:

   void
   normal_stop (void)
   {
    struct target_waitstatus last;
    ptid_t last_ptid;
    struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);

    ...
    get_last_target_status (&last_ptid, &last);

    ...
    if (last.kind == TARGET_WAITKIND_NO_RESUMED)
      {
        gdb_assert (sync_execution || !target_can_async_p ());

        target_terminal_ours_for_output ();
        printf_filtered (_("No unwaited-for children left.\n"));
      }

   And due to the nesting in execute command, the last event is now
   TARGET_WAITKIND_NO_RESUMED, not the actual breakpoint event being
   handled.  This could be seen to be broken in itself, but we can
   leave fixing that for another pass.  The assertion is reached, and
   fails.

execute_command has a comment explaining when it should synchronously
wait for events:

      /* If the interpreter is in sync mode (we're running a user
	 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.  */

However, the code did not follow this comment -- it didn't check to
see if the command actually started the target, just whether the
target was executing a sync command at this point.

This patch fixes the problem by noting whether the target was
executing in sync_execution mode before running the command, and then
augmenting the condition to test this as well.

2014-03-20  Tom Tromey  <tromey@redhat.com>

	PR gdb/14135
	* top.c (execute_command): Only dispatch events if the command
	started the target.
---
 gdb/ChangeLog | 6 ++++++
 gdb/top.c     | 4 +++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index acb25f8..1db0ee9 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,11 @@
 2014-03-20  Tom Tromey  <tromey@redhat.com>
 
+	PR gdb/14135
+	* top.c (execute_command): Only dispatch events if the command
+	started the target.
+
+2014-03-20  Tom Tromey  <tromey@redhat.com>
+
 	PR cli/15718
 	* infcall.c: Include event-top.h.
 	(run_inferior_call): Call async_disable_stdin if needed.
diff --git a/gdb/top.c b/gdb/top.c
index e1a1331..fa20025 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -406,6 +406,8 @@ execute_command (char *p, int from_tty)
     {
       const char *cmd = p;
       char *arg;
+      int was_sync = sync_execution;
+
       line = p;
 
       /* If trace-commands is set then this will print this command.  */
@@ -461,7 +463,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)
+      if (!interpreter_async && !was_sync && sync_execution)
 	{
 	  while (gdb_do_one_event () >= 0)
 	    if (!sync_execution)
-- 
1.7.11.7