RE: [PATCH 2/2] gdbserver: track current process as well as current thread

["Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com>]

On Thursday, June 22, 2023 7:50 PM, Andrew Burgess wrote:
> "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?

With Pedro's patch above, before reading/writing memory upon GDB's request,
gdbserver does a `switch_to_process`.  This sets current_thread to nullptr
while setting current_process_ to non-null.  At that moment, calling
current_process would have violated the assertion.

-Baris

> 
> 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


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928