Re: [PATCH 02/18] Remote all-stop-on-top-of-non-stop

[Pedro Alves <palves@redhat.com>]

Hi Yao,

Thanks for the review, and sorry for the delay in getting
back to this...

On 10/23/2015 04:30 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> @@ -2636,8 +2633,37 @@ attach_command_post_wait (char *args, int from_tty, int async_exec)
>>    target_post_attach (ptid_get_pid (inferior_ptid));
>>  
>>    post_create_inferior (&current_target, from_tty);
>> +}
>> +
>> +/* What to do after the first program stops after attaching.  */
>> +enum attach_post_wait_mode
>> +{
>> +  /* Do nothing.  Leaves threads as they are.  */
>> +  ATTACH_POST_WAIT_NOTHING,
>> +
>> +  /* Re-resume threads that are marked running.  */
>> +  ATTACH_POST_WAIT_RESUME,
>> +
>> +  /* Stop all threads.  */
>> +  ATTACH_POST_WAIT_STOP,
>> +};
>> +
>> +/* Called after we've attached to a process and we've seen it stop for
>> +   the first time.  If ASYNC_EXEC is true, re-resume threads that
>> +   should be running.  Else if ATTACH, */
>> +
> 
> Comments are not complete.

Whoops, at some point I was going to add an 'attach' parameter, then I
converted it to the new enum.  Fixed.

> 
>> +
>> +  /* Now go over all threads that are stopped, and print their current
>> +     frame.  If all-stop, then if there's a signalled thread, pick
>> +     that as current.  */
> 
> Is it the code you described in the last paragraph of the commit log?

Yes.

> 
>> +  ALL_NON_EXITED_THREADS (thread)
>> +    {
>> +      struct target_waitstatus *ws;
>> +
>> +      if (first == NULL)
>> +	first = thread;
>> +
>> +      if (!non_stop)
>> +	set_running (thread->ptid, 0);
>> +      else if (thread->state != THREAD_STOPPED)
>> +	continue;
>> +
>> +      ws = &thread->suspend.waitstatus;
>> +
>> +      if (selected == NULL
>> +	  && thread->suspend.waitstatus_pending_p)
>> +	selected = thread;
>> +
>> +      if (lowest == NULL || thread->num < lowest->num)
>> +	lowest = thread;
>> +
>> +      if (non_stop)
>> +	print_one_stopped_thread (thread);
>> +    }
>> +
>> +  /* In all-stop, we only print the status of one thread, and leave
>> +     others with their status pending.  */
>> +  if (!non_stop)
>> +    {
>> +      thread = selected;
>> +      if (thread == NULL)
>> +	thread = lowest;
>> +      if (thread == NULL)
>> +	thread = first;
> 
> Looks lowest can't be NULL, so first isn't used.

"lowest" here actually means "lowest stopped".  It can thus be NULL if
all threads are running.  I've renamed it to 'lowest_stopped' now
to make that clearer.

> 
>> +
>> +      print_one_stopped_thread (thread);
>> +    }
>> +
>> +  /* For "info program".  */
>> +  thread = inferior_thread ();
>> +  if (thread->state == THREAD_STOPPED)
>> +    set_last_target_status (inferior_ptid, thread->suspend.waitstatus);
>>  }
>>  
>>  static void
>> @@ -3826,7 +3942,7 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
> 
>> @@ -12936,11 +13040,15 @@ remote_async (struct target_ops *ops, int enable)
>>  	 event loop to process them.  */
>>        if (!QUEUE_is_empty (stop_reply_p, stop_reply_queue))
>>  	mark_async_event_handler (remote_async_inferior_event_token);
>> +      if (target_is_non_stop_p ())
>> +	mark_async_event_handler (rs->notif_state->get_pending_events_token);
> 
> I don't understand why do we need to mark the pending event token.
> Previously, we only need to do so when GDB sees a notification.

Thanks for pointing this out.  I should have added some comments here,
as it took me a while to re-understand it.  :-)

To explain why I added this, I need to start with the "else" branch ... 

> 
>>      }
>>    else
>>      {
>>        serial_async (rs->remote_desc, NULL, NULL);
>>        clear_async_event_handler (remote_async_inferior_event_token);
>> +      if (target_is_non_stop_p ())
>> +	clear_async_event_handler (rs->notif_state->get_pending_events_token);

... here.

Currently, it's possible that target_async(0) is called while the
pending event token is marked.  If we clear the pending event token when
disabling async, we need to mark it again when re-enabling async.  To simplify
(and avoid keeping track of whether the token was marked), I just always
marked the token when enabling async.  It just results in at most one
harmless spurious event-loop wake up.

That explains why we now need to mark the pending event token
while previously we didn't, but not why do we need to _clear_ the
pending event token while previously we didn't.  To understand
that, it helps to identify the target_async(0) call in question.  That's
called after hitting a breakpoint and reporting an event to the
user (in inf-loop.c).

    case INF_EXEC_COMPLETE:
      if (!non_stop)
	{
	  /* Unregister the inferior from the event loop.  This is done
	     so that when the inferior is not running we don't get
	     distracted by spurious inferior output.  */
	  if (target_has_execution && target_can_async_p ())
	    target_async (0);
	}

Thus if we don't clear the get_pending_events_token when disabling async,
target_wait ends up called even after target_async is disabled.  That
would normally be harmless, but annoying as it breaks testing
with "set debug infrun 1", because you end up with a spurious
TARGET_WAITKIND_IGNORE event after gdb has already printed the prompt,
which breaks the testsuite.

But in this case, it's actually worse.  It results in the event loop being
_continuously_ woken (#4 - #8):

 #1 - the event loop wakes up for get_pending_events_token, which results in
 #2 - remote_notif_stop_can_get_pending_events -> mark remote_async_inferior_event_token.
 #3 - event loop wakes up for remote_async_inferior_event_token
 #4 - fetch_inferior_event -> target_wait -> remote_wait -> TARGET_WAITKIND_IGNORE
 #5 - handle_inferior_event -> prepare_to_wait
 #6 - because async is off, prepare_to_wait calls mark_infrun_async_event_handler.
 #7 - event loop wakes up for infrun_async_inferior_event_token -> fetch_inferior_event.
 #8 - goto #4.

E.g., just running to main with logging enabled shows:

...
notif: discard queued event: 'Stop' in Thread 0
infrun: target_wait (-1.0.0, status) =
infrun:   -1.0.0 [Thread 0],
infrun:   status->kind = ignore
infrun: TARGET_WAITKIND_IGNORE
infrun: prepare_to_wait
notif: discard queued event: 'Stop' in Thread 0
infrun: target_wait (-1.0.0, status) =
infrun:   -1.0.0 [Thread 0],
infrun:   status->kind = ignore
infrun: TARGET_WAITKIND_IGNORE
infrun: prepare_to_wait
notif: discard queued event: 'Stop' in Thread 0
infrun: target_wait (-1.0.0, status) =
infrun:   -1.0.0 [Thread 0],
infrun:   status->kind = ignore
infrun: TARGET_WAITKIND_IGNORE
infrun: prepare_to_wait
notif: discard queued event: 'Stop' in Thread 0
infrun: target_wait (-1.0.0, status) =
infrun:   -1.0.0 [Thread 0],
infrun:   status->kind = ignore
infrun: TARGET_WAITKIND_IGNORE
infrun: prepare_to_wait
notif: discard queued event: 'Stop' in Thread 0
infrun: target_wait (-1.0.0, status) =
infrun:   -1.0.0 [Thread 0],
infrun:   status->kind = ignore
infrun: TARGET_WAITKIND_IGNORE
infrun: prepare_to_wait
notif: discard queued event: 'Stop' in Thread 0
infrun: target_wait (-1.0.0, status) =
infrun:   -1.0.0 [Thread 0],
infrun:   status->kind = ignore
infrun: TARGET_WAITKIND_IGNORE
infrun: prepare_to_wait
notif: discard queued event: 'Stop' in Thread 0
...

Forever and ever.  The testsuite actually regresses due to this, as some
tests enabling infrun logging (e.g., breakpoint-in-ro-region.exp).

The patch below (applies on top of the series) fixes this by making it so
that the pending event token is already clear when we get to the
target_async(0) call in question.  The testsuite does pass cleanly with
this patch and without the patch, but regresses if I apply only the
remote_async hunk (that is, if I revert the changes in the patch #2
under discussion).

The remote_notif_get_pending_events hunk below feels like a good
improvement to me, as it gets rid of unnecessary event loop wake ups.
However, I still think that we need the
clear_async_event_handler / mark_async_event_handler calls in
remote_async like I had them before.

That's because we have other target_async(0) calls that unlike the
inf-loop.c call happen while the target _is_ still running.  In particular,
I'm thinking of the one in top.c:gdb_readline_wrapper, when we're about to
show a secondary prompt and nest a secondary event loop that should not
get woken by target events.

So I'm thinking that I should leave the original patch as is, and
add some extra comments to remote_async.  WDYT?

From 2a73e1b66417edce50d97e566865f2273a988482 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 19 Nov 2015 18:40:13 +0000
Subject: [PATCH] clear pending events token before acking

---
 gdb/remote.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 9e69bfb..04057cc 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6640,6 +6640,10 @@ remote_notif_get_pending_events (struct notif_client *nc)
 			    "notif: process: '%s' ack pending event\n",
 			    nc->name);
 
+      /* As we're acking the pending notification, we no longer need
+	 to wake the event loop to do it.  */
+      clear_async_event_handler (rs->notif_state->get_pending_events_token);
+
       /* acknowledge */
       nc->ack (nc, rs->buf, rs->notif_state->pending_event[nc->id]);
       rs->notif_state->pending_event[nc->id] = NULL;
@@ -13158,15 +13162,11 @@ remote_async (struct target_ops *ops, int enable)
 	 event loop to process them.  */
       if (!QUEUE_is_empty (stop_reply_p, stop_reply_queue))
 	mark_async_event_handler (remote_async_inferior_event_token);
-      if (target_is_non_stop_p ())
-	mark_async_event_handler (rs->notif_state->get_pending_events_token);
     }
   else
     {
       serial_async (rs->remote_desc, NULL, NULL);
       clear_async_event_handler (remote_async_inferior_event_token);
-      if (target_is_non_stop_p ())
-	clear_async_event_handler (rs->notif_state->get_pending_events_token);
     }
 }
 
-- 
1.9.3