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

John Baldwin
2020-07-08T00:20:22Z

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