[PATCH v2 0/5] A few fixes to OpenBSD's native target

[PATCH v2 0/5] A few fixes to OpenBSD's native target

[John Baldwin]

This addresses some feedback from Simon but also adds two additional
patches that allow some simple fork following to mostly work for me on
OpenBSD/amd64 6.9.

The one remaining issue I see is that if I disable detach on fork and
allow a child process to run to completion then go back and continue
the parent process, the parent process is killed with a SIGHUP.  I
think this is probably an issue in OpenBSD's kernel though rather than
GDB and I don't plan on investigating that further.

John Baldwin (5):
  Don't compile x86 debug register support on OpenBSD.
  x86-bsd-nat: Only define gdb_ptrace when using debug registers.
  obsd-nat: Various fixes to obsd_nat_target::wait.
  obsd-nat: Various fixes for fork following.
  obsd-nat: Report both thread and PID in ::pid_to_str.

 gdb/configure.nat |   5 +--
 gdb/obsd-nat.c    | 112 +++++++++++++++-------------------------------
 gdb/obsd-nat.h    |   2 -
 gdb/x86-bsd-nat.c |  16 +++----
 gdb/x86-bsd-nat.h |   9 +++-
 5 files changed, 53 insertions(+), 91 deletions(-)

-- 
2.31.1

[PATCH v2 1/5] Don't compile x86 debug register support on OpenBSD.

[John Baldwin]

Simon Marchi tried gdb on OpenBSD, and it immediately segfaults when
running a program.  Simon tracked down the problem to x86_dr_low.get_status
being nullptr at this point:

    (lldb) print x86_dr_low.get_status
    (unsigned long (*)()) $0 = 0x0000000000000000
    (lldb) bt
    * thread #1, stop reason = step over
      * frame #0: 0x0000033b64b764aa gdb`x86_dr_stopped_data_address(state=0x0000033d7162a310, addr_p=0x00007f7ffffc5688) at x86-dregs.c:645:12
        frame #1: 0x0000033b64b766de gdb`x86_dr_stopped_by_watchpoint(state=0x0000033d7162a310) at x86-dregs.c:687:10
        frame #2: 0x0000033b64ea5f72 gdb`x86_stopped_by_watchpoint() at x86-nat.c:206:10
        frame #3: 0x0000033b64637fbb gdb`x86_nat_target<obsd_nat_target>::stopped_by_watchpoint(this=0x0000033b65252820) at x86-nat.h:100:12
        frame #4: 0x0000033b64d3ff11 gdb`target_stopped_by_watchpoint() at target.c:468:46
        frame #5: 0x0000033b6469b001 gdb`watchpoints_triggered(ws=0x00007f7ffffc61c8) at breakpoint.c:4790:32
        frame #6: 0x0000033b64a8bb8b gdb`handle_signal_stop(ecs=0x00007f7ffffc61a0) at infrun.c:6072:29
        frame #7: 0x0000033b64a7e3a7 gdb`handle_inferior_event(ecs=0x00007f7ffffc61a0) at infrun.c:5694:7
        frame #8: 0x0000033b64a7c1a0 gdb`fetch_inferior_event() at infrun.c:4090:5
        frame #9: 0x0000033b64a51921 gdb`inferior_event_handler(event_type=INF_REG_EVENT) at inf-loop.c:41:7
        frame #10: 0x0000033b64a827c9 gdb`infrun_async_inferior_event_handler(data=0x0000000000000000) at infrun.c:9384:3
        frame #11: 0x0000033b6465bd4f gdb`check_async_event_handlers() at async-event.c:335:4
        frame #12: 0x0000033b65070917 gdb`gdb_do_one_event() at event-loop.cc:216:10
        frame #13: 0x0000033b64af0db1 gdb`start_event_loop() at main.c:421:13
        frame #14: 0x0000033b64aefe9a gdb`captured_command_loop() at main.c:481:3
        frame #15: 0x0000033b64aed5c2 gdb`captured_main(data=0x00007f7ffffc6470) at main.c:1353:4
        frame #16: 0x0000033b64aed4f2 gdb`gdb_main(args=0x00007f7ffffc6470) at main.c:1368:7
        frame #17: 0x0000033b6459d787 gdb`main(argc=5, argv=0x00007f7ffffc6518) at gdb.c:32:10
        frame #18: 0x0000033b6459d521 gdb`___start + 321

On BSDs, get_status is set in _initialize_x86_bsd_nat, but only if
HAVE_PT_GETDBREGS is defined.  PT_GETDBREGS doesn't exist on OpenBSD, so
get_status (and the other fields of x86_dr_low) are left as nullptr.

OpenBSD doesn't support getting or setting the x86 debug registers, so
fix by omitting debug register support entirely on OpenBSD:

- Change x86bsd_nat_target to only inherit from x86_nat_target if
  PT_GETDBREGS is supported.

- Don't include x86-nat.o and nat/x86-dregs.o for OpenBSD/amd64.  They
  were already omitted for OpenBSD/i386.
---
 gdb/configure.nat | 5 ++---
 gdb/x86-bsd-nat.h | 9 +++++++--
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/gdb/configure.nat b/gdb/configure.nat
index e34cccffd98..655c75dd1ab 100644
--- a/gdb/configure.nat
+++ b/gdb/configure.nat
@@ -451,9 +451,8 @@ case ${gdb_host} in
 	case ${gdb_host_cpu} in
 	    i386)
 		# Host: OpenBSD/amd64
-		NATDEPFILES="${NATDEPFILES} obsd-nat.o amd64-nat.o x86-nat.o \
-		x86-bsd-nat.o amd64-bsd-nat.o amd64-obsd-nat.o bsd-kvm.o \
-		nat/x86-dregs.o"
+		NATDEPFILES="${NATDEPFILES} obsd-nat.o amd64-nat.o \
+		x86-bsd-nat.o amd64-bsd-nat.o amd64-obsd-nat.o bsd-kvm.o"
 		LOADLIBES='-lkvm'
 		;;
 	    mips)
diff --git a/gdb/x86-bsd-nat.h b/gdb/x86-bsd-nat.h
index 02d61c20b0b..caf62e38df6 100644
--- a/gdb/x86-bsd-nat.h
+++ b/gdb/x86-bsd-nat.h
@@ -27,18 +27,23 @@ extern size_t x86bsd_xsave_len;
 
 /* A prototype *BSD/x86 target.  */
 
+#ifdef HAVE_PT_GETDBREGS
 template<typename BaseTarget>
 class x86bsd_nat_target : public x86_nat_target<BaseTarget>
 {
   using base_class = x86_nat_target<BaseTarget>;
 public:
-#ifdef HAVE_PT_GETDBREGS
   void mourn_inferior () override
   {
     x86_cleanup_dregs ();
     base_class::mourn_inferior ();
   }
+};
+#else /* !HAVE_PT_GETDBREGS */
+template<typename BaseTarget>
+class x86bsd_nat_target : public BaseTarget
+{
+};
 #endif /* HAVE_PT_GETDBREGS */
-};
 
 #endif /* x86-bsd-nat.h */
-- 
2.31.1

[PATCH v2 2/5] x86-bsd-nat: Only define gdb_ptrace when using debug registers.

[John Baldwin]

This fixes an unused function warning on OpenBSD which does not
support PT_GETDBREGS.
---
 gdb/x86-bsd-nat.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/gdb/x86-bsd-nat.c b/gdb/x86-bsd-nat.c
index 453fc44116c..6aac76f1826 100644
--- a/gdb/x86-bsd-nat.c
+++ b/gdb/x86-bsd-nat.c
@@ -33,6 +33,14 @@
 #include "inf-ptrace.h"
 \f
 
+#ifdef PT_GETXSTATE_INFO
+size_t x86bsd_xsave_len;
+#endif
+
+/* Support for debug registers.  */
+
+#ifdef HAVE_PT_GETDBREGS
+
 static PTRACE_TYPE_RET
 gdb_ptrace (PTRACE_TYPE_ARG1 request, ptid_t ptid, PTRACE_TYPE_ARG3 addr)
 {
@@ -46,14 +54,6 @@ gdb_ptrace (PTRACE_TYPE_ARG1 request, ptid_t ptid, PTRACE_TYPE_ARG3 addr)
 #endif
 }
 
-#ifdef PT_GETXSTATE_INFO
-size_t x86bsd_xsave_len;
-#endif
-
-/* Support for debug registers.  */
-
-#ifdef HAVE_PT_GETDBREGS
-
 /* Helper macro to access debug register X.  FreeBSD/amd64 and modern
    versions of FreeBSD/i386 provide this macro in system headers.  Define
    a local version for systems that do not provide it.  */
-- 
2.31.1

[PATCH v2 3/5] obsd-nat: Various fixes to obsd_nat_target::wait.

[John Baldwin]

- Call inf_ptrace_target::wait instead of duplicating the code.
  Replace a check for WIFSTOPPED on the returned status from waitpid
  by checking for TARGET_WAITKIND_STOPPED in the parsed status as is
  done in fbsd_nat_target::wait.

- Don't use inferior_ptid when deciding if a new process is a child vs
  parent of the fork.  Instead, use find_inferior_pid and assume that
  if an inferior already exists, the pid in question is the parent;
  otherwise, the pid is the child.

- Don't use inferior_ptid when deciding if the ptid of the process
  needs to be updated with an LWP ID, or if this is a new thread.
  Instead, use the approach from fbsd-nat which is to check if a ptid
  without an LWP exists and if so update the ptid of that thread
  instead of adding a new thread.
---
 gdb/obsd-nat.c | 61 +++++++++++---------------------------------------
 1 file changed, 13 insertions(+), 48 deletions(-)

diff --git a/gdb/obsd-nat.c b/gdb/obsd-nat.c
index 46fdc0676ea..8549c6ea66b 100644
--- a/gdb/obsd-nat.c
+++ b/gdb/obsd-nat.c
@@ -26,7 +26,7 @@
 #include <sys/ptrace.h>
 #include "gdbsupport/gdb_wait.h"
 
-#include "inf-child.h"
+#include "inf-ptrace.h"
 #include "obsd-nat.h"
 
 /* OpenBSD 5.2 and later include rthreads which uses a thread model
@@ -76,47 +76,14 @@ ptid_t
 obsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 		       target_wait_flags options)
 {
-  pid_t pid;
-  int status, save_errno;
-
-  do
-    {
-      set_sigint_trap ();
-
-      do
-	{
-	  pid = waitpid (ptid.pid (), &status, 0);
-	  save_errno = errno;
-	}
-      while (pid == -1 && errno == EINTR);
-
-      clear_sigint_trap ();
-
-      if (pid == -1)
-	{
-	  fprintf_unfiltered (gdb_stderr,
-			      _("Child process unexpectedly missing: %s.\n"),
-			      safe_strerror (save_errno));
-
-	  /* Claim it exited with unknown signal.  */
-	  ourstatus->kind = TARGET_WAITKIND_SIGNALLED;
-	  ourstatus->value.sig = GDB_SIGNAL_UNKNOWN;
-	  return inferior_ptid;
-	}
-
-      /* Ignore terminated detached child processes.  */
-      if (!WIFSTOPPED (status) && pid != inferior_ptid.pid ())
-	pid = -1;
-    }
-  while (pid == -1);
-
-  ptid = ptid_t (pid);
-
-  if (WIFSTOPPED (status))
+  ptid_t wptid = inf_ptrace_target::wait (ptid, ourstatus, options);
+  if (ourstatus->kind == TARGET_WAITKIND_STOPPED)
     {
       ptrace_state_t pe;
-      pid_t fpid;
+      pid_t fpid, pid;
+      int status;
 
+      pid = wptid.pid ();
       if (ptrace (PT_GET_PROCESS_STATE, pid, (caddr_t)&pe, sizeof pe) == -1)
 	perror_with_name (("ptrace"));
 
@@ -137,7 +104,7 @@ obsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 
 	  gdb_assert (pe.pe_report_event == PTRACE_FORK);
 	  gdb_assert (pe.pe_other_pid == pid);
-	  if (fpid == inferior_ptid.pid ())
+	  if (find_inferior_pid (this, fpid) != nullptr)
 	    {
 	      ourstatus->value.related_pid = ptid_t (pe.pe_other_pid);
 	      return ptid_t (fpid);
@@ -146,18 +113,16 @@ obsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 	  return ptid_t (pid);
 	}
 
-      ptid = ptid_t (pid, pe.pe_tid, 0);
-      if (!in_thread_list (this, ptid))
+      wptid = ptid_t (pid, pe.pe_tid, 0);
+      if (!in_thread_list (this, wptid))
 	{
-	  if (inferior_ptid.lwp () == 0)
-	    thread_change_ptid (this, inferior_ptid, ptid);
+	  if (in_thread_list (this, ptid_t (pid)))
+	    thread_change_ptid (this, ptid_t (pid), wptid);
 	  else
-	    add_thread (this, ptid);
+	    add_thread (this, wptid);
 	}
     }
-
-  store_waitstatus (ourstatus, status);
-  return ptid;
+  return wptid;
 }
 
 #endif /* PT_GET_THREAD_FIRST */
-- 
2.31.1

[PATCH v2 4/5] obsd-nat: Various fixes for fork following.

[John Baldwin]

- Don't use #ifdef's on ptrace ops.  obsd-nat.h didn't include
  <sys/ptrace.h>, so the virtual methods weren't always overridden
  causing the fork following to not work.  In addition, the thread and
  fork code is intertwined in ::wait and and the lack of #ifdef's
  there already assumed both were present.

- Move duplicated code to enable PTRACE_FORK event reporting to a
  single function and invoke it on new child processes reported via
  PTRACE_FORK.

- Don't return early from PTRACE_FORK handling, but instead reset
  wptid to the correct ptid if the child reports its event before the
  parent.  This allows the ptid fixup code to add thread IDs if the
  first event for a process is a PTRACE_FORK event.  This also
  properly returns ptid's with thread IDs when reporting PTRACE_FORK
  events.

- Handle detach_fork by skipping the PT_DETACH.
---
 gdb/obsd-nat.c | 51 +++++++++++++++++++++++---------------------------
 gdb/obsd-nat.h |  2 --
 2 files changed, 23 insertions(+), 30 deletions(-)

diff --git a/gdb/obsd-nat.c b/gdb/obsd-nat.c
index 8549c6ea66b..0edaa66cf1b 100644
--- a/gdb/obsd-nat.c
+++ b/gdb/obsd-nat.c
@@ -33,8 +33,6 @@
    that maps userland threads directly onto kernel threads in a 1:1
    fashion.  */
 
-#ifdef PT_GET_THREAD_FIRST
-
 std::string
 obsd_nat_target::pid_to_str (ptid_t ptid)
 {
@@ -72,6 +70,19 @@ obsd_nat_target::update_thread_list ()
     }
 }
 
+static void
+obsd_enable_proc_events (pid_t pid)
+{
+  ptrace_event_t pe;
+
+  /* Set the initial event mask.  */
+  memset (&pe, 0, sizeof pe);
+  pe.pe_set_event |= PTRACE_FORK;
+  if (ptrace (PT_SET_EVENT_MASK, pid,
+	      (PTRACE_TYPE_ARG3)&pe, sizeof pe) == -1)
+    perror_with_name (("ptrace"));
+}
+
 ptid_t
 obsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 		       target_wait_flags options)
@@ -87,6 +98,8 @@ obsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
       if (ptrace (PT_GET_PROCESS_STATE, pid, (caddr_t)&pe, sizeof pe) == -1)
 	perror_with_name (("ptrace"));
 
+      wptid = ptid_t (pid, pe.pe_tid, 0);
+
       switch (pe.pe_report_event)
 	{
 	case PTRACE_FORK:
@@ -107,13 +120,15 @@ obsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 	  if (find_inferior_pid (this, fpid) != nullptr)
 	    {
 	      ourstatus->value.related_pid = ptid_t (pe.pe_other_pid);
-	      return ptid_t (fpid);
+	      wptid = ptid_t (fpid, pe.pe_tid, 0);
 	    }
 
-	  return ptid_t (pid);
+	  obsd_enable_proc_events (ourstatus->value.related_pid.pid ());
+	  break;
 	}
 
-      wptid = ptid_t (pid, pe.pe_tid, 0);
+      /* Ensure the ptid is updated with an LWP id on the first stop
+         of a process.  */
       if (!in_thread_list (this, wptid))
 	{
 	  if (in_thread_list (this, ptid_t (pid)))
@@ -125,34 +140,16 @@ obsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
   return wptid;
 }
 
-#endif /* PT_GET_THREAD_FIRST */
-
-#ifdef PT_GET_PROCESS_STATE
-
 void
 obsd_nat_target::post_attach (int pid)
 {
-  ptrace_event_t pe;
-
-  /* Set the initial event mask.  */
-  memset (&pe, 0, sizeof pe);
-  pe.pe_set_event |= PTRACE_FORK;
-  if (ptrace (PT_SET_EVENT_MASK, pid,
-	      (PTRACE_TYPE_ARG3)&pe, sizeof pe) == -1)
-    perror_with_name (("ptrace"));
+  obsd_enable_proc_events (pid);
 }
 
 void
 obsd_nat_target::post_startup_inferior (ptid_t pid)
 {
-  ptrace_event_t pe;
-
-  /* Set the initial event mask.  */
-  memset (&pe, 0, sizeof pe);
-  pe.pe_set_event |= PTRACE_FORK;
-  if (ptrace (PT_SET_EVENT_MASK, pid.pid (),
-	      (PTRACE_TYPE_ARG3)&pe, sizeof pe) == -1)
-    perror_with_name (("ptrace"));
+  obsd_enable_proc_events (pid.pid ());
 }
 
 /* Target hook for follow_fork.  On entry and at return inferior_ptid is
@@ -162,7 +159,7 @@ void
 obsd_nat_target::follow_fork (ptid_t child_ptid, target_waitkind fork_kind,
 			      bool follow_child, bool detach_fork)
 {
-  if (!follow_child)
+  if (!follow_child && detach_fork)
     {
       /* Breakpoints have already been detached from the child by
 	 infrun.c.  */
@@ -183,5 +180,3 @@ obsd_nat_target::remove_fork_catchpoint (int pid)
 {
   return 0;
 }
-
-#endif /* PT_GET_PROCESS_STATE */
diff --git a/gdb/obsd-nat.h b/gdb/obsd-nat.h
index ddd4baf7761..43a793e0a31 100644
--- a/gdb/obsd-nat.h
+++ b/gdb/obsd-nat.h
@@ -29,7 +29,6 @@ class obsd_nat_target : public inf_ptrace_target
   void update_thread_list () override;
   ptid_t wait (ptid_t, struct target_waitstatus *, target_wait_flags) override;
 
-#ifdef PT_GET_PROCESS_STATE
   void follow_fork (ptid_t, target_waitkind, bool, bool) override;
 
   int insert_fork_catchpoint (int) override;
@@ -39,7 +38,6 @@ class obsd_nat_target : public inf_ptrace_target
   void post_startup_inferior (ptid_t) override;
 
   void post_attach (int) override;
-#endif
 };
 
 #endif /* obsd-nat.h */
-- 
2.31.1

[PATCH v2 5/5] obsd-nat: Report both thread and PID in ::pid_to_str.

[John Baldwin]

This improves the output of info threads when debugging multiple
inferiors (e.g. after a fork with detach_on_fork disabled).
---
 gdb/obsd-nat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/obsd-nat.c b/gdb/obsd-nat.c
index 0edaa66cf1b..be3e5f95b16 100644
--- a/gdb/obsd-nat.c
+++ b/gdb/obsd-nat.c
@@ -37,7 +37,7 @@ std::string
 obsd_nat_target::pid_to_str (ptid_t ptid)
 {
   if (ptid.lwp () != 0)
-    return string_printf ("thread %ld", ptid.lwp ());
+    return string_printf ("thread %ld of process %d", ptid.lwp (), ptid.pid ());
 
   return normal_pid_to_str (ptid);
 }
-- 
2.31.1

Re: [PATCH v2 3/5] obsd-nat: Various fixes to obsd_nat_target::wait.

[Simon Marchi]

On 2021-07-27 1:41 p.m., John Baldwin wrote:
> - Call inf_ptrace_target::wait instead of duplicating the code.
>   Replace a check for WIFSTOPPED on the returned status from waitpid
>   by checking for TARGET_WAITKIND_STOPPED in the parsed status as is
>   done in fbsd_nat_target::wait.
> 
> - Don't use inferior_ptid when deciding if a new process is a child vs
>   parent of the fork.  Instead, use find_inferior_pid and assume that
>   if an inferior already exists, the pid in question is the parent;
>   otherwise, the pid is the child.
> 
> - Don't use inferior_ptid when deciding if the ptid of the process
>   needs to be updated with an LWP ID, or if this is a new thread.
>   Instead, use the approach from fbsd-nat which is to check if a ptid
>   without an LWP exists and if so update the ptid of that thread
>   instead of adding a new thread.
> ---
>  gdb/obsd-nat.c | 61 +++++++++++---------------------------------------
>  1 file changed, 13 insertions(+), 48 deletions(-)
> 
> diff --git a/gdb/obsd-nat.c b/gdb/obsd-nat.c
> index 46fdc0676ea..8549c6ea66b 100644
> --- a/gdb/obsd-nat.c
> +++ b/gdb/obsd-nat.c
> @@ -26,7 +26,7 @@
>  #include <sys/ptrace.h>
>  #include "gdbsupport/gdb_wait.h"
>  
> -#include "inf-child.h"
> +#include "inf-ptrace.h"
>  #include "obsd-nat.h"
>  
>  /* OpenBSD 5.2 and later include rthreads which uses a thread model
> @@ -76,47 +76,14 @@ ptid_t
>  obsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>  		       target_wait_flags options)
>  {
> -  pid_t pid;
> -  int status, save_errno;
> -
> -  do
> -    {
> -      set_sigint_trap ();
> -
> -      do
> -	{
> -	  pid = waitpid (ptid.pid (), &status, 0);
> -	  save_errno = errno;
> -	}
> -      while (pid == -1 && errno == EINTR);
> -
> -      clear_sigint_trap ();
> -
> -      if (pid == -1)
> -	{
> -	  fprintf_unfiltered (gdb_stderr,
> -			      _("Child process unexpectedly missing: %s.\n"),
> -			      safe_strerror (save_errno));
> -
> -	  /* Claim it exited with unknown signal.  */
> -	  ourstatus->kind = TARGET_WAITKIND_SIGNALLED;
> -	  ourstatus->value.sig = GDB_SIGNAL_UNKNOWN;
> -	  return inferior_ptid;
> -	}
> -
> -      /* Ignore terminated detached child processes.  */
> -      if (!WIFSTOPPED (status) && pid != inferior_ptid.pid ())
> -	pid = -1;
> -    }
> -  while (pid == -1);
> -
> -  ptid = ptid_t (pid);
> -
> -  if (WIFSTOPPED (status))
> +  ptid_t wptid = inf_ptrace_target::wait (ptid, ourstatus, options);
> +  if (ourstatus->kind == TARGET_WAITKIND_STOPPED)
>      {
>        ptrace_state_t pe;
> -      pid_t fpid;
> +      pid_t fpid, pid;

Declare fpid in the scope where it's used, where first initialized, that
will help understand it's only used there.

> +      int status;
>  
> +      pid = wptid.pid ();
>        if (ptrace (PT_GET_PROCESS_STATE, pid, (caddr_t)&pe, sizeof pe) == -1)
>  	perror_with_name (("ptrace"));
>  
> @@ -137,7 +104,7 @@ obsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>  
>  	  gdb_assert (pe.pe_report_event == PTRACE_FORK);
>  	  gdb_assert (pe.pe_other_pid == pid);
> -	  if (fpid == inferior_ptid.pid ())
> +	  if (find_inferior_pid (this, fpid) != nullptr)
>  	    {
>  	      ourstatus->value.related_pid = ptid_t (pe.pe_other_pid);
>  	      return ptid_t (fpid);
> @@ -146,18 +113,16 @@ obsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>  	  return ptid_t (pid);

I don't know if it matters here, but is the pid reported by waitpid the
thread id, a bit like on Linux?  In other words, when a thread in a
multi-threaded program calls fork, what id does waitpid report for the
parent?  And depending on the answert, should we return a ptid with an
lwp component, instead of a pid-only ptid?

Simon

Re: [PATCH v2 3/5] obsd-nat: Various fixes to obsd_nat_target::wait.

[Simon Marchi]

> I don't know if it matters here, but is the pid reported by waitpid the
> thread id, a bit like on Linux?  In other words, when a thread in a
> multi-threaded program calls fork, what id does waitpid report for the
> parent?  And depending on the answert, should we return a ptid with an
> lwp component, instead of a pid-only ptid?

I think that's done in the following patch actually, so forget about
that.

Simon

Re: [PATCH v2 4/5] obsd-nat: Various fixes for fork following.

[Simon Marchi]

On 2021-07-27 1:41 p.m., John Baldwin wrote:
> - Don't use #ifdef's on ptrace ops.  obsd-nat.h didn't include
>   <sys/ptrace.h>, so the virtual methods weren't always overridden
>   causing the fork following to not work.  In addition, the thread and
>   fork code is intertwined in ::wait and and the lack of #ifdef's
>   there already assumed both were present.

In any case, PT_GET_PROCESS_STATE was added here, 16 years ago:

  https://github.com/openbsd/src/commit/f38bed7f869bd3503530c554b4860228ea4e8641

So it's reasonable to assume it's there (especially for a port with no
maintainer).

> 
> - Move duplicated code to enable PTRACE_FORK event reporting to a
>   single function and invoke it on new child processes reported via
>   PTRACE_FORK.
> 
> - Don't return early from PTRACE_FORK handling, but instead reset
>   wptid to the correct ptid if the child reports its event before the
>   parent.  This allows the ptid fixup code to add thread IDs if the
>   first event for a process is a PTRACE_FORK event.  This also
>   properly returns ptid's with thread IDs when reporting PTRACE_FORK
>   events.
> 
> - Handle detach_fork by skipping the PT_DETACH.
> ---
>  gdb/obsd-nat.c | 51 +++++++++++++++++++++++---------------------------
>  gdb/obsd-nat.h |  2 --
>  2 files changed, 23 insertions(+), 30 deletions(-)
> 
> diff --git a/gdb/obsd-nat.c b/gdb/obsd-nat.c
> index 8549c6ea66b..0edaa66cf1b 100644
> --- a/gdb/obsd-nat.c
> +++ b/gdb/obsd-nat.c
> @@ -33,8 +33,6 @@
>     that maps userland threads directly onto kernel threads in a 1:1
>     fashion.  */
>  
> -#ifdef PT_GET_THREAD_FIRST

10 years ago for this one.  Still long enough, and removing it makes the
code much simpler to understand.

> -
>  std::string
>  obsd_nat_target::pid_to_str (ptid_t ptid)
>  {
> @@ -72,6 +70,19 @@ obsd_nat_target::update_thread_list ()
>      }
>  }
>  
> +static void
> +obsd_enable_proc_events (pid_t pid)

Could you please write a small, oneliner comment for this one?

Simon

Re: [PATCH v2 0/5] A few fixes to OpenBSD's native target

[Simon Marchi]

On 2021-07-27 1:41 p.m., John Baldwin wrote:
> This addresses some feedback from Simon but also adds two additional
> patches that allow some simple fork following to mostly work for me on
> OpenBSD/amd64 6.9.
> 
> The one remaining issue I see is that if I disable detach on fork and
> allow a child process to run to completion then go back and continue
> the parent process, the parent process is killed with a SIGHUP.  I
> think this is probably an issue in OpenBSD's kernel though rather than
> GDB and I don't plan on investigating that further.
> 
> John Baldwin (5):
>   Don't compile x86 debug register support on OpenBSD.
>   x86-bsd-nat: Only define gdb_ptrace when using debug registers.
>   obsd-nat: Various fixes to obsd_nat_target::wait.
>   obsd-nat: Various fixes for fork following.
>   obsd-nat: Report both thread and PID in ::pid_to_str.
> 
>  gdb/configure.nat |   5 +--
>  gdb/obsd-nat.c    | 112 +++++++++++++++-------------------------------
>  gdb/obsd-nat.h    |   2 -
>  gdb/x86-bsd-nat.c |  16 +++----
>  gdb/x86-bsd-nat.h |   9 +++-
>  5 files changed, 53 insertions(+), 91 deletions(-)
> 

Thanks a lot for this, this LGTM, I just noted a few nits to fix.  I'll rebase
my follow-fork patch on top of this once it's merged.

Simon

Re: [PATCH v2 3/5] obsd-nat: Various fixes to obsd_nat_target::wait.

[John Baldwin]

On 7/29/21 12:11 PM, Simon Marchi wrote:
>> I don't know if it matters here, but is the pid reported by waitpid the
>> thread id, a bit like on Linux?  In other words, when a thread in a
>> multi-threaded program calls fork, what id does waitpid report for the
>> parent?  And depending on the answert, should we return a ptid with an
>> lwp component, instead of a pid-only ptid?
> 
> I think that's done in the following patch actually, so forget about
> that.

Yes, sorry, I was just trying to fix the use of inferior_ptid in this
patch and focused more on fixing fork following in the next one.

To be clear though, the BSD's report a pid from waitpid().  Threads
on Free/Net/OpenBSD use an orthogonal namespace for thread IDs that
is separate from process IDs and there are 1 or more thread IDs
associated with each process.  On FreeBSD, thread IDs start at PID_MAX + 1,
and there are a few places in FreeBSD where one can use either a thread
ID or process ID (ptrace() is one syscall for which this is true).  The
kernel compares the value against PID_MAX in those cases to determine if
it is a thread (LWP) ID or process ID.

-- 
John Baldwin

Re: [PATCH v2 3/5] obsd-nat: Various fixes to obsd_nat_target::wait.

[Simon Marchi]

> To be clear though, the BSD's report a pid from waitpid().  Threads
> on Free/Net/OpenBSD use an orthogonal namespace for thread IDs that
> is separate from process IDs and there are 1 or more thread IDs
> associated with each process.  On FreeBSD, thread IDs start at PID_MAX + 1,
> and there are a few places in FreeBSD where one can use either a thread
> ID or process ID (ptrace() is one syscall for which this is true).  The
> kernel compares the value against PID_MAX in those cases to determine if
> it is a thread (LWP) ID or process ID.

Ah, thanks.  I knew about NetBSD because Kamil mentioned it, I didn't
know it was somewhat similar across BSDs.

Simon