From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f47.google.com (mail-wr1-f47.google.com [209.85.221.47]) by sourceware.org (Postfix) with ESMTPS id 1A1C73858D39 for ; Mon, 18 Jul 2022 22:23:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1A1C73858D39 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wr1-f47.google.com with SMTP id bu1so19027872wrb.9 for ; Mon, 18 Jul 2022 15:23:28 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=i5NkGW4x64nEZRXGeIUcwwgno+qJWSwRcRH9MrUB85I=; b=maD3KM7vXi0SZc3rmRNPvIiehxKNag6zjeW8EoGpHZ8anayM8aK+pi2cBw2tYx9dLp 5YpvKFmWvBwUu2ralnZ2e6Y1uQskHEgbPl5o5iSef8FNU8xZPYyUIl9GWVOvxmSOlRnm GP9D/pezY5OTFnt+L1acmJSmskeNNEP2NTzvys+2aonzKe+m5zMWjx3fV6qmCx7C7yV6 csWBcVaDTHGBY1n/VB4nryKs/28ZqzlQ7dgRRadIIF7osVdUFQniZiqorawXTlFFk1hy XrYwdo7ct2hnGJqlUD2NaD7WTamaqbv8hA9lfkTTDY7BCT5T8x17zhJMbsUiF6NHAtef mGmQ== X-Gm-Message-State: AJIora+t0DOcwTgkByvVqe/SJ8SlsALh8eEsY4BjoRH8Fi3E4U7k7rFR 3uFTjS2esNWi+a1IGEHGwsavd58yctU= X-Google-Smtp-Source: AGRyM1uqZTACHeUdBtALREh/eKYhOc7uMfK8FEiTc6lywnwWpJrAMM7wStVK018aHUqKiVdjnPbH9w== X-Received: by 2002:a5d:4e8f:0:b0:21e:2b0c:ba6c with SMTP id e15-20020a5d4e8f000000b0021e2b0cba6cmr1544668wru.305.1658183005711; Mon, 18 Jul 2022 15:23:25 -0700 (PDT) Received: from ?IPv6:2001:8a0:f924:2600:209d:85e2:409e:8726? ([2001:8a0:f924:2600:209d:85e2:409e:8726]) by smtp.gmail.com with ESMTPSA id d13-20020adfe84d000000b0020fcaba73bcsm11679697wrn.104.2022.07.18.15.23.24 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 18 Jul 2022 15:23:24 -0700 (PDT) Subject: Re: [PATCH][PR gdb/28874] gdb/remote: switch to added threads if there isn't one already To: Mikaela Szekely , gdb-patches@sourceware.org References: <20220419220135.1714665-1-mikaela.szekely@qyriad.me> From: Pedro Alves Message-ID: <51575f88-677b-49b5-6a26-d95e123e0a77@palves.net> Date: Mon, 18 Jul 2022 23:23:23 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.2 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP, WEIRD_PORT autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Jul 2022 22:23:31 -0000 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 , 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::_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::operator()(ptid_t) const (this=0x5555565f20c8, __args#0=...) at /usr/include/c++/9/bits/std_function.h:688 #5 0x0000555555d178d8 in gdb::observers::observable::notify (this=0x555556551620 , 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 > 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 >> 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 >>>