Re: [PATCH v2 3/4] gdb/progspace: add solib_ops pointer in program_space

[Guinevere Larsen <guinevere@redhat.com>]

On 6/9/25 4:40 PM, Simon Marchi wrote:
> New in v2: remove commented out code in target.c and spurious change in
> gdb.mi/mi-detach.exp.
>
> The subsequent C++ification patch in this series will allocate one
> instance of solib_ops per program space.  That instance will be held in
> struct program_space.  As a small step towards this, add an `solib_ops
> *` field to `struct program_space`.  This field represents the solib_ops
> currently used to manage the solibs in that program space.  Initialize
> it with the result of `gdbarch_so_ops` in `post_create_inferior`, and
> use it whenever we need to do some solib stuff, rather than using
> `gdbarch_so_ops` directly.
>
> The difficulty here is knowing when exactly to set and unset the solib
> ops.  What I have here passes the testsuite on Linux, but with more
> testing we will probably discover more spots where it's needed.
>
> The C++ification patch will turn this field into a unique pointer.
>
> Change-Id: Ide8ddc57328895720fcd645d46dc34491f84c656
> ---
>   gdb/corelow.c         |  2 +-
>   gdb/infcmd.c          | 15 ++++++++-------
>   gdb/inferior.h        |  9 ++++++++-
>   gdb/infrun.c          | 11 +++++++++--
>   gdb/progspace.h       | 20 ++++++++++++++++++++
>   gdb/solib.c           | 44 +++++++++++++++++++++++--------------------
>   gdb/target.c          |  1 +
>   gdb/tracectf.c        |  2 +-
>   gdb/tracefile-tfile.c |  2 +-
>   9 files changed, 73 insertions(+), 33 deletions(-)
>
> diff --git a/gdb/corelow.c b/gdb/corelow.c
> index b5895de9e9c6..d92585c67bfd 100644
> --- a/gdb/corelow.c
> +++ b/gdb/corelow.c
> @@ -1180,7 +1180,7 @@ core_target_open (const char *arg, int from_tty)
>     if (current_program_space->exec_bfd () == nullptr)
>       set_gdbarch_from_file (current_program_space->core_bfd ());
>   
> -  post_create_inferior (from_tty);
> +  post_create_inferior (from_tty, true);
>   
>     /* Now go through the target stack looking for threads since there
>        may be a thread_stratum target loaded on top of target core by
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index e9b58ce55210..1adad5c3eaa3 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -224,14 +224,11 @@ strip_bg_char (const char *args, int *bg_char_p)
>     return make_unique_xstrdup (args);
>   }
>   
> -/* Common actions to take after creating any sort of inferior, by any
> -   means (running, attaching, connecting, et cetera).  The target
> -   should be stopped.  */
> +/* See inferior.h.  */
>   
>   void
> -post_create_inferior (int from_tty)
> +post_create_inferior (int from_tty, bool set_pspace_solib_ops)
>   {
> -
>     /* Be sure we own the terminal in case write operations are performed.  */
>     target_terminal::ours_for_output ();
>   
> @@ -261,6 +258,10 @@ post_create_inferior (int from_tty)
>   	throw;
>       }
>   
> +  if (set_pspace_solib_ops)
> +    current_program_space->set_solib_ops
> +      (*gdbarch_so_ops (current_inferior ()->arch ()));
> +
>     if (current_program_space->exec_bfd ())
>       {
>         const unsigned solib_add_generation
> @@ -482,7 +483,7 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how)
>   
>     /* Pass zero for FROM_TTY, because at this point the "run" command
>        has done its thing; now we are setting up the running program.  */
> -  post_create_inferior (0);
> +  post_create_inferior (0, true);
>   
>     /* Queue a pending event so that the program stops immediately.  */
>     if (run_how == RUN_STOP_AT_FIRST_INSN)
> @@ -2506,7 +2507,7 @@ setup_inferior (int from_tty)
>     /* Take any necessary post-attaching actions for this platform.  */
>     target_post_attach (inferior_ptid.pid ());
>   
> -  post_create_inferior (from_tty);
> +  post_create_inferior (from_tty, true);
>   }
>   
>   /* What to do after the first program stops after attaching.  */
> diff --git a/gdb/inferior.h b/gdb/inferior.h
> index 54f5229b420f..fe94e289bdc7 100644
> --- a/gdb/inferior.h
> +++ b/gdb/inferior.h
> @@ -213,7 +213,14 @@ extern ptid_t gdb_startup_inferior (pid_t pid, int num_traps);
>   
>   extern void setup_inferior (int from_tty);
>   
> -extern void post_create_inferior (int from_tty);
> +/* Common actions to take after creating any sort of inferior, by any
> +   means (running, attaching, connecting, et cetera).  The target
> +   should be stopped.
> +
> +   If SET_PSPACE_SOLIB_OPS is true, initialize the program space's solib
> +   provider using the current inferior's architecture.  */
> +
> +extern void post_create_inferior (int from_tty, bool set_pspace_solib_ops);
>   
>   extern void attach_command (const char *, int);
>   
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 2e02642c52a4..5cdf66d26dcb 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -472,6 +472,7 @@ holding the child stopped.  Try \"set %ps\" or \"%ps\".\n"),
>   
>     inferior *parent_inf = current_inferior ();
>     inferior *child_inf = nullptr;
> +  bool child_has_new_pspace = false;
>   
>     gdb_assert (parent_inf->thread_waiting_for_vfork_done == nullptr);
>   
> @@ -536,6 +537,7 @@ holding the child stopped.  Try \"set %ps\" or \"%ps\".\n"),
>   	  else
>   	    {
>   	      child_inf->pspace = new program_space (new_address_space ());
> +	      child_has_new_pspace = true;
>   	      child_inf->aspace = child_inf->pspace->aspace;
>   	      child_inf->removable = true;
>   	      clone_program_space (child_inf->pspace, parent_inf->pspace);
> @@ -625,6 +627,7 @@ holding the child stopped.  Try \"set %ps\" or \"%ps\".\n"),
>         else
>   	{
>   	  child_inf->pspace = new program_space (new_address_space ());
> +	  child_has_new_pspace = true;
>   	  child_inf->aspace = child_inf->pspace->aspace;
>   	  child_inf->removable = true;
>   	  child_inf->symfile_flags = SYMFILE_NO_READ;
> @@ -723,7 +726,8 @@ holding the child stopped.  Try \"set %ps\" or \"%ps\".\n"),
>   	maybe_restore.emplace ();
>   
>         switch_to_thread (*child_inf->threads ().begin ());
> -      post_create_inferior (0);
> +
> +      post_create_inferior (0, child_has_new_pspace);
>       }
>   
>     return false;
> @@ -1321,6 +1325,7 @@ follow_exec (ptid_t ptid, const char *exec_file_target)
>        we don't want those to be satisfied by the libraries of the
>        previous incarnation of this process.  */
>     no_shared_libraries (current_program_space);
> +  current_program_space->unset_solib_ops ();
>   
>     inferior *execing_inferior = current_inferior ();
>     inferior *following_inferior;
> @@ -1377,6 +1382,8 @@ follow_exec (ptid_t ptid, const char *exec_file_target)
>        registers.  */
>     target_find_description ();
>   
> +  current_program_space->set_solib_ops
> +    (*gdbarch_so_ops (following_inferior->arch ()));
>     gdb::observers::inferior_execd.notify (execing_inferior, following_inferior);
>   
>     breakpoint_re_set ();
> @@ -3818,7 +3825,7 @@ start_remote (int from_tty)
>     /* Now that the inferior has stopped, do any bookkeeping like
>        loading shared libraries.  We want to do this before normal_stop,
>        so that the displayed frame is up to date.  */
> -  post_create_inferior (from_tty);
> +  post_create_inferior (from_tty, true);
>   
>     normal_stop ();
>   }
> diff --git a/gdb/progspace.h b/gdb/progspace.h
> index 5e5d5edd9b4b..abb448195d74 100644
> --- a/gdb/progspace.h
> +++ b/gdb/progspace.h
> @@ -231,6 +231,23 @@ struct program_space
>        is outside all objfiles in this progspace.  */
>     struct objfile *objfile_for_address (CORE_ADDR address);
>   
> +  /* Set this program space's solib provider.
> +
> +     The solib provider must be unset prior to call this method.  */
> +  void set_solib_ops (const struct solib_ops &ops)
> +  {
> +    gdb_assert (m_solib_ops == nullptr);
> +    m_solib_ops = &ops;
> +  };
> +
> +  /* Unset this program space's solib provider.  */
> +  void unset_solib_ops ()
> +  { m_solib_ops = nullptr; }
> +
> +  /* Get this program space's solib provider.  */
> +  const struct solib_ops *solib_ops () const
> +  { return m_solib_ops; }
> +
>     /* Return the list of all the solibs in this program space.  */
>     owning_intrusive_list<solib> &solibs ()
>     { return m_solib_list; }
> @@ -355,6 +372,9 @@ struct program_space
>     /* All known objfiles are kept in a linked list.  */
>     owning_intrusive_list<objfile> m_objfiles_list;
>   
> +  /* solib_ops implementation used to provide solibs in this program space.  */
> +  const struct solib_ops *m_solib_ops = nullptr;
> +
>     /* List of shared objects mapped into this space.  Managed by
>        solib.c.  */
>     owning_intrusive_list<solib> m_solib_list;
> diff --git a/gdb/solib.c b/gdb/solib.c
> index de3469da6e91..dceab2b865bf 100644
> --- a/gdb/solib.c
> +++ b/gdb/solib.c
> @@ -707,7 +707,10 @@ notify_solib_unloaded (program_space *pspace, const solib &so,
>   void
>   update_solib_list (int from_tty)
>   {
> -  const solib_ops *ops = gdbarch_so_ops (current_inferior ()->arch ());
> +  const solib_ops *ops = current_program_space->solib_ops ();
> +
> +  if (ops == nullptr)
> +    return;
>   
>     /* We can reach here due to changing solib-search-path or the
>        sysroot, before having any inferior.  */
> @@ -1021,7 +1024,7 @@ print_solib_list_table (std::vector<const solib *> solib_list,
>     gdbarch *gdbarch = current_inferior ()->arch ();
>     /* "0x", a little whitespace, and two hex digits per byte of pointers.  */
>     int addr_width = 4 + (gdbarch_ptr_bit (gdbarch) / 4);
> -  const solib_ops *ops = gdbarch_so_ops (gdbarch);
> +  const solib_ops *ops = current_program_space->solib_ops ();
>     struct ui_out *uiout = current_uiout;
>     bool so_missing_debug_info = false;
>   
> @@ -1155,7 +1158,8 @@ info_sharedlibrary_command (const char *pattern, int from_tty)
>   static void
>   info_linker_namespace_command (const char *pattern, int from_tty)
>   {
> -  const solib_ops *ops = gdbarch_so_ops (current_inferior ()->arch ());
> +  const solib_ops *ops = current_program_space->solib_ops ();
> +
>     /* This command only really makes sense for inferiors that support
>        linker namespaces, so we can leave early.  */
>     if (ops->num_active_namespaces == nullptr)
> @@ -1273,9 +1277,9 @@ solib_name_from_address (struct program_space *pspace, CORE_ADDR address)
>   bool
>   solib_keep_data_in_core (CORE_ADDR vaddr, unsigned long size)
>   {
> -  const solib_ops *ops = gdbarch_so_ops (current_inferior ()->arch ());
> +  const solib_ops *ops = current_program_space->solib_ops ();
>   
> -  if (ops->keep_data_in_core)
> +  if (ops != nullptr && ops->keep_data_in_core != nullptr)

Can this function be called before the program space is fully setup?

Otherwise I think we should either assume that it was set correctly, or 
assert it. Better to be loud and easy to find the bug than accidentally 
adding a regression in a hard-to-spot place.

This goes to all other similar places where you check for ops != nullptr.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

>       return ops->keep_data_in_core (vaddr, size) != 0;
>     else
>       return false;
> @@ -1286,8 +1290,6 @@ solib_keep_data_in_core (CORE_ADDR vaddr, unsigned long size)
>   void
>   clear_solib (program_space *pspace)
>   {
> -  const solib_ops *ops = gdbarch_so_ops (current_inferior ()->arch ());
> -
>     for (solib &so : pspace->solibs ())
>       {
>         bool still_in_use
> @@ -1299,7 +1301,9 @@ clear_solib (program_space *pspace)
>   
>     pspace->solibs ().clear ();
>   
> -  if (ops->clear_solib != nullptr)
> +  const solib_ops *ops = pspace->solib_ops ();
> +
> +  if (ops != nullptr && ops->clear_solib != nullptr)
>       ops->clear_solib (pspace);
>   }
>   
> @@ -1311,9 +1315,9 @@ clear_solib (program_space *pspace)
>   void
>   solib_create_inferior_hook (int from_tty)
>   {
> -  const solib_ops *ops = gdbarch_so_ops (current_inferior ()->arch ());
> +  const solib_ops *ops = current_program_space->solib_ops ();
>   
> -  if (ops->solib_create_inferior_hook != nullptr)
> +  if (ops != nullptr && ops->solib_create_inferior_hook != nullptr)
>       ops->solib_create_inferior_hook (from_tty);
>   }
>   
> @@ -1322,10 +1326,10 @@ solib_create_inferior_hook (int from_tty)
>   bool
>   in_solib_dynsym_resolve_code (CORE_ADDR pc)
>   {
> -  const auto in_dynsym_resolve_code
> -    = gdbarch_so_ops (current_inferior ()->arch ())->in_dynsym_resolve_code;
> +  const solib_ops *ops = current_program_space->solib_ops ();
>   
> -  return in_dynsym_resolve_code && in_dynsym_resolve_code (pc);
> +  return (ops != nullptr && ops->in_dynsym_resolve_code != nullptr
> +	  && ops->in_dynsym_resolve_code (pc));
>   }
>   
>   /* Implements the "sharedlibrary" command.  */
> @@ -1367,9 +1371,9 @@ no_shared_libraries_command (const char *ignored, int from_tty)
>   void
>   update_solib_breakpoints (void)
>   {
> -  const solib_ops *ops = gdbarch_so_ops (current_inferior ()->arch ());
> +  const solib_ops *ops = current_program_space->solib_ops ();
>   
> -  if (ops->update_breakpoints != NULL)
> +  if (ops != nullptr && ops->update_breakpoints != nullptr)
>       ops->update_breakpoints ();
>   }
>   
> @@ -1378,9 +1382,9 @@ update_solib_breakpoints (void)
>   void
>   handle_solib_event (void)
>   {
> -  const solib_ops *ops = gdbarch_so_ops (current_inferior ()->arch ());
> +  const solib_ops *ops = current_program_space->solib_ops ();
>   
> -  if (ops->handle_event != NULL)
> +  if (ops != nullptr && ops->handle_event != nullptr)
>       ops->handle_event ();
>   
>     current_inferior ()->pspace->clear_solib_cache ();
> @@ -1464,8 +1468,6 @@ reload_shared_libraries (const char *ignored, int from_tty,
>   {
>     reload_shared_libraries_1 (from_tty);
>   
> -  const solib_ops *ops = gdbarch_so_ops (current_inferior ()->arch ());
> -
>     /* Creating inferior hooks here has two purposes.  First, if we reload
>        shared libraries then the address of solib breakpoint we've computed
>        previously might be no longer valid.  For example, if we forgot to set
> @@ -1477,9 +1479,11 @@ reload_shared_libraries (const char *ignored, int from_tty,
>        about ld.so.  */
>     if (target_has_execution ())
>       {
> +      const solib_ops *ops = current_program_space->solib_ops ();
> +
>         /* Reset or free private data structures not associated with
>   	 solib entries.  */
> -      if (ops->clear_solib != nullptr)
> +      if (ops != nullptr && ops->clear_solib != nullptr)
>   	ops->clear_solib (current_program_space);
>   
>         /* Remove any previous solib event breakpoint.  This is usually
> diff --git a/gdb/target.c b/gdb/target.c
> index 522bed8e939c..7a296572830e 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -2464,6 +2464,7 @@ target_pre_inferior ()
>     if (!gdbarch_has_global_solist (current_inferior ()->arch ()))
>       {
>         no_shared_libraries (current_program_space);
> +      current_program_space->unset_solib_ops ();
>   
>         invalidate_target_mem_regions ();
>   
> diff --git a/gdb/tracectf.c b/gdb/tracectf.c
> index 1650e676d814..ce0b0079a29e 100644
> --- a/gdb/tracectf.c
> +++ b/gdb/tracectf.c
> @@ -1174,7 +1174,7 @@ ctf_target_open (const char *args, int from_tty)
>     merge_uploaded_trace_state_variables (&uploaded_tsvs);
>     merge_uploaded_tracepoints (&uploaded_tps);
>   
> -  post_create_inferior (from_tty);
> +  post_create_inferior (from_tty, true);
>   }
>   
>   /* This is the implementation of target_ops method to_close.  Destroy
> diff --git a/gdb/tracefile-tfile.c b/gdb/tracefile-tfile.c
> index 5f8b338df4cc..093b10e51d6e 100644
> --- a/gdb/tracefile-tfile.c
> +++ b/gdb/tracefile-tfile.c
> @@ -567,7 +567,7 @@ tfile_target_open (const char *arg, int from_tty)
>   
>     merge_uploaded_tracepoints (&uploaded_tps);
>   
> -  post_create_inferior (from_tty);
> +  post_create_inferior (from_tty, true);
>   }
>   
>   /* Interpret the given line from the definitions part of the trace