[PATCH 0/3] Fix gdbserver + "maint set target-non-stop" problems

[PATCH 0/3] Fix gdbserver + "maint set target-non-stop" problems

[Pedro Alves]

While playing with Simon's commit-resume series, I ran the testsuite
against gdbserver with "maint set target-non-stop on".  That ran into
a few problems.  This mini series fixes a few of them.

Pedro Alves (3):
  Fix any_thread_of_inferior
  Fix bkpt-other-inferior.exp race
  Fix problem exposed by gdb.server/stop-reply-no-thread-multi.exp

 gdb/remote.c                                     | 10 ++++++----
 gdb/testsuite/gdb.server/bkpt-other-inferior.exp | 10 ++++++----
 gdb/thread.c                                     |  4 ++--
 3 files changed, 14 insertions(+), 10 deletions(-)


base-commit: 675da9a57e0ab1c384e0dfd20ddf715a83c18673
-- 
2.26.2

[PATCH 1/3] Fix any_thread_of_inferior

[Pedro Alves]

Running gdb-term.exp against gdbserver with "maint set target-non-stop
on", runs into this:

  [infrun] fetch_inferior_event: exit
  [infrun] fetch_inferior_event: enter
  /home/pedro/gdb/binutils-gdb/src/gdb/thread.c:72: internal-error: thread_info* inferior_thread(): Assertion `current_thread_ != nullptr' failed.
  A problem internal to GDB has been detected,
  further debugging may prove unreliable.

  This is a bug, please report it.  For instructions, see:
  <https://www.gnu.org/software/gdb/bugs/>.

  FAIL: gdb.base/gdb-sigterm.exp: expect eof #2 (GDB internal error)
  Resyncing due to internal error.
  ERROR: : spawn id exp9 not open
      while executing
  "expect {
  -i exp9 -timeout 10
	      -re "Quit this debugging session\\? \\(y or n\\) $" {
		  send_gdb "n\n" answer
		  incr count
	      }
	      -re "Create ..."
      ("uplevel" body line 1)
      invoked from within
  "uplevel $body" NONE : spawn id exp9 not open
  ERROR: Could not resync from internal error (timeout)
  gdb.base/gdb-sigterm.exp: expect eof #2: stepped 0 times
  UNRESOLVED: gdb.base/gdb-sigterm.exp: 50 SIGTERM passes

The assertion fails here:

  ...
  #5  0x000055af4b4a7164 in internal_error (file=0x55af4b5e5de8 "/home/pedro/gdb/binutils-gdb/src/gdb/thread.c", line=72, fmt=0x55af4b5e5ce9 "%s: Assertion `%s' failed.") at /home/pedro/gdb/binutils-gdb/src/gdbsupport/errors.cc:55
  #6  0x000055af4b25fc43 in inferior_thread () at /home/pedro/gdb/binutils-gdb/src/gdb/thread.c:72
  #7  0x000055af4b26177e in any_thread_of_inferior (inf=0x55af4cf874f0) at /home/pedro/gdb/binutils-gdb/src/gdb/thread.c:638
  #8  0x000055af4b26eec8 in kill_or_detach (inf=0x55af4cf874f0, from_tty=0) at /home/pedro/gdb/binutils-gdb/src/gdb/top.c:1665
  #9  0x000055af4b26f37f in quit_force (exit_arg=0x0, from_tty=0) at /home/pedro/gdb/binutils-gdb/src/gdb/top.c:1767
  #10 0x000055af4b2f72a7 in quit () at /home/pedro/gdb/binutils-gdb/src/gdb/utils.c:633
  #11 0x000055af4b2f730b in maybe_quit () at /home/pedro/gdb/binutils-gdb/src/gdb/utils.c:657
  #12 0x000055af4b1adb74 in ser_base_wait_for (scb=0x55af4d02e460, timeout=0) at /home/pedro/gdb/binutils-gdb/src/gdb/ser-base.c:236
  #13 0x000055af4b1adf0f in do_ser_base_readchar (scb=0x55af4d02e460, timeout=0) at /home/pedro/gdb/binutils-gdb/src/gdb/ser-base.c:365
  #14 0x000055af4b1ae06d in generic_readchar (scb=0x55af4d02e460, timeout=0, do_readchar=0x55af4b1adeb1 <do_ser_base_readchar(serial*, int)>) at /home/pedro/gdb/binutils-gdb/src/gdb/ser-base.c:444
  ...

The bug is that any_thread_of_inferior incorrectly assumes that
there's always a selected thread.  This fixes it.

gdb/ChangeLog:

	* thread.c (any_thread_of_inferior): Check if there's a selected
	thread before calling inferior_thread().

Change-Id: Ica4b9ec746121a7a7c22bef09baea72103b3853d
---
 gdb/thread.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/thread.c b/gdb/thread.c
index 3e7d6e14bf7..fc6db96fbcb 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -637,8 +637,8 @@ any_thread_of_inferior (inferior *inf)
 {
   gdb_assert (inf->pid != 0);
 
-  /* Prefer the current thread.  */
-  if (inf == current_inferior ())
+  /* Prefer the current thread, if there's one.  */
+  if (inf == current_inferior () && inferior_ptid != null_ptid)
     return inferior_thread ();
 
   for (thread_info *tp : inf->non_exited_threads ())
-- 
2.26.2

[PATCH 2/3] Fix bkpt-other-inferior.exp race

[Pedro Alves]

gdb.server/bkpt-other-inferior.exp sometimes fails like so:

 (gdb) inferior 2
 [Switching to inferior 2 [process 368191] (<noexec>)]
 [Switching to thread 2.1 (Thread 368191.368191)]
 [remote] Sending packet: $m7ffff7fd0100,1#5b
 [remote] Packet received: 48
 [remote] Sending packet: $m7ffff7fd0100,1#5b
 [remote] Packet received: 48
 [remote] Sending packet: $m7ffff7fd0100,9#63
 [remote] Packet received: 4889e7e8e80c000049
 #0  0x00007ffff7fd0100 in ?? ()
 (gdb) PASS: gdb.server/bkpt-other-inferior.exp: inf 2: switch to inferior
 break -q main
 Breakpoint 2 at 0x1138: file /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.server/server.c, line 21.
 (gdb) PASS: gdb.server/bkpt-other-inferior.exp: inf 2: set breakpoint
 delete breakpoints
 Delete all breakpoints? (y or n) y
 (gdb) [remote] wait: enter
 [remote] wait: exit
 FAIL: gdb.server/bkpt-other-inferior.exp: inf 2: delete all breakpoints in delete_breakpoints (timeout)
 ERROR: breakpoints not deleted
 Remote debugging from host ::1, port 55876
 monitor exit

The problem is here:

 (gdb) [remote] wait: enter

The testcase isn't expecting any output after the prompt.  This fixes
it by removing the anchor.

Change-Id: I2fd152fd9c46b1c5e7fa678cc4d4054dac0b2bd4
---
 gdb/testsuite/gdb.server/bkpt-other-inferior.exp | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/gdb/testsuite/gdb.server/bkpt-other-inferior.exp b/gdb/testsuite/gdb.server/bkpt-other-inferior.exp
index 8992f190d21..df7af35fbb7 100644
--- a/gdb/testsuite/gdb.server/bkpt-other-inferior.exp
+++ b/gdb/testsuite/gdb.server/bkpt-other-inferior.exp
@@ -71,23 +71,25 @@ gdb_test_multiple "file" $test {
 # breakpoint on inferior 1, since it has symbols, and should not
 # result in any access to inferior 2's remote target.
 
-gdb_test_no_output "set debug remote 1"
-
 foreach inf_sel {1 2} {
     with_test_prefix "inf $inf_sel" {
 	gdb_test "inferior $inf_sel" "Switching to inferior $inf_sel.*" \
 	    "switch to inferior"
 
+	gdb_test_no_output "set debug remote 1"
+
 	set test "set breakpoint"
 	gdb_test_multiple "break -q main" $test {
-	    -re "Sending packet.*$gdb_prompt $" {
+	    -re "Sending packet.*$gdb_prompt " {
 		fail $test
 	    }
-	    -re "^break -q main\r\nBreakpoint .* at .*$gdb_prompt $" {
+	    -re "^break -q main\r\nBreakpoint .* at .*$gdb_prompt " {
 		pass $test
 	    }
 	}
 
+	gdb_test_no_output "set debug remote 0"
+
 	delete_breakpoints
     }
 }
-- 
2.26.2

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

[Pedro Alves]

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(-)

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;
+	}
     }
 }
 
-- 
2.26.2

Re: [PATCH 2/3] Fix bkpt-other-inferior.exp race

[Simon Marchi]

On 2021-03-15 7:43 p.m., Pedro Alves wrote:
> gdb.server/bkpt-other-inferior.exp sometimes fails like so:
> 
>  (gdb) inferior 2
>  [Switching to inferior 2 [process 368191] (<noexec>)]
>  [Switching to thread 2.1 (Thread 368191.368191)]
>  [remote] Sending packet: $m7ffff7fd0100,1#5b
>  [remote] Packet received: 48
>  [remote] Sending packet: $m7ffff7fd0100,1#5b
>  [remote] Packet received: 48
>  [remote] Sending packet: $m7ffff7fd0100,9#63
>  [remote] Packet received: 4889e7e8e80c000049
>  #0  0x00007ffff7fd0100 in ?? ()
>  (gdb) PASS: gdb.server/bkpt-other-inferior.exp: inf 2: switch to inferior
>  break -q main
>  Breakpoint 2 at 0x1138: file /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.server/server.c, line 21.
>  (gdb) PASS: gdb.server/bkpt-other-inferior.exp: inf 2: set breakpoint
>  delete breakpoints
>  Delete all breakpoints? (y or n) y
>  (gdb) [remote] wait: enter
>  [remote] wait: exit
>  FAIL: gdb.server/bkpt-other-inferior.exp: inf 2: delete all breakpoints in delete_breakpoints (timeout)
>  ERROR: breakpoints not deleted
>  Remote debugging from host ::1, port 55876
>  monitor exit
> 
> The problem is here:
> 
>  (gdb) [remote] wait: enter
> 
> The testcase isn't expecting any output after the prompt.  This fixes
> it by removing the anchor.

I don't really understand what happens here.  When I replicate the test
manually, I don't see this pair of enter/exit.  Also, since the test
specifically exists to ensure there won't be communcation with the
remote target when inserting the breakpoint, why should we expect the
remote target's wait to get called?

Is there a way to hack something to make the failure happen all the
time?

Simon

Re: [PATCH 1/3] Fix any_thread_of_inferior

[Simon Marchi]

On 2021-03-15 7:43 p.m., Pedro Alves wrote:
> Running gdb-term.exp against gdbserver with "maint set target-non-stop
> on", runs into this:
> 
>   [infrun] fetch_inferior_event: exit
>   [infrun] fetch_inferior_event: enter
>   /home/pedro/gdb/binutils-gdb/src/gdb/thread.c:72: internal-error: thread_info* inferior_thread(): Assertion `current_thread_ != nullptr' failed.
>   A problem internal to GDB has been detected,
>   further debugging may prove unreliable.
> 
>   This is a bug, please report it.  For instructions, see:
>   <https://www.gnu.org/software/gdb/bugs/>.
> 
>   FAIL: gdb.base/gdb-sigterm.exp: expect eof #2 (GDB internal error)
>   Resyncing due to internal error.
>   ERROR: : spawn id exp9 not open
>       while executing
>   "expect {
>   -i exp9 -timeout 10
> 	      -re "Quit this debugging session\\? \\(y or n\\) $" {
> 		  send_gdb "n\n" answer
> 		  incr count
> 	      }
> 	      -re "Create ..."
>       ("uplevel" body line 1)
>       invoked from within
>   "uplevel $body" NONE : spawn id exp9 not open
>   ERROR: Could not resync from internal error (timeout)
>   gdb.base/gdb-sigterm.exp: expect eof #2: stepped 0 times
>   UNRESOLVED: gdb.base/gdb-sigterm.exp: 50 SIGTERM passes
> 
> The assertion fails here:
> 
>   ...
>   #5  0x000055af4b4a7164 in internal_error (file=0x55af4b5e5de8 "/home/pedro/gdb/binutils-gdb/src/gdb/thread.c", line=72, fmt=0x55af4b5e5ce9 "%s: Assertion `%s' failed.") at /home/pedro/gdb/binutils-gdb/src/gdbsupport/errors.cc:55
>   #6  0x000055af4b25fc43 in inferior_thread () at /home/pedro/gdb/binutils-gdb/src/gdb/thread.c:72
>   #7  0x000055af4b26177e in any_thread_of_inferior (inf=0x55af4cf874f0) at /home/pedro/gdb/binutils-gdb/src/gdb/thread.c:638
>   #8  0x000055af4b26eec8 in kill_or_detach (inf=0x55af4cf874f0, from_tty=0) at /home/pedro/gdb/binutils-gdb/src/gdb/top.c:1665
>   #9  0x000055af4b26f37f in quit_force (exit_arg=0x0, from_tty=0) at /home/pedro/gdb/binutils-gdb/src/gdb/top.c:1767
>   #10 0x000055af4b2f72a7 in quit () at /home/pedro/gdb/binutils-gdb/src/gdb/utils.c:633
>   #11 0x000055af4b2f730b in maybe_quit () at /home/pedro/gdb/binutils-gdb/src/gdb/utils.c:657
>   #12 0x000055af4b1adb74 in ser_base_wait_for (scb=0x55af4d02e460, timeout=0) at /home/pedro/gdb/binutils-gdb/src/gdb/ser-base.c:236
>   #13 0x000055af4b1adf0f in do_ser_base_readchar (scb=0x55af4d02e460, timeout=0) at /home/pedro/gdb/binutils-gdb/src/gdb/ser-base.c:365
>   #14 0x000055af4b1ae06d in generic_readchar (scb=0x55af4d02e460, timeout=0, do_readchar=0x55af4b1adeb1 <do_ser_base_readchar(serial*, int)>) at /home/pedro/gdb/binutils-gdb/src/gdb/ser-base.c:444
>   ...
> 
> The bug is that any_thread_of_inferior incorrectly assumes that
> there's always a selected thread.  This fixes it.
> 
> gdb/ChangeLog:
> 
> 	* thread.c (any_thread_of_inferior): Check if there's a selected
> 	thread before calling inferior_thread().
> 
> Change-Id: Ica4b9ec746121a7a7c22bef09baea72103b3853d
> ---
>  gdb/thread.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/thread.c b/gdb/thread.c
> index 3e7d6e14bf7..fc6db96fbcb 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -637,8 +637,8 @@ any_thread_of_inferior (inferior *inf)
>  {
>    gdb_assert (inf->pid != 0);
>  
> -  /* Prefer the current thread.  */
> -  if (inf == current_inferior ())
> +  /* Prefer the current thread, if there's one.  */
> +  if (inf == current_inferior () && inferior_ptid != null_ptid)
>      return inferior_thread ();
>  
>    for (thread_info *tp : inf->non_exited_threads ())
> 

Makes sense.  current_thread_ is always supposed to be in sync with
inferior_ptid because we are always supposed to use switch_to_thread or
similar function to switch thread, right?

Simon

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

[Simon Marchi]

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

Re: [PATCH 2/3] Fix bkpt-other-inferior.exp race

[Pedro Alves]

On 17/03/21 15:12, Simon Marchi wrote:
> 
> 
> On 2021-03-15 7:43 p.m., Pedro Alves wrote:
>> gdb.server/bkpt-other-inferior.exp sometimes fails like so:
>>
>>  (gdb) inferior 2
>>  [Switching to inferior 2 [process 368191] (<noexec>)]
>>  [Switching to thread 2.1 (Thread 368191.368191)]
>>  [remote] Sending packet: $m7ffff7fd0100,1#5b
>>  [remote] Packet received: 48
>>  [remote] Sending packet: $m7ffff7fd0100,1#5b
>>  [remote] Packet received: 48
>>  [remote] Sending packet: $m7ffff7fd0100,9#63
>>  [remote] Packet received: 4889e7e8e80c000049
>>  #0  0x00007ffff7fd0100 in ?? ()
>>  (gdb) PASS: gdb.server/bkpt-other-inferior.exp: inf 2: switch to inferior
>>  break -q main
>>  Breakpoint 2 at 0x1138: file /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.server/server.c, line 21.
>>  (gdb) PASS: gdb.server/bkpt-other-inferior.exp: inf 2: set breakpoint
>>  delete breakpoints
>>  Delete all breakpoints? (y or n) y
>>  (gdb) [remote] wait: enter
>>  [remote] wait: exit
>>  FAIL: gdb.server/bkpt-other-inferior.exp: inf 2: delete all breakpoints in delete_breakpoints (timeout)
>>  ERROR: breakpoints not deleted
>>  Remote debugging from host ::1, port 55876
>>  monitor exit
>>
>> The problem is here:
>>
>>  (gdb) [remote] wait: enter
>>
>> The testcase isn't expecting any output after the prompt.  This fixes
>> it by removing the anchor.
> 
> I don't really understand what happens here.  When I replicate the test
> manually, I don't see this pair of enter/exit.  

Sorry, I forget mentioning in the commit log that this triggers
with "maint set target-non-stop on".

If I run the testcase in a loop like this:

 $ (set -e; while true; do make check RUNTESTFLAGS="--target_board=native-extended-gdbserver-asns" TESTS="*/bkpt-other-inferior.exp"; done)

using this board file:

 $ cat native-extended-gdbserver-asns.exp 
 load_board_description "native-extended-gdbserver"

 set GDBFLAGS "${GDBFLAGS} -ex \"maint set target-non-stop on\""
 
 send_user "configuring for gdbserver local testing (extended-remote, as-ns)\n"

... then it fails after a few iterations.

Even on a passing run, I see the "[remote] wait:" in the log.  It's just
a timing issue, most times expect consumes the GDB prompt before GDB outputs
that remote log print.

> Also, since the test
> specifically exists to ensure there won't be communcation with the
> remote target when inserting the breakpoint, why should we expect the
> remote target's wait to get called?
> 

See updated commit log in the updated patch below.

From b6b70ac7266589a3eddd58ac0e7263fe8181dff3 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Sat, 13 Feb 2021 16:25:26 +0000
Subject: [PATCH] Fix bkpt-other-inferior.exp race

When testing with "maint set target-non-stop on",
gdb.server/bkpt-other-inferior.exp sometimes fails like so:

 (gdb) inferior 2
 [Switching to inferior 2 [process 368191] (<noexec>)]
 [Switching to thread 2.1 (Thread 368191.368191)]
 [remote] Sending packet: $m7ffff7fd0100,1#5b
 [remote] Packet received: 48
 [remote] Sending packet: $m7ffff7fd0100,1#5b
 [remote] Packet received: 48
 [remote] Sending packet: $m7ffff7fd0100,9#63
 [remote] Packet received: 4889e7e8e80c000049
 #0  0x00007ffff7fd0100 in ?? ()
 (gdb) PASS: gdb.server/bkpt-other-inferior.exp: inf 2: switch to inferior
 break -q main
 Breakpoint 2 at 0x1138: file /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.server/server.c, line 21.
 (gdb) PASS: gdb.server/bkpt-other-inferior.exp: inf 2: set breakpoint
 delete breakpoints
 Delete all breakpoints? (y or n) y
 (gdb) [remote] wait: enter
 [remote] wait: exit
 FAIL: gdb.server/bkpt-other-inferior.exp: inf 2: delete all breakpoints in delete_breakpoints (timeout)
 ERROR: breakpoints not deleted
 Remote debugging from host ::1, port 55876
 monitor exit

The problem is here:

 (gdb) [remote] wait: enter

The testcase isn't expecting any output after the prompt.

Why is that "[remote] wait" output?  What happens is that "delete
breakpoints" queries the user, and `query` disables/reenables target
async, which results in the remote target's async event handler ending
up marked:

 (top-gdb) bt
 #0  mark_async_event_handler (async_handler_ptr=0x556bffffffff) at ../../src/gdb/async-event.c:295
 #1  0x0000556bf71b711f in infrun_async (enable=1) at ../../src/gdb/infrun.c:119
 #2  0x0000556bf7471387 in target_async (enable=1) at ../../src/gdb/target.c:3684
 #3  0x0000556bf748a0bd in gdb_readline_wrapper_cleanup::~gdb_readline_wrapper_cleanup (this=0x7ffe3cf30eb0, __in_chrg=<optimized out>) at ../../src/gdb/top.c:1074
 #4  0x0000556bf74874e2 in gdb_readline_wrapper (prompt=0x556bfa17da60 "Delete all breakpoints? (y or n) ") at ../../src/gdb/top.c:1096
 #5  0x0000556bf75111c5 in defaulted_query(const char *, char, typedef __va_list_tag __va_list_tag *) (ctlstr=0x556bf7717f34 "Delete all breakpoints? ", defchar=0 '\000', args=0x7ffe3cf31020) at ../../src/gdb/utils.c:893
 #6  0x0000556bf751166f in query (ctlstr=0x556bf7717f34 "Delete all breakpoints? ") at ../../src/gdb/utils.c:985
 #7  0x0000556bf6f11404 in delete_command (arg=0x0, from_tty=1) at ../../src/gdb/breakpoint.c:13500
 ...

... which then later results in a target_wait call:

 (top-gdb) bt
 #0  remote_target::wait_ns (this=0x7ffe3cf30f80, ptid=..., status=0xde530314f0802800, options=...) at ../../src/gdb/remote.c:7937
 #1  0x0000556bf7369dcb in remote_target::wait (this=0x556bfa0b2180, ptid=..., status=0x7ffe3cf31568, options=...) at ../../src/gdb/remote.c:8173
 #2  0x0000556bf745e527 in target_wait (ptid=..., status=0x7ffe3cf31568, options=...) at ../../src/gdb/target.c:2000
 #3  0x0000556bf71be686 in do_target_wait_1 (inf=0x556bfa1573d0, ptid=..., status=0x7ffe3cf31568, options=...) at ../../src/gdb/infrun.c:3463
 #4  0x0000556bf71be88b in <lambda(inferior*)>::operator()(inferior *) const (__closure=0x7ffe3cf31320, inf=0x556bfa1573d0) at ../../src/gdb/infrun.c:3526
 #5  0x0000556bf71bebcd in do_target_wait (wait_ptid=..., ecs=0x7ffe3cf31540, options=...) at ../../src/gdb/infrun.c:3539
 #6  0x0000556bf71bf97b in fetch_inferior_event () at ../../src/gdb/infrun.c:3879
 #7  0x0000556bf71a27f8 in inferior_event_handler (event_type=INF_REG_EVENT) at ../../src/gdb/inf-loop.c:42
 #8  0x0000556bf71cc8b7 in infrun_async_inferior_event_handler (data=0x0) at ../../src/gdb/infrun.c:9220
 #9  0x0000556bf6ecb80f in check_async_event_handlers () at ../../src/gdb/async-event.c:327
 #10 0x0000556bf76b011a in gdb_do_one_event () at ../../src/gdbsupport/event-loop.cc:216
 ...

... which returns TARGET_WAITKIND_IGNORE.

Fix this by only enabling remote output around setting the breakpoint.

gdb/testsuite/ChangeLog:

	* gdb.server/bkpt-other-inferior.exp: Only enable remote output
	around setting the breakpoint.

Change-Id: I2fd152fd9c46b1c5e7fa678cc4d4054dac0b2bd4
---
 gdb/testsuite/gdb.server/bkpt-other-inferior.exp | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/gdb.server/bkpt-other-inferior.exp b/gdb/testsuite/gdb.server/bkpt-other-inferior.exp
index 8992f190d21..5afe621de71 100644
--- a/gdb/testsuite/gdb.server/bkpt-other-inferior.exp
+++ b/gdb/testsuite/gdb.server/bkpt-other-inferior.exp
@@ -71,13 +71,13 @@ gdb_test_multiple "file" $test {
 # breakpoint on inferior 1, since it has symbols, and should not
 # result in any access to inferior 2's remote target.
 
-gdb_test_no_output "set debug remote 1"
-
 foreach inf_sel {1 2} {
     with_test_prefix "inf $inf_sel" {
 	gdb_test "inferior $inf_sel" "Switching to inferior $inf_sel.*" \
 	    "switch to inferior"
 
+	gdb_test_no_output "set debug remote 1"
+
 	set test "set breakpoint"
 	gdb_test_multiple "break -q main" $test {
 	    -re "Sending packet.*$gdb_prompt $" {
@@ -88,6 +88,8 @@ foreach inf_sel {1 2} {
 	    }
 	}
 
+	gdb_test_no_output "set debug remote 0"
+
 	delete_breakpoints
     }
 }

base-commit: 7b9f985957798ba4dacc454f22c9e426c6897cb8
-- 
2.26.2

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

[Pedro Alves]

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

Re: [PATCH 1/3] Fix any_thread_of_inferior

[Pedro Alves]

On 17/03/21 15:14, Simon Marchi wrote:

> On 2021-03-15 7:43 p.m., Pedro Alves wrote:

>> diff --git a/gdb/thread.c b/gdb/thread.c
>> index 3e7d6e14bf7..fc6db96fbcb 100644
>> --- a/gdb/thread.c
>> +++ b/gdb/thread.c
>> @@ -637,8 +637,8 @@ any_thread_of_inferior (inferior *inf)
>>  {
>>    gdb_assert (inf->pid != 0);
>>  
>> -  /* Prefer the current thread.  */
>> -  if (inf == current_inferior ())
>> +  /* Prefer the current thread, if there's one.  */
>> +  if (inf == current_inferior () && inferior_ptid != null_ptid)
>>      return inferior_thread ();
>>  
>>    for (thread_info *tp : inf->non_exited_threads ())
>>
> 
> Makes sense.  current_thread_ is always supposed to be in sync with
> inferior_ptid because we are always supposed to use switch_to_thread or
> similar function to switch thread, right?

Right.  Part of the multi-target work was ensuring that was done throughout
the codebase.

I've merged this patch.

Thanks,
Pedro Alves

Re: [PATCH 2/3] Fix bkpt-other-inferior.exp race

[Simon Marchi]

On 2021-03-19 3:11 p.m., Pedro Alves wrote:
> On 17/03/21 15:12, Simon Marchi wrote:
>>
>>
>> On 2021-03-15 7:43 p.m., Pedro Alves wrote:
>>> gdb.server/bkpt-other-inferior.exp sometimes fails like so:
>>>
>>>  (gdb) inferior 2
>>>  [Switching to inferior 2 [process 368191] (<noexec>)]
>>>  [Switching to thread 2.1 (Thread 368191.368191)]
>>>  [remote] Sending packet: $m7ffff7fd0100,1#5b
>>>  [remote] Packet received: 48
>>>  [remote] Sending packet: $m7ffff7fd0100,1#5b
>>>  [remote] Packet received: 48
>>>  [remote] Sending packet: $m7ffff7fd0100,9#63
>>>  [remote] Packet received: 4889e7e8e80c000049
>>>  #0  0x00007ffff7fd0100 in ?? ()
>>>  (gdb) PASS: gdb.server/bkpt-other-inferior.exp: inf 2: switch to inferior
>>>  break -q main
>>>  Breakpoint 2 at 0x1138: file /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.server/server.c, line 21.
>>>  (gdb) PASS: gdb.server/bkpt-other-inferior.exp: inf 2: set breakpoint
>>>  delete breakpoints
>>>  Delete all breakpoints? (y or n) y
>>>  (gdb) [remote] wait: enter
>>>  [remote] wait: exit
>>>  FAIL: gdb.server/bkpt-other-inferior.exp: inf 2: delete all breakpoints in delete_breakpoints (timeout)
>>>  ERROR: breakpoints not deleted
>>>  Remote debugging from host ::1, port 55876
>>>  monitor exit
>>>
>>> The problem is here:
>>>
>>>  (gdb) [remote] wait: enter
>>>
>>> The testcase isn't expecting any output after the prompt.  This fixes
>>> it by removing the anchor.
>>
>> I don't really understand what happens here.  When I replicate the test
>> manually, I don't see this pair of enter/exit.  
> 
> Sorry, I forget mentioning in the commit log that this triggers
> with "maint set target-non-stop on".
> 
> If I run the testcase in a loop like this:
> 
>  $ (set -e; while true; do make check RUNTESTFLAGS="--target_board=native-extended-gdbserver-asns" TESTS="*/bkpt-other-inferior.exp"; done)
> 
> using this board file:
> 
>  $ cat native-extended-gdbserver-asns.exp 
>  load_board_description "native-extended-gdbserver"
> 
>  set GDBFLAGS "${GDBFLAGS} -ex \"maint set target-non-stop on\""
>  
>  send_user "configuring for gdbserver local testing (extended-remote, as-ns)\n"
> 
> ... then it fails after a few iterations.
> 
> Even on a passing run, I see the "[remote] wait:" in the log.  It's just
> a timing issue, most times expect consumes the GDB prompt before GDB outputs
> that remote log print.

Ok, I see.

I think these debug outputs are useful (that's why I added them), but
it's a shame that they interfere with the testing...

That LGTM then.

Simon

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

[Simon Marchi]

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