Re: [PATCH v3 6/6] gdb/solib: C++ify solib_ops

[Pedro Alves <pedro@palves.net>]

Hi!

On 2025-06-16 20:33, Simon Marchi wrote:
> Convert solib_ops into an abstract base class (with abstract methods,
> some of them with default implementations) and convert all the existing
> solib_ops instances to solib_ops derived classes / implementations.

Thanks a lot for doing this.

Some small nits below.  Other than that:

Approved-By: Pedro Alves <pedro@palves.net>

> 
> Prior to this patch, solib_ops is a structure holding function pointers,
> of which there are only a handful of global instances (in the
> `solib-*.c` files).  When passing an `solib_ops *` around, it's a
> pointer to one of these instances.  After this patch, there are no more
> global solib_ops instances.  Instances are created as needed and stored
> in struct program_space.  These instances could eventually be made to
> contain the program space-specific data, which is currently kept in
> per-program space registries (I have some pending patches for that).
> 
> Prior to this patch, `gdbarch_so_ops` is a gdbarch method that returns a
> pointer to the appropriate solib_ops implementation for the gdbarch.
> This is replaced with the `gdbarch_new_solib_ops` method, which returns
> a new instance of the appropriate solib_ops implementation for this
> gdbarch.  This requires introducing some factory functions for the
> various solib_ops implementation, to be used as `gdbarch_new_solib_ops`
> callbacks.  For instance:
> 
>     solib_ops_up
>     new_linux_ilp32_svr4_solib_ops ()
>     {
>       return std::make_unique<linux_ilp32_svr4_solib_ops> ();
>     }
> 

(Annoying) nit: I'd say that factory functions that return a non-raw pointer are more
typically named make_foo, like std::make_unique.  new_foo suggests that they would return a
raw pointer, like operator new.

> There is a little "base class template" hack in mips-linux-tdep.c,
> because both mips_linux_ilp32_svr4_solib_ops and
> mips_linux_lp64_svr4_solib_ops need to derive from different SVR4 base
> classes (linux_ilp32_svr4_solib_ops and linux_lp64_svr4_solib_ops), but
> they both want to override the in_dynsym_resolve_code method with the
> same implementation.  Let me know if there's a more straightforward way
> to do this.

I don't know why would call this a hack.  This is a CRTP mixin, a standard
technique, that we use in other places in GDB, for the same reason.  I think
you should just say that instead.

We should be able to simplify such code a bit with C++23, with the new
explicit object parameter syntax.

> 
> The solib_ops::supports_namespaces method is new: the support for
> namespaces was previously predicated by the presence or absence of a
> find_solib_ns method.  It now needs to be explicit.
> 
> There is a new progspace::release_solib_ops method, which is only needed
> for rocm_solib_ops.  For the moment, rocm_solib_ops replaces and wraps
> the existing svr4_solib_ops instance, in order to combine the results of
> the two.  The plan is to have a subsequent patch to allow program spaces to have
> multiple solib_ops, removing the need that release_solib_ops.


Some word missing in "need that release_solib_ops".  I guess "for".


> 
> Speaking of rocm_solib_ops: it previously overrode only a few methods by
> copying svr4_solib_ops and overwriting some function pointers.  Now, it
> needs to implement all the methods that svr4_solib_ops implements, in
> order to forward the call.  Otherwise, the default solib_ops method woul

woul -> would




> --- a/gdb/progspace.h
> +++ b/gdb/progspace.h
> @@ -21,6 +21,7 @@
>  #ifndef GDB_PROGSPACE_H
>  #define GDB_PROGSPACE_H
>  
> +#include "solib.h"
>  #include "target.h"
>  #include "gdb_bfd.h"
>  #include "registry.h"
> @@ -234,19 +235,23 @@ struct program_space
>    /* 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)
> +  void set_solib_ops (solib_ops_up ops)
>    {
>      gdb_assert (m_solib_ops == nullptr);
> -    m_solib_ops = &ops;
> +    m_solib_ops = std::move (ops);
>    };
>  
> -  /* Unset this program space's solib provider.  */
> +  /* Unset and free this program space's solib provider.  */
>    void unset_solib_ops ()
>    { m_solib_ops = nullptr; }
>  
> +  /* Unset and return this program space's solib provider.  */
> +  solib_ops_up release_solib_ops ()
> +  { return std::move (m_solib_ops); }
> +
>    /* Get this program space's solib provider.  */
> -  const struct solib_ops *solib_ops () const
> -  { return m_solib_ops; }
> +  struct solib_ops *solib_ops () const
> +  { return m_solib_ops.get (); }

Curious that const dropped here and other places.  Was const previously
just wrong?