[PATCH 1/2] gdb: some int to bool conversion in remote.c

[PATCH 1/2] gdb: some int to bool conversion in remote.c

[Andrew Burgess]

Convert a couple of local variables from int to bool.  There should be
no user visible changes after this commit.

gdb/ChangeLog:

	* remote.c (check_pending_events_prevent_wildcard_vcont): Change
	argument type, update and re-wrap, header comment.
	(remote_target::commit_resumed): Convert any_process_wildcard and
	may_global_wildcard_vcont from int to bool.
---
 gdb/ChangeLog |  7 +++++++
 gdb/remote.c  | 31 ++++++++++++++-----------------
 2 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index d3a66599122..389835caa09 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -780,7 +780,7 @@ class remote_target : public process_stratum_target
   int stop_reply_queue_length ();
 
   void check_pending_events_prevent_wildcard_vcont
-    (int *may_global_wildcard_vcont);
+    (bool *may_global_wildcard_vcont);
 
   void discard_pending_stop_replies_in_queue ();
   struct stop_reply *remote_notif_remove_queued_reply (ptid_t ptid);
@@ -6634,9 +6634,6 @@ vcont_builder::push_action (ptid_t ptid, bool step, gdb_signal siggnal)
 void
 remote_target::commit_resumed ()
 {
-  int any_process_wildcard;
-  int may_global_wildcard_vcont;
-
   /* If connected in all-stop mode, we'd send the remote resume
      request directly from remote_resume.  Likewise if
      reverse-debugging, as there are no defined vCont actions for
@@ -6693,7 +6690,7 @@ remote_target::commit_resumed ()
      (vCont;c).  We can still send process-wide wildcards though.  */
 
   /* Start by assuming a global wildcard (vCont;c) is possible.  */
-  may_global_wildcard_vcont = 1;
+  bool may_global_wildcard_vcont = true;
 
   /* And assume every process is individually wildcard-able too.  */
   for (inferior *inf : all_non_exited_inferiors (this))
@@ -6721,7 +6718,7 @@ remote_target::commit_resumed ()
 
 	  /* And if we can't wildcard a process, we can't wildcard
 	     everything either.  */
-	  may_global_wildcard_vcont = 0;
+	  may_global_wildcard_vcont = false;
 	  continue;
 	}
 
@@ -6732,7 +6729,7 @@ remote_target::commit_resumed ()
 	 can't do a global wildcard, as that would resume the fork
 	 child.  */
       if (is_pending_fork_parent_thread (tp))
-	may_global_wildcard_vcont = 0;
+	may_global_wildcard_vcont = false;
     }
 
   /* We didn't have any resumed thread pending a vCont resume, so nothing to
@@ -6782,13 +6779,13 @@ remote_target::commit_resumed ()
   /* Now check whether we can send any process-wide wildcard.  This is
      to avoid sending a global wildcard in the case nothing is
      supposed to be resumed.  */
-  any_process_wildcard = 0;
+  bool any_process_wildcard = false;
 
   for (inferior *inf : all_non_exited_inferiors (this))
     {
       if (get_remote_inferior (inf)->may_wildcard_vcont)
 	{
-	  any_process_wildcard = 1;
+	  any_process_wildcard = true;
 	  break;
 	}
     }
@@ -7271,15 +7268,15 @@ remote_target::remove_new_fork_children (threads_listing_context *context)
       context->remove_thread (event->ws.value.related_pid);
 }
 
-/* Check whether any event pending in the vStopped queue would prevent
-   a global or process wildcard vCont action.  Clear
-   *may_global_wildcard if we can't do a global wildcard (vCont;c),
-   and clear the event inferior's may_wildcard_vcont flag if we can't
-   do a process-wide wildcard resume (vCont;c:pPID.-1).  */
+/* Check whether any event pending in the vStopped queue would prevent a
+   global or process wildcard vCont action.  Set *may_global_wildcard to
+   false if we can't do a global wildcard (vCont;c), and clear the event
+   inferior's may_wildcard_vcont flag if we can't do a process-wide
+   wildcard resume (vCont;c:pPID.-1).  */
 
 void
 remote_target::check_pending_events_prevent_wildcard_vcont
-  (int *may_global_wildcard)
+  (bool *may_global_wildcard)
 {
   struct notif_client *notif = &notif_client_stop;
 
@@ -7292,12 +7289,12 @@ remote_target::check_pending_events_prevent_wildcard_vcont
 
       if (event->ws.kind == TARGET_WAITKIND_FORKED
 	  || event->ws.kind == TARGET_WAITKIND_VFORKED)
-	*may_global_wildcard = 0;
+	*may_global_wildcard = false;
 
       /* This may be the first time we heard about this process.
 	 Regardless, we must not do a global wildcard resume, otherwise
 	 we'd resume this process too.  */
-      *may_global_wildcard = 0;
+      *may_global_wildcard = false;
       if (event->ptid != null_ptid)
 	{
 	  inferior *inf = find_inferior_ptid (this, event->ptid);
-- 
2.25.4

[PATCH 2/2] gdb: avoid sending unnecessary wildcard vCont actions

[Andrew Burgess]

Looking at remote_target::commit_resumed the algorithm for sending
wildcard actions starts by assuming that we can send a wildcard
action, and then looks for pending events, or resume requests, that
prevent wildcard actions (either global, or inferior specific).

This is fine, and there's no real problems with this, but it does lead
to one slight weirdness, if we are in non-stop mode, and
displaced-stepping is off, then, when we do a step, all threads should
be stopped, and we should step only the one thread that we wish to
have move on.  However, in the situation where there the inferior only
has a single thread then we actually end up sending a packet like
this:

  [remote] Sending packet: $vCont;s:pPID.TID;c#XX

Notice the trailing ';c'.

Now, there's nothing technically wrong here, we are correctly asking
the one and only thread PID.TID to perform a step, there are no other
threads, so telling everyone else to continue is harmless.

However, this annoys me.  It annoys me because we know we want only
the one, and only one, thread PID.TID to step, why send over redundant
information!

This commit fixes this tiny blip by keeping a counter of the number of
threads that could be resumed in each inferior.  We count up as we see
threads that need to be resumed, and then count down when non-wildcard
actions are sent (e.g. the s:pPID.TID in our example above).  Then,
when we are considering sending wildcard actions, we only send one if
there are any threads that will be impacted by the wildcard action.

Now, in the above situation we send out

  [remote] Sending packet: $vCont;s:pPID.TID#XX

There should be no change in behaviour when considering gdb/gdbserver
working together after this commit.

gdb/ChangeLog:

	* remote.c (struct remote_inferior): Add
	pending_vcont_resume_thread_count field.
	(remote_target::commit_resumed): Add use of
	pending_vcont_resume_thread_count field, only send wildcard action
	if there are threads the wildcard will cover.
---
 gdb/ChangeLog |  8 ++++++++
 gdb/remote.c  | 23 +++++++++++++++++++----
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 389835caa09..0a23a62a845 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6485,6 +6485,11 @@ struct remote_inferior : public private_inferior
 {
   /* Whether we can send a wildcard vCont for this process.  */
   bool may_wildcard_vcont = true;
+
+  /* The number of threads in this inferior that have a pending vCont
+     resume.  Used when building vCont packets, and it only valid during
+     remote_target::commit_resumed.  */
+  int pending_vcont_resume_thread_count = 0;
 };
 
 /* Get the remote private inferior data associated to INF.  */
@@ -6698,6 +6703,7 @@ remote_target::commit_resumed ()
       remote_inferior *priv = get_remote_inferior (inf);
 
       priv->may_wildcard_vcont = true;
+      priv->pending_vcont_resume_thread_count = 0;
     }
 
   /* Check for any pending events (not reported or processed yet) and
@@ -6723,7 +6729,10 @@ remote_target::commit_resumed ()
 	}
 
       if (priv->get_resume_state () == resume_state::RESUMED_PENDING_VCONT)
-	any_pending_vcont_resume = true;
+	{
+	  get_remote_inferior (tp->inf)->pending_vcont_resume_thread_count++;
+	  any_pending_vcont_resume = true;
+	}
 
       /* If a thread is the parent of an unfollowed fork, then we
 	 can't do a global wildcard, as that would resume the fork
@@ -6771,7 +6780,10 @@ remote_target::commit_resumed ()
          it will be included in a wildcard resume instead.  */
       if (info.step || info.sig != GDB_SIGNAL_0
 	  || !get_remote_inferior (tp->inf)->may_wildcard_vcont)
-	vcont_builder.push_action (tp->ptid, info.step, info.sig);
+	{
+	  vcont_builder.push_action (tp->ptid, info.step, info.sig);
+	  get_remote_inferior (tp->inf)->pending_vcont_resume_thread_count--;
+	}
 
       remote_thr->set_resumed ();
     }
@@ -6783,7 +6795,8 @@ remote_target::commit_resumed ()
 
   for (inferior *inf : all_non_exited_inferiors (this))
     {
-      if (get_remote_inferior (inf)->may_wildcard_vcont)
+      if (get_remote_inferior (inf)->may_wildcard_vcont
+	  && get_remote_inferior (inf)->pending_vcont_resume_thread_count > 0)
 	{
 	  any_process_wildcard = true;
 	  break;
@@ -6804,7 +6817,9 @@ remote_target::commit_resumed ()
 	{
 	  for (inferior *inf : all_non_exited_inferiors (this))
 	    {
-	      if (get_remote_inferior (inf)->may_wildcard_vcont)
+	      if (get_remote_inferior (inf)->may_wildcard_vcont
+		  && (get_remote_inferior (inf)
+		      ->pending_vcont_resume_thread_count > 0))
 		{
 		  vcont_builder.push_action (ptid_t (inf->pid),
 					     false, GDB_SIGNAL_0);
-- 
2.25.4

Re: [PATCH 1/2] gdb: some int to bool conversion in remote.c

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> Convert a couple of local variables from int to bool.  There should be
Andrew> no user visible changes after this commit.

Thanks.  I feel like this kind of thing normally can just go in under
the obvious rule.

Tom

Re: [PATCH 2/2] gdb: avoid sending unnecessary wildcard vCont actions

[Simon Marchi]

On 2021-05-11 10:39 a.m., Andrew Burgess wrote:
> Looking at remote_target::commit_resumed the algorithm for sending
> wildcard actions starts by assuming that we can send a wildcard
> action, and then looks for pending events, or resume requests, that
> prevent wildcard actions (either global, or inferior specific).
> 
> This is fine, and there's no real problems with this, but it does lead
> to one slight weirdness, if we are in non-stop mode, and
> displaced-stepping is off, then, when we do a step, all threads should
> be stopped, and we should step only the one thread that we wish to
> have move on.  However, in the situation where there the inferior only
> has a single thread then we actually end up sending a packet like
> this:
> 
>   [remote] Sending packet: $vCont;s:pPID.TID;c#XX
> 
> Notice the trailing ';c'.
> 
> Now, there's nothing technically wrong here, we are correctly asking
> the one and only thread PID.TID to perform a step, there are no other
> threads, so telling everyone else to continue is harmless.
> 
> However, this annoys me.  It annoys me because we know we want only
> the one, and only one, thread PID.TID to step, why send over redundant
> information!
> 
> This commit fixes this tiny blip by keeping a counter of the number of
> threads that could be resumed in each inferior.  We count up as we see
> threads that need to be resumed, and then count down when non-wildcard
> actions are sent (e.g. the s:pPID.TID in our example above).  Then,
> when we are considering sending wildcard actions, we only send one if
> there are any threads that will be impacted by the wildcard action.
> 
> Now, in the above situation we send out
> 
>   [remote] Sending packet: $vCont;s:pPID.TID#XX
> 
> There should be no change in behaviour when considering gdb/gdbserver
> working together after this commit.

The code looks correct, but in my opinion this little oddity is not
significant enough to justify making this code more complex (it's
already complex enough as it is).

Simon

Re: [PATCH 2/2] gdb: avoid sending unnecessary wildcard vCont actions

[Andrew Burgess]

* Simon Marchi <simon.marchi@polymtl.ca> [2021-05-13 13:55:05 -0400]:

> On 2021-05-11 10:39 a.m., Andrew Burgess wrote:
> > Looking at remote_target::commit_resumed the algorithm for sending
> > wildcard actions starts by assuming that we can send a wildcard
> > action, and then looks for pending events, or resume requests, that
> > prevent wildcard actions (either global, or inferior specific).
> > 
> > This is fine, and there's no real problems with this, but it does lead
> > to one slight weirdness, if we are in non-stop mode, and
> > displaced-stepping is off, then, when we do a step, all threads should
> > be stopped, and we should step only the one thread that we wish to
> > have move on.  However, in the situation where there the inferior only
> > has a single thread then we actually end up sending a packet like
> > this:
> > 
> >   [remote] Sending packet: $vCont;s:pPID.TID;c#XX
> > 
> > Notice the trailing ';c'.
> > 
> > Now, there's nothing technically wrong here, we are correctly asking
> > the one and only thread PID.TID to perform a step, there are no other
> > threads, so telling everyone else to continue is harmless.
> > 
> > However, this annoys me.  It annoys me because we know we want only
> > the one, and only one, thread PID.TID to step, why send over redundant
> > information!
> > 
> > This commit fixes this tiny blip by keeping a counter of the number of
> > threads that could be resumed in each inferior.  We count up as we see
> > threads that need to be resumed, and then count down when non-wildcard
> > actions are sent (e.g. the s:pPID.TID in our example above).  Then,
> > when we are considering sending wildcard actions, we only send one if
> > there are any threads that will be impacted by the wildcard action.
> > 
> > Now, in the above situation we send out
> > 
> >   [remote] Sending packet: $vCont;s:pPID.TID#XX
> > 
> > There should be no change in behaviour when considering gdb/gdbserver
> > working together after this commit.
> 
> The code looks correct, but in my opinion this little oddity is not
> significant enough to justify making this code more complex (it's
> already complex enough as it is).

Thanks for the feedback.

I spotted this oddity when looking at gdb/27830 [1], this bug boils
down to what should happen when a new thread is started.  Right now we
(almost) always assume that any new thread should be set running, but
this just isn't true.  If we are single stepping a thread in non-stop
mode, and with displaced stepping off, then, should a new thread
appear we should report the new thread as created/stopped rather than
setting it running.

Solving this for native targets is easy enough, but we don't really
have an easy/obvious way in the remote protocol to specify how new
threads should be handled (well, we have QThreadEvents, which might be
the solution).

Anyway, I wondered if it was possible to infer from the last vCont
packet how new threads should be handled, which got me looking at
these packets...

Given I don't, right now, know that fixing this is a requirement, I'll
hold off pushing this until either (a) I know it's required, or (b) I
can figure out a simpler way to have write the logic so we can send
the exactly correct contents.

Thanks,
Andrew


[1] https://sourceware.org/bugzilla/show_bug.cgi?id=27830

Re: [PATCH 2/2] gdb: avoid sending unnecessary wildcard vCont actions

[Simon Marchi]

On 2021-05-14 8:49 a.m., Andrew Burgess wrote:
> I spotted this oddity when looking at gdb/27830 [1], this bug boils
> down to what should happen when a new thread is started.  Right now we
> (almost) always assume that any new thread should be set running, but
> this just isn't true.  If we are single stepping a thread in non-stop
> mode, and with displaced stepping off, then, should a new thread
> appear we should report the new thread as created/stopped rather than
> setting it running.
> 
> Solving this for native targets is easy enough, but we don't really
> have an easy/obvious way in the remote protocol to specify how new
> threads should be handled (well, we have QThreadEvents, which might be
> the solution).
> 
> Anyway, I wondered if it was possible to infer from the last vCont
> packet how new threads should be handled, which got me looking at
> these packets...
> 
> Given I don't, right now, know that fixing this is a requirement, I'll
> hold off pushing this until either (a) I know it's required, or (b) I
> can figure out a simpler way to have write the logic so we can send
> the exactly correct contents.

Ok, thanks for the explanation.  If there is actually a bug to be fixed
with that, then that's another story.

Simon