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

[Simon Marchi <simark@simark.ca>]

On 6/17/25 3:45 PM, Guinevere Larsen wrote:
> 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.

Thank you!

Given your review, I plan to push this series probably tomorrow.  You
can start rebasing and adjusting your series on top, and then I can
hopefully review it this week (note that I'm off this Friday, so it
would have to be Thursday max).

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

You're right, it's redundant, I'll fix it.  Note to self: returning
early means that the table will not be output at all.  Check if
returning early is a problem for e.g. MI, when the inferior is not
running.

Simon