[PATCH][PR gdb/26819] Fix initial thread state of non-threaded remote targets

[PATCH][PR gdb/26819] Fix initial thread state of non-threaded remote targets

[Jan Matyáš]

Hello,

below is a patch for PR gdb/26819, which fixes the behavior of remote
targets which are thread-unaware. GDB sets initially all remote threads to
"resumed" state, which also needs to be done

Credits for this patch go to Simon Marchi. I merely assembled the patch per
comments and suggestions from PR gdb/26819.

Part of the patch is an extension to the GDB test suite which covers this
bug and can be used to reproduce the original issue & verify the fix.

Any review comments are very welcome.

Thanks and regards,
Jan


From 3b3d9c035db61aef61966d1b5e857c4e4238b286 Mon Sep 17 00:00:00 2001
From: Jan Matyas <jmatyas@codasip.com>
Date: Wed, 24 Feb 2021 09:44:20 +0100
Subject: [PATCH] [PR gdb/26819] Fix initial thread state of
 non-threaded remote targets

This change fixes the initial state of the main thread of remote
targets which have no concept of threading. Such targets are
treated as single-threaded by gdb, and this single thread needs
to be initially set to the "resumed" state, in the same manner as
threads in thread-aware remote targets (see remote.c,
remote_target::remote_add_thread).

Without this fix, the following assert was triggered on thread-
unaware remote targets:

remote_target::select_thread_for_ambiguous_stop_reply(const
  target_waitstatus*): Assertion `first_resumed_thread != nullptr'
  failed.

The bug can be reproduced using gdbserver:
- by disabling packets 'T' and 'qThreadInfo', or
- by disabling all thread-related packets

The test suite has been updated to include these two scenarios,
see: gdb/testsuite/gdb.server/stop-reply-no-thread.exp.
---
 gdb/ChangeLog                                     |  9 +++++++++
 gdb/remote.c                                      | 13 +++++++++----
 gdb/testsuite/ChangeLog                           |  7 +++++++
 gdb/testsuite/gdb.server/stop-reply-no-thread.exp | 10 ++++++++++
 4 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d189a0919b..9b05c832b9 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,12 @@
+2021-02-24  Jan Matyas  <jmatyas@codasip.com>
+
+ PR gdb/26819
+ * remote.c (start_remote): Ensure the single thread,
+ automatically added for remote targets without the concept
+ of threading, is initially in set to the "resumed" state.
+ * remote.c (add_current_inferior_and_thread): Add return value -
+ return the main thread.
+
 2021-02-23  Simon Marchi  <simon.marchi@polymtl.ca>

  PR gdb/26828
diff --git a/gdb/remote.c b/gdb/remote.c
index 31c6e17a1c..95db85f0a1 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -741,7 +741,7 @@ class remote_target : public process_stratum_target
   int remote_resume_with_vcont (ptid_t ptid, int step,
  gdb_signal siggnal);

-  void add_current_inferior_and_thread (const char *wait_status);
+  thread_info *add_current_inferior_and_thread (const char *wait_status);

   ptid_t wait_ns (ptid_t ptid, struct target_waitstatus *status,
   target_wait_flags options);
@@ -4409,9 +4409,11 @@ remote_target::get_current_thread (const char
*wait_status)
    whose response is a stop reply from which we can also try
    extracting the thread.  If the target doesn't support the explicit
    qC query, we infer the current thread from that stop reply, passed
-   in in WAIT_STATUS, which may be NULL.  */
+   in in WAIT_STATUS, which may be NULL.

-void
+   The function returns pointer to the main thread of the inferior. */
+
+thread_info *
 remote_target::add_current_inferior_and_thread (const char *wait_status)
 {
   struct remote_state *rs = get_remote_state ();
@@ -4445,6 +4447,8 @@ remote_target::add_current_inferior_and_thread (const
char *wait_status)
      yet.  */
   thread_info *tp = add_thread_silent (this, curr_ptid);
   switch_to_thread_no_regs (tp);
+
+  return tp;
 }

 /* Print info about a thread that was found already stopped on
@@ -4800,7 +4804,8 @@ remote_target::start_remote (int from_tty, int
extended_p)
   /* Target has no concept of threads at all.  GDB treats
      non-threaded target as single-threaded; add a main
      thread.  */
-  add_current_inferior_and_thread (wait_status);
+  thread_info *tp = add_current_inferior_and_thread (wait_status);
+  get_remote_thread_info (tp)->set_resumed ();
  }
       else
  {
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 70b836b8c2..6f0efb8ec2 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,10 @@
+2021-02-24  Jan Matyas  <jmatyas@codasip.com>
+
+ PR gdb/26819
+ * gdb.server/stop-reply-no-thread.exp: Added two test
+ scenarios that cover remote targets which do not have
+ the concept of threads.
+
 2021-02-18  Andrew Burgess  <andrew.burgess@embecosm.com>

  * gdb.arch/i386-biarch-core.exp: Add target check.
diff --git a/gdb/testsuite/gdb.server/stop-reply-no-thread.exp
b/gdb/testsuite/gdb.server/stop-reply-no-thread.exp
index 68bf42ac1a..823bdf894c 100644
--- a/gdb/testsuite/gdb.server/stop-reply-no-thread.exp
+++ b/gdb/testsuite/gdb.server/stop-reply-no-thread.exp
@@ -112,3 +112,13 @@ foreach_with_prefix to_disable { "" Tthread T } {
  run_test $to_disable $t_nonstop
     }
 }
+
+# Regression for PR gdb/26819: Cover the case when GDBserver does not
report
+# any threads (i.e. the remote target has no concept of threads).
+#
+# Scenario 1: Disable 'T' and 'qfThreadInfo' packets
+# Scenario 2: Disable all threading packets
+foreach_with_prefix to_disable { "T,qfThreadInfo" "threads" } {
+    # Non-stop mode not applicable - off.
+    run_test $to_disable off
+}
-- 
2.20.1

Re: [PATCH][PR gdb/26819] Fix initial thread state of non-threaded remote targets

[Simon Marchi]

On 2021-02-24 4:15 a.m., Jan Matyáš wrote:> Hello,
> 
> below is a patch for PR gdb/26819, which fixes the behavior of remote
> targets which are thread-unaware. GDB sets initially all remote threads to
> "resumed" state, which also needs to be done
> 
> Credits for this patch go to Simon Marchi. I merely assembled the patch per
> comments and suggestions from PR gdb/26819.
> 
> Part of the patch is an extension to the GDB test suite which covers this
> bug and can be used to reproduce the original issue & verify the fix.
> 
> Any review comments are very welcome.
> 
> Thanks and regards,
> Jan

Woohoo, thanks for the patch.  I'll take a look again and perhaps wait
for Andrew to take a look as well before merging.

However, since you probably sent the patch using an email client, lines
were wrapped and the patch is corrupt / does not apply.  Could you try
to send it again using git-send-email?  This is really the preferred way
to send patches by email.  You can use the `-v2` switch to mark it as
version 2.

Please let me know if you need help using git-send-email.

Simon

[PATCH] [PR gdb/26819] Fix initial thread state of non-threaded remote targets

[Jan Matyas]

Note:
This is an identical patch as posted previously in this thread,
only sent using git-send-email so that line breaks are preserved.

This change fixes the initial state of the main thread of remote
targets which have no concept of threading. Such targets are
treated as single-threaded by gdb, and this single thread needs
to be initially set to the "resumed" state, in the same manner as
threads in thread-aware remote targets (see remote.c,
remote_target::remote_add_thread).

Without this fix, the following assert was triggered on thread-
unaware remote targets:

remote_target::select_thread_for_ambiguous_stop_reply(const
  target_waitstatus*): Assertion `first_resumed_thread != nullptr'
  failed.

The bug can be reproduced using gdbserver
* by disabling packets 'T' and 'qThreadInfo', or
* by disabling all thread-related packets.

The test suite has been updated to include these two scenarios,
see: gdb/testsuite/gdb.server/stop-reply-no-thread.exp.
---
 gdb/ChangeLog                                     |  9 +++++++++
 gdb/remote.c                                      | 13 +++++++++----
 gdb/testsuite/ChangeLog                           |  7 +++++++
 gdb/testsuite/gdb.server/stop-reply-no-thread.exp | 10 ++++++++++
 4 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d189a0919b..9b05c832b9 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,12 @@
+2021-02-24  Jan Matyas  <jmatyas@codasip.com>
+
+	PR gdb/26819
+	* remote.c (start_remote): Ensure the single thread,
+	automatically added for remote targets without the concept
+	of threading, is initially in set to the "resumed" state.
+	* remote.c (add_current_inferior_and_thread): Add return value -
+	return the main thread.
+
 2021-02-23  Simon Marchi  <simon.marchi@polymtl.ca>
 
 	PR gdb/26828
diff --git a/gdb/remote.c b/gdb/remote.c
index 31c6e17a1c..95db85f0a1 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -741,7 +741,7 @@ class remote_target : public process_stratum_target
   int remote_resume_with_vcont (ptid_t ptid, int step,
 				gdb_signal siggnal);
 
-  void add_current_inferior_and_thread (const char *wait_status);
+  thread_info *add_current_inferior_and_thread (const char *wait_status);
 
   ptid_t wait_ns (ptid_t ptid, struct target_waitstatus *status,
 		  target_wait_flags options);
@@ -4409,9 +4409,11 @@ remote_target::get_current_thread (const char *wait_status)
    whose response is a stop reply from which we can also try
    extracting the thread.  If the target doesn't support the explicit
    qC query, we infer the current thread from that stop reply, passed
-   in in WAIT_STATUS, which may be NULL.  */
+   in in WAIT_STATUS, which may be NULL.
 
-void
+   The function returns pointer to the main thread of the inferior. */
+
+thread_info *
 remote_target::add_current_inferior_and_thread (const char *wait_status)
 {
   struct remote_state *rs = get_remote_state ();
@@ -4445,6 +4447,8 @@ remote_target::add_current_inferior_and_thread (const char *wait_status)
      yet.  */
   thread_info *tp = add_thread_silent (this, curr_ptid);
   switch_to_thread_no_regs (tp);
+
+  return tp;
 }
 
 /* Print info about a thread that was found already stopped on
@@ -4800,7 +4804,8 @@ remote_target::start_remote (int from_tty, int extended_p)
 	  /* Target has no concept of threads at all.  GDB treats
 	     non-threaded target as single-threaded; add a main
 	     thread.  */
-	  add_current_inferior_and_thread (wait_status);
+	  thread_info *tp = add_current_inferior_and_thread (wait_status);
+	  get_remote_thread_info (tp)->set_resumed ();
 	}
       else
 	{
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 70b836b8c2..6f0efb8ec2 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,10 @@
+2021-02-24  Jan Matyas  <jmatyas@codasip.com>
+
+	PR gdb/26819
+	* gdb.server/stop-reply-no-thread.exp: Added two test
+	scenarios that cover remote targets which do not have
+	the concept of threads.
+
 2021-02-18  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* gdb.arch/i386-biarch-core.exp: Add target check.
diff --git a/gdb/testsuite/gdb.server/stop-reply-no-thread.exp b/gdb/testsuite/gdb.server/stop-reply-no-thread.exp
index 68bf42ac1a..823bdf894c 100644
--- a/gdb/testsuite/gdb.server/stop-reply-no-thread.exp
+++ b/gdb/testsuite/gdb.server/stop-reply-no-thread.exp
@@ -112,3 +112,13 @@ foreach_with_prefix to_disable { "" Tthread T } {
 	run_test $to_disable $t_nonstop
     }
 }
+
+# Regression for PR gdb/26819: Cover the case when GDBserver does not report
+# any threads (i.e. the remote target has no concept of threads).
+#
+# Scenario 1: Disable 'T' and 'qfThreadInfo' packets
+# Scenario 2: Disable all threading packets
+foreach_with_prefix to_disable { "T,qfThreadInfo" "threads" } {
+    # Non-stop mode not applicable - off.
+    run_test $to_disable off
+}
-- 
2.20.1

Re: [PATCH] [PR gdb/26819] Fix initial thread state of non-threaded remote targets

[Simon Marchi]

On 2021-02-25 2:27 a.m., Jan Matyas wrote:
> Note:
> This is an identical patch as posted previously in this thread,
> only sent using git-send-email so that line breaks are preserved.
> 
> This change fixes the initial state of the main thread of remote
> targets which have no concept of threading. Such targets are
> treated as single-threaded by gdb, and this single thread needs
> to be initially set to the "resumed" state, in the same manner as
> threads in thread-aware remote targets (see remote.c,
> remote_target::remote_add_thread).
> 
> Without this fix, the following assert was triggered on thread-
> unaware remote targets:
> 
> remote_target::select_thread_for_ambiguous_stop_reply(const
>   target_waitstatus*): Assertion `first_resumed_thread != nullptr'
>   failed.
> 
> The bug can be reproduced using gdbserver
> * by disabling packets 'T' and 'qThreadInfo', or
> * by disabling all thread-related packets.
> 
> The test suite has been updated to include these two scenarios,
> see: gdb/testsuite/gdb.server/stop-reply-no-thread.exp.

Thanks!  The patch still looked good to me, so I pushed it.

Simon