Re: [PATCH v2 00/24] Multi-target support

[Pedro Alves <palves@redhat.com>]

On 10/18/19 9:22 PM, John Baldwin wrote:
> On 10/17/19 3:50 PM, Pedro Alves wrote:
>> Here's v2 of the multi-target patchset, which addresses all the review
>> comments so far, I believe.  Patch 15 is new, so all following patches
>> are shifted by one.
>>
>> This time, I've adjusted the host-specific nat files to function API
>> changes.  I tried to find spots that would need changes using grep.  I
>> built the series on AIX, x86/SPARC Solaris, 64-bit Windows, x86
>> GNU/Linux -m64/-m32, and Aarch64 GNU/Linux.  I'm currently running
>> this through the buildbot, will have results tomorrow.  I don't expect
>> any serious major issue, if any, as so far runs that completed seem
>> OK.
> 
> I (finally) have a patch to fix the build on FreeBSD/amd64 (and probably
> FreeBSD on other platforms) here:
> 
> https://github.com/bsdjhb/gdb/commit/e58a36eaef6244d2040ce6f377497ee898978db4
> 

Thanks!

> It's a combined patch but has some commentary on bsd-kvm.c which is
> kind of special.  That target adds new commands that need to find a
> target to operate on.  I opted to have it look at the current inferior
> and if (using a dynamic cast) it is a bsd-kvm target the commands
> modify the state of that inferior.  I haven't tested it though as I don't
> use bsd-kvm.c.

I'd rather not merge the bsd-kvm.c parts into my patches as is, for the
reason that it doesn't appear necessary for a minimal keep-working-as-before
change.  I think it would be fine as a follow up patch, though.

A couple comments:

 - I think we should get away from doing this

      inferior_ptid = null_ptid;
      exit_inferior_silent (current_inferior ());

   in 'bsd_kvm_target::close ()'.

 - Class private fields should be named 'm_foo', so m_kd, m_corefile, etc.

For bsd-kvm.c, I _think_ that the only change necessary to keep things
building would be this:

--- a/gdb/bsd-kvm.c
+++ b/gdb/bsd-kvm.c
@@ -136,7 +136,7 @@ bsd_kvm_target_open (const char *arg, int from_tty)
   core_kd = temp_kd;
   push_target (&bsd_kvm_ops);
 
-  add_thread_silent (bsd_kvm_ptid);
+  add_thread_silent (&bsd_kvm_ops, bsd_kvm_ptid);
   inferior_ptid = bsd_kvm_ptid;

Right?  See updated patch below.

> I did try a simple test of creating two inferiors both of which were
> running /bin/ls and gdb hung trying to run the second inferior via
> 'start'.  I suspect this is some kind of bug in the FreeBSD target not
> being multi-target ready that I will have to debug.

Does that work without the multi-target series?  I mean, maybe it's more
a multi-process debugging issue than a multi-target issue?

From 3f879fe9334cbd69f50efd0131c2bed5498cde92 Mon Sep 17 00:00:00 2001
From: John Baldwin <jhb@FreeBSD.org>
Date: Fri, 18 Oct 2019 13:12:11 -0700
Subject: [PATCH] Update FreeBSD native build for multi-target-v2.

---
 gdb/bsd-kvm.c  |  2 +-
 gdb/fbsd-nat.c | 33 +++++++++++++++++----------------
 2 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/gdb/bsd-kvm.c b/gdb/bsd-kvm.c
index 21f978728d..5ffc303f46 100644
--- a/gdb/bsd-kvm.c
+++ b/gdb/bsd-kvm.c
@@ -136,7 +136,7 @@ bsd_kvm_target_open (const char *arg, int from_tty)
   core_kd = temp_kd;
   push_target (&bsd_kvm_ops);
 
-  add_thread_silent (bsd_kvm_ptid);
+  add_thread_silent (&bsd_kvm_ops, bsd_kvm_ptid);
   inferior_ptid = bsd_kvm_ptid;
 
   target_fetch_registers (get_current_regcache (), -1);
diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 0274ff542e..bb34a9769d 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -991,11 +991,11 @@ fbsd_enable_proc_events (pid_t pid)
    called to discover new threads each time the thread list is updated.  */
 
 static void
-fbsd_add_threads (pid_t pid)
+fbsd_add_threads (fbsd_nat_target *target, pid_t pid)
 {
   int i, nlwps;
 
-  gdb_assert (!in_thread_list (ptid_t (pid)));
+  gdb_assert (!in_thread_list (target, ptid_t (pid)));
   nlwps = ptrace (PT_GETNUMLWPS, pid, NULL, 0);
   if (nlwps == -1)
     perror_with_name (("ptrace"));
@@ -1010,7 +1010,7 @@ fbsd_add_threads (pid_t pid)
     {
       ptid_t ptid = ptid_t (pid, lwps[i], 0);
 
-      if (!in_thread_list (ptid))
+      if (!in_thread_list (target, ptid))
 	{
 #ifdef PT_LWP_EVENTS
 	  struct ptrace_lwpinfo pl;
@@ -1026,7 +1026,7 @@ fbsd_add_threads (pid_t pid)
 	    fprintf_unfiltered (gdb_stdlog,
 				"FLWP: adding thread for LWP %u\n",
 				lwps[i]);
-	  add_thread (ptid);
+	  add_thread (target, ptid);
 	}
     }
 }
@@ -1043,7 +1043,7 @@ fbsd_nat_target::update_thread_list ()
 #else
   prune_threads ();
 
-  fbsd_add_threads (inferior_ptid.pid ());
+  fbsd_add_threads (this, inferior_ptid.pid ());
 #endif
 }
 
@@ -1174,7 +1174,7 @@ fbsd_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo)
   if (ptid.lwp_p ())
     {
       /* If ptid is a specific LWP, suspend all other LWPs in the process.  */
-      inferior *inf = find_inferior_ptid (ptid);
+      inferior *inf = find_inferior_ptid (this, ptid);
 
       for (thread_info *tp : inf->non_exited_threads ())
         {
@@ -1193,7 +1193,7 @@ fbsd_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo)
     {
       /* If ptid is a wildcard, resume all matching threads (they won't run
 	 until the process is continued however).  */
-      for (thread_info *tp : all_non_exited_threads (ptid))
+      for (thread_info *tp : all_non_exited_threads (this, ptid))
 	if (ptrace (PT_RESUME, tp->ptid.lwp (), NULL, 0) == -1)
 	  perror_with_name (("ptrace"));
       ptid = inferior_ptid;
@@ -1239,7 +1239,8 @@ fbsd_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo)
    core, return true.  */
 
 static bool
-fbsd_handle_debug_trap (ptid_t ptid, const struct ptrace_lwpinfo &pl)
+fbsd_handle_debug_trap (fbsd_nat_target *target, ptid_t ptid,
+			const struct ptrace_lwpinfo &pl)
 {
 
   /* Ignore traps without valid siginfo or for signals other than
@@ -1266,7 +1267,7 @@ fbsd_handle_debug_trap (ptid_t ptid, const struct ptrace_lwpinfo &pl)
   if (pl.pl_siginfo.si_code == TRAP_BRKPT)
     {
       /* Fixup PC for the software breakpoint.  */
-      struct regcache *regcache = get_thread_regcache (ptid);
+      struct regcache *regcache = get_thread_regcache (target, ptid);
       struct gdbarch *gdbarch = regcache->arch ();
       int decr_pc = gdbarch_decr_pc_after_break (gdbarch);
 
@@ -1340,7 +1341,7 @@ fbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 		 threads might be skipped during post_attach that
 		 have not yet reported their PL_FLAG_EXITED event.
 		 Ignore EXITED events for an unknown LWP.  */
-	      thread_info *thr = find_thread_ptid (wptid);
+	      thread_info *thr = find_thread_ptid (this, wptid);
 	      if (thr != nullptr)
 		{
 		  if (debug_fbsd_lwp)
@@ -1364,13 +1365,13 @@ fbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 	     PL_FLAG_BORN in case the first stop reported after
 	     attaching to an existing process is a PL_FLAG_BORN
 	     event.  */
-	  if (in_thread_list (ptid_t (pid)))
+	  if (in_thread_list (this, ptid_t (pid)))
 	    {
 	      if (debug_fbsd_lwp)
 		fprintf_unfiltered (gdb_stdlog,
 				    "FLWP: using LWP %u for first thread\n",
 				    pl.pl_lwpid);
-	      thread_change_ptid (ptid_t (pid), wptid);
+	      thread_change_ptid (this, ptid_t (pid), wptid);
 	    }
 
 #ifdef PT_LWP_EVENTS
@@ -1380,13 +1381,13 @@ fbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 		 threads might be added by fbsd_add_threads that have
 		 not yet reported their PL_FLAG_BORN event.  Ignore
 		 BORN events for an already-known LWP.  */
-	      if (!in_thread_list (wptid))
+	      if (!in_thread_list (this, wptid))
 		{
 		  if (debug_fbsd_lwp)
 		    fprintf_unfiltered (gdb_stdlog,
 					"FLWP: adding thread for LWP %u\n",
 					pl.pl_lwpid);
-		  add_thread (wptid);
+		  add_thread (this, wptid);
 		}
 	      ourstatus->kind = TARGET_WAITKIND_SPURIOUS;
 	      return wptid;
@@ -1474,7 +1475,7 @@ fbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 #endif
 
 #ifdef USE_SIGTRAP_SIGINFO
-	  if (fbsd_handle_debug_trap (wptid, pl))
+	  if (fbsd_handle_debug_trap (this, wptid, pl))
 	    return wptid;
 #endif
 
@@ -1633,7 +1634,7 @@ void
 fbsd_nat_target::post_attach (int pid)
 {
   fbsd_enable_proc_events (pid);
-  fbsd_add_threads (pid);
+  fbsd_add_threads (this, pid);
 }
 
 #ifdef PL_FLAG_EXEC
-- 
2.14.5