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

[Simon Marchi <simark@simark.ca>]

On 2020-01-21 3:29 a.m., Maciej W. Rozycki wrote:
> Hi Simon,
> 
>>> This is because `remote_target::resume' enables the asynchronous event 
>>> loop, as indicated by the first `infrun: infrun_async(1)' record above, 
>>> and then the confirmation dialogue temporarily disables it and then 
>>> reenables, as indicated by the second `infrun: infrun_async(1)' record 
>>> above.  The problem with that approach is that the reenabling also marks 
>>> the handler for the `infrun_async_inferior_event_token' event ready, 
>>> even though it was not before the temporary disabling, by calling 
>>> `mark_async_event_handler' on it, and that triggers the infinite loop as 
>>> there's no inferior anymore and consequently no event waiting that would 
>>> stop it.
>>
>> I don't understand this description.  Let's assume that the second call to
>> infrun_async indeed left infrun_async_inferior_event_token.ready to true.
>> Then I would expect the event loop to call it once, setting the ready flag
>> to false.  After that, the ready being false, I don't see why the callback
>> of infrun_async_inferior_event_token would get called in a loop.
> 
>  Thanks for looking into it.  It's been a while however, so my memory has 
> become fuzzy on this.
> 
>  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?

>  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;
    }

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 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.

Does that look accurate?

Simon