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

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

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

Thanks
-Baris


> >  }
> >
> >  /* 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

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