Re: [PATCH v2 4/5] gdb: add program_space parameter to clear_solib

[Simon Marchi <simon.marchi@efficios.com>]

On 2/8/24 09:29, Andrew Burgess wrote:
> Simon Marchi <simon.marchi@efficios.com> writes:
> 
>> Make the current_program_space reference bubble up one level.
>>
>> Remove one unnecessary declaration of clear_solib.
>>
>> Change-Id: I234e2c8c0b71713364fc7b76cee2bee2b026bd6d
>> ---
>>  gdb/corelow.c |  2 +-
>>  gdb/solib.c   | 16 ++++++++--------
>>  gdb/solib.h   |  6 +++---
>>  gdb/symtab.h  |  4 ----
>>  4 files changed, 12 insertions(+), 16 deletions(-)
>>
>> diff --git a/gdb/corelow.c b/gdb/corelow.c
>> index 43c1f69b1317..f291b2aba191 100644
>> --- a/gdb/corelow.c
>> +++ b/gdb/corelow.c
>> @@ -335,7 +335,7 @@ core_target::clear_core ()
>>  
>>        /* Clear out solib state while the bfd is still open.  See
>>  	 comments in clear_solib in solib.c.  */
>> -      clear_solib ();
>> +      clear_solib (current_program_space);
>>  
>>        current_program_space->cbfd.reset (nullptr);
>>      }
>> diff --git a/gdb/solib.c b/gdb/solib.c
>> index 98cda039a833..991ff156d12f 100644
>> --- a/gdb/solib.c
>> +++ b/gdb/solib.c
>> @@ -1180,23 +1180,23 @@ solib_keep_data_in_core (CORE_ADDR vaddr, unsigned long size)
>>      return false;
>>  }
>>  
>> -/* Called by free_all_symtabs */
>> +/* See solib.h.  */
>>  
>>  void
>> -clear_solib (void)
>> +clear_solib (program_space *pspace)
>>  {
>>    const solib_ops *ops = gdbarch_so_ops (current_inferior ()->arch ());
>>  
>> -  disable_breakpoints_in_shlibs (current_program_space);
> 
> In the previous patch, when you added this use of current_program_space,
> I considered commenting that you should instead do:
> 
>   disable_breakpoints_in_shlibs (current_inferior ()->pspace);
> 
> because we already use current_inferior() in this function, and the
> current_program_space is really a property of the current_inferior.

Most of the time they match, but not always, unfortunately (otherwise,
we would not need the current_program_space global, we could always use
current_inferior()->pspace).

> But I figured such a comment was a bit trivial, and didn't make much
> difference.

As explained in my self-reply on v1, I think that clear_solib is called
with a program space (or with the current program space, prior to the
patch) that doesn't match `current_inferior ()->pspace`.  So I don't
think that doing:

    disable_breakpoints_in_shlibs (current_inferior ()->pspace);

would do what we want.  It would call disable_breakpoints_in_shlibs with
the wrong pspace.

> But with this patch I think instead of passing the PSPACE argument we
> should be passing an 'inferior *inf' argument here, which we'd then
> forward to the gdbarch_so_ops call, and then use inf->pspace instead of
> PSPACE throughout this function.
> 
> What are your thoughts?

The solib operations are really a per-pspace thing, so I think it makes
sense to pass a pspace.  The inferior doesn't need to be involved.  The
only reason why we need the inferior here is to get an arch, to get the
solib_ops implementation used for that pspace, to be able to tell it:
clear all you know about that pspace.

In the series that I have in the pipeline, a first step is to add a
pspace -> solib_ops backlink to indicate what is the solib_ops used to
provide the solibs for that pspace.  That will remove the need for the
current_inferior call here, and should make that function completely
inferior-agnostic.  A second step (not relevant for this discussion, but
just FYI) will be to make it possible for multiple solib_ops to provide
solibs for a given pspace.  That will be useful for heterogenous
programs where you have CPU code and GPU code loaded in the same address
space.  The pspace -> solib_ops backlink will then become a list of
solib_ops pointers.

Simon