[PATCH v8 0/6] Handle already-exited threads in 'stop_all_threads'

[PATCH v8 0/6] Handle already-exited threads in 'stop_all_threads'

[Pedro Alves]

Here's v8 of Tankut's series that fixes an infinite loop when GDB
attempts to stop a thread in 'stop_all_threads', if the thread has
already died.  The problem is fixed by handling waitstatuses that
denote exiting.  It also includes a broader change to handle the case
of a whole process exiting while GDB is stopping all threads.

I'm resending the whole series for Tankut under a new cover letter
because I find that sending just chunks of the series makes it hard to
piece things together and make sense of where we are.

Compared to v7, the main change is in the last patch of the series.
I've thought through how to handle the case of a process exiting while
we're stopping all threads in stop_all_threads.  This version switches
back to avoiding deleting the last thread of an inferior in remote.c's
target_update_thread_list implementation instead of the previous
proposal of not deleting threads with a status pending and re-adding
threads in stop_all_threads.  See the updated tail end of the commit
log of the last patch, which describes the new change in more detail.
It then tweaks a few things in common code related to that change, and
also makes the new testcases spawn more than 2 inferiors.

Tankut Baris Aktemur (6):
  gdb: protect some 'regcache_read_pc' calls
  gdb/infrun: move a 'regcache_read_pc' call down to first use
  gdb/infrun: extract out a code piece into 'mark_non_executing_threads'
    function
  gdb: introduce 'all_non_exited_process_targets' and
    'switch_to_target_no_thread'
  gdb/infrun: enable/disable thread events of all targets in
    stop_all_threads
  gdb/infrun: handle already-exited threads when attempting to stop

 gdb/infrun.c                           | 188 +++++++++++++++++++++------------
 gdb/process-stratum-target.c           |  24 +++++
 gdb/process-stratum-target.h           |  10 ++
 gdb/regcache.c                         |  18 ++++
 gdb/remote.c                           |  20 ++++
 gdb/testsuite/gdb.base/annota1.exp     |   2 +-
 gdb/testsuite/gdb.cp/annota2.exp       |   2 +-
 gdb/testsuite/gdb.multi/multi-exit.c   |  22 ++++
 gdb/testsuite/gdb.multi/multi-exit.exp | 138 ++++++++++++++++++++++++
 gdb/testsuite/gdb.multi/multi-kill.c   |  42 ++++++++
 gdb/testsuite/gdb.multi/multi-kill.exp | 127 ++++++++++++++++++++++
 gdb/thread.c                           |   2 +-
 gdbsupport/common-regcache.h           |   5 +
 13 files changed, 531 insertions(+), 69 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


base-commit: f7e23710fcb7133a3bbe7795ad0ddd2defca358a
-- 
2.14.5

[PATCH v8 1/6] gdb: protect some 'regcache_read_pc' calls

[Pedro Alves]

From: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>

It possible that a thread whose PC we attempt to read is already dead.
In this case, 'regcache_read_pc' errors out.  This impacts the
"proceed" execution flow, where GDB quits early before having a chance
to check if there exists a pending event.  To remedy, keep going with
a 0 value for the PC if 'regcache_read_pc' fails.  Because the value
of PC before resuming a thread is mostly used for storing and checking
the next time the thread stops, this tolerance is expected to be
harmless for a dead thread/process.

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

	* regcache.c (regcache_read_pc_protected): New function
	implementation that returns 0 if the PC cannot read via
	'regcache_read_pc'.
	* infrun.c (proceed): Call 'regcache_read_pc_protected'
	instead of 'regcache_read_pc'.
	(keep_going_pass_signal): Ditto.

gdbsupport/ChangeLog:
2020-04-03  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* common-regcache.h (regcache_read_pc_protected): New function
	declaration.
---
 gdb/infrun.c                 |  7 ++++---
 gdb/regcache.c               | 18 ++++++++++++++++++
 gdbsupport/common-regcache.h |  5 +++++
 3 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 3c6b201a9fc..5e01336ab09 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2995,7 +2995,8 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
   gdbarch = regcache->arch ();
   const address_space *aspace = regcache->aspace ();
 
-  pc = regcache_read_pc (regcache);
+  pc = regcache_read_pc_protected (regcache);
+
   thread_info *cur_thr = inferior_thread ();
 
   /* Fill in with reasonable starting values.  */
@@ -3122,7 +3123,7 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
      advanced.  Must do this before resuming any thread, as in
      all-stop/remote, once we resume we can't send any other packet
      until the target stops again.  */
-  cur_thr->prev_pc = regcache_read_pc (regcache);
+  cur_thr->prev_pc = regcache_read_pc_protected (regcache);
 
   {
     scoped_restore save_defer_tc = make_scoped_defer_target_commit_resume ();
@@ -7929,7 +7930,7 @@ keep_going_pass_signal (struct execution_control_state *ecs)
 
   /* Save the pc before execution, to compare with pc after stop.  */
   ecs->event_thread->prev_pc
-    = regcache_read_pc (get_thread_regcache (ecs->event_thread));
+    = regcache_read_pc_protected (get_thread_regcache (ecs->event_thread));
 
   if (ecs->event_thread->control.trap_expected)
     {
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 4f079c91a7f..1be794520ec 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -1220,6 +1220,24 @@ regcache_read_pc (struct regcache *regcache)
   return pc_val;
 }
 
+/* See gdbsupport/common-regcache.h.  */
+
+CORE_ADDR
+regcache_read_pc_protected (regcache *regcache)
+{
+  CORE_ADDR pc;
+  try
+    {
+      pc = regcache_read_pc (regcache);
+    }
+  catch (const gdb_exception_error &ex)
+    {
+      pc = 0;
+    }
+
+  return pc;
+}
+
 void
 regcache_write_pc (struct regcache *regcache, CORE_ADDR pc)
 {
diff --git a/gdbsupport/common-regcache.h b/gdbsupport/common-regcache.h
index 18446ff8416..650536e8a88 100644
--- a/gdbsupport/common-regcache.h
+++ b/gdbsupport/common-regcache.h
@@ -56,6 +56,11 @@ extern int regcache_register_size (const struct regcache *regcache, int n);
 
 extern CORE_ADDR regcache_read_pc (struct regcache *regcache);
 
+/* Read the PC register.  If PC cannot be read, return 0.
+   This is a wrapper around 'regcache_read_pc'.  */
+
+extern CORE_ADDR regcache_read_pc_protected (regcache *regcache);
+
 /* Read a raw register into a unsigned integer.  */
 extern enum register_status regcache_raw_read_unsigned
   (struct regcache *regcache, int regnum, ULONGEST *val);
-- 
2.14.5

[PATCH v8 2/6] gdb/infrun: move a 'regcache_read_pc' call down to first use

[Pedro Alves]

From: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>

In infrun.c's resume_1 function, move the definition of the local
variable PC down to its first use.  This is useful if the thread we want
to resume is already gone with a pending exit event, because we avoid
the error we would see otherwise when trying to read the PC.

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

	* infrun.c (resume_1): Move a 'regcache_read_pc' call down to first
	use.
---
 gdb/infrun.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 5e01336ab09..db88a1eef15 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2279,7 +2279,6 @@ resume_1 (enum gdb_signal sig)
   struct regcache *regcache = get_current_regcache ();
   struct gdbarch *gdbarch = regcache->arch ();
   struct thread_info *tp = inferior_thread ();
-  CORE_ADDR pc = regcache_read_pc (regcache);
   const address_space *aspace = regcache->aspace ();
   ptid_t resume_ptid;
   /* This represents the user's step vs continue request.  When
@@ -2358,6 +2357,8 @@ resume_1 (enum gdb_signal sig)
       step = 0;
     }
 
+  CORE_ADDR pc = regcache_read_pc (regcache);
+
   if (debug_infrun)
     fprintf_unfiltered (gdb_stdlog,
 			"infrun: resume (step=%d, signal=%s), "
-- 
2.14.5

[PATCH v8 3/6] gdb/infrun: extract out a code piece into 'mark_non_executing_threads' function

[Pedro Alves]

From: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>

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 db88a1eef15..c5bf2d0ad74 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4712,6 +4712,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
@@ -5145,41 +5186,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.14.5

[PATCH v8 4/6] gdb: introduce 'all_non_exited_process_targets' and 'switch_to_target_no_thread'

[Pedro Alves]

From: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>

Introduce two new convenience functions:

1. all_non_exited_process_targets: returns a collection of all process
stratum targets that have non-exited inferiors on them.  Useful for
iterating targets.

2. switch_to_target_no_thread: switch the context to the first
inferior of the given target, and to no selected thread.

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

	* process-stratum-target.h: Include <set>.
	(all_non_exited_process_targets, switch_to_target_no_thread): New
	function declarations.
	* process-stratum-target.c (all_non_exited_process_targets)
	(switch_to_target_no_thread): New function implementations.
---
 gdb/process-stratum-target.c | 24 ++++++++++++++++++++++++
 gdb/process-stratum-target.h | 10 ++++++++++
 2 files changed, 34 insertions(+)

diff --git a/gdb/process-stratum-target.c b/gdb/process-stratum-target.c
index f3fd9ee905d..9fb358a6a9f 100644
--- a/gdb/process-stratum-target.c
+++ b/gdb/process-stratum-target.c
@@ -83,3 +83,27 @@ process_stratum_target::has_execution (inferior *inf)
      through hoops.  */
   return inf->pid != 0;
 }
+
+/* See process-stratum-target.h.  */
+
+std::set<process_stratum_target *>
+all_non_exited_process_targets ()
+{
+  std::set<process_stratum_target *> targets;
+  for (inferior *inf : all_non_exited_inferiors ())
+    targets.insert (inf->process_target ());
+
+  return targets;
+}
+
+/* See process-stratum-target.h.  */
+
+void
+switch_to_target_no_thread (process_stratum_target *target)
+{
+  for (inferior *inf : all_inferiors (target))
+    {
+      switch_to_inferior_no_thread (inf);
+      break;
+    }
+}
diff --git a/gdb/process-stratum-target.h b/gdb/process-stratum-target.h
index 1be02100dcf..7e7905bf750 100644
--- a/gdb/process-stratum-target.h
+++ b/gdb/process-stratum-target.h
@@ -21,6 +21,7 @@
 #define PROCESS_STRATUM_TARGET_H
 
 #include "target.h"
+#include <set>
 
 /* Abstract base class inherited by all process_stratum targets.  */
 
@@ -82,4 +83,13 @@ as_process_stratum_target (target_ops *target)
   return static_cast<process_stratum_target *> (target);
 }
 
+/* Return a collection of targets that have non-exited inferiors.  */
+
+extern std::set<process_stratum_target *> all_non_exited_process_targets ();
+
+/* Switch to the first inferior (and program space) of TARGET, and
+   switch to no thread selected.  */
+
+extern void switch_to_target_no_thread (process_stratum_target *target);
+
 #endif /* !defined (PROCESS_STRATUM_TARGET_H) */
-- 
2.14.5

RE: [PATCH v8 4/6] gdb: introduce 'all_non_exited_process_targets' and 'switch_to_target_no_thread'

[Aktemur, Tankut Baris]

On Wednesday, May 13, 2020 10:54 PM, Pedro Alves wrote:
> Introduce two new convenience functions:
> 
> 1. all_non_exited_process_targets: returns a collection of all process
> stratum targets that have non-exited inferiors on them.  Useful for
> iterating targets.
> 
> 2. switch_to_target_no_thread: switch the context to the first
> inferior of the given target, and to no selected thread.
> 
> gdb/ChangeLog:
> 2020-04-30  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
> 
> 	* process-stratum-target.h: Include <set>.
> 	(all_non_exited_process_targets, switch_to_target_no_thread): New
> 	function declarations.
> 	* process-stratum-target.c (all_non_exited_process_targets)
> 	(switch_to_target_no_thread): New function implementations.
> ---
>  gdb/process-stratum-target.c | 24 ++++++++++++++++++++++++
>  gdb/process-stratum-target.h | 10 ++++++++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/gdb/process-stratum-target.c b/gdb/process-stratum-target.c
> index f3fd9ee905d..9fb358a6a9f 100644
> --- a/gdb/process-stratum-target.c
> +++ b/gdb/process-stratum-target.c
> @@ -83,3 +83,27 @@ process_stratum_target::has_execution (inferior *inf)
>       through hoops.  */
>    return inf->pid != 0;
>  }
> +
> +/* See process-stratum-target.h.  */
> +
> +std::set<process_stratum_target *>
> +all_non_exited_process_targets ()
> +{

Christian suggested adding a comment here to state why a set was needed.
I thought about this:

/* Inferiors may share targets.  To eliminate duplicates, use a set.  */

> +  std::set<process_stratum_target *> targets;
> +  for (inferior *inf : all_non_exited_inferiors ())
> +    targets.insert (inf->process_target ());
> +
> +  return targets;
> +}

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 v8 4/6] gdb: introduce 'all_non_exited_process_targets' and 'switch_to_target_no_thread'

[Pedro Alves]

On 5/14/20 9:44 AM, Aktemur, Tankut Baris wrote:

> Christian suggested adding a comment here to state why a set was needed.
> I thought about this:
> 
> /* Inferiors may share targets.  To eliminate duplicates, use a set.  */

Sure, sounds good.

(Note I had done one change compared to your previous version I forgot
to mention -- the "#include <set>" should be in the .h file, rather
than in the .c file, because std::set is used in the .h file already.)

Thanks,
Pedro Alves

RE: [PATCH v8 4/6] gdb: introduce 'all_non_exited_process_targets' and 'switch_to_target_no_thread'

[Aktemur, Tankut Baris]

On Thursday, May 14, 2020 1:13 PM, Pedro Alves wrote:
> (Note I had done one change compared to your previous version I forgot
> to mention -- the "#include <set>" should be in the .h file, rather
> than in the .c file, because std::set is used in the .h file already.)

Yes, I diff'ed the patches pairwise and noticed that.  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

[PATCH v8 5/6] gdb/infrun: enable/disable thread events of all targets in stop_all_threads

[Pedro Alves]

From: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>

In stop_all_threads, the thread events of the current top target are
enabled at the beginning of the function and then disabled at the end
(at scope exit time).  Because there may be multiple targets whose
thread lists will be updated and whose threads are stopped,
enable/disable thread events for all targets.

This update caused a change in the annotations.  In particular, a
"frames-invalid" annotation is printed one more time due to switching
the current inferior.  Hence, gdb.base/annota1.exp and
gdb.cp/annota2.exp tests are also updated.

Regression-tested on X86_64 Linux using the default board file and the
native-extended-gdbserver board file.

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

	* infrun.c (stop_all_threads): Enable/disable thread events of all
	targets.

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

	* gdb.base/annota1.exp: Update the expected output.
	* gdb.cp/annota2.exp: Ditto.
---
 gdb/infrun.c                       | 15 +++++++++++++--
 gdb/testsuite/gdb.base/annota1.exp |  2 +-
 gdb/testsuite/gdb.cp/annota2.exp   |  2 +-
 3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index c5bf2d0ad74..6602bc28d5e 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4769,8 +4769,12 @@ stop_all_threads (void)
 
   scoped_restore_current_thread restore_thread;
 
-  target_thread_events (1);
-  SCOPE_EXIT { target_thread_events (0); };
+  /* Enable thread events of all targets.  */
+  for (auto *target : all_non_exited_process_targets ())
+    {
+      switch_to_target_no_thread (target);
+      target_thread_events (true);
+    }
 
   /* Request threads to stop, and then wait for the stops.  Because
      threads we already know about can spawn more threads while we're
@@ -4962,6 +4966,13 @@ stop_all_threads (void)
 	}
     }
 
+  /* Disable thread events of all targets.  */
+  for (auto *target : all_non_exited_process_targets ())
+    {
+      switch_to_target_no_thread (target);
+      target_thread_events (false);
+    }
+
   if (debug_infrun)
     fprintf_unfiltered (gdb_stdlog, "infrun: stop_all_threads done\n");
 }
diff --git a/gdb/testsuite/gdb.base/annota1.exp b/gdb/testsuite/gdb.base/annota1.exp
index 9d3bf73431c..829d144cc20 100644
--- a/gdb/testsuite/gdb.base/annota1.exp
+++ b/gdb/testsuite/gdb.base/annota1.exp
@@ -223,7 +223,7 @@ gdb_test_multiple "break printf" "break printf" {
 #
 # get to printf
 #
-set pat_begin "\r\n\032\032post-prompt\r\nContinuing.\r\n\r\n\032\032starting\r\n\r\n\032\032frames-invalid\r\n${breakpoints_invalid}"
+set pat_begin "\r\n\032\032post-prompt\r\nContinuing.\r\n\r\n\032\032starting\r\n\r\n\032\032frames-invalid\r\n${breakpoints_invalid}\r\n\032\032frames-invalid\r\n"
 set pat_adjust "warning: Breakpoint 3 address previously adjusted from $hex to $hex.\r\n"
 set pat_end "\r\n\032\032breakpoint 3\r\n\r\nBreakpoint 3, \r\n\032\032frame-begin 0 $hex\r\n\r\n(\032\032frame-address\r\n$hex\r\n\032\032frame-address-end\r\n in \r\n)*.*\032\032frame-function-name\r\n.*printf(@.*)?\r\n\032\032frame-args\r\n.*\032\032frame-end\r\n\r\n\032\032stopped\r\n$gdb_prompt$"
 
diff --git a/gdb/testsuite/gdb.cp/annota2.exp b/gdb/testsuite/gdb.cp/annota2.exp
index dd3a0a5d6de..1b4f04bb445 100644
--- a/gdb/testsuite/gdb.cp/annota2.exp
+++ b/gdb/testsuite/gdb.cp/annota2.exp
@@ -218,7 +218,7 @@ set pat [multi_line "" \
 	     "\032\032post-prompt" \
 	     "" \
 	     "\032\032starting" \
-	     "\(${frames_invalid}\)*${breakpoints_invalid}" \
+	     "\(${frames_invalid}\)*${breakpoints_invalid}\(${frames_invalid}\)*" \
 	     "\032\032watchpoint 3" \
 	     ".*atchpoint 3: a.x" \
 	     "" \
-- 
2.14.5

RE: [PATCH v8 5/6] gdb/infrun: enable/disable thread events of all targets in stop_all_threads

[Aktemur, Tankut Baris]

On Wednesday, May 13, 2020 10:54 PM, Pedro Alves wrote:
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index c5bf2d0ad74..6602bc28d5e 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -4769,8 +4769,12 @@ stop_all_threads (void)
> 
>    scoped_restore_current_thread restore_thread;
> 
> -  target_thread_events (1);
> -  SCOPE_EXIT { target_thread_events (0); };
> +  /* Enable thread events of all targets.  */
> +  for (auto *target : all_non_exited_process_targets ())
> +    {
> +      switch_to_target_no_thread (target);
> +      target_thread_events (true);
> +    }
> 
>    /* Request threads to stop, and then wait for the stops.  Because
>       threads we already know about can spawn more threads while we're
> @@ -4962,6 +4966,13 @@ stop_all_threads (void)
>  	}
>      }
> 
> +  /* Disable thread events of all targets.  */
> +  for (auto *target : all_non_exited_process_targets ())
> +    {
> +      switch_to_target_no_thread (target);
> +      target_thread_events (false);
> +    }
> +
>    if (debug_infrun)
>      fprintf_unfiltered (gdb_stdlog, "infrun: stop_all_threads done\n");
>  }

In internal communication, Markus pointed out that while the original
SCOPED_EXIT block would run also in the case of exceptions, the new code
does not.  Here is an update that uses SCOPED_EXIT for the thread event
disabling loop, too:

diff --git a/gdb/infrun.c b/gdb/infrun.c
index c5bf2d0ad74..04fcc390d17 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4769,8 +4769,22 @@ stop_all_threads (void)

   scoped_restore_current_thread restore_thread;

-  target_thread_events (1);
-  SCOPE_EXIT { target_thread_events (0); };
+  /* Enable thread events of all targets.  */
+  for (auto *target : all_non_exited_process_targets ())
+    {
+      switch_to_target_no_thread (target);
+      target_thread_events (true);
+    }
+
+  SCOPE_EXIT
+    {
+      /* Disable thread events of all targets.  */
+      for (auto *target : all_non_exited_process_targets ())
+       {
+         switch_to_target_no_thread (target);
+         target_thread_events (false);
+       }
+    };

   /* Request threads to stop, and then wait for the stops.  Because
      threads we already know about can spawn more threads while we're

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 v8 5/6] gdb/infrun: enable/disable thread events of all targets in stop_all_threads

[Pedro Alves]

On 5/14/20 9:44 AM, Aktemur, Tankut Baris wrote:
> On Wednesday, May 13, 2020 10:54 PM, Pedro Alves wrote:
>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>> index c5bf2d0ad74..6602bc28d5e 100644
>> --- a/gdb/infrun.c
>> +++ b/gdb/infrun.c
>> @@ -4769,8 +4769,12 @@ stop_all_threads (void)
>>
>>    scoped_restore_current_thread restore_thread;
>>
>> -  target_thread_events (1);
>> -  SCOPE_EXIT { target_thread_events (0); };
>> +  /* Enable thread events of all targets.  */
>> +  for (auto *target : all_non_exited_process_targets ())
>> +    {
>> +      switch_to_target_no_thread (target);
>> +      target_thread_events (true);
>> +    }
>>
>>    /* Request threads to stop, and then wait for the stops.  Because
>>       threads we already know about can spawn more threads while we're
>> @@ -4962,6 +4966,13 @@ stop_all_threads (void)
>>  	}
>>      }
>>
>> +  /* Disable thread events of all targets.  */
>> +  for (auto *target : all_non_exited_process_targets ())
>> +    {
>> +      switch_to_target_no_thread (target);
>> +      target_thread_events (false);
>> +    }
>> +
>>    if (debug_infrun)
>>      fprintf_unfiltered (gdb_stdlog, "infrun: stop_all_threads done\n");
>>  }
> 
> In internal communication, Markus pointed out that while the original
> SCOPED_EXIT block would run also in the case of exceptions, the new code
> does not.  Here is an update that uses SCOPED_EXIT for the thread event
> disabling loop, too:
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index c5bf2d0ad74..04fcc390d17 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -4769,8 +4769,22 @@ stop_all_threads (void)
> 
>    scoped_restore_current_thread restore_thread;
> 
> -  target_thread_events (1);
> -  SCOPE_EXIT { target_thread_events (0); };
> +  /* Enable thread events of all targets.  */
> +  for (auto *target : all_non_exited_process_targets ())
> +    {
> +      switch_to_target_no_thread (target);
> +      target_thread_events (true);
> +    }
> +
> +  SCOPE_EXIT
> +    {
> +      /* Disable thread events of all targets.  */
> +      for (auto *target : all_non_exited_process_targets ())
> +       {
> +         switch_to_target_no_thread (target);
> +         target_thread_events (false);
> +       }
> +    };
> 

Yes, I noticed it too, but forgot to address it.  Thanks for bringing
it up.

I think the:

  if (debug_infrun)
    fprintf_unfiltered (gdb_stdlog, "infrun: stop_all_threads done\n");

bit should also move to within SCOPE_EXIT, since we currently
print that before the destructors are run.  But that could be seen
as an unrelated change.

Thanks,
Pedro Alves

RE: [PATCH v8 5/6] gdb/infrun: enable/disable thread events of all targets in stop_all_threads

[Aktemur, Tankut Baris]

On Thursday, May 14, 2020 1:17 PM, Pedro Alves wrote:
> On 5/14/20 9:44 AM, Aktemur, Tankut Baris wrote:
> > On Wednesday, May 13, 2020 10:54 PM, Pedro Alves wrote:
> >> diff --git a/gdb/infrun.c b/gdb/infrun.c
> >> index c5bf2d0ad74..6602bc28d5e 100644
> >> --- a/gdb/infrun.c
> >> +++ b/gdb/infrun.c
> >> @@ -4769,8 +4769,12 @@ stop_all_threads (void)
> >>
> >>    scoped_restore_current_thread restore_thread;
> >>
> >> -  target_thread_events (1);
> >> -  SCOPE_EXIT { target_thread_events (0); };
> >> +  /* Enable thread events of all targets.  */
> >> +  for (auto *target : all_non_exited_process_targets ())
> >> +    {
> >> +      switch_to_target_no_thread (target);
> >> +      target_thread_events (true);
> >> +    }
> >>
> >>    /* Request threads to stop, and then wait for the stops.  Because
> >>       threads we already know about can spawn more threads while we're
> >> @@ -4962,6 +4966,13 @@ stop_all_threads (void)
> >>  	}
> >>      }
> >>
> >> +  /* Disable thread events of all targets.  */
> >> +  for (auto *target : all_non_exited_process_targets ())
> >> +    {
> >> +      switch_to_target_no_thread (target);
> >> +      target_thread_events (false);
> >> +    }
> >> +
> >>    if (debug_infrun)
> >>      fprintf_unfiltered (gdb_stdlog, "infrun: stop_all_threads done\n");
> >>  }
> >
> > In internal communication, Markus pointed out that while the original
> > SCOPED_EXIT block would run also in the case of exceptions, the new code
> > does not.  Here is an update that uses SCOPED_EXIT for the thread event
> > disabling loop, too:
> >
> > diff --git a/gdb/infrun.c b/gdb/infrun.c
> > index c5bf2d0ad74..04fcc390d17 100644
> > --- a/gdb/infrun.c
> > +++ b/gdb/infrun.c
> > @@ -4769,8 +4769,22 @@ stop_all_threads (void)
> >
> >    scoped_restore_current_thread restore_thread;
> >
> > -  target_thread_events (1);
> > -  SCOPE_EXIT { target_thread_events (0); };
> > +  /* Enable thread events of all targets.  */
> > +  for (auto *target : all_non_exited_process_targets ())
> > +    {
> > +      switch_to_target_no_thread (target);
> > +      target_thread_events (true);
> > +    }
> > +
> > +  SCOPE_EXIT
> > +    {
> > +      /* Disable thread events of all targets.  */
> > +      for (auto *target : all_non_exited_process_targets ())
> > +       {
> > +         switch_to_target_no_thread (target);
> > +         target_thread_events (false);
> > +       }
> > +    };
> >
> 
> Yes, I noticed it too, but forgot to address it.  Thanks for bringing
> it up.
> 
> I think the:
> 
>   if (debug_infrun)
>     fprintf_unfiltered (gdb_stdlog, "infrun: stop_all_threads done\n");
> 
> bit should also move to within SCOPE_EXIT, since we currently
> print that before the destructors are run.  But that could be seen
> as an unrelated change.

OK, I'll move that piece into SCOPED_EXIT, too.

> 
> Thanks,
> Pedro Alves

-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

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

[Pedro Alves]

From: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>

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 10449)
  infrun: clear_proceed_status_thread (process 10453)
  infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
  infrun: proceed: resuming process 10449
  infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 10449] at 0x55555555514e
  infrun: infrun_async(1)
  infrun: prepare_to_wait
  infrun: proceed: resuming process 10453
  infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 10453] at 0x55555555514e
  infrun: prepare_to_wait
  infrun: Found 2 inferiors, starting at #0
  infrun: target_wait (-1.0.0, status) =
  infrun:   10449.10449.0 [process 10449],
  infrun:   status->kind = exited, status = 0
  infrun: handle_inferior_event status->kind = exited, status = 0
  [Inferior 1 (process 10449) exited normally]
  infrun: stop_waiting
  infrun: stop_all_threads
  infrun: stop_all_threads, pass=0, iterations=0
  infrun:   process 10453 executing, need stop
  infrun: target_wait (-1.0.0, status) =
  infrun:   10453.10453.0 [process 10453],
  infrun:   status->kind = exited, status = 0
  infrun: stop_all_threads status->kind = exited, status = 0 process 10453
  infrun:   process 10453 executing, already stopping
  infrun: target_wait (-1.0.0, status) =
  infrun:   -1.0.0 [process -1],
  infrun:   status->kind = no-resumed
  infrun: infrun_async(0)
  infrun: stop_all_threads status->kind = no-resumed process -1
  infrun:   process 10453 executing, already stopping
  infrun: stop_all_threads status->kind = no-resumed process -1
  infrun:   process 10453 executing, already stopping
  infrun: stop_all_threads status->kind = no-resumed process -1
  infrun:   process 10453 executing, already stopping
  infrun: stop_all_threads status->kind = no-resumed process -1
  infrun:   process 10453 executing, already stopping
  infrun: stop_all_threads status->kind = no-resumed process -1
  infrun:   process 10453 executing, already stopping
  infrun: stop_all_threads status->kind = no-resumed process -1
  infrun:   process 10453 executing, already stopping
  infrun: stop_all_threads status->kind = no-resumed process -1
  infrun:   process 10453 executing, already stopping
  infrun: stop_all_threads status->kind = no-resumed process -1
  infrun:   process 10453 executing, already stopping
  infrun: stop_all_threads status->kind = no-resumed process -1
  infrun:   process 10453 executing, already stopping
  infrun: stop_all_threads status->kind = no-resumed process -1
  infrun:   process 10453 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 31229)
  infrun: clear_proceed_status_thread (process 31233)
  infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
  infrun: proceed: resuming process 31229
  infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 31229] at 0x55555555514e
  infrun: infrun_async(1)
  infrun: prepare_to_wait
  infrun: proceed: resuming process 31233
  infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 31233] at 0x55555555514e
  infrun: prepare_to_wait
  infrun: Found 2 inferiors, starting at #0
  infrun: target_wait (-1.0.0, status) =
  infrun:   31229.31229.0 [process 31229],
  infrun:   status->kind = exited, status = 0
  infrun: handle_inferior_event status->kind = exited, status = 0
  [Inferior 1 (process 31229) exited normally]
  infrun: stop_waiting
  infrun: stop_all_threads
  infrun: stop_all_threads, pass=0, iterations=0
  infrun:   process 31233 executing, need stop
  infrun: target_wait (-1.0.0, status) =
  infrun:   31233.31233.0 [process 31233],
  infrun:   status->kind = exited, status = 0
  infrun: stop_all_threads status->kind = exited, status = 0 process 31233
  infrun: saving status status->kind = exited, status = 0 for 31233.31233.0
  infrun:   process 31233 not executing
  infrun: stop_all_threads, pass=1, iterations=1
  infrun:   process 31233 not executing
  infrun: stop_all_threads done
  (gdb)

The exit event from Inferior 1 is received and shown to the user.
The exit event from Inferior 2 is not displayed, but kept pending.

  (gdb) info inferiors
    Num  Description       Connection           Executable
  * 1    <null>                                 a.out
    2    process 31233     1 (native)           a.out
  (gdb) inferior 2
  [Switching to inferior 2 [process 31233] (a.out)]
  [Switching to thread 2.1 (process 31233)]
  Couldn't get registers: No such process.
  (gdb) continue
  Continuing.
  infrun: clear_proceed_status_thread (process 31233)
  infrun: clear_proceed_status_thread: thread process 31233 has pending wait status status->kind = exited, status = 0 (currently_stepping=0).
  infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
  infrun: proceed: resuming process 31233
  infrun: resume: thread process 31233 has pending wait status status->kind = exited, status = 0 (currently_stepping=0).
  infrun: prepare_to_wait
  infrun: Using pending wait status status->kind = exited, status = 0 for process 31233.
  infrun: target_wait (-1.0.0, status) =
  infrun:   31233.31233.0 [process 31233],
  infrun:   status->kind = exited, status = 0
  infrun: handle_inferior_event status->kind = exited, status = 0
  [Inferior 2 (process 31233) exited normally]
  infrun: stop_waiting
  (gdb) info inferiors
    Num  Description       Connection           Executable
    1    <null>                                 a.out
  * 2    <null>                                 a.out
  (gdb)

When a process exits and we leave the process exit event pending, we
need to make sure that at least one thread is left listed in the
inferior's thread list.  This is necessary in order to make sure we
have a thread that we can later resume, so the process exit event can
be collected/reported.

When native debugging, the GNU/Linux back end already makes sure that
the last LWP isn't deleted.

When remote debugging against GNU/Linux GDBserver, the GNU/Linux
GDBserver backend also makes sure that the last thread isn't deleted
until the process exit event is reported to GDBserver core.

However, between the backend reporting the process exit event to
GDBserver core, and GDB consuming the event, GDB may update the thread
list and find no thread left in the process.  The process exit event
will be pending somewhere in GDBserver's stop reply queue, or
gdb/remote.c's queue, or whathever other event queue inbetween
GDBserver and infrun.c's handle_inferior_event.

This patch tweaks remote.c's target_update_thread_list implementation
to avoid deleting the last thread of an inferior.

In the past, this case of inferior-with-no-threads lead to a special
case at the bottom of handle_no_resumed, where it reads:

  /* Note however that we may find no resumed thread because the whole
     process exited meanwhile (thus updating the thread list results
     in an empty thread list).  In this case we know we'll be getting
     a process exit event shortly.  */
  for (inferior *inf : all_non_exited_inferiors (ecs->target))

In current master, that code path is still reacheable with the
gdb.threads/continue-pending-after-query.exp testcase, when tested
against GDBserver, with "maint set target-non-stop" forced "on".

With this patch, the scenario that loop was concerned about is still
properly handled, because the loop above it finds the process's last
thread with "executing" set to true, and thus the handle_no_resumed
function still returns true.

Since GNU/Linux native and remote are the only targets that support
non-stop mode, and with this patch, we always make sure the inferior
has at least one thread, this patch also removes that "inferior with
no threads" special case handling from handle_no_resumed.

Since remote.c now has a special case where we treat a thread that has
already exited as if it was still alive, we might need to tweak
remote.c's target_thread_alive implementation to return true for that
thread without querying the remote side (which would say "no, not
alive").  After inspecting all the target_thread_alive calls in the
codebase, it seems that only the one from prune_threads could result
in that thread being accidentaly deleted.  There's only one call to
prune_threads in GDB's common code, so this patch handles this by
replacing the prune_threads call with a delete_exited_threads call.
This seems like an improvement anyway, because we'll still be doing
what the comment suggests we want to do, and, we avoid remote protocol
traffic.

Regression-tested on X86_64 Linux.

gdb/ChangeLog:
yyyy-mm-dd  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
	    Tom de Vries  <tdevries@suse.de>
	    Pedro Alves  <palves@redhat.com>

	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.
	(handle_no_resumed): Remove code handling a live inferior with no
	threads.
	* remote.c (single_non_exited_thread_p): New.
	(remote_target::update_thread_list): Do not delete a thread if is
	the last thread of the process.
	* thread.c (thread_select): Call delete_exited_threads instead of
	prune_threads.

gdb/testsuite/ChangeLog:
yyyy-mm-dd  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
	    Pedro Alves  <palves@redhat.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.
---
 gdb/infrun.c                           |  86 ++++++++++++++------
 gdb/remote.c                           |  20 +++++
 gdb/testsuite/gdb.multi/multi-exit.c   |  22 ++++++
 gdb/testsuite/gdb.multi/multi-exit.exp | 138 +++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.multi/multi-kill.c   |  42 ++++++++++
 gdb/testsuite/gdb.multi/multi-kill.exp | 127 ++++++++++++++++++++++++++++++
 gdb/thread.c                           |   2 +-
 7 files changed, 411 insertions(+), 26 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 6602bc28d5e..c9a092e4943 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4791,7 +4791,11 @@ stop_all_threads (void)
 	{
 	  int need_wait = 0;
 
-	  update_thread_list ();
+	  for (auto *target : all_non_exited_process_targets ())
+	    {
+	      switch_to_target_no_thread (target);
+	      update_thread_list ();
+	    }
 
 	  /* Go through all threads looking for threads that we need
 	     to tell the target to stop.  */
@@ -4866,13 +4870,63 @@ stop_all_threads (void)
 				  target_pid_to_str (event.ptid).c_str ());
 	    }
 
-	  if (event.ws.kind == TARGET_WAITKIND_NO_RESUMED
-	      || event.ws.kind == TARGET_WAITKIND_THREAD_EXITED
-	      || event.ws.kind == TARGET_WAITKIND_EXITED
-	      || event.ws.kind == TARGET_WAITKIND_SIGNALLED)
+	  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)
 	    {
-	      /* All resumed threads exited
-		 or one thread/process exited/signalled.  */
+	      /* One thread/process exited/signalled.  */
+
+	      thread_info *t = nullptr;
+
+	      /* The target may have reported just a pid.  If so, try
+		 the first non-exited thread.  */
+	      if (event.ptid.is_pid ())
+		{
+		  int pid  = event.ptid.pid ();
+		  inferior *inf = find_inferior_pid (event.target, pid);
+		  for (thread_info *tp : inf->non_exited_threads ())
+		    {
+		      t = tp;
+		      break;
+		    }
+
+		  /* If there is no available thread, the event would
+		     have to be appended to a per-inferior event list,
+		     which, does not exist (and if it did, we'd have
+		     to adjust run control command to be able to
+		     resume such an inferior).  We assert here instead
+		     of going into an infinite loop.  */
+		  gdb_assert (t != nullptr);
+
+		  if (debug_infrun)
+		    fprintf_unfiltered (gdb_stdlog,
+					"infrun: stop_all_threads, using %s\n",
+					target_pid_to_str (t->ptid).c_str ());
+		}
+	      else
+		{
+		  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.  */
+		  switch_to_thread_no_regs (t);
+		  mark_non_executing_threads (event.target, event.ptid,
+					      event.ws);
+		  save_waitstatus (t, &event.ws);
+		  t->stop_requested = false;
+		}
 	    }
 	  else
 	    {
@@ -5060,24 +5114,6 @@ handle_no_resumed (struct execution_control_state *ecs)
 	}
     }
 
-  /* Note however that we may find no resumed thread because the whole
-     process exited meanwhile (thus updating the thread list results
-     in an empty thread list).  In this case we know we'll be getting
-     a process exit event shortly.  */
-  for (inferior *inf : all_non_exited_inferiors (ecs->target))
-    {
-      thread_info *thread = any_live_thread_of_inferior (inf);
-      if (thread == NULL)
-	{
-	  if (debug_infrun)
-	    fprintf_unfiltered (gdb_stdlog,
-				"infrun: TARGET_WAITKIND_NO_RESUMED "
-				"(expect process exit)\n");
-	  prepare_to_wait (ecs);
-	  return 1;
-	}
-    }
-
   /* Go ahead and report the event.  */
   return 0;
 }
diff --git a/gdb/remote.c b/gdb/remote.c
index 5db406e045c..8e12ba9603e 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3785,6 +3785,18 @@ remote_target::remote_get_threads_with_qthreadinfo (threads_listing_context *con
   return 0;
 }
 
+/* Return true if INF only has one non-exited thread.  */
+
+static bool
+single_non_exited_thread_p (inferior *inf)
+{
+  int count = 0;
+  for (thread_info *tp ATTRIBUTE_UNUSED : inf->non_exited_threads ())
+    if (++count > 1)
+      break;
+  return count == 1;
+}
+
 /* Implement the to_update_thread_list function for the remote
    targets.  */
 
@@ -3824,6 +3836,14 @@ remote_target::update_thread_list ()
 
 	  if (!context.contains_thread (tp->ptid))
 	    {
+	      /* Do not remove the thread if it is the last thread in
+		 the inferior.  This situation happens when we have a
+		 pending exit process status to process.  Otherwise we
+		 may end up with a seemingly live inferior (i.e.  pid
+		 != 0) that has no threads.  */
+	      if (single_non_exited_thread_p (tp->inf))
+		continue;
+
 	      /* Not found.  */
 	      delete_thread (tp);
 	    }
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..3c4e99164ed
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/multi-exit.exp
@@ -0,0 +1,138 @@
+# 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
+
+if {[use_gdb_stub]} {
+    return 0
+}
+
+if {[build_executable "failed to prepare" $testfile $srcfile]} {
+    return -1
+}
+
+# We are testing GDB's ability to stop all threads.
+# Hence, go with the all-stop-on-top-of-non-stop mode.
+save_vars { GDBFLAGS } {
+    append GDBFLAGS " -ex \"maint set target-non-stop on\""
+    clean_restart ${binfile}
+}
+
+with_test_prefix "inf 1" {
+    gdb_load $binfile
+
+    if {[gdb_start_cmd] < 0} {
+	fail "could not start"
+	return -1
+    }
+    gdb_test "" ".*reakpoint .*, main .*${srcfile}.*" "start"
+}
+
+# Start inferior NUM.
+
+proc start_inferior {num} {
+    global srcfile binfile
+
+    gdb_test "add-inferior" "Added inferior $num.*" \
+	"add empty inferior $num"
+    gdb_test "inferior $num" "Switching to inferior $num.*" \
+	"switch to inferior $num"
+
+    with_test_prefix "inf $num" {
+	gdb_load $binfile
+
+	if {[gdb_start_cmd] < 0} {
+	    fail "could not start"
+	    return -1
+	}
+	gdb_test "" ".*reakpoint .*, main .*${srcfile}.*" "start"
+    }
+
+    return 0
+}
+
+# Sufficient inferiors to make sure that at least some other inferior
+# exits while we're handling a process exit event.
+set NUM_INFS 10
+
+for {set i 2} {$i <= $NUM_INFS} {incr i} {
+    if {[start_inferior $i] < 0} {
+	return -1
+    }
+}
+
+# We want to continue all processes.
+gdb_test_no_output "set schedule-multiple on"
+
+# Check that "continue" continues to the end of an inferior, as many
+# times as we have inferiors.
+
+for {set i 1} {$i <= $NUM_INFS} {incr i} {
+    with_test_prefix "inf $i" {
+	set live_inferior ""
+
+	# Pick any live inferior.
+	gdb_test_multiple "info inferiors" "" {
+	    -re "($decimal) *process.*$gdb_prompt $" {
+		set live_inferior $expect_out(1,string)
+	    }
+	}
+
+	if {$live_inferior == ""} {
+	    return -1
+	}
+
+	gdb_test "inferior $live_inferior" \
+	    ".*Switching to inferior $live_inferior.*" \
+	    "switch to another inferior"
+
+	set exited_inferior ""
+
+	# We want GDB to complete the command and return the prompt
+	# instead of going into an infinite loop.
+	gdb_test_multiple "continue" "first continue" {
+	    -re "Inferior ($decimal) \[^\n\r\]+ exited normally.*$gdb_prompt $" {
+		set exited_inferior $expect_out(1,string)
+		pass $gdb_test_name
+	    }
+	}
+
+	if {$exited_inferior == ""} {
+	    return -1
+	}
+    }
+}
+
+# Finally, check that we can re-run all inferiors.  Note that if any
+# inferior was still alive this would catch it, as "run" would query
+# "Start it from the beginning?".
+
+delete_breakpoints
+
+for {set i 1} {$i <= $NUM_INFS} {incr i} {
+    with_test_prefix "inf $i" {
+	gdb_test "run" "$inferior_exited_re normally\]" \
+	    "re-run inferior"
+    }
+}
diff --git a/gdb/testsuite/gdb.multi/multi-kill.c b/gdb/testsuite/gdb.multi/multi-kill.c
new file mode 100644
index 00000000000..66642bbb0e6
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/multi-kill.c
@@ -0,0 +1,42 @@
+/* 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/>.  */
+
+#include <sys/types.h>
+#include <unistd.h>
+
+static pid_t pid;
+
+static void
+initialized ()
+{
+}
+
+int
+main ()
+{
+  pid = getpid ();
+  initialized ();
+
+  /* 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..7deaadc68e8
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/multi-kill.exp
@@ -0,0 +1,127 @@
+# 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
+
+if {[use_gdb_stub]} {
+    return 0
+}
+
+if {[build_executable "failed to prepare" $testfile $srcfile {debug}]} {
+    return -1
+}
+
+# We are testing GDB's ability to stop all threads.
+# Hence, go with the all-stop-on-top-of-non-stop mode.
+save_vars { GDBFLAGS } {
+    append GDBFLAGS " -ex \"maint set target-non-stop on\""
+    clean_restart ${binfile}
+}
+
+# Start inferior NUM and record its PID in the TESTPID array.
+
+proc start_inferior {num} {
+    with_test_prefix "start_inferior $num" {
+	global testpid binfile srcfile
+
+	if {$num != 1} {
+	    gdb_test "add-inferior" "Added inferior .*" \
+		"add empty inferior"
+	    gdb_test "inferior $num" "Switching to inferior .*" \
+		"switch to inferior"
+	}
+
+	gdb_load $binfile
+
+	gdb_breakpoint "initialized" {temporary}
+	gdb_run_cmd
+	gdb_test "" ".*reakpoint .*, initialized .*${srcfile}.*" "run"
+
+	set testpid($num) [get_integer_valueof "pid" -1]
+	if {$testpid($num) == -1} {
+	    return -1
+	}
+
+	return 0
+    }
+}
+
+# Sufficient inferiors to make sure that at least some other inferior
+# is killed while we're handling a killed event.
+set NUM_INFS 10
+
+for {set i 1} {$i <= $NUM_INFS} {incr i} {
+    if {[start_inferior $i] < 0} {
+	return -1
+    }
+}
+
+# We want to continue all processes.
+gdb_test_no_output "set schedule-multiple on"
+
+# Resume, but then kill all from outside.
+gdb_test_multiple "continue" "continue processes" {
+    -re "Continuing.\[\r\n\]+" {
+	# Kill all processes at once.
+
+	set kill_cmd "kill -9"
+	for {set i 1} {$i <= $NUM_INFS} {incr i} {
+	    append kill_cmd " $testpid($i)"
+	}
+
+	remote_exec target $kill_cmd
+	exp_continue
+    }
+    -re "Program terminated with signal.*$gdb_prompt" {
+	pass $gdb_test_name
+    }
+}
+
+# Check that "continue" collects the process kill event, as many times
+# as we have inferiors left.
+
+for {set i 2} {$i <= $NUM_INFS} {incr i} {
+    with_test_prefix "inf $i" {
+	set live_inferior ""
+
+	# Pick any live inferior.
+	gdb_test_multiple "info inferiors" "" {
+	    -re "($decimal) *process.*$gdb_prompt $" {
+		set live_inferior $expect_out(1,string)
+	    }
+	}
+
+	if {$live_inferior == ""} {
+	    return -1
+	}
+
+	gdb_test "inferior $live_inferior" \
+	    ".*Switching to inferior $live_inferior.*" \
+	    "switch to inferior"
+
+	gdb_test "continue" \
+	    "Program terminated with signal SIGKILL, .*" \
+	    "continue to SIGKILL"
+    }
+}
diff --git a/gdb/thread.c b/gdb/thread.c
index 03805bd2565..02672f01fcf 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -2043,7 +2043,7 @@ thread_select (const char *tidstr, thread_info *tp)
 
   /* Since the current thread may have changed, see if there is any
      exited thread we can now delete.  */
-  prune_threads ();
+  delete_exited_threads ();
 }
 
 /* Print thread and frame switch command response.  */
-- 
2.14.5

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

[Aktemur, Tankut Baris]

On Wednesday, May 13, 2020 10:54 PM, Pedro Alves wrote:
> 
> When a process exits and we leave the process exit event pending, we
> need to make sure that at least one thread is left listed in the
> inferior's thread list.  This is necessary in order to make sure we
> have a thread that we can later resume, so the process exit event can
> be collected/reported.
> 
> When native debugging, the GNU/Linux back end already makes sure that
> the last LWP isn't deleted.
> 
> When remote debugging against GNU/Linux GDBserver, the GNU/Linux
> GDBserver backend also makes sure that the last thread isn't deleted
> until the process exit event is reported to GDBserver core.
> 
> However, between the backend reporting the process exit event to
> GDBserver core, and GDB consuming the event, GDB may update the thread
> list and find no thread left in the process.  The process exit event
> will be pending somewhere in GDBserver's stop reply queue, or
> gdb/remote.c's queue, or whathever other event queue inbetween
> GDBserver and infrun.c's handle_inferior_event.
> 
> This patch tweaks remote.c's target_update_thread_list implementation
> to avoid deleting the last thread of an inferior.
> 
> In the past, this case of inferior-with-no-threads lead to a special

"lead" -> "led"?

> case at the bottom of handle_no_resumed, where it reads:
> 
>   /* Note however that we may find no resumed thread because the whole
>      process exited meanwhile (thus updating the thread list results
>      in an empty thread list).  In this case we know we'll be getting
>      a process exit event shortly.  */
>   for (inferior *inf : all_non_exited_inferiors (ecs->target))
> 
> In current master, that code path is still reacheable with the

"reacheable" -> "reachable" 

> gdb.threads/continue-pending-after-query.exp testcase, when tested
> against GDBserver, with "maint set target-non-stop" forced "on".
> 
> With this patch, the scenario that loop was concerned about is still
> properly handled, because the loop above it finds the process's last
> thread with "executing" set to true, and thus the handle_no_resumed
> function still returns true.
> 
> Since GNU/Linux native and remote are the only targets that support
> non-stop mode, and with this patch, we always make sure the inferior
> has at least one thread, this patch also removes that "inferior with
> no threads" special case handling from handle_no_resumed.
> 
> Since remote.c now has a special case where we treat a thread that has
> already exited as if it was still alive, we might need to tweak
> remote.c's target_thread_alive implementation to return true for that
> thread without querying the remote side (which would say "no, not
> alive").  After inspecting all the target_thread_alive calls in the
> codebase, it seems that only the one from prune_threads could result
> in that thread being accidentaly deleted.  There's only one call to

"accidentaly" -> "accidentally"

> prune_threads in GDB's common code, so this patch handles this by
> replacing the prune_threads call with a delete_exited_threads call.
> This seems like an improvement anyway, because we'll still be doing
> what the comment suggests we want to do, and, we avoid remote protocol
> traffic.

The whole explanation above is very useful.  Thank you.

> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 6602bc28d5e..c9a092e4943 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -4791,7 +4791,11 @@ stop_all_threads (void)
>  	{
>  	  int need_wait = 0;
> 
> -	  update_thread_list ();
> +	  for (auto *target : all_non_exited_process_targets ())
> +	    {
> +	      switch_to_target_no_thread (target);
> +	      update_thread_list ();
> +	    }

I'm glad that the artificial thread addition/resurrection piece is gone
and the thread lists are now updated straightforwardly.  I'm also glad
that all_non_exited_process_targets found itself another use :).

> +		  /* If there is no available thread, the event would
> +		     have to be appended to a per-inferior event list,
> +		     which, does not exist (and if it did, we'd have
> +		     to adjust run control command to be able to
> +		     resume such an inferior).  We assert here instead
> +		     of going into an infinite loop.  */

Just a nit: no comma after "which".

> diff --git a/gdb/remote.c b/gdb/remote.c
> index 5db406e045c..8e12ba9603e 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -3785,6 +3785,18 @@ remote_target::remote_get_threads_with_qthreadinfo (threads_listing_context *con
>    return 0;
>  }
> 
> +/* Return true if INF only has one non-exited thread.  */
> +
> +static bool
> +single_non_exited_thread_p (inferior *inf)

Given that the return type is bool and not int, can the '_p' in the name be
discarded and a question phrase, like 'has_single_non_exited_thread', be
used?

> +{
> +  int count = 0;
> +  for (thread_info *tp ATTRIBUTE_UNUSED : inf->non_exited_threads ())
> +    if (++count > 1)
> +      break;
> +  return count == 1;
> +}
> +

> diff --git a/gdb/testsuite/gdb.multi/multi-exit.exp b/gdb/testsuite/gdb.multi/multi-exit.exp
> new file mode 100644
> index 00000000000..3c4e99164ed
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/multi-exit.exp
> @@ -0,0 +1,138 @@
> +# 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
> +
> +if {[use_gdb_stub]} {
> +    return 0
> +}
> +
> +if {[build_executable "failed to prepare" $testfile $srcfile]} {
> +    return -1
> +}
> +
> +# We are testing GDB's ability to stop all threads.
> +# Hence, go with the all-stop-on-top-of-non-stop mode.
> +save_vars { GDBFLAGS } {
> +    append GDBFLAGS " -ex \"maint set target-non-stop on\""
> +    clean_restart ${binfile}
> +}
> +
> +with_test_prefix "inf 1" {
> +    gdb_load $binfile
> +
> +    if {[gdb_start_cmd] < 0} {
> +	fail "could not start"
> +	return -1
> +    }
> +    gdb_test "" ".*reakpoint .*, main .*${srcfile}.*" "start"
> +}

Similar to what you did in multi-kill.exp, to eliminate code duplication,
this 'with_test_prefix' code block can be removed, and the start_inferior
procedure can be used for inferior 1, too, by ...

> +
> +# Start inferior NUM.
> +
> +proc start_inferior {num} {
> +    global srcfile binfile
> +
> +    gdb_test "add-inferior" "Added inferior $num.*" \
> +	"add empty inferior $num"
> +    gdb_test "inferior $num" "Switching to inferior $num.*" \
> +	"switch to inferior $num"

... guarding the statements above with 'if {$num != 1}'.  And then ...

> +
> +    with_test_prefix "inf $num" {
> +	gdb_load $binfile
> +
> +	if {[gdb_start_cmd] < 0} {
> +	    fail "could not start"
> +	    return -1
> +	}
> +	gdb_test "" ".*reakpoint .*, main .*${srcfile}.*" "start"
> +    }
> +
> +    return 0
> +}
> +
> +# Sufficient inferiors to make sure that at least some other inferior
> +# exits while we're handling a process exit event.
> +set NUM_INFS 10
> +
> +for {set i 2} {$i <= $NUM_INFS} {incr i} {

... this loop could start from 1.

> +    if {[start_inferior $i] < 0} {
> +	return -1
> +    }
> +}
> +
> +# We want to continue all processes.
> +gdb_test_no_output "set schedule-multiple on"
> +
> +# Check that "continue" continues to the end of an inferior, as many
> +# times as we have inferiors.
> +
> +for {set i 1} {$i <= $NUM_INFS} {incr i} {
> +    with_test_prefix "inf $i" {
> +	set live_inferior ""
> +
> +	# Pick any live inferior.
> +	gdb_test_multiple "info inferiors" "" {
> +	    -re "($decimal) *process.*$gdb_prompt $" {
> +		set live_inferior $expect_out(1,string)
> +	    }
> +	}
> +
> +	if {$live_inferior == ""} {
> +	    return -1
> +	}
> +
> +	gdb_test "inferior $live_inferior" \
> +	    ".*Switching to inferior $live_inferior.*" \
> +	    "switch to another inferior"
> +
> +	set exited_inferior ""
> +
> +	# We want GDB to complete the command and return the prompt
> +	# instead of going into an infinite loop.
> +	gdb_test_multiple "continue" "first continue" {

Just a nit: Now that there is no "second" continue, the "first" in the
test name can be discarded, I think.  How about just "continue"?

> +	    -re "Inferior ($decimal) \[^\n\r\]+ exited normally.*$gdb_prompt $" {
> +		set exited_inferior $expect_out(1,string)
> +		pass $gdb_test_name
> +	    }
> +	}
> +
> +	if {$exited_inferior == ""} {
> +	    return -1
> +	}
> +    }
> +}
> +
> +# Finally, check that we can re-run all inferiors.  Note that if any
> +# inferior was still alive this would catch it, as "run" would query
> +# "Start it from the beginning?".
> +
> +delete_breakpoints
> +
> +for {set i 1} {$i <= $NUM_INFS} {incr i} {
> +    with_test_prefix "inf $i" {

I think we need to switch to inferior $i here, otherwise we re-run
the latest inferior from the previous loop for ten times.

> +	gdb_test "run" "$inferior_exited_re normally\]" \
> +	    "re-run inferior"
> +    }
> +}

> diff --git a/gdb/testsuite/gdb.multi/multi-kill.exp b/gdb/testsuite/gdb.multi/multi-kill.exp
> new file mode 100644
> index 00000000000..7deaadc68e8
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/multi-kill.exp
> @@ -0,0 +1,127 @@
> +# 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
> +
> +if {[use_gdb_stub]} {
> +    return 0
> +}
> +
> +if {[build_executable "failed to prepare" $testfile $srcfile {debug}]} {
> +    return -1
> +}
> +
> +# We are testing GDB's ability to stop all threads.
> +# Hence, go with the all-stop-on-top-of-non-stop mode.
> +save_vars { GDBFLAGS } {
> +    append GDBFLAGS " -ex \"maint set target-non-stop on\""
> +    clean_restart ${binfile}
> +}
> +
> +# Start inferior NUM and record its PID in the TESTPID array.
> +
> +proc start_inferior {num} {
> +    with_test_prefix "start_inferior $num" {
> +	global testpid binfile srcfile
> +
> +	if {$num != 1} {
> +	    gdb_test "add-inferior" "Added inferior .*" \
> +		"add empty inferior"
> +	    gdb_test "inferior $num" "Switching to inferior .*" \
> +		"switch to inferior"
> +	}
> +
> +	gdb_load $binfile
> +
> +	gdb_breakpoint "initialized" {temporary}
> +	gdb_run_cmd
> +	gdb_test "" ".*reakpoint .*, initialized .*${srcfile}.*" "run"
> +
> +	set testpid($num) [get_integer_valueof "pid" -1]
> +	if {$testpid($num) == -1} {
> +	    return -1
> +	}
> +
> +	return 0
> +    }
> +}
> +
> +# Sufficient inferiors to make sure that at least some other inferior
> +# is killed while we're handling a killed event.
> +set NUM_INFS 10
> +
> +for {set i 1} {$i <= $NUM_INFS} {incr i} {
> +    if {[start_inferior $i] < 0} {
> +	return -1
> +    }
> +}
> +
> +# We want to continue all processes.
> +gdb_test_no_output "set schedule-multiple on"
> +
> +# Resume, but then kill all from outside.
> +gdb_test_multiple "continue" "continue processes" {
> +    -re "Continuing.\[\r\n\]+" {
> +	# Kill all processes at once.
> +
> +	set kill_cmd "kill -9"
> +	for {set i 1} {$i <= $NUM_INFS} {incr i} {
> +	    append kill_cmd " $testpid($i)"
> +	}
> +
> +	remote_exec target $kill_cmd
> +	exp_continue
> +    }
> +    -re "Program terminated with signal.*$gdb_prompt" {

I'm not sure if it really matters, but for consistency with the other
similar regexps here and multi-exit.exp, how about ending this regexp
with " $"?  That is:

       -re "Program terminated with signal.*$gdb_prompt $" {

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 v8 6/6] gdb/infrun: handle already-exited threads when attempting to stop

[Pedro Alves]

>>
>> +/* Return true if INF only has one non-exited thread.  */
>> +
>> +static bool
>> +single_non_exited_thread_p (inferior *inf)
> Given that the return type is bool and not int, can the '_p' in the name be
> discarded and a question phrase, like 'has_single_non_exited_thread', be
> used?

Using "_p" (for predicate) is a common idiom in GDB/GCC code,
but I don't mind.

> Similar to what you did in multi-kill.exp, to eliminate code duplication,
> this 'with_test_prefix' code block can be removed, and the start_inferior
> procedure can be used for inferior 1, too, by ...
> 
>> +
>> +# Start inferior NUM.
>> +
>> +proc start_inferior {num} {
>> +    global srcfile binfile
>> +
>> +    gdb_test "add-inferior" "Added inferior $num.*" \
>> +	"add empty inferior $num"
>> +    gdb_test "inferior $num" "Switching to inferior $num.*" \
>> +	"switch to inferior $num"
> ... guarding the statements above with 'if {$num != 1}'.  And then ...
> 
>> +
>> +    with_test_prefix "inf $num" {
>> +	gdb_load $binfile
>> +
>> +	if {[gdb_start_cmd] < 0} {
>> +	    fail "could not start"
>> +	    return -1
>> +	}
>> +	gdb_test "" ".*reakpoint .*, main .*${srcfile}.*" "start"
>> +    }
>> +
>> +    return 0
>> +}
>> +
>> +# Sufficient inferiors to make sure that at least some other inferior
>> +# exits while we're handling a process exit event.
>> +set NUM_INFS 10
>> +
>> +for {set i 2} {$i <= $NUM_INFS} {incr i} {
> ... this loop could start from 1.
> 

Indeed.  In your update, you just missed putting the add-inferior/inferior
commands under with_test_prefix as well (and then remove the inferior
number from the test message), like I had done on multi-kill.exp.  I.e.,:

diff --git a/gdb/testsuite/gdb.multi/multi-exit.exp b/gdb/testsuite/gdb.multi/multi-exit.exp
index 2e3ed39a268..393093b3784 100644
--- a/gdb/testsuite/gdb.multi/multi-exit.exp
+++ b/gdb/testsuite/gdb.multi/multi-exit.exp
@@ -42,16 +42,16 @@ save_vars { GDBFLAGS } {
 # Start inferior NUM.
 
 proc start_inferior {num} {
-    global srcfile binfile
-
-    if {$num != 1} {
-	gdb_test "add-inferior" "Added inferior $num.*" \
-	    "add empty inferior $num"
-	gdb_test "inferior $num" "Switching to inferior $num.*" \
-	    "switch to inferior $num"
-    }
+    with_test_prefix "start_inferior $num" {
+	global srcfile binfile
+
+	if {$num != 1} {
+	    gdb_test "add-inferior" "Added inferior $num.*" \
+		"add empty inferior"
+	    gdb_test "inferior $num" "Switching to inferior $num.*" \
+		"switch to inferior"
+	}
 
-    with_test_prefix "inf $num" {
 	gdb_load $binfile
 
 	if {[gdb_start_cmd] < 0} {

Thanks,
Pedro Alves

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

[Aktemur, Tankut Baris]

On Thursday, May 14, 2020 1:17 PM, Pedro Alves wrote:
> Indeed.  In your update, you just missed putting the add-inferior/inferior
> commands under with_test_prefix as well (and then remove the inferior
> number from the test message), like I had done on multi-kill.exp.  I.e.,:

Ah, yes.  Thanks for catching that; will fix.

-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 v8 6/6] gdb/infrun: handle already-exited threads when attempting to stop

[Tom de Vries]

On 13-05-2020 22:53, Pedro Alves via Gdb-patches wrote:
> +	set testpid($num) [get_integer_valueof "pid" -1]

I'm running into:
...
ERROR: tcl error sourcing
/data/gdb_versions/devel/src/gdb/testsuite/gdb.multi/multi-kill.exp.
ERROR: can't set "testpid(1)": variable isn't array
    while executing
"set testpid($num) [get_integer_valueof "pid" -1]"
    ("uplevel" body line 15)
    invoked from within
"uplevel 1 $body"
    invoked from within
"with_test_prefix "start_inferior $num" {
        global testpid binfile srcfile

        if {$num != 1} {
            gdb_test "add-inferior" "Added inferior .*"  "add emp..."
    (procedure "start_inferior" line 2)
    invoked from within
"start_inferior $i"
    (file
"/data/gdb_versions/devel/src/gdb/testsuite/gdb.multi/multi-kill.exp"
line 75)
    invoked from within
"source /data/gdb_versions/devel/src/gdb/testsuite/gdb.multi/multi-kill.exp"
    ("uplevel" body line 1)
    invoked from within
"uplevel #0 source
/data/gdb_versions/devel/src/gdb/testsuite/gdb.multi/multi-kill.exp"
    invoked from within
...

Thanks,
- Tom

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

[Aktemur, Tankut Baris]

On Thursday, May 14, 2020 8:00 PM, Tom de Vries wrote:
> On 13-05-2020 22:53, Pedro Alves via Gdb-patches wrote:
> > +	set testpid($num) [get_integer_valueof "pid" -1]
> 
> I'm running into:
> ...
> ERROR: tcl error sourcing
> /data/gdb_versions/devel/src/gdb/testsuite/gdb.multi/multi-kill.exp.
> ERROR: can't set "testpid(1)": variable isn't array
>     while executing
> "set testpid($num) [get_integer_valueof "pid" -1]"

I'm not able to reproduce this problem.  The test runs fine on my system
with 58 expected passes and no failures.  Could it be related to the tcl
version?

As a rough guess, could you try the patch below to initialize the array
variable?

diff --git a/gdb/testsuite/gdb.multi/multi-kill.exp b/gdb/testsuite/gdb.multi/multi-kill.exp
index ce6075045fc..a21f8a78b0d 100644
--- a/gdb/testsuite/gdb.multi/multi-kill.exp
+++ b/gdb/testsuite/gdb.multi/multi-kill.exp
@@ -70,6 +70,7 @@ proc start_inferior {num} {
 # Sufficient inferiors to make sure that at least some other inferior
 # is killed while we're handling a killed event.
 set NUM_INFS 10
+array set testpid {}

 for {set i 1} {$i <= $NUM_INFS} {incr i} {
     if {[start_inferior $i] < 0} {


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 v8 6/6] gdb/infrun: handle already-exited threads when attempting to stop

[Pedro Alves]

On 5/14/20 7:54 PM, Aktemur, Tankut Baris wrote:
> On Thursday, May 14, 2020 8:00 PM, Tom de Vries wrote:
>> On 13-05-2020 22:53, Pedro Alves via Gdb-patches wrote:
>>> +	set testpid($num) [get_integer_valueof "pid" -1]
>>
>> I'm running into:
>> ...
>> ERROR: tcl error sourcing
>> /data/gdb_versions/devel/src/gdb/testsuite/gdb.multi/multi-kill.exp.
>> ERROR: can't set "testpid(1)": variable isn't array
>>     while executing
>> "set testpid($num) [get_integer_valueof "pid" -1]"
> 
> I'm not able to reproduce this problem.  The test runs fine on my system
> with 58 expected passes and no failures.  Could it be related to the tcl
> version?

It's related to testcase run order, and the fact that global variables
can leak between testcases.

> 
> As a rough guess, could you try the patch below to initialize the array
> variable?
> 
> diff --git a/gdb/testsuite/gdb.multi/multi-kill.exp b/gdb/testsuite/gdb.multi/multi-kill.exp
> index ce6075045fc..a21f8a78b0d 100644
> --- a/gdb/testsuite/gdb.multi/multi-kill.exp
> +++ b/gdb/testsuite/gdb.multi/multi-kill.exp
> @@ -70,6 +70,7 @@ proc start_inferior {num} {
>  # Sufficient inferiors to make sure that at least some other inferior
>  # is killed while we're handling a killed event.
>  set NUM_INFS 10
> +array set testpid {}

That's not sufficient, because the variable can leak to other tests.
For example:

 $ runtest gdb.threads/check-libthread-db.exp gdb.multi/multi-kill.exp

 Running /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.multi/multi-kill.exp ...
 Running /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.threads/check-libthread-db.exp ...
 ERROR: tcl error sourcing /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.threads/check-libthread-db.exp.
 ERROR: can't set "testpid": variable is array
     while executing
 "set testpid [spawn_id_get_pid $test_spawn_id]"
     ("uplevel" body line 8)

This works for me.

From 96aa00638e9a1201874e316ad53c38614da8cc04 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 14 May 2020 19:07:35 +0100
Subject: [PATCH] Fix global variable collision in gdb.multi/multi-kill.exp

gdb/testsuite/ChangeLog:
2020-05-14  Pedro Alves  <palves@redhat.com>

	* gdb.multi/multi-kill.exp: Wrap in namespace.
	(start_inferior): Add TESTPID parameter.  Use it instead of the
	testpid global.
	(top level): Define empty TESTPID array, and pass it down to
	start_inferior.
---
 gdb/testsuite/gdb.multi/multi-kill.exp | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/gdb/testsuite/gdb.multi/multi-kill.exp b/gdb/testsuite/gdb.multi/multi-kill.exp
index ce6075045fc..03bf8449cf8 100644
--- a/gdb/testsuite/gdb.multi/multi-kill.exp
+++ b/gdb/testsuite/gdb.multi/multi-kill.exp
@@ -39,11 +39,15 @@ save_vars { GDBFLAGS } {
     clean_restart ${binfile}
 }
 
+# Wrap the entire test in a namespace to avoid contaminating other tests.
+namespace eval $testfile {
+
 # Start inferior NUM and record its PID in the TESTPID array.
 
-proc start_inferior {num} {
+proc start_inferior {num testpid} {
     with_test_prefix "start_inferior $num" {
-	global testpid binfile srcfile
+	upvar $testpid tpid
+	global binfile srcfile
 
 	if {$num != 1} {
 	    gdb_test "add-inferior" "Added inferior .*" \
@@ -58,8 +62,8 @@ proc start_inferior {num} {
 	gdb_run_cmd
 	gdb_test "" ".*reakpoint .*, initialized .*${srcfile}.*" "run"
 
-	set testpid($num) [get_integer_valueof "pid" -1]
-	if {$testpid($num) == -1} {
+	set tpid($num) [get_integer_valueof "pid" -1]
+	if {$tpid($num) == -1} {
 	    return -1
 	}
 
@@ -71,8 +75,11 @@ proc start_inferior {num} {
 # is killed while we're handling a killed event.
 set NUM_INFS 10
 
+# The array holding each inferior's PID, indexed by inferior number.
+array set testpid {}
+
 for {set i 1} {$i <= $NUM_INFS} {incr i} {
-    if {[start_inferior $i] < 0} {
+    if {[start_inferior $i testpid] < 0} {
 	return -1
     }
 }
@@ -125,3 +132,5 @@ for {set i 2} {$i <= $NUM_INFS} {incr i} {
 	    "continue to SIGKILL"
     }
 }
+
+}

base-commit: a05575d39a5348bd9979fc09e658a03ff22722b9
-- 
2.14.5

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

[Aktemur, Tankut Baris]

On Thursday, May 14, 2020 8:59 PM, Pedro Alves wrote:
> On 5/14/20 7:54 PM, Aktemur, Tankut Baris wrote:
> > On Thursday, May 14, 2020 8:00 PM, Tom de Vries wrote:
> >> On 13-05-2020 22:53, Pedro Alves via Gdb-patches wrote:
> >>> +	set testpid($num) [get_integer_valueof "pid" -1]
> >>
> >> I'm running into:
> >> ...
> >> ERROR: tcl error sourcing
> >> /data/gdb_versions/devel/src/gdb/testsuite/gdb.multi/multi-kill.exp.
> >> ERROR: can't set "testpid(1)": variable isn't array
> >>     while executing
> >> "set testpid($num) [get_integer_valueof "pid" -1]"
> >
> > I'm not able to reproduce this problem.  The test runs fine on my system
> > with 58 expected passes and no failures.  Could it be related to the tcl
> > version?
> 
> It's related to testcase run order, and the fact that global variables
> can leak between testcases.
> 
> >
> > As a rough guess, could you try the patch below to initialize the array
> > variable?
> >
> > diff --git a/gdb/testsuite/gdb.multi/multi-kill.exp b/gdb/testsuite/gdb.multi/multi-kill.exp
> > index ce6075045fc..a21f8a78b0d 100644
> > --- a/gdb/testsuite/gdb.multi/multi-kill.exp
> > +++ b/gdb/testsuite/gdb.multi/multi-kill.exp
> > @@ -70,6 +70,7 @@ proc start_inferior {num} {
> >  # Sufficient inferiors to make sure that at least some other inferior
> >  # is killed while we're handling a killed event.
> >  set NUM_INFS 10
> > +array set testpid {}
> 
> That's not sufficient, because the variable can leak to other tests.
> For example:
> 
>  $ runtest gdb.threads/check-libthread-db.exp gdb.multi/multi-kill.exp
> 
>  Running /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.multi/multi-kill.exp ...
>  Running /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.threads/check-libthread-db.exp ...
>  ERROR: tcl error sourcing /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.threads/check-libthread-db.exp.
>  ERROR: can't set "testpid": variable is array
>      while executing
>  "set testpid [spawn_id_get_pid $test_spawn_id]"
>      ("uplevel" body line 8)
> 
> This works for me.
> 
> From 96aa00638e9a1201874e316ad53c38614da8cc04 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Thu, 14 May 2020 19:07:35 +0100
> Subject: [PATCH] Fix global variable collision in gdb.multi/multi-kill.exp
> 
> gdb/testsuite/ChangeLog:
> 2020-05-14  Pedro Alves  <palves@redhat.com>
> 
> 	* gdb.multi/multi-kill.exp: Wrap in namespace.
> 	(start_inferior): Add TESTPID parameter.  Use it instead of the
> 	testpid global.
> 	(top level): Define empty TESTPID array, and pass it down to
> 	start_inferior.
> ---
>  gdb/testsuite/gdb.multi/multi-kill.exp | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.multi/multi-kill.exp b/gdb/testsuite/gdb.multi/multi-kill.exp
> index ce6075045fc..03bf8449cf8 100644
> --- a/gdb/testsuite/gdb.multi/multi-kill.exp
> +++ b/gdb/testsuite/gdb.multi/multi-kill.exp
> @@ -39,11 +39,15 @@ save_vars { GDBFLAGS } {
>      clean_restart ${binfile}
>  }
> 
> +# Wrap the entire test in a namespace to avoid contaminating other tests.
> +namespace eval $testfile {

TIL.  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 v8 6/6] gdb/infrun: handle already-exited threads when attempting to stop

[Pedro Alves]

On 5/15/20 8:53 AM, Aktemur, Tankut Baris wrote:
> On Thursday, May 14, 2020 8:59 PM, Pedro Alves wrote:
>>
>> +# Wrap the entire test in a namespace to avoid contaminating other tests.
>> +namespace eval $testfile {
> 
> TIL.  Thanks.

I wrote a commit log, and merged it now, as below.

From 272c36b87f81fd64e5f4669730da72c39d0716b3 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 15 May 2020 11:09:51 +0100
Subject: [PATCH] Fix global variable collision in gdb.multi/multi-kill.exp

The new gdb.multi/multi-kill.exp testcase added an 'testpid' array,
which may conflict with other global 'testpid' variables used by other
testcases, resulting in:

 ...
 ERROR: tcl error sourcing
 /data/gdb_versions/devel/src/gdb/testsuite/gdb.multi/multi-kill.exp.
 ERROR: can't set "testpid(1)": variable isn't array
     while executing
 "set testpid($num) [get_integer_valueof "pid" -1]"

or

 $ runtest gdb.threads/check-libthread-db.exp gdb.multi/multi-kill.exp
 ...
 Running /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.multi/multi-kill.exp ...
 Running /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.threads/check-libthread-db.exp ...
 ERROR: tcl error sourcing /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.threads/check-libthread-db.exp.
 ERROR: can't set "testpid": variable is array
     while executing
 "set testpid [spawn_id_get_pid $test_spawn_id]"
     ("uplevel" body line 8)

Fix this with a namespace, like gdb.linespec/explicit.exp does.

gdb/testsuite/ChangeLog:
2020-05-15  Pedro Alves  <palves@redhat.com>

	* gdb.multi/multi-kill.exp: Wrap in namespace.
	(start_inferior): Add TESTPID parameter.  Use it instead of the
	testpid global.
	(top level): Define empty TESTPID array, and pass it down to
	start_inferior.
---
 gdb/testsuite/ChangeLog                |  8 ++++++++
 gdb/testsuite/gdb.multi/multi-kill.exp | 19 ++++++++++++++-----
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 6c025832e3b..93ad65b32a2 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,11 @@
+2020-05-15  Pedro Alves  <palves@redhat.com>
+
+	* gdb.multi/multi-kill.exp: Wrap in namespace.
+	(start_inferior): Add TESTPID parameter.  Use it instead of the
+	testpid global.
+	(top level): Define empty TESTPID array, and pass it down to
+	start_inferior.
+
 2020-05-14  Tom de Vries  <tdevries@suse.de>
 
 	* gdb.fortran/nested-funcs-2.exp: Use gdb_test_stdio to test inferior
diff --git a/gdb/testsuite/gdb.multi/multi-kill.exp b/gdb/testsuite/gdb.multi/multi-kill.exp
index ce6075045fc..03bf8449cf8 100644
--- a/gdb/testsuite/gdb.multi/multi-kill.exp
+++ b/gdb/testsuite/gdb.multi/multi-kill.exp
@@ -39,11 +39,15 @@ save_vars { GDBFLAGS } {
     clean_restart ${binfile}
 }
 
+# Wrap the entire test in a namespace to avoid contaminating other tests.
+namespace eval $testfile {
+
 # Start inferior NUM and record its PID in the TESTPID array.
 
-proc start_inferior {num} {
+proc start_inferior {num testpid} {
     with_test_prefix "start_inferior $num" {
-	global testpid binfile srcfile
+	upvar $testpid tpid
+	global binfile srcfile
 
 	if {$num != 1} {
 	    gdb_test "add-inferior" "Added inferior .*" \
@@ -58,8 +62,8 @@ proc start_inferior {num} {
 	gdb_run_cmd
 	gdb_test "" ".*reakpoint .*, initialized .*${srcfile}.*" "run"
 
-	set testpid($num) [get_integer_valueof "pid" -1]
-	if {$testpid($num) == -1} {
+	set tpid($num) [get_integer_valueof "pid" -1]
+	if {$tpid($num) == -1} {
 	    return -1
 	}
 
@@ -71,8 +75,11 @@ proc start_inferior {num} {
 # is killed while we're handling a killed event.
 set NUM_INFS 10
 
+# The array holding each inferior's PID, indexed by inferior number.
+array set testpid {}
+
 for {set i 1} {$i <= $NUM_INFS} {incr i} {
-    if {[start_inferior $i] < 0} {
+    if {[start_inferior $i testpid] < 0} {
 	return -1
     }
 }
@@ -125,3 +132,5 @@ for {set i 2} {$i <= $NUM_INFS} {incr i} {
 	    "continue to SIGKILL"
     }
 }
+
+}

base-commit: 013707794a67269dd34fd8ae6e354e982c547dc0
-- 
2.14.5

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

[Tom de Vries]

On 14-05-2020 20:58, Pedro Alves wrote:
> On 5/14/20 7:54 PM, Aktemur, Tankut Baris wrote:
>> On Thursday, May 14, 2020 8:00 PM, Tom de Vries wrote:
>>> On 13-05-2020 22:53, Pedro Alves via Gdb-patches wrote:
>>>> +	set testpid($num) [get_integer_valueof "pid" -1]
>>>
>>> I'm running into:
>>> ...
>>> ERROR: tcl error sourcing
>>> /data/gdb_versions/devel/src/gdb/testsuite/gdb.multi/multi-kill.exp.
>>> ERROR: can't set "testpid(1)": variable isn't array
>>>     while executing
>>> "set testpid($num) [get_integer_valueof "pid" -1]"
>>
>> I'm not able to reproduce this problem.  The test runs fine on my system
>> with 58 expected passes and no failures.  Could it be related to the tcl
>> version?
> 
> It's related to testcase run order, and the fact that global variables
> can leak between testcases.
> 
>>
>> As a rough guess, could you try the patch below to initialize the array
>> variable?
>>
>> diff --git a/gdb/testsuite/gdb.multi/multi-kill.exp b/gdb/testsuite/gdb.multi/multi-kill.exp
>> index ce6075045fc..a21f8a78b0d 100644
>> --- a/gdb/testsuite/gdb.multi/multi-kill.exp
>> +++ b/gdb/testsuite/gdb.multi/multi-kill.exp
>> @@ -70,6 +70,7 @@ proc start_inferior {num} {
>>  # Sufficient inferiors to make sure that at least some other inferior
>>  # is killed while we're handling a killed event.
>>  set NUM_INFS 10
>> +array set testpid {}
> 
> That's not sufficient, because the variable can leak to other tests.
> For example:
> 
>  $ runtest gdb.threads/check-libthread-db.exp gdb.multi/multi-kill.exp
> 
>  Running /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.multi/multi-kill.exp ...
>  Running /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.threads/check-libthread-db.exp ...
>  ERROR: tcl error sourcing /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.threads/check-libthread-db.exp.
>  ERROR: can't set "testpid": variable is array
>      while executing
>  "set testpid [spawn_id_get_pid $test_spawn_id]"
>      ("uplevel" body line 8)
> 
> This works for me.
> 
> From 96aa00638e9a1201874e316ad53c38614da8cc04 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Thu, 14 May 2020 19:07:35 +0100
> Subject: [PATCH] Fix global variable collision in gdb.multi/multi-kill.exp
> 
> gdb/testsuite/ChangeLog:
> 2020-05-14  Pedro Alves  <palves@redhat.com>
> 
> 	* gdb.multi/multi-kill.exp: Wrap in namespace.
> 	(start_inferior): Add TESTPID parameter.  Use it instead of the
> 	testpid global.
> 	(top level): Define empty TESTPID array, and pass it down to
> 	start_inferior.
> ---
>  gdb/testsuite/gdb.multi/multi-kill.exp | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.multi/multi-kill.exp b/gdb/testsuite/gdb.multi/multi-kill.exp
> index ce6075045fc..03bf8449cf8 100644
> --- a/gdb/testsuite/gdb.multi/multi-kill.exp
> +++ b/gdb/testsuite/gdb.multi/multi-kill.exp
> @@ -39,11 +39,15 @@ save_vars { GDBFLAGS } {
>      clean_restart ${binfile}
>  }
>  
> +# Wrap the entire test in a namespace to avoid contaminating other tests.
> +namespace eval $testfile {
> +
>  # Start inferior NUM and record its PID in the TESTPID array.
>  
> -proc start_inferior {num} {
> +proc start_inferior {num testpid} {
>      with_test_prefix "start_inferior $num" {
> -	global testpid binfile srcfile
> +	upvar $testpid tpid
> +	global binfile srcfile
>  
>  	if {$num != 1} {
>  	    gdb_test "add-inferior" "Added inferior .*" \
> @@ -58,8 +62,8 @@ proc start_inferior {num} {
>  	gdb_run_cmd
>  	gdb_test "" ".*reakpoint .*, initialized .*${srcfile}.*" "run"
>  
> -	set testpid($num) [get_integer_valueof "pid" -1]
> -	if {$testpid($num) == -1} {
> +	set tpid($num) [get_integer_valueof "pid" -1]
> +	if {$tpid($num) == -1} {
>  	    return -1
>  	}
>  
> @@ -71,8 +75,11 @@ proc start_inferior {num} {
>  # is killed while we're handling a killed event.
>  set NUM_INFS 10
>  
> +# The array holding each inferior's PID, indexed by inferior number.
> +array set testpid {}
> +

Actually, we still need to declare the variable as a namespace variable
using:
...
variable testpid
array set testpid {}
...
otherwise we run into:
...
ERROR: tcl error sourcing
/data/gdb_versions/devel/src/gdb/testsuite/gdb.multi/multi-kill.exp.
ERROR: can't array set "testpid": variable isn't array
    while executing
"array set testpid {}"
    (in namespace eval "::multi-kill" script line 36)
    invoked from within
"namespace eval $testfile {
...

Thanks,
- Tom

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

[Pedro Alves]

On 5/15/20 11:17 AM, Tom de Vries wrote:
> On 14-05-2020 20:58, Pedro Alves wrote:
> Actually, we still need to declare the variable as a namespace variable
> using:
> ...
> variable testpid
> array set testpid {}
> ...
> otherwise we run into:
> ...
> ERROR: tcl error sourcing
> /data/gdb_versions/devel/src/gdb/testsuite/gdb.multi/multi-kill.exp.
> ERROR: can't array set "testpid": variable isn't array
>     while executing
> "array set testpid {}"
>     (in namespace eval "::multi-kill" script line 36)
>     invoked from within
> "namespace eval $testfile {
> ...

Whoops, sorry, and thanks.  I had already merged the patch, so
I went ahead and applied that fix, as below.

From 3c5c3649729ba3f94f9d28f10aa2270c8c7e4aa9 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 15 May 2020 11:22:47 +0100
Subject: [PATCH] Fix gdb.multi/multi-kill.exp

The previous patch misssed declaring the 'testpid' array as namespace
variable.  While it at, might as well go back to having start_inferior
refer to the "global" testpid, using "variable" too.

gdb/testsuite/ChangeLog:
2020-05-15  Pedro Alves  <palves@redhat.com>

	* gdb.multi/multi-kill.exp (start_inferior): Remove
	'testpid' parameter.  Refer to namespace variable directly.
	(testpid): Declare as namespace variable.
---
 gdb/testsuite/ChangeLog                |  6 ++++++
 gdb/testsuite/gdb.multi/multi-kill.exp | 11 ++++++-----
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 93ad65b32a2..b35d10e01bd 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2020-05-15  Pedro Alves  <palves@redhat.com>
+
+	* gdb.multi/multi-kill.exp (start_inferior): Remove
+	'testpid' parameter.  Refer to namespace variable directly.
+	(testpid): Declare as namespace variable.
+
 2020-05-15  Pedro Alves  <palves@redhat.com>
 
 	* gdb.multi/multi-kill.exp: Wrap in namespace.
diff --git a/gdb/testsuite/gdb.multi/multi-kill.exp b/gdb/testsuite/gdb.multi/multi-kill.exp
index 03bf8449cf8..b4853a1ea40 100644
--- a/gdb/testsuite/gdb.multi/multi-kill.exp
+++ b/gdb/testsuite/gdb.multi/multi-kill.exp
@@ -44,9 +44,9 @@ namespace eval $testfile {
 
 # Start inferior NUM and record its PID in the TESTPID array.
 
-proc start_inferior {num testpid} {
+proc start_inferior {num} {
     with_test_prefix "start_inferior $num" {
-	upvar $testpid tpid
+	variable testpid
 	global binfile srcfile
 
 	if {$num != 1} {
@@ -62,8 +62,8 @@ proc start_inferior {num testpid} {
 	gdb_run_cmd
 	gdb_test "" ".*reakpoint .*, initialized .*${srcfile}.*" "run"
 
-	set tpid($num) [get_integer_valueof "pid" -1]
-	if {$tpid($num) == -1} {
+	set testpid($num) [get_integer_valueof "pid" -1]
+	if {$testpid($num) == -1} {
 	    return -1
 	}
 
@@ -76,10 +76,11 @@ proc start_inferior {num testpid} {
 set NUM_INFS 10
 
 # The array holding each inferior's PID, indexed by inferior number.
+variable testpid
 array set testpid {}
 
 for {set i 1} {$i <= $NUM_INFS} {incr i} {
-    if {[start_inferior $i testpid] < 0} {
+    if {[start_inferior $i] < 0} {
 	return -1
     }
 }

base-commit: 272c36b87f81fd64e5f4669730da72c39d0716b3
-- 
2.14.5

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

[Tom de Vries]

On 14-05-2020 20:58, Pedro Alves wrote:
> On 5/14/20 7:54 PM, Aktemur, Tankut Baris wrote:
>> On Thursday, May 14, 2020 8:00 PM, Tom de Vries wrote:
>>> On 13-05-2020 22:53, Pedro Alves via Gdb-patches wrote:
>>>> +	set testpid($num) [get_integer_valueof "pid" -1]
>>>
>>> I'm running into:
>>> ...
>>> ERROR: tcl error sourcing
>>> /data/gdb_versions/devel/src/gdb/testsuite/gdb.multi/multi-kill.exp.
>>> ERROR: can't set "testpid(1)": variable isn't array
>>>     while executing
>>> "set testpid($num) [get_integer_valueof "pid" -1]"
>>
>> I'm not able to reproduce this problem.  The test runs fine on my system
>> with 58 expected passes and no failures.  Could it be related to the tcl
>> version?
> 
> It's related to testcase run order, and the fact that global variables
> can leak between testcases.
> 
>>
>> As a rough guess, could you try the patch below to initialize the array
>> variable?
>>
>> diff --git a/gdb/testsuite/gdb.multi/multi-kill.exp b/gdb/testsuite/gdb.multi/multi-kill.exp
>> index ce6075045fc..a21f8a78b0d 100644
>> --- a/gdb/testsuite/gdb.multi/multi-kill.exp
>> +++ b/gdb/testsuite/gdb.multi/multi-kill.exp
>> @@ -70,6 +70,7 @@ proc start_inferior {num} {
>>  # Sufficient inferiors to make sure that at least some other inferior
>>  # is killed while we're handling a killed event.
>>  set NUM_INFS 10
>> +array set testpid {}
> 
> That's not sufficient, because the variable can leak to other tests.
> For example:
> 
>  $ runtest gdb.threads/check-libthread-db.exp gdb.multi/multi-kill.exp
> 
>  Running /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.multi/multi-kill.exp ...
>  Running /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.threads/check-libthread-db.exp ...
>  ERROR: tcl error sourcing /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.threads/check-libthread-db.exp.
>  ERROR: can't set "testpid": variable is array
>      while executing
>  "set testpid [spawn_id_get_pid $test_spawn_id]"
>      ("uplevel" body line 8)

It occurred to me that we can find leaked global arrays using a
runtest.exp patch. I've filed a testsuite cleanup PR ( PR25996 - "wrap
global arrays in namespace",
https://sourceware.org/bugzilla/show_bug.cgi?id=25996 ).

Thanks,
- Tom

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

[Pedro Alves]

On 5/15/20 12:53 PM, Tom de Vries wrote:

> It occurred to me that we can find leaked global arrays using a
> runtest.exp patch. I've filed a testsuite cleanup PR ( PR25996 - "wrap
> global arrays in namespace",
> https://sourceware.org/bugzilla/show_bug.cgi?id=25996 ).

Wouldn't it work to do this in gdb_init / gdb_finish ?

Pedro Alves

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

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1019 bytes --]

On 15-05-2020 14:02, Pedro Alves wrote:
> On 5/15/20 12:53 PM, Tom de Vries wrote:
> 
>> It occurred to me that we can find leaked global arrays using a
>> runtest.exp patch. I've filed a testsuite cleanup PR ( PR25996 - "wrap
>> global arrays in namespace",
>> https://sourceware.org/bugzilla/show_bug.cgi?id=25996 ).
> 
> Wouldn't it work to do this in gdb_init / gdb_finish ?
> 

Indeed, thanks for the pointer.

With attached patch, we have:
...
Running src/gdb/testsuite/gdb.ada/info_auto_lang.exp ...
WARNING: info_auto_lang.exp defined global array type_in_c
WARNING: info_auto_lang.exp defined global array rbreak_func_in_ada
WARNING: info_auto_lang.exp defined global array var_in_c
WARNING: info_auto_lang.exp defined global array func_in_c
WARNING: info_auto_lang.exp defined global array func_in_ada
WARNING: info_auto_lang.exp defined global array type_in_ada
WARNING: info_auto_lang.exp defined global array var_in_ada
WARNING: info_auto_lang.exp defined global array rbreak_func_in_c
...

Thanks,
- Tom


[-- Attachment #2: 0001-gdb-testsuite-Warn-about-leaked-global-array.patch --]
[-- Type: text/x-patch, Size: 2154 bytes --]

[gdb/testsuite] Warn about leaked global array

---
 gdb/testsuite/lib/gdb.exp | 54 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index f7d20bd94f..91491a1f0f 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5048,6 +5048,58 @@ proc standard_testfile {args} {
     }
 }
 
+# Returns 1 if __VAR is a global array.
+
+proc global_array_exists { __var } {
+    global $__var
+    return [array exists $__var]
+}
+
+# Unset global variable __VAR.
+
+proc global_unset { __var } {
+    global $__var
+    unset $__var
+}
+
+# Save global vars to variable gdb_global_vars.
+
+proc save_global_vars { test_file_name } {
+    global gdb_test_file_name
+    set gdb_test_file_name $test_file_name
+
+    global gdb_global_vars
+    set gdb_global_vars [list]
+
+    set gdb_global_vars [info globals]
+}
+
+# Check global variables not in gdb_global_vars.
+
+proc check_global_vars { } {
+    global gdb_global_vars
+    set vars [info globals]
+    set skip [list "expect_out" "spawn_out"]
+    foreach var $vars {
+        set found [lsearch -exact $gdb_global_vars $var]
+        if { $found != -1 } {
+            # Already present
+            continue
+        }
+        set found [lsearch -exact $skip $var]
+        if { $found != -1 } {
+            continue
+        }
+        if { ![global_array_exists $var] } {
+	    continue
+        }
+
+	global gdb_test_file_name
+        warning "$gdb_test_file_name.exp defined global array $var"
+        global_unset $var
+    }
+}
+
 # The default timeout used when testing GDB commands.  We want to use
 # the same timeout as the default dejagnu timeout, unless the user has
 # already provided a specific value (probably through a site.exp file).
@@ -5177,10 +5229,12 @@ proc gdb_init { test_file_name } {
     global gdb_instances
     set gdb_instances 0
 
+    save_global_vars $test_file_name
     return [default_gdb_init $test_file_name]
 }
 
 proc gdb_finish { } {
+    check_global_vars
     global gdbserver_reconnect_p
     global gdb_prompt
     global cleanfiles

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

[Pedro Alves]

On 5/15/20 3:16 PM, Tom de Vries wrote:

> With attached patch, we have:
> ...
> Running src/gdb/testsuite/gdb.ada/info_auto_lang.exp ...
> WARNING: info_auto_lang.exp defined global array type_in_c
> WARNING: info_auto_lang.exp defined global array rbreak_func_in_ada
> WARNING: info_auto_lang.exp defined global array var_in_c
> WARNING: info_auto_lang.exp defined global array func_in_c
> WARNING: info_auto_lang.exp defined global array func_in_ada
> WARNING: info_auto_lang.exp defined global array type_in_ada
> WARNING: info_auto_lang.exp defined global array var_in_ada
> WARNING: info_auto_lang.exp defined global array rbreak_func_in_c
> ...

Seems like a good thing to have to me.

>  
> +# Returns 1 if __VAR is a global array.
> +
> +proc global_array_exists { __var } {

...

> +proc global_unset { __var } {

Curious -- is there a reason for the underscores?

> +proc check_global_vars { } {
> +    global gdb_global_vars
> +    set vars [info globals]
> +    set skip [list "expect_out" "spawn_out"]
> +    foreach var $vars {
> +        set found [lsearch -exact $gdb_global_vars $var]
> +        if { $found != -1 } {
> +            # Already present

Missing period.

> +            continue
> +        }
> +        set found [lsearch -exact $skip $var]
> +        if { $found != -1 } {
> +            continue
> +        }
> +        if { ![global_array_exists $var] } {
> +	    continue
> +        }
> +
> +	global gdb_test_file_name
> +        warning "$gdb_test_file_name.exp defined global array $var"

Tabs vs spaces.

Thanks,
Pedro Alves

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

[Tom de Vries]

On 15-05-2020 17:46, Pedro Alves wrote:
> On 5/15/20 3:16 PM, Tom de Vries wrote:
> 
>> With attached patch, we have:
>> ...
>> Running src/gdb/testsuite/gdb.ada/info_auto_lang.exp ...
>> WARNING: info_auto_lang.exp defined global array type_in_c
>> WARNING: info_auto_lang.exp defined global array rbreak_func_in_ada
>> WARNING: info_auto_lang.exp defined global array var_in_c
>> WARNING: info_auto_lang.exp defined global array func_in_c
>> WARNING: info_auto_lang.exp defined global array func_in_ada
>> WARNING: info_auto_lang.exp defined global array type_in_ada
>> WARNING: info_auto_lang.exp defined global array var_in_ada
>> WARNING: info_auto_lang.exp defined global array rbreak_func_in_c
>> ...
> 
> Seems like a good thing to have to me.
> 

Great.  Probably it's a good idea to fix the known occurrences before
enabling this by default.

>>  
>> +# Returns 1 if __VAR is a global array.
>> +
>> +proc global_array_exists { __var } {
> 
> ...
> 
>> +proc global_unset { __var } {
> 
> Curious -- is there a reason for the underscores?
> 

Yeah, I'm trying to avoid this:
...
$ cat test.tcl
#!/usr/bin/tclsh

proc foo { var } {
    global $var
}

foo var
$ ./test.tcl
variable "var" already exists
    while executing
"global $var"
    (procedure "foo" line 2)
    invoked from within
"foo var"
    (file "./test.tcl" line 7)
...

Thanks,
- Tom

>> +proc check_global_vars { } {
>> +    global gdb_global_vars
>> +    set vars [info globals]
>> +    set skip [list "expect_out" "spawn_out"]
>> +    foreach var $vars {
>> +        set found [lsearch -exact $gdb_global_vars $var]
>> +        if { $found != -1 } {
>> +            # Already present
> 
> Missing period.
> 
>> +            continue
>> +        }
>> +        set found [lsearch -exact $skip $var]
>> +        if { $found != -1 } {
>> +            continue
>> +        }
>> +        if { ![global_array_exists $var] } {
>> +	    continue
>> +        }
>> +
>> +	global gdb_test_file_name
>> +        warning "$gdb_test_file_name.exp defined global array $var"
> 
> Tabs vs spaces.
> 
> Thanks,
> Pedro Alves
> 

[PATCH][gdb/testsuite] Warn about leaked global array

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 697 bytes --]

[ was: Re: [PATCH v8 6/6] gdb/infrun: handle already-exited threads when
attempting to stop ]

On 15-05-2020 19:17, Tom de Vries wrote:
> On 15-05-2020 17:46, Pedro Alves wrote:
>> Curious -- is there a reason for the underscores?
>>
> 
> Yeah, I'm trying to avoid this:
> ...
> $ cat test.tcl
> #!/usr/bin/tclsh
> 
> proc foo { var } {
>     global $var
> }
> 
> foo var
> $ ./test.tcl
> variable "var" already exists
>     while executing
> "global $var"
>     (procedure "foo" line 2)
>     invoked from within
> "foo var"
>     (file "./test.tcl" line 7)
> ...

I managed to find a better way of dealing with this, using "upvar #0".
I've also added more comments to the patch.

Thanks,
- Tom


[-- Attachment #2: 0001-gdb-testsuite-Warn-about-leaked-global-array.patch --]
[-- Type: text/x-patch, Size: 4585 bytes --]

[gdb/testsuite] Warn about leaked global array

A variable name cannot be used both as scalar and array without an
intermediate unset.  Trying to do so will result in tcl errors, for
example, for:
...
  set var "bla"
  set var(1) "bla"
...
we get:
...
  can't set "var(1)": variable isn't array
...
and for the reverse statement order we get:
...
  can't set "var": variable is array
...

So, since a global name in one test-case can leak to another
test-case, setting a global name in one test-case can result in
a tcl error in another test-case that reuses the name in a different
way.

Warn about leaking a global array from a test-case.

Tested on x86_64-linux.

gdb/testsuite/ChangeLog:

2020-05-18  Tom de Vries  <tdevries@suse.de>

	* lib/gdb.exp (global_array_exists, global_unset, save_global_vars)
	(check_global_vars): New proc.
	(gdb_init): Call save_global_vars.
	(gdb_finish): Call check_global_vars.

---
 gdb/testsuite/lib/gdb.exp | 91 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index f7d20bd94f..285736ee92 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5048,6 +5048,95 @@ proc standard_testfile {args} {
     }
 }
 
+# Returns 1 if GLOBALVARNAME is a global array.
+
+proc global_array_exists { globalvarname } {
+    # Introduce local alias of global variable $globalvarname.  Using
+    # "global $globalvarname" instead is simpler, but this may result in a
+    # clash with local name "globalvarname".
+    upvar #0 $globalvarname globalvar
+    return [array exists globalvar]
+}
+
+# Unset global variable GLOBALVARNAME.
+
+proc global_unset { globalvarname } {
+    # Introduce local alias of global variable $globalvarname.
+    upvar #0 $globalvarname globalvar
+    unset globalvar
+}
+
+# Save global vars to variable gdb_global_vars.
+
+proc save_global_vars { test_file_name } {
+    # Save for the warning.
+    global gdb_test_file_name
+    set gdb_test_file_name $test_file_name
+
+    # Sample state before running test.
+    global gdb_global_vars
+    set gdb_global_vars [info globals]
+}
+
+# Check global variables not in gdb_global_vars.
+
+proc check_global_vars { } {
+    # Sample state after running test.
+    global gdb_global_vars
+    set vars [info globals]
+
+    # I'm not sure these two should actually be global, but at least there
+    # seems to be no harm in having these as globals, given that we don't
+    # expect to reuse these names as scalars.
+    set skip [list "expect_out" "spawn_out"]
+
+    foreach var $vars {
+        if { ![global_array_exists $var] } {
+	    continue
+        }
+
+        set found [lsearch -exact $gdb_global_vars $var]
+        if { $found != -1 } {
+            # Already present before running test.
+            continue
+        }
+
+        set found [lsearch -exact $skip $var]
+        if { $found != -1 } {
+            continue
+        }
+
+	# A variable name cannot be used both as scalar and array without an
+	# intermediate unset.  Trying to do so will result in tcl errors, for
+	# example, for:
+	#   set var "bla"
+	#   set var(1) "bla"
+	# we get:
+	#   can't set "var(1)": variable isn't array
+	# and for the reverse statement order we get:
+	#   can't set "var": variable is array
+	#
+	# So, since a global name in one test-case can leak to another
+	# test-case, setting a global name in one test-case can result in
+	# a tcl error in another test-case that reuses the name in a different
+	# way.
+	#
+	# Warn about leaking a global array from the test-case.
+	# The way to fix this is to wrap the test-case in a namespace and to
+	# change the global variable into a namespace variable:
+	# namespace eval $testfile {
+	#     variable var
+	#     ...
+	# }
+	global gdb_test_file_name
+	warning "$gdb_test_file_name.exp defined global array $var"
+
+	# If the variable remains set, we won't warn for the next test where
+	# it's leaked, so unset.
+        global_unset $var
+    }
+}
+
 # The default timeout used when testing GDB commands.  We want to use
 # the same timeout as the default dejagnu timeout, unless the user has
 # already provided a specific value (probably through a site.exp file).
@@ -5177,10 +5266,12 @@ proc gdb_init { test_file_name } {
     global gdb_instances
     set gdb_instances 0
 
+    save_global_vars $test_file_name
     return [default_gdb_init $test_file_name]
 }
 
 proc gdb_finish { } {
+    check_global_vars
     global gdbserver_reconnect_p
     global gdb_prompt
     global cleanfiles

Re: [PATCH][gdb/testsuite] Warn about leaked global array

[Pedro Alves]

On 5/18/20 7:18 AM, Tom de Vries wrote:

> I managed to find a better way of dealing with this, using "upvar #0".
> I've also added more comments to the patch.

Cool.

> +# Check global variables not in gdb_global_vars.
> +
> +proc check_global_vars { } {
> +    # Sample state after running test.
> +    global gdb_global_vars
> +    set vars [info globals]
> +
> +    # I'm not sure these two should actually be global, but at least there
> +    # seems to be no harm in having these as globals, given that we don't
> +    # expect to reuse these names as scalars.
> +    set skip [list "expect_out" "spawn_out"]

They can be local or global.  See man expect:

 Expect takes a rather liberal view of scoping. In particular, variables read 
 by commands specific to the Expect program will be sought first from the local
 scope, and if not found, in the global scope. For example, this obviates the need to
 place "global timeout" in every procedure you   write that uses expect. On the other hand,
 variables written are always in the local scope (unless a "global" command has been issued).
 The most common problem this causes is when spawn is executed in a procedure.
 Outside the procedure, spawn_id no longer exists, so the spawned process is no
 longer accessible simply because of scoping. Add a "global spawn_id" to such a procedure. 

For example, mi_gdb_test does:

proc mi_gdb_test { args } {
    global verbose
    global mi_gdb_prompt
    global GDB expect_out
               ^^^^^^^^^^
    global inferior_exited_re async
    upvar timeout timeout

and then gdb.mi/mi-basics.exp does:

proc test_path_specification {} {
...
    global expect_out

...
    mi_gdb_test "-environment-path" "\\\^done,path=\"(.*)\"" "environment-path"
    set orig_path $expect_out(3,string)
...

So making expect_out global allows gdb.mi/mi-basics.exp to reference it.

We could alternatively switch mi_gdb_test (and whatever other procedure
does a similar thing) to doing "upvar expect_out expect_out" and
get rid of "global expect_out".  Not sure it's worth the bother.

> +	global gdb_test_file_name
> +	warning "$gdb_test_file_name.exp defined global array $var"
> +
> +	# If the variable remains set, we won't warn for the next test where
> +	# it's leaked, so unset.
> +        global_unset $var

Tabs / spaces above.

Thanks,
Pedro Alves

Re: [PATCH][gdb/testsuite] Warn about leaked global array

[Tom de Vries]

On 18-05-2020 12:41, Pedro Alves wrote:
> On 5/18/20 7:18 AM, Tom de Vries wrote:
> 
>> I managed to find a better way of dealing with this, using "upvar #0".
>> I've also added more comments to the patch.
> 
> Cool.
> 
>> +# Check global variables not in gdb_global_vars.
>> +
>> +proc check_global_vars { } {
>> +    # Sample state after running test.
>> +    global gdb_global_vars
>> +    set vars [info globals]
>> +
>> +    # I'm not sure these two should actually be global, but at least there
>> +    # seems to be no harm in having these as globals, given that we don't
>> +    # expect to reuse these names as scalars.
>> +    set skip [list "expect_out" "spawn_out"]
> 
> They can be local or global.  See man expect:
> 
>  Expect takes a rather liberal view of scoping. In particular, variables read 
>  by commands specific to the Expect program will be sought first from the local
>  scope, and if not found, in the global scope. For example, this obviates the need to
>  place "global timeout" in every procedure you   write that uses expect. On the other hand,
>  variables written are always in the local scope (unless a "global" command has been issued).
>  The most common problem this causes is when spawn is executed in a procedure.
>  Outside the procedure, spawn_id no longer exists, so the spawned process is no
>  longer accessible simply because of scoping. Add a "global spawn_id" to such a procedure. 
> 
> For example, mi_gdb_test does:
> 
> proc mi_gdb_test { args } {
>     global verbose
>     global mi_gdb_prompt
>     global GDB expect_out
>                ^^^^^^^^^^
>     global inferior_exited_re async
>     upvar timeout timeout
> 
> and then gdb.mi/mi-basics.exp does:
> 
> proc test_path_specification {} {
> ...
>     global expect_out
> 
> ...
>     mi_gdb_test "-environment-path" "\\\^done,path=\"(.*)\"" "environment-path"
>     set orig_path $expect_out(3,string)
> ...
> 
> So making expect_out global allows gdb.mi/mi-basics.exp to reference it.
> 
> We could alternatively switch mi_gdb_test (and whatever other procedure
> does a similar thing) to doing "upvar expect_out expect_out" and
> get rid of "global expect_out".  Not sure it's worth the bother.
> 

OK, that's helpful, thanks for the info.

>> +	global gdb_test_file_name
>> +	warning "$gdb_test_file_name.exp defined global array $var"
>> +
>> +	# If the variable remains set, we won't warn for the next test where
>> +	# it's leaked, so unset.
>> +        global_unset $var
> 
> Tabs / spaces above.
> 

Fixed, and resubmitted as part of a patch series that also includes
warning fixes here (
https://sourceware.org/pipermail/gdb-patches/2020-May/168771.html ).

Also, fixing the warnings turned out to be more complicated than I
thought, so I ended up moving the bit about how to fix the warning into
a gdb/testsuite/README section (part of patch 2/3).

Thanks,
- Tom