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.133.124]) by sourceware.org (Postfix) with ESMTPS id 654633858C2D for ; Tue, 25 Apr 2023 13:57:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 654633858C2D 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=1682431029; 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=/Z9q6vWI1fwW++GkAL/dWI4SJAuMgDJipg6uFjjjSso=; b=Q8kZYaaRqCBNkYF8AyguK6mSopS+Pf6lgnZeESMBwp++AOhSsz8EGW2eDgZIdw2t4ik3Hc DKzyB9uuuMniSn1ZUUUjs11EvxJy/Kjq2ZM2TMmyIJA2UFyB4r2bWWtTIsnhMK2fKa7Kpv jq1kEl2RS07Tn2a415IPMcea8rhPLew= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-182-R8AqOkkdMiyyUO8RoLIN3A-1; Tue, 25 Apr 2023 09:57:07 -0400 X-MC-Unique: R8AqOkkdMiyyUO8RoLIN3A-1 Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-3f080f534acso36469515e9.0 for ; Tue, 25 Apr 2023 06:57:07 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1682431026; x=1685023026; 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=/Z9q6vWI1fwW++GkAL/dWI4SJAuMgDJipg6uFjjjSso=; b=TlwPxXx1aF0PyF1JfQlnYyYsVaAUlz/THwwQGWUIleir9bLTMS+BKYzjOTUxjKZ10q IhFs8r/t0b0gVE8ye7nE+yVU2KuJo4D+dFuKDlW8nOK2rVZPK4+l8+XWC+Sf6zs+ckk8 0gavAZyCV4mJSw4OxUv+M8AxccWcz9tDdKV3uVpAGDi4G2Dx5oDmSWE5AL4dJC9DLHog RlBY7oHHTg+TXSF/iKB8scBSOjuKa9aKtzgsdKgyASBVhyya53xkidUVEzg86Vf5YJPp D+6tHdVHCyFXYNd6A2anDT10Di7S9JdagnpabhTlQaVMHwvIOdQoyt111D5t87YnTv8v uqrQ== X-Gm-Message-State: AAQBX9ezgh5Wx/0g7MXfj+4DyU8kE8w/vxCPYR0dG0dIzDLLVQrw8lhU +qY4K76WuaKbX8QWAlS06PNsT4VJibXU+4BimTOg96L6uhZ5m1PE8Q4IzT0nUEo5EeH0+m+5dYa 32RNeee9tD1Nf/DSeSCvDcG6gxAsrvA== X-Received: by 2002:a5d:58cd:0:b0:304:4031:acb0 with SMTP id o13-20020a5d58cd000000b003044031acb0mr9070775wrf.52.1682431026044; Tue, 25 Apr 2023 06:57:06 -0700 (PDT) X-Google-Smtp-Source: AKy350YtmnI5dN8ROWX3ov30OYUsyiZv2QF109M04RRDDCkjyGKZhLVW5JXd4pxO54jEFHd3rBhJNA== X-Received: by 2002:a5d:58cd:0:b0:304:4031:acb0 with SMTP id o13-20020a5d58cd000000b003044031acb0mr9070767wrf.52.1682431025593; Tue, 25 Apr 2023 06:57:05 -0700 (PDT) Received: from localhost (11.72.115.87.dyn.plus.net. [87.115.72.11]) by smtp.gmail.com with ESMTPSA id v11-20020a5d43cb000000b002ff77b033b1sm13209969wrr.33.2023.04.25.06.57.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 25 Apr 2023 06:57:05 -0700 (PDT) From: Andrew Burgess To: Pedro Alves , gdb-patches@sourceware.org Subject: Re: [PATCH 2/2] gdbserver: track current process as well as current thread In-Reply-To: <20220419224739.3029868-3-pedro@palves.net> References: <20220419224739.3029868-1-pedro@palves.net> <20220419224739.3029868-3-pedro@palves.net> Date: Tue, 25 Apr 2023 14:57:04 +0100 Message-ID: <87v8hkawun.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=-11.6 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_H2,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: 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 > } > > /* See gdbsupport/common-gdbthread.h. */ > @@ -223,6 +230,10 @@ switch_to_thread (process_stratum_target *ops, ptid_t ptid) > void > switch_to_thread (thread_info *thread) > { > + if (thread != nullptr) > + current_process_ = get_thread_process (thread); > + else > + current_process_ = nullptr; > current_thread = thread; > } > > @@ -231,9 +242,8 @@ switch_to_thread (thread_info *thread) > void > switch_to_process (process_info *proc) > { > - int pid = pid_of (proc); > - > - switch_to_thread (find_any_thread_of_pid (pid)); > + current_process_ = proc; > + current_thread = nullptr; > } > > /* See gdbsupport/common-inferior.h. */ > @@ -254,6 +264,7 @@ set_inferior_cwd (std::string cwd) > > scoped_restore_current_thread::scoped_restore_current_thread () > { > + m_process = current_process_; > m_thread = current_thread; > } > > @@ -262,5 +273,8 @@ scoped_restore_current_thread::~scoped_restore_current_thread () > if (m_dont_restore) > return; > > - switch_to_thread (m_thread); > + if (m_thread != nullptr) > + switch_to_thread (m_thread); > + else > + switch_to_process (m_process); > } > diff --git a/gdbserver/server.cc b/gdbserver/server.cc > index 33c42714e72..f9c02a9c6da 100644 > --- a/gdbserver/server.cc > +++ b/gdbserver/server.cc > @@ -1067,7 +1067,7 @@ gdb_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len) > /* (assume no half-trace half-real blocks for now) */ > } > > - if (set_desired_thread ()) > + if (set_desired_process ()) > res = read_inferior_memory (memaddr, myaddr, len); > else > res = 1; > @@ -1088,7 +1088,7 @@ gdb_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len) > { > int ret; > > - if (set_desired_thread ()) > + if (set_desired_process ()) > ret = target_write_memory (memaddr, myaddr, len); > else > ret = EIO; > diff --git a/gdbserver/target.cc b/gdbserver/target.cc > index 883242377c0..adcfe6e7bcc 100644 > --- a/gdbserver/target.cc > +++ b/gdbserver/target.cc > @@ -29,16 +29,56 @@ > > process_stratum_target *the_target; > > -int > +/* See target.h. */ > + > +bool > set_desired_thread () > { > client_state &cs = get_client_state (); > thread_info *found = find_thread_ptid (cs.general_thread); > > - switch_to_thread (found); > + if (found == nullptr) > + { > + process_info *proc = find_process_pid (cs.general_thread.pid ()); > + if (proc == nullptr) > + { > + threads_debug_printf > + ("did not find thread nor process for general_thread %s", > + cs.general_thread.to_string ().c_str ()); > + } > + else > + { > + threads_debug_printf > + ("did not find thread for general_thread %s, but found process", > + cs.general_thread.to_string ().c_str ()); > + } > + switch_to_process (proc); > + } > + else > + switch_to_thread (found); > + > return (current_thread != NULL); > } > > +/* See target.h. */ > + > +bool > +set_desired_process () > +{ > + client_state &cs = get_client_state (); > + > + process_info *proc = find_process_pid (cs.general_thread.pid ()); > + if (proc == nullptr) > + { > + threads_debug_printf > + ("did not find process for general_thread %s", > + cs.general_thread.to_string ().c_str ()); > + } > + switch_to_process (proc); > + > + return proc != nullptr; > +} > + > int > read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len) > { > diff --git a/gdbserver/target.h b/gdbserver/target.h > index f3172e2ed7e..6c536a30778 100644 > --- a/gdbserver/target.h > +++ b/gdbserver/target.h > @@ -699,7 +699,20 @@ target_thread_pending_child (thread_info *thread) > > int read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len); > > -int set_desired_thread (); > +/* Set GDBserver's current thread to the thread the client requested > + via Hg. Also switches the current process to the requested > + process. If the requested thread is not found in the thread list, > + then the current thread is set to NULL. Likewise, if the requested > + process is not found in the process list, then the current process > + is set to NULL. Returns true if the requested thread was found, > + false otherwise. */ > + > +bool set_desired_thread (); > + > +/* Set GDBserver's current process to the process the client requested > + via Hg. The current thread is set to NULL. */ > + > +bool set_desired_process (); > > std::string target_pid_to_str (ptid_t); > > -- > 2.26.2