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

[Simon Marchi <simon.marchi@polymtl.ca>]

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