From: Andrew Burgess <aburgess@redhat.com>
To: "Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com>,
Pedro Alves <pedro@palves.net>,
"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Subject: RE: [PATCH 2/2] gdbserver: track current process as well as current thread
Date: Thu, 22 Jun 2023 18:49:56 +0100 [thread overview]
Message-ID: <87o7l7s7y3.fsf@redhat.com> (raw)
In-Reply-To: <DM4PR11MB7303455DC975982F20623E01C4659@DM4PR11MB7303.namprd11.prod.outlook.com>
"Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com> writes:
> On Tuesday, April 25, 2023 3:57 PM, Andrew Burgess wrote:
>> 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
>
> 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
next prev parent reply other threads:[~2023-06-22 17:50 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
2023-04-26 6:35 ` Aktemur, Tankut Baris
2023-06-19 16:46 ` Aktemur, Tankut Baris
2023-06-22 17:49 ` Andrew Burgess [this message]
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=87o7l7s7y3.fsf@redhat.com \
--to=aburgess@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=pedro@palves.net \
--cc=tankut.baris.aktemur@intel.com \
/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).