Re: [PATCH v2 2/4] Unregister the last inferior from the event loop

["Maciej W. Rozycki" <macro@wdc.com>]

On Tue, 21 Jan 2020, Simon Marchi wrote:

> >  I have therefore gone reading through the code again and what I can see 
> > is that in the async mode the `ready' (readiness) flag is never cleared: 
> > see `infrun_async' and `remote_target::async', which are the only places 
> > to call `clear_async_event_handler' and then only when the async mode is 
> > being disabled.
> 
> The ready flag is also cleared in `check_async_event_handlers`, just before
> invoking the callback.  So I was thinking, why would the callback get called
> repeatedly if the ready flag is cleared just before it gets called, and
> there's nothing setting it back to true.  The answer is probably that the
> busy loop is within that callback, as seen below?

 Indeed it was cleared in `check_async_event_handlers' and then set again 
via `mark_infrun_async_event_handler' in `prepare_to_wait' called from 
`handle_inferior_event'.

> >  On the other hand waiting for an inferior event does get disabled in 
> > `handle_inferior_event' regardless of the readiness flag, by calling 
> > `stop_waiting', for certain events, but not for TARGET_WAITKIND_IGNORE.  
> > Instead for that event `prepare_to_wait' is called, which makes sense to 
> > me because such an event does not indicate whether waiting should or 
> > should not be disabled, and with an asynchronous target you can normally 
> > (i.e. if not indicated by a specific event received otherwise, e.g. 
> > TARGET_WAITKIND_EXITED) expect a further event to be received anytime.
> > 
> >  Does this clarify the problematic scenario to you?
> 
> Ok, so if I understand, the infinite loop is this one, inside 
> wait_for_inferior?
> 
>   while (1)
>     {
>       ...
> 
>       /* Now figure out what to do with the result of the result.  */
>       handle_inferior_event (ecs);
> 
>       if (!ecs->wait_some_more)
> 	break;
>     }

 Nope, it was in `start_event_loop':

  while (1)
    {
      ...
	  result = gdb_do_one_event ();
      ...
      if (result < 0)
	break;
    }

calling `check_async_event_handlers', 
`infrun_async_inferior_event_handler', `inferior_event_handler' and 
ultimately `fetch_inferior_event':

    /* Now figure out what to do with the result of the result.  */
    handle_inferior_event (ecs);

    if (!ecs->wait_some_more)
      ...

> After the remote target has been unpushed, the remaining target is probably
> just the "exec file" target, which does not provide a ::wait implementation,
> and therefore inherits default_target_wait:
> 
> ptid_t
> default_target_wait (struct target_ops *ops,
> 		     ptid_t ptid, struct target_waitstatus *status,
> 		     int options)
> {
>   status->kind = TARGET_WAITKIND_IGNORE;
>   return minus_one_ptid;
> }

 And it was `dummy_target::wait' indeed invoking `default_target_wait'.

> And because that returns TARGET_WAITKIND_IGNORE, which results in
> ecs->wait_some_more getting set by handle_inferior_event/prepare_to_wait,
> it results in the infinite loop in wait_for_inferior.

 Right.

> Does that look accurate?

 Almost, except that code in question has since been heavily refactored 
and the call to `target_wait' does not happen anymore.  Now I need to 
bisect the tree, find the offending commit and figure out whether it has 
actually provided an alternative fix for the issue I have observed or just 
papered it over by chance somehow.

 Thanks for your input, I wish it happened earlier, before the code has 
been rearranged, sigh...

  Maciej