Re: [PATCH v3 5/6] gdb/progspace: add solib_ops pointer in program_space

[Guinevere Larsen <guinevere@redhat.com>]

On 6/16/25 4:33 PM, Simon Marchi wrote:
> New in v3:
>
>   - add some ops nullptr checks in print_solib_list_table and
>     info_linker_namespace_command
>
> 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.
>
> With this patch, the message we get when running "info
> linker-namespaces" becomes always the same, so update the test in
> gdb.base/dlmopen-ns-ids.exp.
>
> Change-Id: Ide8ddc57328895720fcd645d46dc34491f84c656

Thanks for working on this series! I have one very minor nit in this 
patch, but with that fixes, you can feel free to add my review tag

Reviewed-By: Guinevere Larsen <guinevere@redhat.com>

Feel free to add it to patches 1-4 as well, as I looked over them and 
they all LGTM. I still don't feel qualified to offer it for patch 6, though.

> ---
>   gdb/corelow.c                             |  2 +-
>   gdb/infcmd.c                              | 15 ++++---
>   gdb/inferior.h                            |  9 +++-
>   gdb/infrun.c                              | 11 ++++-
>   gdb/progspace.h                           | 20 +++++++++
>   gdb/solib.c                               | 55 +++++++++++++----------
>   gdb/target.c                              |  1 +
>   gdb/testsuite/gdb.base/dlmopen-ns-ids.exp |  3 +-
>   gdb/tracectf.c                            |  2 +-
>   gdb/tracefile-tfile.c                     |  2 +-
>   10 files changed, 83 insertions(+), 37 deletions(-)
>
<snip>
> diff --git a/gdb/solib.c b/gdb/solib.c
> index d008a72c5a38..675f64e2ae51 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,16 +1024,21 @@ 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;
>   
> +  if (ops == nullptr)
> +    return;
> +
>     /* There are 3 conditions for this command to print solib namespaces,
>        first PRINT_NAMESPACE has to be true, second the solib_ops has to
>        support multiple namespaces, and third there must be more than one
>        active namespace.  Fold all these into the PRINT_NAMESPACE condition.  */
> -  print_namespace = print_namespace && ops->num_active_namespaces != nullptr
> -		    && ops->num_active_namespaces () > 1;
> +  print_namespace = (print_namespace
> +		     && ops != nullptr
This check is redundant with the previous early exit. The only reason I 
bring this up is because the comment brings up "3 conditions" but there 
are 4 checks in this expression. The comment isn't wrong, but it does 
the code confusing until you figure out that checking if solib_ops is 
present isn't one of the conditions.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

> +		     && ops->num_active_namespaces != nullptr
> +		     && ops->num_active_namespaces () > 1);
>   
>     int num_cols = 4;
>     if (print_namespace)