[PATCH 00/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[PATCH 00/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[Pedro Alves]

In PR/25412, Simon noticed that after the multi-target series, the
tid-reuse.exp testcase manages to create a duplicate thread in the
thread list.  Or rather, two threads with the same PTID.

This in turn exposes a design problem in GDB.  The inferior_thread()
function looks up the current thread based on inferior_ptid:

 struct thread_info*
 inferior_thread (void)
 {
   struct thread_info *tp = find_thread_ptid (current_inferior (), inferior_ptid);
   gdb_assert (tp);
   return tp;
 }

But if there can be more than one thread in the thread list with the
same ptid_t, inferior_thread() may well return the wrong thread.

This series fixes this by making the current thread be a global
thread_info pointer that is written to directly by switch_to_thread,
etc., and making inferior_thread() return that pointer, instead of
having inferior_thread() lookup up the inferior_ptid thread, by
ptid_t.  You can look at this as a continuation of the effort of using
more thread_info pointers instead of ptids when possible.

This change required auditing the whole codebase for places where we
were writing to inferior_ptid directly to change the current thread,
and change them to use switch_to_thread instead or one of its
siblings, because otherwise inferior_thread() and inferior_ptid would
get out of sync and inferior_thread() would return a thread unrelated
to the new inferior_ptid we wanted to switch to.  That was all
(hopefully) done in all the patches leading up to the last one.

After this, inferior_ptid still exists, but it is mostly read-only and
mainly used by target backend code.  It is also relied on by a number
of target methods as a global input argument.  E.g., the target_resume
interface and the memory reading routines -- we still need it there
because we need to be able to access memory off of processes for which
we don't have a corresponding inferior/thread object, like when
handling forks.  Maybe we could pass down a context explicitly to
target_read_memory, etc.

Most of the host-/native-specific code here is untested.  I did my
best, but I won't be surprised if more tweaking is necessary.

Testing on native x86_64 GNU/Linux is regression free for me.  Testing
against gdbserver has regressed significantly in the past months and
is becoming difficult to run with a high number of long timeout
sequences; really looks like people aren't paying much attention to
that.  I think this series doesn't regress gdbserver, but it's getting
hard to tell.  :-/

Pedro Alves (28):
  Don't write to inferior_ptid in linux_get_siginfo_data
  gcore, handle exited threads better
  Refactor delete_program_space as a destructor
  Don't write to inferior_ptid in gdbarch-selftests.c, mock
    address_space too
  Don't write to inferior_ptid in inf-ptrace.c
  Don't write to inferior_ptid in target.c
  Don't write to inferior_ptid in infrun.c
  Don't write to inferior_ptid in procfs.c
  Don't write to inferior_ptid in tracefile-tfile.c
  Don't write to inferior_ptid in tracectf.c
  Don't write to inferior_ptid in remote.c
  Don't write to inferior_ptid in remote-sim.c
  Don't write to inferior_ptid in nto-procfs.c
  Don't write to inferior_ptid in go32-nat.c
  Don't write to inferior_ptid in gnu-nat.c
  Don't write to inferior_ptid in darwin-nat.c
  Don't write to inferior_ptid in corelow.c
  Don't write to inferior_ptid in bsd-kvm.c
  Don't write to inferior_ptid in btrace_fetch
  Don't write to inferior_ptid in bsd-kvm.c
  Don't write to inferior_ptid in fork-child.c
  Don't write to inferior_ptid in go32-nat.c
  Don't write to inferior_ptid in remote-sim.c
  Don't write to inferior_ptid in windows-nat.c, part I
  Don't write to inferior_ptid in windows-nat.c, part II
  Don't write to inferior_ptid in ravenscar-thread.c
  Don't write to inferior_ptid in aix-thread.c
  Decouple inferior_ptid/inferior_thread(); dup ptids in thread list
    (PR/25412)

 gdb/aix-thread.c        |   2 +-
 gdb/bsd-kvm.c           |   6 +--
 gdb/btrace.c            |   6 ---
 gdb/corelow.c           |  20 +++++-----
 gdb/darwin-nat.c        |  16 ++++----
 gdb/fork-child.c        |   3 --
 gdb/gdbarch-selftests.c |  17 ++++----
 gdb/gdbthread.h         |  17 ++++----
 gdb/gnu-nat.c           |  14 +++----
 gdb/go32-nat.c          |   8 +---
 gdb/inf-ptrace.c        |  19 +++++----
 gdb/inferior.c          |   2 +-
 gdb/infrun.c            | 101 +++++++++++++++++++-----------------------------
 gdb/linux-tdep.c        |  64 ++++++++++++++++--------------
 gdb/nat/windows-nat.c   |   1 -
 gdb/nat/windows-nat.h   |   3 --
 gdb/nto-procfs.c        |  24 ++++++------
 gdb/procfs.c            |  18 ++++-----
 gdb/progspace.c         |  79 ++++++++++++++++++++-----------------
 gdb/progspace.h         |   4 --
 gdb/ravenscar-thread.c  |  49 ++++++++++++-----------
 gdb/remote-sim.c        |  10 ++---
 gdb/remote.c            |  34 ++++++++--------
 gdb/target.c            |   2 +-
 gdb/thread.c            |  97 ++++++++++++++++------------------------------
 gdb/tracectf.c          |   7 ++--
 gdb/tracefile-tfile.c   |   7 ++--
 gdb/windows-nat.c       |  68 +++++++++++++++-----------------
 28 files changed, 315 insertions(+), 383 deletions(-)


base-commit: dd1cab0694592099854e66467319253954c93764
-- 
2.14.5

[PATCH 01/28] Don't write to inferior_ptid in linux_get_siginfo_data

[Pedro Alves]

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* linux-tdep.c (btrace_fetch): Save/restore current thread instead
	of saving/restoring inferior_ptid.
---
 gdb/linux-tdep.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index e50946ce37..f204831e39 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -1618,8 +1618,8 @@ linux_get_siginfo_data (thread_info *thread, struct gdbarch *gdbarch)
   if (!gdbarch_get_siginfo_type_p (gdbarch))
     return gdb::byte_vector ();
 
-  scoped_restore save_inferior_ptid = make_scoped_restore (&inferior_ptid);
-  inferior_ptid = thread->ptid;
+  scoped_restore_current_thread save_current_thread;
+  switch_to_thread (thread);
 
   siginfo_type = gdbarch_get_siginfo_type (gdbarch);
 
-- 
2.14.5

[PATCH 02/28] gcore, handle exited threads better

[Pedro Alves]

An early (and since discarded) version of this series tried to make
exited threads have distinct PTID between each other, and that change
exposed a problem in linux-tdep.c...  This was exposed by the
gdb.threads/gcore-stale-thread.exp testcase, which is exactly about
calling gcore with an exited thread selected:

 (gdb) [Thread 0x7ffff7fb6740 (LWP 31523) exited]
 PASS: gdb.threads/gcore-stale-thread.exp: continue to breakpoint: break-here
 gcore /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.threads/gcore-stale-thread/gcore-stale-thread.core
 /home/pedro/gdb/binutils-gdb/build/../src/gdb/inferior.c:66: internal-error: void set_current_inferior(inferior*): Assertion `inf != NULL' failed.
 A problem internal to GDB has been detected,

That was find_inferior_ptid being called on the "exited" ptid, which
on that previous (and discarded attempt) had pid==-1.  The problem is
that linux-tdep.c, where it looks for the signalled thread, isn't
considering exited threads.  Also, while at it, that code isn't
considering multi-target either, since it is using
iterate_over_threads which iterates over all threads of all targets.
Fixed by switching to range-for iteration instead.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* linux-tdep.c (find_signalled_thread(thread_info *,void *)):
	Delete.
	(find_signalled_thread()): New, factored out from
	linux_make_corefile_notes and adjusted to handle exited threads.
	(linux_make_corefile_notes): Adjust to use the new
	find_signalled_thread.
---
 gdb/linux-tdep.c | 60 +++++++++++++++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 27 deletions(-)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index f204831e39..3e39a54ba2 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -1402,18 +1402,6 @@ linux_find_memory_regions (struct gdbarch *gdbarch,
 					 &data);
 }
 
-/* Determine which signal stopped execution.  */
-
-static int
-find_signalled_thread (struct thread_info *info, void *data)
-{
-  if (info->suspend.stop_signal != GDB_SIGNAL_0
-      && info->ptid.pid () == inferior_ptid.pid ())
-    return 1;
-
-  return 0;
-}
-
 /* This is used to pass information from
    linux_make_mappings_corefile_notes through
    linux_find_memory_regions_full.  */
@@ -1861,6 +1849,30 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
   return 1;
 }
 
+/* Find the signalled thread.  In case there's more than one signalled
+   thread, prefer the current thread, if it is signalled.  If no
+   thread was signalled, default to the current thread, unless it has
+   exited, in which case return NULL.  */
+
+static thread_info *
+find_signalled_thread ()
+{
+  thread_info *curr_thr = inferior_thread ();
+  if (curr_thr->state != THREAD_EXITED
+      && curr_thr->suspend.stop_signal != GDB_SIGNAL_0)
+    return curr_thr;
+
+  for (thread_info *thr : current_inferior ()->non_exited_threads ())
+    if (thr->suspend.stop_signal != GDB_SIGNAL_0)
+      return thr;
+
+  /* Default to the current thread, unless it has exited.  */
+  if (curr_thr->state != THREAD_EXITED)
+    return curr_thr;
+
+  return nullptr;
+}
+
 /* Build the note section for a corefile, and return it in a malloc
    buffer.  */
 
@@ -1870,7 +1882,6 @@ linux_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size)
   struct linux_corefile_thread_data thread_args;
   struct elf_internal_linux_prpsinfo prpsinfo;
   char *note_data = NULL;
-  struct thread_info *curr_thr, *signalled_thr;
 
   if (! gdbarch_iterate_over_regset_sections_p (gdbarch))
     return NULL;
@@ -1898,26 +1909,21 @@ linux_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size)
     }
 
   /* Like the kernel, prefer dumping the signalled thread first.
-     "First thread" is what tools use to infer the signalled thread.
-     In case there's more than one signalled thread, prefer the
-     current thread, if it is signalled.  */
-  curr_thr = inferior_thread ();
-  if (curr_thr->suspend.stop_signal != GDB_SIGNAL_0)
-    signalled_thr = curr_thr;
-  else
-    {
-      signalled_thr = iterate_over_threads (find_signalled_thread, NULL);
-      if (signalled_thr == NULL)
-	signalled_thr = curr_thr;
-    }
+     "First thread" is what tools use to infer the signalled
+     thread.  */
+  thread_info *signalled_thr = find_signalled_thread ();
 
   thread_args.gdbarch = gdbarch;
   thread_args.obfd = obfd;
   thread_args.note_data = note_data;
   thread_args.note_size = note_size;
-  thread_args.stop_signal = signalled_thr->suspend.stop_signal;
+  if (signalled_thr != nullptr)
+    thread_args.stop_signal = signalled_thr->suspend.stop_signal;
+  else
+    thread_args.stop_signal = GDB_SIGNAL_0;
 
-  linux_corefile_thread (signalled_thr, &thread_args);
+  if (signalled_thr != nullptr)
+    linux_corefile_thread (signalled_thr, &thread_args);
   for (thread_info *thr : current_inferior ()->non_exited_threads ())
     {
       if (thr == signalled_thr)
-- 
2.14.5

[PATCH 03/28] Refactor delete_program_space as a destructor

[Pedro Alves]

Currently, while the program_space's ctor adds the new pspace to the
pspaces list, the destructor doesn't remove the pspace from the
pspace list.  Instead, you're supposed to use delete_program_space, to
both remove the pspace from the list, and deleting the pspace.

This patch eliminates delete_program_space, and makes the pspace dtor
remove the deleted pspace from the pspace list itself, i.e., makes the
dtor do the mirror opposite of the ctor.

I found this helps with a following patch that will allocate a mock
program_space on the stack.  It's easier to just let the regular dtor
remove the mock pspace from the pspace list than arrange to call
delete_program_space instead of the pspace dtor in that situation.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* inferior.c (delete_inferior): Use delete operator directly
	instead of delete_program_space.
	* progspace.c (add_program_space): New, factored out from
	program_space::program_space.
	(remove_program_space): New, factored out from
	delete_program_space.
	(program_space::program_space): Rewrite.
	(program_space::~program_space): Call remove_program_space.
	(delete_program_space): Delete.
	* progspace.h (delete_program_space): Remove.
---
 gdb/inferior.c  |  2 +-
 gdb/progspace.c | 79 +++++++++++++++++++++++++++++++--------------------------
 gdb/progspace.h |  4 ---
 3 files changed, 44 insertions(+), 41 deletions(-)

diff --git a/gdb/inferior.c b/gdb/inferior.c
index c2e9da3d3d..ceee00e9ee 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -162,7 +162,7 @@ delete_inferior (struct inferior *todel)
 
   /* If this program space is rendered useless, remove it. */
   if (program_space_empty_p (inf->pspace))
-    delete_program_space (inf->pspace);
+    delete inf->pspace;
 
   delete inf;
 }
diff --git a/gdb/progspace.c b/gdb/progspace.c
index 1361040347..55648e4929 100644
--- a/gdb/progspace.c
+++ b/gdb/progspace.c
@@ -109,26 +109,58 @@ init_address_spaces (void)
 
 \f
 
-/* Adds a new empty program space to the program space list, and binds
-   it to ASPACE.  Returns the pointer to the new object.  */
+/* Add a program space from the program spaces list.  */
 
-program_space::program_space (address_space *aspace_)
-: num (++last_program_space_num), aspace (aspace_)
+static void
+add_program_space (program_space *pspace)
 {
-  program_space_alloc_data (this);
-
   if (program_spaces == NULL)
-    program_spaces = this;
+    program_spaces = pspace;
   else
     {
-      struct program_space *last;
+      program_space *last;
 
       for (last = program_spaces; last->next != NULL; last = last->next)
 	;
-      last->next = this;
+      last->next = pspace;
+    }
+}
+
+/* Remove a program space from the program spaces list.  */
+
+static void
+remove_program_space (program_space *pspace)
+{
+  program_space *ss, **ss_link;
+  gdb_assert (pspace != NULL);
+
+  ss = program_spaces;
+  ss_link = &program_spaces;
+  while (ss != NULL)
+    {
+      if (ss == pspace)
+	{
+	  *ss_link = ss->next;
+	  return;
+	}
+
+      ss_link = &ss->next;
+      ss = *ss_link;
     }
 }
 
+/* Adds a new empty program space to the program space list, and binds
+   it to ASPACE.  Returns the pointer to the new object.  */
+
+program_space::program_space (address_space *aspace_)
+  : num (++last_program_space_num),
+    aspace (aspace_)
+{
+  program_space_alloc_data (this);
+
+  add_program_space (this);
+}
+
 /* Releases program space PSPACE, and all its contents (shared
    libraries, objfiles, and any other references to the PSPACE in
    other modules).  It is an internal error to call this when PSPACE
@@ -139,6 +171,8 @@ program_space::~program_space ()
 {
   gdb_assert (this != current_program_space);
 
+  remove_program_space (this);
+
   scoped_restore_current_program_space restore_pspace;
 
   set_current_program_space (this);
@@ -259,33 +293,6 @@ program_space_empty_p (struct program_space *pspace)
   return 1;
 }
 
-/* Remove a program space from the program spaces list and release it.  It is
-   an error to call this function while PSPACE is the current program space. */
-
-void
-delete_program_space (struct program_space *pspace)
-{
-  struct program_space *ss, **ss_link;
-  gdb_assert (pspace != NULL);
-  gdb_assert (pspace != current_program_space);
-
-  ss = program_spaces;
-  ss_link = &program_spaces;
-  while (ss != NULL)
-    {
-      if (ss == pspace)
-	{
-	  *ss_link = ss->next;
-	  break;
-	}
-
-      ss_link = &ss->next;
-      ss = *ss_link;
-    }
-
-  delete pspace;
-}
-
 /* Prints the list of program spaces and their details on UIOUT.  If
    REQUESTED is not -1, it's the ID of the pspace that should be
    printed.  Otherwise, all spaces are printed.  */
diff --git a/gdb/progspace.h b/gdb/progspace.h
index 71a6f2841e..a0e9e009c0 100644
--- a/gdb/progspace.h
+++ b/gdb/progspace.h
@@ -362,10 +362,6 @@ extern struct program_space *current_program_space;
 #define ALL_PSPACES(pspace) \
   for ((pspace) = program_spaces; (pspace) != NULL; (pspace) = (pspace)->next)
 
-/* Remove a program space from the program spaces list and release it.  It is
-   an error to call this function while PSPACE is the current program space. */
-extern void delete_program_space (struct program_space *pspace);
-
 /* Returns the number of program spaces listed.  */
 extern int number_of_program_spaces (void);
 
-- 
2.14.5

Re: [PATCH 03/28] Refactor delete_program_space as a destructor

[Simon Marchi]

On 2020-04-14 1:54 p.m., Pedro Alves via Gdb-patches wrote:
> +/* Adds a new empty program space to the program space list, and binds
> +   it to ASPACE.  Returns the pointer to the new object.  */

You could take the opportunity to update this comment.  At least, the "Return the pointer"
part is not really relevant for a constructor.

Otherwise, this LGTM.

Simon

Re: [PATCH 03/28] Refactor delete_program_space as a destructor

[Pedro Alves]

On 4/15/20 4:54 PM, Simon Marchi wrote:
> On 2020-04-14 1:54 p.m., Pedro Alves via Gdb-patches wrote:
>> +/* Adds a new empty program space to the program space list, and binds
>> +   it to ASPACE.  Returns the pointer to the new object.  */
> 
> You could take the opportunity to update this comment.  At least, the "Return the pointer"
> part is not really relevant for a constructor.
> 
> Otherwise, this LGTM.
> 

Thanks.  Here's what I'm merging.


From 381ce63f2f010ef5c246be720ef177cf46a19179 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 16 Apr 2020 14:50:07 +0100
Subject: [PATCH] Refactor delete_program_space as a destructor

Currently, while the program_space's ctor adds the new pspace to the
pspaces list, the destructor doesn't remove the pspace from the pspace
list.  Instead, you're supposed to use delete_program_space, to both
remove the pspace from the list, and deleting the pspace.

This patch eliminates delete_program_space, and makes the pspace dtor
remove the deleted pspace from the pspace list itself, i.e., makes the
dtor do the mirror opposite of the ctor.

I found this helps with a following patch that will allocate a mock
program_space on the stack.  It's easier to just let the regular dtor
remove the mock pspace from the pspace list than arrange to call
delete_program_space instead of the pspace dtor in that situation.

While at it, move the ctor/dtor intro comments to the header file, and
make the ctor explicit.

gdb/ChangeLog:
2020-04-16  Pedro Alves  <palves@redhat.com>

	* inferior.c (delete_inferior): Use delete operator directly
	instead of delete_program_space.
	* progspace.c (add_program_space): New, factored out from
	program_space::program_space.
	(remove_program_space): New, factored out from
	delete_program_space.
	(program_space::program_space): Remove intro comment.  Rewrite.
	(program_space::~program_space): Remove intro comment.  Call
	remove_program_space.
	(delete_program_space): Delete.
	* progspace.h (program_space::program_space): Make explicit.  Move
	intro comment here, adjusted.
	(program_space::~program_space): Move intro comment here,
	adjusted.
	(delete_program_space): Remove.
---
 gdb/ChangeLog   | 18 +++++++++++++
 gdb/inferior.c  |  2 +-
 gdb/progspace.c | 84 +++++++++++++++++++++++++++++----------------------------
 gdb/progspace.h | 14 ++++++----
 4 files changed, 71 insertions(+), 47 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7ba862edd3..6c280e3f49 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,21 @@
+2020-04-16  Pedro Alves  <palves@redhat.com>
+
+	* inferior.c (delete_inferior): Use delete operator directly
+	instead of delete_program_space.
+	* progspace.c (add_program_space): New, factored out from
+	program_space::program_space.
+	(remove_program_space): New, factored out from
+	delete_program_space.
+	(program_space::program_space): Remove intro comment.  Rewrite.
+	(program_space::~program_space): Remove intro comment.  Call
+	remove_program_space.
+	(delete_program_space): Delete.
+	* progspace.h (program_space::program_space): Make explicit.  Move
+	intro comment here, adjusted.
+	(program_space::~program_space): Move intro comment here,
+	adjusted.
+	(delete_program_space): Remove.
+
 2020-04-16  Tom Tromey  <tromey@adacore.com>
 
 	* windows-nat.c (windows_nat::handle_access_violation): New
diff --git a/gdb/inferior.c b/gdb/inferior.c
index c2e9da3d3d..ceee00e9ee 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -162,7 +162,7 @@ delete_inferior (struct inferior *todel)
 
   /* If this program space is rendered useless, remove it. */
   if (program_space_empty_p (inf->pspace))
-    delete_program_space (inf->pspace);
+    delete inf->pspace;
 
   delete inf;
 }
diff --git a/gdb/progspace.c b/gdb/progspace.c
index 1361040347..6419f01770 100644
--- a/gdb/progspace.c
+++ b/gdb/progspace.c
@@ -109,36 +109,65 @@ init_address_spaces (void)
 
 \f
 
-/* Adds a new empty program space to the program space list, and binds
-   it to ASPACE.  Returns the pointer to the new object.  */
+/* Add a program space from the program spaces list.  */
 
-program_space::program_space (address_space *aspace_)
-: num (++last_program_space_num), aspace (aspace_)
+static void
+add_program_space (program_space *pspace)
 {
-  program_space_alloc_data (this);
-
   if (program_spaces == NULL)
-    program_spaces = this;
+    program_spaces = pspace;
   else
     {
-      struct program_space *last;
+      program_space *last;
 
       for (last = program_spaces; last->next != NULL; last = last->next)
 	;
-      last->next = this;
+      last->next = pspace;
     }
 }
 
-/* Releases program space PSPACE, and all its contents (shared
-   libraries, objfiles, and any other references to the PSPACE in
-   other modules).  It is an internal error to call this when PSPACE
-   is the current program space, since there should always be a
-   program space.  */
+/* Remove a program space from the program spaces list.  */
+
+static void
+remove_program_space (program_space *pspace)
+{
+  program_space *ss, **ss_link;
+  gdb_assert (pspace != NULL);
+
+  ss = program_spaces;
+  ss_link = &program_spaces;
+  while (ss != NULL)
+    {
+      if (ss == pspace)
+	{
+	  *ss_link = ss->next;
+	  return;
+	}
+
+      ss_link = &ss->next;
+      ss = *ss_link;
+    }
+}
+
+/* See progspace.h.  */
+
+program_space::program_space (address_space *aspace_)
+  : num (++last_program_space_num),
+    aspace (aspace_)
+{
+  program_space_alloc_data (this);
+
+  add_program_space (this);
+}
+
+/* See progspace.h.  */
 
 program_space::~program_space ()
 {
   gdb_assert (this != current_program_space);
 
+  remove_program_space (this);
+
   scoped_restore_current_program_space restore_pspace;
 
   set_current_program_space (this);
@@ -259,33 +288,6 @@ program_space_empty_p (struct program_space *pspace)
   return 1;
 }
 
-/* Remove a program space from the program spaces list and release it.  It is
-   an error to call this function while PSPACE is the current program space. */
-
-void
-delete_program_space (struct program_space *pspace)
-{
-  struct program_space *ss, **ss_link;
-  gdb_assert (pspace != NULL);
-  gdb_assert (pspace != current_program_space);
-
-  ss = program_spaces;
-  ss_link = &program_spaces;
-  while (ss != NULL)
-    {
-      if (ss == pspace)
-	{
-	  *ss_link = ss->next;
-	  break;
-	}
-
-      ss_link = &ss->next;
-      ss = *ss_link;
-    }
-
-  delete pspace;
-}
-
 /* Prints the list of program spaces and their details on UIOUT.  If
    REQUESTED is not -1, it's the ID of the pspace that should be
    printed.  Otherwise, all spaces are printed.  */
diff --git a/gdb/progspace.h b/gdb/progspace.h
index 71a6f2841e..2b887847e1 100644
--- a/gdb/progspace.h
+++ b/gdb/progspace.h
@@ -209,7 +209,15 @@ struct unwrapping_objfile_range
 
 struct program_space
 {
-  program_space (address_space *aspace_);
+  /* Constructs a new empty program space, binds it to ASPACE, and
+     adds it to the program space list.  */
+  explicit program_space (address_space *aspace);
+
+  /* Releases a program space, and all its contents (shared libraries,
+     objfiles, and any other references to the program space in other
+     modules).  It is an internal error to call this when the program
+     space is the current program space, since there should always be
+     a program space.  */
   ~program_space ();
 
   typedef unwrapping_objfile_range objfiles_range;
@@ -362,10 +370,6 @@ extern struct program_space *current_program_space;
 #define ALL_PSPACES(pspace) \
   for ((pspace) = program_spaces; (pspace) != NULL; (pspace) = (pspace)->next)
 
-/* Remove a program space from the program spaces list and release it.  It is
-   an error to call this function while PSPACE is the current program space. */
-extern void delete_program_space (struct program_space *pspace);
-
 /* Returns the number of program spaces listed.  */
 extern int number_of_program_spaces (void);
 

base-commit: a010605fef0eba73c564c3dd22e0a6ecbc26b10e
-- 
2.14.5

[PATCH 04/28] Don't write to inferior_ptid in gdbarch-selftests.c, mock address_space too

[Pedro Alves]

Use switch_to_thread instead of writing to inferior_ptid.  This
requires a couple of improvements to the mocking environment.  One is
to mock a pspace too, and assigning it to the inferior.  In turn, this
requires heap-allocating the address space, so that the regular
program_space dtor destroys the address space correctly.

(Note that new the mock program_space is allocated on the stack, and
thus depends on the previous patch that eliminated
delete_program_space.)

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* gdbarch-selftests.c: Include "progspace-and-thread.h".
	(register_to_value_test): Mock a program_space too.  Heap-allocate
	the address space.  Don't write to inferior_ptid.  Use
	switch_to_thread instead.
---
 gdb/gdbarch-selftests.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/gdb/gdbarch-selftests.c b/gdb/gdbarch-selftests.c
index 787a3f4058..9ab5e9fbe4 100644
--- a/gdb/gdbarch-selftests.c
+++ b/gdb/gdbarch-selftests.c
@@ -27,6 +27,7 @@
 #include "target-float.h"
 #include "gdbsupport/def-vector.h"
 #include "gdbarch.h"
+#include "progspace-and-thread.h"
 
 namespace selftests {
 
@@ -75,24 +76,25 @@ register_to_value_test (struct gdbarch *gdbarch)
 
   test_target_ops mock_target;
   ptid_t mock_ptid (1, 1);
+  program_space mock_pspace (new_address_space ());
   inferior mock_inferior (mock_ptid.pid ());
-  address_space mock_aspace {};
   mock_inferior.gdbarch = gdbarch;
-  mock_inferior.aspace = &mock_aspace;
+  mock_inferior.aspace = mock_pspace.aspace;
+  mock_inferior.pspace = &mock_pspace;
   thread_info mock_thread (&mock_inferior, mock_ptid);
 
+  scoped_restore_current_pspace_and_thread restore_pspace_thread;
+
   scoped_restore restore_thread_list
     = make_scoped_restore (&mock_inferior.thread_list, &mock_thread);
 
   /* Add the mock inferior to the inferior list so that look ups by
      target+ptid can find it.  */
   scoped_restore restore_inferior_list
-    = make_scoped_restore (&inferior_list);
-  inferior_list = &mock_inferior;
+    = make_scoped_restore (&inferior_list, &mock_inferior);
 
   /* Switch to the mock inferior.  */
-  scoped_restore_current_inferior restore_current_inferior;
-  set_current_inferior (&mock_inferior);
+  switch_to_inferior_no_thread (&mock_inferior);
 
   /* Push the process_stratum target so we can mock accessing
      registers.  */
@@ -102,8 +104,7 @@ register_to_value_test (struct gdbarch *gdbarch)
   SCOPE_EXIT { pop_all_targets_at_and_above (process_stratum); };
 
   /* Switch to the mock thread.  */
-  scoped_restore restore_inferior_ptid
-    = make_scoped_restore (&inferior_ptid, mock_ptid);
+  switch_to_thread (&mock_thread);
 
   struct frame_info *frame = get_current_frame ();
   const int num_regs = gdbarch_num_cooked_regs (gdbarch);
-- 
2.14.5

[PATCH 05/28] Don't write to inferior_ptid in inf-ptrace.c

[Pedro Alves]

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* inf-ptrace.c (inf_ptrace_target::create_inferior): Switch to the
	added thread.
	(inf_ptrace_target::attach): Don't write to inferior_ptid.  Switch
	to the added thread.
	(inf_ptrace_target::detach_success): Use switch_to_no_thread
	instead of writing to inferior_ptid.
---
 gdb/inf-ptrace.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index 1fa7aa3f73..8005187975 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -126,9 +126,6 @@ inf_ptrace_target::create_inferior (const char *exec_file,
 				    const std::string &allargs,
 				    char **env, int from_tty)
 {
-  pid_t pid;
-  ptid_t ptid;
-
   /* Do not change either targets above or the same target if already present.
      The reason is the target stack is shared across multiple inferiors.  */
   int ops_already_pushed = target_is_pushed (this);
@@ -141,14 +138,15 @@ inf_ptrace_target::create_inferior (const char *exec_file,
       unpusher.reset (this);
     }
 
-  pid = fork_inferior (exec_file, allargs, env, inf_ptrace_me, NULL,
-		       NULL, NULL, NULL);
+  pid_t pid = fork_inferior (exec_file, allargs, env, inf_ptrace_me, NULL,
+			     NULL, NULL, NULL);
 
-  ptid = ptid_t (pid);
+  ptid_t ptid (pid);
   /* We have something that executes now.  We'll be running through
      the shell at this point (if startup-with-shell is true), but the
      pid shouldn't change.  */
-  add_thread_silent (this, ptid);
+  thread_info *thr = add_thread_silent (this, ptid);
+  switch_to_thread (thr);
 
   unpusher.release ();
 
@@ -243,11 +241,12 @@ inf_ptrace_target::attach (const char *args, int from_tty)
   inf = current_inferior ();
   inferior_appeared (inf, pid);
   inf->attach_flag = 1;
-  inferior_ptid = ptid_t (pid);
 
   /* Always add a main thread.  If some target extends the ptrace
      target, it should decorate the ptid later with more info.  */
-  thread_info *thr = add_thread_silent (this, inferior_ptid);
+  thread_info *thr = add_thread_silent (this, ptid_t (pid));
+  switch_to_thread (thr);
+
   /* Don't consider the thread stopped until we've processed its
      initial SIGSTOP stop.  */
   set_executing (this, thr->ptid, true);
@@ -302,7 +301,7 @@ inf_ptrace_target::detach (inferior *inf, int from_tty)
 void
 inf_ptrace_target::detach_success (inferior *inf)
 {
-  inferior_ptid = null_ptid;
+  switch_to_no_thread ();
   detach_inferior (inf);
 
   maybe_unpush_target ();
-- 
2.14.5

[PATCH 06/28] Don't write to inferior_ptid in target.c

[Pedro Alves]

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* target.c (generic_mourn_inferior): Use switch_to_no_thread
	instead of writing to inferior_ptid.
---
 gdb/target.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/target.c b/gdb/target.c
index 2dc356d935..2514ce521b 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3196,7 +3196,7 @@ generic_mourn_inferior (void)
 {
   inferior *inf = current_inferior ();
 
-  inferior_ptid = null_ptid;
+  switch_to_no_thread ();
 
   /* Mark breakpoints uninserted in case something tries to delete a
      breakpoint while we delete the inferior's threads (which would
-- 
2.14.5

[PATCH 07/28] Don't write to inferior_ptid in infrun.c

[Pedro Alves]

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* infrun.c (generic_mourn_inferior): Use switch_to_thread instead
	of writing to inferior_ptid.
	(scoped_restore_exited_inferior): Delete.
	(handle_vfork_child_exec_or_exit): Simplify using
	scoped_restore_current_pspace_and_thread.  Use switch_to_thread
	instead of writing to inferior_ptid.
	(THREAD_STOPPED_BY): Delete.
	(thread_stopped_by_watchpoint, thread_stopped_by_sw_breakpoint)
	(thread_stopped_by_hw_breakpoint): Delete.
	(save_waitstatus): Use
	scoped_restore_current_thread+switch_to_thread, and call
	target_stopped_by_watchpoint instead of
	thread_stopped_by_watchpoint, target_stopped_by_sw_breakpoint
	instead of thread_stopped_by_sw_breakpoint, and
	target_stopped_by_hw_breakpoint instead of
	thread_stopped_by_hw_breakpoint.
	(handle_inferior_event)
	<TARGET_WAITKIND_EXITED/TARGET_WAITKIND_SIGNALLED>: Don't write to
	inferior_ptid directly, nor
	set_current_inferior/set_current_program_space.  Use
	switch_to_thread / switch_to_inferior_no_thread instead.
---
 gdb/infrun.c | 101 +++++++++++++++++++++++------------------------------------
 1 file changed, 40 insertions(+), 61 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 8ff34c382d..631cd90ae3 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -484,8 +484,8 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
 	  switch_to_no_thread ();
 	  child_inf->symfile_flags = SYMFILE_NO_READ;
 	  push_target (parent_inf->process_target ());
-	  add_thread_silent (child_inf->process_target (), child_ptid);
-	  inferior_ptid = child_ptid;
+	  thread_info *child_thr
+	    = add_thread_silent (child_inf->process_target (), child_ptid);
 
 	  /* If this is a vfork child, then the address-space is
 	     shared with the parent.  */
@@ -503,6 +503,11 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
 	      child_inf->pending_detach = 0;
 	      parent_inf->vfork_child = child_inf;
 	      parent_inf->pending_detach = 0;
+
+	      /* Now that the inferiors and program spaces are all
+		 wired up, we can switch to the child thread (which
+		 switches inferior and program space too).  */
+	      switch_to_thread (child_thr);
 	    }
 	  else
 	    {
@@ -512,6 +517,10 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
 	      set_current_program_space (child_inf->pspace);
 	      clone_program_space (child_inf->pspace, parent_inf->pspace);
 
+	      /* solib_create_inferior_hook relies on the current
+		 thread.  */
+	      switch_to_thread (child_thr);
+
 	      /* Let the shared library layer (e.g., solib-svr4) learn
 		 about this new process, relocate the cloned exec, pull
 		 in shared libraries, and install the solib event
@@ -627,8 +636,7 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
 	push_target (target);
       }
 
-      add_thread_silent (target, child_ptid);
-      inferior_ptid = child_ptid;
+      thread_info *child_thr = add_thread_silent (target, child_ptid);
 
       /* If this is a vfork child, then the address-space is shared
 	 with the parent.  If we detached from the parent, then we can
@@ -656,6 +664,8 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
 	     the core, this wouldn't be required.  */
 	  solib_create_inferior_hook (0);
 	}
+
+      switch_to_thread (child_thr);
     }
 
   return target_follow_fork (follow_child, detach_fork);
@@ -897,22 +907,6 @@ proceed_after_vfork_done (struct thread_info *thread,
   return 0;
 }
 
-/* Save/restore inferior_ptid, current program space and current
-   inferior.  Only use this if the current context points at an exited
-   inferior (and therefore there's no current thread to save).  */
-class scoped_restore_exited_inferior
-{
-public:
-  scoped_restore_exited_inferior ()
-    : m_saved_ptid (&inferior_ptid)
-  {}
-
-private:
-  scoped_restore_tmpl<ptid_t> m_saved_ptid;
-  scoped_restore_current_program_space m_pspace;
-  scoped_restore_current_inferior m_inferior;
-};
-
 /* Called whenever we notice an exec or exit event, to handle
    detaching or resuming a vfork parent.  */
 
@@ -935,7 +929,6 @@ handle_vfork_child_exec_or_exit (int exec)
 	 time.  */
       if (vfork_parent->pending_detach)
 	{
-	  struct thread_info *tp;
 	  struct program_space *pspace;
 	  struct address_space *aspace;
 
@@ -943,20 +936,10 @@ handle_vfork_child_exec_or_exit (int exec)
 
 	  vfork_parent->pending_detach = 0;
 
-	  gdb::optional<scoped_restore_exited_inferior>
-	    maybe_restore_inferior;
-	  gdb::optional<scoped_restore_current_pspace_and_thread>
-	    maybe_restore_thread;
-
-	  /* If we're handling a child exit, then inferior_ptid points
-	     at the inferior's pid, not to a thread.  */
-	  if (!exec)
-	    maybe_restore_inferior.emplace ();
-	  else
-	    maybe_restore_thread.emplace ();
+	  scoped_restore_current_pspace_and_thread restore_thread;
 
 	  /* We're letting loose of the parent.  */
-	  tp = any_live_thread_of_inferior (vfork_parent);
+	  thread_info *tp = any_live_thread_of_inferior (vfork_parent);
 	  switch_to_thread (tp);
 
 	  /* We're about to detach from the parent, which implicitly
@@ -1025,11 +1008,11 @@ handle_vfork_child_exec_or_exit (int exec)
 	     go ahead and create a new one for this exiting
 	     inferior.  */
 
-	  /* Switch to null_ptid while running clone_program_space, so
+	  /* Switch to no-thread while running clone_program_space, so
 	     that clone_program_space doesn't want to read the
 	     selected frame of a dead process.  */
-	  scoped_restore restore_ptid
-	    = make_scoped_restore (&inferior_ptid, null_ptid);
+	  scoped_restore_current_thread restore_thread;
+	  switch_to_no_thread ();
 
 	  inf->pspace = new program_space (maybe_new_address_space ());
 	  inf->aspace = inf->pspace->aspace;
@@ -4612,25 +4595,6 @@ wait_one ()
     }
 }
 
-/* Generate a wrapper for target_stopped_by_REASON that works on PTID
-   instead of the current thread.  */
-#define THREAD_STOPPED_BY(REASON)		\
-static int					\
-thread_stopped_by_ ## REASON (ptid_t ptid)	\
-{						\
-  scoped_restore save_inferior_ptid = make_scoped_restore (&inferior_ptid); \
-  inferior_ptid = ptid;				\
-						\
-  return target_stopped_by_ ## REASON ();	\
-}
-
-/* Generate thread_stopped_by_watchpoint.  */
-THREAD_STOPPED_BY (watchpoint)
-/* Generate thread_stopped_by_sw_breakpoint.  */
-THREAD_STOPPED_BY (sw_breakpoint)
-/* Generate thread_stopped_by_hw_breakpoint.  */
-THREAD_STOPPED_BY (hw_breakpoint)
-
 /* Save the thread's event and stop reason to process it later.  */
 
 static void
@@ -4662,19 +4626,22 @@ save_waitstatus (struct thread_info *tp, const target_waitstatus *ws)
 
       adjust_pc_after_break (tp, &tp->suspend.waitstatus);
 
-      if (thread_stopped_by_watchpoint (tp->ptid))
+      scoped_restore_current_thread restore_thread;
+      switch_to_thread (tp);
+
+      if (target_stopped_by_watchpoint ())
 	{
 	  tp->suspend.stop_reason
 	    = TARGET_STOPPED_BY_WATCHPOINT;
 	}
       else if (target_supports_stopped_by_sw_breakpoint ()
-	       && thread_stopped_by_sw_breakpoint (tp->ptid))
+	       && target_stopped_by_sw_breakpoint ())
 	{
 	  tp->suspend.stop_reason
 	    = TARGET_STOPPED_BY_SW_BREAKPOINT;
 	}
       else if (target_supports_stopped_by_hw_breakpoint ()
-	       && thread_stopped_by_hw_breakpoint (tp->ptid))
+	       && target_stopped_by_hw_breakpoint ())
 	{
 	  tp->suspend.stop_reason
 	    = TARGET_STOPPED_BY_HW_BREAKPOINT;
@@ -5268,9 +5235,21 @@ handle_inferior_event (struct execution_control_state *ecs)
 
     case TARGET_WAITKIND_EXITED:
     case TARGET_WAITKIND_SIGNALLED:
-      inferior_ptid = ecs->ptid;
-      set_current_inferior (find_inferior_ptid (ecs->target, ecs->ptid));
-      set_current_program_space (current_inferior ()->pspace);
+      {
+	/* Depending on the system, ecs->ptid may point to a thread or
+	   to a process.  On some targets, target_mourn_inferior may
+	   need to have access to the just-exited thread.  That is the
+	   case of GNU/Linux's "checkpoint" support, for example.
+	   Call the switch_to_xxx routine as appropriate.  */
+	thread_info *thr = find_thread_ptid (ecs->target, ecs->ptid);
+	if (thr != nullptr)
+	  switch_to_thread (thr);
+	else
+	  {
+	    inferior *inf = find_inferior_ptid (ecs->target, ecs->ptid);
+	    switch_to_inferior_no_thread (inf);
+	  }
+      }
       handle_vfork_child_exec_or_exit (0);
       target_terminal::ours ();	/* Must do this before mourn anyway.  */
 
-- 
2.14.5

[PATCH 08/28] Don't write to inferior_ptid in procfs.c

[Pedro Alves]

The inferior_ptid write in procfs_do_thread_registers should be
unnecessary because the target fetch_registers method should (and
does) extract the ptid from the regcache.

Not tested.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* procfs.c (procfs_target::attach): Don't write to inferior_ptid.
	(procfs_target::detach): Use switch_to_no_thread
	instead of writing to inferior_ptid directly.
	(do_attach): Change return type to void.  Switch to the added
	thread.
	(procfs_target::create_inferior): Switch to the added thread.
	(procfs_do_thread_registers): Don't write to inferior_ptid.
---
 gdb/procfs.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/gdb/procfs.c b/gdb/procfs.c
index 09a036f90b..dbb042cedf 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -1805,7 +1805,7 @@ proc_iterate_over_threads (procinfo *pi,
 /* Here are all of the gdb target vector functions and their
    friends.  */
 
-static ptid_t do_attach (ptid_t ptid);
+static void do_attach (ptid_t ptid);
 static void do_detach ();
 static void proc_trace_syscalls_1 (procinfo *pi, int syscallnum,
 				   int entry_or_exit, int mode, int from_tty);
@@ -1894,7 +1894,7 @@ procfs_target::attach (const char *args, int from_tty)
 
       fflush (stdout);
     }
-  inferior_ptid = do_attach (ptid_t (pid));
+  do_attach (ptid_t (pid));
   if (!target_is_pushed (this))
     push_target (this);
 }
@@ -1918,12 +1918,12 @@ procfs_target::detach (inferior *inf, int from_tty)
 
   do_detach ();
 
-  inferior_ptid = null_ptid;
+  switch_to_no_thread ();
   detach_inferior (inf);
   maybe_unpush_target ();
 }
 
-static ptid_t
+static void
 do_attach (ptid_t ptid)
 {
   procinfo *pi;
@@ -1991,9 +1991,8 @@ do_attach (ptid_t ptid)
 
   /* Add it to gdb's thread list.  */
   ptid = ptid_t (pi->pid, lwpid, 0);
-  add_thread (&the_procfs_target, ptid);
-
-  return ptid;
+  thread_info *thr = add_thread (&the_procfs_target, ptid);
+  switch_to_thread (thr);
 }
 
 static void
@@ -3092,7 +3091,8 @@ procfs_target::create_inferior (const char *exec_file,
   /* We have something that executes now.  We'll be running through
      the shell at this point (if startup-with-shell is true), but the
      pid shouldn't change.  */
-  add_thread_silent (this, ptid_t (pid));
+  thread_info *thr = add_thread_silent (this, ptid_t (pid));
+  switch_to_thread (thr);
 
   procfs_init_inferior (pid);
 }
@@ -3755,8 +3755,6 @@ procfs_do_thread_registers (bfd *obfd, ptid_t ptid,
      once it is implemented in this platform:
      gdbarch_iterate_over_regset_sections().  */
 
-  scoped_restore save_inferior_ptid = make_scoped_restore (&inferior_ptid);
-  inferior_ptid = ptid;
   target_fetch_registers (regcache, -1);
 
   fill_gregset (regcache, &gregs, -1);
-- 
2.14.5

[PATCH 09/28] Don't write to inferior_ptid in tracefile-tfile.c

[Pedro Alves]

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* tracefile-tfile.c (tfile_target_open): Don't write to
	inferior_ptid directly, instead switch to added thread.
	(tfile_target::close): Use switch_to_no_thread instead of writing
	to inferior_ptid directly.
---
 gdb/tracefile-tfile.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/gdb/tracefile-tfile.c b/gdb/tracefile-tfile.c
index ea19177475..fd7bab822a 100644
--- a/gdb/tracefile-tfile.c
+++ b/gdb/tracefile-tfile.c
@@ -556,8 +556,9 @@ tfile_target_open (const char *arg, int from_tty)
     }
 
   inferior_appeared (current_inferior (), TFILE_PID);
-  inferior_ptid = ptid_t (TFILE_PID);
-  add_thread_silent (&tfile_ops, inferior_ptid);
+
+  thread_info *thr = add_thread_silent (&tfile_ops, ptid_t (TFILE_PID));
+  switch_to_thread (thr);
 
   if (ts->traceframe_count <= 0)
     warning (_("No traceframes present in this file."));
@@ -618,7 +619,7 @@ tfile_target::close ()
 {
   gdb_assert (trace_fd != -1);
 
-  inferior_ptid = null_ptid;	/* Avoid confusion from thread stuff.  */
+  switch_to_no_thread ();	/* Avoid confusion from thread stuff.  */
   exit_inferior_silent (current_inferior ());
 
   ::close (trace_fd);
-- 
2.14.5

[PATCH 10/28] Don't write to inferior_ptid in tracectf.c

[Pedro Alves]

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* tracectf.c (ctf_target_open): Switch to added thread instead of
	writing to inferior_ptid directly.
	(ctf_target::close): Use switch_to_no_thread instead of writing to
	inferior_ptid directly.
---
 gdb/tracectf.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/gdb/tracectf.c b/gdb/tracectf.c
index 1c7003c2c9..2c9a7495bf 100644
--- a/gdb/tracectf.c
+++ b/gdb/tracectf.c
@@ -1168,8 +1168,9 @@ ctf_target_open (const char *dirname, int from_tty)
   push_target (&ctf_ops);
 
   inferior_appeared (current_inferior (), CTF_PID);
-  inferior_ptid = ptid_t (CTF_PID);
-  add_thread_silent (&ctf_ops, inferior_ptid);
+
+  thread_info *thr = add_thread_silent (&ctf_ops, ptid_t (CTF_PID));
+  switch_to_thread (thr);
 
   merge_uploaded_trace_state_variables (&uploaded_tsvs);
   merge_uploaded_tracepoints (&uploaded_tps);
@@ -1187,7 +1188,7 @@ ctf_target::close ()
   xfree (trace_dirname);
   trace_dirname = NULL;
 
-  inferior_ptid = null_ptid;	/* Avoid confusion from thread stuff.  */
+  switch_to_no_thread ();	/* Avoid confusion from thread stuff.  */
   exit_inferior_silent (current_inferior ());
 
   trace_reset_local_state ();
-- 
2.14.5

[PATCH 11/28] Don't write to inferior_ptid in remote.c

[Pedro Alves]

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* remote.c (remote_target::remote_notice_new_inferior): Use
	switch_to_thread instead of writing to inferior_ptid directly.
	(remote_target::add_current_inferior_and_thread): Use
	switch_to_no_thread instead of writing to inferior_ptid directly.
	(extended_remote_target::attach): Use switch_to_inferior_no_thread
	and switch_to_thread instead of using set_current_inferior or
	writing to inferior_ptid directly.
---
 gdb/remote.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index bfbc0bc21d..8f8d34f3e5 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -2492,8 +2492,9 @@ remote_target::remote_notice_new_inferior (ptid_t currthread, int executing)
 	    thread_change_ptid (this, inferior_ptid, currthread);
 	  else
 	    {
-	      remote_add_thread (currthread, running, executing);
-	      inferior_ptid = currthread;
+	      thread_info *thr
+		= remote_add_thread (currthread, running, executing);
+	      switch_to_thread (thr);
 	    }
 	  return;
 	}
@@ -4328,9 +4329,10 @@ remote_target::add_current_inferior_and_thread (char *wait_status)
   struct remote_state *rs = get_remote_state ();
   bool fake_pid_p = false;
 
-  inferior_ptid = null_ptid;
+  switch_to_no_thread ();
 
-  /* Now, if we have thread information, update inferior_ptid.  */
+  /* Now, if we have thread information, update the current thread's
+     ptid.  */
   ptid_t curr_ptid = get_current_thread (wait_status);
 
   if (curr_ptid != null_ptid)
@@ -5742,7 +5744,7 @@ remote_target::remote_detach_1 (inferior *inf, int from_tty)
     }
   else
     {
-      inferior_ptid = null_ptid;
+      switch_to_no_thread ();
       detach_inferior (current_inferior ());
     }
 }
@@ -5888,33 +5890,33 @@ extended_remote_target::attach (const char *args, int from_tty)
 	     target_pid_to_str (ptid_t (pid)).c_str ());
     }
 
-  set_current_inferior (remote_add_inferior (false, pid, 1, 0));
+  switch_to_inferior_no_thread (remote_add_inferior (false, pid, 1, 0));
 
   inferior_ptid = ptid_t (pid);
 
   if (target_is_non_stop_p ())
     {
-      struct thread_info *thread;
-
       /* Get list of threads.  */
       update_thread_list ();
 
-      thread = first_thread_of_inferior (current_inferior ());
-      if (thread)
-	inferior_ptid = thread->ptid;
-      else
-	inferior_ptid = ptid_t (pid);
+      thread_info *thread = first_thread_of_inferior (current_inferior ());
+      if (thread != nullptr)
+	switch_to_thread (thread);
 
       /* Invalidate our notion of the remote current thread.  */
       record_currthread (rs, minus_one_ptid);
     }
   else
     {
-      /* Now, if we have thread information, update inferior_ptid.  */
-      inferior_ptid = remote_current_thread (inferior_ptid);
+      /* Now, if we have thread information, update the main thread's
+	 ptid.  */
+      ptid_t curr_ptid = remote_current_thread (ptid_t (pid));
 
       /* Add the main thread to the thread list.  */
-      thread_info *thr = add_thread_silent (this, inferior_ptid);
+      thread_info *thr = add_thread_silent (this, curr_ptid);
+
+      switch_to_thread (thr);
+
       /* Don't consider the thread stopped until we've processed the
 	 saved stop reply.  */
       set_executing (this, thr->ptid, true);
-- 
2.14.5

[PATCH 12/28] Don't write to inferior_ptid in remote-sim.c

[Pedro Alves]

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* remote-sim.c (gdbsim_target_open): Use switch_to_no_thread
	instead of writing to inferior_ptid directly.
---
 gdb/remote-sim.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/remote-sim.c b/gdb/remote-sim.c
index 347dfd7013..da1ee96519 100644
--- a/gdb/remote-sim.c
+++ b/gdb/remote-sim.c
@@ -761,7 +761,7 @@ gdbsim_target_open (const char *args, int from_tty)
 
   /* There's nothing running after "target sim" or "load"; not until
      "run".  */
-  inferior_ptid = null_ptid;
+  switch_to_no_thread ();
 
   gdbsim_is_open = 1;
 }
-- 
2.14.5

[PATCH 13/28] Don't write to inferior_ptid in nto-procfs.c

[Pedro Alves]

A best effort patch, which fixes some bit rot and removes some
inferior_ptid references -- this port clearly hasn't been built in a
long while.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* nto-procfs.c (nto_procfs_target::update_thread_list): Avoid
	inferior_ptid.
	(nto_procfs_target::attach): Avoid inferior_ptid.  Switch to
	thread.
	(nto_procfs_target::detach): Avoid referencing
	inferior_ptid.  Use switch_to_no_thread instead of writing to
	inferior_ptid directly.
	(nto_procfs_target::mourn_inferior): Use switch_to_no_thread
	instead of writing to inferior_ptid directly.
	(nto_procfs_target::create_inferior): Avoid inferior_ptid.  Switch
	to thread.
---
 gdb/nto-procfs.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/gdb/nto-procfs.c b/gdb/nto-procfs.c
index 58e572c094..f649b6cf58 100644
--- a/gdb/nto-procfs.c
+++ b/gdb/nto-procfs.c
@@ -393,7 +393,7 @@ nto_procfs_target::update_thread_list ()
 
   prune_threads ();
 
-  pid = inferior_ptid.pid ();
+  pid = current_inferior ()->pid;
 
   status.tid = 1;
 
@@ -712,7 +712,7 @@ nto_procfs_target::attach (const char *args, int from_tty)
 	printf_unfiltered ("Attaching to %s\n",
 			   target_pid_to_str (ptid_t (pid)).c_str ());
     }
-  inferior_ptid = do_attach (ptid_t (pid));
+  ptid_t ptid = do_attach (ptid_t (pid));
   inf = current_inferior ();
   inferior_appeared (inf, pid);
   inf->attach_flag = 1;
@@ -720,7 +720,9 @@ nto_procfs_target::attach (const char *args, int from_tty)
   if (!target_is_pushed (ops))
     push_target (ops);
 
-  procfs_update_thread_list (ops);
+  update_thread_list ();
+
+  switch_to_thread (find_thread_ptid (this, ptid));
 }
 
 void
@@ -1000,19 +1002,16 @@ nto_procfs_target::xfer_partial (enum target_object object,
 void
 nto_procfs_target::detach (inferior *inf, int from_tty)
 {
-  int pid;
-
   target_announce_detach ();
 
   if (siggnal)
-    SignalKill (nto_node (), inferior_ptid.pid (), 0, 0, 0, 0);
+    SignalKill (nto_node (), inf->pid, 0, 0, 0, 0);
 
   close (ctl_fd);
   ctl_fd = -1;
 
-  pid = inferior_ptid.pid ();
-  inferior_ptid = null_ptid;
-  detach_inferior (pid);
+  switch_to_no_thread ();
+  detach_inferior (inf->pid);
   init_thread_list ();
   inf_child_maybe_unpush_target (ops);
 }
@@ -1132,7 +1131,7 @@ nto_procfs_target::mourn_inferior ()
       SignalKill (nto_node (), inferior_ptid.pid (), 0, SIGKILL, 0, 0);
       close (ctl_fd);
     }
-  inferior_ptid = null_ptid;
+  switch_to_no_thread ();
   init_thread_list ();
   inf_child_mourn_inferior (ops);
 }
@@ -1303,8 +1302,9 @@ nto_procfs_target::create_inferior (const char *exec_file,
   if (fds[2] != STDERR_FILENO)
     close (fds[2]);
 
-  inferior_ptid = do_attach (ptid_t (pid));
-  procfs_update_thread_list (ops);
+  ptid_t ptid = do_attach (ptid_t (pid));
+  update_thread_list ();
+  switch_to_thread (find_thread_ptid (this, ptid));
 
   inf = current_inferior ();
   inferior_appeared (inf, pid);
-- 
2.14.5

[PATCH 14/28] Don't write to inferior_ptid in go32-nat.c

[Pedro Alves]

generic_mourn_inferior already takes care of switching to no thread.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* go32-nat.c (go32_nat_target::create_inferior): Don't write to
	inferior_ptid.
---
 gdb/go32-nat.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/gdb/go32-nat.c b/gdb/go32-nat.c
index b3ebd6cf22..8423001ff6 100644
--- a/gdb/go32-nat.c
+++ b/gdb/go32-nat.c
@@ -770,8 +770,6 @@ go32_nat_target::create_inferior (const char *exec_file,
 void
 go32_nat_target::mourn_inferior ()
 {
-  ptid_t ptid;
-
   redir_cmdline_delete (&child_cmd);
   resume_signal = -1;
   resume_is_step = 0;
@@ -787,8 +785,6 @@ go32_nat_target::mourn_inferior ()
      the OS cleans up when the debuggee exits.  */
   x86_cleanup_dregs ();
 
-  ptid = inferior_ptid;
-  inferior_ptid = null_ptid;
   prog_has_started = 0;
 
   generic_mourn_inferior ();
-- 
2.14.5

[PATCH 15/28] Don't write to inferior_ptid in gnu-nat.c

[Pedro Alves]

Untested.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* gnu-nat.c (gnu_nat_target::create_inferior): Switch to the added
	thread.
	(gnu_nat_target::attach): Don't write to inferior_ptid directly.
	Instead use switch_to_thread.
	(gnu_nat_target::detach): Use switch_to_no_thread
	instead of writing to inferior_ptid directly.  Used passed-in
	inferior instead of looking up the inferior by pid.
---
 gdb/gnu-nat.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
index 3b438a9a43..d4ecf50014 100644
--- a/gdb/gnu-nat.c
+++ b/gdb/gnu-nat.c
@@ -2149,7 +2149,8 @@ gnu_nat_target::create_inferior (const char *exec_file,
   /* We have something that executes now.  We'll be running through
      the shell at this point (if startup-with-shell is true), but the
      pid shouldn't change.  */
-  add_thread_silent (ptid_t (pid));
+  thread_info *thr = add_thread_silent (ptid_t (pid));
+  switch_to_thread (thr);
 
   /* Attach to the now stopped child, which is actually a shell...  */
   inf_debug (inf, "attaching to child: %d", pid);
@@ -2226,7 +2227,8 @@ gnu_nat_target::attach (const char *args, int from_tty)
 
   inf_update_procs (inf);
 
-  inferior_ptid = ptid_t (pid, inf_pick_first_thread (), 0);
+  thread_info *thr = find_thread_ptid (ptid_t (pid, inf_pick_first_thread ()));
+  switch_to_thread (thr);
 
   /* We have to initialize the terminal settings now, since the code
      below might try to restore them.  */
@@ -2255,8 +2257,6 @@ gnu_nat_target::attach (const char *args, int from_tty)
 void
 gnu_nat_target::detach (inferior *inf, int from_tty)
 {
-  int pid;
-
   if (from_tty)
     {
       const char *exec_file = get_exec_file (0);
@@ -2268,12 +2268,10 @@ gnu_nat_target::detach (inferior *inf, int from_tty)
 	printf_unfiltered ("Detaching from pid %d\n", gnu_current_inf->pid);
     }
 
-  pid = gnu_current_inf->pid;
-
   inf_detach (gnu_current_inf);
 
-  inferior_ptid = null_ptid;
-  detach_inferior (find_inferior_pid (pid));
+  switch_to_no_thread ();
+  detach_inferior (inf);
 
   maybe_unpush_target ();
 }
-- 
2.14.5

[PATCH 16/28] Don't write to inferior_ptid in darwin-nat.c

[Pedro Alves]

Untested.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* darwin-nat.c (darwin_nat_target::decode_message): Don't write to
	inferior_ptid.
	(darwin_nat_target::stop_inferior, darwin_nat_target::kill): Avoid
	inferior_ptid.
	(darwin_attach_pid): Use switch_to_no_thread instead of writing to
	inferior_ptid directly.
	(darwin_nat_target::init_thread_list): Switch to thread, instead
	of writing to inferior_ptid.
	(darwin_nat_target::attach): Don't write to inferior_ptid.
	(darwin_nat_target::get_ada_task_ptid): Avoid inferior_ptid.
---
 gdb/darwin-nat.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index 3bd8d8ce00..aacd40ba6f 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -1141,8 +1141,7 @@ darwin_nat_target::decode_message (mach_msg_header_t *hdr,
 	      /* Looks necessary on Leopard and harmless...  */
 	      wait4 (inf->pid, &wstatus, 0, NULL);
 
-	      inferior_ptid = ptid_t (inf->pid, 0, 0);
-	      return inferior_ptid;
+	      return ptid_t (inf->pid);
 	    }
 	  else
 	    {
@@ -1432,7 +1431,7 @@ darwin_nat_target::stop_inferior (inferior *inf)
   /* Wait until the process is really stopped.  */
   while (1)
     {
-      ptid = wait_1 (inferior_ptid, &wstatus);
+      ptid = wait_1 (pid_t (inf->pid), &wstatus);
       if (wstatus.kind == TARGET_WAITKIND_STOPPED
 	  && wstatus.value.sig == GDB_SIGNAL_STOP)
 	break;
@@ -1557,13 +1556,13 @@ darwin_nat_target::kill ()
 
       darwin_resume_inferior (inf);
 
-      ptid = wait_1 (inferior_ptid, &wstatus);
+      ptid = wait_1 (pid_t (inf->pid), &wstatus);
     }
   else if (errno != ESRCH)
     warning (_("Failed to kill inferior: kill (%d, 9) returned [%s]"),
 	     inf->pid, safe_strerror (errno));
 
-  target_mourn_inferior (inferior_ptid);
+  target_mourn_inferior (pid_t (inf->pid));
 }
 
 static void
@@ -1683,7 +1682,7 @@ darwin_attach_pid (struct inferior *inf)
   catch (const gdb_exception &ex)
     {
       exit_inferior (inf);
-      inferior_ptid = null_ptid;
+      switch_to_no_thread ();
 
       throw;
     }
@@ -1722,7 +1721,7 @@ darwin_nat_target::init_thread_list (inferior *inf)
   struct thread_info *first_thread
     = thread_info_from_private_thread_info (first_pti);
 
-  inferior_ptid = first_thread->ptid;
+  switch_to_thread (first_thread);
 }
 
 /* The child must synchronize with gdb: gdb must set the exception port
@@ -2057,7 +2056,6 @@ darwin_nat_target::attach (const char *args, int from_tty)
     error (_("Can't attach to process %d: %s (%d)"),
            pid, safe_strerror (errno), errno);
 
-  inferior_ptid = ptid_t (pid);
   inf = current_inferior ();
   inferior_appeared (inf, pid);
   inf->attach_flag = 1;
@@ -2469,7 +2467,7 @@ darwin_nat_target::get_ada_task_ptid (long lwp, long thread)
                  names_count * sizeof (mach_port_t));
 
   if (res)
-    return ptid_t (inferior_ptid.pid (), 0, res);
+    return ptid_t (current_inferior ()->pid, 0, res);
   else
     return null_ptid;
 }
-- 
2.14.5

Re: [PATCH 16/28] Don't write to inferior_ptid in darwin-nat.c

[Simon Marchi]

On 2020-04-14 1:54 p.m., Pedro Alves via Gdb-patches wrote:
> Untested.
> 
> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> 	* darwin-nat.c (darwin_nat_target::decode_message): Don't write to
> 	inferior_ptid.
> 	(darwin_nat_target::stop_inferior, darwin_nat_target::kill): Avoid
> 	inferior_ptid.
> 	(darwin_attach_pid): Use switch_to_no_thread instead of writing to
> 	inferior_ptid directly.
> 	(darwin_nat_target::init_thread_list): Switch to thread, instead
> 	of writing to inferior_ptid.
> 	(darwin_nat_target::attach): Don't write to inferior_ptid.
> 	(darwin_nat_target::get_ada_task_ptid): Avoid inferior_ptid.
> ---
>  gdb/darwin-nat.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)

I tried to build it, here's a small patch to fix some typos.  It would be good to
check the other patches that you were not able to build for that pid_t/ptid_t typo.


diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index aacd40ba6f..367db9c1fe 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -1431,7 +1431,7 @@ darwin_nat_target::stop_inferior (inferior *inf)
   /* Wait until the process is really stopped.  */
   while (1)
     {
-      ptid = wait_1 (pid_t (inf->pid), &wstatus);
+      ptid = wait_1 (ptid_t (inf->pid), &wstatus);
       if (wstatus.kind == TARGET_WAITKIND_STOPPED
 	  && wstatus.value.sig == GDB_SIGNAL_STOP)
 	break;
@@ -1556,13 +1556,13 @@ darwin_nat_target::kill ()

       darwin_resume_inferior (inf);

-      ptid = wait_1 (pid_t (inf->pid), &wstatus);
+      ptid = wait_1 (ptid_t (inf->pid), &wstatus);
     }
   else if (errno != ESRCH)
     warning (_("Failed to kill inferior: kill (%d, 9) returned [%s]"),
 	     inf->pid, safe_strerror (errno));

-  target_mourn_inferior (pid_t (inf->pid));
+  target_mourn_inferior (ptid_t (inf->pid));
 }

 static void

Re: [PATCH 16/28] Don't write to inferior_ptid in darwin-nat.c

[Pedro Alves]

On 4/16/20 2:33 AM, Simon Marchi wrote:
> On 2020-04-14 1:54 p.m., Pedro Alves via Gdb-patches wrote:
>> Untested.
>>
>> gdb/ChangeLog:
>> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
>>
>> 	* darwin-nat.c (darwin_nat_target::decode_message): Don't write to
>> 	inferior_ptid.
>> 	(darwin_nat_target::stop_inferior, darwin_nat_target::kill): Avoid
>> 	inferior_ptid.
>> 	(darwin_attach_pid): Use switch_to_no_thread instead of writing to
>> 	inferior_ptid directly.
>> 	(darwin_nat_target::init_thread_list): Switch to thread, instead
>> 	of writing to inferior_ptid.
>> 	(darwin_nat_target::attach): Don't write to inferior_ptid.
>> 	(darwin_nat_target::get_ada_task_ptid): Avoid inferior_ptid.
>> ---
>>  gdb/darwin-nat.c | 16 +++++++---------
>>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> I tried to build it, here's a small patch to fix some typos.  It would be good to
> check the other patches that you were not able to build for that pid_t/ptid_t typo.

Thanks, I merged that fix locally.

I diffed the whole series and didn't find any other case.

Pedro Alves

[PATCH 17/28] Don't write to inferior_ptid in corelow.c

[Pedro Alves]

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* corelow.c (core_target::close): Use switch_to_no_thread instead
	of writing to inferior_ptid directly.
	(add_to_thread_list, core_target_open): Use switch_to_thread
	instead of writing to inferior_ptid directly.
---
 gdb/corelow.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/gdb/corelow.c b/gdb/corelow.c
index b60010453d..b6a12c0818 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -160,8 +160,8 @@ core_target::close ()
 {
   if (core_bfd)
     {
-      inferior_ptid = null_ptid;    /* Avoid confusion from thread
-				       stuff.  */
+      switch_to_no_thread ();    /* Avoid confusion from thread
+				    stuff.  */
       exit_inferior_silent (current_inferior ());
 
       /* Clear out solib state while the bfd is still open.  See
@@ -182,7 +182,6 @@ core_target::close ()
 static void
 add_to_thread_list (bfd *abfd, asection *asect, void *reg_sect_arg)
 {
-  ptid_t ptid;
   int core_tid;
   int pid, lwpid;
   asection *reg_sect = (asection *) reg_sect_arg;
@@ -210,15 +209,15 @@ add_to_thread_list (bfd *abfd, asection *asect, void *reg_sect_arg)
       inf->fake_pid_p = fake_pid_p;
     }
 
-  ptid = ptid_t (pid, lwpid, 0);
+  ptid_t ptid (pid, lwpid);
 
-  add_thread (inf->process_target (), ptid);
+  thread_info *thr = add_thread (inf->process_target (), ptid);
 
 /* Warning, Will Robinson, looking at BFD private data! */
 
   if (reg_sect != NULL
       && asect->filepos == reg_sect->filepos)	/* Did we find .reg?  */
-    inferior_ptid = ptid;			/* Yes, make it current.  */
+    switch_to_thread (thr);			/* Yes, make it current.  */
 }
 
 /* Issue a message saying we have no core to debug, if FROM_TTY.  */
@@ -339,7 +338,7 @@ core_target_open (const char *arg, int from_tty)
 
   push_target (std::move (target_holder));
 
-  inferior_ptid = null_ptid;
+  switch_to_no_thread ();
 
   /* Need to flush the register cache (and the frame cache) from a
      previous debug session.  If inferior_ptid ends up the same as the
@@ -368,11 +367,10 @@ core_target_open (const char *arg, int from_tty)
       if (thread == NULL)
 	{
 	  inferior_appeared (current_inferior (), CORELOW_PID);
-	  inferior_ptid = ptid_t (CORELOW_PID);
-	  add_thread_silent (target, inferior_ptid);
+	  thread = add_thread_silent (target, ptid_t (CORELOW_PID));
 	}
-      else
-	switch_to_thread (thread);
+
+      switch_to_thread (thread);
     }
 
   if (exec_bfd == nullptr)
-- 
2.14.5

[PATCH 18/28] Don't write to inferior_ptid in bsd-kvm.c

[Pedro Alves]

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* bsd-kvm.c (bsd_kvm_target::close): Use switch_to_no_thread
	instead of writing to inferior_ptid directly.
---
 gdb/bsd-kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/bsd-kvm.c b/gdb/bsd-kvm.c
index b1b1fee5f4..f35c85a2ea 100644
--- a/gdb/bsd-kvm.c
+++ b/gdb/bsd-kvm.c
@@ -155,7 +155,7 @@ bsd_kvm_target::close ()
       core_kd = NULL;
     }
 
-  inferior_ptid = null_ptid;
+  switch_to_no_thread ();
   exit_inferior_silent (current_inferior ());
 }
 
-- 
2.14.5

[PATCH 19/28] Don't write to inferior_ptid in btrace_fetch

[Pedro Alves]

AFAICT, this isn't required nowadays.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* btrace.c (btrace_fetch): Don't save/restore inferior_ptid.
---
 gdb/btrace.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/gdb/btrace.c b/gdb/btrace.c
index bbf8749649..889507bfd2 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -1908,12 +1908,6 @@ btrace_fetch (struct thread_info *tp, const struct btrace_cpu *cpu)
   if (btinfo->replay != NULL)
     return;
 
-  /* With CLI usage, TP->PTID always equals INFERIOR_PTID here.  Now that we
-     can store a gdb.Record object in Python referring to a different thread
-     than the current one, temporarily set INFERIOR_PTID.  */
-  scoped_restore save_inferior_ptid = make_scoped_restore (&inferior_ptid);
-  inferior_ptid = tp->ptid;
-
   /* We should not be called on running or exited threads.  */
   gdb_assert (can_access_registers_thread (tp));
 
-- 
2.14.5

RE: [PATCH 19/28] Don't write to inferior_ptid in btrace_fetch

[Metzger, Markus T]

Hello Pedro,

> AFAICT, this isn't required nowadays.

How is this working nowadays?  There are still references to inferior_ptid.

Regards,
Markus.
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 19/28] Don't write to inferior_ptid in btrace_fetch

[Pedro Alves]

On 4/15/20 5:52 AM, Metzger, Markus T via Gdb-patches wrote:
> Hello Pedro,
> 
>> AFAICT, this isn't required nowadays.
> 
> How is this working nowadays?  There are still references to inferior_ptid.

Clearly I didn't think this one through.  I put a tp->ptid==inferior_ptid
assertion in place, and that regressed gdb.python/py-record-btrace-threads.exp.
This should mean that the target_read_btrace calls in btrace_fetch can hit
the wrong thread.

So here's an alternative patch.  No regressions when tested with:

 $ make check TESTS="gdb.btrace/*.exp */*btrace*.exp"

From 92c8fa469295b5737e6cb5e882024af2e9872c4c Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 13 Apr 2020 20:02:22 +0100
Subject: [PATCH 19/28] Don't write to inferior_ptid in btrace.c

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* btrace.c (btrace_fetch): Use switch_to_thread instead of writing
	to inferior_ptid.
---
 gdb/btrace.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/gdb/btrace.c b/gdb/btrace.c
index bbf8749649..2964455680 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -1908,13 +1908,13 @@ btrace_fetch (struct thread_info *tp, const struct btrace_cpu *cpu)
   if (btinfo->replay != NULL)
     return;
 
-  /* With CLI usage, TP->PTID always equals INFERIOR_PTID here.  Now that we
-     can store a gdb.Record object in Python referring to a different thread
-     than the current one, temporarily set INFERIOR_PTID.  */
-  scoped_restore save_inferior_ptid = make_scoped_restore (&inferior_ptid);
-  inferior_ptid = tp->ptid;
+  /* With CLI usage, TP is always the current thread when we get here.
+     However, since we can also store a gdb.Record object in Python
+     referring to a different thread than the current one, we need to
+     temporarily set the current thread.  */
+  scoped_restore_current_thread restore_thread;
+  switch_to_thread (tp);
 
   /* We should not be called on running or exited threads.  */
   gdb_assert (can_access_registers_thread (tp));

-- 
2.14.5

RE: [PATCH 19/28] Don't write to inferior_ptid in btrace_fetch

[Metzger, Markus T]

> From 92c8fa469295b5737e6cb5e882024af2e9872c4c Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Mon, 13 Apr 2020 20:02:22 +0100
> Subject: [PATCH 19/28] Don't write to inferior_ptid in btrace.c
> 
> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> 	* btrace.c (btrace_fetch): Use switch_to_thread instead of writing
> 	to inferior_ptid.
> ---
>  gdb/btrace.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)

Looks good to me.

> So here's an alternative patch.  No regressions when tested with:
> 
>  $ make check TESTS="gdb.btrace/*.exp */*btrace*.exp"

And thanks for explicitly running the btrace tests.

Regards,
Markus.

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 20/28] Don't write to inferior_ptid in bsd-kvm.c

[Pedro Alves]

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* bsd-kvm.c (bsd_kvm_target_open): Switch to thread after adding
	it, instead of writing to inferior_ptid.
---
 gdb/bsd-kvm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/bsd-kvm.c b/gdb/bsd-kvm.c
index f35c85a2ea..d40bfe66e4 100644
--- a/gdb/bsd-kvm.c
+++ b/gdb/bsd-kvm.c
@@ -136,8 +136,8 @@ bsd_kvm_target_open (const char *arg, int from_tty)
   core_kd = temp_kd;
   push_target (&bsd_kvm_ops);
 
-  add_thread_silent (&bsd_kvm_ops, bsd_kvm_ptid);
-  inferior_ptid = bsd_kvm_ptid;
+  thread_info *thr = add_thread_silent (&bsd_kvm_ops, bsd_kvm_ptid);
+  switch_to_thread (thr);
 
   target_fetch_registers (get_current_regcache (), -1);
 
-- 
2.14.5

[PATCH 21/28] Don't write to inferior_ptid in fork-child.c

[Pedro Alves]

This is no longer necessary.  All targets that call fork_inferior now
also call switch_to_thread as soon as they add the main thread.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* fork-child.c (postfork_hook): Don't write to inferior_ptid.
---
 gdb/fork-child.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/gdb/fork-child.c b/gdb/fork-child.c
index 41d5e2a0a4..90a01b2b16 100644
--- a/gdb/fork-child.c
+++ b/gdb/fork-child.c
@@ -82,9 +82,6 @@ postfork_hook (pid_t pid)
 
   inferior_appeared (inf, pid);
 
-  /* Needed for wait_for_inferior stuff.  */
-  inferior_ptid = ptid_t (pid);
-
   gdb_assert (saved_ui != NULL);
   current_ui = saved_ui;
   saved_ui = NULL;
-- 
2.14.5

[PATCH 22/28] Don't write to inferior_ptid in go32-nat.c

[Pedro Alves]

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* go32-nat.c (go32_nat_target::create_inferior): Switch to thread
	after creating it, instead of writing to inferior_ptid.  Don't
	write to inferior_ptid.
---
 gdb/go32-nat.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/go32-nat.c b/gdb/go32-nat.c
index 8423001ff6..003a9d5b5b 100644
--- a/gdb/go32-nat.c
+++ b/gdb/go32-nat.c
@@ -753,14 +753,14 @@ go32_nat_target::create_inferior (const char *exec_file,
   save_npx ();
 #endif
 
-  inferior_ptid = ptid_t (SOME_PID);
   inf = current_inferior ();
   inferior_appeared (inf, SOME_PID);
 
   if (!target_is_pushed (this))
     push_target (this);
 
-  add_thread_silent (inferior_ptid);
+  thread_info *thr = add_thread_silent (ptid_t (SOME_PID));
+  switch_to_thread (thr);
 
   clear_proceed_status (0);
   insert_breakpoints ();
-- 
2.14.5

[PATCH 23/28] Don't write to inferior_ptid in remote-sim.c

[Pedro Alves]

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* remote-sim.c (gdbsim_target::create_inferior): Switch to thread
	after creating it, instead of writing to inferior_ptid.
	(gdbsim_target::wait): Don't write to inferior_ptid.
---
 gdb/remote-sim.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/remote-sim.c b/gdb/remote-sim.c
index da1ee96519..9af6486bca 100644
--- a/gdb/remote-sim.c
+++ b/gdb/remote-sim.c
@@ -653,9 +653,10 @@ gdbsim_target::create_inferior (const char *exec_file,
       != SIM_RC_OK)
     error (_("Unable to create sim inferior."));
 
-  inferior_ptid = sim_data->remote_sim_ptid;
-  inferior_appeared (current_inferior (), inferior_ptid.pid ());
-  add_thread_silent (this, inferior_ptid);
+  inferior_appeared (current_inferior (),
+		     sim_data->remote_sim_ptid.pid ());
+  thread_info *thr = add_thread_silent (this, sim_data->remote_sim_ptid);
+  switch_to_thread (thr);
 
   insert_breakpoints ();	/* Needed to get correct instruction
 				   in cache.  */
@@ -945,7 +946,6 @@ gdbsim_target::wait (ptid_t ptid, struct target_waitstatus *status, int options)
       if (sim_data == NULL)
 	error (_("Unable to wait for pid %d.  Inferior not found."),
 	       ptid.pid ());
-      inferior_ptid = ptid;
     }
 
   if (remote_debug)
-- 
2.14.5

Re: [PATCH 23/28] Don't write to inferior_ptid in remote-sim.c

[Simon Marchi]

On 2020-04-14 1:54 p.m., Pedro Alves via Gdb-patches wrote:
> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> 	* remote-sim.c (gdbsim_target::create_inferior): Switch to thread
> 	after creating it, instead of writing to inferior_ptid.
> 	(gdbsim_target::wait): Don't write to inferior_ptid.

I noticed there's 2 "remote-sim.c" patches in this series.  If there's no reason for them
to be separate, I think they should be merged.  Otherwise, they should be given different
subjects.

Simon

Re: [PATCH 23/28] Don't write to inferior_ptid in remote-sim.c

[Pedro Alves]

On 4/16/20 1:53 AM, Simon Marchi wrote:
> On 2020-04-14 1:54 p.m., Pedro Alves via Gdb-patches wrote:
>> gdb/ChangeLog:
>> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
>>
>> 	* remote-sim.c (gdbsim_target::create_inferior): Switch to thread
>> 	after creating it, instead of writing to inferior_ptid.
>> 	(gdbsim_target::wait): Don't write to inferior_ptid.
> 
> I noticed there's 2 "remote-sim.c" patches in this series.  If there's no reason for them
> to be separate, I think they should be merged.  Otherwise, they should be given different
> subjects.
> 

Whoops, thanks for noticing.  There's no reason for them to be separate.
Just wrote them in different passes and didn't notice it.

Here's what they look like merged as one.

From 36fd318f59ba41310d0820953a37743fc989c927 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 16 Apr 2020 15:54:13 +0100
Subject: [PATCH] Don't write to inferior_ptid in remote-sim.c

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* remote-sim.c (gdbsim_target::create_inferior): Switch to thread
	after creating it, instead of writing to inferior_ptid.
	(gdbsim_target_open): Use switch_to_no_thread instead of writing
	to inferior_ptid directly.
	(gdbsim_target::wait): Don't write to inferior_ptid.
---
 gdb/remote-sim.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/gdb/remote-sim.c b/gdb/remote-sim.c
index 347dfd7013..9af6486bca 100644
--- a/gdb/remote-sim.c
+++ b/gdb/remote-sim.c
@@ -653,9 +653,10 @@ gdbsim_target::create_inferior (const char *exec_file,
       != SIM_RC_OK)
     error (_("Unable to create sim inferior."));
 
-  inferior_ptid = sim_data->remote_sim_ptid;
-  inferior_appeared (current_inferior (), inferior_ptid.pid ());
-  add_thread_silent (this, inferior_ptid);
+  inferior_appeared (current_inferior (),
+		     sim_data->remote_sim_ptid.pid ());
+  thread_info *thr = add_thread_silent (this, sim_data->remote_sim_ptid);
+  switch_to_thread (thr);
 
   insert_breakpoints ();	/* Needed to get correct instruction
 				   in cache.  */
@@ -761,7 +762,7 @@ gdbsim_target_open (const char *args, int from_tty)
 
   /* There's nothing running after "target sim" or "load"; not until
      "run".  */
-  inferior_ptid = null_ptid;
+  switch_to_no_thread ();
 
   gdbsim_is_open = 1;
 }
@@ -945,7 +946,6 @@ gdbsim_target::wait (ptid_t ptid, struct target_waitstatus *status, int options)
       if (sim_data == NULL)
 	error (_("Unable to wait for pid %d.  Inferior not found."),
 	       ptid.pid ());
-      inferior_ptid = ptid;
     }
 
   if (remote_debug)

-- 
2.14.5

[PATCH 24/28] Don't write to inferior_ptid in windows-nat.c, part I

[Pedro Alves]

The inferior_ptid hack in do_initial_win32_stuff, added back in 2008:

  https://sourceware.org/ml/gdb-patches/2008-10/msg00012.html

with:

  commit 9f9d052e600ed9436f9fd558d62a189c8cc3d43e
  Author:     Pierre Muller <muller@sourceware.org>
  AuthorDate: Thu Oct 2 14:20:07 2008 +0000

	      * win32-nat.c (do_initial_win32_stuff): Set inferior_ptid.

is no longer needed.  Back then, current_inferior looked like this:

  struct inferior*
  current_inferior (void)
  {
    struct inferior *inf = find_inferior_pid (ptid_get_pid (inferior_ptid));
    gdb_assert (inf);
    return inf;
  }

Nowadays, current_inferior() just returns the global current_inferior_
pointer, which didn't exist back then.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* windows-nat.c (do_initial_windows_stuff): No longer set inferior_ptid.
---
 gdb/windows-nat.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 881240c693..1bbebf00b9 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -1985,12 +1985,6 @@ do_initial_windows_stuff (struct target_ops *ops, DWORD pid, int attaching)
   inferior_appeared (inf, pid);
   inf->attach_flag = attaching;
 
-  /* Make the new process the current inferior, so terminal handling
-     can rely on it.  When attaching, we don't know about any thread
-     id here, but that's OK --- nothing should be referencing the
-     current thread until we report an event out of windows_wait.  */
-  inferior_ptid = ptid_t (pid);
-
   target_terminal::init ();
   target_terminal::inferior ();
 
-- 
2.14.5

[PATCH 25/28] Don't write to inferior_ptid in windows-nat.c, part II

[Pedro Alves]

Writing to inferior_ptid in
windows_nat_target::get_windows_debug_event is just incorrect and not
necessary.  We'll report the event to GDB's core, which then takes
care of switching inferior_ptid / current thread.

Related (see windows_nat_target::get_windows_debug_event), there's
also a "current_windows_thread" global that is just begging to get out
of sync with core GDB's current thread.  This patch removes it.
gdbserver already does not have an equivalent global in win32-low.cc.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* nat/windows-nat.c (current_windows_thread): Remove.
	* nat/windows-nat.h (current_windows_thread): Remove.
	* windows-nat.c (windows_nat_target::stopped_by_sw_breakpoint):
	Adjust.
	(display_selectors): Adjust to fetch the current
	windows_thread_info based on inferior_ptid.
	(fake_create_process): No longer write to current_windows_thread.
	(windows_nat_target::get_windows_debug_event):
	Don't set inferior_ptid or current_windows_thread.
	(windows_nat_target::wait): Adjust to not rely on
	current_windows_thread.
	(windows_nat_target::detach): Use switch_to_no_thread instead of
	writing to inferior_ptid directly.
---
 gdb/nat/windows-nat.c |  1 -
 gdb/nat/windows-nat.h |  3 ---
 gdb/windows-nat.c     | 62 ++++++++++++++++++++++++++-------------------------
 3 files changed, 32 insertions(+), 34 deletions(-)

diff --git a/gdb/nat/windows-nat.c b/gdb/nat/windows-nat.c
index cd7c1d177c..a382b9edb9 100644
--- a/gdb/nat/windows-nat.c
+++ b/gdb/nat/windows-nat.c
@@ -36,7 +36,6 @@ DEBUG_EVENT current_event;
    ContinueDebugEvent.  */
 static DEBUG_EVENT last_wait_event;
 
-windows_thread_info *current_windows_thread;
 DWORD desired_stop_thread_id = -1;
 std::vector<pending_stop> pending_stops;
 EXCEPTION_RECORD siginfo_er;
diff --git a/gdb/nat/windows-nat.h b/gdb/nat/windows-nat.h
index aea1519672..dc27827d9c 100644
--- a/gdb/nat/windows-nat.h
+++ b/gdb/nat/windows-nat.h
@@ -168,9 +168,6 @@ extern enum gdb_signal last_sig;
    stop.  */
 extern DEBUG_EVENT current_event;
 
-/* Info on currently selected thread */
-extern windows_thread_info *current_windows_thread;
-
 /* The ID of the thread for which we anticipate a stop event.
    Normally this is -1, meaning we'll accept an event in any
    thread.  */
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 1bbebf00b9..78e7e31c67 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -317,7 +317,9 @@ struct windows_nat_target final : public x86_nat_target<inf_child_target>
 
   bool stopped_by_sw_breakpoint () override
   {
-    return current_windows_thread->stopped_at_software_breakpoint;
+    windows_thread_info *th
+      = thread_rec (inferior_ptid, DONT_INVALIDATE_CONTEXT);
+    return th->stopped_at_software_breakpoint;
   }
 
   bool supports_stopped_by_sw_breakpoint () override
@@ -1125,11 +1127,15 @@ display_selector (HANDLE thread, DWORD sel)
 static void
 display_selectors (const char * args, int from_tty)
 {
-  if (!current_windows_thread)
+  if (inferior_ptid == null_ptid)
     {
       puts_filtered ("Impossible to display selectors now.\n");
       return;
     }
+
+  windows_thread_info *current_windows_thread
+    = thread_rec (inferior_ptid, DONT_INVALIDATE_CONTEXT);
+
   if (!args)
     {
 #ifdef __x86_64__
@@ -1338,12 +1344,11 @@ fake_create_process (void)
        (unsigned) GetLastError ());
       /*  We can not debug anything in that case.  */
     }
-  current_windows_thread
-    = windows_add_thread (ptid_t (current_event.dwProcessId,
-				  current_event.dwThreadId, 0),
-			  current_event.u.CreateThread.hThread,
-			  current_event.u.CreateThread.lpThreadLocalBase,
-			  true /* main_thread_p */);
+  windows_add_thread (ptid_t (current_event.dwProcessId, 0,
+			      current_event.dwThreadId),
+		      current_event.u.CreateThread.hThread,
+		      current_event.u.CreateThread.lpThreadLocalBase,
+		      true /* main_thread_p */);
   return current_event.dwThreadId;
 }
 
@@ -1516,10 +1521,9 @@ windows_nat_target::get_windows_debug_event (int pid,
       thread_id = stop->thread_id;
       *ourstatus = stop->status;
 
-      inferior_ptid = ptid_t (current_event.dwProcessId, thread_id, 0);
-      current_windows_thread = thread_rec (inferior_ptid,
-					   INVALIDATE_CONTEXT);
-      current_windows_thread->reload_context = 1;
+      ptid_t ptid (current_event.dwProcessId, thread_id);
+      th = thread_rec (ptid, INVALIDATE_CONTEXT);
+      th->reload_context = 1;
 
       return thread_id;
     }
@@ -1734,14 +1738,6 @@ windows_nat_target::get_windows_debug_event (int pid,
       thread_id = 0;
       CHECK (windows_continue (continue_status, desired_stop_thread_id, 0));
     }
-  else
-    {
-      inferior_ptid = ptid_t (current_event.dwProcessId, thread_id, 0);
-      current_windows_thread = th;
-      if (!current_windows_thread)
-	current_windows_thread = thread_rec (inferior_ptid,
-					     INVALIDATE_CONTEXT);
-    }
 
 out:
   return thread_id;
@@ -1798,16 +1794,22 @@ windows_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 	{
 	  ptid_t result = ptid_t (current_event.dwProcessId, retval, 0);
 
-	  if (current_windows_thread != nullptr)
+	  if (ourstatus->kind != TARGET_WAITKIND_EXITED
+	      && ourstatus->kind !=  TARGET_WAITKIND_SIGNALLED)
 	    {
-	      current_windows_thread->stopped_at_software_breakpoint = false;
-	      if (current_event.dwDebugEventCode == EXCEPTION_DEBUG_EVENT
-		  && ((current_event.u.Exception.ExceptionRecord.ExceptionCode
-		       == EXCEPTION_BREAKPOINT)
-		      || (current_event.u.Exception.ExceptionRecord.ExceptionCode
-			  == STATUS_WX86_BREAKPOINT))
-		  && windows_initialization_done)
-		current_windows_thread->stopped_at_software_breakpoint = true;
+	      windows_thread_info *th = thread_rec (result, INVALIDATE_CONTEXT);
+
+	      if (th != nullptr)
+		{
+		  th->stopped_at_software_breakpoint = false;
+		  if (current_event.dwDebugEventCode == EXCEPTION_DEBUG_EVENT
+		      && ((current_event.u.Exception.ExceptionRecord.ExceptionCode
+			   == EXCEPTION_BREAKPOINT)
+			  || (current_event.u.Exception.ExceptionRecord.ExceptionCode
+			      == STATUS_WX86_BREAKPOINT))
+		      && windows_initialization_done)
+		    th->stopped_at_software_breakpoint = true;
+		}
 	    }
 
 	  return result;
@@ -2166,7 +2168,7 @@ windows_nat_target::detach (inferior *inf, int from_tty)
     }
 
   x86_cleanup_dregs ();
-  inferior_ptid = null_ptid;
+  switch_to_no_thread ();
   detach_inferior (inf);
 
   maybe_unpush_target ();
-- 
2.14.5

Re: [PATCH 25/28] Don't write to inferior_ptid in windows-nat.c, part II

[Hannes Domani]

 I did my tests now, thanks for your git branch.

Am Dienstag, 14. April 2020, 19:55:30 MESZ hat Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> Folgendes geschrieben:

> Writing to inferior_ptid in
> windows_nat_target::get_windows_debug_event is just incorrect and not
> necessary.  We'll report the event to GDB's core, which then takes
> care of switching inferior_ptid / current thread.
>
> Related (see windows_nat_target::get_windows_debug_event), there's
> also a "current_windows_thread" global that is just begging to get out
> of sync with core GDB's current thread.  This patch removes it.
> gdbserver already does not have an equivalent global in win32-low.cc.
>
> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
>
>     * nat/windows-nat.c (current_windows_thread): Remove.
>     * nat/windows-nat.h (current_windows_thread): Remove.
>     * windows-nat.c (windows_nat_target::stopped_by_sw_breakpoint):
>     Adjust.
>     (display_selectors): Adjust to fetch the current
>     windows_thread_info based on inferior_ptid.
>     (fake_create_process): No longer write to current_windows_thread.
>     (windows_nat_target::get_windows_debug_event):
>     Don't set inferior_ptid or current_windows_thread.
>     (windows_nat_target::wait): Adjust to not rely on
>     current_windows_thread.
>     (windows_nat_target::detach): Use switch_to_no_thread instead of
>     writing to inferior_ptid directly.
> ---
> gdb/nat/windows-nat.c |  1 -
> gdb/nat/windows-nat.h |  3 ---
> gdb/windows-nat.c    | 62 ++++++++++++++++++++++++++-------------------------
> 3 files changed, 32 insertions(+), 34 deletions(-)
>
> diff --git a/gdb/nat/windows-nat.c b/gdb/nat/windows-nat.c
> index cd7c1d177c..a382b9edb9 100644
> --- a/gdb/nat/windows-nat.c
> +++ b/gdb/nat/windows-nat.c
> @@ -36,7 +36,6 @@ DEBUG_EVENT current_event;
>     ContinueDebugEvent.  */
> static DEBUG_EVENT last_wait_event;
>
> -windows_thread_info *current_windows_thread;
> DWORD desired_stop_thread_id = -1;
> std::vector<pending_stop> pending_stops;
> EXCEPTION_RECORD siginfo_er;
> diff --git a/gdb/nat/windows-nat.h b/gdb/nat/windows-nat.h
> index aea1519672..dc27827d9c 100644
> --- a/gdb/nat/windows-nat.h
> +++ b/gdb/nat/windows-nat.h
> @@ -168,9 +168,6 @@ extern enum gdb_signal last_sig;
>     stop.  */
> extern DEBUG_EVENT current_event;
>
> -/* Info on currently selected thread */
> -extern windows_thread_info *current_windows_thread;
> -
> /* The ID of the thread for which we anticipate a stop event.
>     Normally this is -1, meaning we'll accept an event in any
>     thread.  */
> diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
> index 1bbebf00b9..78e7e31c67 100644
> --- a/gdb/windows-nat.c
> +++ b/gdb/windows-nat.c
> @@ -317,7 +317,9 @@ struct windows_nat_target final : public x86_nat_target<inf_child_target>
>
>   bool stopped_by_sw_breakpoint () override
>   {
> -    return current_windows_thread->stopped_at_software_breakpoint;
> +    windows_thread_info *th
> +      = thread_rec (inferior_ptid, DONT_INVALIDATE_CONTEXT);
> +    return th->stopped_at_software_breakpoint;
>   }
>
>   bool supports_stopped_by_sw_breakpoint () override
> @@ -1125,11 +1127,15 @@ display_selector (HANDLE thread, DWORD sel)
> static void
> display_selectors (const char * args, int from_tty)
> {
> -  if (!current_windows_thread)
> +  if (inferior_ptid == null_ptid)
>     {
>       puts_filtered ("Impossible to display selectors now.\n");
>       return;
>     }
> +
> +  windows_thread_info *current_windows_thread
> +    = thread_rec (inferior_ptid, DONT_INVALIDATE_CONTEXT);
> +
>   if (!args)
>     {
> #ifdef __x86_64__
> @@ -1338,12 +1344,11 @@ fake_create_process (void)
>         (unsigned) GetLastError ());
>       /*  We can not debug anything in that case.  */
>     }
> -  current_windows_thread
> -    = windows_add_thread (ptid_t (current_event.dwProcessId,
> -                  current_event.dwThreadId, 0),
> -              current_event.u.CreateThread.hThread,
> -              current_event.u.CreateThread.lpThreadLocalBase,
> -              true /* main_thread_p */);
> +  windows_add_thread (ptid_t (current_event.dwProcessId, 0,
> +                  current_event.dwThreadId),
> +              current_event.u.CreateThread.hThread,
> +              current_event.u.CreateThread.lpThreadLocalBase,
> +              true /* main_thread_p */);
>   return current_event.dwThreadId;
> }
>
> @@ -1516,10 +1521,9 @@ windows_nat_target::get_windows_debug_event (int pid,
>       thread_id = stop->thread_id;
>       *ourstatus = stop->status;
>
> -      inferior_ptid = ptid_t (current_event.dwProcessId, thread_id, 0);
> -      current_windows_thread = thread_rec (inferior_ptid,
> -                      INVALIDATE_CONTEXT);
> -      current_windows_thread->reload_context = 1;
> +      ptid_t ptid (current_event.dwProcessId, thread_id);
> +      th = thread_rec (ptid, INVALIDATE_CONTEXT);
> +      th->reload_context = 1;
>
>       return thread_id;
>     }
> @@ -1734,14 +1738,6 @@ windows_nat_target::get_windows_debug_event (int pid,
>       thread_id = 0;
>       CHECK (windows_continue (continue_status, desired_stop_thread_id, 0));
>     }
> -  else
> -    {
> -      inferior_ptid = ptid_t (current_event.dwProcessId, thread_id, 0);
> -      current_windows_thread = th;
> -      if (!current_windows_thread)
> -    current_windows_thread = thread_rec (inferior_ptid,
> -                        INVALIDATE_CONTEXT);
> -    }
>
> out:
>   return thread_id;

The removal of these lines breaks debugging on Windows, right on startup I get:

(gdb) r
Starting program: C:\qiewer\heob\heob64.exe
C:/src/repos/binutils-gdb.git/gdb/thread.c:86: internal-error: thread_info* inferior_thread(): Assertion `tp' failed.

Full stacktrace of the assertion failure:

(gdb) bt
#0  internal_error (file=0xa713a8 <selftests::test_target_info+168> "C:/src/repos/binutils-gdb.git/gdb/thread.c", line=86,
    fmt=0xa71388 <selftests::test_target_info+136> "%s: Assertion `%s' failed.") at C:/src/repos/binutils-gdb.git/gdbsupport/errors.cc:51
#1  0x000000000066cfd9 in inferior_thread () at C:/src/repos/binutils-gdb.git/gdb/thread.c:86
#2  inferior_thread () at C:/src/repos/binutils-gdb.git/gdb/thread.c:83
#3  0x00000000005603a0 in post_create_inferior (target=0x999ba0 <the_windows_nat_target>, from_tty=from_tty@entry=0)
    at C:/src/repos/binutils-gdb.git/gdb/infcmd.c:444
#4  0x0000000000563727 in run_command_1 (args=<optimized out>, from_tty=1, run_how=RUN_NORMAL) at C:/src/repos/binutils-gdb.git/gdb/infcmd.c:670
#5  0x00000000004810c2 in cmd_func (cmd=0xa713a8 <selftests::test_target_info+168>, args=0x56 <error: Cannot access memory at address 0x56>,
    from_tty=10949512) at C:/src/repos/binutils-gdb.git/gdb/cli/cli-decode.c:1952
#6  0x00000000006735a8 in execute_command (p=<optimized out>, p@entry=0x13bae0 "", from_tty=1) at C:/src/repos/binutils-gdb.git/gdb/top.c:655
#7  0x00000000005153f4 in command_handler (command=0x13bae0 "") at C:/src/repos/binutils-gdb.git/gdb/event-top.c:588
#8  0x00000000005162a2 in command_line_handler (rl=...) at C:/src/repos/binutils-gdb.git/gdb/event-top.c:773
#9  0x0000000000515bd3 in gdb_rl_callback_handler (rl=0x13b4c0 "r") at c:/msys64/mingw64/x86_64-w64-mingw32/include/c++/9.3.0/bits/unique_ptr.h:153
#10 0x00000000006e4eec in rl_callback_read_char () at C:/src/repos/binutils-gdb.git/readline/readline/callback.c:281
#11 0x0000000000514f4e in gdb_rl_callback_read_char_wrapper_noexcept () at C:/src/repos/binutils-gdb.git/gdb/event-top.c:177
#12 0x0000000000515a84 in gdb_rl_callback_read_char_wrapper (client_data=<optimized out>) at C:/src/repos/binutils-gdb.git/gdb/event-top.c:193
#13 0x0000000000514d92 in stdin_event_handler (error=<optimized out>, client_data=0x13b720) at C:/src/repos/binutils-gdb.git/gdb/event-top.c:516
#14 0x00000000007a91e0 in handle_file_event (ready_mask=2, file_ptr=0x1e93c0) at C:/src/repos/binutils-gdb.git/gdbsupport/event-loop.cc:548
#15 gdb_wait_for_event (block=<optimized out>) at C:/src/repos/binutils-gdb.git/gdbsupport/event-loop.cc:698
#16 gdb_wait_for_event (block=<optimized out>) at C:/src/repos/binutils-gdb.git/gdbsupport/event-loop.cc:561
#17 0x00000000007a9318 in gdb_do_one_event () at C:/src/repos/binutils-gdb.git/gdbsupport/event-loop.cc:215
#18 0x0000000000592c4d in start_event_loop () at C:/src/repos/binutils-gdb.git/gdb/main.c:356
#19 captured_command_loop () at C:/src/repos/binutils-gdb.git/gdb/main.c:416
#20 0x0000000000594b55 in captured_main (data=0xfd4fdc0) at C:/src/repos/binutils-gdb.git/gdb/main.c:1254
#21 gdb_main (args=args@entry=0xfd4fe20) at C:/src/repos/binutils-gdb.git/gdb/main.c:1269
#22 0x000000000098a757 in main (argc=3, argv=0x1048a0) at C:/src/repos/binutils-gdb.git/gdb/gdb.c:32
(gdb) up
#1  0x000000000066cfd9 in inferior_thread () at C:/src/repos/binutils-gdb.git/gdb/thread.c:86
86        gdb_assert (tp);
(gdb) l
81
82      struct thread_info*
83      inferior_thread (void)
84      {
85        struct thread_info *tp = find_thread_ptid (current_inferior (), inferior_ptid);
86        gdb_assert (tp);
87        return tp;
88      }
89
90      /* Delete the breakpoint pointed at by BP_P, if there's one.  */


This replacement works fine for me so far:

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 78e7e31c67..b22df182a1 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -1738,6 +1738,12 @@ windows_nat_target::get_windows_debug_event (int pid,
       thread_id = 0;
       CHECK (windows_continue (continue_status, desired_stop_thread_id, 0));
     }
+  else
+    {
+      if (th != nullptr)
+       switch_to_thread (&the_windows_nat_target,
+                         ptid_t (current_event.dwProcessId, thread_id, 0));
+    }

 out:
   return thread_id;


Btw., I think this variable in get_windows_debug_event is no longer useful:
  static windows_thread_info dummy_thread_info (0, 0, 0);


Hannes

Re: [PATCH 25/28] Don't write to inferior_ptid in windows-nat.c, part II

[Pedro Alves]

On 4/14/20 11:41 PM, Hannes Domani via Gdb-patches wrote:

> The removal of these lines breaks debugging on Windows, right on startup I get:
> 
> (gdb) r
> Starting program: C:\qiewer\heob\heob64.exe
> C:/src/repos/binutils-gdb.git/gdb/thread.c:86: internal-error: thread_info* inferior_thread(): Assertion `tp' failed.
> 
> Full stacktrace of the assertion failure:
> 
> (gdb) bt
> #0  internal_error (file=0xa713a8 <selftests::test_target_info+168> "C:/src/repos/binutils-gdb.git/gdb/thread.c", line=86,
>     fmt=0xa71388 <selftests::test_target_info+136> "%s: Assertion `%s' failed.") at C:/src/repos/binutils-gdb.git/gdbsupport/errors.cc:51
> #1  0x000000000066cfd9 in inferior_thread () at C:/src/repos/binutils-gdb.git/gdb/thread.c:86
> #2  inferior_thread () at C:/src/repos/binutils-gdb.git/gdb/thread.c:83
> #3  0x00000000005603a0 in post_create_inferior (target=0x999ba0 <the_windows_nat_target>, from_tty=from_tty@entry=0)
>     at C:/src/repos/binutils-gdb.git/gdb/infcmd.c:444
> #4  0x0000000000563727 in run_command_1 (args=<optimized out>, from_tty=1, run_how=RUN_NORMAL) at C:/src/repos/binutils-gdb.git/gdb/infcmd.c:670

> Btw., I think this variable in get_windows_debug_event is no longer useful:
>   static windows_thread_info dummy_thread_info (0, 0, 0);
> 

Thanks for pointing that out.

> 
> This replacement works fine for me so far:

Does this instead work for you?  This adds a single
switch_to_thread at the end of the initialization.

From 00648f5a0bd7b8d85a2bdf49a2d8f71087826062 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 15 Apr 2020 15:52:32 +0100
Subject: [PATCH] Fix Windows

---
 gdb/windows-nat.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 78e7e31c67..dd15fa1fab 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -358,6 +358,8 @@ struct windows_nat_target final : public x86_nat_target<inf_child_target>
   const char *thread_name (struct thread_info *) override;
 
   int get_windows_debug_event (int pid, struct target_waitstatus *ourstatus);
+
+  void do_initial_windows_stuff (DWORD pid, bool attaching);
 };
 
 static windows_nat_target the_windows_nat_target;
@@ -1508,8 +1510,6 @@ windows_nat_target::get_windows_debug_event (int pid,
 {
   BOOL debug_event;
   DWORD continue_status, event_code;
-  windows_thread_info *th;
-  static windows_thread_info dummy_thread_info (0, 0, 0);
   DWORD thread_id = 0;
 
   /* If there is a relevant pending stop, report it now.  See the
@@ -1522,7 +1522,7 @@ windows_nat_target::get_windows_debug_event (int pid,
       *ourstatus = stop->status;
 
       ptid_t ptid (current_event.dwProcessId, thread_id);
-      th = thread_rec (ptid, INVALIDATE_CONTEXT);
+      windows_thread_info *th = thread_rec (ptid, INVALIDATE_CONTEXT);
       th->reload_context = 1;
 
       return thread_id;
@@ -1537,7 +1537,6 @@ windows_nat_target::get_windows_debug_event (int pid,
 
   event_code = current_event.dwDebugEventCode;
   ourstatus->kind = TARGET_WAITKIND_SPURIOUS;
-  th = NULL;
   have_saved_context = 0;
 
   switch (event_code)
@@ -1563,7 +1562,7 @@ windows_nat_target::get_windows_debug_event (int pid,
 	}
       /* Record the existence of this thread.  */
       thread_id = current_event.dwThreadId;
-      th = windows_add_thread
+      windows_add_thread
         (ptid_t (current_event.dwProcessId, current_event.dwThreadId, 0),
 	 current_event.u.CreateThread.hThread,
 	 current_event.u.CreateThread.lpThreadLocalBase,
@@ -1580,7 +1579,6 @@ windows_nat_target::get_windows_debug_event (int pid,
 				     current_event.dwThreadId, 0),
 			     current_event.u.ExitThread.dwExitCode,
 			     false /* main_thread_p */);
-      th = &dummy_thread_info;
       break;
 
     case CREATE_PROCESS_DEBUG_EVENT:
@@ -1594,7 +1592,7 @@ windows_nat_target::get_windows_debug_event (int pid,
 
       current_process_handle = current_event.u.CreateProcessInfo.hProcess;
       /* Add the main thread.  */
-      th = windows_add_thread
+      windows_add_thread
         (ptid_t (current_event.dwProcessId,
 		 current_event.dwThreadId, 0),
 	 current_event.u.CreateProcessInfo.hThread,
@@ -1731,7 +1729,7 @@ windows_nat_target::get_windows_debug_event (int pid,
 	  && windows_initialization_done)
 	{
 	  ptid_t ptid = ptid_t (current_event.dwProcessId, thread_id, 0);
-	  th = thread_rec (ptid, INVALIDATE_CONTEXT);
+	  windows_thread_info *th = thread_rec (ptid, INVALIDATE_CONTEXT);
 	  th->stopped_at_software_breakpoint = true;
 	}
       pending_stops.push_back ({thread_id, *ourstatus, current_event});
@@ -1944,8 +1942,8 @@ windows_add_all_dlls (void)
     }
 }
 
-static void
-do_initial_windows_stuff (struct target_ops *ops, DWORD pid, int attaching)
+void
+windows_nat_target::do_initial_windows_stuff (DWORD pid, bool attaching)
 {
   int i;
   struct inferior *inf;
@@ -1961,8 +1959,8 @@ do_initial_windows_stuff (struct target_ops *ops, DWORD pid, int attaching)
 #endif
   current_event.dwProcessId = pid;
   memset (&current_event, 0, sizeof (current_event));
-  if (!target_is_pushed (ops))
-    push_target (ops);
+  if (!target_is_pushed (this))
+    push_target (this);
   disable_breakpoints_in_shlibs ();
   windows_clear_solib ();
   clear_proceed_status (0);
@@ -1992,11 +1990,13 @@ do_initial_windows_stuff (struct target_ops *ops, DWORD pid, int attaching)
 
   windows_initialization_done = 0;
 
+  ptid_t last_ptid;
+
   while (1)
     {
       struct target_waitstatus status;
 
-      ops->wait (minus_one_ptid, &status, 0);
+      last_ptid = this->wait (minus_one_ptid, &status, 0);
 
       /* Note windows_wait returns TARGET_WAITKIND_SPURIOUS for thread
 	 events.  */
@@ -2004,9 +2004,11 @@ do_initial_windows_stuff (struct target_ops *ops, DWORD pid, int attaching)
 	  && status.kind != TARGET_WAITKIND_SPURIOUS)
 	break;
 
-      ops->resume (minus_one_ptid, 0, GDB_SIGNAL_0);
+      this->resume (minus_one_ptid, 0, GDB_SIGNAL_0);
     }
 
+  switch_to_thread (find_thread_ptid (this, last_ptid));
+
   /* Now that the inferior has been started and all DLLs have been mapped,
      we can iterate over all DLLs and load them in.
 
@@ -2138,7 +2140,7 @@ windows_nat_target::attach (const char *args, int from_tty)
     }
 #endif
 
-  do_initial_windows_stuff (this, pid, 1);
+  do_initial_windows_stuff (pid, 1);
   target_terminal::ours ();
 }
 
@@ -2978,9 +2980,8 @@ windows_nat_target::create_inferior (const char *exec_file,
   else
     saw_create = 0;
 
-  do_initial_windows_stuff (this, pi.dwProcessId, 0);
+  do_initial_windows_stuff (pi.dwProcessId, 0);
 
   /* windows_continue (DBG_CONTINUE, -1, 0); */
 }

-- 
2.14.5

Re: [PATCH 25/28] Don't write to inferior_ptid in windows-nat.c, part II

[Hannes Domani]

 Am Mittwoch, 15. April 2020, 17:08:27 MESZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben:

> On 4/14/20 11:41 PM, Hannes Domani via Gdb-patches wrote:
>
> > The removal of these lines breaks debugging on Windows, right on startup I get:
> >
> > (gdb) r
> > Starting program: C:\qiewer\heob\heob64.exe
> > C:/src/repos/binutils-gdb.git/gdb/thread.c:86: internal-error: thread_info* inferior_thread(): Assertion `tp' failed.
> >
> > Full stacktrace of the assertion failure:
> >
> > (gdb) bt
> > #0  internal_error (file=0xa713a8 <selftests::test_target_info+168> "C:/src/repos/binutils-gdb.git/gdb/thread.c", line=86,
> >     fmt=0xa71388 <selftests::test_target_info+136> "%s: Assertion `%s' failed.") at C:/src/repos/binutils-gdb.git/gdbsupport/errors.cc:51
> > #1  0x000000000066cfd9 in inferior_thread () at C:/src/repos/binutils-gdb.git/gdb/thread.c:86
> > #2  inferior_thread () at C:/src/repos/binutils-gdb.git/gdb/thread.c:83
> > #3  0x00000000005603a0 in post_create_inferior (target=0x999ba0 <the_windows_nat_target>, from_tty=from_tty@entry=0)
> >     at C:/src/repos/binutils-gdb.git/gdb/infcmd.c:444
> > #4  0x0000000000563727 in run_command_1 (args=<optimized out>, from_tty=1, run_how=RUN_NORMAL) at C:/src/repos/binutils-gdb.git/gdb/infcmd.c:670
>
> > Btw., I think this variable in get_windows_debug_event is no longer useful:
> >  static windows_thread_info dummy_thread_info (0, 0, 0);
> >
>
> Thanks for pointing that out.
>
> >
> > This replacement works fine for me so far:
>
> Does this instead work for you?  This adds a single
> switch_to_thread at the end of the initialization.

Yes, this also works for me.


Hannes

[PATCH 26/28] Don't write to inferior_ptid in ravenscar-thread.c

[Pedro Alves]

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* ravenscar-thread.c (ravenscar_thread_target): Update.
	(ravenscar_thread_target::update_inferior_ptid): Rename to ...
	(ravenscar_thread_target::add_active_thread): ... this.  Don't
	set m_base_ptid here.  Update to avoid referencing inferior_ptid.
	(ravenscar_thread_target::wait): Don't write to inferior_ptid.
---
 gdb/ravenscar-thread.c | 49 ++++++++++++++++++++++++-------------------------
 1 file changed, 24 insertions(+), 25 deletions(-)

diff --git a/gdb/ravenscar-thread.c b/gdb/ravenscar-thread.c
index fd3beb03ec..93e02b1f40 100644
--- a/gdb/ravenscar-thread.c
+++ b/gdb/ravenscar-thread.c
@@ -77,8 +77,11 @@ static const target_info ravenscar_target_info = {
 struct ravenscar_thread_target final : public target_ops
 {
   ravenscar_thread_target ()
+    : m_base_ptid (inferior_ptid)
   {
-    update_inferior_ptid ();
+    thread_info *thr = add_active_thread ();
+    if (thr != nullptr)
+      switch_to_thread (thr);
   }
 
   const target_info &info () const override
@@ -126,9 +129,9 @@ struct ravenscar_thread_target final : public target_ops
   /* PTID of the last thread that received an event.
      This can be useful to determine the associated task that received
      the event, to make it the current task.  */
-  ptid_t m_base_ptid = null_ptid;
+  ptid_t m_base_ptid;
 
-  void update_inferior_ptid ();
+  thread_info *add_active_thread ();
   ptid_t active_task (int cpu);
   bool task_is_currently_active (ptid_t ptid);
   bool runtime_initialized ();
@@ -213,37 +216,36 @@ get_base_thread_from_ravenscar_task (ptid_t ptid)
   return ptid_t (ptid.pid (), base_cpu, 0);
 }
 
-/* Fetch the ravenscar running thread from target memory and
-   update inferior_ptid accordingly.  */
+/* Fetch the ravenscar running thread from target memory, make sure
+   there's a corresponding thread in the thread list, and return it.
+   If the runtime is not initialized, return NULL.  */
 
-void
-ravenscar_thread_target::update_inferior_ptid ()
+thread_info *
+ravenscar_thread_target::add_active_thread ()
 {
   process_stratum_target *proc_target
     = as_process_stratum_target (this->beneath ());
 
   int base_cpu;
 
-  m_base_ptid = inferior_ptid;
-
-  gdb_assert (!is_ravenscar_task (inferior_ptid));
+  gdb_assert (!is_ravenscar_task (m_base_ptid));
   base_cpu = ravenscar_get_thread_base_cpu (m_base_ptid);
 
-  /* If the runtime has not been initialized yet, the inferior_ptid is
-     the only ptid that there is.  */
   if (!runtime_initialized ())
-    return;
+    return nullptr;
 
   /* Make sure we set m_base_ptid before calling active_task
      as the latter relies on it.  */
-  inferior_ptid = active_task (base_cpu);
-  gdb_assert (inferior_ptid != null_ptid);
+  ptid_t active_ptid = active_task (base_cpu);
+  gdb_assert (active_ptid != null_ptid);
 
   /* The running thread may not have been added to
      system.tasking.debug's list yet; so ravenscar_update_thread_list
      may not always add it to the thread list.  Add it here.  */
-  if (!find_thread_ptid (proc_target, inferior_ptid))
-    add_thread (proc_target, inferior_ptid);
+  thread_info *active_thr = find_thread_ptid (proc_target, active_ptid);
+  if (active_thr == nullptr)
+    active_thr = add_thread (proc_target, active_ptid);
+  return active_thr;
 }
 
 /* The Ravenscar Runtime exports a symbol which contains the ID of
@@ -343,12 +345,11 @@ ravenscar_thread_target::wait (ptid_t ptid,
     = as_process_stratum_target (this->beneath ());
   ptid_t event_ptid;
 
-  inferior_ptid = m_base_ptid;
   if (ptid != minus_one_ptid)
     ptid = m_base_ptid;
   event_ptid = beneath->wait (ptid, status, 0);
-  /* Find any new threads that might have been created, and update
-     inferior_ptid to the active thread.
+  /* Find any new threads that might have been created, and return the
+     active thread.
 
      Only do it if the program is still alive, though.  Otherwise,
      this causes problems when debugging through the remote protocol,
@@ -357,13 +358,11 @@ ravenscar_thread_target::wait (ptid_t ptid,
   if (status->kind != TARGET_WAITKIND_EXITED
       && status->kind != TARGET_WAITKIND_SIGNALLED)
     {
-      inferior_ptid = event_ptid;
+      m_base_ptid = event_ptid;
       this->update_thread_list ();
-      this->update_inferior_ptid ();
+      return this->add_active_thread ()->ptid;
     }
-  else
-    inferior_ptid = m_base_ptid;
-  return inferior_ptid;
+  return m_base_ptid;
 }
 
 /* Add the thread associated to the given TASK to the thread list
-- 
2.14.5

Re: [PATCH 26/28] Don't write to inferior_ptid in ravenscar-thread.c

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> writes:

Pedro>    ravenscar_thread_target ()
Pedro> +    : m_base_ptid (inferior_ptid)
Pedro>    {
Pedro> -    update_inferior_ptid ();
Pedro> +    thread_info *thr = add_active_thread ();
Pedro> +    if (thr != nullptr)
Pedro> +      switch_to_thread (thr);
Pedro>    }

I wonder if this will help with the announcement problem fixed here:

https://sourceware.org/pipermail/gdb-patches/2019-March/156455.html

It's not all that important, I still want to try to get that series in.
It's held up on

https://sourceware.org/pipermail/gdb-patches/2019-March/156453.html

Tom

Re: [PATCH 26/28] Don't write to inferior_ptid in ravenscar-thread.c

[Pedro Alves]

On 4/17/20 7:45 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Pedro>    ravenscar_thread_target ()
> Pedro> +    : m_base_ptid (inferior_ptid)
> Pedro>    {
> Pedro> -    update_inferior_ptid ();
> Pedro> +    thread_info *thr = add_active_thread ();
> Pedro> +    if (thr != nullptr)
> Pedro> +      switch_to_thread (thr);
> Pedro>    }
> 
> I wonder if this will help with the announcement problem fixed here:
> 
> https://sourceware.org/pipermail/gdb-patches/2019-March/156455.html
> 

I don't think so.  update_inferior_ptid added a thread, and
add_active_thread will still add it.

> It's not all that important, I still want to try to get that series in.
> It's held up on
> 
> https://sourceware.org/pipermail/gdb-patches/2019-March/156453.html
> 

Offhand that seemed like a gross hack to me when I first saw it, which is
why I've had trouble responding.  :-(  But maybe I just misunderstood it.

Thanks,
Pedro Alves

Re: [PATCH 26/28] Don't write to inferior_ptid in ravenscar-thread.c

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> writes:

>> https://sourceware.org/pipermail/gdb-patches/2019-March/156453.html

Pedro> Offhand that seemed like a gross hack to me when I first saw it, which is
Pedro> why I've had trouble responding.  :-(  But maybe I just misunderstood it.

Yeah, it pretty much is.  On the other hand it helps in this situation.

Probably the gross hack is ravenscar-tdep.c, which is a sort of "green
thread" layer.  Maybe there's some more principled way to tackle this.

Tom

[PATCH 27/28] Don't write to inferior_ptid in aix-thread.c

[Pedro Alves]

There are other writes in the file, but they seem more harmless.  This
one is changing the current thread permanently.

Untested.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* aix-thread.c (pd_update): Use switch_to_thread.
---
 gdb/aix-thread.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index f2bd05fef2..3963a08c84 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -902,7 +902,7 @@ pd_update (int set_infpid)
     {
       ptid = thread->ptid;
       if (set_infpid)
-	inferior_ptid = ptid;
+	switch_to_thread (thread);
     }
   return ptid;
 }
-- 
2.14.5

[PATCH 28/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[Pedro Alves]

In PR/25412, Simon noticed that after the multi-target series, the
tid-reuse.exp testcase manages to create a duplicate thread in the
thread list.  Or rather, two threads with the same PTID.

add_thread_silent has code in place to detect the case of a new thread
reusing some older thread's ptid, but it doesn't work correctly
anymore when the old thread is NOT the current thread and it has a
refcount higher than 0.  Either condition prevents a thread from being
deleted, but the refcount case wasn't being considered.  I think the
reason that case wasn't considered is that that code predates
thread_info refcounting.  Back when it was originally written,
delete_thread always deleted the thread.

That add_thread_silent code in question has some now-unnecessary
warts, BTW.  For instance, this:

  /* Make switch_to_thread not read from the thread.  */
  new_thr->state = THREAD_EXITED;

... used to be required because switch_to_thread would update
'stop_pc' otherwise.  I.e., it would read registers from an exited
thread otherwise.  switch_to_thread no longer reads the stop_pc, since:

  commit f2ffa92bbce9dd5fbedc138ac2a3bc8a88327d09
  Author:     Pedro Alves <palves@redhat.com>
  AuthorDate: Thu Jun 28 20:18:24 2018 +0100

      gdb: Eliminate the 'stop_pc' global

Also, if the ptid of the now-gone current thread is reused, we
currently return from add_thread_silent with the current thread
pointing at the _new_ thread.  Either pointing at the old thread, or
at no thread selected would be reasonable.  But pointing at an
unrelated thread (the new thread that happens to reuse the ptid) is
just broken.  Seems like I was the one who wrote it like that but I
have no clue why, FWIW.

Currently, an exited thread kept in the thread list still holds its
original ptid.  The idea was that we need the ptid to be able to
temporarily switch to another thread and then switch back to the
original thread, because thread switching is really inferior_ptid
switching.  Switching back to the original thread requires a ptid
lookup.

Now, in order to avoid exited threads with the same ptid as a live
thread in the same thread list, one thing I considered (and tried) was
to change an exited thread's ptid to minus_one_ptid.  However, with
that, there's a case that we won't handle well, which is if we end up
with more than one exited thread in the list, since then all exited
threads will all have the same ptid.  Since inferior_thread() relies
on inferior_ptid, may well return the wrong thread.

My next attempt to address this, was to switch an exited thread's ptid
to a globally unique "exited" ptid, which is a ptid with pid == -1 and
tid == 'the thread's global GDB thread number'.  Note that GDB assumes
that the GDB global thread number is monotonically increasing and
doesn't wrap around.  (We should probably make GDB thread numbers
64-bit to prevent that happening in practice; they're currently signed
32-bit.)  This attempt went a long way, but still ran into a number of
issues.  It was a major hack too, obviously.

My next attempt is the one that I'm proposing, which is to bite the
bullet and break the connection between inferior_ptid and
inferior_thread(), aka the current thread.  I.e., make the current
thread be a global thread_info pointer that is written to directly by
switch_to_thread, etc., and making inferior_thread() return that
pointer, instead of having inferior_thread() lookup up the
inferior_ptid thread, by ptid_t.  You can look at this as a
continuation of the effort of using more thread_info pointers instead
of ptids when possible.

By making the current thread a global thread_info pointer, we can make
switch_to_thread simply write to the global thread pointer, which
makes scoped_restore_current_thread able to restore back to an exited
thread without relying on unrelyable ptid look ups.  I.e., this makes
it not a real problem to have more than one thread with the same ptid
in the thread list.  There will always be only one live thread with a
given ptid, so code that looks up a live thread by ptid will always be
able to find the right one.

This change required auditing the whole codebase for places where we
were writing to inferior_ptid directly to change the current thread,
and change them to use switch_to_thread instead or one of its
siblings, because otherwise inferior_thread() would return a thread
unrelated to the changed-to inferior_ptid.  That was all (hopefully)
done in previous patches.

After this, inferior_ptid is mainly used by target backend code.  It
is also relied on by a number of target methods.  E.g., the
target_resume interface and the memory reading routines -- we still
need it there because we need to be able to access memory off of
processes for which we don't have a corresponding inferior/thread
object, like when handling forks.  Maybe we could pass down a context
explicitly to target_read_memory, etc.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* gdbthread.h (delete_thread, delete_thread_silent)
	(find_thread_ptid): Update comments.
	* thread.c (current_thread_): New global.
	(is_current_thread): Move higher, and reimplement.
	(inferior_thread): Reimplement.
	(set_thread_exited): Use bool.  Add assertions.
	(add_thread_silent): Simplify thread-reuse handling by always
	calling delete_thread.
	(delete_thread): Remove intro comment.
	(find_thread_ptid): Skip exited threads.
	(switch_to_thread_no_regs): Write to current_thread_.
	(switch_to_no_thread): Check CURRENT_THREAD_ instead of
	INFERIOR_PTID.  Clear current_thread_.
---
 gdb/gdbthread.h | 17 +++++-----
 gdb/thread.c    | 97 ++++++++++++++++++++-------------------------------------
 2 files changed, 42 insertions(+), 72 deletions(-)

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 717a2ad08c..6764c8fc49 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -433,12 +433,13 @@ extern struct thread_info *add_thread_with_info (process_stratum_target *targ,
 						 ptid_t ptid,
 						 private_thread_info *);
 
-/* Delete an existing thread list entry.  */
+/* Delete thread THREAD and notify of thread exit.  If the thread is
+   currently not deletable, don't actually delete it but still tag it
+   as exited and do the notification.  */
 extern void delete_thread (struct thread_info *thread);
 
-/* Delete an existing thread list entry, and be quiet about it.  Used
-   after the process this thread having belonged to having already
-   exited, for example.  */
+/* Like delete_thread, but be quiet about it.  Used when the process
+   this thread belonged to has already exited, for example.  */
 extern void delete_thread_silent (struct thread_info *thread);
 
 /* Delete a step_resume_breakpoint from the thread database.  */
@@ -478,15 +479,15 @@ extern bool in_thread_list (process_stratum_target *targ, ptid_t ptid);
    global id, not the system's).  */
 extern int valid_global_thread_id (int global_id);
 
-/* Find thread PTID of inferior INF.  */
+/* Find (non-exited) thread PTID of inferior INF.  */
 extern thread_info *find_thread_ptid (inferior *inf, ptid_t ptid);
 
-/* Search function to lookup a thread by 'pid'.  */
+/* Search function to lookup a (non-exited) thread by 'ptid'.  */
 extern struct thread_info *find_thread_ptid (process_stratum_target *targ,
 					     ptid_t ptid);
 
-/* Search function to lookup a thread by 'ptid'.  Only searches in
-   threads of INF.  */
+/* Search function to lookup a (non-exited) thread by 'ptid'.  Only
+   searches in threads of INF.  */
 extern struct thread_info *find_thread_ptid (inferior *inf, ptid_t ptid);
 
 /* Find thread by GDB global thread ID.  */
diff --git a/gdb/thread.c b/gdb/thread.c
index c6e3d356a5..c233995f76 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -55,6 +55,9 @@
 
 static int highest_thread_num;
 
+/* The current/selected thread.  */
+static thread_info *current_thread_;
+
 /* RAII type used to increase / decrease the refcount of each thread
    in a given list of threads.  */
 
@@ -78,13 +81,19 @@ class scoped_inc_dec_ref
   const std::vector<thread_info *> &m_thrds;
 };
 
+/* Returns true if THR is the current thread.  */
+
+static bool
+is_current_thread (const thread_info *thr)
+{
+  return thr == current_thread_;
+}
 
 struct thread_info*
 inferior_thread (void)
 {
-  struct thread_info *tp = find_thread_ptid (current_inferior (), inferior_ptid);
-  gdb_assert (tp);
-  return tp;
+  gdb_assert (current_thread_ != nullptr);
+  return current_thread_;
 }
 
 /* Delete the breakpoint pointed at by BP_P, if there's one.  */
@@ -194,7 +203,7 @@ clear_thread_inferior_resources (struct thread_info *tp)
 /* Set the TP's state as exited.  */
 
 static void
-set_thread_exited (thread_info *tp, int silent)
+set_thread_exited (thread_info *tp, bool silent)
 {
   /* Dead threads don't need to step-over.  Remove from queue.  */
   if (tp->step_over_next != NULL)
@@ -245,7 +254,12 @@ new_thread (struct inferior *inf, ptid_t ptid)
       struct thread_info *last;
 
       for (last = inf->thread_list; last->next != NULL; last = last->next)
-	;
+	gdb_assert (ptid != last->ptid
+		    || last->state == THREAD_EXITED);
+
+      gdb_assert (ptid != last->ptid
+		  || last->state == THREAD_EXITED);
+
       last->next = tp;
     }
 
@@ -255,51 +269,15 @@ new_thread (struct inferior *inf, ptid_t ptid)
 struct thread_info *
 add_thread_silent (process_stratum_target *targ, ptid_t ptid)
 {
-  inferior *inf;
-
-  thread_info *tp = find_thread_ptid (targ, ptid);
-  if (tp)
-    /* Found an old thread with the same id.  It has to be dead,
-       otherwise we wouldn't be adding a new thread with the same id.
-       The OS is reusing this id --- delete it, and recreate a new
-       one.  */
-    {
-      /* In addition to deleting the thread, if this is the current
-	 thread, then we need to take care that delete_thread doesn't
-	 really delete the thread if it is inferior_ptid.  Create a
-	 new template thread in the list with an invalid ptid, switch
-	 to it, delete the original thread, reset the new thread's
-	 ptid, and switch to it.  */
-
-      if (inferior_ptid == ptid)
-	{
-	  thread_info *new_thr = new_thread (tp->inf, null_ptid);
-
-	  /* Make switch_to_thread not read from the thread.  */
-	  new_thr->state = THREAD_EXITED;
-	  switch_to_no_thread ();
-
-	  /* Now we can delete it.  */
-	  delete_thread (tp);
-
-	  /* Now reset its ptid, and reswitch inferior_ptid to it.  */
-	  new_thr->ptid = ptid;
-	  new_thr->state = THREAD_STOPPED;
-	  switch_to_thread (new_thr);
-
-	  gdb::observers::new_thread.notify (new_thr);
-
-	  /* All done.  */
-	  return new_thr;
-	}
+  inferior *inf = find_inferior_ptid (targ, ptid);
 
-      inf = tp->inf;
-
-      /* Just go ahead and delete it.  */
-      delete_thread (tp);
-    }
-  else
-    inf = find_inferior_ptid (targ, ptid);
+  /* We may have an old thread with the same id in the thread list.
+     If we do, it must be dead, otherwise we wouldn't be adding a new
+     thread with the same id.  The OS is reusing this id --- delete
+     the old thread, and create a new one.  */
+  thread_info *tp = find_thread_ptid (inf, ptid);
+  if (tp != nullptr)
+    delete_thread (tp);
 
   tp = new_thread (inf, ptid);
   gdb::observers::new_thread.notify (tp);
@@ -349,14 +327,6 @@ thread_info::~thread_info ()
   xfree (this->name);
 }
 
-/* Returns true if THR is the current thread.  */
-
-static bool
-is_current_thread (const thread_info *thr)
-{
-  return thr->inf == current_inferior () && thr->ptid == inferior_ptid;
-}
-
 /* See gdbthread.h.  */
 
 bool
@@ -482,10 +452,7 @@ delete_thread_1 (thread_info *thr, bool silent)
   delete tp;
 }
 
-/* Delete thread THREAD and notify of thread exit.  If this is the
-   current thread, don't actually delete it, but tag it as exited and
-   do the notification.  If this is the user selected thread, clear
-   it.  */
+/* See gdbthread.h.  */
 
 void
 delete_thread (thread_info *thread)
@@ -535,7 +502,7 @@ find_thread_ptid (process_stratum_target *targ, ptid_t ptid)
 struct thread_info *
 find_thread_ptid (inferior *inf, ptid_t ptid)
 {
-  for (thread_info *tp : inf->threads ())
+  for (thread_info *tp : inf->non_exited_threads ())
     if (tp->ptid == ptid)
       return tp;
 
@@ -1317,7 +1284,8 @@ switch_to_thread_no_regs (struct thread_info *thread)
   set_current_program_space (inf->pspace);
   set_current_inferior (inf);
 
-  inferior_ptid = thread->ptid;
+  current_thread_ = thread;
+  inferior_ptid = current_thread_->ptid;
 }
 
 /* See gdbthread.h.  */
@@ -1325,9 +1293,10 @@ switch_to_thread_no_regs (struct thread_info *thread)
 void
 switch_to_no_thread ()
 {
-  if (inferior_ptid == null_ptid)
+  if (current_thread_ == nullptr)
     return;
 
+  current_thread_ = nullptr;
   inferior_ptid = null_ptid;
   reinit_frame_cache ();
 }
-- 
2.14.5

Re: [PATCH 28/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[Simon Marchi]

On 2020-04-14 1:54 p.m., Pedro Alves via Gdb-patches wrote:
> In PR/25412, Simon noticed that after the multi-target series, the
> tid-reuse.exp testcase manages to create a duplicate thread in the
> thread list.  Or rather, two threads with the same PTID.
> 
> add_thread_silent has code in place to detect the case of a new thread
> reusing some older thread's ptid, but it doesn't work correctly
> anymore when the old thread is NOT the current thread and it has a
> refcount higher than 0.  Either condition prevents a thread from being
> deleted, but the refcount case wasn't being considered.  I think the
> reason that case wasn't considered is that that code predates
> thread_info refcounting.  Back when it was originally written,
> delete_thread always deleted the thread.
> 
> That add_thread_silent code in question has some now-unnecessary
> warts, BTW.  For instance, this:
> 
>   /* Make switch_to_thread not read from the thread.  */
>   new_thr->state = THREAD_EXITED;
> 
> ... used to be required because switch_to_thread would update
> 'stop_pc' otherwise.  I.e., it would read registers from an exited
> thread otherwise.  switch_to_thread no longer reads the stop_pc, since:
> 
>   commit f2ffa92bbce9dd5fbedc138ac2a3bc8a88327d09
>   Author:     Pedro Alves <palves@redhat.com>
>   AuthorDate: Thu Jun 28 20:18:24 2018 +0100
> 
>       gdb: Eliminate the 'stop_pc' global
> 
> Also, if the ptid of the now-gone current thread is reused, we
> currently return from add_thread_silent with the current thread
> pointing at the _new_ thread.  Either pointing at the old thread, or
> at no thread selected would be reasonable.  But pointing at an
> unrelated thread (the new thread that happens to reuse the ptid) is
> just broken.  Seems like I was the one who wrote it like that but I
> have no clue why, FWIW.
> 
> Currently, an exited thread kept in the thread list still holds its
> original ptid.  The idea was that we need the ptid to be able to
> temporarily switch to another thread and then switch back to the
> original thread, because thread switching is really inferior_ptid
> switching.  Switching back to the original thread requires a ptid
> lookup.
> 
> Now, in order to avoid exited threads with the same ptid as a live
> thread in the same thread list, one thing I considered (and tried) was
> to change an exited thread's ptid to minus_one_ptid.  However, with
> that, there's a case that we won't handle well, which is if we end up
> with more than one exited thread in the list, since then all exited
> threads will all have the same ptid.  Since inferior_thread() relies
> on inferior_ptid, may well return the wrong thread.
> 
> My next attempt to address this, was to switch an exited thread's ptid
> to a globally unique "exited" ptid, which is a ptid with pid == -1 and
> tid == 'the thread's global GDB thread number'.  Note that GDB assumes
> that the GDB global thread number is monotonically increasing and
> doesn't wrap around.  (We should probably make GDB thread numbers
> 64-bit to prevent that happening in practice; they're currently signed
> 32-bit.)  This attempt went a long way, but still ran into a number of
> issues.  It was a major hack too, obviously.
> 
> My next attempt is the one that I'm proposing, which is to bite the
> bullet and break the connection between inferior_ptid and
> inferior_thread(), aka the current thread.  I.e., make the current
> thread be a global thread_info pointer that is written to directly by
> switch_to_thread, etc., and making inferior_thread() return that
> pointer, instead of having inferior_thread() lookup up the
> inferior_ptid thread, by ptid_t.  You can look at this as a
> continuation of the effort of using more thread_info pointers instead
> of ptids when possible.
> 
> By making the current thread a global thread_info pointer, we can make
> switch_to_thread simply write to the global thread pointer, which
> makes scoped_restore_current_thread able to restore back to an exited
> thread without relying on unrelyable ptid look ups.  I.e., this makes
> it not a real problem to have more than one thread with the same ptid
> in the thread list.  There will always be only one live thread with a
> given ptid, so code that looks up a live thread by ptid will always be
> able to find the right one.
> 
> This change required auditing the whole codebase for places where we
> were writing to inferior_ptid directly to change the current thread,
> and change them to use switch_to_thread instead or one of its
> siblings, because otherwise inferior_thread() would return a thread
> unrelated to the changed-to inferior_ptid.  That was all (hopefully)
> done in previous patches.
> 
> After this, inferior_ptid is mainly used by target backend code.  It
> is also relied on by a number of target methods.  E.g., the
> target_resume interface and the memory reading routines -- we still
> need it there because we need to be able to access memory off of
> processes for which we don't have a corresponding inferior/thread
> object, like when handling forks.  Maybe we could pass down a context
> explicitly to target_read_memory, etc.
> 
> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> 	* gdbthread.h (delete_thread, delete_thread_silent)
> 	(find_thread_ptid): Update comments.
> 	* thread.c (current_thread_): New global.
> 	(is_current_thread): Move higher, and reimplement.
> 	(inferior_thread): Reimplement.
> 	(set_thread_exited): Use bool.  Add assertions.
> 	(add_thread_silent): Simplify thread-reuse handling by always
> 	calling delete_thread.
> 	(delete_thread): Remove intro comment.
> 	(find_thread_ptid): Skip exited threads.
> 	(switch_to_thread_no_regs): Write to current_thread_.
> 	(switch_to_no_thread): Check CURRENT_THREAD_ instead of
> 	INFERIOR_PTID.  Clear current_thread_.

I checked the rest of the series, I didn't find any obvious problem.

I have a patch [1] that I might consider sending upstream some day, which replaces the
inferior thread lists with maps, using the ptid as the key.  This was made for targets
that have a lot of threads (like, thousands), where looking up a thread object from a
ptid would take a bit of time, and that would add up.  If there can be two threads in
the list with the same ptid, that won't work with a map using the ptid as the key.

So I was wondering, when we want to delete a thread but its refcount is not 0, does it
need to stay in the thread list?  What if we remove it from the list, and whatever
decrements its refcount to 0 deletes it?  Do you think that could work?

It's possible, however, that with your series, the number of times we look up a thread
from its ptid is reduced enough that it makes my patch not really useful.

Simon

[1] https://github.com/simark/binutils-gdb/commits/inferior-thread-map

Re: [PATCH 28/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[Pedro Alves]

On 4/16/20 8:39 PM, Simon Marchi wrote:

> I checked the rest of the series, I didn't find any obvious problem.

Thanks.

> 
> I have a patch [1] that I might consider sending upstream some day, which replaces the
> inferior thread lists with maps, using the ptid as the key.  This was made for targets
> that have a lot of threads (like, thousands), where looking up a thread object from a
> ptid would take a bit of time, and that would add up.  If there can be two threads in
> the list with the same ptid, that won't work with a map using the ptid as the key.
> 
> So I was wondering, when we want to delete a thread but its refcount is not 0, does it
> need to stay in the thread list?  What if we remove it from the list, and whatever
> decrements its refcount to 0 deletes it?  Do you think that could work?

I think it could.  That's something that I considered, as something that is made
possible with this patchset, because, before we needed it to stay in the list so
that the lookup inside inferior_thread(), but after the series the lookup is gone.
We'd also need to look out for places that want to walk all threads, instead of
just non-exited ones, but on a quick look, I didn't find any left that couldn't
be converted to walk to non-exited threads only.

Another thing that makes it possible is that "info threads" doesn't show the
exited threads in the list.  Even if we wanted to show the current thread
in the list if it's exited (which is something I considered), we could still
easily do that, though.  Currently, we just show the
"The current thread <Thread ID %s> has terminated.  See `help thread'"
message at the end of the thread list.

> 
> It's possible, however, that with your series, the number of times we look up a thread
> from its ptid is reduced enough that it makes my patch not really useful.
> 
> Simon
> 
> [1] https://github.com/simark/binutils-gdb/commits/inferior-thread-map

How are you making sure that iterating the threads walks them in
creation/insertion order instead of ptid_t order?

Thanks,
Pedro Alves

Re: [PATCH 28/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[Simon Marchi]

On 2020-04-16 4:12 p.m., Pedro Alves via Gdb-patches wrote:
> On 4/16/20 8:39 PM, Simon Marchi wrote:
> 
>> I checked the rest of the series, I didn't find any obvious problem.
> 
> Thanks.
> 
>>
>> I have a patch [1] that I might consider sending upstream some day, which replaces the
>> inferior thread lists with maps, using the ptid as the key.  This was made for targets
>> that have a lot of threads (like, thousands), where looking up a thread object from a
>> ptid would take a bit of time, and that would add up.  If there can be two threads in
>> the list with the same ptid, that won't work with a map using the ptid as the key.
>>
>> So I was wondering, when we want to delete a thread but its refcount is not 0, does it
>> need to stay in the thread list?  What if we remove it from the list, and whatever
>> decrements its refcount to 0 deletes it?  Do you think that could work?
> 
> I think it could.  That's something that I considered, as something that is made
> possible with this patchset, because, before we needed it to stay in the list so
> that the lookup inside inferior_thread(), but after the series the lookup is gone.
> We'd also need to look out for places that want to walk all threads, instead of
> just non-exited ones, but on a quick look, I didn't find any left that couldn't
> be converted to walk to non-exited threads only.
> 
> Another thing that makes it possible is that "info threads" doesn't show the
> exited threads in the list.  Even if we wanted to show the current thread
> in the list if it's exited (which is something I considered), we could still
> easily do that, though.  Currently, we just show the
> "The current thread <Thread ID %s> has terminated.  See `help thread'"
> message at the end of the thread list.

Indeed, I checked "info threads" before posting this, because it's one case I
know the deleted thread was referred to.

>> It's possible, however, that with your series, the number of times we look up a thread
>> from its ptid is reduced enough that it makes my patch not really useful.
>>
>> Simon
>>
>> [1] https://github.com/simark/binutils-gdb/commits/inferior-thread-map
> 
> How are you making sure that iterating the threads walks them in
> creation/insertion order instead of ptid_t order?

The only spot that I found the order mattered was precisely for "info threads".
There, I gather the list of threads to display, sort them by per_inf_num, and
print them.

https://github.com/simark/binutils-gdb/commit/25c67729996dc83912d34a5901016404b3638bc1#diff-5479a5443dca2232f8552dfb5b30ac3dR1104

Simon

Re: [PATCH 28/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[Pedro Alves]

On 4/16/20 9:38 PM, Simon Marchi wrote:
> On 2020-04-16 4:12 p.m., Pedro Alves via Gdb-patches wrote:

>> How are you making sure that iterating the threads walks them in
>> creation/insertion order instead of ptid_t order?
> 
> The only spot that I found the order mattered was precisely for "info threads".
> There, I gather the list of threads to display, sort them by per_inf_num, and
> print them.
> 
> https://github.com/simark/binutils-gdb/commit/25c67729996dc83912d34a5901016404b3638bc1#diff-5479a5443dca2232f8552dfb5b30ac3dR1104
> 

Hmm, OK.  I'm not 100% sure that's a good idea; it makes me a bit nervous
because I'm not sure if this has an impact on run control.  Something to
think about.  We could instead have the new ptid_t map for ptid_t look ups, but
still also maintain the linked list in order to walk the thread list.  (That
would also be a much simpler patch, I think.)

Thanks,
Pedro Alves

Re: [PATCH 28/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[Simon Marchi]

On 2020-04-17 6:29 a.m., Pedro Alves wrote:
> On 4/16/20 9:38 PM, Simon Marchi wrote:
>> On 2020-04-16 4:12 p.m., Pedro Alves via Gdb-patches wrote:
> 
>>> How are you making sure that iterating the threads walks them in
>>> creation/insertion order instead of ptid_t order?
>>
>> The only spot that I found the order mattered was precisely for "info threads".
>> There, I gather the list of threads to display, sort them by per_inf_num, and
>> print them.
>>
>> https://github.com/simark/binutils-gdb/commit/25c67729996dc83912d34a5901016404b3638bc1#diff-5479a5443dca2232f8552dfb5b30ac3dR1104
>>
> 
> Hmm, OK.  I'm not 100% sure that's a good idea; it makes me a bit nervous
> because I'm not sure if this has an impact on run control.  Something to
> think about.  We could instead have the new ptid_t map for ptid_t look ups, but
> still also maintain the linked list in order to walk the thread list.  (That
> would also be a much simpler patch, I think.)

I also thought about that as a backup solution.  However, the testsuite didn't show
any more regressions, so I stayed with just the map.

Simon

Re: [PATCH 28/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[Pedro Alves]

On 4/17/20 3:06 PM, Simon Marchi wrote:
> On 2020-04-17 6:29 a.m., Pedro Alves wrote:
>> On 4/16/20 9:38 PM, Simon Marchi wrote:
>>> On 2020-04-16 4:12 p.m., Pedro Alves via Gdb-patches wrote:
>>
>>>> How are you making sure that iterating the threads walks them in
>>>> creation/insertion order instead of ptid_t order?
>>>
>>> The only spot that I found the order mattered was precisely for "info threads".
>>> There, I gather the list of threads to display, sort them by per_inf_num, and
>>> print them.
>>>
>>> https://github.com/simark/binutils-gdb/commit/25c67729996dc83912d34a5901016404b3638bc1#diff-5479a5443dca2232f8552dfb5b30ac3dR1104
>>>
>>
>> Hmm, OK.  I'm not 100% sure that's a good idea; it makes me a bit nervous
>> because I'm not sure if this has an impact on run control.  Something to
>> think about.  We could instead have the new ptid_t map for ptid_t look ups, but
>> still also maintain the linked list in order to walk the thread list.  (That
>> would also be a much simpler patch, I think.)
> 
> I also thought about that as a backup solution.  However, the testsuite didn't show
> any more regressions, so I stayed with just the map.

Maybe.  Though, this is likely to make debugging gdb harder, both
interactive debugging ("p thread->next->next->state" kind of things), and
also infrun logs -- The order which gdb resumes, suspends, etc. threads
will change as threads appear/disappear.  Sticking to creation order is
likely to avoid headaches, IMO.

Thanks,
Pedro Alves

Re: [PATCH 28/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> writes:

Pedro> My next attempt is the one that I'm proposing, which is to bite the
Pedro> bullet and break the connection between inferior_ptid and
Pedro> inferior_thread(), aka the current thread.  I.e., make the current
Pedro> thread be a global thread_info pointer that is written to directly by
Pedro> switch_to_thread, etc., and making inferior_thread() return that
Pedro> pointer, instead of having inferior_thread() lookup up the
Pedro> inferior_ptid thread, by ptid_t.

Does this mean that now the current thread and current inferior can get
out of sync?  Or that these can be out of sync with inferior_ptid?

This patch looks like it tries to avoid that when writing to the current
thread -- but in the cover letter you mentioned that there are other
assignments to inferior_ptid.

I wouldn't block the patches based on this but I'd like to understand
the direction.  I guess I'd prefer to remove globals and possibilities
for divergence if it's possible.

Tom

Re: [PATCH 28/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[Pedro Alves]

Hi Tom,

Sorry for the delay in responding.  I hate when I end up doing this.

On 4/17/20 7:53 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Pedro> My next attempt is the one that I'm proposing, which is to bite the
> Pedro> bullet and break the connection between inferior_ptid and
> Pedro> inferior_thread(), aka the current thread.  I.e., make the current
> Pedro> thread be a global thread_info pointer that is written to directly by
> Pedro> switch_to_thread, etc., and making inferior_thread() return that
> Pedro> pointer, instead of having inferior_thread() lookup up the
> Pedro> inferior_ptid thread, by ptid_t.
> 
> Does this mean that now the current thread and current inferior can get
> out of sync?  

They already could, because the current thread was based on inferior_ptid,
while the current inferior is stored in a global "inferior *current_inferior_"
pointer.

> Or that these can be out of sync with inferior_ptid?

Yes, before, inferior_ptid could get out of sync with the current
inferior, but not with the current thread.  Afterwards, the current
"thread_info *" can also get out of sync with inferior_ptid.  That's
why most of the series eliminates writes to inferior_ptid directly.

Over time, inferior_ptid usages have been disappearing, replaced by
references to "thread_info *" or "inferior *" objects directly.  

I think that's much better than referring to objects via integers that may
be ambiguous between targets, and worrying about what target_ops owns what
in the ptid_t fields, how to extend ptid for different types of execution
objects, etc.  I think that ideally, in the GDB core, references to
inferior_ptid would virtually disappear.  I see this series as another step
in that direction.

> 
> This patch looks like it tries to avoid that when writing to the current
> thread -- but in the cover letter you mentioned that there are other
> assignments to inferior_ptid.

I think you are referring to this comment:

> "After this, inferior_ptid still exists, but it is mostly read-only and
> mainly used by target backend code.  It is also relied on by a number
> of target methods as a global input argument.  E.g., the target_resume
> interface and the memory reading routines -- we still need it there
> because we need to be able to access memory off of processes for which
> we don't have a corresponding inferior/thread object, like when
> handling forks.  Maybe we could pass down a context explicitly to
> target_read_memory, etc."

In the target_resume case, we always switch inferior_ptid using
switch_to_thread or similar, so not a big deal.

The worrying case I found is the memory access routines.  TBC, that's a case
where the connection between current thread and inferior_ptid was already
being broken previously.  Let me expand on the "handling forks" bit:

When we detach breakpoints from a fork child that we're about to
detach from due to "set detach-on-fork off", we need to uninstall memory
breakpoints from the child, because breakpoint instructions that were
inserted on the parent end up in the child as well, because the child's
address space is just a copy of the parent's.  So to do that, we switch
inferior_ptid to the fork child, and call the breakpoint removal target
method, so that it applies to the fork child.  Note that there are no
thread_info or inferior objects for that fork child in the thread/inferior
lists at all.  So if something in those code breakpoint removal paths (all
the way down the the memory xfer routines) calls inferior_thread() it would
already assert for not finding the thread in the list.  The patch series
doesn't change that.

> I wouldn't block the patches based on this but I'd like to understand
> the direction.  I guess I'd prefer to remove globals and possibilities
> for divergence if it's possible.

Me too, but it's just that inferior_ptid is so pervasive that it's
impossible to eliminate all in one go.

Thanks,
Pedro Alves

Re: [PATCH 28/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[Andrew Burgess]

* Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> [2020-04-14 18:54:34 +0100]:

> In PR/25412, Simon noticed that after the multi-target series, the
> tid-reuse.exp testcase manages to create a duplicate thread in the
> thread list.  Or rather, two threads with the same PTID.
> 
> add_thread_silent has code in place to detect the case of a new thread
> reusing some older thread's ptid, but it doesn't work correctly
> anymore when the old thread is NOT the current thread and it has a
> refcount higher than 0.  Either condition prevents a thread from being
> deleted, but the refcount case wasn't being considered.  I think the
> reason that case wasn't considered is that that code predates
> thread_info refcounting.  Back when it was originally written,
> delete_thread always deleted the thread.
> 
> That add_thread_silent code in question has some now-unnecessary
> warts, BTW.  For instance, this:
> 
>   /* Make switch_to_thread not read from the thread.  */
>   new_thr->state = THREAD_EXITED;
> 
> ... used to be required because switch_to_thread would update
> 'stop_pc' otherwise.  I.e., it would read registers from an exited
> thread otherwise.  switch_to_thread no longer reads the stop_pc, since:
> 
>   commit f2ffa92bbce9dd5fbedc138ac2a3bc8a88327d09
>   Author:     Pedro Alves <palves@redhat.com>
>   AuthorDate: Thu Jun 28 20:18:24 2018 +0100
> 
>       gdb: Eliminate the 'stop_pc' global
> 
> Also, if the ptid of the now-gone current thread is reused, we
> currently return from add_thread_silent with the current thread
> pointing at the _new_ thread.  Either pointing at the old thread, or
> at no thread selected would be reasonable.  But pointing at an
> unrelated thread (the new thread that happens to reuse the ptid) is
> just broken.  Seems like I was the one who wrote it like that but I
> have no clue why, FWIW.
> 
> Currently, an exited thread kept in the thread list still holds its
> original ptid.  The idea was that we need the ptid to be able to
> temporarily switch to another thread and then switch back to the
> original thread, because thread switching is really inferior_ptid
> switching.  Switching back to the original thread requires a ptid
> lookup.
> 
> Now, in order to avoid exited threads with the same ptid as a live
> thread in the same thread list, one thing I considered (and tried) was
> to change an exited thread's ptid to minus_one_ptid.  However, with
> that, there's a case that we won't handle well, which is if we end up
> with more than one exited thread in the list, since then all exited
> threads will all have the same ptid.  Since inferior_thread() relies
> on inferior_ptid, may well return the wrong thread.
> 
> My next attempt to address this, was to switch an exited thread's ptid
> to a globally unique "exited" ptid, which is a ptid with pid == -1 and
> tid == 'the thread's global GDB thread number'.  Note that GDB assumes
> that the GDB global thread number is monotonically increasing and
> doesn't wrap around.  (We should probably make GDB thread numbers
> 64-bit to prevent that happening in practice; they're currently signed
> 32-bit.)  This attempt went a long way, but still ran into a number of
> issues.  It was a major hack too, obviously.
> 
> My next attempt is the one that I'm proposing, which is to bite the
> bullet and break the connection between inferior_ptid and
> inferior_thread(), aka the current thread.  I.e., make the current
> thread be a global thread_info pointer that is written to directly by
> switch_to_thread, etc., and making inferior_thread() return that
> pointer, instead of having inferior_thread() lookup up the
> inferior_ptid thread, by ptid_t.  You can look at this as a
> continuation of the effort of using more thread_info pointers instead
> of ptids when possible.
> 
> By making the current thread a global thread_info pointer, we can make
> switch_to_thread simply write to the global thread pointer, which
> makes scoped_restore_current_thread able to restore back to an exited
> thread without relying on unrelyable ptid look ups.  I.e., this makes
> it not a real problem to have more than one thread with the same ptid
> in the thread list.  There will always be only one live thread with a
> given ptid, so code that looks up a live thread by ptid will always be
> able to find the right one.
> 
> This change required auditing the whole codebase for places where we
> were writing to inferior_ptid directly to change the current thread,
> and change them to use switch_to_thread instead or one of its
> siblings, because otherwise inferior_thread() would return a thread
> unrelated to the changed-to inferior_ptid.  That was all (hopefully)
> done in previous patches.
> 
> After this, inferior_ptid is mainly used by target backend code.  It
> is also relied on by a number of target methods.  E.g., the
> target_resume interface and the memory reading routines -- we still
> need it there because we need to be able to access memory off of
> processes for which we don't have a corresponding inferior/thread
> object, like when handling forks.  Maybe we could pass down a context
> explicitly to target_read_memory, etc.
> 
> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> 	* gdbthread.h (delete_thread, delete_thread_silent)
> 	(find_thread_ptid): Update comments.
> 	* thread.c (current_thread_): New global.
> 	(is_current_thread): Move higher, and reimplement.
> 	(inferior_thread): Reimplement.
> 	(set_thread_exited): Use bool.  Add assertions.
> 	(add_thread_silent): Simplify thread-reuse handling by always
> 	calling delete_thread.
> 	(delete_thread): Remove intro comment.
> 	(find_thread_ptid): Skip exited threads.
> 	(switch_to_thread_no_regs): Write to current_thread_.
> 	(switch_to_no_thread): Check CURRENT_THREAD_ instead of
> 	INFERIOR_PTID.  Clear current_thread_.
> ---
>  gdb/gdbthread.h | 17 +++++-----
>  gdb/thread.c    | 97 ++++++++++++++++++++-------------------------------------
>  2 files changed, 42 insertions(+), 72 deletions(-)

This commit causes a regression for gdb.gdb/unittest.exp when GDB is
configured with --enable-targets=all.

The failure is:

  gdb/thread.c:95: internal-error: thread_info* inferior_thread(): Assertion `current_thread_ != nullptr' failed.

Here's a partial backtrace:

  #8  0x00000000013e4d0f in internal_error (file=0x16c55c7 "../../src.dev-1/gdb/thread.c", line=95, 
      fmt=0x16c54d9 "%s: Assertion `%s' failed.") at ../../src.dev-1/gdbsupport/errors.cc:55
  #9  0x0000000000c34e63 in inferior_thread () at ../../src.dev-1/gdb/thread.c:95
  #10 0x0000000000a73d62 in get_current_regcache () at ../../src.dev-1/gdb/regcache.c:391
  #11 0x00000000009051c0 in current_options () at ../../src.dev-1/gdb/mep-tdep.c:873
  #12 0x00000000009052eb in mep_register_name (gdbarch=0x33862a0, regnr=152) at ../../src.dev-1/gdb/mep-tdep.c:958
  #13 0x00000000009054aa in mep_register_reggroup_p (gdbarch=0x33862a0, regnum=152, group=0x26334c0 <save_group>)
      at ../../src.dev-1/gdb/mep-tdep.c:1029
  #14 0x00000000007975e8 in gdbarch_register_reggroup_p (gdbarch=0x33862a0, regnum=152, reggroup=0x26334c0 <save_group>)
      at ../../src.dev-1/gdb/gdbarch.c:3595
  #15 0x0000000000a7355a in reg_buffer::save(gdb::function_view<register_status (int, unsigned char*)>) (
      this=0x7fffffffad00, cooked_read=...) at ../../src.dev-1/gdb/regcache.c:248
  #16 0x0000000000780b36 in readonly_detached_regcache::readonly_detached_regcache(gdbarch*, gdb::function_view<register_status (int, unsigned char*)>) (this=0x7fffffffad00, gdbarch=0x33862a0, cooked_read=...)
      at ../../src.dev-1/gdb/regcache.h:455
  #17 0x0000000000a73409 in readonly_detached_regcache::readonly_detached_regcache (this=0x7fffffffad00, src=...)
      at ../../src.dev-1/gdb/regcache.c:213
  #18 0x0000000000a773b5 in selftests::cooked_read_test (gdbarch=0x33862a0) at ../../src.dev-1/gdb/regcache.c:1690
  #19 0x0000000000b48c29 in selftests::gdbarch_selftest::operator() (this=0x320fef0)
      at ../../src.dev-1/gdb/selftest-arch.c:73
  #20 0x00000000013fe5ac in selftests::run_tests (filter=0x0) at ../../src.dev-1/gdbsupport/selftest.cc:88
  #21 0x00000000008e78ba in maintenance_selftest (args=0x0, from_tty=1) at ../../src.dev-1/gdb/maint.c:1044
  #22 0x00000000005b0557 in do_const_cfunc (c=0x32004f0, args=0x0, from_tty=1)
      at ../../src.dev-1/gdb/cli/cli-decode.c:95
  #23 0x00000000005b3839 in cmd_func (cmd=0x32004f0, args=0x0, from_tty=1) at ../../src.dev-1/gdb/cli/cli-decode.c:2113
  #24 0x0000000000c4953b in execute_command (p=0x2924895 "", from_tty=1) at ../../src.dev-1/gdb/top.c:655
  #25 0x0000000000759c26 in command_handler (command=0x2924880 "maintenance selftest ")
      at ../../src.dev-1/gdb/event-top.c:588
  #26 0x000000000075a052 in command_line_handler (rl=...) at ../../src.dev-1/gdb/event-top.c:773

I suspect that the problem might be this line in regcache.c:cooked_read_test:

  /* Switch to the mock thread.  */
  scoped_restore restore_inferior_ptid
    = make_scoped_restore (&inferior_ptid, mock_ptid);

My suspicion from a quick read of your patch above is that we need to
do more than just set inferior_ptid here now.

Thanks,
Andrew

Re: [PATCH 28/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[Pedro Alves]

On 6/23/20 2:37 PM, Andrew Burgess wrote:

> This commit causes a regression for gdb.gdb/unittest.exp when GDB is
> configured with --enable-targets=all.
> 
> The failure is:
> 
>   gdb/thread.c:95: internal-error: thread_info* inferior_thread(): Assertion `current_thread_ != nullptr' failed.
> 
> Here's a partial backtrace:
> 
>   #8  0x00000000013e4d0f in internal_error (file=0x16c55c7 "../../src.dev-1/gdb/thread.c", line=95, 
>       fmt=0x16c54d9 "%s: Assertion `%s' failed.") at ../../src.dev-1/gdbsupport/errors.cc:55
>   #9  0x0000000000c34e63 in inferior_thread () at ../../src.dev-1/gdb/thread.c:95
>   #10 0x0000000000a73d62 in get_current_regcache () at ../../src.dev-1/gdb/regcache.c:391
>   #11 0x00000000009051c0 in current_options () at ../../src.dev-1/gdb/mep-tdep.c:873
>   #12 0x00000000009052eb in mep_register_name (gdbarch=0x33862a0, regnr=152) at ../../src.dev-1/gdb/mep-tdep.c:958
>   #13 0x00000000009054aa in mep_register_reggroup_p (gdbarch=0x33862a0, regnum=152, group=0x26334c0 <save_group>)
>       at ../../src.dev-1/gdb/mep-tdep.c:1029
>   #14 0x00000000007975e8 in gdbarch_register_reggroup_p (gdbarch=0x33862a0, regnum=152, reggroup=0x26334c0 <save_group>)
>       at ../../src.dev-1/gdb/gdbarch.c:3595
>   #15 0x0000000000a7355a in reg_buffer::save(gdb::function_view<register_status (int, unsigned char*)>) (
>       this=0x7fffffffad00, cooked_read=...) at ../../src.dev-1/gdb/regcache.c:248
>   #16 0x0000000000780b36 in readonly_detached_regcache::readonly_detached_regcache(gdbarch*, gdb::function_view<register_status (int, unsigned char*)>) (this=0x7fffffffad00, gdbarch=0x33862a0, cooked_read=...)
>       at ../../src.dev-1/gdb/regcache.h:455
>   #17 0x0000000000a73409 in readonly_detached_regcache::readonly_detached_regcache (this=0x7fffffffad00, src=...)
>       at ../../src.dev-1/gdb/regcache.c:213
>   #18 0x0000000000a773b5 in selftests::cooked_read_test (gdbarch=0x33862a0) at ../../src.dev-1/gdb/regcache.c:1690
>   #19 0x0000000000b48c29 in selftests::gdbarch_selftest::operator() (this=0x320fef0)
>       at ../../src.dev-1/gdb/selftest-arch.c:73
>   #20 0x00000000013fe5ac in selftests::run_tests (filter=0x0) at ../../src.dev-1/gdbsupport/selftest.cc:88
>   #21 0x00000000008e78ba in maintenance_selftest (args=0x0, from_tty=1) at ../../src.dev-1/gdb/maint.c:1044
>   #22 0x00000000005b0557 in do_const_cfunc (c=0x32004f0, args=0x0, from_tty=1)
>       at ../../src.dev-1/gdb/cli/cli-decode.c:95
>   #23 0x00000000005b3839 in cmd_func (cmd=0x32004f0, args=0x0, from_tty=1) at ../../src.dev-1/gdb/cli/cli-decode.c:2113
>   #24 0x0000000000c4953b in execute_command (p=0x2924895 "", from_tty=1) at ../../src.dev-1/gdb/top.c:655
>   #25 0x0000000000759c26 in command_handler (command=0x2924880 "maintenance selftest ")
>       at ../../src.dev-1/gdb/event-top.c:588
>   #26 0x000000000075a052 in command_line_handler (rl=...) at ../../src.dev-1/gdb/event-top.c:773
> 
> I suspect that the problem might be this line in regcache.c:cooked_read_test:
> 
>   /* Switch to the mock thread.  */
>   scoped_restore restore_inferior_ptid
>     = make_scoped_restore (&inferior_ptid, mock_ptid);
> 
> My suspicion from a quick read of your patch above is that we need to
> do more than just set inferior_ptid here now.

Indeed.  Thanks for reporting it.  

I fixed this in gdbarch-selftests.c, where this code is duplicated,
but missed regcache.c.  I'll fix it.

Thanks,
Pedro Alves

[PATCH] Fix "maint selftest" regression, add struct, scoped_mock_context

[Pedro Alves]

On 6/23/20 3:26 PM, Pedro Alves via Gdb-patches wrote:
> On 6/23/20 2:37 PM, Andrew Burgess wrote:

>> I suspect that the problem might be this line in regcache.c:cooked_read_test:
>>
>>   /* Switch to the mock thread.  */
>>   scoped_restore restore_inferior_ptid
>>     = make_scoped_restore (&inferior_ptid, mock_ptid);
>>
>> My suspicion from a quick read of your patch above is that we need to
>> do more than just set inferior_ptid here now.
> 
> Indeed.  Thanks for reporting it.  
> 
> I fixed this in gdbarch-selftests.c, where this code is duplicated,
> but missed regcache.c.  I'll fix it.
> 

Like so.  WDYT?

From d3438cc6c48bd77880da7ef7a449edbbfe990e37 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 23 Jun 2020 15:18:41 +0100
Subject: [PATCH] Fix "maint selftest" regression, add struct
 scoped_mock_context

This commit:

 commit 3922b302645fda04da42a5279399578ae2f6206c
 Author:     Pedro Alves <palves@redhat.com>
 AuthorDate: Thu Jun 18 21:28:37 2020 +0100

    Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR 25412)

caused a regression for gdb.gdb/unittest.exp when GDB is configured
with --enable-targets=all.  The failure is:

  gdb/thread.c:95: internal-error: thread_info* inferior_thread(): Assertion `current_thread_ != nullptr' failed.

The problem is in this line in regcache.c:cooked_read_test:

  /* Switch to the mock thread.  */
  scoped_restore restore_inferior_ptid
    = make_scoped_restore (&inferior_ptid, mock_ptid);

Both gdbarch-selftest.c and regcache.c set up a similar mock context,
but the series the patch above belongs to only updated the
gdbarch-selftest.c context to not write to inferior_ptid directly, and
missed updating regcache.c's.

Instead of copying the fix over to regcache.c, share the mock context
setup code in a new RAII class, based on gdbarch-selftest.c's version.

Also remove the "target already pushed" error from regcache.c, like it
had been removed from gdbarch-selftest.c in the multi-target series.
That check is unnecessary because each inferior now has its own target
stack, and the unit test pushes a target on a separate (mock)
inferior, not the current inferior on entry.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* gdbarch-selftests.c: Don't include inferior.h, gdbthread.h or
	progspace-and-thread.h.  Include scoped-mock-context.h instead.
	(register_to_value_test): Use scoped_mock_context.
	* regcache.c: Include "scoped-mock-context.h".
	(cooked_read_test): Dont error out if a target is already pushed.
	Use scoped_mock_context.  Adjust.
	* scoped-mock-context.h: New file.
---
 gdb/gdbarch-selftests.c   | 39 ++--------------------
 gdb/regcache.c            | 71 +++++++++-------------------------------
 gdb/scoped-mock-context.h | 82 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 99 insertions(+), 93 deletions(-)
 create mode 100644 gdb/scoped-mock-context.h

diff --git a/gdb/gdbarch-selftests.c b/gdb/gdbarch-selftests.c
index 91aa9d87344..4f9adbd9e9c 100644
--- a/gdb/gdbarch-selftests.c
+++ b/gdb/gdbarch-selftests.c
@@ -20,14 +20,12 @@
 #include "defs.h"
 #include "gdbsupport/selftest.h"
 #include "selftest-arch.h"
-#include "inferior.h"
-#include "gdbthread.h"
 #include "target.h"
 #include "test-target.h"
 #include "target-float.h"
 #include "gdbsupport/def-vector.h"
 #include "gdbarch.h"
-#include "progspace-and-thread.h"
+#include "scoped-mock-context.h"
 
 namespace selftests {
 
@@ -71,40 +69,7 @@ register_to_value_test (struct gdbarch *gdbarch)
       builtin->builtin_char32,
     };
 
-  /* Create a mock environment.  An inferior with a thread, with a
-     process_stratum target pushed.  */
-
-  test_target_ops mock_target;
-  ptid_t mock_ptid (1, 1);
-  program_space mock_pspace (new_address_space ());
-  inferior mock_inferior (mock_ptid.pid ());
-  mock_inferior.gdbarch = gdbarch;
-  mock_inferior.aspace = mock_pspace.aspace;
-  mock_inferior.pspace = &mock_pspace;
-  thread_info mock_thread (&mock_inferior, mock_ptid);
-
-  scoped_restore_current_pspace_and_thread restore_pspace_thread;
-
-  scoped_restore restore_thread_list
-    = make_scoped_restore (&mock_inferior.thread_list, &mock_thread);
-
-  /* Add the mock inferior to the inferior list so that look ups by
-     target+ptid can find it.  */
-  scoped_restore restore_inferior_list
-    = make_scoped_restore (&inferior_list, &mock_inferior);
-
-  /* Switch to the mock inferior.  */
-  switch_to_inferior_no_thread (&mock_inferior);
-
-  /* Push the process_stratum target so we can mock accessing
-     registers.  */
-  push_target (&mock_target);
-
-  /* Pop it again on exit (return/exception).  */
-  SCOPE_EXIT { pop_all_targets_at_and_above (process_stratum); };
-
-  /* Switch to the mock thread.  */
-  switch_to_thread (&mock_thread);
+  scoped_mock_context<test_target_ops> mockctx (gdbarch);
 
   struct frame_info *frame = get_current_frame ();
   const int num_regs = gdbarch_num_cooked_regs (gdbarch);
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 6a4359d0f36..4ebb8cb0452 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -22,6 +22,7 @@
 #include "gdbthread.h"
 #include "target.h"
 #include "test-target.h"
+#include "scoped-mock-context.h"
 #include "gdbarch.h"
 #include "gdbcmd.h"
 #include "regcache.h"
@@ -1596,49 +1597,7 @@ class readwrite_regcache : public regcache
 static void
 cooked_read_test (struct gdbarch *gdbarch)
 {
-  /* Error out if debugging something, because we're going to push the
-     test target, which would pop any existing target.  */
-  if (current_top_target ()->stratum () >= process_stratum)
-    error (_("target already pushed"));
-
-  /* Create a mock environment.  An inferior with a thread, with a
-     process_stratum target pushed.  */
-
-  target_ops_no_register mock_target;
-  ptid_t mock_ptid (1, 1);
-  inferior mock_inferior (mock_ptid.pid ());
-  address_space mock_aspace {};
-  mock_inferior.gdbarch = gdbarch;
-  mock_inferior.aspace = &mock_aspace;
-  thread_info mock_thread (&mock_inferior, mock_ptid);
-  mock_inferior.thread_list = &mock_thread;
-
-  /* Add the mock inferior to the inferior list so that look ups by
-     target+ptid can find it.  */
-  scoped_restore restore_inferior_list
-    = make_scoped_restore (&inferior_list);
-  inferior_list = &mock_inferior;
-
-  /* Switch to the mock inferior.  */
-  scoped_restore_current_inferior restore_current_inferior;
-  set_current_inferior (&mock_inferior);
-
-  /* Push the process_stratum target so we can mock accessing
-     registers.  */
-  push_target (&mock_target);
-
-  /* Pop it again on exit (return/exception).  */
-  struct on_exit
-  {
-    ~on_exit ()
-    {
-      pop_all_targets_at_and_above (process_stratum);
-    }
-  } pop_targets;
-
-  /* Switch to the mock thread.  */
-  scoped_restore restore_inferior_ptid
-    = make_scoped_restore (&inferior_ptid, mock_ptid);
+  scoped_mock_context<target_ops_no_register> mockctx (gdbarch);
 
   /* Test that read one raw register from regcache_no_target will go
      to the target layer.  */
@@ -1653,21 +1612,21 @@ cooked_read_test (struct gdbarch *gdbarch)
 	break;
     }
 
-  readwrite_regcache readwrite (&mock_target, gdbarch);
+  readwrite_regcache readwrite (&mockctx.mock_target, gdbarch);
   gdb::def_vector<gdb_byte> buf (register_size (gdbarch, nonzero_regnum));
 
   readwrite.raw_read (nonzero_regnum, buf.data ());
 
   /* raw_read calls target_fetch_registers.  */
-  SELF_CHECK (mock_target.fetch_registers_called > 0);
-  mock_target.reset ();
+  SELF_CHECK (mockctx.mock_target.fetch_registers_called > 0);
+  mockctx.mock_target.reset ();
 
   /* Mark all raw registers valid, so the following raw registers
      accesses won't go to target.  */
   for (auto i = 0; i < gdbarch_num_regs (gdbarch); i++)
     readwrite.raw_update (i);
 
-  mock_target.reset ();
+  mockctx.mock_target.reset ();
   /* Then, read all raw and pseudo registers, and don't expect calling
      to_{fetch,store}_registers.  */
   for (int regnum = 0; regnum < gdbarch_num_cooked_regs (gdbarch); regnum++)
@@ -1680,18 +1639,18 @@ cooked_read_test (struct gdbarch *gdbarch)
       SELF_CHECK (REG_VALID == readwrite.cooked_read (regnum,
 						      inner_buf.data ()));
 
-      SELF_CHECK (mock_target.fetch_registers_called == 0);
-      SELF_CHECK (mock_target.store_registers_called == 0);
-      SELF_CHECK (mock_target.xfer_partial_called == 0);
+      SELF_CHECK (mockctx.mock_target.fetch_registers_called == 0);
+      SELF_CHECK (mockctx.mock_target.store_registers_called == 0);
+      SELF_CHECK (mockctx.mock_target.xfer_partial_called == 0);
 
-      mock_target.reset ();
+      mockctx.mock_target.reset ();
     }
 
   readonly_detached_regcache readonly (readwrite);
 
   /* GDB may go to target layer to fetch all registers and memory for
      readonly regcache.  */
-  mock_target.reset ();
+  mockctx.mock_target.reset ();
 
   for (int regnum = 0; regnum < gdbarch_num_cooked_regs (gdbarch); regnum++)
     {
@@ -1749,11 +1708,11 @@ cooked_read_test (struct gdbarch *gdbarch)
 	    }
 	}
 
-      SELF_CHECK (mock_target.fetch_registers_called == 0);
-      SELF_CHECK (mock_target.store_registers_called == 0);
-      SELF_CHECK (mock_target.xfer_partial_called == 0);
+      SELF_CHECK (mockctx.mock_target.fetch_registers_called == 0);
+      SELF_CHECK (mockctx.mock_target.store_registers_called == 0);
+      SELF_CHECK (mockctx.mock_target.xfer_partial_called == 0);
 
-      mock_target.reset ();
+      mockctx.mock_target.reset ();
     }
 }
 
diff --git a/gdb/scoped-mock-context.h b/gdb/scoped-mock-context.h
new file mode 100644
index 00000000000..461c2a35388
--- /dev/null
+++ b/gdb/scoped-mock-context.h
@@ -0,0 +1,82 @@
+/* RAII type to create a temporary mock context.
+
+   Copyright (C) 2020 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+#ifndef SCOPED_MOCK_CONTEXT_H
+#define SCOPED_MOCK_CONTEXT_H
+
+#include "inferior.h"
+#include "gdbthread.h"
+#include "progspace.h"
+#include "progspace-and-thread.h"
+
+#if GDB_SELF_TEST
+namespace selftests {
+
+/* RAII type to create (and switch to) a temporary mock context.  An
+   inferior with a thread, with a process_stratum target pushed.  */
+
+template<typename Target>
+struct scoped_mock_context
+{
+  /* Order here is important.  */
+
+  Target mock_target;
+  ptid_t mock_ptid {1, 1};
+  program_space mock_pspace {new_address_space ()};
+  inferior mock_inferior {mock_ptid.pid ()};
+  thread_info mock_thread {&mock_inferior, mock_ptid};
+
+  scoped_restore_current_pspace_and_thread restore_pspace_thread;
+
+  scoped_restore_tmpl<thread_info *> restore_thread_list
+    {&mock_inferior.thread_list, &mock_thread};
+
+  /* Add the mock inferior to the inferior list so that look ups by
+     target+ptid can find it.  */
+  scoped_restore_tmpl<inferior *> restore_inferior_list
+    {&inferior_list, &mock_inferior};
+
+  explicit scoped_mock_context (gdbarch *gdbarch)
+  {
+    mock_inferior.gdbarch = gdbarch;
+    mock_inferior.aspace = mock_pspace.aspace;
+    mock_inferior.pspace = &mock_pspace;
+
+    /* Switch to the mock inferior.  */
+    switch_to_inferior_no_thread (&mock_inferior);
+
+    /* Push the process_stratum target so we can mock accessing
+       registers.  */
+    gdb_assert (mock_target.stratum () == process_stratum);
+    push_target (&mock_target);
+
+    /* Switch to the mock thread.  */
+    switch_to_thread (&mock_thread);
+  }
+
+  ~scoped_mock_context ()
+  {
+    pop_all_targets_at_and_above (process_stratum);
+  }
+};
+
+} // namespace selftests
+#endif /* GDB_SELF_TEST */
+
+#endif /* !defined (SCOPED_MOCK_CONTEXT_H) */

base-commit: 39ff0b812324f4b050bb0b367b269db6d4d0cb8b
-- 
2.14.5

Re: [PATCH] Fix "maint selftest" regression, add struct, scoped_mock_context

[Andrew Burgess]

* Pedro Alves <palves@redhat.com> [2020-06-23 16:38:02 +0100]:

> On 6/23/20 3:26 PM, Pedro Alves via Gdb-patches wrote:
> > On 6/23/20 2:37 PM, Andrew Burgess wrote:
> 
> >> I suspect that the problem might be this line in regcache.c:cooked_read_test:
> >>
> >>   /* Switch to the mock thread.  */
> >>   scoped_restore restore_inferior_ptid
> >>     = make_scoped_restore (&inferior_ptid, mock_ptid);
> >>
> >> My suspicion from a quick read of your patch above is that we need to
> >> do more than just set inferior_ptid here now.
> > 
> > Indeed.  Thanks for reporting it.  
> > 
> > I fixed this in gdbarch-selftests.c, where this code is duplicated,
> > but missed regcache.c.  I'll fix it.
> > 
> 
> Like so.  WDYT?

LGTM.

Thanks,
Andrew

> 
> From d3438cc6c48bd77880da7ef7a449edbbfe990e37 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Tue, 23 Jun 2020 15:18:41 +0100
> Subject: [PATCH] Fix "maint selftest" regression, add struct
>  scoped_mock_context
> 
> This commit:
> 
>  commit 3922b302645fda04da42a5279399578ae2f6206c
>  Author:     Pedro Alves <palves@redhat.com>
>  AuthorDate: Thu Jun 18 21:28:37 2020 +0100
> 
>     Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR 25412)
> 
> caused a regression for gdb.gdb/unittest.exp when GDB is configured
> with --enable-targets=all.  The failure is:
> 
>   gdb/thread.c:95: internal-error: thread_info* inferior_thread(): Assertion `current_thread_ != nullptr' failed.
> 
> The problem is in this line in regcache.c:cooked_read_test:
> 
>   /* Switch to the mock thread.  */
>   scoped_restore restore_inferior_ptid
>     = make_scoped_restore (&inferior_ptid, mock_ptid);
> 
> Both gdbarch-selftest.c and regcache.c set up a similar mock context,
> but the series the patch above belongs to only updated the
> gdbarch-selftest.c context to not write to inferior_ptid directly, and
> missed updating regcache.c's.
> 
> Instead of copying the fix over to regcache.c, share the mock context
> setup code in a new RAII class, based on gdbarch-selftest.c's version.
> 
> Also remove the "target already pushed" error from regcache.c, like it
> had been removed from gdbarch-selftest.c in the multi-target series.
> That check is unnecessary because each inferior now has its own target
> stack, and the unit test pushes a target on a separate (mock)
> inferior, not the current inferior on entry.
> 
> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> 	* gdbarch-selftests.c: Don't include inferior.h, gdbthread.h or
> 	progspace-and-thread.h.  Include scoped-mock-context.h instead.
> 	(register_to_value_test): Use scoped_mock_context.
> 	* regcache.c: Include "scoped-mock-context.h".
> 	(cooked_read_test): Dont error out if a target is already pushed.
> 	Use scoped_mock_context.  Adjust.
> 	* scoped-mock-context.h: New file.
> ---
>  gdb/gdbarch-selftests.c   | 39 ++--------------------
>  gdb/regcache.c            | 71 +++++++++-------------------------------
>  gdb/scoped-mock-context.h | 82 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 99 insertions(+), 93 deletions(-)
>  create mode 100644 gdb/scoped-mock-context.h
> 
> diff --git a/gdb/gdbarch-selftests.c b/gdb/gdbarch-selftests.c
> index 91aa9d87344..4f9adbd9e9c 100644
> --- a/gdb/gdbarch-selftests.c
> +++ b/gdb/gdbarch-selftests.c
> @@ -20,14 +20,12 @@
>  #include "defs.h"
>  #include "gdbsupport/selftest.h"
>  #include "selftest-arch.h"
> -#include "inferior.h"
> -#include "gdbthread.h"
>  #include "target.h"
>  #include "test-target.h"
>  #include "target-float.h"
>  #include "gdbsupport/def-vector.h"
>  #include "gdbarch.h"
> -#include "progspace-and-thread.h"
> +#include "scoped-mock-context.h"
>  
>  namespace selftests {
>  
> @@ -71,40 +69,7 @@ register_to_value_test (struct gdbarch *gdbarch)
>        builtin->builtin_char32,
>      };
>  
> -  /* Create a mock environment.  An inferior with a thread, with a
> -     process_stratum target pushed.  */
> -
> -  test_target_ops mock_target;
> -  ptid_t mock_ptid (1, 1);
> -  program_space mock_pspace (new_address_space ());
> -  inferior mock_inferior (mock_ptid.pid ());
> -  mock_inferior.gdbarch = gdbarch;
> -  mock_inferior.aspace = mock_pspace.aspace;
> -  mock_inferior.pspace = &mock_pspace;
> -  thread_info mock_thread (&mock_inferior, mock_ptid);
> -
> -  scoped_restore_current_pspace_and_thread restore_pspace_thread;
> -
> -  scoped_restore restore_thread_list
> -    = make_scoped_restore (&mock_inferior.thread_list, &mock_thread);
> -
> -  /* Add the mock inferior to the inferior list so that look ups by
> -     target+ptid can find it.  */
> -  scoped_restore restore_inferior_list
> -    = make_scoped_restore (&inferior_list, &mock_inferior);
> -
> -  /* Switch to the mock inferior.  */
> -  switch_to_inferior_no_thread (&mock_inferior);
> -
> -  /* Push the process_stratum target so we can mock accessing
> -     registers.  */
> -  push_target (&mock_target);
> -
> -  /* Pop it again on exit (return/exception).  */
> -  SCOPE_EXIT { pop_all_targets_at_and_above (process_stratum); };
> -
> -  /* Switch to the mock thread.  */
> -  switch_to_thread (&mock_thread);
> +  scoped_mock_context<test_target_ops> mockctx (gdbarch);
>  
>    struct frame_info *frame = get_current_frame ();
>    const int num_regs = gdbarch_num_cooked_regs (gdbarch);
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 6a4359d0f36..4ebb8cb0452 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -22,6 +22,7 @@
>  #include "gdbthread.h"
>  #include "target.h"
>  #include "test-target.h"
> +#include "scoped-mock-context.h"
>  #include "gdbarch.h"
>  #include "gdbcmd.h"
>  #include "regcache.h"
> @@ -1596,49 +1597,7 @@ class readwrite_regcache : public regcache
>  static void
>  cooked_read_test (struct gdbarch *gdbarch)
>  {
> -  /* Error out if debugging something, because we're going to push the
> -     test target, which would pop any existing target.  */
> -  if (current_top_target ()->stratum () >= process_stratum)
> -    error (_("target already pushed"));
> -
> -  /* Create a mock environment.  An inferior with a thread, with a
> -     process_stratum target pushed.  */
> -
> -  target_ops_no_register mock_target;
> -  ptid_t mock_ptid (1, 1);
> -  inferior mock_inferior (mock_ptid.pid ());
> -  address_space mock_aspace {};
> -  mock_inferior.gdbarch = gdbarch;
> -  mock_inferior.aspace = &mock_aspace;
> -  thread_info mock_thread (&mock_inferior, mock_ptid);
> -  mock_inferior.thread_list = &mock_thread;
> -
> -  /* Add the mock inferior to the inferior list so that look ups by
> -     target+ptid can find it.  */
> -  scoped_restore restore_inferior_list
> -    = make_scoped_restore (&inferior_list);
> -  inferior_list = &mock_inferior;
> -
> -  /* Switch to the mock inferior.  */
> -  scoped_restore_current_inferior restore_current_inferior;
> -  set_current_inferior (&mock_inferior);
> -
> -  /* Push the process_stratum target so we can mock accessing
> -     registers.  */
> -  push_target (&mock_target);
> -
> -  /* Pop it again on exit (return/exception).  */
> -  struct on_exit
> -  {
> -    ~on_exit ()
> -    {
> -      pop_all_targets_at_and_above (process_stratum);
> -    }
> -  } pop_targets;
> -
> -  /* Switch to the mock thread.  */
> -  scoped_restore restore_inferior_ptid
> -    = make_scoped_restore (&inferior_ptid, mock_ptid);
> +  scoped_mock_context<target_ops_no_register> mockctx (gdbarch);
>  
>    /* Test that read one raw register from regcache_no_target will go
>       to the target layer.  */
> @@ -1653,21 +1612,21 @@ cooked_read_test (struct gdbarch *gdbarch)
>  	break;
>      }
>  
> -  readwrite_regcache readwrite (&mock_target, gdbarch);
> +  readwrite_regcache readwrite (&mockctx.mock_target, gdbarch);
>    gdb::def_vector<gdb_byte> buf (register_size (gdbarch, nonzero_regnum));
>  
>    readwrite.raw_read (nonzero_regnum, buf.data ());
>  
>    /* raw_read calls target_fetch_registers.  */
> -  SELF_CHECK (mock_target.fetch_registers_called > 0);
> -  mock_target.reset ();
> +  SELF_CHECK (mockctx.mock_target.fetch_registers_called > 0);
> +  mockctx.mock_target.reset ();
>  
>    /* Mark all raw registers valid, so the following raw registers
>       accesses won't go to target.  */
>    for (auto i = 0; i < gdbarch_num_regs (gdbarch); i++)
>      readwrite.raw_update (i);
>  
> -  mock_target.reset ();
> +  mockctx.mock_target.reset ();
>    /* Then, read all raw and pseudo registers, and don't expect calling
>       to_{fetch,store}_registers.  */
>    for (int regnum = 0; regnum < gdbarch_num_cooked_regs (gdbarch); regnum++)
> @@ -1680,18 +1639,18 @@ cooked_read_test (struct gdbarch *gdbarch)
>        SELF_CHECK (REG_VALID == readwrite.cooked_read (regnum,
>  						      inner_buf.data ()));
>  
> -      SELF_CHECK (mock_target.fetch_registers_called == 0);
> -      SELF_CHECK (mock_target.store_registers_called == 0);
> -      SELF_CHECK (mock_target.xfer_partial_called == 0);
> +      SELF_CHECK (mockctx.mock_target.fetch_registers_called == 0);
> +      SELF_CHECK (mockctx.mock_target.store_registers_called == 0);
> +      SELF_CHECK (mockctx.mock_target.xfer_partial_called == 0);
>  
> -      mock_target.reset ();
> +      mockctx.mock_target.reset ();
>      }
>  
>    readonly_detached_regcache readonly (readwrite);
>  
>    /* GDB may go to target layer to fetch all registers and memory for
>       readonly regcache.  */
> -  mock_target.reset ();
> +  mockctx.mock_target.reset ();
>  
>    for (int regnum = 0; regnum < gdbarch_num_cooked_regs (gdbarch); regnum++)
>      {
> @@ -1749,11 +1708,11 @@ cooked_read_test (struct gdbarch *gdbarch)
>  	    }
>  	}
>  
> -      SELF_CHECK (mock_target.fetch_registers_called == 0);
> -      SELF_CHECK (mock_target.store_registers_called == 0);
> -      SELF_CHECK (mock_target.xfer_partial_called == 0);
> +      SELF_CHECK (mockctx.mock_target.fetch_registers_called == 0);
> +      SELF_CHECK (mockctx.mock_target.store_registers_called == 0);
> +      SELF_CHECK (mockctx.mock_target.xfer_partial_called == 0);
>  
> -      mock_target.reset ();
> +      mockctx.mock_target.reset ();
>      }
>  }
>  
> diff --git a/gdb/scoped-mock-context.h b/gdb/scoped-mock-context.h
> new file mode 100644
> index 00000000000..461c2a35388
> --- /dev/null
> +++ b/gdb/scoped-mock-context.h
> @@ -0,0 +1,82 @@
> +/* RAII type to create a temporary mock context.
> +
> +   Copyright (C) 2020 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   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/>.  */
> +
> +#ifndef SCOPED_MOCK_CONTEXT_H
> +#define SCOPED_MOCK_CONTEXT_H
> +
> +#include "inferior.h"
> +#include "gdbthread.h"
> +#include "progspace.h"
> +#include "progspace-and-thread.h"
> +
> +#if GDB_SELF_TEST
> +namespace selftests {
> +
> +/* RAII type to create (and switch to) a temporary mock context.  An
> +   inferior with a thread, with a process_stratum target pushed.  */
> +
> +template<typename Target>
> +struct scoped_mock_context
> +{
> +  /* Order here is important.  */
> +
> +  Target mock_target;
> +  ptid_t mock_ptid {1, 1};
> +  program_space mock_pspace {new_address_space ()};
> +  inferior mock_inferior {mock_ptid.pid ()};
> +  thread_info mock_thread {&mock_inferior, mock_ptid};
> +
> +  scoped_restore_current_pspace_and_thread restore_pspace_thread;
> +
> +  scoped_restore_tmpl<thread_info *> restore_thread_list
> +    {&mock_inferior.thread_list, &mock_thread};
> +
> +  /* Add the mock inferior to the inferior list so that look ups by
> +     target+ptid can find it.  */
> +  scoped_restore_tmpl<inferior *> restore_inferior_list
> +    {&inferior_list, &mock_inferior};
> +
> +  explicit scoped_mock_context (gdbarch *gdbarch)
> +  {
> +    mock_inferior.gdbarch = gdbarch;
> +    mock_inferior.aspace = mock_pspace.aspace;
> +    mock_inferior.pspace = &mock_pspace;
> +
> +    /* Switch to the mock inferior.  */
> +    switch_to_inferior_no_thread (&mock_inferior);
> +
> +    /* Push the process_stratum target so we can mock accessing
> +       registers.  */
> +    gdb_assert (mock_target.stratum () == process_stratum);
> +    push_target (&mock_target);
> +
> +    /* Switch to the mock thread.  */
> +    switch_to_thread (&mock_thread);
> +  }
> +
> +  ~scoped_mock_context ()
> +  {
> +    pop_all_targets_at_and_above (process_stratum);
> +  }
> +};
> +
> +} // namespace selftests
> +#endif /* GDB_SELF_TEST */
> +
> +#endif /* !defined (SCOPED_MOCK_CONTEXT_H) */
> 
> base-commit: 39ff0b812324f4b050bb0b367b269db6d4d0cb8b
> -- 
> 2.14.5
> 

Re: [PATCH] Fix "maint selftest" regression, add struct, scoped_mock_context

[Pedro Alves]

On 6/23/20 5:34 PM, Andrew Burgess wrote:
> * Pedro Alves <palves@redhat.com> [2020-06-23 16:38:02 +0100]:
> 
>> On 6/23/20 3:26 PM, Pedro Alves via Gdb-patches wrote:
>>> On 6/23/20 2:37 PM, Andrew Burgess wrote:
>>
>>>> I suspect that the problem might be this line in regcache.c:cooked_read_test:
>>>>
>>>>   /* Switch to the mock thread.  */
>>>>   scoped_restore restore_inferior_ptid
>>>>     = make_scoped_restore (&inferior_ptid, mock_ptid);
>>>>
>>>> My suspicion from a quick read of your patch above is that we need to
>>>> do more than just set inferior_ptid here now.
>>>
>>> Indeed.  Thanks for reporting it.  
>>>
>>> I fixed this in gdbarch-selftests.c, where this code is duplicated,
>>> but missed regcache.c.  I'll fix it.
>>>
>>
>> Like so.  WDYT?
> 
> LGTM.
> 

Thanks, I've pushed it.

Pedro Alves

Re: [PATCH 00/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[Hannes Domani]

 Am Dienstag, 14. April 2020, 19:54:45 MESZ hat Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> Folgendes geschrieben:

> In PR/25412, Simon noticed that after the multi-target series, the
> tid-reuse.exp testcase manages to create a duplicate thread in the
> thread list.  Or rather, two threads with the same PTID.

I'm sorry to hijack this thread, but I just wanted to apply this patch series
for testing, but I'm already stuck on the first step.
How do you get patches from these mails that I then can use with "git am"
(or some other git command)?

For single patches I would just use the raw email, that seems to work fine,
but I thought there probably exists a simpler way for larger series that I
don't know about.


Regards
Hannes Domani

Re: [PATCH 00/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[Pedro Alves]

On 4/14/20 7:46 PM, Hannes Domani via Gdb-patches wrote:
>  Am Dienstag, 14. April 2020, 19:54:45 MESZ hat Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> Folgendes geschrieben:
> 
>> In PR/25412, Simon noticed that after the multi-target series, the
>> tid-reuse.exp testcase manages to create a duplicate thread in the
>> thread list.  Or rather, two threads with the same PTID.
> 
> I'm sorry to hijack this thread, but I just wanted to apply this patch series
> for testing, but I'm already stuck on the first step.

Thanks much for testing!

> How do you get patches from these mails that I then can use with "git am"
> (or some other git command)?
> 
> For single patches I would just use the raw email, that seems to work fine,
> but I thought there probably exists a simpler way for larger series that I
> don't know about.

In Thunderbird, I select all the emails, right click, and then select
"save selected messages" -> "as Mbox file".  Them I use git am.
I'm not sure whether that's native functionality, or whether it
comes from the "ImportExportTools" add-on.

I have 

[format]
        useAutoBase = true

in my .gitconfig, which is what puts that 

 base-commit: dd1cab0694592099854e66467319253954c93764

line at the bottom of the cover letter.  That's the commit
on top of which the series applies.

But, to make it simpler, I've now pushed the series to the
users/palves/current-thread branch on sourceware.org, so you
can simply pick it from there.

Thanks,
Pedro Alves

Re: [PATCH 00/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[Simon Marchi]

On 2020-04-14 3:24 p.m., Pedro Alves via Gdb-patches wrote:
> On 4/14/20 7:46 PM, Hannes Domani via Gdb-patches wrote:
>>  Am Dienstag, 14. April 2020, 19:54:45 MESZ hat Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> Folgendes geschrieben:
>>
>>> In PR/25412, Simon noticed that after the multi-target series, the
>>> tid-reuse.exp testcase manages to create a duplicate thread in the
>>> thread list.  Or rather, two threads with the same PTID.
>>
>> I'm sorry to hijack this thread, but I just wanted to apply this patch series
>> for testing, but I'm already stuck on the first step.
> 
> Thanks much for testing!
> 
>> How do you get patches from these mails that I then can use with "git am"
>> (or some other git command)?
>>
>> For single patches I would just use the raw email, that seems to work fine,
>> but I thought there probably exists a simpler way for larger series that I
>> don't know about.
> 
> In Thunderbird, I select all the emails, right click, and then select
> "save selected messages" -> "as Mbox file".  Them I use git am.
> I'm not sure whether that's native functionality, or whether it
> comes from the "ImportExportTools" add-on.

I don't see it in my Thunderbird, so it must be the add-on.

I do almost the same, but using some built-in features: select the messages and
hit "ctrl-s" to save the messages in some directory.  In this case, they are
saved as one message per file.  I then do `git am path/to/that/directory/*`.

For a single patch, I sometimes display the source (ctrl-u) and copy it all.  I
then do `git am`, which makes it read on stdin.  Paste, then ctrl-d to send
EOF.  The caveat here is that if the patch contains some weird ascii character
(like the "horizontal tab" characters that are sometimes in the GDB source),
it looks like the paste messes it up and the patch doesn't apply.  Saving the
file as described previously works fine.

This works well when the patches were send with git-send-email, and is almost
always a pain otherwise.

> 
> I have 
> 
> [format]
>         useAutoBase = true
> 
> in my .gitconfig, which is what puts that 
> 
>  base-commit: dd1cab0694592099854e66467319253954c93764
> 
> line at the bottom of the cover letter.  That's the commit
> on top of which the series applies.

This means you also save the cover letter in your mbox file?

Simon

Re: [PATCH 00/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[Pedro Alves]

On 4/15/20 4:04 PM, Simon Marchi wrote:

>>
>> I have 
>>
>> [format]
>>         useAutoBase = true
>>
>> in my .gitconfig, which is what puts that 
>>
>>  base-commit: dd1cab0694592099854e66467319253954c93764
>>
>> line at the bottom of the cover letter.  That's the commit
>> on top of which the series applies.
> 
> This means you also save the cover letter in your mbox file?
Nope, I don't think "git am" understands that base-commit
line.  I tried it now, and "git am" complained like so:

 $ git am /tmp/test.mbox
 Patch is empty.
 When you have resolved this problem, run "git am --continue".
 If you prefer to skip this patch, run "git am --skip" instead.
 To restore the original branch and stop patching, run "git am --abort".

No patch was applied, so this was the cover letter.

That line in the cover letter is put there when I do "git format-patch"
(or "git send-email"), so it's in the opposite direction of previously
described procedure of getting patches from Thunderbird into "git am".

Thanks,
Pedro Alves

Re: [PATCH 00/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[Simon Marchi]

On 2020-04-14 1:54 p.m., Pedro Alves via Gdb-patches wrote:
> In PR/25412, Simon noticed that after the multi-target series, the
> tid-reuse.exp testcase manages to create a duplicate thread in the
> thread list.  Or rather, two threads with the same PTID.
> 
> This in turn exposes a design problem in GDB.  The inferior_thread()
> function looks up the current thread based on inferior_ptid:
> 
>  struct thread_info*
>  inferior_thread (void)
>  {
>    struct thread_info *tp = find_thread_ptid (current_inferior (), inferior_ptid);
>    gdb_assert (tp);
>    return tp;
>  }
> 
> But if there can be more than one thread in the thread list with the
> same ptid_t, inferior_thread() may well return the wrong thread.

I don't quite understand this part of the explanation.  Thread lists are
per-inferior, and an inferior is specific to one target, and ptids are
unique per-target.  So it's not possible at the moment (or at least not
expected) to have two threads with the same ptid in one specific list.

I am having trouble connecting the dots between this explanation and the
problem originally reported, which IIRC correctly involves a single
inferior.

> This series fixes this by making the current thread be a global
> thread_info pointer that is written to directly by switch_to_thread,
> etc., and making inferior_thread() return that pointer, instead of
> having inferior_thread() lookup up the inferior_ptid thread, by
> ptid_t.  You can look at this as a continuation of the effort of using
> more thread_info pointers instead of ptids when possible.

Even though I don't understand the explanation above, I think this is
a good step forward.

> This change required auditing the whole codebase for places where we
> were writing to inferior_ptid directly to change the current thread,
> and change them to use switch_to_thread instead or one of its
> siblings, because otherwise inferior_thread() and inferior_ptid would
> get out of sync and inferior_thread() would return a thread unrelated
> to the new inferior_ptid we wanted to switch to.  That was all
> (hopefully) done in all the patches leading up to the last one.
> 
> After this, inferior_ptid still exists, but it is mostly read-only and
> mainly used by target backend code.  It is also relied on by a number
> of target methods as a global input argument.  E.g., the target_resume
> interface and the memory reading routines -- we still need it there
> because we need to be able to access memory off of processes for which
> we don't have a corresponding inferior/thread object, like when
> handling forks.  Maybe we could pass down a context explicitly to
> target_read_memory, etc.

I would like to do that eventually.

> Most of the host-/native-specific code here is untested.  I did my
> best, but I won't be surprised if more tweaking is necessary.
> 
> Testing on native x86_64 GNU/Linux is regression free for me.  Testing
> against gdbserver has regressed significantly in the past months and
> is becoming difficult to run with a high number of long timeout
> sequences; really looks like people aren't paying much attention to
> that.  I think this series doesn't regress gdbserver, but it's getting
> hard to tell.  :-/

Oops... we'll have to do some bisecting.

Simon

Re: [PATCH 00/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[Pedro Alves]

On 4/15/20 3:46 PM, Simon Marchi wrote:
> On 2020-04-14 1:54 p.m., Pedro Alves via Gdb-patches wrote:
>> In PR/25412, Simon noticed that after the multi-target series, the
>> tid-reuse.exp testcase manages to create a duplicate thread in the
>> thread list.  Or rather, two threads with the same PTID.
>>
>> This in turn exposes a design problem in GDB.  The inferior_thread()
>> function looks up the current thread based on inferior_ptid:
>>
>>  struct thread_info*
>>  inferior_thread (void)
>>  {
>>    struct thread_info *tp = find_thread_ptid (current_inferior (), inferior_ptid);
>>    gdb_assert (tp);
>>    return tp;
>>  }
>>
>> But if there can be more than one thread in the thread list with the
>> same ptid_t, inferior_thread() may well return the wrong thread.
> 
> I don't quite understand this part of the explanation.  Thread lists are
> per-inferior, and an inferior is specific to one target, and ptids are
> unique per-target.  So it's not possible at the moment (or at least not
> expected) to have two threads with the same ptid in one specific list.

The tid-reuse.exp testcase is about:

 # Test running a program that spawns enough threads that the tid of an
 # exited thread is reused.  GDB should not crash when this happens.

I.e., the testcase spawns enough short-lived threads that we end up
in add_thread_silent adding a new thread with the same ptid as
a preexisting thread, because the kernel tids eventually wrap around.
In this scenario, you know the preexisting thread with that ptid must
be gone, since otherwise the kernel would not be reusing the thread id.

But if that thread that is gone was the currently selected
thread, or somewhere throughout gdb something holds a reference
to it, then we can't delete that exiting thread immediately.  That's
how you end up with more than one thread with the same ptid in the
thread list.  Only one of those threads will be live, all other
"dups" will be exited threads.

So given that currently inferior_thread() does a look up by
ptid, it can happen that GDB meant to have the live thread
selected temporarily, say, but inferior_thread() returns the
exited thread. Like, this could fail:

switch_to_thread (exited_thread);
{
 scoped_restore_current_thread restore_thread;
 switch_to_thread (live_thread);
 gdb_assert (inferior_thread () == live_thread);
}

Note this happens with a single inferior.

Thanks,
Pedro Alves

Re: [PATCH 00/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[Simon Marchi]

On 2020-04-15 11:33 a.m., Pedro Alves wrote:
> On 4/15/20 3:46 PM, Simon Marchi wrote:
>> On 2020-04-14 1:54 p.m., Pedro Alves via Gdb-patches wrote:
>>> In PR/25412, Simon noticed that after the multi-target series, the
>>> tid-reuse.exp testcase manages to create a duplicate thread in the
>>> thread list.  Or rather, two threads with the same PTID.
>>>
>>> This in turn exposes a design problem in GDB.  The inferior_thread()
>>> function looks up the current thread based on inferior_ptid:
>>>
>>>  struct thread_info*
>>>  inferior_thread (void)
>>>  {
>>>    struct thread_info *tp = find_thread_ptid (current_inferior (), inferior_ptid);
>>>    gdb_assert (tp);
>>>    return tp;
>>>  }
>>>
>>> But if there can be more than one thread in the thread list with the
>>> same ptid_t, inferior_thread() may well return the wrong thread.
>>
>> I don't quite understand this part of the explanation.  Thread lists are
>> per-inferior, and an inferior is specific to one target, and ptids are
>> unique per-target.  So it's not possible at the moment (or at least not
>> expected) to have two threads with the same ptid in one specific list.
> 
> The tid-reuse.exp testcase is about:
> 
>  # Test running a program that spawns enough threads that the tid of an
>  # exited thread is reused.  GDB should not crash when this happens.
> 
> I.e., the testcase spawns enough short-lived threads that we end up
> in add_thread_silent adding a new thread with the same ptid as
> a preexisting thread, because the kernel tids eventually wrap around.
> In this scenario, you know the preexisting thread with that ptid must
> be gone, since otherwise the kernel would not be reusing the thread id.
> 
> But if that thread that is gone was the currently selected
> thread, or somewhere throughout gdb something holds a reference
> to it, then we can't delete that exiting thread immediately.  That's
> how you end up with more than one thread with the same ptid in the
> thread list.  Only one of those threads will be live, all other
> "dups" will be exited threads.
> 
> So given that currently inferior_thread() does a look up by
> ptid, it can happen that GDB meant to have the live thread
> selected temporarily, say, but inferior_thread() returns the
> exited thread. Like, this could fail:
> 
> switch_to_thread (exited_thread);
> {
>  scoped_restore_current_thread restore_thread;
>  switch_to_thread (live_thread);
>  gdb_assert (inferior_thread () == live_thread);
> }
> 
> Note this happens with a single inferior.
> 
> Thanks,
> Pedro Alves
> 

Ok thanks, that clarifies it.

Simon

Re: [PATCH 00/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> writes:

I meant to mention, I skimmed through this series and didn't see
anything objectionable.  It's hard to know about this kind of thing
without trying it (which I didn't do); but IMO it's fine to land this
and then iron out anything that pops up.

Pedro> After this, inferior_ptid still exists, but it is mostly read-only and
Pedro> mainly used by target backend code.

Could it be made completely read-only?

Pedro> It is also relied on by a number
Pedro> of target methods as a global input argument.  E.g., the target_resume
Pedro> interface and the memory reading routines -- we still need it there
Pedro> because we need to be able to access memory off of processes for which
Pedro> we don't have a corresponding inferior/thread object, like when
Pedro> handling forks.  Maybe we could pass down a context explicitly to
Pedro> target_read_memory, etc.

This sounds like a good direction.

Tom

Re: [PATCH 00/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[Pedro Alves]

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

On 4/17/20 9:20 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> I meant to mention, I skimmed through this series and didn't see
> anything objectionable.  It's hard to know about this kind of thing
> without trying it (which I didn't do); but IMO it's fine to land this
> and then iron out anything that pops up.
> 
> Pedro> After this, inferior_ptid still exists, but it is mostly read-only and
> Pedro> mainly used by target backend code.
> 
> Could it be made completely read-only?

We could, with e.g. a hack like in the attached patch.

That patch is sufficient to make GDB build on x86-64 GNU/Linux, but
certainly it would require more changes across all the native backends.
I'm not sure it really buys us much, since the remaining writes are just
as easily found by grepping for "inferior_ptid =".

> 
> Pedro> It is also relied on by a number
> Pedro> of target methods as a global input argument.  E.g., the target_resume
> Pedro> interface and the memory reading routines -- we still need it there
> Pedro> because we need to be able to access memory off of processes for which
> Pedro> we don't have a corresponding inferior/thread object, like when
> Pedro> handling forks.  Maybe we could pass down a context explicitly to
> Pedro> target_read_memory, etc.
> 
> This sounds like a good direction.

Thanks,
Pedro Alves

[-- Attachment #2: 0001-inferior_ptid_t.patch --]
[-- Type: text/x-patch, Size: 8081 bytes --]

From dd45a29433d527344f0c89bed01dfa645d845351 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 18 Jun 2020 20:35:10 +0100
Subject: [PATCH] inferior_ptid_t

---
 gdb/breakpoint.c   |  2 +-
 gdb/infcmd.c       |  2 +-
 gdb/inferior.h     | 33 ++++++++++++++++++++++++++++++++-
 gdb/infrun.c       |  5 ++---
 gdb/linux-nat.c    |  2 +-
 gdb/proc-service.c |  2 +-
 gdb/regcache.c     |  5 +++--
 gdb/remote.c       |  2 +-
 gdb/thread.c       |  4 ++--
 9 files changed, 44 insertions(+), 13 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index aead882acd..dc080122bf 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3584,7 +3584,7 @@ detach_breakpoints (ptid_t ptid)
     error (_("Cannot detach breakpoints of inferior_ptid"));
 
   /* Set inferior_ptid; remove_breakpoint_1 uses this global.  */
-  inferior_ptid = ptid;
+  inferior_ptid.set (ptid);
   ALL_BP_LOCATIONS (bl, blp_tmp)
   {
     if (bl->pspace != inf->pspace)
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 891da91c80..9b06f03433 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -88,7 +88,7 @@ static char *inferior_io_terminal_scratch;
    being debugged it should be nonzero (currently 3 is used) for remote
    debugging.  */
 
-ptid_t inferior_ptid;
+inferior_ptid_t inferior_ptid;
 
 /* Nonzero if stopped due to completion of a stack dummy routine.  */
 
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 95af474eed..79bd9cb351 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -122,10 +122,41 @@ extern void clear_sigint_trap (void);
 extern void set_inferior_io_terminal (const char *terminal_name);
 extern const char *get_inferior_io_terminal (void);
 
+class inferior_ptid_t
+{
+public:
+  int pid () const { return m_ptid.pid (); }
+  bool lwp_p () const { return m_ptid.lwp_p (); }
+  long lwp () const { return m_ptid.lwp (); }
+  bool tid_p () const { return m_ptid.tid_p (); }
+  long tid () const { return m_ptid.tid (); }
+  long is_tid () const { return m_ptid.tid (); }
+  bool is_pid () const { return m_ptid.is_pid (); }
+
+  bool matches (const ptid_t &filter) const
+  { return m_ptid.matches (filter); }
+
+  void set (ptid_t ptid) { m_ptid = ptid; }
+
+  operator ptid_t () { return m_ptid; }
+
+  friend bool operator== (const inferior_ptid_t &lhs, const ptid_t &rhs)
+  { return lhs.m_ptid == rhs; }
+  friend bool operator== (const ptid_t &lhs, const inferior_ptid_t &rhs)
+  { return lhs == rhs.m_ptid; }
+  friend bool operator!= (const inferior_ptid_t &lhs, const ptid_t &rhs)
+  { return lhs.m_ptid != rhs; }
+  friend bool operator!= (const ptid_t &lhs, const inferior_ptid_t &rhs)
+  { return lhs != rhs.m_ptid; }
+
+private:
+  ptid_t m_ptid = null_ptid;
+};
+
 /* Collected pid, tid, etc. of the debugged inferior.  When there's
    no inferior, inferior_ptid.pid () will be 0.  */
 
-extern ptid_t inferior_ptid;
+extern inferior_ptid_t inferior_ptid;
 
 extern void generic_mourn_inferior (void);
 
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 7bc405f103..0f38485536 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1835,7 +1835,7 @@ write_memory_ptid (ptid_t ptid, CORE_ADDR memaddr,
 {
   scoped_restore save_inferior_ptid = make_scoped_restore (&inferior_ptid);
 
-  inferior_ptid = ptid;
+  inferior_ptid.set (ptid);
   write_memory (memaddr, myaddr, len);
 }
 
@@ -2071,7 +2071,7 @@ static void
 infrun_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid)
 {
   if (inferior_ptid == old_ptid)
-    inferior_ptid = new_ptid;
+    inferior_ptid.set (new_ptid);
 }
 
 \f
@@ -9700,7 +9700,6 @@ enabled by default on some platforms."),
 			   &setlist, &showlist);
 
   /* ptid initializations */
-  inferior_ptid = null_ptid;
   target_last_wait_ptid = minus_one_ptid;
 
   gdb::observers::thread_ptid_changed.attach (infrun_thread_ptid_changed);
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 0a2bfdc57d..65438743d1 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -2444,7 +2444,7 @@ static int
 check_stopped_by_watchpoint (struct lwp_info *lp)
 {
   scoped_restore save_inferior_ptid = make_scoped_restore (&inferior_ptid);
-  inferior_ptid = lp->ptid;
+  inferior_ptid.set (lp->ptid);
 
   if (linux_target->low_stopped_by_watchpoint ())
     {
diff --git a/gdb/proc-service.c b/gdb/proc-service.c
index e0383700a1..2d4749bd21 100644
--- a/gdb/proc-service.c
+++ b/gdb/proc-service.c
@@ -77,7 +77,7 @@ ps_xfer_memory (const struct ps_prochandle *ph, psaddr_t addr,
   set_current_program_space (ph->thread->inf->pspace);
 
   scoped_restore save_inferior_ptid = make_scoped_restore (&inferior_ptid);
-  inferior_ptid = ph->thread->ptid;
+  inferior_ptid.set (ph->thread->ptid);
 
   CORE_ADDR core_addr = ps_addr_to_core_addr (addr);
 
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 6a4359d0f3..756ea16c03 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -1637,8 +1637,9 @@ cooked_read_test (struct gdbarch *gdbarch)
   } pop_targets;
 
   /* Switch to the mock thread.  */
-  scoped_restore restore_inferior_ptid
-    = make_scoped_restore (&inferior_ptid, mock_ptid);
+  ptid_t save_ptid = inferior_ptid;
+  inferior_ptid.set (mock_ptid);
+  SCOPE_EXIT { inferior_ptid.set (save_ptid); };
 
   /* Test that read one raw register from regcache_no_target will go
      to the target layer.  */
diff --git a/gdb/remote.c b/gdb/remote.c
index fd89f2c084..8259831662 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5910,7 +5910,7 @@ extended_remote_target::attach (const char *args, int from_tty)
 
   switch_to_inferior_no_thread (remote_add_inferior (false, pid, 1, 0));
 
-  inferior_ptid = ptid_t (pid);
+  inferior_ptid.set (ptid_t (pid));
 
   if (target_is_non_stop_p ())
     {
diff --git a/gdb/thread.c b/gdb/thread.c
index f0722d3588..73d8f23b5d 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1285,7 +1285,7 @@ switch_to_thread_no_regs (struct thread_info *thread)
   set_current_inferior (inf);
 
   current_thread_ = thread;
-  inferior_ptid = current_thread_->ptid;
+  inferior_ptid.set (current_thread_->ptid);
 }
 
 /* See gdbthread.h.  */
@@ -1297,7 +1297,7 @@ switch_to_no_thread ()
     return;
 
   current_thread_ = nullptr;
-  inferior_ptid = null_ptid;
+  inferior_ptid.set (null_ptid);
   reinit_frame_cache ();
 }
 

base-commit: 2c074f49026acbe0597e0d2d2f7385195dcac565
prerequisite-patch-id: cf75804d57b7783ba470ada8e827d46c73c0d9e1
prerequisite-patch-id: 2f376c3e9de1bb5426723bc3081d27dc0221340b
prerequisite-patch-id: 7532dda900933a23405793d23231b7201b13c9e5
prerequisite-patch-id: b8e62093defdd58b9320d2d0cabef5797e962be8
prerequisite-patch-id: 060947c675effc4648dfc5f9929b084ee2b644a5
prerequisite-patch-id: e7c21d96301996d2e937aab3642fcda95c64c2c6
prerequisite-patch-id: 87412331bbd9689c4c93fc21fc70bc886230f57d
prerequisite-patch-id: 89d26483bf1c3300911e98f8a969fe97114dc671
prerequisite-patch-id: 59e116edd554952f722a8a741bf29d2080fdad3c
prerequisite-patch-id: 963bb9047825cf80900913b7c0ca4d975a00eb9d
prerequisite-patch-id: dbee563bfde550440a76eb4efa9441a84c19d366
prerequisite-patch-id: 1699f61662e53f0cb5d907d52d21480fdbab912c
prerequisite-patch-id: 7b0edf3f61a48e06ff1468be1234fa307e978629
prerequisite-patch-id: 259ed6608d652dc47d90539bce36c3002d3a75e4
prerequisite-patch-id: 6fb426c105967a6dcc0778c15a9881520537bb17
prerequisite-patch-id: c50eef1048b695c5d2305c38aaa510ccd30c671d
prerequisite-patch-id: 807aa4cdc04dac6c477d415115cf43283708fcb6
prerequisite-patch-id: 47b63abf5b4378c3d7c86774c302ccf29eda8182
prerequisite-patch-id: 00f4830b5d5dcea8d12ec42615ebf46df55909cf
prerequisite-patch-id: c06aebbe76e78f013baffeb2fcd3d6858b696b8a
prerequisite-patch-id: 84773b2ba6ae77d03da98678e2cf4ad966ad84fb
prerequisite-patch-id: 7a79ad19e2848b4b8ddc0acc7462955c327afe55
prerequisite-patch-id: b119df65172817eaccc0032178bb4173348d4ea4
prerequisite-patch-id: 0611a6654ffd0287a490debede469daea32c656b
prerequisite-patch-id: 729699cf50d1057fa11919434fcac6590a32e2eb
prerequisite-patch-id: 082610a38a64e718d229eb4484ef87dcba547bda
prerequisite-patch-id: 3e123af7bab734e29247f210a29a3d7c8946b533
prerequisite-patch-id: da39a3ee5e6b4b0d3255bfef95601890afd80709
-- 
2.14.5

Re: [PATCH 00/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[Pedro Alves]

On 4/14/20 6:54 PM, Pedro Alves via Gdb-patches wrote:
> In PR/25412, Simon noticed that after the multi-target series, the
> tid-reuse.exp testcase manages to create a duplicate thread in the
> thread list.  Or rather, two threads with the same PTID.
> 
> This in turn exposes a design problem in GDB.  The inferior_thread()
> function looks up the current thread based on inferior_ptid:
> 

...

> This series fixes this by making the current thread be a global
> thread_info pointer that is written to directly by switch_to_thread,
> etc., and making inferior_thread() return that pointer, instead of
> having inferior_thread() lookup up the inferior_ptid thread, by
> ptid_t.  You can look at this as a continuation of the effort of using
> more thread_info pointers instead of ptids when possible.

I've pushed this now.

Thanks,
Pedro Alves

Re: [PATCH 00/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[John Baldwin]

On 4/14/20 10:54 AM, Pedro Alves via Gdb-patches wrote:
> In PR/25412, Simon noticed that after the multi-target series, the
> tid-reuse.exp testcase manages to create a duplicate thread in the
> thread list.  Or rather, two threads with the same PTID.
> 
> This in turn exposes a design problem in GDB.  The inferior_thread()
> function looks up the current thread based on inferior_ptid:
> 
>  struct thread_info*
>  inferior_thread (void)
>  {
>    struct thread_info *tp = find_thread_ptid (current_inferior (), inferior_ptid);
>    gdb_assert (tp);
>    return tp;
>  }
> 
> But if there can be more than one thread in the thread list with the
> same ptid_t, inferior_thread() may well return the wrong thread.
> 
> This series fixes this by making the current thread be a global
> thread_info pointer that is written to directly by switch_to_thread,
> etc., and making inferior_thread() return that pointer, instead of
> having inferior_thread() lookup up the inferior_ptid thread, by
> ptid_t.  You can look at this as a continuation of the effort of using
> more thread_info pointers instead of ptids when possible.
> 
> This change required auditing the whole codebase for places where we
> were writing to inferior_ptid directly to change the current thread,
> and change them to use switch_to_thread instead or one of its
> siblings, because otherwise inferior_thread() and inferior_ptid would
> get out of sync and inferior_thread() would return a thread unrelated
> to the new inferior_ptid we wanted to switch to.  That was all
> (hopefully) done in all the patches leading up to the last one.
> 
> After this, inferior_ptid still exists, but it is mostly read-only and
> mainly used by target backend code.  It is also relied on by a number
> of target methods as a global input argument.  E.g., the target_resume
> interface and the memory reading routines -- we still need it there
> because we need to be able to access memory off of processes for which
> we don't have a corresponding inferior/thread object, like when
> handling forks.  Maybe we could pass down a context explicitly to
> target_read_memory, etc.
> 
> Most of the host-/native-specific code here is untested.  I did my
> best, but I won't be surprised if more tweaking is necessary.
> 
> Testing on native x86_64 GNU/Linux is regression free for me.  Testing
> against gdbserver has regressed significantly in the past months and
> is becoming difficult to run with a high number of long timeout
> sequences; really looks like people aren't paying much attention to
> that.  I think this series doesn't regress gdbserver, but it's getting
> hard to tell.  :-/

This appears to have broken native debugging on FreeBSD for me in that
when I run a process to completion it triggers an assertion failure:

(gdb) r
Starting program: /bin/echo 

Child process unexpectedly missing: No child processes.
/home/john/work/git/gdb/gdb/inferior.c:293: internal-error: struct inferior *find_inferior_pid(process_stratum_target *, int): Assertion `pid != 0' failed.

I tracked this down to this code in inf_ptrace::wait():

      /* Ignore terminated detached child processes.  */
      if (!WIFSTOPPED (status) && pid != inferior_ptid.pid ())
        pid = -1;

At this point, inferior_ptid() is all zeroes and the process
has reported a non-stopped exit status (WIFEXITED) so this
ignores the exit event and loops back around to call waitpid()
again which then fails with ECHILD.

Looks like we always clear the inferior thread (and thus
inferior_ptid) in do_target_wait_1:

  /* We know that we are looking for an event in the target of inferior
     INF, but we don't know which thread the event might come from.  As
     such we want to make sure that INFERIOR_PTID is reset so that none of
     the wait code relies on it - doing so is always a mistake.  */
  switch_to_inferior_no_thread (inf);

Commenting out the check in inf_ptrace::wait() "fixes" the issue for
me on FreeBSD, but I'm not sure that is the right fix.

It seems to me that multiprocess probably needs to return events for
not just the current inferior pid but for any valid pid for example,
and though multiprocess is still broken for me on FreeBSD if I comment
out the check against inferior_ptid, I can now see that I was getting
an event for the "wrong" inferior that was previously discarded but
with the check commented out now gets reported to the core.

(The reason I get an event for the wrong process is that for some
reason the core asks the native target to resume the wrong process:

> ./gdb /bin/ls
(gdb) set debug fbsd-nat 
(gdb) set debug fbsd-lwp 
(gdb) start
Temporary breakpoint 1 at 0x20430a: file /usr/src/bin/ls/ls.c, line 236.
Starting program: /bin/ls 
FNAT: stop for LWP 101518 event 1 flags 0x18
FLWP: using LWP 101518 for first thread
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101518 event 1 flags 0x20
FNAT: si_signo 20 si_code 1
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101518 event 1 flags 0x20
FNAT: si_signo 20 si_code 1
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101518 event 1 flags 0x20
FNAT: si_signo 20 si_code 1
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101518 event 1 flags 0x20
FNAT: si_signo 20 si_code 1
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101518 event 1 flags 0x20
FNAT: si_signo 20 si_code 1
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101518 event 1 flags 0x18
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101518 event 1 flags 0x20
FNAT: si_signo 5 si_code 1
FNAT: sw breakpoint trap for LWP 101518
FLWP: fbsd_resume for ptid (70453, 101518, 0)
FNAT: stop for LWP 101518 event 1 flags 0x20
FNAT: si_signo 5 si_code 2
FNAT: trace trap for LWP 101518
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101518 event 1 flags 0x20
FNAT: si_signo 5 si_code 1
FNAT: sw breakpoint trap for LWP 101518

Temporary breakpoint 1, main (argc=1, argv=0x7fffffffe710)
    at /usr/src/bin/ls/ls.c:236
236		const char *errstr = NULL;
(gdb) add-inferior 
[New inferior 2]
Added inferior 2 on connection 1 (native)
(gdb) inferior 2
[Switching to inferior 2 [<null>] (<noexec>)]
(gdb) file /bin/ls
Reading symbols from /bin/ls...
Reading symbols from /usr/lib/debug//bin/ls.debug...
(gdb) start
Temporary breakpoint 2 at 0x20430a: -qualified main. (2 locations)
Starting program: /bin/ls 
FNAT: stop for LWP 101641 event 1 flags 0x18
FLWP: using LWP 101641 for first thread
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101641 event 1 flags 0x20
FNAT: si_signo 20 si_code 1
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101641 event 1 flags 0x20
FNAT: si_signo 20 si_code 1
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101641 event 1 flags 0x20
FNAT: si_signo 20 si_code 1
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101641 event 1 flags 0x24
FNAT: si_signo 20 si_code 1
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101641 event 1 flags 0x24
FNAT: si_signo 20 si_code 1
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101641 event 1 flags 0x20
FNAT: si_signo 20 si_code 1
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101641 event 1 flags 0x18
FLWP: fbsd_resume for ptid (70453, 101518, 0)

Program terminated with signal SIGTRAP, Trace/breakpoint trap.
The program no longer exists.

Here you can see that the last call to fbsd_resume() used the ptid from
inferior 1 instead of inferior 2, and inferior 1 didn't discard it's
pending SIGTRAP but instead was killed by it.)

-- 
John Baldwin

Re: [PATCH 00/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[Pedro Alves]

On 7/8/20 12:16 AM, John Baldwin wrote:
> This appears to have broken native debugging on FreeBSD for me in that
> when I run a process to completion it triggers an assertion failure:
> 
> (gdb) r
> Starting program: /bin/echo 
> 
> Child process unexpectedly missing: No child processes.
> /home/john/work/git/gdb/gdb/inferior.c:293: internal-error: struct inferior *find_inferior_pid(process_stratum_target *, int): Assertion `pid != 0' failed.
> 
> I tracked this down to this code in inf_ptrace::wait():
> 
>       /* Ignore terminated detached child processes.  */
>       if (!WIFSTOPPED (status) && pid != inferior_ptid.pid ())
>         pid = -1;
> 
> At this point, inferior_ptid() is all zeroes and the process
> has reported a non-stopped exit status (WIFEXITED) so this
> ignores the exit event and loops back around to call waitpid()
> again which then fails with ECHILD.
> 
> Looks like we always clear the inferior thread (and thus
> inferior_ptid) in do_target_wait_1:
> 
>   /* We know that we are looking for an event in the target of inferior
>      INF, but we don't know which thread the event might come from.  As
>      such we want to make sure that INFERIOR_PTID is reset so that none of
>      the wait code relies on it - doing so is always a mistake.  */
>   switch_to_inferior_no_thread (inf);
> 
> Commenting out the check in inf_ptrace::wait() "fixes" the issue for
> me on FreeBSD, but I'm not sure that is the right fix.

linux-nat.c has something logic, here in linux_nat_filter_event:

  /* Make sure we don't report an event for the exit of an LWP not in
     our list, i.e. not part of the current process.  This can happen
     if we detach from a program we originally forked and then it
     exits.  */
  if (!WIFSTOPPED (status) && !lp)
    return NULL;

For inf-ptrace.c, the right fix should be something around this:

diff --git c/gdb/inf-ptrace.c w/gdb/inf-ptrace.c
index d25d226abb..ae0b0f7ff0 100644
--- c/gdb/inf-ptrace.c
+++ w/gdb/inf-ptrace.c
@@ -347,7 +347,7 @@ inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
        }
 
       /* Ignore terminated detached child processes.  */
-      if (!WIFSTOPPED (status) && pid != inferior_ptid.pid ())
+      if (!WIFSTOPPED (status) && find_inferior_pid (this, pid) == nullptr)
        pid = -1;
     }
   while (pid == -1);


The inferior_ptid reference in the error path above that code, where it
reads:

  "Claim it exited with unknown signal"

is also wrong.  I'm not sure what to do about that one.
Not clear if that path can really happen.  linux-nat.c calls
perror_with_name in some places if waitpid returns -1,
and in the main waitpid call, just assumes that error -1 never
happens...

> 
> It seems to me that multiprocess probably needs to return events for
> not just the current inferior pid but for any valid pid for example,

Yes.

> and though multiprocess is still broken for me on FreeBSD if I comment
> out the check against inferior_ptid, I can now see that I was getting
> an event for the "wrong" inferior that was previously discarded but
> with the check commented out now gets reported to the core.
> 
> (The reason I get an event for the wrong process is that for some
> reason the core asks the native target to resume the wrong process:
> 
>> ./gdb /bin/ls
> (gdb) set debug fbsd-nat 
> (gdb) set debug fbsd-lwp 
> (gdb) start
> Temporary breakpoint 1 at 0x20430a: file /usr/src/bin/ls/ls.c, line 236.
> Starting program: /bin/ls 
> FNAT: stop for LWP 101518 event 1 flags 0x18
> FLWP: using LWP 101518 for first thread
> FLWP: fbsd_resume for ptid (-1, 0, 0)
> FNAT: stop for LWP 101518 event 1 flags 0x20
> FNAT: si_signo 20 si_code 1
> FLWP: fbsd_resume for ptid (-1, 0, 0)
> FNAT: stop for LWP 101518 event 1 flags 0x20
> FNAT: si_signo 20 si_code 1
> FLWP: fbsd_resume for ptid (-1, 0, 0)
> FNAT: stop for LWP 101518 event 1 flags 0x20
> FNAT: si_signo 20 si_code 1
> FLWP: fbsd_resume for ptid (-1, 0, 0)
> FNAT: stop for LWP 101518 event 1 flags 0x20
> FNAT: si_signo 20 si_code 1
> FLWP: fbsd_resume for ptid (-1, 0, 0)
> FNAT: stop for LWP 101518 event 1 flags 0x20
> FNAT: si_signo 20 si_code 1
> FLWP: fbsd_resume for ptid (-1, 0, 0)
> FNAT: stop for LWP 101518 event 1 flags 0x18
> FLWP: fbsd_resume for ptid (-1, 0, 0)
> FNAT: stop for LWP 101518 event 1 flags 0x20
> FNAT: si_signo 5 si_code 1
> FNAT: sw breakpoint trap for LWP 101518
> FLWP: fbsd_resume for ptid (70453, 101518, 0)
> FNAT: stop for LWP 101518 event 1 flags 0x20
> FNAT: si_signo 5 si_code 2
> FNAT: trace trap for LWP 101518
> FLWP: fbsd_resume for ptid (-1, 0, 0)
> FNAT: stop for LWP 101518 event 1 flags 0x20
> FNAT: si_signo 5 si_code 1
> FNAT: sw breakpoint trap for LWP 101518
> 
> Temporary breakpoint 1, main (argc=1, argv=0x7fffffffe710)
>     at /usr/src/bin/ls/ls.c:236
> 236		const char *errstr = NULL;
> (gdb) add-inferior 
> [New inferior 2]
> Added inferior 2 on connection 1 (native)
> (gdb) inferior 2
> [Switching to inferior 2 [<null>] (<noexec>)]
> (gdb) file /bin/ls
> Reading symbols from /bin/ls...
> Reading symbols from /usr/lib/debug//bin/ls.debug...
> (gdb) start
> Temporary breakpoint 2 at 0x20430a: -qualified main. (2 locations)
> Starting program: /bin/ls 
> FNAT: stop for LWP 101641 event 1 flags 0x18
> FLWP: using LWP 101641 for first thread
> FLWP: fbsd_resume for ptid (-1, 0, 0)
> FNAT: stop for LWP 101641 event 1 flags 0x20
> FNAT: si_signo 20 si_code 1
> FLWP: fbsd_resume for ptid (-1, 0, 0)
> FNAT: stop for LWP 101641 event 1 flags 0x20
> FNAT: si_signo 20 si_code 1
> FLWP: fbsd_resume for ptid (-1, 0, 0)
> FNAT: stop for LWP 101641 event 1 flags 0x20
> FNAT: si_signo 20 si_code 1
> FLWP: fbsd_resume for ptid (-1, 0, 0)
> FNAT: stop for LWP 101641 event 1 flags 0x24
> FNAT: si_signo 20 si_code 1
> FLWP: fbsd_resume for ptid (-1, 0, 0)
> FNAT: stop for LWP 101641 event 1 flags 0x24
> FNAT: si_signo 20 si_code 1
> FLWP: fbsd_resume for ptid (-1, 0, 0)
> FNAT: stop for LWP 101641 event 1 flags 0x20
> FNAT: si_signo 20 si_code 1
> FLWP: fbsd_resume for ptid (-1, 0, 0)
> FNAT: stop for LWP 101641 event 1 flags 0x18
> FLWP: fbsd_resume for ptid (70453, 101518, 0)
> 
> Program terminated with signal SIGTRAP, Trace/breakpoint trap.
> The program no longer exists.
> 
> Here you can see that the last call to fbsd_resume() used the ptid from
> inferior 1 instead of inferior 2, and inferior 1 didn't discard it's
> pending SIGTRAP but instead was killed by it.)
"set debug infrun 1" will probably reveal what is happening.

Thanks,
Pedro Alves

Re: [PATCH 00/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[John Baldwin]

On 7/7/20 4:53 PM, Pedro Alves wrote:
> On 7/8/20 12:16 AM, John Baldwin wrote:
>> This appears to have broken native debugging on FreeBSD for me in that
>> when I run a process to completion it triggers an assertion failure:
>>
>> (gdb) r
>> Starting program: /bin/echo 
>>
>> Child process unexpectedly missing: No child processes.
>> /home/john/work/git/gdb/gdb/inferior.c:293: internal-error: struct inferior *find_inferior_pid(process_stratum_target *, int): Assertion `pid != 0' failed.
>>
>> I tracked this down to this code in inf_ptrace::wait():
>>
>>       /* Ignore terminated detached child processes.  */
>>       if (!WIFSTOPPED (status) && pid != inferior_ptid.pid ())
>>         pid = -1;
>>
>> At this point, inferior_ptid() is all zeroes and the process
>> has reported a non-stopped exit status (WIFEXITED) so this
>> ignores the exit event and loops back around to call waitpid()
>> again which then fails with ECHILD.
>>
>> Looks like we always clear the inferior thread (and thus
>> inferior_ptid) in do_target_wait_1:
>>
>>   /* We know that we are looking for an event in the target of inferior
>>      INF, but we don't know which thread the event might come from.  As
>>      such we want to make sure that INFERIOR_PTID is reset so that none of
>>      the wait code relies on it - doing so is always a mistake.  */
>>   switch_to_inferior_no_thread (inf);
>>
>> Commenting out the check in inf_ptrace::wait() "fixes" the issue for
>> me on FreeBSD, but I'm not sure that is the right fix.
> 
> linux-nat.c has something logic, here in linux_nat_filter_event:
> 
>   /* Make sure we don't report an event for the exit of an LWP not in
>      our list, i.e. not part of the current process.  This can happen
>      if we detach from a program we originally forked and then it
>      exits.  */
>   if (!WIFSTOPPED (status) && !lp)
>     return NULL;
> 
> For inf-ptrace.c, the right fix should be something around this:
> 
> diff --git c/gdb/inf-ptrace.c w/gdb/inf-ptrace.c
> index d25d226abb..ae0b0f7ff0 100644
> --- c/gdb/inf-ptrace.c
> +++ w/gdb/inf-ptrace.c
> @@ -347,7 +347,7 @@ inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>         }
>  
>        /* Ignore terminated detached child processes.  */
> -      if (!WIFSTOPPED (status) && pid != inferior_ptid.pid ())
> +      if (!WIFSTOPPED (status) && find_inferior_pid (this, pid) == nullptr)
>         pid = -1;
>      }
>    while (pid == -1);

Yes, this works.  I hadn't found that function and was thinking of
writing it from scratch, but this is better of course.
 
> The inferior_ptid reference in the error path above that code, where it
> reads:
> 
>   "Claim it exited with unknown signal"
> 
> is also wrong.  I'm not sure what to do about that one.
> Not clear if that path can really happen.  linux-nat.c calls
> perror_with_name in some places if waitpid returns -1,
> and in the main waitpid call, just assumes that error -1 never
> happens...

It happened for me in this case since wait failed with -1 when we discarded
the exit info incorrectly.  I can change it to return null_ptid which would
match what it does now, or perhaps we could just add an assertion here rather
than relying on the  null_ptid tripping over another assertion later?

>> Here you can see that the last call to fbsd_resume() used the ptid from
>> inferior 1 instead of inferior 2, and inferior 1 didn't discard it's
>> pending SIGTRAP but instead was killed by it.)
> "set debug infrun 1" will probably reveal what is happening.

Yes, I have some more details on this in my other followup where infrun
debugging was very helpful, though I realize I failed to include any of
the infrun debug output.

-- 
John Baldwin

Multiprocess on FreeBSD

[John Baldwin]

On 7/7/20 4:16 PM, John Baldwin wrote:
> FNAT: stop for LWP 101641 event 1 flags 0x18
> FLWP: fbsd_resume for ptid (70453, 101518, 0)
> 
> Program terminated with signal SIGTRAP, Trace/breakpoint trap.
> The program no longer exists.
> 
> Here you can see that the last call to fbsd_resume() used the ptid from
> inferior 1 instead of inferior 2, and inferior 1 didn't discard it's
> pending SIGTRAP but instead was killed by it.)

On this part, I turned on 'set debug infrun 1' and can see that
the core is trying to step over the breakpoint in the first
inferior.  However, it is not clearing the SIGTRAP signal to
GDB_SIGNAL_0 when resuming the first inferior but leaving it as
GDB_SIGNAL_TRAP.  This appears to be because start_step_over()
calls keep_going_pass_signal() instead of keep_going(), so
tp->control.stop_signal doesn't get reset.  I'm not sure if
stop_signal should have been cleared earlier somehow?

Hmm, if I add a hack to ignore SIGTRAP in fbsd_nat_target::resume(),
then what happens is that the first inferior runs to completion.
I can run the second inferior after switching to it, but I think the
behavior I probably want is for the core to not resume threads from
the non-current inferior instead?

One last test I tried is to do this sequence instead in the first
inferior:

(gdb) file /bin/ls
(gdb) start
(gdb) n

This gets the first inferior off of the breakpoint and the thread is
stopped for a hardware stepping trap instead.

I can then add an inferior do start and have it work fine, and I
can switch back and forth between the two inferiors stepping each
one.

So I think the issue here is that for some reason when the 'start'
command for inferior 2 tries to resume the inferior after getting
across the initial events for exec, it ends up resuming the wrong
inferior due to confusion about the thread stopped at a breakpoint.
However, it doesn't get confused if the thread is stopped for some
other reason.

-- 
John Baldwin

Re: Multiprocess on FreeBSD

[John Baldwin]

On 7/7/20 5:10 PM, John Baldwin wrote:
> On 7/7/20 4:16 PM, John Baldwin wrote:
>> FNAT: stop for LWP 101641 event 1 flags 0x18
>> FLWP: fbsd_resume for ptid (70453, 101518, 0)
>>
>> Program terminated with signal SIGTRAP, Trace/breakpoint trap.
>> The program no longer exists.
>>
>> Here you can see that the last call to fbsd_resume() used the ptid from
>> inferior 1 instead of inferior 2, and inferior 1 didn't discard it's
>> pending SIGTRAP but instead was killed by it.)
> 
> On this part, I turned on 'set debug infrun 1' and can see that
> the core is trying to step over the breakpoint in the first
> inferior.  However, it is not clearing the SIGTRAP signal to
> GDB_SIGNAL_0 when resuming the first inferior but leaving it as
> GDB_SIGNAL_TRAP.  This appears to be because start_step_over()
> calls keep_going_pass_signal() instead of keep_going(), so
> tp->control.stop_signal doesn't get reset.  I'm not sure if
> stop_signal should have been cleared earlier somehow?
> 
> Hmm, if I add a hack to ignore SIGTRAP in fbsd_nat_target::resume(),
> then what happens is that the first inferior runs to completion.
> I can run the second inferior after switching to it, but I think the
> behavior I probably want is for the core to not resume threads from
> the non-current inferior instead?
> 
> One last test I tried is to do this sequence instead in the first
> inferior:
> 
> (gdb) file /bin/ls
> (gdb) start
> (gdb) n
> 
> This gets the first inferior off of the breakpoint and the thread is
> stopped for a hardware stepping trap instead.
> 
> I can then add an inferior do start and have it work fine, and I
> can switch back and forth between the two inferiors stepping each
> one.
> 
> So I think the issue here is that for some reason when the 'start'
> command for inferior 2 tries to resume the inferior after getting
> across the initial events for exec, it ends up resuming the wrong
> inferior due to confusion about the thread stopped at a breakpoint.
> However, it doesn't get confused if the thread is stopped for some
> other reason.

I had to add a 'fbsd_nat_target::supports_multi_process' method and
now it seems to work fine.

It does depend on the fix to use find_inferior_pid() to work.

-- 
John Baldwin