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

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

[John Baldwin]

The first patch is an alternative fix for the issue Simon ran into
with OpenBSD trying to use the x86 debug registers when it doesn't
support their use by a debugger.

The second patch pacifies a compiler warning due to the lack of debug
register support.

The third patch fixes an issue where an existing process tripped an
assertion where find_inferior_pid was called with a pid of 0
(unfortunately stack traces don't work for OpenBSD core dumps, so I
couldn't easily tell where hte failure was).  However, the use of
inferior_ptid is not correct anymore in ::wait methods and fixing that
alone allows a process to exit cleanly without error.

This is enough to get a single threaded program to run and exit
cleanly (which is all I tested).  There are likely other issues in the
OpenBSD target, but this at least gets it working for the simple case
again.

John Baldwin (3):
  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.

 gdb/configure.nat |  5 ++--
 gdb/obsd-nat.c    | 61 ++++++++++-------------------------------------
 gdb/x86-bsd-nat.c | 16 ++++++-------
 gdb/x86-bsd-nat.h |  9 +++++--
 4 files changed, 30 insertions(+), 61 deletions(-)

-- 
2.31.1

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

[John Baldwin]

- 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 e34cccffd9..655c75dd1a 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 02d61c20b0..caf62e38df 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

Re: [PATCH 1/3] Don't compile x86 debug register support on OpenBSD.

[Simon Marchi via Gdb-patches]

On 2021-07-26 8:24 p.m., John Baldwin wrote:
> - 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.

LGTM, but please add a little bit of info to the commit message, to
explain why this change is needed.  You can copy some of that from my
patch's message.

Simon

[PATCH 2/3] 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 453fc44116..6aac76f182 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

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

[Simon Marchi via Gdb-patches]

On 2021-07-26 8:24 p.m., John Baldwin wrote:
> 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 453fc44116..6aac76f182 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.  */
> 


OK.

Simon

[PATCH 3/3] 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 46fdc0676e..a6612a982b 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, pid) != 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

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

[Simon Marchi via Gdb-patches]

On 2021-07-26 8:24 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 46fdc0676e..a6612a982b 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, pid) != nullptr)
>  	    {
>  	      ourstatus->value.related_pid = ptid_t (pe.pe_other_pid);
>  	      return ptid_t (fpid);

I don't really have comments on this, since I am not familiar with the
OpenBSD specifics.  But I don't understand this code above.  fpid refers
to the fork child pid?  And pe.pe_other_pid as well?  So it's as if we
return that the child has forked?  What am I getting wrong?

Simon

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

[John Baldwin]

On 7/26/21 6:54 PM, Simon Marchi wrote:
> 
> 
> On 2021-07-26 8:24 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 46fdc0676e..a6612a982b 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, pid) != nullptr)
>>   	    {
>>   	      ourstatus->value.related_pid = ptid_t (pe.pe_other_pid);
>>   	      return ptid_t (fpid);
> 
> I don't really have comments on this, since I am not familiar with the
> OpenBSD specifics.  But I don't understand this code above.  fpid refers
> to the fork child pid?  And pe.pe_other_pid as well?  So it's as if we
> return that the child has forked?  What am I getting wrong?

My reading of the code is that when a process forks, both the parent and
child report identical PTRACE_FORK events where pe_other_pid is the pid
of the other process.  For TARGET_WAITKIND_FORKED, both processes need to
have reported their events (which is then reported as a single logical
event to the core), so when ::wait sees the first PTRACE_FORK event, it
immediately waits for the other process to report an event (and asserts
it is also a PTRACE_FORK).  fpid is the pid of the second process to
report an event, and pid is the pid of the original process that reported
an event.  The find_inferior_pid check is used to determine which of
'pid' or 'fpid' is the parent.  If find_inferior_pid (this, pid) finds
an existing inferior, then 'pid' is the parent and 'fpid' is the child.

It does seem though that both the existing code (that was checking
inferior_ptid) and the new version both return a ptid for the child
process.  fbsd-nat.c returns the ptid of the parent with the child
pid set in the ourstatus->value.related_pid.  Oh, I see what I did wrong,
the old code was comparing 'fpid' against inferior_ptid and I changed
this to 'pid'.

-- 
John Baldwin