Re: [PATCH 00/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[John Baldwin <jhb@FreeBSD.org>]

On 7/7/20 4:53 PM, Pedro Alves wrote:
> On 7/8/20 12:16 AM, John Baldwin wrote:
>> This appears to have broken native debugging on FreeBSD for me in that
>> when I run a process to completion it triggers an assertion failure:
>>
>> (gdb) r
>> Starting program: /bin/echo 
>>
>> Child process unexpectedly missing: No child processes.
>> /home/john/work/git/gdb/gdb/inferior.c:293: internal-error: struct inferior *find_inferior_pid(process_stratum_target *, int): Assertion `pid != 0' failed.
>>
>> I tracked this down to this code in inf_ptrace::wait():
>>
>>       /* Ignore terminated detached child processes.  */
>>       if (!WIFSTOPPED (status) && pid != inferior_ptid.pid ())
>>         pid = -1;
>>
>> At this point, inferior_ptid() is all zeroes and the process
>> has reported a non-stopped exit status (WIFEXITED) so this
>> ignores the exit event and loops back around to call waitpid()
>> again which then fails with ECHILD.
>>
>> Looks like we always clear the inferior thread (and thus
>> inferior_ptid) in do_target_wait_1:
>>
>>   /* We know that we are looking for an event in the target of inferior
>>      INF, but we don't know which thread the event might come from.  As
>>      such we want to make sure that INFERIOR_PTID is reset so that none of
>>      the wait code relies on it - doing so is always a mistake.  */
>>   switch_to_inferior_no_thread (inf);
>>
>> Commenting out the check in inf_ptrace::wait() "fixes" the issue for
>> me on FreeBSD, but I'm not sure that is the right fix.
> 
> linux-nat.c has something logic, here in linux_nat_filter_event:
> 
>   /* Make sure we don't report an event for the exit of an LWP not in
>      our list, i.e. not part of the current process.  This can happen
>      if we detach from a program we originally forked and then it
>      exits.  */
>   if (!WIFSTOPPED (status) && !lp)
>     return NULL;
> 
> For inf-ptrace.c, the right fix should be something around this:
> 
> diff --git c/gdb/inf-ptrace.c w/gdb/inf-ptrace.c
> index d25d226abb..ae0b0f7ff0 100644
> --- c/gdb/inf-ptrace.c
> +++ w/gdb/inf-ptrace.c
> @@ -347,7 +347,7 @@ inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>         }
>  
>        /* Ignore terminated detached child processes.  */
> -      if (!WIFSTOPPED (status) && pid != inferior_ptid.pid ())
> +      if (!WIFSTOPPED (status) && find_inferior_pid (this, pid) == nullptr)
>         pid = -1;
>      }
>    while (pid == -1);

Yes, this works.  I hadn't found that function and was thinking of
writing it from scratch, but this is better of course.
 
> The inferior_ptid reference in the error path above that code, where it
> reads:
> 
>   "Claim it exited with unknown signal"
> 
> is also wrong.  I'm not sure what to do about that one.
> Not clear if that path can really happen.  linux-nat.c calls
> perror_with_name in some places if waitpid returns -1,
> and in the main waitpid call, just assumes that error -1 never
> happens...

It happened for me in this case since wait failed with -1 when we discarded
the exit info incorrectly.  I can change it to return null_ptid which would
match what it does now, or perhaps we could just add an assertion here rather
than relying on the  null_ptid tripping over another assertion later?

>> Here you can see that the last call to fbsd_resume() used the ptid from
>> inferior 1 instead of inferior 2, and inferior 1 didn't discard it's
>> pending SIGTRAP but instead was killed by it.)
> "set debug infrun 1" will probably reveal what is happening.

Yes, I have some more details on this in my other followup where infrun
debugging was very helpful, though I realize I failed to include any of
the infrun debug output.

-- 
John Baldwin