[3/3] Fix problem exposed by gdb.server/stop-reply-no-thread-multi.exp

Message ID 20210315234339.457551-4-pedro@palves.net
State New
Headers show
Series
  • Fix gdbserver + "maint set target-non-stop" problems
Related show

Commit Message

Pedro Alves March 15, 2021, 11:43 p.m.
Running gdb.server/stop-reply-no-thread-multi.exp with "maint set
target-non-stop on" occasionally hit an internal error like this:

  ...
  continue
  Continuing.
  warning: multi-threaded target stopped without sending a thread-id, using first non-exited thread
  /home/pedro/gdb/binutils-gdb/src/gdb/inferior.c:291: internal-error: inferior* find_inferior_pid(process_stratum_target*, int): Assertion `pid != 0' failed.
  A problem internal to GDB has been detected,
  further debugging may prove unreliable.

  This is a bug, please report it.
  FAIL: gdb.server/stop-reply-no-thread-multi.exp: to_disable=Tthread: continue until exit (GDB internal error)

The backtrace looks like this:

 ...
 #5  0x0000560357b0879c in internal_error (file=0x560357be6c18 "/home/pedro/gdb/binutils-gdb/src/gdb/inferior.c", line=291, fmt=0x560357be6b21 "%s: Assertion `%s' failed.") at /home/pedro/gdb/binutils-gdb/src/gdbsupport/errors.cc:55
 #6  0x000056035762061b in find_inferior_pid (targ=0x5603596e9560, pid=0) at /home/pedro/gdb/binutils-gdb/src/gdb/inferior.c:291
 #7  0x00005603576206e6 in find_inferior_ptid (targ=0x5603596e9560, ptid=...) at /home/pedro/gdb/binutils-gdb/src/gdb/inferior.c:305
 #8  0x00005603577d43ed in remote_target::check_pending_events_prevent_wildcard_vcont (this=0x5603596e9560, may_global_wildcard=0x7fff84fb05f0) at /home/pedro/gdb/binutils-gdb/src/gdb/remote.c:7215
 #9  0x00005603577d2a9c in remote_target::commit_resumed (this=0x5603596e9560) at /home/pedro/gdb/binutils-gdb/src/gdb/remote.c:6680
 ...

pid is 0 in this case because the queued event is a process exit event
with no pid associated:

 (top-gdb) p event->ws
 During symbol reading: .debug_line address at offset 0x563c9a is 0 [in module /home/pedro/gdb/binutils-gdb/build/gdb/gdb]
 $1 = {kind = TARGET_WAITKIND_EXITED, value = {integer = 0, sig = GDB_SIGNAL_0, related_pid = {m_pid = 0, m_lwp = 0, m_tid = 0}, execd_pathname = 0x0, syscall_number = 0}}
 (top-gdb)

This fixes it.

gdb/ChangeLog:

	* remote.c
	(remote_target::check_pending_events_prevent_wildcard_vcont):
	Check whether the event's ptid is not null_ptid before looking up
	the corresponding inferior.

Change-Id: Ia30cf275305ee4dcbbd33f731534cd71d1550eaa
---
 gdb/remote.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

-- 
2.26.2

Comments

Lancelot SIX via Gdb-patches March 17, 2021, 3:27 p.m. | #1
On 2021-03-15 7:43 p.m., Pedro Alves wrote:
> Running gdb.server/stop-reply-no-thread-multi.exp with "maint set

> target-non-stop on" occasionally hit an internal error like this:

> 

>   ...

>   continue

>   Continuing.

>   warning: multi-threaded target stopped without sending a thread-id, using first non-exited thread

>   /home/pedro/gdb/binutils-gdb/src/gdb/inferior.c:291: internal-error: inferior* find_inferior_pid(process_stratum_target*, int): Assertion `pid != 0' failed.

>   A problem internal to GDB has been detected,

>   further debugging may prove unreliable.

> 

>   This is a bug, please report it.

>   FAIL: gdb.server/stop-reply-no-thread-multi.exp: to_disable=Tthread: continue until exit (GDB internal error)

> 

> The backtrace looks like this:

> 

>  ...

>  #5  0x0000560357b0879c in internal_error (file=0x560357be6c18 "/home/pedro/gdb/binutils-gdb/src/gdb/inferior.c", line=291, fmt=0x560357be6b21 "%s: Assertion `%s' failed.") at /home/pedro/gdb/binutils-gdb/src/gdbsupport/errors.cc:55

>  #6  0x000056035762061b in find_inferior_pid (targ=0x5603596e9560, pid=0) at /home/pedro/gdb/binutils-gdb/src/gdb/inferior.c:291

>  #7  0x00005603576206e6 in find_inferior_ptid (targ=0x5603596e9560, ptid=...) at /home/pedro/gdb/binutils-gdb/src/gdb/inferior.c:305

>  #8  0x00005603577d43ed in remote_target::check_pending_events_prevent_wildcard_vcont (this=0x5603596e9560, may_global_wildcard=0x7fff84fb05f0) at /home/pedro/gdb/binutils-gdb/src/gdb/remote.c:7215

>  #9  0x00005603577d2a9c in remote_target::commit_resumed (this=0x5603596e9560) at /home/pedro/gdb/binutils-gdb/src/gdb/remote.c:6680

>  ...

> 

> pid is 0 in this case because the queued event is a process exit event

> with no pid associated:

> 

>  (top-gdb) p event->ws

>  During symbol reading: .debug_line address at offset 0x563c9a is 0 [in module /home/pedro/gdb/binutils-gdb/build/gdb/gdb]

>  $1 = {kind = TARGET_WAITKIND_EXITED, value = {integer = 0, sig = GDB_SIGNAL_0, related_pid = {m_pid = 0, m_lwp = 0, m_tid = 0}, execd_pathname = 0x0, syscall_number = 0}}

>  (top-gdb)

> 

> This fixes it.


Makes sense.  Since people very rarely run the testsuite with "maint set
target-non-stop on", should the test be adjusted to try in that
configuration as well by default?  I know we can't make all tests test
all configurations, but it seems to me like this one has good potential
to uncover other problems.

Simon
Pedro Alves March 19, 2021, 7:32 p.m. | #2
On 17/03/21 15:27, Simon Marchi wrote:

> Makes sense.  Since people very rarely run the testsuite with "maint set

> target-non-stop on", should the test be adjusted to try in that

> configuration as well by default?  I know we can't make all tests test

> all configurations, but it seems to me like this one has good potential

> to uncover other problems.


Good idea.  Here's an updated patch doing that.

From 11a8e3c9611bef123815117cbb6553c564a553ff Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>

Date: Sat, 13 Feb 2021 19:16:44 +0000
Subject: [PATCH] Fix problem exposed by
 gdb.server/stop-reply-no-thread-multi.exp

Running gdb.server/stop-reply-no-thread-multi.exp with "maint set
target-non-stop on" occasionally hit an internal error like this:

  ...
  continue
  Continuing.
  warning: multi-threaded target stopped without sending a thread-id, using first non-exited thread
  /home/pedro/gdb/binutils-gdb/src/gdb/inferior.c:291: internal-error: inferior* find_inferior_pid(process_stratum_target*, int): Assertion `pid != 0' failed.
  A problem internal to GDB has been detected,
  further debugging may prove unreliable.

  This is a bug, please report it.
  FAIL: gdb.server/stop-reply-no-thread-multi.exp: to_disable=Tthread: continue until exit (GDB internal error)

The backtrace looks like this:

 ...
 #5  0x0000560357b0879c in internal_error (file=0x560357be6c18 "/home/pedro/gdb/binutils-gdb/src/gdb/inferior.c", line=291, fmt=0x560357be6b21 "%s: Assertion `%s' failed.") at /home/pedro/gdb/binutils-gdb/src/gdbsupport/errors.cc:55
 #6  0x000056035762061b in find_inferior_pid (targ=0x5603596e9560, pid=0) at /home/pedro/gdb/binutils-gdb/src/gdb/inferior.c:291
 #7  0x00005603576206e6 in find_inferior_ptid (targ=0x5603596e9560, ptid=...) at /home/pedro/gdb/binutils-gdb/src/gdb/inferior.c:305
 #8  0x00005603577d43ed in remote_target::check_pending_events_prevent_wildcard_vcont (this=0x5603596e9560, may_global_wildcard=0x7fff84fb05f0) at /home/pedro/gdb/binutils-gdb/src/gdb/remote.c:7215
 #9  0x00005603577d2a9c in remote_target::commit_resumed (this=0x5603596e9560) at /home/pedro/gdb/binutils-gdb/src/gdb/remote.c:6680
 ...

pid is 0 in this case because the queued event is a process exit event
with no pid associated:

 (top-gdb) p event->ws
 During symbol reading: .debug_line address at offset 0x563c9a is 0 [in module /home/pedro/gdb/binutils-gdb/build/gdb/gdb]
 $1 = {kind = TARGET_WAITKIND_EXITED, value = {integer = 0, sig = GDB_SIGNAL_0, related_pid = {m_pid = 0, m_lwp = 0, m_tid = 0}, execd_pathname = 0x0, syscall_number = 0}}
 (top-gdb)

This fixes it, and adds a "maint set target-non-stop on/off" axis to the testcase.

gdb/ChangeLog:

	* remote.c
	(remote_target::check_pending_events_prevent_wildcard_vcont):
	Check whether the event's ptid is not null_ptid before looking up
	the corresponding inferior.

gdb/testsuite/ChangeLog:

	* gdb.server/stop-reply-no-thread-multi.exp (run_test): Add
	"target_non_stop" parameter and use it.
	(top level): Add "maint set target-non-stop on/off" testing axis.

Change-Id: Ia30cf275305ee4dcbbd33f731534cd71d1550eaa
---
 gdb/remote.c                                   | 10 ++++++----
 .../gdb.server/stop-reply-no-thread-multi.exp  | 18 ++++++++++++++----
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index ae15f416153..a752bd9a4cc 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -7164,14 +7164,16 @@ remote_target::check_pending_events_prevent_wildcard_vcont
 	  || event->ws.kind == TARGET_WAITKIND_VFORKED)
 	*may_global_wildcard = 0;
 
-      struct inferior *inf = find_inferior_ptid (this, event->ptid);
-
       /* 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;
-      if (inf != NULL)
-	get_remote_inferior (inf)->may_wildcard_vcont = false;
+      if (event->ptid != null_ptid)
+	{
+	  inferior *inf = find_inferior_ptid (this, event->ptid);
+	  if (inf != NULL)
+	    get_remote_inferior (inf)->may_wildcard_vcont = false;
+	}
     }
 }
 
diff --git a/gdb/testsuite/gdb.server/stop-reply-no-thread-multi.exp b/gdb/testsuite/gdb.server/stop-reply-no-thread-multi.exp
index 6350f5771e3..50cf10fe313 100644
--- a/gdb/testsuite/gdb.server/stop-reply-no-thread-multi.exp
+++ b/gdb/testsuite/gdb.server/stop-reply-no-thread-multi.exp
@@ -41,10 +41,15 @@ if { [build_executable "failed to prepare" $testfile $srcfile {debug pthreads}]
 }
 
 # Run the tests with different features of GDBserver disabled.
-proc run_test { disable_feature } {
+# TARGET_NON_STOP is passed to "maint set target-non-stop".
+proc run_test { target_non_stop disable_feature } {
     global binfile gdb_prompt decimal hex
+    global GDBFLAGS
 
-    clean_restart ${binfile}
+    save_vars { GDBFLAGS } {
+	append GDBFLAGS " -ex \"maint set target-non-stop $target_non_stop\""
+	clean_restart ${binfile}
+    }
 
     # Make sure we're disconnected, in case we're testing with an
     # extended-remote board, therefore already connected.
@@ -131,6 +136,11 @@ proc run_test { disable_feature } {
 #
 # T: Start GDBserver with the entire 'T' stop reply packet disabled,
 #    GDBserver will instead send the 'S' stop reply.
-foreach_with_prefix to_disable { "" Tthread T } {
-    run_test $to_disable
+#
+# Also test both all-stop and non-stop variants of the remote
+# protocol.
+foreach_with_prefix target-non-stop {"off" "on"} {
+    foreach_with_prefix to_disable { "" Tthread T } {
+	run_test ${target-non-stop} $to_disable
+    }
 }

base-commit: 7b9f985957798ba4dacc454f22c9e426c6897cb8
-- 
2.26.2
Lancelot SIX via Gdb-patches March 25, 2021, 5:25 p.m. | #3
On 2021-03-19 3:32 p.m., Pedro Alves wrote:
> On 17/03/21 15:27, Simon Marchi wrote:

> 

>> Makes sense.  Since people very rarely run the testsuite with "maint set

>> target-non-stop on", should the test be adjusted to try in that

>> configuration as well by default?  I know we can't make all tests test

>> all configurations, but it seems to me like this one has good potential

>> to uncover other problems.

> 

> Good idea.  Here's an updated patch doing that.


LGTM, thanks.

Simon

Patch

diff --git a/gdb/remote.c b/gdb/remote.c
index ae15f416153..a752bd9a4cc 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -7164,14 +7164,16 @@  remote_target::check_pending_events_prevent_wildcard_vcont
 	  || event->ws.kind == TARGET_WAITKIND_VFORKED)
 	*may_global_wildcard = 0;
 
-      struct inferior *inf = find_inferior_ptid (this, event->ptid);
-
       /* 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;
-      if (inf != NULL)
-	get_remote_inferior (inf)->may_wildcard_vcont = false;
+      if (event->ptid != null_ptid)
+	{
+	  inferior *inf = find_inferior_ptid (this, event->ptid);
+	  if (inf != NULL)
+	    get_remote_inferior (inf)->may_wildcard_vcont = false;
+	}
     }
 }