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

[John Baldwin <jhb@FreeBSD.org>]

On 4/14/20 10:54 AM, Pedro Alves via Gdb-patches wrote:
> In PR/25412, Simon noticed that after the multi-target series, the
> tid-reuse.exp testcase manages to create a duplicate thread in the
> thread list.  Or rather, two threads with the same PTID.
> 
> This in turn exposes a design problem in GDB.  The inferior_thread()
> function looks up the current thread based on inferior_ptid:
> 
>  struct thread_info*
>  inferior_thread (void)
>  {
>    struct thread_info *tp = find_thread_ptid (current_inferior (), inferior_ptid);
>    gdb_assert (tp);
>    return tp;
>  }
> 
> But if there can be more than one thread in the thread list with the
> same ptid_t, inferior_thread() may well return the wrong thread.
> 
> This series fixes this by making the current thread be a global
> thread_info pointer that is written to directly by switch_to_thread,
> etc., and making inferior_thread() return that pointer, instead of
> having inferior_thread() lookup up the inferior_ptid thread, by
> ptid_t.  You can look at this as a continuation of the effort of using
> more thread_info pointers instead of ptids when possible.
> 
> This change required auditing the whole codebase for places where we
> were writing to inferior_ptid directly to change the current thread,
> and change them to use switch_to_thread instead or one of its
> siblings, because otherwise inferior_thread() and inferior_ptid would
> get out of sync and inferior_thread() would return a thread unrelated
> to the new inferior_ptid we wanted to switch to.  That was all
> (hopefully) done in all the patches leading up to the last one.
> 
> After this, inferior_ptid still exists, but it is mostly read-only and
> mainly used by target backend code.  It is also relied on by a number
> of target methods as a global input argument.  E.g., the target_resume
> interface and the memory reading routines -- we still need it there
> because we need to be able to access memory off of processes for which
> we don't have a corresponding inferior/thread object, like when
> handling forks.  Maybe we could pass down a context explicitly to
> target_read_memory, etc.
> 
> Most of the host-/native-specific code here is untested.  I did my
> best, but I won't be surprised if more tweaking is necessary.
> 
> Testing on native x86_64 GNU/Linux is regression free for me.  Testing
> against gdbserver has regressed significantly in the past months and
> is becoming difficult to run with a high number of long timeout
> sequences; really looks like people aren't paying much attention to
> that.  I think this series doesn't regress gdbserver, but it's getting
> hard to tell.  :-/

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.

It seems to me that multiprocess probably needs to return events for
not just the current inferior pid but for any valid pid for example,
and though multiprocess is still broken for me on FreeBSD if I comment
out the check against inferior_ptid, I can now see that I was getting
an event for the "wrong" inferior that was previously discarded but
with the check commented out now gets reported to the core.

(The reason I get an event for the wrong process is that for some
reason the core asks the native target to resume the wrong process:

> ./gdb /bin/ls
(gdb) set debug fbsd-nat 
(gdb) set debug fbsd-lwp 
(gdb) start
Temporary breakpoint 1 at 0x20430a: file /usr/src/bin/ls/ls.c, line 236.
Starting program: /bin/ls 
FNAT: stop for LWP 101518 event 1 flags 0x18
FLWP: using LWP 101518 for first thread
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101518 event 1 flags 0x20
FNAT: si_signo 20 si_code 1
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101518 event 1 flags 0x20
FNAT: si_signo 20 si_code 1
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101518 event 1 flags 0x20
FNAT: si_signo 20 si_code 1
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101518 event 1 flags 0x20
FNAT: si_signo 20 si_code 1
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101518 event 1 flags 0x20
FNAT: si_signo 20 si_code 1
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101518 event 1 flags 0x18
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101518 event 1 flags 0x20
FNAT: si_signo 5 si_code 1
FNAT: sw breakpoint trap for LWP 101518
FLWP: fbsd_resume for ptid (70453, 101518, 0)
FNAT: stop for LWP 101518 event 1 flags 0x20
FNAT: si_signo 5 si_code 2
FNAT: trace trap for LWP 101518
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101518 event 1 flags 0x20
FNAT: si_signo 5 si_code 1
FNAT: sw breakpoint trap for LWP 101518

Temporary breakpoint 1, main (argc=1, argv=0x7fffffffe710)
    at /usr/src/bin/ls/ls.c:236
236		const char *errstr = NULL;
(gdb) add-inferior 
[New inferior 2]
Added inferior 2 on connection 1 (native)
(gdb) inferior 2
[Switching to inferior 2 [<null>] (<noexec>)]
(gdb) file /bin/ls
Reading symbols from /bin/ls...
Reading symbols from /usr/lib/debug//bin/ls.debug...
(gdb) start
Temporary breakpoint 2 at 0x20430a: -qualified main. (2 locations)
Starting program: /bin/ls 
FNAT: stop for LWP 101641 event 1 flags 0x18
FLWP: using LWP 101641 for first thread
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101641 event 1 flags 0x20
FNAT: si_signo 20 si_code 1
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101641 event 1 flags 0x20
FNAT: si_signo 20 si_code 1
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101641 event 1 flags 0x20
FNAT: si_signo 20 si_code 1
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101641 event 1 flags 0x24
FNAT: si_signo 20 si_code 1
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101641 event 1 flags 0x24
FNAT: si_signo 20 si_code 1
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101641 event 1 flags 0x20
FNAT: si_signo 20 si_code 1
FLWP: fbsd_resume for ptid (-1, 0, 0)
FNAT: stop for LWP 101641 event 1 flags 0x18
FLWP: fbsd_resume for ptid (70453, 101518, 0)

Program terminated with signal SIGTRAP, Trace/breakpoint trap.
The program no longer exists.

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.)

-- 
John Baldwin