Re: [PATCH][PR gdb/28874] gdb/remote: switch to added threads if there isn't one already

[Pedro Alves <pedro@palves.net>]

Hi!  Sorry for the delay in moving this.

On 2022-07-18 7:51 p.m., Mikaela Szekely via Gdb-patches wrote:
> Is there anything specific that's blocking this patch from being
> accepted? This is still an active bug in GDB.

I can't speak for others, but for me, it's that I miss a sufficiently convincing rationale
for the change as is, and I haven't found the time to investigate this deeply enough myself.  Off
hand, this doesn't look like the right fix to me.  Making the "add thread" function
switch threads as side effect looks odd and likely to cause surprises.

I gave it a look today.  I was able to tweak the remote log in the bug and reproduce the issue here,
by running gdb against gdbreplay.

So we attach in extended-remote mode, and then use "attach 1" to attach to something.  That's when
GDB fails the assertion.  The backtrace looks like this:

(top-gdb) bt
#0  internal_error (file=0xb00000000 <error: Cannot access memory at address 0xb00000000>, line=0, fmt=0x5555565b8ca0 "en_US.UTF-8") at ../../src/gdbsupport/errors.cc:51
#1  0x0000555555d1eba8 in inferior_thread () at ../../src/gdb/thread.c:85
#2  0x0000555555af6dfd in mi_on_resume (ptid=...) at ../../src/gdb/mi/mi-interp.c:1030
#3  0x0000555555a5351d in std::_Function_handler<void (ptid_t), void (*)(ptid_t)>::_M_invoke(std::_Any_data const&, ptid_t&&) (__functor=..., __args#0=...) at /usr/include/c++/9/bits/std_function.h:300
#4  0x0000555555d19002 in std::function<void (ptid_t)>::operator()(ptid_t) const (this=0x5555565f20c8, __args#0=...) at /usr/include/c++/9/bits/std_function.h:688
#5  0x0000555555d178d8 in gdb::observers::observable<ptid_t>::notify (this=0x555556551620 <gdb::observers::target_resumed>, args#0=...) at ../../src/gdb/../gdbsupport/observable.h:166
#6  0x0000555555d21520 in set_running (targ=0x555556bd5990, ptid=..., running=true) at ../../src/gdb/thread.c:872
#7  0x0000555555c1116f in remote_target::remote_add_thread (this=0x555556bd5990, ptid=..., running=true, executing=true, silent_p=true) at ../../src/gdb/remote.c:2589
#8  0x0000555555c19348 in extended_remote_target::attach (this=0x555556bd5990, args=0x555556bf3610 "1", from_tty=1) at ../../src/gdb/remote.c:6187
#9  0x0000555555a2bc5e in attach_command (args=0x555556bf3610 "1", from_tty=1) at ../../src/gdb/infcmd.c:2598
#10 0x00005555557c2420 in do_simple_func (args=0x5555565c0937 "1", from_tty=1, c=0x555556625cf0) at ../../src/gdb/cli/cli-decode.c:95
#11 0x00005555557c7d1e in cmd_func (cmd=0x555556625cf0, args=0x5555565c0937 "1", from_tty=1) at ../../src/gdb/cli/cli-decode.c:2516
#12 0x0000555555d2cf91 in execute_command (p=0x5555565c0937 "1", from_tty=1) at ../../src/gdb/top.c:699

The relevant bit of the remote/gdbreplay log is this:

 c attach 1
 w $vAttach;1#37
 r +$T05#B9
 w +$qC#b4
 r +$#00
 w +

and earlier:

 w +$Hg0#df
 r +$#00

so it all looks like this remote stub doesn't know anything about threads.  There's
no "thread" component in the T05 stop reply, qC is not supported, and not even
Hg is supported.

In remote_add_thread, we see that the ptid of the thread that GDB added to the list is:

 (top-gdb) p ptid
 $4 = {m_pid = 1, m_lwp = 0, m_tid = 0}

This is what later causes the problem -- this is interpreted as a wildcard ptid,
meaning "all threads of process 1".  mi_on_resume looks at the current thread
when we have a wildcard resume.  One immediate thought is that we shouldn't have gotten here
with a wildcard resume, as we've supposedly passed the ptid of a single thread, right
after adding it.  That thread should have an ID != 0, but GDB wasn't able to figure out what
was the ID of the current thread on the remote side, because the remote stub doesn't support
the qC packet.  Even if we taught extended_remote_target::attach to peek the thread ID from
the stop reply that is the response to vAttach, that wouldn't help here, as the stub
doesn't include a thread ID in that response...

Now, mi_on_resume is kind of busted itself, as it is currently kind of serving
a double duty -- it has code that assumes that it is called when infrun resumes
the target (see comments in mi_on_resume_1) which is not the situation here.

A thread ptid like (pid,0,0) is a bit broken, as thread ID == 0 has a special meaning in the
protocol, meaning "any thread".  OTOH, it could be argued that it should be kind of OK
if there truly is only one thread on the remote end, so "any thread" ends up equivalent
to "the single thread".  That's how this used to work, as we have code like this:

static int
remote_thread_always_alive (ptid_t ptid)
{
...
  if (ptid.pid () != 0 && ptid.lwp () == 0)
    /* The main thread is always alive.  This can happen after a
       vAttach, if the remote side doesn't support
       multi-threading.  */
    return 1;

Maybe we should replace that with making extended_remote_target::attach fill in
a fake lwp field (e.g., == 1), and then in the several places we check
for that vAttach special case, we'd check for "fake_tid" flag in the remote_thread_info
object.

Anyhow, I'm not sure exactly what the right fix it, right now.  This needs more thought,
contemplating all the different scenarios and modes.

It would also be interesting to find out what was the change that first broke this.  How is
it that we managed to not hit the assertion until now?

Pedro Alves

> 
> On Sat, Jun 25, 2022 at 11:29 AM Mikaela Szekely
> <mikaela.szekely@qyriad.me> wrote:
>>
>> Are there any thoughts on this? I'm happy to answer any questions if
>> there are any.
>>
>> On Tue, Apr 19, 2022 at 4:01 PM Mikaela Szekely
>> <mikaela.szekely@qyriad.me> wrote:
>>>
>>> Bug PR gdb/28874 reports a failed assertion when attaching to remote
>>> targets. This assertion is checked after a call to
>>> gdb::observers::new_thread.notify from set_running.
>>> The call to set_running in remote_target::remote_add_thread, however,
>>> appears to always be called before any call to switch_to_thread
>>> for remote targets that have set non-stop off.
>>>
>>> This commit proposes a new internal function has_inferior_thread in
>>> gdb/thread.c, which allows remote_target::remote_add_thread to check if
>>> the current thread has been set or not. And thus I have also adjusted
>>> the logic of ::remote_add_thread to call switch_to_thread upon thread
>>> creation if has_inferior_thread returns false.
>>>
>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28874
>>> ---
>>>  gdb/gdbthread.h | 3 +++
>>>  gdb/remote.c    | 6 ++++++
>>>  gdb/thread.c    | 7 +++++++
>>>  3 files changed, 16 insertions(+)
>>>
>>> diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
>>> index 1a33eb61221..bf230410a12 100644
>>> --- a/gdb/gdbthread.h
>>> +++ b/gdb/gdbthread.h
>>> @@ -875,6 +875,9 @@ class scoped_restore_current_thread
>>>    enum language m_lang;
>>>  };
>>>
>>> +/* Returns true if there is a current inferior thread. */
>>> +extern bool has_inferior_thread (void);
>>> +
>>>  /* Returns a pointer into the thread_info corresponding to
>>>     INFERIOR_PTID.  INFERIOR_PTID *must* be in the thread list.  */
>>>  extern struct thread_info* inferior_thread (void);
>>> diff --git a/gdb/remote.c b/gdb/remote.c
>>> index aa6a67a96e0..d35387650b3 100644
>>> --- a/gdb/remote.c
>>> +++ b/gdb/remote.c
>>> @@ -2572,6 +2572,12 @@ remote_target::remote_add_thread (ptid_t ptid, bool running, bool executing,
>>>    else
>>>      thread = add_thread (this, ptid);
>>>
>>> +  /* If there isn't already an active thread, then switch to the
>>> +     thread we just added, so bare-metal targets don't assert the
>>> +     current thread as null. */
>>> +  if (!has_inferior_thread())
>>> +    switch_to_thread(thread);
>>> +
>>>    /* We start by assuming threads are resumed.  That state then gets updated
>>>       when we process a matching stop reply.  */
>>>    get_remote_thread_info (thread)->set_resumed ();
>>> diff --git a/gdb/thread.c b/gdb/thread.c
>>> index 9d0693bd0b8..e7b418b1c85 100644
>>> --- a/gdb/thread.c
>>> +++ b/gdb/thread.c
>>> @@ -79,6 +79,13 @@ is_current_thread (const thread_info *thr)
>>>    return thr == current_thread_;
>>>  }
>>>
>>> +
>>> +bool
>>> +has_inferior_thread (void)
>>> +{
>>> +  return current_thread_ != nullptr;
>>> +}
>>> +
>>>  struct thread_info*
>>>  inferior_thread (void)
>>>  {
>>> --
>>> 2.35.1
>>>