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

[Simon Marchi <simark@simark.ca>]

On 2025-06-20 14:20, Pedro Alves wrote:
> 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.

Np, it's a quick change.  Done.

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

Ack, done.

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

I have seen this, but haven't internalized yet how it can be useful.

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

Replacing "that" with "for", actually.  Fixed.

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

Fixed.

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

Ah, it's because I have followup patches (not sure if/when I'll post
them) that change solib implementations to move the data from registries
to fields of solib_ops_* objects directly.  Doing so requires making
some methods non-const, and so to call them we need
program_space::solib_ops to return a non-const solib_ops.

But for the moment, given that solib_ops_* objects don't keep data, it
would make more sense to leave everything const.  I will make all the
methods const again (like I had initially) and make
program_space::solib_ops return a const object.  We can remove the
consts if/when it's justified to do so.

Thanks for the review, I will push the series shortly, when I'm done
addressing these comments.

Simon