[review] infrun: mark an exited thread non-executing when attempting to stop

[review] infrun: mark an exited thread non-executing when attempting to stop

[Tankut Baris Aktemur (Code Review)]

Tankut Baris Aktemur has uploaded a new change for review.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/133
......................................................................

infrun: mark an exited thread non-executing when attempting to stop

In stop_all_threads, GDB sends signals to other threads in an attempt
to stop them.  While in a typical scenario the expected wait status is
TARGET_WAITKIND_STOPPED, it is possible that the thread GDB attempted
to stop has already terminated.  If so, a waitstatus other than
TARGET_WAITKIND_STOPPED would be received.  In that case, mark the
thread as not-executing and set its state to THREAD_EXITED.

If a wait status that denotes thread termination is ignored, GDB goes
into an infinite loop in stop_all_threads.
E.g.:

~~~
$ gdb ./a.out
(gdb) start
...
(gdb) add-inferior -exec ./a.out
...
(gdb) inferior 2
...
(gdb) start
...
(gdb) set schedule-multiple on
(gdb) set debug infrun 2
(gdb) continue
Continuing.
infrun: clear_proceed_status_thread (process 23419)
infrun: clear_proceed_status_thread (process 23703)
infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
infrun: proceed: resuming process 23419
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 23419] at 0x55555555514e
infrun: infrun_async(1)
infrun: prepare_to_wait
infrun: proceed: resuming process 23703
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 23703] at 0x55555555514e
infrun: prepare_to_wait
infrun: target_wait (-1.0.0, status) =
infrun:   23703.23703.0 [process 23703],
infrun:   status->kind = exited, status = 42
infrun: handle_inferior_event status->kind = exited, status = 42
[Inferior 2 (process 23703) exited with code 052]
infrun: stop_waiting
infrun: stop_all_threads
infrun: stop_all_threads, pass=0, iterations=0
infrun:   process 23419 executing, need stop
infrun: target_wait (-1.0.0, status) =
infrun:   23419.23419.0 [process 23419],
infrun:   status->kind = exited, status = 42
infrun: stop_all_threads status->kind = exited, status = 42 process 23419
infrun:   process 23419 executing, already stopping
infrun: target_wait (-1.0.0, status) =
infrun:   -1.0.0 [process -1],
infrun:   status->kind = no-resumed
infrun: stop_all_threads status->kind = no-resumed process -1
infrun:   process 23419 executing, already stopping
infrun: target_wait (-1.0.0, status) =
infrun:   -1.0.0 [process -1],
infrun:   status->kind = no-resumed
infrun: stop_all_threads status->kind = no-resumed process -1
infrun:   process 23419 executing, already stopping
infrun: target_wait (-1.0.0, status) =
infrun:   -1.0.0 [process -1],
infrun:   status->kind = no-resumed
infrun: stop_all_threads status->kind = no-resumed process -1
infrun:   process 23419 executing, already stopping
infrun: target_wait (-1.0.0, status) =
infrun:   -1.0.0 [process -1],
infrun:   status->kind = no-resumed
infrun: stop_all_threads status->kind = no-resumed process -1
infrun:   process 23419 executing, already stopping
~~~

And this polling goes on forever.  This patch prevents the infinite
looping behavior.

gdb/ChangeLog:
2019-10-18  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* infrun.c (stop_all_threads): Do NOT ignore
	TARGET_WAITKIND_NO_RESUMED, TARGET_WAITKIND_THREAD_EXITED,
	TARGET_WAITKIND_EXITED, TARGET_WAITKIND_SIGNALLED wait statuses
	received from threads we attempt to stop; mark the corresponding
	thread as THREAD_EXITED and not-executing.

Change-Id: I7cec98f40283773b79255d998511da434e9cd408
---
M gdb/infrun.c
1 file changed, 9 insertions(+), 0 deletions(-)



diff --git a/gdb/infrun.c b/gdb/infrun.c
index 66a066f..01fcbf6 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4383,6 +4383,15 @@
 	    {
 	      /* All resumed threads exited
 		 or one thread/process exited/signalled.  */
+	      thread_info *t = find_thread_ptid (event_ptid);
+	      if (t != nullptr)
+		{
+		  t->stop_requested = 0;
+		  t->executing = 0;
+		  t->resumed = 0;
+		  t->control.may_range_step = 0;
+		  t->state = THREAD_EXITED;
+		}
 	    }
 	  else
 	    {

[review] infrun: mark an exited thread non-executing when attempting to stop

[Tankut Baris Aktemur (Code Review)]

Tankut Baris Aktemur has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/133
......................................................................


Patch Set 1:

> Patch Set 1: Code-Review+1
> 
> This looks like an old oversight when handling the case of exited threads when we're attempting to stop all of them. Looking at older code, we used to have a message saying a thread had exited while we were stopping it, but it was removed by a cleanup.
> 
> The change looks good for me.

Thank you.  I noticed that the patch has a problem.  It leaves the exited inferior in an alive state with no threads.  It becomes not possible to re-run the program.  I will send a revision, together with updated tests.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I7cec98f40283773b79255d998511da434e9cd408
Gerrit-Change-Number: 133
Gerrit-PatchSet: 1
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Comment-Date: Mon, 04 Nov 2019 08:35:58 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] infrun: handle already-exited threads when attempting to stop

[Tankut Baris Aktemur (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/133
......................................................................

infrun: handle already-exited threads when attempting to stop

In stop_all_threads, GDB sends signals to other threads in an attempt
to stop them.  While in a typical scenario the expected wait status is
TARGET_WAITKIND_STOPPED, it is possible that the thread GDB attempted
to stop has already terminated.  If so, a waitstatus other than
TARGET_WAITKIND_STOPPED would be received.  Handle this case
appropriately.

If a wait status that denotes thread termination is ignored, GDB goes
into an infinite loop in stop_all_threads.
E.g.:

~~~
$ gdb ./a.out
(gdb) start
...
(gdb) add-inferior -exec ./a.out
...
(gdb) inferior 2
...
(gdb) start
...
(gdb) set schedule-multiple on
(gdb) set debug infrun 2
(gdb) continue
Continuing.
infrun: clear_proceed_status_thread (process 23419)
infrun: clear_proceed_status_thread (process 23703)
infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
infrun: proceed: resuming process 23419
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 23419] at 0x55555555514e
infrun: infrun_async(1)
infrun: prepare_to_wait
infrun: proceed: resuming process 23703
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 23703] at 0x55555555514e
infrun: prepare_to_wait
infrun: target_wait (-1.0.0, status) =
infrun:   23703.23703.0 [process 23703],
infrun:   status->kind = exited, status = 0
infrun: handle_inferior_event status->kind = exited, status = 0
[Inferior 2 (process 23703) exited normally]
infrun: stop_waiting
infrun: stop_all_threads
infrun: stop_all_threads, pass=0, iterations=0
infrun:   process 23419 executing, need stop
infrun: target_wait (-1.0.0, status) =
infrun:   23419.23419.0 [process 23419],
infrun:   status->kind = exited, status = 0
infrun: stop_all_threads status->kind = exited, status = 0 process 23419
infrun:   process 23419 executing, already stopping
infrun: target_wait (-1.0.0, status) =
infrun:   -1.0.0 [process -1],
infrun:   status->kind = no-resumed
infrun: stop_all_threads status->kind = no-resumed process -1
infrun:   process 23419 executing, already stopping
infrun: target_wait (-1.0.0, status) =
infrun:   -1.0.0 [process -1],
infrun:   status->kind = no-resumed
infrun: stop_all_threads status->kind = no-resumed process -1
infrun:   process 23419 executing, already stopping
infrun: target_wait (-1.0.0, status) =
infrun:   -1.0.0 [process -1],
infrun:   status->kind = no-resumed
infrun: stop_all_threads status->kind = no-resumed process -1
infrun:   process 23419 executing, already stopping
infrun: target_wait (-1.0.0, status) =
infrun:   -1.0.0 [process -1],
infrun:   status->kind = no-resumed
infrun: stop_all_threads status->kind = no-resumed process -1
infrun:   process 23419 executing, already stopping
~~~

And this polling goes on forever.  This patch prevents the infinite
looping behavior.  For the same scenario above, we obtain the
following behavior:

~~~
...
(gdb) continue
Continuing.
infrun: clear_proceed_status_thread (process 1635)
infrun: clear_proceed_status_thread (process 1763)
infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
infrun: proceed: resuming process 1635
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 1635] at 0x55555555514e
infrun: infrun_async(1)
infrun: prepare_to_wait
infrun: proceed: resuming process 1763
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 1763] at 0x55555555514e
infrun: prepare_to_wait
infrun: target_wait (-1.0.0, status) =
infrun:   1635.1635.0 [process 1635],
infrun:   status->kind = exited, status = 0
infrun: handle_inferior_event status->kind = exited, status = 0
[Inferior 1 (process 1635) exited normally]
infrun: stop_waiting
infrun: stop_all_threads
infrun: stop_all_threads, pass=0, iterations=0
infrun:   process 1763 executing, need stop
infrun: target_wait (-1.0.0, status) =
infrun:   1763.1763.0 [process 1763],
infrun:   status->kind = exited, status = 0
infrun: stop_all_threads status->kind = exited, status = 0 process 1763
[Inferior 2 (process 1763) exited normally]
infrun: stop_all_threads, pass=1, iterations=1
infrun: stop_all_threads done
(gdb) info inferiors
  Num  Description       Executable
* 1    <null>            /path/to/a.out
  2    <null>            /path/to/a.out
(gdb) info threads
No threads.
(gdb)
~~~

gdb/ChangeLog:
2019-11-04  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* infrun.c (stop_all_threads): Do NOT ignore
	TARGET_WAITKIND_NO_RESUMED, TARGET_WAITKIND_THREAD_EXITED,
	TARGET_WAITKIND_EXITED, TARGET_WAITKIND_SIGNALLED wait statuses
	received from threads we attempt to stop.

Change-Id: I7cec98f40283773b79255d998511da434e9cd408
---
M gdb/infrun.c
1 file changed, 29 insertions(+), 2 deletions(-)



diff --git a/gdb/infrun.c b/gdb/infrun.c
index f628fd1..45aa1d9 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4489,8 +4489,35 @@
 	      || ws.kind == TARGET_WAITKIND_EXITED
 	      || ws.kind == TARGET_WAITKIND_SIGNALLED)
 	    {
-	      /* All resumed threads exited
-		 or one thread/process exited/signalled.  */
+	      if (event_ptid != minus_one_ptid)
+		{
+		  thread_info *t = find_thread_ptid (event_ptid);
+		  if (t == nullptr)
+		    {
+		      /* This is the first time we see this thread.  */
+		      t = add_thread (event_ptid);
+		    }
+
+		  /* Set the threads as non-executing to avoid infinitely
+		     waiting for them to stop.  */
+		  mark_non_executing_threads (event_ptid, ws);
+
+		  if (ws.kind == TARGET_WAITKIND_NO_RESUMED)
+		    {
+		      /* Do nothing.  Already marked the threads.  */
+		    }
+		  if (ws.kind == TARGET_WAITKIND_THREAD_EXITED)
+		    delete_thread (t);
+		  else
+		    {
+		      /* TARGET_WAITKIND_EXITED or
+			 TARGET_WAITKIND_SIGNALLED.  */
+		      /* Need to restore the context because
+			 handle_inferior_exit switches it.  */
+		      scoped_restore_current_pspace_and_thread restore;
+		      handle_inferior_exit (event_ptid, ws);
+		    }
+		}
 	    }
 	  else
 	    {

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I7cec98f40283773b79255d998511da434e9cd408
Gerrit-Change-Number: 133
Gerrit-PatchSet: 2
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-MessageType: newpatchset

[review v2] infrun: handle already-exited threads when attempting to stop

[Tankut Baris Aktemur (Code Review)]

Tankut Baris Aktemur has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/133
......................................................................


Patch Set 2:

Kindly pinging.
Thanks.

-Baris


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I7cec98f40283773b79255d998511da434e9cd408
Gerrit-Change-Number: 133
Gerrit-PatchSet: 2
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Comment-Date: Wed, 04 Dec 2019 12:02:10 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] infrun: handle already-exited threads when attempting to stop

[Pedro Alves (Code Review)]

Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/133
......................................................................


Patch Set 2:

> Kindly pinging.
> Thanks.

Thanks.  I'm taking a look.

(FWIW, I'm skimmed this before and put it off because it seemingly didn't come with a testcase.
But today, while digging deeper, I see that this is actually part of a patch series, which _does_ include tests.  Thanks for that.  This is an instance of my annoyance with gerrit's lack of cover letter & threading.)


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I7cec98f40283773b79255d998511da434e9cd408
Gerrit-Change-Number: 133
Gerrit-PatchSet: 2
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Comment-Date: Thu, 05 Dec 2019 19:25:56 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] infrun: handle already-exited threads when attempting to stop

[Pedro Alves (Code Review)]

Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/133
......................................................................


Patch Set 2:

(1 comment)

| --- gdb/infrun.c
| +++ gdb/infrun.c
| @@ -4494,7 +4509,19 @@ stop_all_threads (void)
| +		  if (ws.kind == TARGET_WAITKIND_THREAD_EXITED)
| +		    delete_thread (t);
| +		  else
| +		    {
| +		      /* TARGET_WAITKIND_EXITED or
| +			 TARGET_WAITKIND_SIGNALLED.  */
| +		      /* Need to restore the context because
| +			 handle_inferior_exit switches it.  */
| +		      scoped_restore_current_pspace_and_thread restore;
| +		      handle_inferior_exit (event_ptid, ws);

PS2, Line 4518:

Sorry, but this doesn't look right.

We're inside stop_all_threads, processing some other event, and a
process exit event for one of the processes we're trying to stop comes
along, and this processes it right away.  Once the stop_all_threads
dance is done, we go back to handling the original event, and possibly
reporting a stop to the user.  Meanwhile, whatever state that was set
by handle_inferior_exit, like e.g. $_exitcode, is lost, or now
incorrect for the reported stop.  We also never present a stop on the
CLI for that "spurious" process exit.  Here:

 (gdb)
 continue
 Continuing.
 Executing on build: kill -9 29412 29417    (timeout = 300)
 spawn -ignore SIGHUP kill -9 29412 29417

 Program terminated with signal SIGKILL, Killed.
 The program no longer exists.
     <<<<<<<<<<< no user-visible stop / prompt here. <<<<<<<<<<<<<<<
 Program terminated with signal SIGKILL, Killed.
 The program no longer exists.
 (gdb) PASS: gdb.multi/multi-kill.exp: iteration 1: back to gdb prompt

The fix for this I think must be around leaving the
TARGET_WAITKIND_EXITED/TARGET_WAITKIND_SIGNALLED
event pending, so that it is processed later when we're out of the
stop_all_threads loop and back to
dequeuing the next event.

gdb/linux-nat.c also has its own "stop all threads temporarily" logic,
and that does that -- leaves process exits pending.  See wait_lwp:

              /* If this is the leader exiting, it means the whole
                 process is gone.  Store the status to report to the
                 core.  Store it in lp->waitstatus, because lp->status
                 would be ambiguous (W_EXITCODE(0,0) == 0).  */
              store_waitstatus (&lp->waitstatus, status);
              return 0;

| +		    }
| +		}
|  	    }
|  	  else
|  	    {
|  	      thread_info *t = find_thread_ptid (event_ptid);
|  	      if (t == NULL)
|  		t = add_thread (event_ptid);
|  

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I7cec98f40283773b79255d998511da434e9cd408
Gerrit-Change-Number: 133
Gerrit-PatchSet: 2
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Comment-Date: Fri, 06 Dec 2019 16:19:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] infrun: handle already-exited threads when attempting to stop

[Tankut Baris Aktemur (Code Review)]

Tankut Baris Aktemur has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/133
......................................................................


Patch Set 2:

(1 comment)

| --- gdb/infrun.c
| +++ gdb/infrun.c
| @@ -4494,7 +4509,19 @@ stop_all_threads (void)
| +		  if (ws.kind == TARGET_WAITKIND_THREAD_EXITED)
| +		    delete_thread (t);
| +		  else
| +		    {
| +		      /* TARGET_WAITKIND_EXITED or
| +			 TARGET_WAITKIND_SIGNALLED.  */
| +		      /* Need to restore the context because
| +			 handle_inferior_exit switches it.  */
| +		      scoped_restore_current_pspace_and_thread restore;
| +		      handle_inferior_exit (event_ptid, ws);

PS2, Line 4518:

Thanks for your comment.  I'll work on making the event pending and
then submit a revision.

| +		    }
| +		}
|  	    }
|  	  else
|  	    {
|  	      thread_info *t = find_thread_ptid (event_ptid);
|  	      if (t == NULL)
|  		t = add_thread (event_ptid);
|  

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I7cec98f40283773b79255d998511da434e9cd408
Gerrit-Change-Number: 133
Gerrit-PatchSet: 2
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Comment-Date: Fri, 06 Dec 2019 17:41:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Pedro Alves <palves@redhat.com>
Gerrit-MessageType: comment

[review v2] infrun: handle already-exited threads when attempting to stop

[Tankut Baris Aktemur (Code Review)]

Tankut Baris Aktemur has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/133
......................................................................


Patch Set 2:

(1 comment)

> Patch Set 2:
> 
> (1 comment)

| --- gdb/infrun.c
| +++ gdb/infrun.c
| @@ -4494,7 +4509,19 @@ stop_all_threads (void)
| +		  if (ws.kind == TARGET_WAITKIND_THREAD_EXITED)
| +		    delete_thread (t);
| +		  else
| +		    {
| +		      /* TARGET_WAITKIND_EXITED or
| +			 TARGET_WAITKIND_SIGNALLED.  */
| +		      /* Need to restore the context because
| +			 handle_inferior_exit switches it.  */
| +		      scoped_restore_current_pspace_and_thread restore;
| +		      handle_inferior_exit (event_ptid, ws);

PS2, Line 4518:

> The fix for this I think must be around leaving the TARGET_WAITKIND_EXITED/TARGET_WAITKIND_SIGNALLED
event pending, so that it is processed later when we're out of the
stop_all_threads loop and back to
dequeuing the next event.


I'd like to ask for your opinion on making the second exit event
pending.  One problem is, because the event has not been reported to
the user yet, the user still thinks that the inferior is alive.  So,
after getting the prompt because of the first exit event, they may be
tempted to do "info threads" or switch to the not-yet-reported-
inferior and inspect its state.  This triggers a query (e.g. of
registers) on the process that is already gone.  I tried the following
scenario with the current master branch (the patch that I proposed was
not applied):

~~~
$ gdb ./a.out
(gdb) maint set target-non-stop off
(gdb) start
...
(gdb) add-inferior -exec ./a.out
[New inferior 2]
Added inferior 2
...
(gdb) inferior 2
[Switching to inferior 2 [<null>] (/tmp/a.out)]
(gdb) start
...
(gdb) set schedule-multiple on
(gdb) c
Continuing.
[Inferior 2 (process 16331) exited normally]
(gdb) i inferiors
  Num  Description       Executable
  1    process 16137     /tmp/a.out
* 2    <null>            /tmp/a.out
(gdb) inferior 1
[Switching to inferior 1 [process 16137] (/tmp/a.out)]
[Switching to thread 1.1 (process 16137)]
Couldn't get registers: No such process.
(gdb) i threads
  Id   Target Id         Frame
Couldn't get registers: No such process.
(gdb) c
Continuing.
Couldn't get registers: No such process.
~~~

If I save the exit event in my patch as a pending event (and omit
'maint set target-non-stop off'), I get essentially the same problem.
What is the expected GDB behavior here?  Would it be alright to
actually print both exit events, followed by the gdb-prompt, where the
user can now query $_exitcode or $_exitsignal by switching between
inferiors, assuming those special variables are set correctly per
inferior?

| +		    }
| +		}
|  	    }
|  	  else
|  	    {
|  	      thread_info *t = find_thread_ptid (event_ptid);
|  	      if (t == NULL)
|  		t = add_thread (event_ptid);
|  

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I7cec98f40283773b79255d998511da434e9cd408
Gerrit-Change-Number: 133
Gerrit-PatchSet: 2
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Comment-Date: Mon, 09 Dec 2019 15:09:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Pedro Alves <palves@redhat.com>
Comment-In-Reply-To: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-MessageType: comment

[review v2] infrun: handle already-exited threads when attempting to stop

[Tankut Baris Aktemur (Code Review)]

Tankut Baris Aktemur has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/133
......................................................................


Patch Set 2:

(1 comment)

| --- gdb/infrun.c
| +++ gdb/infrun.c
| @@ -4494,7 +4509,19 @@ stop_all_threads (void)
| +		  if (ws.kind == TARGET_WAITKIND_THREAD_EXITED)
| +		    delete_thread (t);
| +		  else
| +		    {
| +		      /* TARGET_WAITKIND_EXITED or
| +			 TARGET_WAITKIND_SIGNALLED.  */
| +		      /* Need to restore the context because
| +			 handle_inferior_exit switches it.  */
| +		      scoped_restore_current_pspace_and_thread restore;
| +		      handle_inferior_exit (event_ptid, ws);

PS2, Line 4518:

> > The fix for this I think must be around leaving the TARGET_WAITKIND_EXITED/TARGET_WAITKIND_SIGNALLED
> event pending, so that it is processed later when we're out of the stop_all_threads loop and back to
> dequeuing the next event.
> 
> 
> I'd like to ask for your opinion on making the second exit event pending.
> One problem is, because the event has not been reported to the user yet,
> the user still thinks that the inferior is alive.  So, after getting the
> prompt because of the first exit event, they may be tempted to do "info
> threads" or switch to the not-yet-reported-inferior and inspect its state.  
> This triggers a query (e.g. of registers) on the process that is already 
> gone.  I tried the following scenario with the current master branch
> (the patch that I proposed was not applied):

Kindly pinging.

Thanks,

-Baris

| +		    }
| +		}
|  	    }
|  	  else
|  	    {
|  	      thread_info *t = find_thread_ptid (event_ptid);
|  	      if (t == NULL)
|  		t = add_thread (event_ptid);
|  

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I7cec98f40283773b79255d998511da434e9cd408
Gerrit-Change-Number: 133
Gerrit-PatchSet: 2
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Comment-Date: Wed, 08 Jan 2020 15:57:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Pedro Alves <palves@redhat.com>
Comment-In-Reply-To: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-MessageType: comment

Re: [review v2] infrun: handle already-exited threads when attempting to stop

[Pedro Alves]

Hi,

(I tried to reply via gerrit, but I couldn't find how to hit "Quote"
to quote/reply to this message of yours -- seems like via the web ui you
can only reply to the last comment in a thread, which in this case is a
ping, not the message I intended to quote...)

Sorry for the delay, and thanks for the ping.  I've been thinking on and off
about this.  Below you'll find my current thoughts.

On 12/9/19 3:09 PM, Tankut Baris Aktemur (Code Review) wrote:

> I'd like to ask for your opinion on making the second exit event
> pending.  One problem is, because the event has not been reported to
> the user yet, the user still thinks that the inferior is alive.  So,
> after getting the prompt because of the first exit event, they may be
> tempted to do "info threads" or switch to the not-yet-reported-
> inferior and inspect its state.  This triggers a query (e.g. of
> registers) on the process that is already gone.  I tried the following
> scenario with the current master branch (the patch that I proposed was
> not applied):
> 
> ~~~
> $ gdb ./a.out
> (gdb) maint set target-non-stop off
> (gdb) start
> ...
> (gdb) add-inferior -exec ./a.out
> [New inferior 2]
> Added inferior 2
> ...
> (gdb) inferior 2
> [Switching to inferior 2 [<null>] (/tmp/a.out)]
> (gdb) start
> ...
> (gdb) set schedule-multiple on
> (gdb) c
> Continuing.
> [Inferior 2 (process 16331) exited normally]
> (gdb) i inferiors
>   Num  Description       Executable
>   1    process 16137     /tmp/a.out
> * 2    <null>            /tmp/a.out
> (gdb) inferior 1
> [Switching to inferior 1 [process 16137] (/tmp/a.out)]
> [Switching to thread 1.1 (process 16137)]
> Couldn't get registers: No such process.
> (gdb) i threads
>   Id   Target Id         Frame
> Couldn't get registers: No such process.
> (gdb) c
> Continuing.
> Couldn't get registers: No such process.
> ~~~
> 
> If I save the exit event in my patch as a pending event (and omit
> 'maint set target-non-stop off'), I get essentially the same problem.
> What is the expected GDB behavior here?  Would it be alright to
> actually print both exit events, followed by the gdb-prompt, where the
> user can now query $_exitcode or $_exitsignal by switching between
> inferiors, assuming those special variables are set correctly per
> inferior?

I'm really not sure about that.  

As you've seen, this happens in true all-stop too, which can't report
multiple events at the same time, so I think from that angle alone,
GDB should cope better with it.

Plus, this can happen even if an inferior stopped for some other event
while at the same time some other inferior exits.  

Say, inferior 1 hits a breakpoint, and while stopping everything,
inferior 2 exits.  And GDB happens to report the breakpoint hit
first.  And now the user does "info threads" and sees the "No such process"
errors.

You could maybe think, that then maybe we should prioritize
inferior exits over breakpoint hits.  But then, what if inferior 1
stops for a breakpoint, gdb manages to stop all threads without
inferior 2 exiting, and then a SIGKILL is sent to the
supposedly-stopped inferior, from outside GDB?  

Or to make it even simpler, that SIGKILL use case can even
happen in single-inferior debugging.  

Or, in "set non-stop on" mode, the inferior is running and you so
"info threads" just between the process dying, and GDB getting
the SIGCHLD and collecting the ptrace event.

So I think that the state gets into where the inferior dies
before the inferior exit event is reported to the user is just
something that GDB needs to cope with well.

I.e., report the failures to read registers, in "info threads",
"print" etc., and importantly -- _be sure to not get into a state
where the user is stuck_.

The "not get stuck" part is where I think we should improve things.

Your example already shows where we need improvement, in the
the last "c".  A simplified version, using an external SIGKILL is:

 $ gdb --args /usr/bin/tail -f /dev/null
 GNU gdb (GDB) 10.0.50.20200106-git
 ...
 Program received signal SIGINT, Interrupt.
 0x00007ffff7b08881 in __GI___libc_read (fd=4, buf=0x555555766410, nbytes=26) at ../sysdeps/unix/sysv/linux/read.c:26
 26        return SYSCALL_CANCEL (read, fd, buf, nbytes);
 (gdb) info inferiors 
   Num  Description       Executable        
 * 1    process 9425      /usr/bin/tail     
 (gdb) shell kill -9 9425
 (gdb) flushregs 
 Register cache flushed.
 Couldn't get registers: No such process.
 (gdb) info threads
   Id   Target Id           Frame 
 Couldn't get registers: No such process.
 (gdb) c
 Continuing.
 Couldn't get registers: No such process.
 (gdb) 

This error comes from the regcache_read_pc call from
within infrun.c:proceed:

 ...
 #4  0x000000000097e04e in perror_with_name (string=0xb213e9 "Couldn't get registers") at /home/pedro/gdb/binutils-gdb/src/gdb/utils.c:612
 #5  0x0000000000452156 in amd64_linux_nat_target::fetch_registers (this=0x11a64b0 <the_amd64_linux_nat_target>, regcache=0x1eac160, regnum=16) at /home/pedro/gdb/binutils-gdb/src/gdb/amd64-linux-nat.c:225
 #6  0x0000000000901cc4 in target_fetch_registers (regcache=0x1eac160, regno=16) at /home/pedro/gdb/binutils-gdb/src/gdb/target.c:3427
 #7  0x0000000000829f94 in regcache::raw_update (this=0x1eac160, regnum=16) at /home/pedro/gdb/binutils-gdb/src/gdb/regcache.c:471
 #8  0x000000000082a039 in readable_regcache::raw_read (this=0x1eac160, regnum=16, buf=0x7fffffffcc00 "\302%") at /home/pedro/gdb/binutils-gdb/src/gdb/regcache.c:485
 #9  0x000000000082a371 in readable_regcache::cooked_read (this=0x1eac160, regnum=16, buf=0x7fffffffcc00 "\302%") at /home/pedro/gdb/binutils-gdb/src/gdb/regcache.c:577
 #10 0x000000000082eefd in readable_regcache::cooked_read<unsigned long, void> (this=0x1eac160, regnum=16, val=0x7fffffffcca8) at /home/pedro/gdb/binutils-gdb/src/gdb/regcache.c:664
 #11 0x000000000082a7d8 in regcache_cooked_read_unsigned (regcache=0x1eac160, regnum=16, val=0x7fffffffcca8) at /home/pedro/gdb/binutils-gdb/src/gdb/regcache.c:678
 #12 0x000000000082bcf5 in regcache_read_pc (regcache=0x1eac160) at /home/pedro/gdb/binutils-gdb/src/gdb/regcache.c:1182
 #13 0x00000000006e6f62 in proceed (addr=0xffffffffffffffff, siggnal=GDB_SIGNAL_DEFAULT) at /home/pedro/gdb/binutils-gdb/src/gdb/infrun.c:2855
 #14 0x00000000006d7af6 in continue_1 (all_threads=0) at /home/pedro/gdb/binutils-gdb/src/gdb/infcmd.c:804
 ...


I think PTRACE_EVENT_EXIT on Linux could help with at least some of
the use cases on Linux, but still, GDB should cope better on systems
that do not have that feature.

Thanks,
Pedro Alves

[review v2] infrun: handle already-exited threads when attempting to stop

[Tom de Vries (Code Review)]

Tom de Vries has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/133
......................................................................


Patch Set 2:

> I'd like to ask for your opinion on making the second exit event pending

Discussion continued here: https://sourceware.org/ml/gdb-patches/2020-01/msg00212.html .


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I7cec98f40283773b79255d998511da434e9cd408
Gerrit-Change-Number: 133
Gerrit-PatchSet: 2
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-CC: Tom de Vries <tdevries@suse.de>
Gerrit-Comment-Date: Wed, 29 Jan 2020 16:19:53 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] infrun: handle already-exited threads when attempting to stop

[Tom de Vries (Code Review)]

Tom de Vries has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/133
......................................................................


Patch Set 2:

FTR, I build patchset 2 and tested the example from PR25478, and hit an assert:
...
Thread 1 "gdb" hit Breakpoint 1, stop_all_threads () at /data/gdb_versions/devel/src/gdb/infrun.c:4395
4395      int iterations = 0;
(master-gdb) c
Continuing.
infrun: stop_all_threads
infrun: stop_all_threads, pass=0, iterations=0
infrun:   Thread 0x7ffff7fa6740 (LWP 32692) not executing
infrun:   Thread 0x7ffff77fe700 (LWP 32696) executing, need stop
infrun: target_wait (-1.0.0, status) =
infrun:   32692.32696.0 [Thread 0x7ffff77fe700 (LWP 32696)],
infrun:   status->kind = signalled, signal = GDB_SIGNAL_KILL
infrun: stop_all_threads status->kind = signalled, signal = GDB_SIGNAL_KILL Thread 0x7ffff77fe700 (LWP 32696)

Program terminated with signal SIGKILL, Killed.
The program no longer exists.
infrun: stop_all_threads, pass=1, iterations=1
infrun: stop_all_threads done
thread.c:95: internal-error: thread_info* inferior_thread(): Assertion `tp' failed.
...


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I7cec98f40283773b79255d998511da434e9cd408
Gerrit-Change-Number: 133
Gerrit-PatchSet: 2
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-CC: Tom de Vries <tdevries@suse.de>
Gerrit-Comment-Date: Thu, 30 Jan 2020 09:01:05 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] infrun: handle already-exited threads when attempting to stop

[Tom de Vries (Code Review)]

Tom de Vries has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/133
......................................................................


Patch Set 2:

(1 comment)

| --- gdb/infrun.c
| +++ gdb/infrun.c
| @@ -4494,0 +4500,19 @@ stop_all_threads (void)
| +
| +		  /* Set the threads as non-executing to avoid infinitely
| +		     waiting for them to stop.  */
| +		  mark_non_executing_threads (event_ptid, ws);
| +
| +		  if (ws.kind == TARGET_WAITKIND_NO_RESUMED)
| +		    {
| +		      /* Do nothing.  Already marked the threads.  */
| +		    }
| +		  if (ws.kind == TARGET_WAITKIND_THREAD_EXITED)

PS2, Line 4509:

This doesn't look right. Did you mean to write:
 else if (ws.kind == TARGET_WAITKIND_THREAD_EXITED)
?

| +		    delete_thread (t);
| +		  else
| +		    {
| +		      /* TARGET_WAITKIND_EXITED or
| +			 TARGET_WAITKIND_SIGNALLED.  */
| +		      /* Need to restore the context because
| +			 handle_inferior_exit switches it.  */
| +		      scoped_restore_current_pspace_and_thread restore;
| +		      handle_inferior_exit (event_ptid, ws);

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I7cec98f40283773b79255d998511da434e9cd408
Gerrit-Change-Number: 133
Gerrit-PatchSet: 2
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-CC: Tom de Vries <tdevries@suse.de>
Gerrit-Comment-Date: Thu, 30 Jan 2020 09:10:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] infrun: handle already-exited threads when attempting to stop

[Tankut Baris Aktemur (Code Review)]

Tankut Baris Aktemur has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/133
......................................................................


Patch Set 2:

(1 comment)

> Patch Set 2:
> 
> FTR, I build patchset 2 and tested the example from PR25478, and hit an assert:
> ...

I was not able to repeat the problematic scenario with the current master.  The multi-target feature seems to have changed the behavior.  I'll need to first look into whether I can reproduce the infinite loop.

| --- gdb/infrun.c
| +++ gdb/infrun.c
| @@ -4494,0 +4500,19 @@ stop_all_threads (void)
| +
| +		  /* Set the threads as non-executing to avoid infinitely
| +		     waiting for them to stop.  */
| +		  mark_non_executing_threads (event_ptid, ws);
| +
| +		  if (ws.kind == TARGET_WAITKIND_NO_RESUMED)
| +		    {
| +		      /* Do nothing.  Already marked the threads.  */
| +		    }
| +		  if (ws.kind == TARGET_WAITKIND_THREAD_EXITED)

PS2, Line 4509:

Oops, yes.  Thanks for spotting that.

| +		    delete_thread (t);
| +		  else
| +		    {
| +		      /* TARGET_WAITKIND_EXITED or
| +			 TARGET_WAITKIND_SIGNALLED.  */
| +		      /* Need to restore the context because
| +			 handle_inferior_exit switches it.  */
| +		      scoped_restore_current_pspace_and_thread restore;
| +		      handle_inferior_exit (event_ptid, ws);

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I7cec98f40283773b79255d998511da434e9cd408
Gerrit-Change-Number: 133
Gerrit-PatchSet: 2
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-CC: Tom de Vries <tdevries@suse.de>
Gerrit-Comment-Date: Thu, 30 Jan 2020 16:30:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Tom de Vries <tdevries@suse.de>
Gerrit-MessageType: comment

[review v2] infrun: handle already-exited threads when attempting to stop

[Tom de Vries (Code Review)]

Tom de Vries has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/133
......................................................................


Patch Set 2:

> Patch Set 2:
> 
> (1 comment)
> 
> > Patch Set 2:
> > 
> > FTR, I build patchset 2 and tested the example from PR25478, and hit an assert:
> > ...
> 
> I was not able to repeat the problematic scenario with the current master. 

Do you mean with 'problematic scenario' PR25478 as such, or the assert?


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I7cec98f40283773b79255d998511da434e9cd408
Gerrit-Change-Number: 133
Gerrit-PatchSet: 2
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-CC: Tom de Vries <tdevries@suse.de>
Gerrit-Comment-Date: Thu, 30 Jan 2020 17:52:47 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] infrun: handle already-exited threads when attempting to stop

[Tom de Vries (Code Review)]

Tom de Vries has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/133
......................................................................


Patch Set 2:

> Patch Set 2:
> 
> FTR, I build patchset 2 and tested the example from PR25478, and hit an assert:
> ...
> Thread 1 "gdb" hit Breakpoint 1, stop_all_threads () at /data/gdb_versions/devel/src/gdb/infrun.c:4395
> 4395      int iterations = 0;
> (master-gdb) c
> Continuing.
> infrun: stop_all_threads
> infrun: stop_all_threads, pass=0, iterations=0
> infrun:   Thread 0x7ffff7fa6740 (LWP 32692) not executing
> infrun:   Thread 0x7ffff77fe700 (LWP 32696) executing, need stop
> infrun: target_wait (-1.0.0, status) =
> infrun:   32692.32696.0 [Thread 0x7ffff77fe700 (LWP 32696)],
> infrun:   status->kind = signalled, signal = GDB_SIGNAL_KILL
> infrun: stop_all_threads status->kind = signalled, signal = GDB_SIGNAL_KILL Thread 0x7ffff77fe700 (LWP 32696)
> 
> Program terminated with signal SIGKILL, Killed.
> The program no longer exists.
> infrun: stop_all_threads, pass=1, iterations=1
> infrun: stop_all_threads done
> thread.c:95: internal-error: thread_info* inferior_thread(): Assertion `tp' failed.
> ...

I've:
- ported patch set 2 to master
- integrated the fix in patch set 3 of
  https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/759 such that the assert above is
  fixed.
- build and reg-tested on x86_64-linux

Available at https://github.com/vries/gdb/tree/handle-already-exited-threads-when-attempting-to-stop-try-master-2 .

Note: this does not address all the comments on patch set 2.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I7cec98f40283773b79255d998511da434e9cd408
Gerrit-Change-Number: 133
Gerrit-PatchSet: 2
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-CC: Tom de Vries <tdevries@suse.de>
Gerrit-Comment-Date: Mon, 03 Feb 2020 15:13:02 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] infrun: handle already-exited threads when attempting to stop

[Tankut Baris Aktemur (Code Review)]

Tankut Baris Aktemur has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/133
......................................................................


Patch Set 2:

> > > 
> > > FTR, I build patchset 2 and tested the example from PR25478, and hit an assert:
> > > ...
> > 
> > I was not able to repeat the problematic scenario with the current master. 
> 
> Do you mean with 'problematic scenario' PR25478 as such, or the assert?

Sorry for the vagueness.  I was referring to the test scenario I gave in the patch
description.  Like, scenarios that can be used for testing as in
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/134
without having to stop GDB from outside at 'stop_all_threads'.

I just saw that you posted a rebased version of this patch.  I'll look at that,
thank you.

Btw, I think this patch is kind of obsolete after Pedro's comments in
https://sourceware.org/ml/gdb-patches/2020-01/msg00212.html
because the patch causes two stop event reports at once.

-Baris


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I7cec98f40283773b79255d998511da434e9cd408
Gerrit-Change-Number: 133
Gerrit-PatchSet: 2
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-CC: Tom de Vries <tdevries@suse.de>
Gerrit-Comment-Date: Mon, 03 Feb 2020 16:02:11 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] infrun: handle already-exited threads when attempting to stop

[Tom de Vries (Code Review)]

Tom de Vries has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/133
......................................................................


Patch Set 2:

> Patch Set 2:
> I just saw that you posted a rebased version of this patch.  I'll look at that,
> thank you.

That would be great, thanks.

BTW, do you happen to have something more up-to-date than patch set 2? If so, could you share this?


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I7cec98f40283773b79255d998511da434e9cd408
Gerrit-Change-Number: 133
Gerrit-PatchSet: 2
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-CC: Tom de Vries <tdevries@suse.de>
Gerrit-Comment-Date: Mon, 03 Feb 2020 16:26:57 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] infrun: handle already-exited threads when attempting to stop

[Tankut Baris Aktemur (Code Review)]

Tankut Baris Aktemur has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/133
......................................................................


Patch Set 2:

> BTW, do you happen to have something more up-to-date than patch set 2? If so, could you share this?

Yes.  I need to revise the associated tests, and will post it as soon as I can -- hopefully today.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I7cec98f40283773b79255d998511da434e9cd408
Gerrit-Change-Number: 133
Gerrit-PatchSet: 2
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-CC: Tom de Vries <tdevries@suse.de>
Gerrit-Comment-Date: Tue, 04 Feb 2020 09:06:35 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v3] infrun: handle already-exited threads when attempting to stop

[Tankut Baris Aktemur (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/133
......................................................................

infrun: handle already-exited threads when attempting to stop

In stop_all_threads, GDB sends signals to other threads in an attempt
to stop them.  While in a typical scenario the expected wait status is
TARGET_WAITKIND_STOPPED, it is possible that the thread GDB attempted
to stop has already terminated.  If so, a waitstatus other than
TARGET_WAITKIND_STOPPED would be received.  Handle this case
appropriately.

If a wait status that denotes thread termination is ignored, GDB goes
into an infinite loop in stop_all_threads.
E.g.:

~~~
$ gdb ./a.out
(gdb) start
...
(gdb) add-inferior -exec ./a.out
...
(gdb) inferior 2
...
(gdb) start
...
(gdb) set schedule-multiple on
(gdb) set debug infrun 2
(gdb) continue
Continuing.
infrun: clear_proceed_status_thread (process 23419)
infrun: clear_proceed_status_thread (process 23703)
infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
infrun: proceed: resuming process 23419
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 23419] at 0x55555555514e
infrun: infrun_async(1)
infrun: prepare_to_wait
infrun: proceed: resuming process 23703
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 23703] at 0x55555555514e
infrun: prepare_to_wait
infrun: target_wait (-1.0.0, status) =
infrun:   23703.23703.0 [process 23703],
infrun:   status->kind = exited, status = 0
infrun: handle_inferior_event status->kind = exited, status = 0
[Inferior 2 (process 23703) exited normally]
infrun: stop_waiting
infrun: stop_all_threads
infrun: stop_all_threads, pass=0, iterations=0
infrun:   process 23419 executing, need stop
infrun: target_wait (-1.0.0, status) =
infrun:   23419.23419.0 [process 23419],
infrun:   status->kind = exited, status = 0
infrun: stop_all_threads status->kind = exited, status = 0 process 23419
infrun:   process 23419 executing, already stopping
infrun: target_wait (-1.0.0, status) =
infrun:   -1.0.0 [process -1],
infrun:   status->kind = no-resumed
infrun: stop_all_threads status->kind = no-resumed process -1
infrun:   process 23419 executing, already stopping
infrun: target_wait (-1.0.0, status) =
infrun:   -1.0.0 [process -1],
infrun:   status->kind = no-resumed
infrun: stop_all_threads status->kind = no-resumed process -1
infrun:   process 23419 executing, already stopping
infrun: target_wait (-1.0.0, status) =
infrun:   -1.0.0 [process -1],
infrun:   status->kind = no-resumed
infrun: stop_all_threads status->kind = no-resumed process -1
infrun:   process 23419 executing, already stopping
infrun: target_wait (-1.0.0, status) =
infrun:   -1.0.0 [process -1],
infrun:   status->kind = no-resumed
infrun: stop_all_threads status->kind = no-resumed process -1
infrun:   process 23419 executing, already stopping
~~~

And this polling goes on forever.  This patch prevents the infinite
looping behavior.  For the same scenario above, we obtain the
following behavior:

~~~
...
(gdb) continue
Continuing.
infrun: clear_proceed_status_thread (process 8564)
infrun: clear_proceed_status_thread (process 8652)
infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
infrun: proceed: resuming process 8564
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 8564] at 0x55555555514e
infrun: infrun_async(1)
infrun: prepare_to_wait
infrun: proceed: resuming process 8652
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 8652] at 0x55555555514e
infrun: prepare_to_wait
infrun: Found 2 inferiors, starting at #0
infrun: target_wait (-1.0.0, status) =
infrun:   8564.8564.0 [process 8564],
infrun:   status->kind = exited, status = 0
infrun: handle_inferior_event status->kind = exited, status = 0
[Inferior 1 (process 8564) exited normally]
infrun: stop_waiting
infrun: stop_all_threads
infrun: stop_all_threads, pass=0, iterations=0
infrun:   process 8652 executing, need stop
infrun: target_wait (-1.0.0, status) =
infrun:   8652.8652.0 [process 8652],
infrun:   status->kind = exited, status = 0
infrun: stop_all_threads status->kind = exited, status = 0 process 8652
infrun: saving status status->kind = exited, status = 0 for 8652.8652.0
infrun:   process 8652 not executing
infrun: stop_all_threads, pass=1, iterations=1
infrun:   process 8652 not executing
infrun: stop_all_threads done
(gdb) infrun: Using pending wait status status->kind = exited, status = 0 for process 8652.
infrun: target_wait (-1.0.0, status) =
infrun:   8652.8652.0 [process 8652],
infrun:   status->kind = exited, status = 0
infrun: handle_inferior_event status->kind = exited, status = 0
[Inferior 2 (process 8652) exited normally]
infrun: stop_waiting
info inferiors
  Num  Description       Connection           Executable
  1    <null>                                 a.out
* 2    <null>                                 a.out
(gdb) info threads
No threads.
(gdb)
~~~

gdb/ChangeLog:
2020-02-05  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
	    Tom de Vries  <tdevries@suse.de>

	PR threads/25478
	* infrun.c (stop_all_threads): Do NOT ignore
	TARGET_WAITKIND_NO_RESUMED, TARGET_WAITKIND_THREAD_EXITED,
	TARGET_WAITKIND_EXITED, TARGET_WAITKIND_SIGNALLED wait statuses
	received from threads we attempt to stop.

Change-Id: I7cec98f40283773b79255d998511da434e9cd408
---
M gdb/infrun.c
1 file changed, 43 insertions(+), 5 deletions(-)



diff --git a/gdb/infrun.c b/gdb/infrun.c
index 9224376..77c64da 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4799,9 +4799,14 @@
 					"infrun:   %s not executing\n",
 					target_pid_to_str (t->ptid).c_str ());
 
-		  /* The thread may be not executing, but still be
-		     resumed with a pending status to process.  */
-		  t->resumed = false;
+		  if ((t->suspend.waitstatus.kind == TARGET_WAITKIND_SIGNALLED
+		       || t->suspend.waitstatus.kind == TARGET_WAITKIND_EXITED)
+		      && t->suspend.waitstatus_pending_p)
+		    ;
+		  else
+		    /* The thread may be not executing, but still be
+		       resumed with a pending status to process.  */
+		    t->resumed = false;
 		}
 	    }
 
@@ -4829,8 +4834,41 @@
 	      || event.ws.kind == TARGET_WAITKIND_EXITED
 	      || event.ws.kind == TARGET_WAITKIND_SIGNALLED)
 	    {
-	      /* All resumed threads exited
-		 or one thread/process exited/signalled.  */
+	      /* Eventing target is null if there were no waitable
+		 targets.  */
+	      if (event.target != nullptr)
+		{
+		  thread_info *t = find_thread_ptid (event.target,
+						     event.ptid);
+		  /* Check if this is the first time we see this
+		     thread.  Don't bother adding if it exited.  */
+		  if (t == nullptr
+		      && event.ws.kind != TARGET_WAITKIND_THREAD_EXITED)
+		    t = add_thread (event.target, event.ptid);
+
+		  if (event.ws.kind != TARGET_WAITKIND_THREAD_EXITED)
+		    {
+		      /* Set the threads as non-executing to avoid
+			 another stop attempt on them.  */
+		      mark_non_executing_threads (event.target, event.ptid,
+						  event.ws);
+
+		      if (event.ws.kind == TARGET_WAITKIND_NO_RESUMED)
+			{
+			  /* Do nothing.  Already marked the threads.  */
+			}
+		      else
+			{
+			  /* TARGET_WAITKIND_EXITED or
+			     TARGET_WAITKIND_SIGNALLED.  */
+			  save_waitstatus (t, &event.ws);
+			  /* This was cleared in mark_non_executing_threads.
+			     Set it so this pending event is considered by
+			     do_target_wait.  */
+			  t->resumed = true;
+			}
+		    }
+		}
 	    }
 	  else
 	    {

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I7cec98f40283773b79255d998511da434e9cd408
Gerrit-Change-Number: 133
Gerrit-PatchSet: 3
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-CC: Tom de Vries <tdevries@suse.de>
Gerrit-MessageType: newpatchset

[review v3] infrun: handle already-exited threads when attempting to stop

[Tankut Baris Aktemur (Code Review)]

Tankut Baris Aktemur has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/133
......................................................................


Patch Set 3:

> BTW, do you happen to have something more up-to-date than patch set 2? If so, could you share this?

I've uploaded patchset 3.  This version uses pending waitstatuses and also includes fixes from
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/759

I've not fully updated the testcases in
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/143.
There are still unaddressed comments there.

This patchset assumes that an existing problem that is explained in
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/766
is fixed.  Otherwise the scenario described in the commit message is not reproduced.

Thanks
-Baris


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I7cec98f40283773b79255d998511da434e9cd408
Gerrit-Change-Number: 133
Gerrit-PatchSet: 3
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-CC: Tom de Vries <tdevries@suse.de>
Gerrit-Comment-Date: Wed, 05 Feb 2020 13:24:15 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v3] infrun: handle already-exited threads when attempting to stop

[Tom de Vries (Code Review)]

Tom de Vries has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/133
......................................................................


Patch Set 3:

> Patch Set 3:
> 
> > BTW, do you happen to have something more up-to-date than patch set 2? If so, could you share this?
> 
> I've uploaded patchset 3.  This version uses pending waitstatuses and also includes fixes from
> https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/759
> 

FTR, this also fixes the gdb/testsuite/gdb.threads/kill-in-stop-all-threads.exp test-case in patch set 4 of https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/759 .


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I7cec98f40283773b79255d998511da434e9cd408
Gerrit-Change-Number: 133
Gerrit-PatchSet: 3
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-CC: Tom de Vries <tdevries@suse.de>
Gerrit-Comment-Date: Wed, 05 Feb 2020 22:59:31 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[PATCH v4 2/2] gdb/infrun: handle already-exited threads when attempting to stop

[Tankut Baris Aktemur]

In stop_all_threads, GDB sends signals to other threads in an attempt
to stop them.  While in a typical scenario the expected wait status is
TARGET_WAITKIND_STOPPED, it is possible that the thread GDB attempted
to stop has already terminated.  If so, a waitstatus other than
TARGET_WAITKIND_STOPPED would be received.  Handle this case
appropriately.

If a wait status that denotes thread termination is ignored, GDB goes
into an infinite loop in stop_all_threads.
E.g.:

~~~
$ gdb ./a.out
(gdb) start
...
(gdb) add-inferior -exec ./a.out
...
(gdb) inferior 2
...
(gdb) start
...
(gdb) set schedule-multiple on
(gdb) set debug infrun 2
(gdb) continue
Continuing.
infrun: clear_proceed_status_thread (process 23419)
infrun: clear_proceed_status_thread (process 23703)
infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
infrun: proceed: resuming process 23419
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 23419] at 0x55555555514e
infrun: infrun_async(1)
infrun: prepare_to_wait
infrun: proceed: resuming process 23703
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 23703] at 0x55555555514e
infrun: prepare_to_wait
infrun: target_wait (-1.0.0, status) =
infrun:   23703.23703.0 [process 23703],
infrun:   status->kind = exited, status = 0
infrun: handle_inferior_event status->kind = exited, status = 0
[Inferior 2 (process 23703) exited normally]
infrun: stop_waiting
infrun: stop_all_threads
infrun: stop_all_threads, pass=0, iterations=0
infrun:   process 23419 executing, need stop
infrun: target_wait (-1.0.0, status) =
infrun:   23419.23419.0 [process 23419],
infrun:   status->kind = exited, status = 0
infrun: stop_all_threads status->kind = exited, status = 0 process 23419
infrun:   process 23419 executing, already stopping
infrun: target_wait (-1.0.0, status) =
infrun:   -1.0.0 [process -1],
infrun:   status->kind = no-resumed
infrun: stop_all_threads status->kind = no-resumed process -1
infrun:   process 23419 executing, already stopping
infrun: target_wait (-1.0.0, status) =
infrun:   -1.0.0 [process -1],
infrun:   status->kind = no-resumed
infrun: stop_all_threads status->kind = no-resumed process -1
infrun:   process 23419 executing, already stopping
infrun: target_wait (-1.0.0, status) =
infrun:   -1.0.0 [process -1],
infrun:   status->kind = no-resumed
infrun: stop_all_threads status->kind = no-resumed process -1
infrun:   process 23419 executing, already stopping
infrun: target_wait (-1.0.0, status) =
infrun:   -1.0.0 [process -1],
infrun:   status->kind = no-resumed
infrun: stop_all_threads status->kind = no-resumed process -1
infrun:   process 23419 executing, already stopping
~~~

And this polling goes on forever.  This patch prevents the infinite
looping behavior.  For the same scenario above, we obtain the
following behavior:

~~~
...
(gdb) continue
Continuing.
infrun: clear_proceed_status_thread (process 8564)
infrun: clear_proceed_status_thread (process 8652)
infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
infrun: proceed: resuming process 8564
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 8564] at 0x55555555514e
infrun: infrun_async(1)
infrun: prepare_to_wait
infrun: proceed: resuming process 8652
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 8652] at 0x55555555514e
infrun: prepare_to_wait
infrun: Found 2 inferiors, starting at #0
infrun: target_wait (-1.0.0, status) =
infrun:   8564.8564.0 [process 8564],
infrun:   status->kind = exited, status = 0
infrun: handle_inferior_event status->kind = exited, status = 0
[Inferior 1 (process 8564) exited normally]
infrun: stop_waiting
infrun: stop_all_threads
infrun: stop_all_threads, pass=0, iterations=0
infrun:   process 8652 executing, need stop
infrun: target_wait (-1.0.0, status) =
infrun:   8652.8652.0 [process 8652],
infrun:   status->kind = exited, status = 0
infrun: stop_all_threads status->kind = exited, status = 0 process 8652
infrun: saving status status->kind = exited, status = 0 for 8652.8652.0
infrun:   process 8652 not executing
infrun: stop_all_threads, pass=1, iterations=1
infrun:   process 8652 not executing
infrun: stop_all_threads done
(gdb) infrun: Using pending wait status status->kind = exited, status = 0 for process 8652.
infrun: target_wait (-1.0.0, status) =
infrun:   8652.8652.0 [process 8652],
infrun:   status->kind = exited, status = 0
infrun: handle_inferior_event status->kind = exited, status = 0
[Inferior 2 (process 8652) exited normally]
infrun: stop_waiting
info inferiors
  Num  Description       Connection           Executable
  1    <null>                                 a.out
* 2    <null>                                 a.out
(gdb) info threads
No threads.
(gdb)
~~~

gdb/ChangeLog:
2020-02-05  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
	    Tom de Vries  <tdevries@suse.de>

	PR threads/25478
	* infrun.c (stop_all_threads): Do NOT ignore
	TARGET_WAITKIND_NO_RESUMED, TARGET_WAITKIND_THREAD_EXITED,
	TARGET_WAITKIND_EXITED, TARGET_WAITKIND_SIGNALLED wait statuses
	received from threads we attempt to stop.

gdb/testsuite/ChangeLog:
2019-11-04  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* gdb.multi/multi-exit.c: New file.
	* gdb.multi/multi-exit.exp: New file.
	* gdb.multi/multi-kill.c: New file.
	* gdb.multi/multi-kill.exp: New file.

Change-Id: I7cec98f40283773b79255d998511da434e9cd408
---
 gdb/infrun.c                           | 38 ++++++++++--
 gdb/testsuite/gdb.multi/multi-exit.c   | 22 +++++++
 gdb/testsuite/gdb.multi/multi-exit.exp | 81 +++++++++++++++++++++++++
 gdb/testsuite/gdb.multi/multi-kill.c   | 34 +++++++++++
 gdb/testsuite/gdb.multi/multi-kill.exp | 84 ++++++++++++++++++++++++++
 5 files changed, 254 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.multi/multi-exit.c
 create mode 100644 gdb/testsuite/gdb.multi/multi-exit.exp
 create mode 100644 gdb/testsuite/gdb.multi/multi-kill.c
 create mode 100644 gdb/testsuite/gdb.multi/multi-kill.exp

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 9224376d05e..58a6da9ec32 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4799,9 +4799,14 @@ stop_all_threads (void)
 					"infrun:   %s not executing\n",
 					target_pid_to_str (t->ptid).c_str ());
 
-		  /* The thread may be not executing, but still be
-		     resumed with a pending status to process.  */
-		  t->resumed = false;
+		  if ((t->suspend.waitstatus.kind == TARGET_WAITKIND_SIGNALLED
+		       || t->suspend.waitstatus.kind == TARGET_WAITKIND_EXITED)
+		      && t->suspend.waitstatus_pending_p)
+		    ;
+		  else
+		    /* The thread may be not executing, but still be
+		       resumed with a pending status to process.  */
+		    t->resumed = false;
 		}
 	    }
 
@@ -4829,8 +4834,31 @@ stop_all_threads (void)
 	      || event.ws.kind == TARGET_WAITKIND_EXITED
 	      || event.ws.kind == TARGET_WAITKIND_SIGNALLED)
 	    {
-	      /* All resumed threads exited
-		 or one thread/process exited/signalled.  */
+	      /* Eventing target is null if there were no waitable
+		 targets.  */
+	      if (event.target != nullptr)
+		{
+		  thread_info *t = find_thread_ptid (event.target,
+						     event.ptid);
+		  /* Check if this is the first time we see this thread.
+		     Don't bother adding if it individually exited.  */
+		  if (t == nullptr
+		      && event.ws.kind != TARGET_WAITKIND_THREAD_EXITED)
+		    t = add_thread (event.target, event.ptid);
+
+		  if (t != nullptr)
+		    {
+		      /* Set the threads as non-executing to avoid
+			 another stop attempt on them.  */
+		      mark_non_executing_threads (event.target, event.ptid,
+						  event.ws);
+		      save_waitstatus (t, &event.ws);
+		      /* This was cleared in mark_non_executing_threads.
+			 Set it so this pending event is considered by
+			 do_target_wait.  */
+		      t->resumed = true;
+		    }
+		}
 	    }
 	  else
 	    {
diff --git a/gdb/testsuite/gdb.multi/multi-exit.c b/gdb/testsuite/gdb.multi/multi-exit.c
new file mode 100644
index 00000000000..f4825c8a7c1
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/multi-exit.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2020 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int
+main ()
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.multi/multi-exit.exp b/gdb/testsuite/gdb.multi/multi-exit.exp
new file mode 100644
index 00000000000..959f0e8740a
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/multi-exit.exp
@@ -0,0 +1,81 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2020 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test receiving TARGET_WAITKIND_EXITED events from multiple
+# inferiors.  In all stop-mode, upon receiving the exit event from one
+# of the inferiors, GDB will try to stop the other inferior, too.  So,
+# a stop request will be sent.  Receiving a TARGET_WAITKIND_EXITED
+# status kind as a response to that stop request instead of a
+# TARGET_WAITKIND_STOPPED should be handled by GDB without problems.
+
+standard_testfile
+
+# This is a test specific for native GDB's ability to stop all
+# threads.
+if {![can_spawn_for_attach]} {
+    return 0
+}
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} {
+    return -1
+}
+
+gdb_test "add-inferior -exec $binfile" "Added inferior 2.*" \
+    "add the second inferior"
+
+proc test {} {
+    # Start the processes separately.
+    gdb_test_no_output "set schedule-multiple off"
+
+    gdb_test "inferior 1" ".*Switching to inferior 1.*"
+    if {![runto_main]} {
+	fail "starting inferior 1"
+	return -1
+    }
+
+    gdb_test "inferior 2" ".*Switching to inferior 2.*"
+    if {![runto_main]} {
+	fail "starting inferior 2"
+	return -1
+    }
+
+    # Now continue both processes.
+    gdb_test_no_output "set schedule-multiple on"
+
+    # We want GDB to complete the command and return the prompt
+    # instead of going into an infinite loop.
+    # Both inferiors exit.
+    global inferior_exited_re gdb_prompt
+    set count 0
+    gdb_test_multiple "continue" "continue" {
+	-re "$inferior_exited_re normally\]\[\r\n\]+" {
+	    incr count
+	    if {$count == 2} {
+		pass $gdb_test_name
+	    } else {
+		exp_continue
+	    }
+	}
+	-re "$gdb_prompt " {
+	    exp_continue
+	}
+    }
+}
+
+with_test_prefix "iteration 1" test
+# Repeat to also test re-runnability.
+with_test_prefix "iteration 2" test
diff --git a/gdb/testsuite/gdb.multi/multi-kill.c b/gdb/testsuite/gdb.multi/multi-kill.c
new file mode 100644
index 00000000000..3622c499202
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/multi-kill.c
@@ -0,0 +1,34 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2020 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* This program is intended to be started outside of gdb, and then
+   attached to by gdb.  */
+
+#include <unistd.h>
+
+int
+main ()
+{
+  /* Don't run forever in case GDB crashes and DejaGNU fails to kill
+     this program.  */
+  alarm (10);
+
+  while (1)
+    ;
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.multi/multi-kill.exp b/gdb/testsuite/gdb.multi/multi-kill.exp
new file mode 100644
index 00000000000..6d509d9f0ba
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/multi-kill.exp
@@ -0,0 +1,84 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2020 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test receiving TARGET_WAITKIND_SIGNALLED events from multiple
+# inferiors.  In all stop-mode, upon receiving the exit event from one
+# of the inferiors, GDB will try to stop the other inferior, too.  So,
+# a stop request will be sent.  Receiving a TARGET_WAITKIND_SIGNALLED
+# status kind as a response to that stop request instead of a
+# TARGET_WAITKIND_STOPPED should be handled by GDB without problems.
+
+standard_testfile
+
+# This is a test specific for native GDB's ability to stop all
+# threads.
+if {![can_spawn_for_attach]} {
+    return 0
+}
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} {
+    return -1
+}
+
+gdb_test "add-inferior -exec $binfile" "Added inferior 2.*" \
+    "add the second inferior"
+
+# We want both processes in a running state.
+gdb_test_no_output "set schedule-multiple on"
+
+proc test {} {
+    # Start the programs, attach to them, then kill both from outside.
+    global binfile
+    global gdb_prompt
+
+    set spawn_id_list [spawn_wait_for_attach [list $binfile $binfile]]
+    set test_spawn_id1 [lindex $spawn_id_list 0]
+    set test_spawn_id2 [lindex $spawn_id_list 1]
+    set testpid1 [spawn_id_get_pid $test_spawn_id1]
+    set testpid2 [spawn_id_get_pid $test_spawn_id2]
+
+    gdb_test "inferior 1" ".*Switching to inferior 1.*"
+
+    gdb_test "attach $testpid1" \
+	"Attaching to program: .*, process $testpid1.*(in|at).*" \
+	"attach to program 1"
+
+    gdb_test "inferior 2" ".*Switching to inferior 2.*"
+
+    gdb_test "attach $testpid2" \
+	"Attaching to program: .*, process $testpid2.*(in|at).*" \
+	"attach to program 2"
+
+    gdb_test_multiple "continue" "continue processes" {
+	-re "Continuing.\[\r\n\]+" {
+	    # Kill both processes at once.
+	    remote_exec build "kill -9 ${testpid1} ${testpid2}"
+	    exp_continue
+	}
+	-re "Program terminated with signal.*$gdb_prompt" {
+	    pass $gdb_test_name
+	}
+    }
+
+    # Make sure that the processes are gone.
+    kill_wait_spawned_process $test_spawn_id1
+    kill_wait_spawned_process $test_spawn_id2
+}
+
+with_test_prefix "iteration 1" test
+# Repeat to also test re-runnability.
+with_test_prefix "iteration 2" test
-- 
2.17.1

[PATCH v4 0/2] Handling already-exited threads in 'stop_all_threads'

[Tankut Baris Aktemur]

Hi,

GDB goes into an infinite loop when it attempts to stop a thread in
'stop_all_threads', if the thread has already died.  This patch aims
to fix this by handling waitstatuses that denote exiting.

This is a port of

  https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/133

from Gerrit to email, because Gerrit is planned to be retired.
This version of the patch is a revision of patchset 3 in Gerrit,
hence I enumerated it as v4.  The new revision simplifies the
waitstatus handling in PATCH 2/2 and also merges the tests into
that same patch (because it is more desirable that the tests
accompany the fix rather than coming as a separate commit).

PATCH 1/2 was already approved on Gerrit.

For a previous discussion, see

  https://sourceware.org/ml/gdb-patches/2020-01/msg00212.html

And also see

  https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/759

The problematic scenario explained and tested in PATCH 2/2 is not
reproducible if the bug mentioned in

  https://sourceware.org/ml/gdb-patches/2020-02/msg00122.html

is not addressed.  Therefore the patch proposed here has to be applied
on top of a solution for that.  Such a branch is available at

  https://github.com/barisaktemur/gdb/tree/thread-exit-in-stop-all-threads-v4

Thanks.
Baris

Tankut Baris Aktemur (2):
  gdb/infrun: extract out a code piece into 'mark_non_executing_threads'
    function
  gdb/infrun: handle already-exited threads when attempting to stop

 gdb/infrun.c                           | 115 ++++++++++++++++---------
 gdb/testsuite/gdb.multi/multi-exit.c   |  22 +++++
 gdb/testsuite/gdb.multi/multi-exit.exp |  81 +++++++++++++++++
 gdb/testsuite/gdb.multi/multi-kill.c   |  34 ++++++++
 gdb/testsuite/gdb.multi/multi-kill.exp |  84 ++++++++++++++++++
 5 files changed, 296 insertions(+), 40 deletions(-)
 create mode 100644 gdb/testsuite/gdb.multi/multi-exit.c
 create mode 100644 gdb/testsuite/gdb.multi/multi-exit.exp
 create mode 100644 gdb/testsuite/gdb.multi/multi-kill.c
 create mode 100644 gdb/testsuite/gdb.multi/multi-kill.exp

-- 
2.17.1

[PATCH v4 1/2] gdb/infrun: extract out a code piece into 'mark_non_executing_threads' function

[Tankut Baris Aktemur]

This is a refactoring.  The extracted function is placed deliberately
before 'stop_all_threads' because the function will be re-used there
in a subsequent patch for handling an exit status kind received from
a thread that GDB attempted to stop.

gdb/ChangeLog:
2019-11-04  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* infrun.c (handle_inferior_event): Extract out a piece of code
	into...
	(mark_non_executing_threads): ...this new function.

Change-Id: I2b088f4a724f4260cb37068264964525cf62a118
---
 gdb/infrun.c | 77 ++++++++++++++++++++++++++++------------------------
 1 file changed, 42 insertions(+), 35 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index fd72a744339..9224376d05e 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4673,6 +4673,47 @@ save_waitstatus (struct thread_info *tp, const target_waitstatus *ws)
     }
 }
 
+/* Mark the non-executing threads accordingly.  In all-stop, all
+   threads of all processes are stopped when we get any event
+   reported.  In non-stop mode, only the event thread stops.  */
+
+static void
+mark_non_executing_threads (process_stratum_target *target,
+			    ptid_t event_ptid,
+			    struct target_waitstatus ws)
+{
+  ptid_t mark_ptid;
+
+  if (!target_is_non_stop_p ())
+    mark_ptid = minus_one_ptid;
+  else if (ws.kind == TARGET_WAITKIND_SIGNALLED
+	   || ws.kind == TARGET_WAITKIND_EXITED)
+    {
+      /* If we're handling a process exit in non-stop mode, even
+	 though threads haven't been deleted yet, one would think
+	 that there is nothing to do, as threads of the dead process
+	 will be soon deleted, and threads of any other process were
+	 left running.  However, on some targets, threads survive a
+	 process exit event.  E.g., for the "checkpoint" command,
+	 when the current checkpoint/fork exits, linux-fork.c
+	 automatically switches to another fork from within
+	 target_mourn_inferior, by associating the same
+	 inferior/thread to another fork.  We haven't mourned yet at
+	 this point, but we must mark any threads left in the
+	 process as not-executing so that finish_thread_state marks
+	 them stopped (in the user's perspective) if/when we present
+	 the stop to the user.  */
+      mark_ptid = ptid_t (event_ptid.pid ());
+    }
+  else
+    mark_ptid = event_ptid;
+
+  set_executing (target, mark_ptid, false);
+
+  /* Likewise the resumed flag.  */
+  set_resumed (target, mark_ptid, false);
+}
+
 /* See infrun.h.  */
 
 void
@@ -5107,41 +5148,7 @@ handle_inferior_event (struct execution_control_state *ecs)
 	}
     }
 
-  /* Mark the non-executing threads accordingly.  In all-stop, all
-     threads of all processes are stopped when we get any event
-     reported.  In non-stop mode, only the event thread stops.  */
-  {
-    ptid_t mark_ptid;
-
-    if (!target_is_non_stop_p ())
-      mark_ptid = minus_one_ptid;
-    else if (ecs->ws.kind == TARGET_WAITKIND_SIGNALLED
-	     || ecs->ws.kind == TARGET_WAITKIND_EXITED)
-      {
-	/* If we're handling a process exit in non-stop mode, even
-	   though threads haven't been deleted yet, one would think
-	   that there is nothing to do, as threads of the dead process
-	   will be soon deleted, and threads of any other process were
-	   left running.  However, on some targets, threads survive a
-	   process exit event.  E.g., for the "checkpoint" command,
-	   when the current checkpoint/fork exits, linux-fork.c
-	   automatically switches to another fork from within
-	   target_mourn_inferior, by associating the same
-	   inferior/thread to another fork.  We haven't mourned yet at
-	   this point, but we must mark any threads left in the
-	   process as not-executing so that finish_thread_state marks
-	   them stopped (in the user's perspective) if/when we present
-	   the stop to the user.  */
-	mark_ptid = ptid_t (ecs->ptid.pid ());
-      }
-    else
-      mark_ptid = ecs->ptid;
-
-    set_executing (ecs->target, mark_ptid, false);
-
-    /* Likewise the resumed flag.  */
-    set_resumed (ecs->target, mark_ptid, false);
-  }
+  mark_non_executing_threads (ecs->target, ecs->ptid, ecs->ws);
 
   switch (ecs->ws.kind)
     {
-- 
2.17.1

[review v3] infrun: handle already-exited threads when attempting to stop

[Tankut Baris Aktemur (Code Review)]

Tankut Baris Aktemur has abandoned this change. ( https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/133 )

Change subject: infrun: handle already-exited threads when attempting to stop
......................................................................


Abandoned

Migrated to https://sourceware.org/ml/gdb-patches/2020-02/msg00153.html
-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I7cec98f40283773b79255d998511da434e9cd408
Gerrit-Change-Number: 133
Gerrit-PatchSet: 3
Gerrit-Owner: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Gerrit-CC: Tom de Vries <tdevries@suse.de>
Gerrit-MessageType: abandon

[PING][PATCH v4 0/2] Handling already-exited threads in 'stop_all_threads'

[Tankut Baris Aktemur]

> Hi,
>
> GDB goes into an infinite loop when it attempts to stop a thread in
> 'stop_all_threads', if the thread has already died.  This patch aims
> to fix this by handling waitstatuses that denote exiting.
>
> This is a port of
>
>   https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/133
>
> from Gerrit to email, because Gerrit is planned to be retired.
> This version of the patch is a revision of patchset 3 in Gerrit,
> hence I enumerated it as v4.  The new revision simplifies the
> waitstatus handling in PATCH 2/2 and also merges the tests into
> that same patch (because it is more desirable that the tests
> accompany the fix rather than coming as a separate commit).
>
> PATCH 1/2 was already approved on Gerrit.
>
> For a previous discussion, see
>
>   https://sourceware.org/ml/gdb-patches/2020-01/msg00212.html
>
> And also see
>
>   https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/759

Kindly pinging for the patch available at

  https://sourceware.org/legacy-ml/gdb-patches/2020-02/msg00153.html

> The problematic scenario explained and tested in PATCH 2/2 is not
> reproducible if the bug mentioned in
>
>   https://sourceware.org/ml/gdb-patches/2020-02/msg00122.html
>
> is not addressed.

The fix for this problem was merged yesterday as

53cccef118b gdb/infrun: stop all threads if there exists a non-stop target
a0714d305fb gdb: define convenience function 'exists_non_stop_target'

Thanks.
Baris

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v4 2/2] gdb/infrun: handle already-exited threads when attempting to stop

[Pedro Alves]

Hi,

On 2/7/20 12:04 PM, Tankut Baris Aktemur wrote:

> ...
> (gdb) continue
> Continuing.
> infrun: clear_proceed_status_thread (process 8564)
> infrun: clear_proceed_status_thread (process 8652)
> infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
> infrun: proceed: resuming process 8564
> infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 8564] at 0x55555555514e
> infrun: infrun_async(1)
> infrun: prepare_to_wait
> infrun: proceed: resuming process 8652
> infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 8652] at 0x55555555514e
> infrun: prepare_to_wait
> infrun: Found 2 inferiors, starting at #0
> infrun: target_wait (-1.0.0, status) =
> infrun:   8564.8564.0 [process 8564],
> infrun:   status->kind = exited, status = 0
> infrun: handle_inferior_event status->kind = exited, status = 0
> [Inferior 1 (process 8564) exited normally]
> infrun: stop_waiting
> infrun: stop_all_threads
> infrun: stop_all_threads, pass=0, iterations=0
> infrun:   process 8652 executing, need stop
> infrun: target_wait (-1.0.0, status) =
> infrun:   8652.8652.0 [process 8652],
> infrun:   status->kind = exited, status = 0
> infrun: stop_all_threads status->kind = exited, status = 0 process 8652
> infrun: saving status status->kind = exited, status = 0 for 8652.8652.0
> infrun:   process 8652 not executing
> infrun: stop_all_threads, pass=1, iterations=1
> infrun:   process 8652 not executing
> infrun: stop_all_threads done

Looks good so far, but ...

> (gdb) infrun: Using pending wait status status->kind = exited, status = 0 for process 8652.
> infrun: target_wait (-1.0.0, status) =
> infrun:   8652.8652.0 [process 8652],
> infrun:   status->kind = exited, status = 0
> infrun: handle_inferior_event status->kind = exited, status = 0
> [Inferior 2 (process 8652) exited normally]

... how come GDB collected the pending event without an intervening
"continue" ?

As previously discussed, the event for the other inferior should be
left pending until that inferior/thread is re-resumed.  That is now
done in your patch.  But, if gdb is consuming the pending event right
after showing the prompt for the first inferior exit, then the second
inferior/thread wasn't correctly marked as NOT resumed.

Once we do that, I think we'll still hit the issues discussed
earlier at <https://sourceware.org/legacy-ml/gdb-patches/2020-01/msg00212.html>
like that:

 (gdb) c
 Continuing.
 Couldn't get registers: No such process.
 (gdb) 

But maybe changes for that have gone in meanwhile and I haven't noticed?


> infrun: stop_waiting
> info inferiors
>   Num  Description       Connection           Executable
>   1    <null>                                 a.out
> * 2    <null>                                 a.out
> (gdb) info threads
> No threads.
> (gdb)
> ~~~
> 
> gdb/ChangeLog:
> 2020-02-05  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
> 	    Tom de Vries  <tdevries@suse.de>
> 
> 	PR threads/25478
> 	* infrun.c (stop_all_threads): Do NOT ignore
> 	TARGET_WAITKIND_NO_RESUMED, TARGET_WAITKIND_THREAD_EXITED,
> 	TARGET_WAITKIND_EXITED, TARGET_WAITKIND_SIGNALLED wait statuses
> 	received from threads we attempt to stop.
> 
> gdb/testsuite/ChangeLog:
> 2019-11-04  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
> 
> 	* gdb.multi/multi-exit.c: New file.
> 	* gdb.multi/multi-exit.exp: New file.
> 	* gdb.multi/multi-kill.c: New file.
> 	* gdb.multi/multi-kill.exp: New file.
> 
> Change-Id: I7cec98f40283773b79255d998511da434e9cd408
> ---
>  gdb/infrun.c                           | 38 ++++++++++--
>  gdb/testsuite/gdb.multi/multi-exit.c   | 22 +++++++
>  gdb/testsuite/gdb.multi/multi-exit.exp | 81 +++++++++++++++++++++++++
>  gdb/testsuite/gdb.multi/multi-kill.c   | 34 +++++++++++
>  gdb/testsuite/gdb.multi/multi-kill.exp | 84 ++++++++++++++++++++++++++
>  5 files changed, 254 insertions(+), 5 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.multi/multi-exit.c
>  create mode 100644 gdb/testsuite/gdb.multi/multi-exit.exp
>  create mode 100644 gdb/testsuite/gdb.multi/multi-kill.c
>  create mode 100644 gdb/testsuite/gdb.multi/multi-kill.exp
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 9224376d05e..58a6da9ec32 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -4799,9 +4799,14 @@ stop_all_threads (void)
>  					"infrun:   %s not executing\n",
>  					target_pid_to_str (t->ptid).c_str ());
>  
> -		  /* The thread may be not executing, but still be
> -		     resumed with a pending status to process.  */
> -		  t->resumed = false;
> +		  if ((t->suspend.waitstatus.kind == TARGET_WAITKIND_SIGNALLED
> +		       || t->suspend.waitstatus.kind == TARGET_WAITKIND_EXITED)
> +		      && t->suspend.waitstatus_pending_p)
> +		    ;
> +		  else
> +		    /* The thread may be not executing, but still be
> +		       resumed with a pending status to process.  */
> +		    t->resumed = false;
>  		}
>  	    }
>  
> @@ -4829,8 +4834,31 @@ stop_all_threads (void)
>  	      || event.ws.kind == TARGET_WAITKIND_EXITED
>  	      || event.ws.kind == TARGET_WAITKIND_SIGNALLED)
>  	    {
> -	      /* All resumed threads exited
> -		 or one thread/process exited/signalled.  */
> +	      /* Eventing target is null if there were no waitable
> +		 targets.  */
> +	      if (event.target != nullptr)

Instead of this nullptr check, can't we write the conditions as:


	  if (event.ws.kind == TARGET_WAITKIND_NO_RESUMED)
            {
	      /* All resumed threads exited.  */
            }
          else if (event.ws.kind == TARGET_WAITKIND_THREAD_EXITED
                   || event.ws.kind == TARGET_WAITKIND_EXITED
                   || event.ws.kind == TARGET_WAITKIND_SIGNALLED)
	    {
	      /* One thread/process exited/signalled.  */
              ...
	    }

?

> +		{
> +		  thread_info *t = find_thread_ptid (event.target,
> +						     event.ptid);
> +		  /* Check if this is the first time we see this thread.
> +		     Don't bother adding if it individually exited.  */
> +		  if (t == nullptr
> +		      && event.ws.kind != TARGET_WAITKIND_THREAD_EXITED)
> +		    t = add_thread (event.target, event.ptid);
> +

This doesn't look right for all targets.  On native GNU/Linux,
the ptid returned by TARGET_WAITKIND_EXITED happens to be the main
thread's ptid usually, 

 infrun: target_wait (-1.0.0, status) =
 infrun:   7941.7941.0 [process 7941],
           ^^^^^^^^^
 infrun:   status->kind = signalled, signal = GDB_SIGNAL_ALRM

but on e.g., remote targets, it will be just the pid of the process with
tid/lwpid fields == 0, 

 infrun: target_wait (-1.0.0, status) =
 infrun:   8262.0.0 [process 8262],
 infrun:   status->kind = signalled, signal = GDB_SIGNAL_ALRM

(Looks like darwin-nat.c is the same.)

so this will probably add a thread with such a ptid, which seems incorrect.
I think we should still be able to find a thread in the list for the exiting
inferior here, so if we don't find a thread by ptid, I guess we could use the
first non-exited thread in the inferior to save the status.  Otherwise
we'd have to save the status in the inferior or in a separate event list.

Actually, looking at windows-nat.c's, EXIT_PROCESS_DEBUG_EVENT handling,
that deletes the event thread before returning the TARGET_WAITKIND_EXITED
event...  :-/  I'm not sure we can delete that windows_delete_thread
call.  Maybe we can.

BTW, the new test fails with --target_board=native-extended-gdbserver:

 Running /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.multi/multi-kill.exp ...
 FAIL: gdb.multi/multi-kill.exp: iteration 2: attach to program 1 (got interactive prompt)
 Running /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.multi/multi-exit.exp ...
 FAIL: gdb.multi/multi-exit.exp: iteration 1: continue (timeout)

Thanks,
Pedro Alves

RE: [PATCH v4 2/2] gdb/infrun: handle already-exited threads when attempting to stop

[Aktemur, Tankut Baris]

On Thursday, April 2, 2020 8:00 PM, Pedro Alves wrote:
> Hi,
> 
> On 2/7/20 12:04 PM, Tankut Baris Aktemur wrote:
> 
> > ...
> > (gdb) continue
> > Continuing.
> > infrun: clear_proceed_status_thread (process 8564)
> > infrun: clear_proceed_status_thread (process 8652)
> > infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
> > infrun: proceed: resuming process 8564
> > infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 8564] at 0x55555555514e
> > infrun: infrun_async(1)
> > infrun: prepare_to_wait
> > infrun: proceed: resuming process 8652
> > infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 8652] at 0x55555555514e
> > infrun: prepare_to_wait
> > infrun: Found 2 inferiors, starting at #0
> > infrun: target_wait (-1.0.0, status) =
> > infrun:   8564.8564.0 [process 8564],
> > infrun:   status->kind = exited, status = 0
> > infrun: handle_inferior_event status->kind = exited, status = 0
> > [Inferior 1 (process 8564) exited normally]
> > infrun: stop_waiting
> > infrun: stop_all_threads
> > infrun: stop_all_threads, pass=0, iterations=0
> > infrun:   process 8652 executing, need stop
> > infrun: target_wait (-1.0.0, status) =
> > infrun:   8652.8652.0 [process 8652],
> > infrun:   status->kind = exited, status = 0
> > infrun: stop_all_threads status->kind = exited, status = 0 process 8652
> > infrun: saving status status->kind = exited, status = 0 for 8652.8652.0
> > infrun:   process 8652 not executing
> > infrun: stop_all_threads, pass=1, iterations=1
> > infrun:   process 8652 not executing
> > infrun: stop_all_threads done
> 
> Looks good so far, but ...
> 
> > (gdb) infrun: Using pending wait status status->kind = exited, status = 0 for process 8652.
> > infrun: target_wait (-1.0.0, status) =
> > infrun:   8652.8652.0 [process 8652],
> > infrun:   status->kind = exited, status = 0
> > infrun: handle_inferior_event status->kind = exited, status = 0
> > [Inferior 2 (process 8652) exited normally]
> 
> ... how come GDB collected the pending event without an intervening
> "continue" ?
> 
> As previously discussed, the event for the other inferior should be
> left pending until that inferior/thread is re-resumed.  That is now
> done in your patch.  But, if gdb is consuming the pending event right
> after showing the prompt for the first inferior exit, then the second
> inferior/thread wasn't correctly marked as NOT resumed.

The next revision, v5, fixes this.  There, we get the GDB prompt and it stops there.

> Once we do that, I think we'll still hit the issues discussed
> earlier at <https://sourceware.org/legacy-ml/gdb-patches/2020-01/msg00212.html>
> like that:
> 
>  (gdb) c
>  Continuing.
>  Couldn't get registers: No such process.
>  (gdb)
> 
> But maybe changes for that have gone in meanwhile and I haven't noticed?

Unfortunately it exists.  In fact, GDB goes into an infinite query.  I applied
Tom de Vries' patch submitted at

  https://sourceware.org/pipermail/gdb-patches/2020-March/166982.html

It would be great if you could check that patch.

There are also three more places where the PC of the thread is queried when 
the second "continue" command is issued to resume the second inferior.  I added
predecessor patches into the series to address these.

Please see Patch 05's commit message for the output of a sample session.  The
"Couldn't get registers" error message is still seen, but only once.  If the user
issues commands that trigger the register query (e.g.  "info threads"), the error
message will be seen again.  I considered that as a separate problem and did not
attempt to fix it as part of this series.

> > @@ -4829,8 +4834,31 @@ stop_all_threads (void)
> >  	      || event.ws.kind == TARGET_WAITKIND_EXITED
> >  	      || event.ws.kind == TARGET_WAITKIND_SIGNALLED)
> >  	    {
> > -	      /* All resumed threads exited
> > -		 or one thread/process exited/signalled.  */
> > +	      /* Eventing target is null if there were no waitable
> > +		 targets.  */
> > +	      if (event.target != nullptr)
> 
> Instead of this nullptr check, can't we write the conditions as:
> 
> 
> 	  if (event.ws.kind == TARGET_WAITKIND_NO_RESUMED)
>             {
> 	      /* All resumed threads exited.  */
>             }
>           else if (event.ws.kind == TARGET_WAITKIND_THREAD_EXITED
>                    || event.ws.kind == TARGET_WAITKIND_EXITED
>                    || event.ws.kind == TARGET_WAITKIND_SIGNALLED)
> 	    {
> 	      /* One thread/process exited/signalled.  */
>               ...
> 	    }
> 
> ?

I made this change in v5.

> > +		{
> > +		  thread_info *t = find_thread_ptid (event.target,
> > +						     event.ptid);
> > +		  /* Check if this is the first time we see this thread.
> > +		     Don't bother adding if it individually exited.  */
> > +		  if (t == nullptr
> > +		      && event.ws.kind != TARGET_WAITKIND_THREAD_EXITED)
> > +		    t = add_thread (event.target, event.ptid);
> > +
> 
> This doesn't look right for all targets.  On native GNU/Linux,
> the ptid returned by TARGET_WAITKIND_EXITED happens to be the main
> thread's ptid usually,
> 
>  infrun: target_wait (-1.0.0, status) =
>  infrun:   7941.7941.0 [process 7941],
>            ^^^^^^^^^
>  infrun:   status->kind = signalled, signal = GDB_SIGNAL_ALRM
> 
> but on e.g., remote targets, it will be just the pid of the process with
> tid/lwpid fields == 0,
> 
>  infrun: target_wait (-1.0.0, status) =
>  infrun:   8262.0.0 [process 8262],
>  infrun:   status->kind = signalled, signal = GDB_SIGNAL_ALRM
> 
> (Looks like darwin-nat.c is the same.)
> 
> so this will probably add a thread with such a ptid, which seems incorrect.
> I think we should still be able to find a thread in the list for the exiting
> inferior here, so if we don't find a thread by ptid, I guess we could use the
> first non-exited thread in the inferior to save the status.  Otherwise
> we'd have to save the status in the inferior or in a separate event list.

In v5, the first non-exited thread is used.  I also added a test scenario that uses
gdbserver to cover this case.

> Actually, looking at windows-nat.c's, EXIT_PROCESS_DEBUG_EVENT handling,
> that deletes the event thread before returning the TARGET_WAITKIND_EXITED
> event...  :-/  I'm not sure we can delete that windows_delete_thread
> call.  Maybe we can.

Err... I'm not able to comment on this.  In v5, I put a gdb_assert after attempting
to use the first non-exited thread.  This way, at least it can make the problem stand
out instead of seemingly hanging.

> BTW, the new test fails with --target_board=native-extended-gdbserver:
> 
>  Running /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.multi/multi-kill.exp ...
>  FAIL: gdb.multi/multi-kill.exp: iteration 2: attach to program 1 (got interactive prompt)
>  Running /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.multi/multi-exit.exp ...
>  FAIL: gdb.multi/multi-exit.exp: iteration 1: continue (timeout)

I added a guard at the beginning of the test, inspired from multi-target.exp, to exit the test
if multiple inferiors cannot be defined.

Thanks.
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928