From: Andrew Burgess <aburgess@redhat.com>
To: Pedro Alves <pedro@palves.net>, gdb-patches@sourceware.org
Subject: Re: [PATCH 2/2] gdbserver: track current process as well as current thread
Date: Tue, 25 Apr 2023 14:57:04 +0100 [thread overview]
Message-ID: <87v8hkawun.fsf@redhat.com> (raw)
In-Reply-To: <20220419224739.3029868-3-pedro@palves.net>
Pedro Alves <pedro@palves.net> 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<process_info *> all_processes;
> std::list<thread_info *> 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
next prev parent reply other threads:[~2023-04-25 13:57 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-19 22:47 [PATCH 0/2] Fix gdbserver/linux memory access regression Pedro Alves
2022-04-19 22:47 ` [PATCH 1/2] Fix gdb.threads/access-mem-running-thread-exit.exp w/ native-extended-gdbserver Pedro Alves
2022-04-19 22:47 ` [PATCH 2/2] gdbserver: track current process as well as current thread Pedro Alves
2023-04-25 13:57 ` Andrew Burgess [this message]
2023-04-26 6:35 ` Aktemur, Tankut Baris
2023-06-19 16:46 ` Aktemur, Tankut Baris
2023-06-22 17:49 ` Andrew Burgess
2023-06-28 8:39 ` Aktemur, Tankut Baris
2022-05-03 14:24 ` [PATCH 0/2] Fix gdbserver/linux memory access regression Pedro Alves
2022-05-04 9:11 ` Luis Machado
2022-05-04 9:42 ` Luis Machado
2022-05-04 9:45 ` Pedro Alves
2022-05-04 9:52 ` Luis Machado
2022-05-04 10:14 ` Pedro Alves
2022-05-04 13:44 ` Pedro Alves
2022-05-04 14:03 ` Luis Machado
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87v8hkawun.fsf@redhat.com \
--to=aburgess@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=pedro@palves.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).