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

[Pedro Alves <palves at redhat dot com>]

Hi Joel,

I'm trying to build a Windows gdb to see this in
action.  Many thanks for the detailed analysis!

Thanks,
Pedro Alves

On 10/16/2015 01:35 AM, Joel Brobecker wrote:
> 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!
>