[PATCH 0/4] Improve 'maint set target-async off' for remote targets

[PATCH 0/4] Improve 'maint set target-async off' for remote targets

[Andrew Burgess]

While working on another patch that involved extended-remote targets
and attaching I tried testing with 'maint set target-non-stop off' and
'maint set target-async off' and ran into some problems.

This series is my attempt to improve the extended-remote's support for
'maint set target-async off'.

Thanks,
Andrew

---

Andrew Burgess (4):
  gdb: add asserts in target.c for target_async_permitted
  gdb/remote: merge ::is_async_p and ::can_async_p methods
  gdb: add assert in remote_target::wait relating to async being off
  gdb/remote: some fixes for 'maint set target-async off'

 gdb/remote.c | 206 ++++++++++++++++++++++-----------------------------
 gdb/target.c |  11 ++-
 2 files changed, 96 insertions(+), 121 deletions(-)

-- 
2.25.4

[PATCH 1/4] gdb: add asserts in target.c for target_async_permitted

[Andrew Burgess]

The target_async_permitted flag allows a user to override whether a
target can act in async mode or not.  Ideally we would check this flag
in target_can_async_p and target_is_async_p, however, there are a few
places in GDB where we call the can_async_p and is_async_p methods
directly without calling through the target_* wrapper.

As a result each target must check the target_async_permitted flag in
their own method implementations.

However, as the target_can_async_p and target_is_async_p functions are
also called in some places, what we can do is add an assert in these
functions that ensures targets are checking target_async_permitted
correctly.

So, the rule is: in target_can_async_p and target_is_async_p, if the
global target_async_permitted is false, then the result of calling the
target_ops method (::can_async_p or ::is_async_p) should also be
false.

Right now, I believe that only the linux_nat_target and the
remote_target support async mode, and these targets do, correctly
check target_async_permitted.  But if, in future, additional targets
add support for async mode, then these asserts should ensure that the
targets are checking target_async_permitted correctly.

Further, the target_async method is used to enable or disable async
mode for a target.  My claim here is that we should only try to enable
async mode for targets if target_async_permitted is true, and if the
target supports async mode.  I've added an assertion to this effect in
the target_async function.

There should be no user visible changes after this commit.
---
 gdb/target.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/gdb/target.c b/gdb/target.c
index 8fe27c775ea..f8e56cceb08 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -391,7 +391,9 @@ target_can_lock_scheduler ()
 bool
 target_can_async_p ()
 {
-  return current_inferior ()->top_target ()->can_async_p ();
+  bool result = current_inferior ()->top_target ()->can_async_p ();
+  gdb_assert (target_async_permitted || !result);
+  return result;
 }
 
 /* See target.h.  */
@@ -399,7 +401,9 @@ target_can_async_p ()
 bool
 target_is_async_p ()
 {
-  return current_inferior ()->top_target ()->is_async_p ();
+  bool result = current_inferior ()->top_target ()->is_async_p ();
+  gdb_assert (target_async_permitted || !result);
+  return result;
 }
 
 exec_direction_kind
@@ -4328,6 +4332,9 @@ maintenance_print_target_stack (const char *cmd, int from_tty)
 void
 target_async (int enable)
 {
+  /* If we are trying to enable async mode then it must be the case that
+     async mode is both allowed, and the target supports async mode.  */
+  gdb_assert (!enable || (target_async_permitted && target_can_async_p ()));
   infrun_async (enable);
   current_inferior ()->top_target ()->async (enable);
 }
-- 
2.25.4

[PATCH 2/4] gdb/remote: merge ::is_async_p and ::can_async_p methods

[Andrew Burgess]

I spotted in passing that remote_target::is_async_p and
remote_target::can_async_p are identical.  This commit just makes
::is_async_p call ::can_async_p, removing some duplicate code.

There should be no user visible changes after this commit.
---
 gdb/remote.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 61bde5aaa94..0c4cc6bad0b 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -546,7 +546,10 @@ class remote_target : public process_stratum_target
 
   bool can_async_p () override;
 
-  bool is_async_p () override;
+  bool is_async_p () override
+  {
+    return can_async_p ();
+  }
 
   void async (int) override;
 
@@ -14390,19 +14393,6 @@ remote_target::can_async_p ()
   return serial_can_async_p (rs->remote_desc);
 }
 
-bool
-remote_target::is_async_p ()
-{
-  struct remote_state *rs = get_remote_state ();
-
-  if (!target_async_permitted)
-    /* We only enable async when the user specifically asks for it.  */
-    return false;
-
-  /* We're async whenever the serial device is.  */
-  return serial_is_async_p (rs->remote_desc);
-}
-
 /* Pass the SERIAL event on and up to the client.  One day this code
    will be able to delay notifying the client of an event until the
    point where an entire packet has been received.  */
-- 
2.25.4

[PATCH 3/4] gdb: add assert in remote_target::wait relating to async being off

[Andrew Burgess]

While working on another patch I ended up in a situation where I had
async mode disabled (with 'maint set target-async off'), but the async
event token got marked anyway.

In this situation GDB was continually calling into
remote_target::wait, however, the async token would never become
unmarked as the unmarking is guarded by target_is_async_p.

We could just unconditionally unmark the token, but that would feel
like just ignoring a bug, so, instead, lets assert that if
!target_is_async_p, then the async token should not be marked.

This assertion would have caught my earlier mistake.

There should be no user visible changes with this commit.
---
 gdb/remote.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 0c4cc6bad0b..64c4cde436b 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -8312,9 +8312,13 @@ remote_target::wait (ptid_t ptid, struct target_waitstatus *status,
   remote_state *rs = get_remote_state ();
 
   /* Start by clearing the flag that asks for our wait method to be called,
-     we'll mark it again at the end if needed.  */
+     we'll mark it again at the end if needed.  If the target is not in
+     async mode then the async token should not be marked.  */
   if (target_is_async_p ())
     clear_async_event_handler (rs->remote_async_inferior_event_token);
+  else
+    gdb_assert (!async_event_handler_marked
+		(rs->remote_async_inferior_event_token));
 
   ptid_t event_ptid;
 
-- 
2.25.4

[PATCH 4/4] gdb/remote: some fixes for 'maint set target-async off'

[Andrew Burgess]

While working on another patch relating to remote targets, I wanted to
test with 'maint set target-async off' in place.  Unfortunately I ran
into some problems.  This commit is an attempt to fix one of the
issues I hit.

In my particular case I was actually running with:

  maint set target-async off
  maint set target-non-stop off

that is, we're telling GDB to force the targets to operate in
non-async mode, and in all-stop mode.  Here's my GDB session showing
the problem:

  (gdb) maintenance set target-async off
  (gdb) maintenance set target-non-stop off
  (gdb) target extended-remote :54321
  Remote debugging using :54321
  (gdb) attach 2365960
  Attaching to process 2365960
  No unwaited-for children left.
  (gdb)

Notice the 'No unwaited-for children left.' error, this is the
problem.  There's no reason why GDB should not be able to attach to
the process.

The problem is this:

  1. The user runs 'attach PID' and this sends GDB into attach_command
  in infcmd.c.  From here we call the ::attach method on the attach
  target, which will be the extended_remote_target.

  2. In extended_remote_target::attach, we attach to the remote target
  and get the first reply (which is a stop packet).  We put off
  processing the stop packet until the end of ::attach.  We setup the
  inferior and thread to represent the process we attached to, and
  download the target description.  Finally, we process the initial
  stop packet.

  If '!target_is_non_stop_p ()' and '!target_can_async_p ()', which is
  the case for us given the maintenance commands we used, we cache the
  stop packet within the remote_state::buf for later processing.

  3. Back in attach_command, if 'target_is_non_stop_p ()' then we
  request that the target stops.  This will either process any cached
  stop replies, or request that the target stops, and process the stop
  replies.  However, this code is not what we use due to non-stop mode
  being disabled.  So, we skip to the next step which is to call
  validate_exec_file.

  4. Calling validate_exec_file can cause packets to be sent to the
  remote target, and replies received, the first path I hit is the
  call to target_pid_to_exec_file, which calls
  remote_target::pid_to_exec_file, which can then try to read the
  executable from the remote.  Sending an receiving packets will make
  use of the remote_state::buf object.

  5. The attempt to attach continues, but the damage is already done...

So, the problem is that, in step #2 we cache a stop reply in the
remote_state::buf, and then in step #4 we reuse the remote_state::buf
object, discarding any cached stop reply.  As a result, the initial
stop, which is sent when GDB first attaches to the target, is lost.

This problem can clearly be seen I feel by looking at the
remote_state::cached_wait_status flag.  This flag tells GDB if there
is a wait status cached in remote_state::buf.  However, in
remote_target::putpkt_binary and remote_target::getpkt_or_notif_sane_1
this flag is just set back to 0, doing this immediately discards any
cached data.

I don't know if this scheme ever made sense, maybe once upon a time,
GDB would, when it found it had no cached stop, simply re-request the
stop information from the target, however, this certainly isn't the
case now, and resetting the cached_wait_status is (I claim) a bad
thing.

So, my first step toward fixing this issue was to replace the two
instances of 'rs->cached_wait_status = 0;' in ::putpkt_binary and
::getpkt_or_notif_sane_1 with 'gdb_assert (rs->cached_wait_status ==
0);', this, at least would show me when GDB was doing something
dangerous, and indeed, this assert is now hit in my test case above.

I did play with using some kind of scoped restore to backup, and
restore the remote_state::buf object in all the places within remote.c
that I was hitting where the ::buf was being corrupted.  The first
problem with this is that, where the ::cached_wait_status flag is
reset is _not_ where ::buf is corrupted.  For the ::putpkt_binary
case, by the time we get to the method the buffer has already been
corrupted in many cases, so we end up needing to add the scoped
save/restore within the callers, which means we need the save/restore
in _lots_ of places.

Plus, using this save/restore model feels like the wrong solution.  I
don't think that it's obvious that the buffer might be holding cached
data, and I think it would be too easy for new corruptions of the
buffer to be introduced, which could easily go unnoticed for a long
time.

So, I really wanted a solution that didn't require us to cache data in
the ::buf object.

Luckily, I think we already have such a solution in place, the
remote_state::stop_reply_queue, it seems like this does exactly the
same task, just in a slightly different way.  With the
::stop_reply_queue, the stop packets are processed upon receipt and
the stop_reply object is added to the queue.  With the ::buf cache
solution, the unprocessed stop reply is cached in the ::buf, and
processed later.

So, finally, in this commit, I propose to remove the
remote_state::cached_wait_status flag and to stop using the ::buf to
cache stop replies.  Instead, stop replies will now always be stored
in the ::stop_reply_queue.

There are two places where we use the ::buf to hold a cached stop
reply, the first is in the ::attach method, and the second is in
remote_target::start_remote, however, the second of these cases is far
less problematic, as after caching the stop reply in ::buf we call the
global start_remote function, which does very little work before
calling normal_stop, which processes the cached stop reply.  However,
my plan is to switch both users over to using ::stop_reply_queue so
that the old (unsafe) ::cached_wait_status mechanism can be completely
removed.

The next problem is that the ::stop_reply_queue is currently only used
for async-mode, and so, in remote_target::push_stop_reply, where we
push stop_reply objects into the ::stop_reply_queue, we currently also
mark the async event token.  I've modified this so we only mark the
async event token if 'target_can_async_p ()' - note, _can_, not _is_
here, ::push_stop_reply is called in places where async mode has been
temporarily disabled, but, in general, the target can (and will) be
switched back into async mode.

Another change of interest is in remote_target::remote_interrupt_as.
Previously this code checked ::cached_wait_status, but didn't check
for events in the ::stop_reply_queue.  Now that ::cached_wait_status
has been removed we now check the queue length instead, which should
have the same result.

Finally, in remote_target::wait_as, I've tried to merge the processing
of the ::stop_reply_queue with how we used to handle the
::cached_wait_status flag.

Currently, when processing the ::stop_reply_queue we call
process_stop_reply and immediately return.  However, when handling
::cached_wait_status we run through the whole of ::wait_as, and return
at the end of the function.

If we consider a standard stop packet, the two differences I see are:

  1. Resetting of the remote_state::waiting_for_stop_reply, flag; this
  is not currently done when processing a stop from the
  ::stop_reply_queue.

  2. The final return value has the possibility of being adjusted at
  the end of ::wait_as, as well as there being calls to
  record_currthread, non of which are done if we process a stop from
  the ::stop_reply_queue.

After this commit I have unified these two paths, that is, when we
process a packet from ::stop_reply_queue we now reset the flag
mentioned in #1 above, and pass through the return value adjustment
code at the end of ::wait_as.

An example of a test that reveals the benefits of this commit is:

  make check-gdb \
    RUNTESTFLAGS="--target_board=native-extended-gdbserver \
                  GDBFLAGS='-ex maint\ set\ target-async\ off \
                            -ex maint\ set\ target-non-stop\ off' \
                  gdb.base/attach.exp"

For testing I've been running test on x86-64/GNU Linux, and run with
target boards unix, native-gdbserver, and native-extended-gdbserver.
For each board I've run with the default GDBFLAGS, as well as with:

  GDBFLAGS='-ex maint\ set\ target-async\ off \
            -ex maint\ set\ target-non-stop\ off' \

Though running with the above GDBFLAGS is clearly a lot more unstable
both before and after my patch, I'm not seeing any consistent new
failures with my patch, except, with the native-extended-gdbserver
board, where I am seeing new failures, but only because more tests are
now running.  For that configuration alone I see the number of
unresolved go down by 49, the number of passes goes up by 446, and the
number of failures also increases by 144.  All of the failures are new
tests as far as I can tell.
---
 gdb/remote.c | 182 ++++++++++++++++++++++-----------------------------
 1 file changed, 78 insertions(+), 104 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 64c4cde436b..b6472e0091c 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -258,15 +258,6 @@ class remote_state
      Otherwise zero, meaning to use the guessed size.  */
   long explicit_packet_size = 0;
 
-  /* remote_wait is normally called when the target is running and
-     waits for a stop reply packet.  But sometimes we need to call it
-     when the target is already stopped.  We can send a "?" packet
-     and have remote_wait read the response.  Or, if we already have
-     the response, we can stash it in BUF and tell remote_wait to
-     skip calling getpkt.  This flag is set when BUF contains a
-     stop reply packet and the target is not waiting.  */
-  int cached_wait_status = 0;
-
   /* True, if in no ack mode.  That is, neither GDB nor the stub will
      expect acks from each other.  The connection is assumed to be
      reliable.  */
@@ -4904,8 +4895,9 @@ remote_target::start_remote (int from_tty, int extended_p)
 
       /* Use the previously fetched status.  */
       gdb_assert (wait_status != NULL);
-      strcpy (rs->buf.data (), wait_status);
-      rs->cached_wait_status = 1;
+      struct notif_event *reply
+	= remote_notif_parse (this, &notif_client_stop, wait_status);
+      push_stop_reply ((struct stop_reply *) reply);
 
       ::start_remote (from_tty); /* Initialize gdb process mechanisms.  */
     }
@@ -5741,7 +5733,6 @@ remote_target::open_1 (const char *name, int from_tty, int extended_p)
   /* Reset the target state; these things will be queried either by
      remote_query_supported or as they are needed.  */
   reset_all_packet_configs_support ();
-  rs->cached_wait_status = 0;
   rs->explicit_packet_size = 0;
   rs->noack_mode = 0;
   rs->extended = extended_p;
@@ -6084,21 +6075,13 @@ extended_remote_target::attach (const char *args, int from_tty)
       /* Use the previously fetched status.  */
       gdb_assert (wait_status != NULL);
 
-      if (target_can_async_p ())
-	{
-	  struct notif_event *reply
-	    =  remote_notif_parse (this, &notif_client_stop, wait_status);
+      struct notif_event *reply
+	=  remote_notif_parse (this, &notif_client_stop, wait_status);
 
-	  push_stop_reply ((struct stop_reply *) reply);
+      push_stop_reply ((struct stop_reply *) reply);
 
-	  target_async (1);
-	}
-      else
-	{
-	  gdb_assert (wait_status != NULL);
-	  strcpy (rs->buf.data (), wait_status);
-	  rs->cached_wait_status = 1;
-	}
+      if (target_can_async_p ())
+	target_async (1);
     }
   else
     {
@@ -7001,9 +6984,9 @@ remote_target::remote_interrupt_as ()
   rs->ctrlc_pending_p = 1;
 
   /* If the inferior is stopped already, but the core didn't know
-     about it yet, just ignore the request.  The cached wait status
+     about it yet, just ignore the request.  The pending stop events
      will be collected in remote_wait.  */
-  if (rs->cached_wait_status)
+  if (stop_reply_queue_length () > 0)
     return;
 
   /* Send interrupt_sequence to remote target.  */
@@ -7432,7 +7415,7 @@ remote_target::queued_stop_reply (ptid_t ptid)
   remote_state *rs = get_remote_state ();
   struct stop_reply *r = remote_notif_remove_queued_reply (ptid);
 
-  if (!rs->stop_reply_queue.empty ())
+  if (!rs->stop_reply_queue.empty () && target_can_async_p ())
     {
       /* There's still at least an event left.  */
       mark_async_event_handler (rs->remote_async_inferior_event_token);
@@ -7457,7 +7440,8 @@ remote_target::push_stop_reply (struct stop_reply *new_event)
 			target_pid_to_str (new_event->ptid).c_str (),
 			int (rs->stop_reply_queue.size ()));
 
-  mark_async_event_handler (rs->remote_async_inferior_event_token);
+  if (target_can_async_p ())
+    mark_async_event_handler (rs->remote_async_inferior_event_token);
 }
 
 /* Returns true if we have a stop reply for PTID.  */
@@ -8166,15 +8150,12 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status,
 
   stop_reply = queued_stop_reply (ptid);
   if (stop_reply != NULL)
-    return process_stop_reply (stop_reply, status);
-
-  if (rs->cached_wait_status)
-    /* Use the cached wait status, but only once.  */
-    rs->cached_wait_status = 0;
+    {
+      rs->waiting_for_stop_reply = 0;
+      event_ptid = process_stop_reply (stop_reply, status);
+    }
   else
     {
-      int ret;
-      int is_notif;
       int forever = ((options & TARGET_WNOHANG) == 0
 		     && rs->wait_forever_enabled_p);
 
@@ -8188,7 +8169,8 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status,
 	 _never_ wait for ever -> test on target_is_async_p().
 	 However, before we do that we need to ensure that the caller
 	 knows how to take the target into/out of async mode.  */
-      ret = getpkt_or_notif_sane (&rs->buf, forever, &is_notif);
+      int is_notif;
+      int ret = getpkt_or_notif_sane (&rs->buf, forever, &is_notif);
 
       /* GDB gets a notification.  Return to core as this event is
 	 not interesting.  */
@@ -8197,73 +8179,73 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status,
 
       if (ret == -1 && (options & TARGET_WNOHANG) != 0)
 	return minus_one_ptid;
-    }
 
-  buf = rs->buf.data ();
+      buf = rs->buf.data ();
 
-  /* Assume that the target has acknowledged Ctrl-C unless we receive
-     an 'F' or 'O' packet.  */
-  if (buf[0] != 'F' && buf[0] != 'O')
-    rs->ctrlc_pending_p = 0;
+      /* Assume that the target has acknowledged Ctrl-C unless we receive
+	 an 'F' or 'O' packet.  */
+      if (buf[0] != 'F' && buf[0] != 'O')
+	rs->ctrlc_pending_p = 0;
 
-  switch (buf[0])
-    {
-    case 'E':		/* Error of some sort.	*/
-      /* We're out of sync with the target now.  Did it continue or
-	 not?  Not is more likely, so report a stop.  */
-      rs->waiting_for_stop_reply = 0;
+      switch (buf[0])
+	{
+	case 'E':		/* Error of some sort.	*/
+	  /* We're out of sync with the target now.  Did it continue or
+	     not?  Not is more likely, so report a stop.  */
+	  rs->waiting_for_stop_reply = 0;
 
-      warning (_("Remote failure reply: %s"), buf);
-      status->set_stopped (GDB_SIGNAL_0);
-      break;
-    case 'F':		/* File-I/O request.  */
-      /* GDB may access the inferior memory while handling the File-I/O
-	 request, but we don't want GDB accessing memory while waiting
-	 for a stop reply.  See the comments in putpkt_binary.  Set
-	 waiting_for_stop_reply to 0 temporarily.  */
-      rs->waiting_for_stop_reply = 0;
-      remote_fileio_request (this, buf, rs->ctrlc_pending_p);
-      rs->ctrlc_pending_p = 0;
-      /* GDB handled the File-I/O request, and the target is running
-	 again.  Keep waiting for events.  */
-      rs->waiting_for_stop_reply = 1;
-      break;
-    case 'N': case 'T': case 'S': case 'X': case 'W':
-      {
-	/* There is a stop reply to handle.  */
-	rs->waiting_for_stop_reply = 0;
+	  warning (_("Remote failure reply: %s"), buf);
+	  status->set_stopped (GDB_SIGNAL_0);
+	  break;
+	case 'F':		/* File-I/O request.  */
+	  /* GDB may access the inferior memory while handling the File-I/O
+	     request, but we don't want GDB accessing memory while waiting
+	     for a stop reply.  See the comments in putpkt_binary.  Set
+	     waiting_for_stop_reply to 0 temporarily.  */
+	  rs->waiting_for_stop_reply = 0;
+	  remote_fileio_request (this, buf, rs->ctrlc_pending_p);
+	  rs->ctrlc_pending_p = 0;
+	  /* GDB handled the File-I/O request, and the target is running
+	     again.  Keep waiting for events.  */
+	  rs->waiting_for_stop_reply = 1;
+	  break;
+	case 'N': case 'T': case 'S': case 'X': case 'W':
+	  {
+	    /* There is a stop reply to handle.  */
+	    rs->waiting_for_stop_reply = 0;
 
-	stop_reply
-	  = (struct stop_reply *) remote_notif_parse (this,
-						      &notif_client_stop,
-						      rs->buf.data ());
+	    stop_reply
+	      = (struct stop_reply *) remote_notif_parse (this,
+							  &notif_client_stop,
+							  rs->buf.data ());
 
-	event_ptid = process_stop_reply (stop_reply, status);
-	break;
-      }
-    case 'O':		/* Console output.  */
-      remote_console_output (buf + 1);
-      break;
-    case '\0':
-      if (rs->last_sent_signal != GDB_SIGNAL_0)
-	{
-	  /* Zero length reply means that we tried 'S' or 'C' and the
-	     remote system doesn't support it.  */
-	  target_terminal::ours_for_output ();
-	  printf_filtered
-	    ("Can't send signals to this remote system.  %s not sent.\n",
-	     gdb_signal_to_name (rs->last_sent_signal));
-	  rs->last_sent_signal = GDB_SIGNAL_0;
-	  target_terminal::inferior ();
-
-	  strcpy (buf, rs->last_sent_step ? "s" : "c");
-	  putpkt (buf);
+	    event_ptid = process_stop_reply (stop_reply, status);
+	    break;
+	  }
+	case 'O':		/* Console output.  */
+	  remote_console_output (buf + 1);
+	  break;
+	case '\0':
+	  if (rs->last_sent_signal != GDB_SIGNAL_0)
+	    {
+	      /* Zero length reply means that we tried 'S' or 'C' and the
+		 remote system doesn't support it.  */
+	      target_terminal::ours_for_output ();
+	      printf_filtered
+		("Can't send signals to this remote system.  %s not sent.\n",
+		 gdb_signal_to_name (rs->last_sent_signal));
+	      rs->last_sent_signal = GDB_SIGNAL_0;
+	      target_terminal::inferior ();
+
+	      strcpy (buf, rs->last_sent_step ? "s" : "c");
+	      putpkt (buf);
+	      break;
+	    }
+	  /* fallthrough */
+	default:
+	  warning (_("Invalid remote reply: %s"), buf);
 	  break;
 	}
-      /* fallthrough */
-    default:
-      warning (_("Invalid remote reply: %s"), buf);
-      break;
     }
 
   if (status->kind () == TARGET_WAITKIND_NO_RESUMED)
@@ -9563,10 +9545,6 @@ remote_target::putpkt_binary (const char *buf, int cnt)
 	       "and then try again."));
     }
 
-  /* We're sending out a new packet.  Make sure we don't look at a
-     stale cached response.  */
-  rs->cached_wait_status = 0;
-
   /* Copy the packet into buffer BUF2, encapsulating it
      and giving it a checksum.  */
 
@@ -9904,10 +9882,6 @@ remote_target::getpkt_or_notif_sane_1 (gdb::char_vector *buf,
   int timeout;
   int val = -1;
 
-  /* We're reading a new response.  Make sure we don't look at a
-     previously cached response.  */
-  rs->cached_wait_status = 0;
-
   strcpy (buf->data (), "timeout");
 
   if (forever)
-- 
2.25.4

Re: [PATCH 0/4] Improve 'maint set target-async off' for remote targets

[John Baldwin]

On 11/23/21 6:08 AM, Andrew Burgess via Gdb-patches wrote:
> While working on another patch that involved extended-remote targets
> and attaching I tried testing with 'maint set target-non-stop off' and
> 'maint set target-async off' and ran into some problems.
> 
> This series is my attempt to improve the extended-remote's support for
> 'maint set target-async off'.

I think this looks ok.  I had some changes related to async mode in my
series to enable async for FreeBSD that I think compose with these
(as opposed to conflicting).  In particular, I fixed target_wait to use
is_async instead of can_async when deciding to permit WNOHANG and I also
pulled some of the can_async checks further up into the core and out
of targets in a few places.

-- 
John Baldwin

Re: [PATCH 1/4] gdb: add asserts in target.c for target_async_permitted

[Simon Marchi]

On 2021-11-23 9:08 a.m., Andrew Burgess via Gdb-patches wrote:
> The target_async_permitted flag allows a user to override whether a
> target can act in async mode or not.  Ideally we would check this flag
> in target_can_async_p and target_is_async_p, however, there are a few
> places in GDB where we call the can_async_p and is_async_p methods
> directly without calling through the target_* wrapper.

Off-hand, I would ask: is it correct for these places to call the target
methods directly?  Should we fix them?

Looking up these spots, I see that they are places where we call
can_async_p on targets possibly before they are pushed, like in
run_one_inferior, so we can't use the target_ wrapper there.

I wouldn't hate it if the targets themselves didn't have to check
target_async_permitted.  For example, linux_nat_target::can_async_p
would simply return true.  And the spots mentioned above could be
changed from:

  int async_p = mi_async && run_target->can_async_p ();

to use a new wrapper overload:

  int async_p = mi_async && target_can_async_p (run_target);

That new wrapper would check target_async_permitted.  I don't think that
is_async_p needs the same treatment: if we never make a target async
because target_async_permitted is false, then is_async_p will always
return false.

Simon

Re: [PATCH 2/4] gdb/remote: merge ::is_async_p and ::can_async_p methods

[Simon Marchi]

On 2021-11-23 9:08 a.m., Andrew Burgess via Gdb-patches wrote:
> I spotted in passing that remote_target::is_async_p and
> remote_target::can_async_p are identical.  This commit just makes
> ::is_async_p call ::can_async_p, removing some duplicate code.
>
> There should be no user visible changes after this commit.
> ---
>  gdb/remote.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 61bde5aaa94..0c4cc6bad0b 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -546,7 +546,10 @@ class remote_target : public process_stratum_target
>
>    bool can_async_p () override;
>
> -  bool is_async_p () override;
> +  bool is_async_p () override
> +  {
> +    return can_async_p ();
> +  }
>
>    void async (int) override;
>
> @@ -14390,19 +14393,6 @@ remote_target::can_async_p ()
>    return serial_can_async_p (rs->remote_desc);
>  }
>
> -bool
> -remote_target::is_async_p ()
> -{
> -  struct remote_state *rs = get_remote_state ();
> -
> -  if (!target_async_permitted)
> -    /* We only enable async when the user specifically asks for it.  */
> -    return false;
> -
> -  /* We're async whenever the serial device is.  */
> -  return serial_is_async_p (rs->remote_desc);
> -}
> -
>  /* Pass the SERIAL event on and up to the client.  One day this code
>     will be able to delay notifying the client of an event until the
>     point where an entire packet has been received.  */
> --
> 2.25.4
>

Hmm, one calls serial_can_async_p and the other calls serial_is_async_p,
I would guess that the distinction is important.

Simon

Re: [PATCH 3/4] gdb: add assert in remote_target::wait relating to async being off

[Simon Marchi]

On 2021-11-23 9:08 a.m., Andrew Burgess via Gdb-patches wrote:
> While working on another patch I ended up in a situation where I had
> async mode disabled (with 'maint set target-async off'), but the async
> event token got marked anyway.
>
> In this situation GDB was continually calling into
> remote_target::wait, however, the async token would never become
> unmarked as the unmarking is guarded by target_is_async_p.
>
> We could just unconditionally unmark the token, but that would feel
> like just ignoring a bug, so, instead, lets assert that if
> !target_is_async_p, then the async token should not be marked.
>
> This assertion would have caught my earlier mistake.
>
> There should be no user visible changes with this commit.
> ---
>  gdb/remote.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 0c4cc6bad0b..64c4cde436b 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -8312,9 +8312,13 @@ remote_target::wait (ptid_t ptid, struct target_waitstatus *status,
>    remote_state *rs = get_remote_state ();
>
>    /* Start by clearing the flag that asks for our wait method to be called,
> -     we'll mark it again at the end if needed.  */
> +     we'll mark it again at the end if needed.  If the target is not in
> +     async mode then the async token should not be marked.  */
>    if (target_is_async_p ())
>      clear_async_event_handler (rs->remote_async_inferior_event_token);
> +  else
> +    gdb_assert (!async_event_handler_marked
> +		(rs->remote_async_inferior_event_token));
>
>    ptid_t event_ptid;

LGTM.

Simon

Re: [PATCH 4/4] gdb/remote: some fixes for 'maint set target-async off'

[Simon Marchi]

On 2021-11-23 9:08 a.m., Andrew Burgess via Gdb-patches wrote:
> While working on another patch relating to remote targets, I wanted to
> test with 'maint set target-async off' in place.  Unfortunately I ran
> into some problems.  This commit is an attempt to fix one of the
> issues I hit.
>
> In my particular case I was actually running with:
>
>   maint set target-async off
>   maint set target-non-stop off
>
> that is, we're telling GDB to force the targets to operate in
> non-async mode, and in all-stop mode.  Here's my GDB session showing
> the problem:
>
>   (gdb) maintenance set target-async off
>   (gdb) maintenance set target-non-stop off
>   (gdb) target extended-remote :54321
>   Remote debugging using :54321
>   (gdb) attach 2365960
>   Attaching to process 2365960
>   No unwaited-for children left.
>   (gdb)
>
> Notice the 'No unwaited-for children left.' error, this is the
> problem.  There's no reason why GDB should not be able to attach to
> the process.
>
> The problem is this:
>
>   1. The user runs 'attach PID' and this sends GDB into attach_command
>   in infcmd.c.  From here we call the ::attach method on the attach
>   target, which will be the extended_remote_target.
>
>   2. In extended_remote_target::attach, we attach to the remote target
>   and get the first reply (which is a stop packet).  We put off
>   processing the stop packet until the end of ::attach.  We setup the
>   inferior and thread to represent the process we attached to, and
>   download the target description.  Finally, we process the initial
>   stop packet.
>
>   If '!target_is_non_stop_p ()' and '!target_can_async_p ()', which is
>   the case for us given the maintenance commands we used, we cache the
>   stop packet within the remote_state::buf for later processing.
>
>   3. Back in attach_command, if 'target_is_non_stop_p ()' then we
>   request that the target stops.  This will either process any cached
>   stop replies, or request that the target stops, and process the stop
>   replies.  However, this code is not what we use due to non-stop mode
>   being disabled.  So, we skip to the next step which is to call
>   validate_exec_file.
>
>   4. Calling validate_exec_file can cause packets to be sent to the
>   remote target, and replies received, the first path I hit is the
>   call to target_pid_to_exec_file, which calls
>   remote_target::pid_to_exec_file, which can then try to read the
>   executable from the remote.  Sending an receiving packets will make
>   use of the remote_state::buf object.
>
>   5. The attempt to attach continues, but the damage is already done...
>
> So, the problem is that, in step #2 we cache a stop reply in the
> remote_state::buf, and then in step #4 we reuse the remote_state::buf
> object, discarding any cached stop reply.  As a result, the initial
> stop, which is sent when GDB first attaches to the target, is lost.
>
> This problem can clearly be seen I feel by looking at the
> remote_state::cached_wait_status flag.  This flag tells GDB if there
> is a wait status cached in remote_state::buf.  However, in
> remote_target::putpkt_binary and remote_target::getpkt_or_notif_sane_1
> this flag is just set back to 0, doing this immediately discards any
> cached data.
>
> I don't know if this scheme ever made sense, maybe once upon a time,
> GDB would, when it found it had no cached stop, simply re-request the
> stop information from the target, however, this certainly isn't the
> case now, and resetting the cached_wait_status is (I claim) a bad
> thing.
>
> So, my first step toward fixing this issue was to replace the two
> instances of 'rs->cached_wait_status = 0;' in ::putpkt_binary and
> ::getpkt_or_notif_sane_1 with 'gdb_assert (rs->cached_wait_status ==
> 0);', this, at least would show me when GDB was doing something
> dangerous, and indeed, this assert is now hit in my test case above.

Good idea.

>
> I did play with using some kind of scoped restore to backup, and
> restore the remote_state::buf object in all the places within remote.c
> that I was hitting where the ::buf was being corrupted.  The first
> problem with this is that, where the ::cached_wait_status flag is
> reset is _not_ where ::buf is corrupted.  For the ::putpkt_binary
> case, by the time we get to the method the buffer has already been
> corrupted in many cases, so we end up needing to add the scoped
> save/restore within the callers, which means we need the save/restore
> in _lots_ of places.
>
> Plus, using this save/restore model feels like the wrong solution.  I
> don't think that it's obvious that the buffer might be holding cached
> data, and I think it would be too easy for new corruptions of the
> buffer to be introduced, which could easily go unnoticed for a long
> time.
>
> So, I really wanted a solution that didn't require us to cache data in
> the ::buf object.
>
> Luckily, I think we already have such a solution in place, the
> remote_state::stop_reply_queue, it seems like this does exactly the
> same task, just in a slightly different way.  With the
> ::stop_reply_queue, the stop packets are processed upon receipt and
> the stop_reply object is added to the queue.  With the ::buf cache
> solution, the unprocessed stop reply is cached in the ::buf, and
> processed later.
>
> So, finally, in this commit, I propose to remove the
> remote_state::cached_wait_status flag and to stop using the ::buf to
> cache stop replies.  Instead, stop replies will now always be stored
> in the ::stop_reply_queue.
>
> There are two places where we use the ::buf to hold a cached stop
> reply, the first is in the ::attach method, and the second is in
> remote_target::start_remote, however, the second of these cases is far
> less problematic, as after caching the stop reply in ::buf we call the
> global start_remote function, which does very little work before
> calling normal_stop, which processes the cached stop reply.  However,
> my plan is to switch both users over to using ::stop_reply_queue so
> that the old (unsafe) ::cached_wait_status mechanism can be completely
> removed.
>
> The next problem is that the ::stop_reply_queue is currently only used
> for async-mode, and so, in remote_target::push_stop_reply, where we
> push stop_reply objects into the ::stop_reply_queue, we currently also
> mark the async event token.  I've modified this so we only mark the
> async event token if 'target_can_async_p ()' - note, _can_, not _is_
> here, ::push_stop_reply is called in places where async mode has been
> temporarily disabled, but, in general, the target can (and will) be
> switched back into async mode.
>
> Another change of interest is in remote_target::remote_interrupt_as.
> Previously this code checked ::cached_wait_status, but didn't check
> for events in the ::stop_reply_queue.  Now that ::cached_wait_status
> has been removed we now check the queue length instead, which should
> have the same result.
>
> Finally, in remote_target::wait_as, I've tried to merge the processing
> of the ::stop_reply_queue with how we used to handle the
> ::cached_wait_status flag.
>
> Currently, when processing the ::stop_reply_queue we call
> process_stop_reply and immediately return.  However, when handling
> ::cached_wait_status we run through the whole of ::wait_as, and return
> at the end of the function.
>
> If we consider a standard stop packet, the two differences I see are:
>
>   1. Resetting of the remote_state::waiting_for_stop_reply, flag; this
>   is not currently done when processing a stop from the
>   ::stop_reply_queue.
>
>   2. The final return value has the possibility of being adjusted at
>   the end of ::wait_as, as well as there being calls to
>   record_currthread, non of which are done if we process a stop from
>   the ::stop_reply_queue.
>
> After this commit I have unified these two paths, that is, when we
> process a packet from ::stop_reply_queue we now reset the flag
> mentioned in #1 above, and pass through the return value adjustment
> code at the end of ::wait_as.
>
> An example of a test that reveals the benefits of this commit is:
>
>   make check-gdb \
>     RUNTESTFLAGS="--target_board=native-extended-gdbserver \
>                   GDBFLAGS='-ex maint\ set\ target-async\ off \
>                             -ex maint\ set\ target-non-stop\ off' \
>                   gdb.base/attach.exp"
>
> For testing I've been running test on x86-64/GNU Linux, and run with
> target boards unix, native-gdbserver, and native-extended-gdbserver.
> For each board I've run with the default GDBFLAGS, as well as with:
>
>   GDBFLAGS='-ex maint\ set\ target-async\ off \
>             -ex maint\ set\ target-non-stop\ off' \
>
> Though running with the above GDBFLAGS is clearly a lot more unstable
> both before and after my patch, I'm not seeing any consistent new
> failures with my patch, except, with the native-extended-gdbserver
> board, where I am seeing new failures, but only because more tests are
> now running.  For that configuration alone I see the number of
> unresolved go down by 49, the number of passes goes up by 446, and the
> number of failures also increases by 144.  All of the failures are new
> tests as far as I can tell.

I don't have time to look at the code right now, but I read the commit
message, I think the direction makes sense.

Simon

Re: [PATCH 2/4] gdb/remote: merge ::is_async_p and ::can_async_p methods

[Andrew Burgess]

Simon Marchi <simark@simark.ca> writes:

> On 2021-11-23 9:08 a.m., Andrew Burgess via Gdb-patches wrote:
>> I spotted in passing that remote_target::is_async_p and
>> remote_target::can_async_p are identical.  This commit just makes
>> ::is_async_p call ::can_async_p, removing some duplicate code.
>>
>> There should be no user visible changes after this commit.
>> ---
>>  gdb/remote.c | 18 ++++--------------
>>  1 file changed, 4 insertions(+), 14 deletions(-)
>>
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index 61bde5aaa94..0c4cc6bad0b 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -546,7 +546,10 @@ class remote_target : public process_stratum_target
>>
>>    bool can_async_p () override;
>>
>> -  bool is_async_p () override;
>> +  bool is_async_p () override
>> +  {
>> +    return can_async_p ();
>> +  }
>>
>>    void async (int) override;
>>
>> @@ -14390,19 +14393,6 @@ remote_target::can_async_p ()
>>    return serial_can_async_p (rs->remote_desc);
>>  }
>>
>> -bool
>> -remote_target::is_async_p ()
>> -{
>> -  struct remote_state *rs = get_remote_state ();
>> -
>> -  if (!target_async_permitted)
>> -    /* We only enable async when the user specifically asks for it.  */
>> -    return false;
>> -
>> -  /* We're async whenever the serial device is.  */
>> -  return serial_is_async_p (rs->remote_desc);
>> -}
>> -
>>  /* Pass the SERIAL event on and up to the client.  One day this code
>>     will be able to delay notifying the client of an event until the
>>     point where an entire packet has been received.  */
>> --
>> 2.25.4
>>
>
> Hmm, one calls serial_can_async_p and the other calls serial_is_async_p,
> I would guess that the distinction is important.

Thank you for spotting this.  I'm embarrassed that I missed that.

This patch is withdrawn.  The rest of the series is still fine though.

Thanks,
Andrew

[PATCHv2 0/6] Improve 'maint set target-async off' for remote targets

[Andrew Burgess]

Changes since v1:

  - The old patch #2 is removed, as Simon pointed out, this was just wrong.

  - The old patch #3, which was approved, is now patch #5, and is unchanged.

  - The old patch #4, is now patch #6, and still needs a review.

  - The old patch #1 has been split up into #1, #2, #3, and #4.  Given
    significant changes since V1 these should probably be reviewed
    afresh.

---

Andrew Burgess (6):
  gdb: introduce a new overload of target_can_async_p
  gdb: hoist target_async_permitted checks into target.c
  gdb: add asserts in target.c for target_async_permitted
  gdb: simplify remote_target::is_async_p
  gdb: add assert in remote_target::wait relating to async being off
  gdb/remote: some fixes for 'maint set target-async off'

 gdb/infcmd.c     |   2 +-
 gdb/linux-nat.c  |   5 +-
 gdb/mi/mi-main.c |   4 +-
 gdb/remote.c     | 206 ++++++++++++++++++++---------------------------
 gdb/target.c     |  21 ++++-
 gdb/target.h     |   4 +
 6 files changed, 118 insertions(+), 124 deletions(-)

-- 
2.25.4

[PATCHv2 1/6] gdb: introduce a new overload of target_can_async_p

[Andrew Burgess]

There are a few places where we call the target_ops::can_async_p
member function directly, instead of using the target_can_async_p
wrapper.

In some of these places this is because we need to ask before the
target has been pushed, and in another location (in target.c) it seems
unnecessary to go through the wrapper when we are already in target.c
code.

However, in the next commit I'd like to hoist some common checks out
of target specific code into target.c.  To achieve this, in this
commit I introduce a new overload of target_can_async_p which takes a
target_ops pointer, and calls the ::can_async_p method directly.

This commit adds the new overload, and has everyone call though it.

There should be no user visible changes after this commit.
---
 gdb/infcmd.c     |  2 +-
 gdb/mi/mi-main.c |  4 ++--
 gdb/target.c     | 10 +++++++++-
 gdb/target.h     |  4 ++++
 4 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 6bbd45618eb..984ce4e042b 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -336,7 +336,7 @@ prepare_execution_command (struct target_ops *target, int background)
 {
   /* If we get a request for running in the bg but the target
      doesn't support it, error out.  */
-  if (background && !target->can_async_p ())
+  if (background && !target_can_async_p (target))
     error (_("Asynchronous execution not supported on this target."));
 
   if (!background)
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index e28fae0cc6c..311697b2d58 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -408,7 +408,7 @@ run_one_inferior (inferior *inf, bool start_p)
 {
   const char *run_cmd = start_p ? "start" : "run";
   struct target_ops *run_target = find_run_target ();
-  int async_p = mi_async && run_target->can_async_p ();
+  int async_p = mi_async && target_can_async_p (run_target);
 
   if (inf->pid != 0)
     {
@@ -473,7 +473,7 @@ mi_cmd_exec_run (const char *command, char **argv, int argc)
     {
       const char *run_cmd = start_p ? "start" : "run";
       struct target_ops *run_target = find_run_target ();
-      int async_p = mi_async && run_target->can_async_p ();
+      int async_p = mi_async && target_can_async_p (run_target);
 
       mi_execute_cli_command (run_cmd, async_p,
 			      async_p ? "&" : NULL);
diff --git a/gdb/target.c b/gdb/target.c
index 8fe27c775ea..c5276ae0fe7 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -396,6 +396,14 @@ target_can_async_p ()
 
 /* See target.h.  */
 
+bool
+target_can_async_p (struct target_ops *target)
+{
+  return target->can_async_p ();
+}
+
+/* See target.h.  */
+
 bool
 target_is_async_p ()
 {
@@ -2602,7 +2610,7 @@ target_wait (ptid_t ptid, struct target_waitstatus *status,
 
   gdb_assert (!proc_target->commit_resumed_state);
 
-  if (!target->can_async_p ())
+  if (!target_can_async_p (target))
     gdb_assert ((options & TARGET_WNOHANG) == 0);
 
   return target->wait (ptid, status, options);
diff --git a/gdb/target.h b/gdb/target.h
index 4dc17fd4806..e709b7d7cfd 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1886,6 +1886,10 @@ extern bool target_async_permitted;
 /* Can the target support asynchronous execution?  */
 extern bool target_can_async_p ();
 
+/* An overload of the above that can be called when the target is not yet
+   pushed, this calls TARGET::can_async_p directly.  */
+extern bool target_can_async_p (struct target_ops *target);
+
 /* Is the target in asynchronous execution mode?  */
 extern bool target_is_async_p ();
 
-- 
2.25.4

[PATCHv2 2/6] gdb: hoist target_async_permitted checks into target.c

[Andrew Burgess]

This commit moves the target_async_permitted check out of each targets
::can_async_p method and into the two target_can_async_p wrapper
functions.

I've left some asserts in the two ::can_async_p methods that I
changed, which will hopefully catch any direct calls to these methods
that might be added in the future.

There should be no user visible changes after this commit.
---
 gdb/linux-nat.c |  5 ++++-
 gdb/remote.c    | 11 ++++-------
 gdb/target.c    |  4 ++++
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index f8f728481ea..3e1d1644d4c 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4088,9 +4088,12 @@ linux_nat_target::is_async_p ()
 bool
 linux_nat_target::can_async_p ()
 {
+  /* This flag should be checked in the common target.c code.  */
+  gdb_assert (target_async_permitted);
+
   /* We're always async, unless the user explicitly prevented it with the
      "maint set target-async" command.  */
-  return target_async_permitted;
+  return true;
 }
 
 bool
diff --git a/gdb/remote.c b/gdb/remote.c
index 724386e0916..6ecea5b7fd7 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -14379,14 +14379,11 @@ remote_target::thread_info_to_thread_handle (struct thread_info *tp)
 bool
 remote_target::can_async_p ()
 {
-  struct remote_state *rs = get_remote_state ();
-
-  /* We don't go async if the user has explicitly prevented it with the
-     "maint set target-async" command.  */
-  if (!target_async_permitted)
-    return false;
+  /* This flag should be checked in the common target.c code.  */
+  gdb_assert (target_async_permitted);
 
-  /* We're async whenever the serial device is.  */
+  /* We're async whenever the serial device can.  */
+  struct remote_state *rs = get_remote_state ();
   return serial_can_async_p (rs->remote_desc);
 }
 
diff --git a/gdb/target.c b/gdb/target.c
index c5276ae0fe7..d693b670350 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -391,6 +391,8 @@ target_can_lock_scheduler ()
 bool
 target_can_async_p ()
 {
+  if (!target_async_permitted)
+    return false;
   return current_inferior ()->top_target ()->can_async_p ();
 }
 
@@ -399,6 +401,8 @@ target_can_async_p ()
 bool
 target_can_async_p (struct target_ops *target)
 {
+  if (!target_async_permitted)
+    return false;
   return target->can_async_p ();
 }
 
-- 
2.25.4

[PATCHv2 3/6] gdb: add asserts in target.c for target_async_permitted

[Andrew Burgess]

The target_async_permitted flag allows a user to override whether a
target can act in async mode or not.  In previous commits I have moved
the checking of this flag out of the various ::can_async_p methods and
into the common target.c code.

In this commit I will add some additional assertions into
target_is_async_p and target_async.  The rules these assertions are
checking are:

  1. A target that returns false for target_can_async_p should never
  become "async enabled", and so ::is_async_p should always return
  false.  This is being checked in target_is_async_p.

  2. GDB should never try to enable async mode for a target that
  returns false for target_can_async_p, this is checked in
  target_async.

There are a few places where we call the ::is_async_p method directly,
in these cases we will obviously not pass through the assert in
target_is_async_p, however, there are also plenty of places where we
do call target_is_async_p so if GDB starts to misbehave we should
catch it quickly enough.

There should be no user visible changes after this commit.
---
 gdb/target.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/target.c b/gdb/target.c
index d693b670350..941433f8953 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -411,7 +411,9 @@ target_can_async_p (struct target_ops *target)
 bool
 target_is_async_p ()
 {
-  return current_inferior ()->top_target ()->is_async_p ();
+  bool result = current_inferior ()->top_target ()->is_async_p ();
+  gdb_assert (target_async_permitted || !result);
+  return result;
 }
 
 exec_direction_kind
@@ -4340,6 +4342,9 @@ maintenance_print_target_stack (const char *cmd, int from_tty)
 void
 target_async (int enable)
 {
+  /* If we are trying to enable async mode then it must be the case that
+     async mode is possible for this target.  */
+  gdb_assert (!enable || target_can_async_p ());
   infrun_async (enable);
   current_inferior ()->top_target ()->async (enable);
 }
-- 
2.25.4

[PATCHv2 4/6] gdb: simplify remote_target::is_async_p

[Andrew Burgess]

This commit simplifies remote_target::is_async_p by removing the
target_async_permitted check.

In previous commits I have added additional assertions around the
target_async_permitted flag into target.c, as a result we should now
be confident that if target_can_async_p returns false, a target will
never have async mode enabled.  Given this, it should not be necessary
to check target_async_permitted in remote_target::is_async_p, if this
flag is false ::is_async_p should return false anyway.  There is an
assert to this effect in target_is_async_p.

There should be no user visible change after this commit.
---
 gdb/remote.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 6ecea5b7fd7..25a4d3cab6e 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -14390,13 +14390,8 @@ remote_target::can_async_p ()
 bool
 remote_target::is_async_p ()
 {
-  struct remote_state *rs = get_remote_state ();
-
-  if (!target_async_permitted)
-    /* We only enable async when the user specifically asks for it.  */
-    return false;
-
   /* We're async whenever the serial device is.  */
+  struct remote_state *rs = get_remote_state ();
   return serial_is_async_p (rs->remote_desc);
 }
 
-- 
2.25.4

[PATCHv2 5/6] gdb: add assert in remote_target::wait relating to async being off

[Andrew Burgess]

While working on another patch I ended up in a situation where I had
async mode disabled (with 'maint set target-async off'), but the async
event token got marked anyway.

In this situation GDB was continually calling into
remote_target::wait, however, the async token would never become
unmarked as the unmarking is guarded by target_is_async_p.

We could just unconditionally unmark the token, but that would feel
like just ignoring a bug, so, instead, lets assert that if
!target_is_async_p, then the async token should not be marked.

This assertion would have caught my earlier mistake.

There should be no user visible changes with this commit.
---
 gdb/remote.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 25a4d3cab6e..da8ed81ba78 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -8309,9 +8309,13 @@ remote_target::wait (ptid_t ptid, struct target_waitstatus *status,
   remote_state *rs = get_remote_state ();
 
   /* Start by clearing the flag that asks for our wait method to be called,
-     we'll mark it again at the end if needed.  */
+     we'll mark it again at the end if needed.  If the target is not in
+     async mode then the async token should not be marked.  */
   if (target_is_async_p ())
     clear_async_event_handler (rs->remote_async_inferior_event_token);
+  else
+    gdb_assert (!async_event_handler_marked
+		(rs->remote_async_inferior_event_token));
 
   ptid_t event_ptid;
 
-- 
2.25.4

[PATCHv2 6/6] gdb/remote: some fixes for 'maint set target-async off'

[Andrew Burgess]

While working on another patch relating to remote targets, I wanted to
test with 'maint set target-async off' in place.  Unfortunately I ran
into some problems.  This commit is an attempt to fix one of the
issues I hit.

In my particular case I was actually running with:

  maint set target-async off
  maint set target-non-stop off

that is, we're telling GDB to force the targets to operate in
non-async mode, and in all-stop mode.  Here's my GDB session showing
the problem:

  (gdb) maintenance set target-async off
  (gdb) maintenance set target-non-stop off
  (gdb) target extended-remote :54321
  Remote debugging using :54321
  (gdb) attach 2365960
  Attaching to process 2365960
  No unwaited-for children left.
  (gdb)

Notice the 'No unwaited-for children left.' error, this is the
problem.  There's no reason why GDB should not be able to attach to
the process.

The problem is this:

  1. The user runs 'attach PID' and this sends GDB into attach_command
  in infcmd.c.  From here we call the ::attach method on the attach
  target, which will be the extended_remote_target.

  2. In extended_remote_target::attach, we attach to the remote target
  and get the first reply (which is a stop packet).  We put off
  processing the stop packet until the end of ::attach.  We setup the
  inferior and thread to represent the process we attached to, and
  download the target description.  Finally, we process the initial
  stop packet.

  If '!target_is_non_stop_p ()' and '!target_can_async_p ()', which is
  the case for us given the maintenance commands we used, we cache the
  stop packet within the remote_state::buf for later processing.

  3. Back in attach_command, if 'target_is_non_stop_p ()' then we
  request that the target stops.  This will either process any cached
  stop replies, or request that the target stops, and process the stop
  replies.  However, this code is not what we use due to non-stop mode
  being disabled.  So, we skip to the next step which is to call
  validate_exec_file.

  4. Calling validate_exec_file can cause packets to be sent to the
  remote target, and replies received, the first path I hit is the
  call to target_pid_to_exec_file, which calls
  remote_target::pid_to_exec_file, which can then try to read the
  executable from the remote.  Sending an receiving packets will make
  use of the remote_state::buf object.

  5. The attempt to attach continues, but the damage is already done...

So, the problem is that, in step #2 we cache a stop reply in the
remote_state::buf, and then in step #4 we reuse the remote_state::buf
object, discarding any cached stop reply.  As a result, the initial
stop, which is sent when GDB first attaches to the target, is lost.

This problem can clearly be seen I feel by looking at the
remote_state::cached_wait_status flag.  This flag tells GDB if there
is a wait status cached in remote_state::buf.  However, in
remote_target::putpkt_binary and remote_target::getpkt_or_notif_sane_1
this flag is just set back to 0, doing this immediately discards any
cached data.

I don't know if this scheme ever made sense, maybe once upon a time,
GDB would, when it found it had no cached stop, simply re-request the
stop information from the target, however, this certainly isn't the
case now, and resetting the cached_wait_status is (I claim) a bad
thing.

So, my first step toward fixing this issue was to replace the two
instances of 'rs->cached_wait_status = 0;' in ::putpkt_binary and
::getpkt_or_notif_sane_1 with 'gdb_assert (rs->cached_wait_status ==
0);', this, at least would show me when GDB was doing something
dangerous, and indeed, this assert is now hit in my test case above.

I did play with using some kind of scoped restore to backup, and
restore the remote_state::buf object in all the places within remote.c
that I was hitting where the ::buf was being corrupted.  The first
problem with this is that, where the ::cached_wait_status flag is
reset is _not_ where ::buf is corrupted.  For the ::putpkt_binary
case, by the time we get to the method the buffer has already been
corrupted in many cases, so we end up needing to add the scoped
save/restore within the callers, which means we need the save/restore
in _lots_ of places.

Plus, using this save/restore model feels like the wrong solution.  I
don't think that it's obvious that the buffer might be holding cached
data, and I think it would be too easy for new corruptions of the
buffer to be introduced, which could easily go unnoticed for a long
time.

So, I really wanted a solution that didn't require us to cache data in
the ::buf object.

Luckily, I think we already have such a solution in place, the
remote_state::stop_reply_queue, it seems like this does exactly the
same task, just in a slightly different way.  With the
::stop_reply_queue, the stop packets are processed upon receipt and
the stop_reply object is added to the queue.  With the ::buf cache
solution, the unprocessed stop reply is cached in the ::buf, and
processed later.

So, finally, in this commit, I propose to remove the
remote_state::cached_wait_status flag and to stop using the ::buf to
cache stop replies.  Instead, stop replies will now always be stored
in the ::stop_reply_queue.

There are two places where we use the ::buf to hold a cached stop
reply, the first is in the ::attach method, and the second is in
remote_target::start_remote, however, the second of these cases is far
less problematic, as after caching the stop reply in ::buf we call the
global start_remote function, which does very little work before
calling normal_stop, which processes the cached stop reply.  However,
my plan is to switch both users over to using ::stop_reply_queue so
that the old (unsafe) ::cached_wait_status mechanism can be completely
removed.

The next problem is that the ::stop_reply_queue is currently only used
for async-mode, and so, in remote_target::push_stop_reply, where we
push stop_reply objects into the ::stop_reply_queue, we currently also
mark the async event token.  I've modified this so we only mark the
async event token if 'target_can_async_p ()' - note, _can_, not _is_
here, ::push_stop_reply is called in places where async mode has been
temporarily disabled, but, in general, the target can (and will) be
switched back into async mode.

Another change of interest is in remote_target::remote_interrupt_as.
Previously this code checked ::cached_wait_status, but didn't check
for events in the ::stop_reply_queue.  Now that ::cached_wait_status
has been removed we now check the queue length instead, which should
have the same result.

Finally, in remote_target::wait_as, I've tried to merge the processing
of the ::stop_reply_queue with how we used to handle the
::cached_wait_status flag.

Currently, when processing the ::stop_reply_queue we call
process_stop_reply and immediately return.  However, when handling
::cached_wait_status we run through the whole of ::wait_as, and return
at the end of the function.

If we consider a standard stop packet, the two differences I see are:

  1. Resetting of the remote_state::waiting_for_stop_reply, flag; this
  is not currently done when processing a stop from the
  ::stop_reply_queue.

  2. The final return value has the possibility of being adjusted at
  the end of ::wait_as, as well as there being calls to
  record_currthread, non of which are done if we process a stop from
  the ::stop_reply_queue.

After this commit I have unified these two paths, that is, when we
process a packet from ::stop_reply_queue we now reset the flag
mentioned in #1 above, and pass through the return value adjustment
code at the end of ::wait_as.

An example of a test that reveals the benefits of this commit is:

  make check-gdb \
    RUNTESTFLAGS="--target_board=native-extended-gdbserver \
                  GDBFLAGS='-ex maint\ set\ target-async\ off \
                            -ex maint\ set\ target-non-stop\ off' \
                  gdb.base/attach.exp"

For testing I've been running test on x86-64/GNU Linux, and run with
target boards unix, native-gdbserver, and native-extended-gdbserver.
For each board I've run with the default GDBFLAGS, as well as with:

  GDBFLAGS='-ex maint\ set\ target-async\ off \
            -ex maint\ set\ target-non-stop\ off' \

Though running with the above GDBFLAGS is clearly a lot more unstable
both before and after my patch, I'm not seeing any consistent new
failures with my patch, except, with the native-extended-gdbserver
board, where I am seeing new failures, but only because more tests are
now running.  For that configuration alone I see the number of
unresolved go down by 49, the number of passes goes up by 446, and the
number of failures also increases by 144.  All of the failures are new
tests as far as I can tell.
---
 gdb/remote.c | 182 ++++++++++++++++++++++-----------------------------
 1 file changed, 78 insertions(+), 104 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index da8ed81ba78..9b91165cf91 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -258,15 +258,6 @@ class remote_state
      Otherwise zero, meaning to use the guessed size.  */
   long explicit_packet_size = 0;
 
-  /* remote_wait is normally called when the target is running and
-     waits for a stop reply packet.  But sometimes we need to call it
-     when the target is already stopped.  We can send a "?" packet
-     and have remote_wait read the response.  Or, if we already have
-     the response, we can stash it in BUF and tell remote_wait to
-     skip calling getpkt.  This flag is set when BUF contains a
-     stop reply packet and the target is not waiting.  */
-  int cached_wait_status = 0;
-
   /* True, if in no ack mode.  That is, neither GDB nor the stub will
      expect acks from each other.  The connection is assumed to be
      reliable.  */
@@ -4901,8 +4892,9 @@ remote_target::start_remote (int from_tty, int extended_p)
 
       /* Use the previously fetched status.  */
       gdb_assert (wait_status != NULL);
-      strcpy (rs->buf.data (), wait_status);
-      rs->cached_wait_status = 1;
+      struct notif_event *reply
+	= remote_notif_parse (this, &notif_client_stop, wait_status);
+      push_stop_reply ((struct stop_reply *) reply);
 
       ::start_remote (from_tty); /* Initialize gdb process mechanisms.  */
     }
@@ -5738,7 +5730,6 @@ remote_target::open_1 (const char *name, int from_tty, int extended_p)
   /* Reset the target state; these things will be queried either by
      remote_query_supported or as they are needed.  */
   reset_all_packet_configs_support ();
-  rs->cached_wait_status = 0;
   rs->explicit_packet_size = 0;
   rs->noack_mode = 0;
   rs->extended = extended_p;
@@ -6081,21 +6072,13 @@ extended_remote_target::attach (const char *args, int from_tty)
       /* Use the previously fetched status.  */
       gdb_assert (wait_status != NULL);
 
-      if (target_can_async_p ())
-	{
-	  struct notif_event *reply
-	    =  remote_notif_parse (this, &notif_client_stop, wait_status);
+      struct notif_event *reply
+	=  remote_notif_parse (this, &notif_client_stop, wait_status);
 
-	  push_stop_reply ((struct stop_reply *) reply);
+      push_stop_reply ((struct stop_reply *) reply);
 
-	  target_async (1);
-	}
-      else
-	{
-	  gdb_assert (wait_status != NULL);
-	  strcpy (rs->buf.data (), wait_status);
-	  rs->cached_wait_status = 1;
-	}
+      if (target_can_async_p ())
+	target_async (1);
     }
   else
     {
@@ -6998,9 +6981,9 @@ remote_target::remote_interrupt_as ()
   rs->ctrlc_pending_p = 1;
 
   /* If the inferior is stopped already, but the core didn't know
-     about it yet, just ignore the request.  The cached wait status
+     about it yet, just ignore the request.  The pending stop events
      will be collected in remote_wait.  */
-  if (rs->cached_wait_status)
+  if (stop_reply_queue_length () > 0)
     return;
 
   /* Send interrupt_sequence to remote target.  */
@@ -7429,7 +7412,7 @@ remote_target::queued_stop_reply (ptid_t ptid)
   remote_state *rs = get_remote_state ();
   struct stop_reply *r = remote_notif_remove_queued_reply (ptid);
 
-  if (!rs->stop_reply_queue.empty ())
+  if (!rs->stop_reply_queue.empty () && target_can_async_p ())
     {
       /* There's still at least an event left.  */
       mark_async_event_handler (rs->remote_async_inferior_event_token);
@@ -7454,7 +7437,8 @@ remote_target::push_stop_reply (struct stop_reply *new_event)
 			target_pid_to_str (new_event->ptid).c_str (),
 			int (rs->stop_reply_queue.size ()));
 
-  mark_async_event_handler (rs->remote_async_inferior_event_token);
+  if (target_can_async_p ())
+    mark_async_event_handler (rs->remote_async_inferior_event_token);
 }
 
 /* Returns true if we have a stop reply for PTID.  */
@@ -8163,15 +8147,12 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status,
 
   stop_reply = queued_stop_reply (ptid);
   if (stop_reply != NULL)
-    return process_stop_reply (stop_reply, status);
-
-  if (rs->cached_wait_status)
-    /* Use the cached wait status, but only once.  */
-    rs->cached_wait_status = 0;
+    {
+      rs->waiting_for_stop_reply = 0;
+      event_ptid = process_stop_reply (stop_reply, status);
+    }
   else
     {
-      int ret;
-      int is_notif;
       int forever = ((options & TARGET_WNOHANG) == 0
 		     && rs->wait_forever_enabled_p);
 
@@ -8185,7 +8166,8 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status,
 	 _never_ wait for ever -> test on target_is_async_p().
 	 However, before we do that we need to ensure that the caller
 	 knows how to take the target into/out of async mode.  */
-      ret = getpkt_or_notif_sane (&rs->buf, forever, &is_notif);
+      int is_notif;
+      int ret = getpkt_or_notif_sane (&rs->buf, forever, &is_notif);
 
       /* GDB gets a notification.  Return to core as this event is
 	 not interesting.  */
@@ -8194,73 +8176,73 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status,
 
       if (ret == -1 && (options & TARGET_WNOHANG) != 0)
 	return minus_one_ptid;
-    }
 
-  buf = rs->buf.data ();
+      buf = rs->buf.data ();
 
-  /* Assume that the target has acknowledged Ctrl-C unless we receive
-     an 'F' or 'O' packet.  */
-  if (buf[0] != 'F' && buf[0] != 'O')
-    rs->ctrlc_pending_p = 0;
+      /* Assume that the target has acknowledged Ctrl-C unless we receive
+	 an 'F' or 'O' packet.  */
+      if (buf[0] != 'F' && buf[0] != 'O')
+	rs->ctrlc_pending_p = 0;
 
-  switch (buf[0])
-    {
-    case 'E':		/* Error of some sort.	*/
-      /* We're out of sync with the target now.  Did it continue or
-	 not?  Not is more likely, so report a stop.  */
-      rs->waiting_for_stop_reply = 0;
+      switch (buf[0])
+	{
+	case 'E':		/* Error of some sort.	*/
+	  /* We're out of sync with the target now.  Did it continue or
+	     not?  Not is more likely, so report a stop.  */
+	  rs->waiting_for_stop_reply = 0;
 
-      warning (_("Remote failure reply: %s"), buf);
-      status->set_stopped (GDB_SIGNAL_0);
-      break;
-    case 'F':		/* File-I/O request.  */
-      /* GDB may access the inferior memory while handling the File-I/O
-	 request, but we don't want GDB accessing memory while waiting
-	 for a stop reply.  See the comments in putpkt_binary.  Set
-	 waiting_for_stop_reply to 0 temporarily.  */
-      rs->waiting_for_stop_reply = 0;
-      remote_fileio_request (this, buf, rs->ctrlc_pending_p);
-      rs->ctrlc_pending_p = 0;
-      /* GDB handled the File-I/O request, and the target is running
-	 again.  Keep waiting for events.  */
-      rs->waiting_for_stop_reply = 1;
-      break;
-    case 'N': case 'T': case 'S': case 'X': case 'W':
-      {
-	/* There is a stop reply to handle.  */
-	rs->waiting_for_stop_reply = 0;
+	  warning (_("Remote failure reply: %s"), buf);
+	  status->set_stopped (GDB_SIGNAL_0);
+	  break;
+	case 'F':		/* File-I/O request.  */
+	  /* GDB may access the inferior memory while handling the File-I/O
+	     request, but we don't want GDB accessing memory while waiting
+	     for a stop reply.  See the comments in putpkt_binary.  Set
+	     waiting_for_stop_reply to 0 temporarily.  */
+	  rs->waiting_for_stop_reply = 0;
+	  remote_fileio_request (this, buf, rs->ctrlc_pending_p);
+	  rs->ctrlc_pending_p = 0;
+	  /* GDB handled the File-I/O request, and the target is running
+	     again.  Keep waiting for events.  */
+	  rs->waiting_for_stop_reply = 1;
+	  break;
+	case 'N': case 'T': case 'S': case 'X': case 'W':
+	  {
+	    /* There is a stop reply to handle.  */
+	    rs->waiting_for_stop_reply = 0;
 
-	stop_reply
-	  = (struct stop_reply *) remote_notif_parse (this,
-						      &notif_client_stop,
-						      rs->buf.data ());
+	    stop_reply
+	      = (struct stop_reply *) remote_notif_parse (this,
+							  &notif_client_stop,
+							  rs->buf.data ());
 
-	event_ptid = process_stop_reply (stop_reply, status);
-	break;
-      }
-    case 'O':		/* Console output.  */
-      remote_console_output (buf + 1);
-      break;
-    case '\0':
-      if (rs->last_sent_signal != GDB_SIGNAL_0)
-	{
-	  /* Zero length reply means that we tried 'S' or 'C' and the
-	     remote system doesn't support it.  */
-	  target_terminal::ours_for_output ();
-	  printf_filtered
-	    ("Can't send signals to this remote system.  %s not sent.\n",
-	     gdb_signal_to_name (rs->last_sent_signal));
-	  rs->last_sent_signal = GDB_SIGNAL_0;
-	  target_terminal::inferior ();
-
-	  strcpy (buf, rs->last_sent_step ? "s" : "c");
-	  putpkt (buf);
+	    event_ptid = process_stop_reply (stop_reply, status);
+	    break;
+	  }
+	case 'O':		/* Console output.  */
+	  remote_console_output (buf + 1);
+	  break;
+	case '\0':
+	  if (rs->last_sent_signal != GDB_SIGNAL_0)
+	    {
+	      /* Zero length reply means that we tried 'S' or 'C' and the
+		 remote system doesn't support it.  */
+	      target_terminal::ours_for_output ();
+	      printf_filtered
+		("Can't send signals to this remote system.  %s not sent.\n",
+		 gdb_signal_to_name (rs->last_sent_signal));
+	      rs->last_sent_signal = GDB_SIGNAL_0;
+	      target_terminal::inferior ();
+
+	      strcpy (buf, rs->last_sent_step ? "s" : "c");
+	      putpkt (buf);
+	      break;
+	    }
+	  /* fallthrough */
+	default:
+	  warning (_("Invalid remote reply: %s"), buf);
 	  break;
 	}
-      /* fallthrough */
-    default:
-      warning (_("Invalid remote reply: %s"), buf);
-      break;
     }
 
   if (status->kind () == TARGET_WAITKIND_NO_RESUMED)
@@ -9560,10 +9542,6 @@ remote_target::putpkt_binary (const char *buf, int cnt)
 	       "and then try again."));
     }
 
-  /* We're sending out a new packet.  Make sure we don't look at a
-     stale cached response.  */
-  rs->cached_wait_status = 0;
-
   /* Copy the packet into buffer BUF2, encapsulating it
      and giving it a checksum.  */
 
@@ -9901,10 +9879,6 @@ remote_target::getpkt_or_notif_sane_1 (gdb::char_vector *buf,
   int timeout;
   int val = -1;
 
-  /* We're reading a new response.  Make sure we don't look at a
-     previously cached response.  */
-  rs->cached_wait_status = 0;
-
   strcpy (buf->data (), "timeout");
 
   if (forever)
-- 
2.25.4

Re: [PATCHv2 2/6] gdb: hoist target_async_permitted checks into target.c

[Simon Marchi]

On 2021-11-24 7:22 a.m., Andrew Burgess via Gdb-patches wrote:
> This commit moves the target_async_permitted check out of each targets
> ::can_async_p method and into the two target_can_async_p wrapper
> functions.
>
> I've left some asserts in the two ::can_async_p methods that I
> changed, which will hopefully catch any direct calls to these methods
> that might be added in the future.
>
> There should be no user visible changes after this commit.
> ---
>  gdb/linux-nat.c |  5 ++++-
>  gdb/remote.c    | 11 ++++-------
>  gdb/target.c    |  4 ++++
>  3 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index f8f728481ea..3e1d1644d4c 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -4088,9 +4088,12 @@ linux_nat_target::is_async_p ()
>  bool
>  linux_nat_target::can_async_p ()
>  {
> +  /* This flag should be checked in the common target.c code.  */
> +  gdb_assert (target_async_permitted);
> +
>    /* We're always async, unless the user explicitly prevented it with the
>       "maint set target-async" command.  */
> -  return target_async_permitted;
> +  return true;

The comment just above that isn't false, but maybe it's not so relevant
to have it here anymore.

>  }
>
>  bool
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 724386e0916..6ecea5b7fd7 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -14379,14 +14379,11 @@ remote_target::thread_info_to_thread_handle (struct thread_info *tp)
>  bool
>  remote_target::can_async_p ()
>  {
> -  struct remote_state *rs = get_remote_state ();
> -
> -  /* We don't go async if the user has explicitly prevented it with the
> -     "maint set target-async" command.  */
> -  if (!target_async_permitted)
> -    return false;
> +  /* This flag should be checked in the common target.c code.  */
> +  gdb_assert (target_async_permitted);
>
> -  /* We're async whenever the serial device is.  */
> +  /* We're async whenever the serial device can.  */
> +  struct remote_state *rs = get_remote_state ();
>    return serial_can_async_p (rs->remote_desc);
>  }
>
> diff --git a/gdb/target.c b/gdb/target.c
> index c5276ae0fe7..d693b670350 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -391,6 +391,8 @@ target_can_lock_scheduler ()
>  bool
>  target_can_async_p ()
>  {
> +  if (!target_async_permitted)
> +    return false;
>    return current_inferior ()->top_target ()->can_async_p ();
>  }

You could make this overload call the other one:

  return target_can_async_p (current_inferior ()->top_target ());

so that there is really a single point where target_async_permitted is
checked.

Otherwise, LGTM.

Simon

Re: [PATCHv2 3/6] gdb: add asserts in target.c for target_async_permitted

[Simon Marchi]

On 2021-11-24 7:22 a.m., Andrew Burgess via Gdb-patches wrote:
> The target_async_permitted flag allows a user to override whether a
> target can act in async mode or not.  In previous commits I have moved
> the checking of this flag out of the various ::can_async_p methods and
> into the common target.c code.
>
> In this commit I will add some additional assertions into
> target_is_async_p and target_async.  The rules these assertions are
> checking are:
>
>   1. A target that returns false for target_can_async_p should never
>   become "async enabled", and so ::is_async_p should always return
>   false.  This is being checked in target_is_async_p.
>
>   2. GDB should never try to enable async mode for a target that
>   returns false for target_can_async_p, this is checked in
>   target_async.
>
> There are a few places where we call the ::is_async_p method directly,
> in these cases we will obviously not pass through the assert in
> target_is_async_p, however, there are also plenty of places where we
> do call target_is_async_p so if GDB starts to misbehave we should
> catch it quickly enough.

Despite what I said in my v1 review, it would actually be nice to have a
target_is_async_p wrapper like the new target_can_async_p wrapper, just
to have the gdb_assert checked consistently.  But I think it can be done
later, to avoid having a v3 of this series just for a minor detail like
that.

The patch LGTM as-is.

Simon

Re: [PATCHv2 5/6] gdb: add assert in remote_target::wait relating to async being off

[Simon Marchi]

On 2021-11-24 7:22 a.m., Andrew Burgess via Gdb-patches wrote:
> While working on another patch I ended up in a situation where I had
> async mode disabled (with 'maint set target-async off'), but the async
> event token got marked anyway.
> 
> In this situation GDB was continually calling into
> remote_target::wait, however, the async token would never become
> unmarked as the unmarking is guarded by target_is_async_p.
> 
> We could just unconditionally unmark the token, but that would feel
> like just ignoring a bug, so, instead, lets assert that if
> !target_is_async_p, then the async token should not be marked.
> 
> This assertion would have caught my earlier mistake.
> 
> There should be no user visible changes with this commit.
> ---
>  gdb/remote.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 25a4d3cab6e..da8ed81ba78 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -8309,9 +8309,13 @@ remote_target::wait (ptid_t ptid, struct target_waitstatus *status,
>    remote_state *rs = get_remote_state ();
>  
>    /* Start by clearing the flag that asks for our wait method to be called,
> -     we'll mark it again at the end if needed.  */
> +     we'll mark it again at the end if needed.  If the target is not in
> +     async mode then the async token should not be marked.  */
>    if (target_is_async_p ())
>      clear_async_event_handler (rs->remote_async_inferior_event_token);
> +  else
> +    gdb_assert (!async_event_handler_marked
> +		(rs->remote_async_inferior_event_token));
>  
>    ptid_t event_ptid;
>  
> -- 
> 2.25.4
> 

LGTM.

I think the series can be merged at least up to here, I think these are
good cleanups.

Simon

Re: [PATCHv2 5/6] gdb: add assert in remote_target::wait relating to async being off

[Andrew Burgess]

* Simon Marchi <simark@simark.ca> [2021-11-24 16:23:30 -0500]:

> On 2021-11-24 7:22 a.m., Andrew Burgess via Gdb-patches wrote:
> > While working on another patch I ended up in a situation where I had
> > async mode disabled (with 'maint set target-async off'), but the async
> > event token got marked anyway.
> > 
> > In this situation GDB was continually calling into
> > remote_target::wait, however, the async token would never become
> > unmarked as the unmarking is guarded by target_is_async_p.
> > 
> > We could just unconditionally unmark the token, but that would feel
> > like just ignoring a bug, so, instead, lets assert that if
> > !target_is_async_p, then the async token should not be marked.
> > 
> > This assertion would have caught my earlier mistake.
> > 
> > There should be no user visible changes with this commit.
> > ---
> >  gdb/remote.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/gdb/remote.c b/gdb/remote.c
> > index 25a4d3cab6e..da8ed81ba78 100644
> > --- a/gdb/remote.c
> > +++ b/gdb/remote.c
> > @@ -8309,9 +8309,13 @@ remote_target::wait (ptid_t ptid, struct target_waitstatus *status,
> >    remote_state *rs = get_remote_state ();
> >  
> >    /* Start by clearing the flag that asks for our wait method to be called,
> > -     we'll mark it again at the end if needed.  */
> > +     we'll mark it again at the end if needed.  If the target is not in
> > +     async mode then the async token should not be marked.  */
> >    if (target_is_async_p ())
> >      clear_async_event_handler (rs->remote_async_inferior_event_token);
> > +  else
> > +    gdb_assert (!async_event_handler_marked
> > +		(rs->remote_async_inferior_event_token));
> >  
> >    ptid_t event_ptid;
> >  
> > -- 
> > 2.25.4
> > 
> 
> LGTM.
> 
> I think the series can be merged at least up to here, I think these are
> good cleanups.

Thanks, I made the change you suggested about one target_can_async_p
function calling the other, and pushed patches 1 to 5.

The final patch still pending.

Thanks,
Andrew

Re: [PATCHv2 5/6] gdb: add assert in remote_target::wait relating to async being off

[Tom de Vries]

On 11/25/21 11:04 AM, Andrew Burgess via Gdb-patches wrote:
> * Simon Marchi <simark@simark.ca> [2021-11-24 16:23:30 -0500]:
> 
>> On 2021-11-24 7:22 a.m., Andrew Burgess via Gdb-patches wrote:
>>> While working on another patch I ended up in a situation where I had
>>> async mode disabled (with 'maint set target-async off'), but the async
>>> event token got marked anyway.
>>>
>>> In this situation GDB was continually calling into
>>> remote_target::wait, however, the async token would never become
>>> unmarked as the unmarking is guarded by target_is_async_p.
>>>
>>> We could just unconditionally unmark the token, but that would feel
>>> like just ignoring a bug, so, instead, lets assert that if
>>> !target_is_async_p, then the async token should not be marked.
>>>
>>> This assertion would have caught my earlier mistake.
>>>
>>> There should be no user visible changes with this commit.
>>> ---
>>>  gdb/remote.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/gdb/remote.c b/gdb/remote.c
>>> index 25a4d3cab6e..da8ed81ba78 100644
>>> --- a/gdb/remote.c
>>> +++ b/gdb/remote.c
>>> @@ -8309,9 +8309,13 @@ remote_target::wait (ptid_t ptid, struct target_waitstatus *status,
>>>    remote_state *rs = get_remote_state ();
>>>  
>>>    /* Start by clearing the flag that asks for our wait method to be called,
>>> -     we'll mark it again at the end if needed.  */
>>> +     we'll mark it again at the end if needed.  If the target is not in
>>> +     async mode then the async token should not be marked.  */
>>>    if (target_is_async_p ())
>>>      clear_async_event_handler (rs->remote_async_inferior_event_token);
>>> +  else
>>> +    gdb_assert (!async_event_handler_marked
>>> +		(rs->remote_async_inferior_event_token));
>>>  
>>>    ptid_t event_ptid;
>>>  
>>> -- 
>>> 2.25.4
>>>
>>
>> LGTM.
>>
>> I think the series can be merged at least up to here, I think these are
>> good cleanups.
> 
> Thanks, I made the change you suggested about one target_can_async_p
> function calling the other, and pushed patches 1 to 5.
> 

I'm running into the assert:
https://sourceware.org/bugzilla/show_bug.cgi?id=28627 .

Thanks,
- Tom

> The final patch still pending.
> 
> Thanks,
> Andrew
> 

Re: [PATCHv2 5/6] gdb: add assert in remote_target::wait relating to async being off

[Andrew Burgess]

* Tom de Vries <tdevries@suse.de> [2021-11-25 12:36:07 +0100]:

> On 11/25/21 11:04 AM, Andrew Burgess via Gdb-patches wrote:
> > * Simon Marchi <simark@simark.ca> [2021-11-24 16:23:30 -0500]:
> > 
> >> On 2021-11-24 7:22 a.m., Andrew Burgess via Gdb-patches wrote:
> >>> While working on another patch I ended up in a situation where I had
> >>> async mode disabled (with 'maint set target-async off'), but the async
> >>> event token got marked anyway.
> >>>
> >>> In this situation GDB was continually calling into
> >>> remote_target::wait, however, the async token would never become
> >>> unmarked as the unmarking is guarded by target_is_async_p.
> >>>
> >>> We could just unconditionally unmark the token, but that would feel
> >>> like just ignoring a bug, so, instead, lets assert that if
> >>> !target_is_async_p, then the async token should not be marked.
> >>>
> >>> This assertion would have caught my earlier mistake.
> >>>
> >>> There should be no user visible changes with this commit.
> >>> ---
> >>>  gdb/remote.c | 6 +++++-
> >>>  1 file changed, 5 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/gdb/remote.c b/gdb/remote.c
> >>> index 25a4d3cab6e..da8ed81ba78 100644
> >>> --- a/gdb/remote.c
> >>> +++ b/gdb/remote.c
> >>> @@ -8309,9 +8309,13 @@ remote_target::wait (ptid_t ptid, struct target_waitstatus *status,
> >>>    remote_state *rs = get_remote_state ();
> >>>  
> >>>    /* Start by clearing the flag that asks for our wait method to be called,
> >>> -     we'll mark it again at the end if needed.  */
> >>> +     we'll mark it again at the end if needed.  If the target is not in
> >>> +     async mode then the async token should not be marked.  */
> >>>    if (target_is_async_p ())
> >>>      clear_async_event_handler (rs->remote_async_inferior_event_token);
> >>> +  else
> >>> +    gdb_assert (!async_event_handler_marked
> >>> +		(rs->remote_async_inferior_event_token));
> >>>  
> >>>    ptid_t event_ptid;
> >>>  
> >>> -- 
> >>> 2.25.4
> >>>
> >>
> >> LGTM.
> >>
> >> I think the series can be merged at least up to here, I think these are
> >> good cleanups.
> > 
> > Thanks, I made the change you suggested about one target_can_async_p
> > function calling the other, and pushed patches 1 to 5.
> > 
> 
> I'm running into the assert:
> https://sourceware.org/bugzilla/show_bug.cgi?id=28627 .

Sorry about that.  I've reverted this patch, and closed the bug.

The cause of the assert is fixed by patch #6 in this series, so I
guess I'll remerge this patch after patch #6 lands.

Thanks,
Andrew

Re: [PATCHv2 5/6] gdb: add assert in remote_target::wait relating to async being off

[Tom de Vries]

On 11/25/21 2:46 PM, Andrew Burgess wrote:
> * Tom de Vries <tdevries@suse.de> [2021-11-25 12:36:07 +0100]:
> 
>> On 11/25/21 11:04 AM, Andrew Burgess via Gdb-patches wrote:
>>> * Simon Marchi <simark@simark.ca> [2021-11-24 16:23:30 -0500]:
>>>
>>>> On 2021-11-24 7:22 a.m., Andrew Burgess via Gdb-patches wrote:
>>>>> While working on another patch I ended up in a situation where I had
>>>>> async mode disabled (with 'maint set target-async off'), but the async
>>>>> event token got marked anyway.
>>>>>
>>>>> In this situation GDB was continually calling into
>>>>> remote_target::wait, however, the async token would never become
>>>>> unmarked as the unmarking is guarded by target_is_async_p.
>>>>>
>>>>> We could just unconditionally unmark the token, but that would feel
>>>>> like just ignoring a bug, so, instead, lets assert that if
>>>>> !target_is_async_p, then the async token should not be marked.
>>>>>
>>>>> This assertion would have caught my earlier mistake.
>>>>>
>>>>> There should be no user visible changes with this commit.
>>>>> ---
>>>>>  gdb/remote.c | 6 +++++-
>>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/gdb/remote.c b/gdb/remote.c
>>>>> index 25a4d3cab6e..da8ed81ba78 100644
>>>>> --- a/gdb/remote.c
>>>>> +++ b/gdb/remote.c
>>>>> @@ -8309,9 +8309,13 @@ remote_target::wait (ptid_t ptid, struct target_waitstatus *status,
>>>>>    remote_state *rs = get_remote_state ();
>>>>>  
>>>>>    /* Start by clearing the flag that asks for our wait method to be called,
>>>>> -     we'll mark it again at the end if needed.  */
>>>>> +     we'll mark it again at the end if needed.  If the target is not in
>>>>> +     async mode then the async token should not be marked.  */
>>>>>    if (target_is_async_p ())
>>>>>      clear_async_event_handler (rs->remote_async_inferior_event_token);
>>>>> +  else
>>>>> +    gdb_assert (!async_event_handler_marked
>>>>> +		(rs->remote_async_inferior_event_token));
>>>>>  
>>>>>    ptid_t event_ptid;
>>>>>  
>>>>> -- 
>>>>> 2.25.4
>>>>>
>>>>
>>>> LGTM.
>>>>
>>>> I think the series can be merged at least up to here, I think these are
>>>> good cleanups.
>>>
>>> Thanks, I made the change you suggested about one target_can_async_p
>>> function calling the other, and pushed patches 1 to 5.
>>>
>>
>> I'm running into the assert:
>> https://sourceware.org/bugzilla/show_bug.cgi?id=28627 .
> 
> Sorry about that.  I've reverted this patch, and closed the bug.
> 

Np :)

Thanks for the quick fix.
- Tom

> The cause of the assert is fixed by patch #6 in this series, so I
> guess I'll remerge this patch after patch #6 lands.
> 
> Thanks,
> Andrew
> 

[PATCHv3 0/2] Improve 'maint set target-async off' for remote targets

[Andrew Burgess]

I pushed patches #1 to #5 from the original series, but it turned out
that the assert added in patch #5 would trigger in some tests.  I
checked my cached results and I definitiely wasn't seeing that assert
during my testing, so my original assumption was that patch #6 fixed
the cause of the assert.

However, upon retesting I found that this was not the case.  After
rebuilding my development branch, the assert was present.  But it
hadn't triggered during my testing.

I honestly have no idea what I originally tested - it clearly showed
the expected improvement for native-extended-remote, so it contained
some version of my work, but obviously not patch #5.

Which is why there's been a delay getting a new version of this patch
on the list, I've retested the remaining patches, then double checked.

The old patch #5 is now patch #2 in this series, and the old patch #6
is now patch #1.  Patch #1 contains a fix that allows the assert in
patch #2, that previously caused problems, to now be just fine.

Changes since v2:

  - Previous patches #1 to #4 are now committed,

  - Previous patch #5 is now patch #2,

  - Previous patch #6 is now patch #1,

  - Change in remote_target::push_stop_reply, the async event token is
    now only marked if the target _is_ in async mode, rather than if
    the target _can_ be in async mode.  When async mode is later
    enabled we were already marking the event token, so this should
    not be a change in most cases.  During remote_target::start_remote
    however, we push events, then process them without ever enabling
    async mode, this use case was what was previously triggering the
    assert in patch #2.

  - Code is otherwise unchanged.

Changes since v1:

  - The old patch #2 is removed, as Simon pointed out, this was just wrong.

  - The old patch #3, which was approved, is now patch #5, and is unchanged.

  - The old patch #4, is now patch #6, and still needs a review.

  - The old patch #1 has been split up into #1, #2, #3, and #4.  Given
    significant changes since V1 these should probably be reviewed
    afresh.
    
---

Andrew Burgess (2):
  gdb/remote: some fixes for 'maint set target-async off'
  gdb: add assert in remote_target::wait relating to async being off

 gdb/remote.c | 192 +++++++++++++++++++++++----------------------------
 1 file changed, 87 insertions(+), 105 deletions(-)

-- 
2.25.4

[PATCHv3 1/2] gdb/remote: some fixes for 'maint set target-async off'

[Andrew Burgess]

While working on another patch relating to remote targets, I wanted to
test with 'maint set target-async off' in place.  Unfortunately I ran
into some problems.  This commit is an attempt to fix one of the
issues I hit.

In my particular case I was actually running with:

  maint set target-async off
  maint set target-non-stop off

that is, we're telling GDB to force the targets to operate in
non-async mode, and in all-stop mode.  Here's my GDB session showing
the problem:

  (gdb) maintenance set target-async off
  (gdb) maintenance set target-non-stop off
  (gdb) target extended-remote :54321
  Remote debugging using :54321
  (gdb) attach 2365960
  Attaching to process 2365960
  No unwaited-for children left.
  (gdb)

Notice the 'No unwaited-for children left.' error, this is the
problem.  There's no reason why GDB should not be able to attach to
the process.

The problem is this:

  1. The user runs 'attach PID' and this sends GDB into attach_command
  in infcmd.c.  From here we call the ::attach method on the attach
  target, which will be the extended_remote_target.

  2. In extended_remote_target::attach, we attach to the remote target
  and get the first reply (which is a stop packet).  We put off
  processing the stop packet until the end of ::attach.  We setup the
  inferior and thread to represent the process we attached to, and
  download the target description.  Finally, we process the initial
  stop packet.

  If '!target_is_non_stop_p ()' and '!target_can_async_p ()', which is
  the case for us given the maintenance commands we used, we cache the
  stop packet within the remote_state::buf for later processing.

  3. Back in attach_command, if 'target_is_non_stop_p ()' then we
  request that the target stops.  This will either process any cached
  stop replies, or request that the target stops, and process the stop
  replies.  However, this code is not what we use due to non-stop mode
  being disabled.  So, we skip to the next step which is to call
  validate_exec_file.

  4. Calling validate_exec_file can cause packets to be sent to the
  remote target, and replies received, the first path I hit is the
  call to target_pid_to_exec_file, which calls
  remote_target::pid_to_exec_file, which can then try to read the
  executable from the remote.  Sending an receiving packets will make
  use of the remote_state::buf object.

  5. The attempt to attach continues, but the damage is already done...

So, the problem is that, in step #2 we cache a stop reply in the
remote_state::buf, and then in step #4 we reuse the remote_state::buf
object, discarding any cached stop reply.  As a result, the initial
stop, which is sent when GDB first attaches to the target, is lost.

This problem can clearly be seen I feel by looking at the
remote_state::cached_wait_status flag.  This flag tells GDB if there
is a wait status cached in remote_state::buf.  However, in
remote_target::putpkt_binary and remote_target::getpkt_or_notif_sane_1
this flag is just set back to 0, doing this immediately discards any
cached data.

I don't know if this scheme ever made sense, maybe once upon a time,
GDB would, when it found it had no cached stop, simply re-request the
stop information from the target, however, this certainly isn't the
case now, and resetting the cached_wait_status is (I claim) a bad
thing.

So, my first step toward fixing this issue was to replace the two
instances of 'rs->cached_wait_status = 0;' in ::putpkt_binary and
::getpkt_or_notif_sane_1 with 'gdb_assert (rs->cached_wait_status ==
0);', this, at least would show me when GDB was doing something
dangerous, and indeed, this assert is now hit in my test case above.

I did play with using some kind of scoped restore to backup, and
restore the remote_state::buf object in all the places within remote.c
that I was hitting where the ::buf was being corrupted.  The first
problem with this is that, where the ::cached_wait_status flag is
reset is _not_ where ::buf is corrupted.  For the ::putpkt_binary
case, by the time we get to the method the buffer has already been
corrupted in many cases, so we end up needing to add the scoped
save/restore within the callers, which means we need the save/restore
in _lots_ of places.

Plus, using this save/restore model feels like the wrong solution.  I
don't think that it's obvious that the buffer might be holding cached
data, and I think it would be too easy for new corruptions of the
buffer to be introduced, which could easily go unnoticed for a long
time.

So, I really wanted a solution that didn't require us to cache data in
the ::buf object.

Luckily, I think we already have such a solution in place, the
remote_state::stop_reply_queue, it seems like this does exactly the
same task, just in a slightly different way.  With the
::stop_reply_queue, the stop packets are processed upon receipt and
the stop_reply object is added to the queue.  With the ::buf cache
solution, the unprocessed stop reply is cached in the ::buf, and
processed later.

So, finally, in this commit, I propose to remove the
remote_state::cached_wait_status flag and to stop using the ::buf to
cache stop replies.  Instead, stop replies will now always be stored
in the ::stop_reply_queue.

There are two places where we use the ::buf to hold a cached stop
reply, the first is in the ::attach method, and the second is in
remote_target::start_remote, however, the second of these cases is far
less problematic, as after caching the stop reply in ::buf we call the
global start_remote function, which does very little work before
calling normal_stop, which processes the cached stop reply.  However,
my plan is to switch both users over to using ::stop_reply_queue so
that the old (unsafe) ::cached_wait_status mechanism can be completely
removed.

The next problem is that the ::stop_reply_queue is currently only used
for async-mode, and so, in remote_target::push_stop_reply, where we
push stop_reply objects into the ::stop_reply_queue, we currently also
mark the async event token.  I've modified this so we only mark the
async event token if 'target_is_async_p ()' - note, _is_, not _can_
here. The ::push_stop_reply method is called in places where async
mode has been temporarily disabled, but, when async mode is switched
back on (see remote_target::async) we will mark the event token if
there are events in the queue.

Another change of interest is in remote_target::remote_interrupt_as.
Previously this code checked ::cached_wait_status, but didn't check
for events in the ::stop_reply_queue.  Now that ::cached_wait_status
has been removed we now check the queue length instead, which should
have the same result.

Finally, in remote_target::wait_as, I've tried to merge the processing
of the ::stop_reply_queue with how we used to handle the
::cached_wait_status flag.

Currently, when processing the ::stop_reply_queue we call
process_stop_reply and immediately return.  However, when handling
::cached_wait_status we run through the whole of ::wait_as, and return
at the end of the function.

If we consider a standard stop packet, the two differences I see are:

  1. Resetting of the remote_state::waiting_for_stop_reply, flag; this
  is not currently done when processing a stop from the
  ::stop_reply_queue.

  2. The final return value has the possibility of being adjusted at
  the end of ::wait_as, as well as there being calls to
  record_currthread, non of which are done if we process a stop from
  the ::stop_reply_queue.

After this commit I have unified these two paths, that is, when we
process a packet from ::stop_reply_queue we now reset the flag
mentioned in #1 above, and pass through the return value adjustment
code at the end of ::wait_as.

An example of a test that reveals the benefits of this commit is:

  make check-gdb \
    RUNTESTFLAGS="--target_board=native-extended-gdbserver \
                  GDBFLAGS='-ex maint\ set\ target-async\ off \
                            -ex maint\ set\ target-non-stop\ off' \
                  gdb.base/attach.exp"

For testing I've been running test on x86-64/GNU Linux, and run with
target boards unix, native-gdbserver, and native-extended-gdbserver.
For each board I've run with the default GDBFLAGS, as well as with:

  GDBFLAGS='-ex maint\ set\ target-async\ off \
            -ex maint\ set\ target-non-stop\ off' \

Though running with the above GDBFLAGS is clearly a lot more unstable
both before and after my patch, I'm not seeing any consistent new
failures with my patch, except, with the native-extended-gdbserver
board, where I am seeing new failures, but only because more tests are
now running.  For that configuration alone I see the number of
unresolved go down by 49, the number of passes goes up by 446, and the
number of failures also increases by 144.  All of the failures are new
tests as far as I can tell.
---
 gdb/remote.c | 186 +++++++++++++++++++++++----------------------------
 1 file changed, 82 insertions(+), 104 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 25a4d3cab6e..b3890d71c59 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -258,15 +258,6 @@ class remote_state
      Otherwise zero, meaning to use the guessed size.  */
   long explicit_packet_size = 0;
 
-  /* remote_wait is normally called when the target is running and
-     waits for a stop reply packet.  But sometimes we need to call it
-     when the target is already stopped.  We can send a "?" packet
-     and have remote_wait read the response.  Or, if we already have
-     the response, we can stash it in BUF and tell remote_wait to
-     skip calling getpkt.  This flag is set when BUF contains a
-     stop reply packet and the target is not waiting.  */
-  int cached_wait_status = 0;
-
   /* True, if in no ack mode.  That is, neither GDB nor the stub will
      expect acks from each other.  The connection is assumed to be
      reliable.  */
@@ -4901,8 +4892,9 @@ remote_target::start_remote (int from_tty, int extended_p)
 
       /* Use the previously fetched status.  */
       gdb_assert (wait_status != NULL);
-      strcpy (rs->buf.data (), wait_status);
-      rs->cached_wait_status = 1;
+      struct notif_event *reply
+	= remote_notif_parse (this, &notif_client_stop, wait_status);
+      push_stop_reply ((struct stop_reply *) reply);
 
       ::start_remote (from_tty); /* Initialize gdb process mechanisms.  */
     }
@@ -5738,7 +5730,6 @@ remote_target::open_1 (const char *name, int from_tty, int extended_p)
   /* Reset the target state; these things will be queried either by
      remote_query_supported or as they are needed.  */
   reset_all_packet_configs_support ();
-  rs->cached_wait_status = 0;
   rs->explicit_packet_size = 0;
   rs->noack_mode = 0;
   rs->extended = extended_p;
@@ -6081,21 +6072,13 @@ extended_remote_target::attach (const char *args, int from_tty)
       /* Use the previously fetched status.  */
       gdb_assert (wait_status != NULL);
 
-      if (target_can_async_p ())
-	{
-	  struct notif_event *reply
-	    =  remote_notif_parse (this, &notif_client_stop, wait_status);
+      struct notif_event *reply
+	=  remote_notif_parse (this, &notif_client_stop, wait_status);
 
-	  push_stop_reply ((struct stop_reply *) reply);
+      push_stop_reply ((struct stop_reply *) reply);
 
-	  target_async (1);
-	}
-      else
-	{
-	  gdb_assert (wait_status != NULL);
-	  strcpy (rs->buf.data (), wait_status);
-	  rs->cached_wait_status = 1;
-	}
+      if (target_can_async_p ())
+	target_async (1);
     }
   else
     {
@@ -6998,9 +6981,9 @@ remote_target::remote_interrupt_as ()
   rs->ctrlc_pending_p = 1;
 
   /* If the inferior is stopped already, but the core didn't know
-     about it yet, just ignore the request.  The cached wait status
+     about it yet, just ignore the request.  The pending stop events
      will be collected in remote_wait.  */
-  if (rs->cached_wait_status)
+  if (stop_reply_queue_length () > 0)
     return;
 
   /* Send interrupt_sequence to remote target.  */
@@ -7429,7 +7412,7 @@ remote_target::queued_stop_reply (ptid_t ptid)
   remote_state *rs = get_remote_state ();
   struct stop_reply *r = remote_notif_remove_queued_reply (ptid);
 
-  if (!rs->stop_reply_queue.empty ())
+  if (!rs->stop_reply_queue.empty () && target_can_async_p ())
     {
       /* There's still at least an event left.  */
       mark_async_event_handler (rs->remote_async_inferior_event_token);
@@ -7454,7 +7437,12 @@ remote_target::push_stop_reply (struct stop_reply *new_event)
 			target_pid_to_str (new_event->ptid).c_str (),
 			int (rs->stop_reply_queue.size ()));
 
-  mark_async_event_handler (rs->remote_async_inferior_event_token);
+  /* Mark the pending event queue only if async mode is currently enabled.
+     If async mode is not currently enabled, then, if it later becomes
+     enabled, and there are events in this queue, we will mark the event
+     token at that point, see remote_target::async.  */
+  if (target_is_async_p ())
+    mark_async_event_handler (rs->remote_async_inferior_event_token);
 }
 
 /* Returns true if we have a stop reply for PTID.  */
@@ -8163,15 +8151,12 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status,
 
   stop_reply = queued_stop_reply (ptid);
   if (stop_reply != NULL)
-    return process_stop_reply (stop_reply, status);
-
-  if (rs->cached_wait_status)
-    /* Use the cached wait status, but only once.  */
-    rs->cached_wait_status = 0;
+    {
+      rs->waiting_for_stop_reply = 0;
+      event_ptid = process_stop_reply (stop_reply, status);
+    }
   else
     {
-      int ret;
-      int is_notif;
       int forever = ((options & TARGET_WNOHANG) == 0
 		     && rs->wait_forever_enabled_p);
 
@@ -8185,7 +8170,8 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status,
 	 _never_ wait for ever -> test on target_is_async_p().
 	 However, before we do that we need to ensure that the caller
 	 knows how to take the target into/out of async mode.  */
-      ret = getpkt_or_notif_sane (&rs->buf, forever, &is_notif);
+      int is_notif;
+      int ret = getpkt_or_notif_sane (&rs->buf, forever, &is_notif);
 
       /* GDB gets a notification.  Return to core as this event is
 	 not interesting.  */
@@ -8194,73 +8180,73 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status,
 
       if (ret == -1 && (options & TARGET_WNOHANG) != 0)
 	return minus_one_ptid;
-    }
 
-  buf = rs->buf.data ();
+      buf = rs->buf.data ();
 
-  /* Assume that the target has acknowledged Ctrl-C unless we receive
-     an 'F' or 'O' packet.  */
-  if (buf[0] != 'F' && buf[0] != 'O')
-    rs->ctrlc_pending_p = 0;
+      /* Assume that the target has acknowledged Ctrl-C unless we receive
+	 an 'F' or 'O' packet.  */
+      if (buf[0] != 'F' && buf[0] != 'O')
+	rs->ctrlc_pending_p = 0;
 
-  switch (buf[0])
-    {
-    case 'E':		/* Error of some sort.	*/
-      /* We're out of sync with the target now.  Did it continue or
-	 not?  Not is more likely, so report a stop.  */
-      rs->waiting_for_stop_reply = 0;
+      switch (buf[0])
+	{
+	case 'E':		/* Error of some sort.	*/
+	  /* We're out of sync with the target now.  Did it continue or
+	     not?  Not is more likely, so report a stop.  */
+	  rs->waiting_for_stop_reply = 0;
 
-      warning (_("Remote failure reply: %s"), buf);
-      status->set_stopped (GDB_SIGNAL_0);
-      break;
-    case 'F':		/* File-I/O request.  */
-      /* GDB may access the inferior memory while handling the File-I/O
-	 request, but we don't want GDB accessing memory while waiting
-	 for a stop reply.  See the comments in putpkt_binary.  Set
-	 waiting_for_stop_reply to 0 temporarily.  */
-      rs->waiting_for_stop_reply = 0;
-      remote_fileio_request (this, buf, rs->ctrlc_pending_p);
-      rs->ctrlc_pending_p = 0;
-      /* GDB handled the File-I/O request, and the target is running
-	 again.  Keep waiting for events.  */
-      rs->waiting_for_stop_reply = 1;
-      break;
-    case 'N': case 'T': case 'S': case 'X': case 'W':
-      {
-	/* There is a stop reply to handle.  */
-	rs->waiting_for_stop_reply = 0;
+	  warning (_("Remote failure reply: %s"), buf);
+	  status->set_stopped (GDB_SIGNAL_0);
+	  break;
+	case 'F':		/* File-I/O request.  */
+	  /* GDB may access the inferior memory while handling the File-I/O
+	     request, but we don't want GDB accessing memory while waiting
+	     for a stop reply.  See the comments in putpkt_binary.  Set
+	     waiting_for_stop_reply to 0 temporarily.  */
+	  rs->waiting_for_stop_reply = 0;
+	  remote_fileio_request (this, buf, rs->ctrlc_pending_p);
+	  rs->ctrlc_pending_p = 0;
+	  /* GDB handled the File-I/O request, and the target is running
+	     again.  Keep waiting for events.  */
+	  rs->waiting_for_stop_reply = 1;
+	  break;
+	case 'N': case 'T': case 'S': case 'X': case 'W':
+	  {
+	    /* There is a stop reply to handle.  */
+	    rs->waiting_for_stop_reply = 0;
 
-	stop_reply
-	  = (struct stop_reply *) remote_notif_parse (this,
-						      &notif_client_stop,
-						      rs->buf.data ());
+	    stop_reply
+	      = (struct stop_reply *) remote_notif_parse (this,
+							  &notif_client_stop,
+							  rs->buf.data ());
 
-	event_ptid = process_stop_reply (stop_reply, status);
-	break;
-      }
-    case 'O':		/* Console output.  */
-      remote_console_output (buf + 1);
-      break;
-    case '\0':
-      if (rs->last_sent_signal != GDB_SIGNAL_0)
-	{
-	  /* Zero length reply means that we tried 'S' or 'C' and the
-	     remote system doesn't support it.  */
-	  target_terminal::ours_for_output ();
-	  printf_filtered
-	    ("Can't send signals to this remote system.  %s not sent.\n",
-	     gdb_signal_to_name (rs->last_sent_signal));
-	  rs->last_sent_signal = GDB_SIGNAL_0;
-	  target_terminal::inferior ();
-
-	  strcpy (buf, rs->last_sent_step ? "s" : "c");
-	  putpkt (buf);
+	    event_ptid = process_stop_reply (stop_reply, status);
+	    break;
+	  }
+	case 'O':		/* Console output.  */
+	  remote_console_output (buf + 1);
+	  break;
+	case '\0':
+	  if (rs->last_sent_signal != GDB_SIGNAL_0)
+	    {
+	      /* Zero length reply means that we tried 'S' or 'C' and the
+		 remote system doesn't support it.  */
+	      target_terminal::ours_for_output ();
+	      printf_filtered
+		("Can't send signals to this remote system.  %s not sent.\n",
+		 gdb_signal_to_name (rs->last_sent_signal));
+	      rs->last_sent_signal = GDB_SIGNAL_0;
+	      target_terminal::inferior ();
+
+	      strcpy (buf, rs->last_sent_step ? "s" : "c");
+	      putpkt (buf);
+	      break;
+	    }
+	  /* fallthrough */
+	default:
+	  warning (_("Invalid remote reply: %s"), buf);
 	  break;
 	}
-      /* fallthrough */
-    default:
-      warning (_("Invalid remote reply: %s"), buf);
-      break;
     }
 
   if (status->kind () == TARGET_WAITKIND_NO_RESUMED)
@@ -9556,10 +9542,6 @@ remote_target::putpkt_binary (const char *buf, int cnt)
 	       "and then try again."));
     }
 
-  /* We're sending out a new packet.  Make sure we don't look at a
-     stale cached response.  */
-  rs->cached_wait_status = 0;
-
   /* Copy the packet into buffer BUF2, encapsulating it
      and giving it a checksum.  */
 
@@ -9897,10 +9879,6 @@ remote_target::getpkt_or_notif_sane_1 (gdb::char_vector *buf,
   int timeout;
   int val = -1;
 
-  /* We're reading a new response.  Make sure we don't look at a
-     previously cached response.  */
-  rs->cached_wait_status = 0;
-
   strcpy (buf->data (), "timeout");
 
   if (forever)
-- 
2.25.4

[PATCHv3 2/2] gdb: add assert in remote_target::wait relating to async being off

[Andrew Burgess]

While working on another patch I ended up in a situation where I had
async mode disabled (with 'maint set target-async off'), but the async
event token got marked anyway.

In this situation GDB was continually calling into
remote_target::wait, however, the async token would never become
unmarked as the unmarking is guarded by target_is_async_p.

We could just unconditionally unmark the token, but that would feel
like just ignoring a bug, so, instead, lets assert that if
!target_is_async_p, then the async token should not be marked.

This assertion would have caught my earlier mistake.

There should be no user visible changes with this commit.
---
 gdb/remote.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index b3890d71c59..9b814d54313 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -8295,9 +8295,13 @@ remote_target::wait (ptid_t ptid, struct target_waitstatus *status,
   remote_state *rs = get_remote_state ();
 
   /* Start by clearing the flag that asks for our wait method to be called,
-     we'll mark it again at the end if needed.  */
+     we'll mark it again at the end if needed.  If the target is not in
+     async mode then the async token should not be marked.  */
   if (target_is_async_p ())
     clear_async_event_handler (rs->remote_async_inferior_event_token);
+  else
+    gdb_assert (!async_event_handler_marked
+		(rs->remote_async_inferior_event_token));
 
   ptid_t event_ptid;
 
-- 
2.25.4

PING: [PATCHv3 0/2] Improve 'maint set target-async off' for remote targets

[Andrew Burgess]

Ping!

Thanks,
Andrew

* Andrew Burgess <aburgess@redhat.com> [2021-12-01 10:40:21 +0000]:

> I pushed patches #1 to #5 from the original series, but it turned out
> that the assert added in patch #5 would trigger in some tests.  I
> checked my cached results and I definitiely wasn't seeing that assert
> during my testing, so my original assumption was that patch #6 fixed
> the cause of the assert.
> 
> However, upon retesting I found that this was not the case.  After
> rebuilding my development branch, the assert was present.  But it
> hadn't triggered during my testing.
> 
> I honestly have no idea what I originally tested - it clearly showed
> the expected improvement for native-extended-remote, so it contained
> some version of my work, but obviously not patch #5.
> 
> Which is why there's been a delay getting a new version of this patch
> on the list, I've retested the remaining patches, then double checked.
> 
> The old patch #5 is now patch #2 in this series, and the old patch #6
> is now patch #1.  Patch #1 contains a fix that allows the assert in
> patch #2, that previously caused problems, to now be just fine.
> 
> Changes since v2:
> 
>   - Previous patches #1 to #4 are now committed,
> 
>   - Previous patch #5 is now patch #2,
> 
>   - Previous patch #6 is now patch #1,
> 
>   - Change in remote_target::push_stop_reply, the async event token is
>     now only marked if the target _is_ in async mode, rather than if
>     the target _can_ be in async mode.  When async mode is later
>     enabled we were already marking the event token, so this should
>     not be a change in most cases.  During remote_target::start_remote
>     however, we push events, then process them without ever enabling
>     async mode, this use case was what was previously triggering the
>     assert in patch #2.
> 
>   - Code is otherwise unchanged.
> 
> Changes since v1:
> 
>   - The old patch #2 is removed, as Simon pointed out, this was just wrong.
> 
>   - The old patch #3, which was approved, is now patch #5, and is unchanged.
> 
>   - The old patch #4, is now patch #6, and still needs a review.
> 
>   - The old patch #1 has been split up into #1, #2, #3, and #4.  Given
>     significant changes since V1 these should probably be reviewed
>     afresh.
>     
> ---
> 
> Andrew Burgess (2):
>   gdb/remote: some fixes for 'maint set target-async off'
>   gdb: add assert in remote_target::wait relating to async being off
> 
>  gdb/remote.c | 192 +++++++++++++++++++++++----------------------------
>  1 file changed, 87 insertions(+), 105 deletions(-)
> 
> -- 
> 2.25.4
> 

Re: [PATCHv3 0/2] Improve 'maint set target-async off' for remote targets

[John Baldwin]

On 12/1/21 2:40 AM, Andrew Burgess via Gdb-patches wrote:
> I pushed patches #1 to #5 from the original series, but it turned out
> that the assert added in patch #5 would trigger in some tests.  I
> checked my cached results and I definitiely wasn't seeing that assert
> during my testing, so my original assumption was that patch #6 fixed
> the cause of the assert.
> 
> However, upon retesting I found that this was not the case.  After
> rebuilding my development branch, the assert was present.  But it
> hadn't triggered during my testing.
> 
> I honestly have no idea what I originally tested - it clearly showed
> the expected improvement for native-extended-remote, so it contained
> some version of my work, but obviously not patch #5.
> 
> Which is why there's been a delay getting a new version of this patch
> on the list, I've retested the remaining patches, then double checked.
> 
> The old patch #5 is now patch #2 in this series, and the old patch #6
> is now patch #1.  Patch #1 contains a fix that allows the assert in
> patch #2, that previously caused problems, to now be just fine.
> 
> Changes since v2:
> 
>    - Previous patches #1 to #4 are now committed,
> 
>    - Previous patch #5 is now patch #2,
> 
>    - Previous patch #6 is now patch #1,
> 
>    - Change in remote_target::push_stop_reply, the async event token is
>      now only marked if the target _is_ in async mode, rather than if
>      the target _can_ be in async mode.  When async mode is later
>      enabled we were already marking the event token, so this should
>      not be a change in most cases.  During remote_target::start_remote
>      however, we push events, then process them without ever enabling
>      async mode, this use case was what was previously triggering the
>      assert in patch #2.
> 
>    - Code is otherwise unchanged.
> 
> Changes since v1:
> 
>    - The old patch #2 is removed, as Simon pointed out, this was just wrong.
> 
>    - The old patch #3, which was approved, is now patch #5, and is unchanged.
> 
>    - The old patch #4, is now patch #6, and still needs a review.
> 
>    - The old patch #1 has been split up into #1, #2, #3, and #4.  Given
>      significant changes since V1 these should probably be reviewed
>      afresh.
>      
> ---
> 
> Andrew Burgess (2):
>    gdb/remote: some fixes for 'maint set target-async off'
>    gdb: add assert in remote_target::wait relating to async being off
> 
>   gdb/remote.c | 192 +++++++++++++++++++++++----------------------------
>   1 file changed, 87 insertions(+), 105 deletions(-)

Patch 2 is straightforward and seems obviously correct to me.  I think Patch 1
also looks ok, and the description sounds sensible.  I'm just not super familiar
enough with the remote target in general to give a detailed review.  I do think
storing the "parsed" stop reply state in an existing queue is probably cleaner
overall (and my guess is the queue was added later and this cached reply just
wasn't converted to use the queue at the time?)

-- 
John Baldwin

Re: [PATCHv3 1/2] gdb/remote: some fixes for 'maint set target-async off'

[Pedro Alves]

On 2021-12-01 10:40, Andrew Burgess via Gdb-patches wrote:

> This problem can clearly be seen I feel by looking at the
> remote_state::cached_wait_status flag.  This flag tells GDB if there
> is a wait status cached in remote_state::buf.  However, in
> remote_target::putpkt_binary and remote_target::getpkt_or_notif_sane_1
> this flag is just set back to 0, doing this immediately discards any
> cached data.
> 
> I don't know if this scheme ever made sense, maybe once upon a time,
> GDB would, when it found it had no cached stop, simply re-request the
> stop information from the target, however, this certainly isn't the
> case now, and resetting the cached_wait_status is (I claim) a bad
> thing.

I don't think that was ever the case.  Take a look at 2d717e4f8a54,
where the cached status was introduced to handle "attach".  It was simply the case
back then that nothing would talk to the target between the initial attach
and consuming the event.  It's not clear to me why putpkt/getpkt would
need to clear the flag back then.  Looks more like a "just in case" safeguard.

> So, finally, in this commit, I propose to remove the
> remote_state::cached_wait_status flag and to stop using the ::buf to
> cache stop replies.  Instead, stop replies will now always be stored
> in the ::stop_reply_queue.

To be honest, I don't recall exactly why I didn't do that when introducing
the stop reply queue.

> @@ -8163,15 +8151,12 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status,
>  
>    stop_reply = queued_stop_reply (ptid);
>    if (stop_reply != NULL)
> -    return process_stop_reply (stop_reply, status);
> -
> -  if (rs->cached_wait_status)
> -    /* Use the cached wait status, but only once.  */
> -    rs->cached_wait_status = 0;
> +    {
> +      rs->waiting_for_stop_reply = 0;

This is a difference described in the commit log, but looking at the resulting code,
I don't understand why clearing this flag is needed here, it looks like dead code to me.
I mean, if we have a cached status already, then we're not waiting for a stop reply
from the target.  Did you run into a case where it was needed?

Otherwise the patch LGTM.  Thanks for doing this.

Re: [PATCHv3 2/2] gdb: add assert in remote_target::wait relating to async being off

[Pedro Alves]

On 2021-12-01 10:40, Andrew Burgess via Gdb-patches wrote:
> While working on another patch I ended up in a situation where I had
> async mode disabled (with 'maint set target-async off'), but the async
> event token got marked anyway.
> 
> In this situation GDB was continually calling into
> remote_target::wait, however, the async token would never become
> unmarked as the unmarking is guarded by target_is_async_p.
> 
> We could just unconditionally unmark the token, but that would feel
> like just ignoring a bug, so, instead, lets assert that if
> !target_is_async_p, then the async token should not be marked.
> 
> This assertion would have caught my earlier mistake.
> 
> There should be no user visible changes with this commit.
> ---
>  gdb/remote.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index b3890d71c59..9b814d54313 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -8295,9 +8295,13 @@ remote_target::wait (ptid_t ptid, struct target_waitstatus *status,
>    remote_state *rs = get_remote_state ();
>  
>    /* Start by clearing the flag that asks for our wait method to be called,
> -     we'll mark it again at the end if needed.  */
> +     we'll mark it again at the end if needed.  If the target is not in
> +     async mode then the async token should not be marked.  */
>    if (target_is_async_p ())
>      clear_async_event_handler (rs->remote_async_inferior_event_token);
> +  else
> +    gdb_assert (!async_event_handler_marked
> +		(rs->remote_async_inferior_event_token));
>  
>    ptid_t event_ptid;
>  

LGTM.

Re: [PATCHv3 1/2] gdb/remote: some fixes for 'maint set target-async off'

[Andrew Burgess]

* Pedro Alves <pedro@palves.net> [2021-12-16 21:15:31 +0000]:

> On 2021-12-01 10:40, Andrew Burgess via Gdb-patches wrote:
> 
> > This problem can clearly be seen I feel by looking at the
> > remote_state::cached_wait_status flag.  This flag tells GDB if there
> > is a wait status cached in remote_state::buf.  However, in
> > remote_target::putpkt_binary and remote_target::getpkt_or_notif_sane_1
> > this flag is just set back to 0, doing this immediately discards any
> > cached data.
> > 
> > I don't know if this scheme ever made sense, maybe once upon a time,
> > GDB would, when it found it had no cached stop, simply re-request the
> > stop information from the target, however, this certainly isn't the
> > case now, and resetting the cached_wait_status is (I claim) a bad
> > thing.
> 
> I don't think that was ever the case.  Take a look at 2d717e4f8a54,
> where the cached status was introduced to handle "attach".  It was simply the case
> back then that nothing would talk to the target between the initial attach
> and consuming the event.  It's not clear to me why putpkt/getpkt would
> need to clear the flag back then.  Looks more like a "just in case" safeguard.

Thanks for this insight.  I've updated the commit message to try and
describe the history here a little more accurately, including adding
the commit SHA you gave above, which should help if anyone needs to
dig into this code in the future.

> 
> > So, finally, in this commit, I propose to remove the
> > remote_state::cached_wait_status flag and to stop using the ::buf to
> > cache stop replies.  Instead, stop replies will now always be stored
> > in the ::stop_reply_queue.
> 
> To be honest, I don't recall exactly why I didn't do that when introducing
> the stop reply queue.
> 
> > @@ -8163,15 +8151,12 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status,
> >  
> >    stop_reply = queued_stop_reply (ptid);
> >    if (stop_reply != NULL)
> > -    return process_stop_reply (stop_reply, status);
> > -
> > -  if (rs->cached_wait_status)
> > -    /* Use the cached wait status, but only once.  */
> > -    rs->cached_wait_status = 0;
> > +    {
> > +      rs->waiting_for_stop_reply = 0;
> 
> This is a difference described in the commit log, but looking at the resulting code,
> I don't understand why clearing this flag is needed here, it looks like dead code to me.
> I mean, if we have a cached status already, then we're not waiting for a stop reply
> from the target.  Did you run into a case where it was needed?

No I never hit a case where it was definitely needed.  Honestly, I
just looked at the different code paths, and saw that it was pretty
easy to merge the paths and carry out all the actions, so did that.

However, you're right to call me out on that.  It's exactly this sort
of "just in case" code that was causing the cached packets to be
discarded originally without warning.

So, I've changed the unconditional "set flag to false" with an assert,
and a comment.  If it turns out we are wrong in our understanding of
the situation, then hopefully, the problem will get reported, and we
can figure out what's going on at that point!

Below is the updated patch.  The only code change is the assert I
mention above.  All other changes are in the commit message.

I'll give this a few days in case you want to follow up, then I'll
push this.

Thanks,
Andrew

---

commit 723053fbcc4e89467d95b28ee317c3d4d5431da1
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Wed Nov 17 17:17:37 2021 +0000

    gdb/remote: some fixes for 'maint set target-async off'
    
    While working on another patch relating to remote targets, I wanted to
    test with 'maint set target-async off' in place.  Unfortunately I ran
    into some problems.  This commit is an attempt to fix one of the
    issues I hit.
    
    In my particular case I was actually running with:
    
      maint set target-async off
      maint set target-non-stop off
    
    that is, we're telling GDB to force the targets to operate in
    non-async mode, and in all-stop mode.  Here's my GDB session showing
    the problem:
    
      (gdb) maintenance set target-async off
      (gdb) maintenance set target-non-stop off
      (gdb) target extended-remote :54321
      Remote debugging using :54321
      (gdb) attach 2365960
      Attaching to process 2365960
      No unwaited-for children left.
      (gdb)
    
    Notice the 'No unwaited-for children left.' error, this is the
    problem.  There's no reason why GDB should not be able to attach to
    the process.
    
    The problem is this:
    
      1. The user runs 'attach PID' and this sends GDB into attach_command
      in infcmd.c.  From here we call the ::attach method on the attach
      target, which will be the extended_remote_target.
    
      2. In extended_remote_target::attach, we attach to the remote target
      and get the first reply (which is a stop packet).  We put off
      processing the stop packet until the end of ::attach.  We setup the
      inferior and thread to represent the process we attached to, and
      download the target description.  Finally, we process the initial
      stop packet.
    
      If '!target_is_non_stop_p ()' and '!target_can_async_p ()', which is
      the case for us given the maintenance commands we used, we cache the
      stop packet within the remote_state::buf for later processing.
    
      3. Back in attach_command, if 'target_is_non_stop_p ()' then we
      request that the target stops.  This will either process any cached
      stop replies, or request that the target stops, and process the stop
      replies.  However, this code is not what we use due to non-stop mode
      being disabled.  So, we skip to the next step which is to call
      validate_exec_file.
    
      4. Calling validate_exec_file can cause packets to be sent to the
      remote target, and replies received, the first path I hit is the
      call to target_pid_to_exec_file, which calls
      remote_target::pid_to_exec_file, which can then try to read the
      executable from the remote.  Sending an receiving packets will make
      use of the remote_state::buf object.
    
      5. The attempt to attach continues, but the damage is already done...
    
    So, the problem is that, in step #2 we cache a stop reply in the
    remote_state::buf, and then in step #4 we reuse the remote_state::buf
    object, discarding any cached stop reply.  As a result, the initial
    stop, which is sent when GDB first attaches to the target, is lost.
    
    This problem can clearly be seen, I feel, by looking at the
    remote_state::cached_wait_status flag.  This flag tells GDB if there
    is a wait status cached in remote_state::buf.  However, in
    remote_target::putpkt_binary and remote_target::getpkt_or_notif_sane_1
    this flag is just set back to 0, doing this immediately discards any
    cached data.
    
    I don't know if this scheme ever made sense,  looking at commit
    2d717e4f8a54, where the cached_wait_status flag was added, it appears
    that there was nothing between where the stop was cached, and where
    the stop was consumed, so, I suspect, there never was a situation
    where we ended up in putpkt_binary or getpkt_or_notif_sane_1 and
    needed to clear to the flag, maybe the clearing was added "just in
    case".  Whatever the history, I claim that this clearing this flag is
    no longer a good idea.
    
    So, my first step toward fixing this issue was to replace the two
    instances of 'rs->cached_wait_status = 0;' in ::putpkt_binary and
    ::getpkt_or_notif_sane_1 with 'gdb_assert (rs->cached_wait_status ==
    0);', this, at least would show me when GDB was doing something
    dangerous, and indeed, this assert is now hit in my test case above.
    
    I did play with using some kind of scoped restore to backup, and
    restore the remote_state::buf object in all the places within remote.c
    that I was hitting where the ::buf was being corrupted.  The first
    problem with this is that, where the ::cached_wait_status flag is
    reset is _not_ where ::buf is corrupted.  For the ::putpkt_binary
    case, by the time we get to the method the buffer has already been
    corrupted in many cases, so we end up needing to add the scoped
    save/restore within the callers, which means we need the save/restore
    in _lots_ of places.
    
    Plus, using this save/restore model feels like the wrong solution.  I
    don't think that it's obvious that the buffer might be holding cached
    data, and I think it would be too easy for new corruptions of the
    buffer to be introduced, which could easily go unnoticed for a long
    time.
    
    So, I really wanted a solution that didn't require us to cache data in
    the ::buf object.
    
    Luckily, I think we already have such a solution in place, the
    remote_state::stop_reply_queue, it seems like this does exactly the
    same task, just in a slightly different way.  With the
    ::stop_reply_queue, the stop packets are processed upon receipt and
    the stop_reply object is added to the queue.  With the ::buf cache
    solution, the unprocessed stop reply is cached in the ::buf, and
    processed later.
    
    So, finally, in this commit, I propose to remove the
    remote_state::cached_wait_status flag and to stop using the ::buf to
    cache stop replies.  Instead, stop replies will now always be stored
    in the ::stop_reply_queue.
    
    There are two places where we use the ::buf to hold a cached stop
    reply, the first is in the ::attach method, and the second is in
    remote_target::start_remote, however, the second of these cases is far
    less problematic, as after caching the stop reply in ::buf we call the
    global start_remote function, which does very little work before
    calling normal_stop, which processes the cached stop reply.  However,
    my plan is to switch both users over to using ::stop_reply_queue so
    that the old (unsafe) ::cached_wait_status mechanism can be completely
    removed.
    
    The next problem is that the ::stop_reply_queue is currently only used
    for async-mode, and so, in remote_target::push_stop_reply, where we
    push stop_reply objects into the ::stop_reply_queue, we currently also
    mark the async event token.  I've modified this so we only mark the
    async event token if 'target_is_async_p ()' - note, _is_, not _can_
    here. The ::push_stop_reply method is called in places where async
    mode has been temporarily disabled, but, when async mode is switched
    back on (see remote_target::async) we will mark the event token if
    there are events in the queue.
    
    Another change of interest is in remote_target::remote_interrupt_as.
    Previously this code checked ::cached_wait_status, but didn't check
    for events in the ::stop_reply_queue.  Now that ::cached_wait_status
    has been removed we now check the queue length instead, which should
    have the same result.
    
    Finally, in remote_target::wait_as, I've tried to merge the processing
    of the ::stop_reply_queue with how we used to handle the
    ::cached_wait_status flag.
    
    Currently, when processing the ::stop_reply_queue we call
    process_stop_reply and immediately return.  However, when handling
    ::cached_wait_status we run through the whole of ::wait_as, and return
    at the end of the function.
    
    If we consider a standard stop packet, the two differences I see are:
    
      1. Resetting of the remote_state::waiting_for_stop_reply, flag; this
      is not currently done when processing a stop from the
      ::stop_reply_queue.
    
      2. The final return value has the possibility of being adjusted at
      the end of ::wait_as, as well as there being calls to
      record_currthread, non of which are done if we process a stop from
      the ::stop_reply_queue.
    
    After discussion on the mailing list:
    
      https://sourceware.org/pipermail/gdb-patches/2021-December/184535.html
    
    it was suggested that, when an event is pushed into the
    ::stop_reply_queue, the ::waiting_for_stop_reply flag is never going
    to be set.  As a result, we don't need to worry about the first
    difference.  I have however, added a gdb_assert to validate the
    assumption that the flag is never going to be set.  If in future the
    situation ever changes, then we should find out pretty quickly.
    
    As for the second difference, I have resolved this by having all stop
    packets taken from the ::stop_reply_queue, pass through the return
    value adjustment code at the end of ::wait_as.
    
    An example of a test that reveals the benefits of this commit is:
    
      make check-gdb \
        RUNTESTFLAGS="--target_board=native-extended-gdbserver \
                      GDBFLAGS='-ex maint\ set\ target-async\ off \
                                -ex maint\ set\ target-non-stop\ off' \
                      gdb.base/attach.exp"
    
    For testing I've been running test on x86-64/GNU Linux, and run with
    target boards unix, native-gdbserver, and native-extended-gdbserver.
    For each board I've run with the default GDBFLAGS, as well as with:
    
      GDBFLAGS='-ex maint\ set\ target-async\ off \
                -ex maint\ set\ target-non-stop\ off' \
    
    Though running with the above GDBFLAGS is clearly a lot more unstable
    both before and after my patch, I'm not seeing any consistent new
    failures with my patch, except, with the native-extended-gdbserver
    board, where I am seeing new failures, but only because more tests are
    now running.  For that configuration alone I see the number of
    unresolved go down by 49, the number of passes goes up by 446, and the
    number of failures also increases by 144.  All of the failures are new
    tests as far as I can tell.

diff --git a/gdb/remote.c b/gdb/remote.c
index 1f977d57fba..74770649d9c 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -258,15 +258,6 @@ class remote_state
      Otherwise zero, meaning to use the guessed size.  */
   long explicit_packet_size = 0;
 
-  /* remote_wait is normally called when the target is running and
-     waits for a stop reply packet.  But sometimes we need to call it
-     when the target is already stopped.  We can send a "?" packet
-     and have remote_wait read the response.  Or, if we already have
-     the response, we can stash it in BUF and tell remote_wait to
-     skip calling getpkt.  This flag is set when BUF contains a
-     stop reply packet and the target is not waiting.  */
-  int cached_wait_status = 0;
-
   /* True, if in no ack mode.  That is, neither GDB nor the stub will
      expect acks from each other.  The connection is assumed to be
      reliable.  */
@@ -4969,8 +4960,9 @@ remote_target::start_remote_1 (int from_tty, int extended_p)
 
       /* Use the previously fetched status.  */
       gdb_assert (wait_status != NULL);
-      strcpy (rs->buf.data (), wait_status);
-      rs->cached_wait_status = 1;
+      struct notif_event *reply
+	= remote_notif_parse (this, &notif_client_stop, wait_status);
+      push_stop_reply ((struct stop_reply *) reply);
 
       ::start_remote (from_tty); /* Initialize gdb process mechanisms.  */
     }
@@ -5804,7 +5796,6 @@ remote_target::open_1 (const char *name, int from_tty, int extended_p)
   /* Reset the target state; these things will be queried either by
      remote_query_supported or as they are needed.  */
   reset_all_packet_configs_support ();
-  rs->cached_wait_status = 0;
   rs->explicit_packet_size = 0;
   rs->noack_mode = 0;
   rs->extended = extended_p;
@@ -6199,21 +6190,13 @@ extended_remote_target::attach (const char *args, int from_tty)
       /* Use the previously fetched status.  */
       gdb_assert (wait_status != NULL);
 
-      if (target_can_async_p ())
-	{
-	  struct notif_event *reply
-	    =  remote_notif_parse (this, &notif_client_stop, wait_status);
+      struct notif_event *reply
+	=  remote_notif_parse (this, &notif_client_stop, wait_status);
 
-	  push_stop_reply ((struct stop_reply *) reply);
+      push_stop_reply ((struct stop_reply *) reply);
 
-	  target_async (1);
-	}
-      else
-	{
-	  gdb_assert (wait_status != NULL);
-	  strcpy (rs->buf.data (), wait_status);
-	  rs->cached_wait_status = 1;
-	}
+      if (target_can_async_p ())
+	target_async (1);
     }
   else
     {
@@ -7084,9 +7067,9 @@ remote_target::remote_interrupt_as ()
   rs->ctrlc_pending_p = 1;
 
   /* If the inferior is stopped already, but the core didn't know
-     about it yet, just ignore the request.  The cached wait status
+     about it yet, just ignore the request.  The pending stop events
      will be collected in remote_wait.  */
-  if (rs->cached_wait_status)
+  if (stop_reply_queue_length () > 0)
     return;
 
   /* Send interrupt_sequence to remote target.  */
@@ -7480,7 +7463,7 @@ remote_target::queued_stop_reply (ptid_t ptid)
   remote_state *rs = get_remote_state ();
   struct stop_reply *r = remote_notif_remove_queued_reply (ptid);
 
-  if (!rs->stop_reply_queue.empty ())
+  if (!rs->stop_reply_queue.empty () && target_can_async_p ())
     {
       /* There's still at least an event left.  */
       mark_async_event_handler (rs->remote_async_inferior_event_token);
@@ -7505,7 +7488,12 @@ remote_target::push_stop_reply (struct stop_reply *new_event)
 			target_pid_to_str (new_event->ptid).c_str (),
 			int (rs->stop_reply_queue.size ()));
 
-  mark_async_event_handler (rs->remote_async_inferior_event_token);
+  /* Mark the pending event queue only if async mode is currently enabled.
+     If async mode is not currently enabled, then, if it later becomes
+     enabled, and there are events in this queue, we will mark the event
+     token at that point, see remote_target::async.  */
+  if (target_is_async_p ())
+    mark_async_event_handler (rs->remote_async_inferior_event_token);
 }
 
 /* Returns true if we have a stop reply for PTID.  */
@@ -8214,15 +8202,14 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status,
 
   stop_reply = queued_stop_reply (ptid);
   if (stop_reply != NULL)
-    return process_stop_reply (stop_reply, status);
-
-  if (rs->cached_wait_status)
-    /* Use the cached wait status, but only once.  */
-    rs->cached_wait_status = 0;
+    {
+      /* Currently non of the paths that push a stop reply onto the queue
+	 will have set the waiting_for_stop_reply flag.  */
+      gdb_assert (!rs->waiting_for_stop_reply);
+      event_ptid = process_stop_reply (stop_reply, status);
+    }
   else
     {
-      int ret;
-      int is_notif;
       int forever = ((options & TARGET_WNOHANG) == 0
 		     && rs->wait_forever_enabled_p);
 
@@ -8236,7 +8223,8 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status,
 	 _never_ wait for ever -> test on target_is_async_p().
 	 However, before we do that we need to ensure that the caller
 	 knows how to take the target into/out of async mode.  */
-      ret = getpkt_or_notif_sane (&rs->buf, forever, &is_notif);
+      int is_notif;
+      int ret = getpkt_or_notif_sane (&rs->buf, forever, &is_notif);
 
       /* GDB gets a notification.  Return to core as this event is
 	 not interesting.  */
@@ -8245,73 +8233,73 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status,
 
       if (ret == -1 && (options & TARGET_WNOHANG) != 0)
 	return minus_one_ptid;
-    }
 
-  buf = rs->buf.data ();
+      buf = rs->buf.data ();
 
-  /* Assume that the target has acknowledged Ctrl-C unless we receive
-     an 'F' or 'O' packet.  */
-  if (buf[0] != 'F' && buf[0] != 'O')
-    rs->ctrlc_pending_p = 0;
+      /* Assume that the target has acknowledged Ctrl-C unless we receive
+	 an 'F' or 'O' packet.  */
+      if (buf[0] != 'F' && buf[0] != 'O')
+	rs->ctrlc_pending_p = 0;
 
-  switch (buf[0])
-    {
-    case 'E':		/* Error of some sort.	*/
-      /* We're out of sync with the target now.  Did it continue or
-	 not?  Not is more likely, so report a stop.  */
-      rs->waiting_for_stop_reply = 0;
+      switch (buf[0])
+	{
+	case 'E':		/* Error of some sort.	*/
+	  /* We're out of sync with the target now.  Did it continue or
+	     not?  Not is more likely, so report a stop.  */
+	  rs->waiting_for_stop_reply = 0;
 
-      warning (_("Remote failure reply: %s"), buf);
-      status->set_stopped (GDB_SIGNAL_0);
-      break;
-    case 'F':		/* File-I/O request.  */
-      /* GDB may access the inferior memory while handling the File-I/O
-	 request, but we don't want GDB accessing memory while waiting
-	 for a stop reply.  See the comments in putpkt_binary.  Set
-	 waiting_for_stop_reply to 0 temporarily.  */
-      rs->waiting_for_stop_reply = 0;
-      remote_fileio_request (this, buf, rs->ctrlc_pending_p);
-      rs->ctrlc_pending_p = 0;
-      /* GDB handled the File-I/O request, and the target is running
-	 again.  Keep waiting for events.  */
-      rs->waiting_for_stop_reply = 1;
-      break;
-    case 'N': case 'T': case 'S': case 'X': case 'W':
-      {
-	/* There is a stop reply to handle.  */
-	rs->waiting_for_stop_reply = 0;
+	  warning (_("Remote failure reply: %s"), buf);
+	  status->set_stopped (GDB_SIGNAL_0);
+	  break;
+	case 'F':		/* File-I/O request.  */
+	  /* GDB may access the inferior memory while handling the File-I/O
+	     request, but we don't want GDB accessing memory while waiting
+	     for a stop reply.  See the comments in putpkt_binary.  Set
+	     waiting_for_stop_reply to 0 temporarily.  */
+	  rs->waiting_for_stop_reply = 0;
+	  remote_fileio_request (this, buf, rs->ctrlc_pending_p);
+	  rs->ctrlc_pending_p = 0;
+	  /* GDB handled the File-I/O request, and the target is running
+	     again.  Keep waiting for events.  */
+	  rs->waiting_for_stop_reply = 1;
+	  break;
+	case 'N': case 'T': case 'S': case 'X': case 'W':
+	  {
+	    /* There is a stop reply to handle.  */
+	    rs->waiting_for_stop_reply = 0;
 
-	stop_reply
-	  = (struct stop_reply *) remote_notif_parse (this,
-						      &notif_client_stop,
-						      rs->buf.data ());
+	    stop_reply
+	      = (struct stop_reply *) remote_notif_parse (this,
+							  &notif_client_stop,
+							  rs->buf.data ());
 
-	event_ptid = process_stop_reply (stop_reply, status);
-	break;
-      }
-    case 'O':		/* Console output.  */
-      remote_console_output (buf + 1);
-      break;
-    case '\0':
-      if (rs->last_sent_signal != GDB_SIGNAL_0)
-	{
-	  /* Zero length reply means that we tried 'S' or 'C' and the
-	     remote system doesn't support it.  */
-	  target_terminal::ours_for_output ();
-	  printf_filtered
-	    ("Can't send signals to this remote system.  %s not sent.\n",
-	     gdb_signal_to_name (rs->last_sent_signal));
-	  rs->last_sent_signal = GDB_SIGNAL_0;
-	  target_terminal::inferior ();
-
-	  strcpy (buf, rs->last_sent_step ? "s" : "c");
-	  putpkt (buf);
+	    event_ptid = process_stop_reply (stop_reply, status);
+	    break;
+	  }
+	case 'O':		/* Console output.  */
+	  remote_console_output (buf + 1);
+	  break;
+	case '\0':
+	  if (rs->last_sent_signal != GDB_SIGNAL_0)
+	    {
+	      /* Zero length reply means that we tried 'S' or 'C' and the
+		 remote system doesn't support it.  */
+	      target_terminal::ours_for_output ();
+	      printf_filtered
+		("Can't send signals to this remote system.  %s not sent.\n",
+		 gdb_signal_to_name (rs->last_sent_signal));
+	      rs->last_sent_signal = GDB_SIGNAL_0;
+	      target_terminal::inferior ();
+
+	      strcpy (buf, rs->last_sent_step ? "s" : "c");
+	      putpkt (buf);
+	      break;
+	    }
+	  /* fallthrough */
+	default:
+	  warning (_("Invalid remote reply: %s"), buf);
 	  break;
 	}
-      /* fallthrough */
-    default:
-      warning (_("Invalid remote reply: %s"), buf);
-      break;
     }
 
   if (status->kind () == TARGET_WAITKIND_NO_RESUMED)
@@ -9596,10 +9584,6 @@ remote_target::putpkt_binary (const char *buf, int cnt)
 	       "and then try again."));
     }
 
-  /* We're sending out a new packet.  Make sure we don't look at a
-     stale cached response.  */
-  rs->cached_wait_status = 0;
-
   /* Copy the packet into buffer BUF2, encapsulating it
      and giving it a checksum.  */
 
@@ -9937,10 +9921,6 @@ remote_target::getpkt_or_notif_sane_1 (gdb::char_vector *buf,
   int timeout;
   int val = -1;
 
-  /* We're reading a new response.  Make sure we don't look at a
-     previously cached response.  */
-  rs->cached_wait_status = 0;
-
   strcpy (buf->data (), "timeout");
 
   if (forever)

Re: [PATCHv3 1/2] gdb/remote: some fixes for 'maint set target-async off'

[Pedro Alves]

On 2021-12-17 13:35, Andrew Burgess wrote:
> * Pedro Alves <pedro@palves.net> [2021-12-16 21:15:31 +0000]:
> 
>> On 2021-12-01 10:40, Andrew Burgess via Gdb-patches wrote:
>>
>>> This problem can clearly be seen I feel by looking at the
>>> remote_state::cached_wait_status flag.  This flag tells GDB if there
>>> is a wait status cached in remote_state::buf.  However, in
>>> remote_target::putpkt_binary and remote_target::getpkt_or_notif_sane_1
>>> this flag is just set back to 0, doing this immediately discards any
>>> cached data.
>>>
>>> I don't know if this scheme ever made sense, maybe once upon a time,
>>> GDB would, when it found it had no cached stop, simply re-request the
>>> stop information from the target, however, this certainly isn't the
>>> case now, and resetting the cached_wait_status is (I claim) a bad
>>> thing.
>>
>> I don't think that was ever the case.  Take a look at 2d717e4f8a54,
>> where the cached status was introduced to handle "attach".  It was simply the case
>> back then that nothing would talk to the target between the initial attach
>> and consuming the event.  It's not clear to me why putpkt/getpkt would
>> need to clear the flag back then.  Looks more like a "just in case" safeguard.
> 
> Thanks for this insight.  I've updated the commit message to try and
> describe the history here a little more accurately, including adding
> the commit SHA you gave above, which should help if anyone needs to
> dig into this code in the future.
> 
>>
>>> So, finally, in this commit, I propose to remove the
>>> remote_state::cached_wait_status flag and to stop using the ::buf to
>>> cache stop replies.  Instead, stop replies will now always be stored
>>> in the ::stop_reply_queue.
>>
>> To be honest, I don't recall exactly why I didn't do that when introducing
>> the stop reply queue.
>>
>>> @@ -8163,15 +8151,12 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status,
>>>  
>>>    stop_reply = queued_stop_reply (ptid);
>>>    if (stop_reply != NULL)
>>> -    return process_stop_reply (stop_reply, status);
>>> -
>>> -  if (rs->cached_wait_status)
>>> -    /* Use the cached wait status, but only once.  */
>>> -    rs->cached_wait_status = 0;
>>> +    {
>>> +      rs->waiting_for_stop_reply = 0;
>>
>> This is a difference described in the commit log, but looking at the resulting code,
>> I don't understand why clearing this flag is needed here, it looks like dead code to me.
>> I mean, if we have a cached status already, then we're not waiting for a stop reply
>> from the target.  Did you run into a case where it was needed?
> 
> No I never hit a case where it was definitely needed.  Honestly, I
> just looked at the different code paths, and saw that it was pretty
> easy to merge the paths and carry out all the actions, so did that.
> 
> However, you're right to call me out on that.  It's exactly this sort
> of "just in case" code that was causing the cached packets to be
> discarded originally without warning.
> 
> So, I've changed the unconditional "set flag to false" with an assert,
> and a comment.  If it turns out we are wrong in our understanding of
> the situation, then hopefully, the problem will get reported, and we
> can figure out what's going on at that point!

Yeah.  I don't imagine how this will ever trigger, but definitely can't hurt
having an assert.

- If we have an event queued it must mean that we have gotten the reply from the target
  already.  Recall this is all-stop RSP, vCont is synchronous.

- And if we see a stop reply, and have it queued, we can't resume the target before
  the queued event is processed, otherwise when we get to process the queued event,
  the target is already running past what the stop the event was for!  If we do ever
  manage to sent a vCont with a pending queued event, then that's a bug.

> 
> Below is the updated patch.  The only code change is the assert I
> mention above.  All other changes are in the commit message.
> 
> I'll give this a few days in case you want to follow up, then I'll
> push this.
> 

Thanks, this is OK.  Just noticed a tiny typo in the new comment:

> +    {
> +      /* Currently non of the paths that push a stop reply onto the queue


non -> none.  I'd argue that "Currently" is redundant, and gives a false
impression that that might change without much consequence other than hitting
this assertion.  I think you should remove that word.

> +	 will have set the waiting_for_stop_reply flag.  */
> +      gdb_assert (!rs->waiting_for_stop_reply);
> +      event_ptid = process_stop_reply (stop_reply, status);
> +    }

Re: [PATCHv3 1/2] gdb/remote: some fixes for 'maint set target-async off'

[Andrew Burgess]

* Pedro Alves <pedro@palves.net> [2021-12-17 14:05:55 +0000]:

> On 2021-12-17 13:35, Andrew Burgess wrote:
> > * Pedro Alves <pedro@palves.net> [2021-12-16 21:15:31 +0000]:
> > 
> >> On 2021-12-01 10:40, Andrew Burgess via Gdb-patches wrote:
> >>
> >>> This problem can clearly be seen I feel by looking at the
> >>> remote_state::cached_wait_status flag.  This flag tells GDB if there
> >>> is a wait status cached in remote_state::buf.  However, in
> >>> remote_target::putpkt_binary and remote_target::getpkt_or_notif_sane_1
> >>> this flag is just set back to 0, doing this immediately discards any
> >>> cached data.
> >>>
> >>> I don't know if this scheme ever made sense, maybe once upon a time,
> >>> GDB would, when it found it had no cached stop, simply re-request the
> >>> stop information from the target, however, this certainly isn't the
> >>> case now, and resetting the cached_wait_status is (I claim) a bad
> >>> thing.
> >>
> >> I don't think that was ever the case.  Take a look at 2d717e4f8a54,
> >> where the cached status was introduced to handle "attach".  It was simply the case
> >> back then that nothing would talk to the target between the initial attach
> >> and consuming the event.  It's not clear to me why putpkt/getpkt would
> >> need to clear the flag back then.  Looks more like a "just in case" safeguard.
> > 
> > Thanks for this insight.  I've updated the commit message to try and
> > describe the history here a little more accurately, including adding
> > the commit SHA you gave above, which should help if anyone needs to
> > dig into this code in the future.
> > 
> >>
> >>> So, finally, in this commit, I propose to remove the
> >>> remote_state::cached_wait_status flag and to stop using the ::buf to
> >>> cache stop replies.  Instead, stop replies will now always be stored
> >>> in the ::stop_reply_queue.
> >>
> >> To be honest, I don't recall exactly why I didn't do that when introducing
> >> the stop reply queue.
> >>
> >>> @@ -8163,15 +8151,12 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status,
> >>>  
> >>>    stop_reply = queued_stop_reply (ptid);
> >>>    if (stop_reply != NULL)
> >>> -    return process_stop_reply (stop_reply, status);
> >>> -
> >>> -  if (rs->cached_wait_status)
> >>> -    /* Use the cached wait status, but only once.  */
> >>> -    rs->cached_wait_status = 0;
> >>> +    {
> >>> +      rs->waiting_for_stop_reply = 0;
> >>
> >> This is a difference described in the commit log, but looking at the resulting code,
> >> I don't understand why clearing this flag is needed here, it looks like dead code to me.
> >> I mean, if we have a cached status already, then we're not waiting for a stop reply
> >> from the target.  Did you run into a case where it was needed?
> > 
> > No I never hit a case where it was definitely needed.  Honestly, I
> > just looked at the different code paths, and saw that it was pretty
> > easy to merge the paths and carry out all the actions, so did that.
> > 
> > However, you're right to call me out on that.  It's exactly this sort
> > of "just in case" code that was causing the cached packets to be
> > discarded originally without warning.
> > 
> > So, I've changed the unconditional "set flag to false" with an assert,
> > and a comment.  If it turns out we are wrong in our understanding of
> > the situation, then hopefully, the problem will get reported, and we
> > can figure out what's going on at that point!
> 
> Yeah.  I don't imagine how this will ever trigger, but definitely can't hurt
> having an assert.
> 
> - If we have an event queued it must mean that we have gotten the reply from the target
>   already.  Recall this is all-stop RSP, vCont is synchronous.
> 
> - And if we see a stop reply, and have it queued, we can't resume the target before
>   the queued event is processed, otherwise when we get to process the queued event,
>   the target is already running past what the stop the event was for!  If we do ever
>   manage to sent a vCont with a pending queued event, then that's a bug.
> 
> > 
> > Below is the updated patch.  The only code change is the assert I
> > mention above.  All other changes are in the commit message.
> > 
> > I'll give this a few days in case you want to follow up, then I'll
> > push this.
> > 
> 
> Thanks, this is OK.  Just noticed a tiny typo in the new comment:
> 
> > +    {
> > +      /* Currently non of the paths that push a stop reply onto the queue
> 
> 
> non -> none.  I'd argue that "Currently" is redundant, and gives a false
> impression that that might change without much consequence other than hitting
> this assertion.  I think you should remove that word.

Thanks for catching that.  I made the changes you suggested, and
pushed this patch.

Thanks,
Andrew


> 
> > +	 will have set the waiting_for_stop_reply flag.  */
> > +      gdb_assert (!rs->waiting_for_stop_reply);
> > +      event_ptid = process_stop_reply (stop_reply, status);
> > +    }
> 

Re: [PATCHv3 2/2] gdb: add assert in remote_target::wait relating to async being off

[Andrew Burgess]

* Pedro Alves <pedro@palves.net> [2021-12-16 21:16:00 +0000]:

> On 2021-12-01 10:40, Andrew Burgess via Gdb-patches wrote:
> > While working on another patch I ended up in a situation where I had
> > async mode disabled (with 'maint set target-async off'), but the async
> > event token got marked anyway.
> > 
> > In this situation GDB was continually calling into
> > remote_target::wait, however, the async token would never become
> > unmarked as the unmarking is guarded by target_is_async_p.
> > 
> > We could just unconditionally unmark the token, but that would feel
> > like just ignoring a bug, so, instead, lets assert that if
> > !target_is_async_p, then the async token should not be marked.
> > 
> > This assertion would have caught my earlier mistake.
> > 
> > There should be no user visible changes with this commit.
> > ---
> >  gdb/remote.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/gdb/remote.c b/gdb/remote.c
> > index b3890d71c59..9b814d54313 100644
> > --- a/gdb/remote.c
> > +++ b/gdb/remote.c
> > @@ -8295,9 +8295,13 @@ remote_target::wait (ptid_t ptid, struct target_waitstatus *status,
> >    remote_state *rs = get_remote_state ();
> >  
> >    /* Start by clearing the flag that asks for our wait method to be called,
> > -     we'll mark it again at the end if needed.  */
> > +     we'll mark it again at the end if needed.  If the target is not in
> > +     async mode then the async token should not be marked.  */
> >    if (target_is_async_p ())
> >      clear_async_event_handler (rs->remote_async_inferior_event_token);
> > +  else
> > +    gdb_assert (!async_event_handler_marked
> > +		(rs->remote_async_inferior_event_token));
> >  
> >    ptid_t event_ptid;
> >  
> 
> LGTM.

Thanks, pushed.

Andrew