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

[John Baldwin <jhb@FreeBSD.org>]

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