From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 9765C3858D35 for ; Thu, 22 Jun 2023 17:50:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9765C3858D35 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1687456203; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=dEGMw1z+Sf4UBDzXMzstRjAYwxFZFeKFE4RAqI3x4W0=; b=NFF19DEqs/Zq63bGeG1bNqrhH2CslBdSSEx/e/hbAvBGpRcUcJ/4E86eBtZl++tZ/7oin5 LWRf2uFjYVnGNVXxbrrPWIzFEvs6/DFSqKMG7Jgmp3UrqRJNZ556crlUgtDhDpXXrBl+Qe cVYiR7tdqy+kJV75DyT+pfVBB4z7oho= Received: from mail-ej1-f70.google.com (mail-ej1-f70.google.com [209.85.218.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-589-lWHzI5-6MxKoMCKPvnCWAg-1; Thu, 22 Jun 2023 13:50:01 -0400 X-MC-Unique: lWHzI5-6MxKoMCKPvnCWAg-1 Received: by mail-ej1-f70.google.com with SMTP id a640c23a62f3a-94a355c9028so508259166b.3 for ; Thu, 22 Jun 2023 10:50:01 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687456200; x=1690048200; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=dEGMw1z+Sf4UBDzXMzstRjAYwxFZFeKFE4RAqI3x4W0=; b=Wcx7UUuW5C88jkSEqmJA3sYCxVZt/wcs9X9gcfBw5a52+dmQi53cS0kZ/G2cY4Jggt b4uEgWdOEMS77Nkkev/YIvz4cxcumpJTHKvKMVIq+BTEtA67mX8SbryKhELMDU9AV6a7 w3r+jfsLyaZgT+iYZxSJ1Q/SHyyEM/COpzvh7Ut+TmQxQdnVRcgPjMqttUH/hQQnz2i8 IdeFNoJ4dxg6cnQ/ODNt21cXhB1eRKxyj9jsVweyKxJer/TjpGbQHp5QZDnLbp3EzlLd ynNUpswKJTrUIHjPQKG9u8/T9w0qk+Yw01kUMxlWaYQVZKGEju/7p0N01X/0JNB1LoXa IvDg== X-Gm-Message-State: AC+VfDwrGV2wU1OQxk05ro4JyXc9UT6RmrC0sw9nPX8/a3ySPzHV+f/w KDNl9u0dd2az5Pud7C+hPphcAqo7MV73LfUvDXCjIfEXPvAJruXgWEgusF6qMDx/OYoFqiDt4mj kC+CUZzUYlG7x4X+n6G7SAB6R0AXSfw== X-Received: by 2002:a17:907:7b96:b0:973:9f60:c57e with SMTP id ne22-20020a1709077b9600b009739f60c57emr19866438ejc.2.1687456200430; Thu, 22 Jun 2023 10:50:00 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ4iT9nTDamxW/PxjUR6+ic6KGmGIVy7FIyASLzNWwMNMpOamiONmBmR/OcTdBAFNiYPVOv1Gw== X-Received: by 2002:a17:907:7b96:b0:973:9f60:c57e with SMTP id ne22-20020a1709077b9600b009739f60c57emr19866431ejc.2.1687456200120; Thu, 22 Jun 2023 10:50:00 -0700 (PDT) Received: from localhost (92.40.219.203.threembb.co.uk. [92.40.219.203]) by smtp.gmail.com with ESMTPSA id n20-20020a170906725400b009878b0b5302sm4980199ejk.98.2023.06.22.10.49.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 22 Jun 2023 10:49:58 -0700 (PDT) From: Andrew Burgess To: "Aktemur, Tankut Baris" , Pedro Alves , "gdb-patches@sourceware.org" Subject: RE: [PATCH 2/2] gdbserver: track current process as well as current thread In-Reply-To: References: <20220419224739.3029868-1-pedro@palves.net> <20220419224739.3029868-3-pedro@palves.net> <87v8hkawun.fsf@redhat.com> Date: Thu, 22 Jun 2023 18:49:56 +0100 Message-ID: <87o7l7s7y3.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-10.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,RCVD_IN_SBL_CSS,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: "Aktemur, Tankut Baris" writes: > On Tuesday, April 25, 2023 3:57 PM, Andrew Burgess wrote: >> Pedro Alves writes: >> >> > The recent commit 421490af33bf ("gdbserver/linux: Access memory even >> > if threads are running") caused a regression in >> > gdb.threads/access-mem-running-thread-exit.exp with gdbserver, which I >> > somehow missed. Like so: >> > >> > (gdb) print global_var >> > Cannot access memory at address 0x555555558010 >> > (gdb) FAIL: gdb.threads/access-mem-running-thread-exit.exp: non-stop: access mem (print >> global_var after writing, inf=2, iter=1) >> > >> > The problem starts with GDB telling GDBserver to select a thread, via >> > the Hg packet, which GDBserver complies with, then that thread exits, >> > and GDB, without knowing the thread is gone, tries to write to memory, >> > through the context of the previously selected Hg thread. >> > >> > GDBserver's GDB-facing memory access routines, gdb_read_memory and >> > gdb_write_memory, call set_desired_thread to make GDBserver re-select >> > the thread that GDB has selected with the Hg packet. Since the thread >> > is gone, set_desired_thread returns false, and the memory access >> > fails. >> > >> > Now, to access memory, it doesn't really matter which thread is >> > selected. All we should need is the target process. Even if the >> > thread that GDB previously selected is gone, and GDB does not yet know >> > about that exit, it shouldn't matter, GDBserver should still know >> > which process that thread belonged to. >> > >> > Fix this by making GDBserver track the current process separately, >> > like GDB also does. Add a new set_desired_process routine that is >> > similar to set_desired_thread, but just sets the current process, >> > leaving the current thread as NULL. Use it in the GDB-facing memory >> > read and write routines, to avoid failing if the selected thread is >> > gone, but the process is still around. >> > >> > Change-Id: I4ff97cb6f42558efbed224b30d5c71f6112d44cd >> > --- >> > gdbserver/gdbthread.h | 1 + >> > gdbserver/inferiors.cc | 26 +++++++++++++++++++------ >> > gdbserver/server.cc | 4 ++-- >> > gdbserver/target.cc | 44 ++++++++++++++++++++++++++++++++++++++++-- >> > gdbserver/target.h | 15 +++++++++++++- >> > 5 files changed, 79 insertions(+), 11 deletions(-) >> > >> > diff --git a/gdbserver/gdbthread.h b/gdbserver/gdbthread.h >> > index 10ae1cb7c87..8b897e73d33 100644 >> > --- a/gdbserver/gdbthread.h >> > +++ b/gdbserver/gdbthread.h >> > @@ -252,6 +252,7 @@ class scoped_restore_current_thread >> > >> > private: >> > bool m_dont_restore = false; >> > + process_info *m_process; >> > thread_info *m_thread; >> > }; >> > >> > diff --git a/gdbserver/inferiors.cc b/gdbserver/inferiors.cc >> > index 678d93c77a1..3d0a8b0e716 100644 >> > --- a/gdbserver/inferiors.cc >> > +++ b/gdbserver/inferiors.cc >> > @@ -26,6 +26,11 @@ >> > std::list all_processes; >> > std::list all_threads; >> > >> > +/* The current process. */ >> > +static process_info *current_process_; >> > + >> > +/* The current thread. This is either a thread of CURRENT_PROCESS, or >> > + NULL. */ >> > struct thread_info *current_thread; >> > >> > /* The current working directory used to start the inferior. >> > @@ -130,6 +135,7 @@ clear_inferiors (void) >> > clear_dlls (); >> > >> > switch_to_thread (nullptr); >> > + current_process_ = nullptr; >> > } >> > >> > struct process_info * >> > @@ -153,6 +159,8 @@ remove_process (struct process_info *process) >> > free_all_breakpoints (process); >> > gdb_assert (find_thread_process (process) == NULL); >> > all_processes.remove (process); >> > + if (current_process () == process) >> > + switch_to_process (nullptr); >> > delete process; >> > } >> > >> > @@ -205,8 +213,7 @@ get_thread_process (const struct thread_info *thread) >> > struct process_info * >> > current_process (void) >> > { >> > - gdb_assert (current_thread != NULL); >> > - return get_thread_process (current_thread); >> > + return current_process_; >> >> A bit late I know, but I wonder if the assert, or something similar, >> should have been retained here? >> >> The comment on this function currently (though Baris has a patch >> proposed to change this), says this function should only be called in a >> context where the result will not be nullptr. Given that, not of the >> (many) existing uses check the return value of this function against >> nullptr. >> >> Happy to roll a patch to add the assert back, just wondered if there was >> a reason for its removal. >> >> Thanks, >> Andrew > > Hi Andrew, > > I think the current process can in fact be null in some brief periods. > For instance, in 'handle_detach' there is > > if (extended_protocol || target_running ()) > { > /* There is still at least one inferior remaining or > we are in extended mode, so don't terminate gdbserver, > and instead treat this like a normal program exit. */ > cs.last_status.set_exited (0); > cs.last_ptid = ptid_t (pid); > > switch_to_thread (nullptr); > } > > So, if the current process exits, gdbserver reports the event to GDB and > sets the current thread to nullptr, which also sets the current process > to nullptr. > > I believe an invariant is this: > > (current_thread == nullptr) || (current_process () == get_thread_process (current_thread)) > I don't think this really addresses my question. Here's how I understand things: 1. Before this patch: a. Comment on 'current_process' says: it's an error to call this function when no thread is selected, and b. The function asserted that a process was selected, 2. This patch removed the assert, but left the comment unchanged, 3. A patch was proposed to updated the comment, 4. I couldn't see any reason in this patch _why_ the assert was removed. I agree that the process _could_ be nullptr, but it _could_ have been nullptr before. My question is: did something change such that there is now a location where we might choose to call current_process when no thread is selected? Given the description of the original patch my guess was no, but maybe I should just add the assert back locally and do a test run to find out? Thanks, Andrew