Re: [PATCH 1/7] Merge async and sync code paths some more

[Joel Brobecker <brobecker at adacore dot com>]

Hi Pedro,

On Wed, Aug 12, 2015 at 06:01:51PM +0100, Pedro Alves wrote:
> This patch makes the execution control code use largely the same
> mechanisms in both sync- and async-capable targets.  This means using
> continuations and use the event loop to react to target events on sync
> targets as well.  The trick is to immediately mark infrun's event loop
> source after resume instead of calling wait_for_inferior.  Then
> fetch_inferior_event is adjusted to do a blocking wait on sync
> targets.
> 
> gdb/ChangeLog:
> 2015-08-12  Pedro Alves  <palves@redhat.com>
> 
> 	* breakpoint.c (bpstat_do_actions_1, until_break_command): Don't
> 	check whether the target can async.
> 	* inf-loop.c (inferior_event_handler): Only call target_async if
> 	the target can async.

This patch unfortunately breaks attaching on Windows. I suspect this
might be related to the fact that target_attach_no_wait is True on
this platform (meaning, once we have attached to the inferior, we do not
have to wait for an extra event, unlike on Linux).

This is what we observe, after attaching to any process. At first,
it seems like everything worked fine, since the process stops, and
we get the prompt back:

    (gdb) att 3156
    Attaching to program `C:\[...]\foo.exe', process 3156
    [New Thread 3156.0xcd8]
    [New Thread 3156.0xfe4]
    0x7770000d in ntdll!DbgBreakPoint () from C:\Windows\SysWOW64\ntdll.dll
    (gdb)

However, enter any commands at all, and GDB appears to be hanging.
For instance:

    (gdb) set lang ada
    [nothing happens]

What happens, in fact, is that despite appearances, GDB is not reading
from the prompt. It is waiting for an event from the inferior. And
since our inferior has stopped, there aren't going to be any event
to read.

In chronological order, what happens is that windows_attach calls
do_initial_windows_stuff, which performs the inferior creation,
and repeatedly waits until we get the first SIGTRAP:

  while (1)
    {
      stop_after_trap = 1;
      wait_for_inferior ();
      tp = inferior_thread ();
      if (tp->suspend.stop_signal != GDB_SIGNAL_TRAP)
        resume (tp->suspend.stop_signal);
      else
        break;
    }

The call to wait_for_inferior triggers a call to do_target_wait
to get the event, followed by handle_inferior_event to process it.
However, because the first couple of events are "spurious" events,
GDB resumes the execution, and prepares the inferior to wait again:

    case TARGET_WAITKIND_SPURIOUS:
      [...]
      resume (GDB_SIGNAL_0);
      prepare_to_wait (ecs);

And prepare_to_wait just does...

  ecs->wait_some_more = 1;
  if (!target_is_async_p ())
    mark_infrun_async_event_handler ();

... which as a result sets the infrun_async_event_handler "ready"
flag to 1.

We get a couple of spurious events before we get our first SIGTRAP,
at which point we exit the "while (1)" loop above, after which
we reach the end of the attach_command, followed by the normal
end-of-command processing (normal_stop, bp handling, printing the
GDB prompt), back finally to the root of the event loop.

Notice that, at this point, nothing has unset the "ready" flag
for the infrun_async_event_handler. So, when another cycle of
gdb_do_one_event from the event loop, we eventually call
check_async_event_handlers, which finds that the infrun async
event handler is "ready", and therefore calls it's associated
"proc" callback, which does...

      inferior_event_handler (INF_REG_EVENT, NULL);

... triggering a call to the target's to_wait method, thus hanging
forever.

Comparing what happens on GNU/Linux, the difference is that we do
expect events from the inferior after attaching. And fetching
those events cause the infrun async event handler's "ready" flag
to be unset before we return to the mainloop.

It seems to me that, for target such as Windows where there is
no need to wait for an event after the we've attached to the inferior,
the flag should be reset somewhere, and the the best place seemed
to be at the end of attach_command, when target_attach_no_wait is
true. That's the infrun_async (0) call.

Although, now that I think of it, shouldn't it we do it like in
the mainloop where we clear the "ready" flag before fetching
the inferior event?

Anyways, adding the "infrun_async (0)" did fix the problem, except
it only fixed it for the first attach. It turns out that the same
problem would resurface if you use the attach command a second time.
For instance:

    (gdb) attach 1234
    [all OK]
    (gdb) detach
    [so far, so good]
    (gdb) attach 1234
    [seems OK, and we get the prompt back, but...]
    (gdb)

... at this point we realize that GDB is no longer responsive.

This is because the "ready" flag is set via
mark_infrun_async_event_handler, which has no guard, so always
changes the "ready" flag. However, because there was no public
API for unsetting the infrun async event handler other than
infrun_async, I used that. But I missed the fact that the behavior
of that function is guarded by a global:

    /* Stores whether infrun_async was previously enabled or disabled.
       Starts off as -1, indicating "never enabled/disabled".  */
    static int infrun_is_async = -1;

    void
    infrun_async (int enable)
    {
      if (infrun_is_async != enable)
        {
          [...]
        }
    }

That global got set to zero at the end of the first attach.
But no one else changed the value of that global since then.
So, on the second attach, the added "infrun_async (0)" has
no effect, thus leading us to the same perpetual wait for
inferior events.

I couldn't figure out why we needed both infrun_async and
mark_infrun_async_event_handler, and in particular, I didn't
see the need for the infrun_is_async != enable guard.
So, this patch just makes infrun_async always call the
relevant {mark,clear}_infrun_async_event_handler routine.

If you agree that's the right thing to do, I propose we delete
{mark,clear}_infrun_async_event_handler, or replace them by
the associated infrun_async routine. Although, from someone
not very familiar with all this async stuff, the function
names {mark,clear}_infrun_async_event_handler speak a little
more to me. On the other hand, I like infrun_async because of
the debug trace. So we could also eliminate infrun_async and
move the debug trace into {mark,clear}_infrun_async_event_handler.
We'd have to make clear_infrun_async_event_handler non-static.

The attached patch is a prototype tested on x86-windows.
If you agree, I'll make a proper patch submission, after
having tested it on GNU/Linux as well.

WDYT?

Thanks!

-- 
Joel

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index fc27904..8fb133c 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2821,6 +2821,15 @@ attach_command (char *args, int from_tty)
 	mark_infrun_async_event_handler ();
       return;
     }
+  else
+    {
+      /* We don't expect any additional attach event from the target.
+	 So make sure that the infrun_async_event_handler is disabled.
+	 Otherwise, the main event loop might believe that we have
+	 inferior events ready, causing us to wait for those event
+	 that will never come, since our inferior is now stopped.  */
+      infrun_async (0);
+    }
 
   /* Done with ARGS.  */
   do_cleanups (args_chain);
diff --git a/gdb/infrun.c b/gdb/infrun.c
index cf91370..2c189f9 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -107,29 +107,20 @@ static int maybe_software_singlestep (struct gdbarch *gdbarch, CORE_ADDR pc);
    when we have pending events ready to be passed to the core.  */
 static struct async_event_handler *infrun_async_inferior_event_token;
 
-/* Stores whether infrun_async was previously enabled or disabled.
-   Starts off as -1, indicating "never enabled/disabled".  */
-static int infrun_is_async = -1;
-
 /* See infrun.h.  */
 
 void
 infrun_async (int enable)
 {
-  if (infrun_is_async != enable)
-    {
-      infrun_is_async = enable;
-
-      if (debug_infrun)
-	fprintf_unfiltered (gdb_stdlog,
-			    "infrun: infrun_async(%d)\n",
-			    enable);
+  if (debug_infrun)
+    fprintf_unfiltered (gdb_stdlog,
+			"infrun: infrun_async(%d)\n",
+			enable);
 
-      if (enable)
-	mark_async_event_handler (infrun_async_inferior_event_token);
-      else
-	clear_async_event_handler (infrun_async_inferior_event_token);
-    }
+  if (enable)
+    mark_async_event_handler (infrun_async_inferior_event_token);
+  else
+    clear_async_event_handler (infrun_async_inferior_event_token);
 }
 
 /* See infrun.h.  */