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

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

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.

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

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?

Thanks,
Andrew

> +  disable_breakpoints_in_shlibs (pspace);
>  
> -  current_program_space->so_list.clear_and_dispose ([] (solib *so) {
> -    notify_solib_unloaded (current_program_space, *so);
> -    current_program_space->remove_target_sections (so);
> +  pspace->so_list.clear_and_dispose ([pspace] (solib *so) {
> +    notify_solib_unloaded (pspace, *so);
> +    pspace->remove_target_sections (so);
>      delete so;
>    });
>  
>    if (ops->clear_solib != nullptr)
> -    ops->clear_solib (current_program_space);
> +    ops->clear_solib (pspace);
>  }
>  
>  /* Shared library startup support.  When GDB starts up the inferior,
> @@ -1244,7 +1244,7 @@ no_shared_libraries (const char *ignored, int from_tty)
>       access to their associated objfiles.  Therefore, we can not purge the
>       solibs' objfiles before clear_solib has been called.  */
>  
> -  clear_solib ();
> +  clear_solib (current_program_space);
>    objfile_purge_solibs ();
>  }
>  
> diff --git a/gdb/solib.h b/gdb/solib.h
> index 69183278318b..f7a93c0718f0 100644
> --- a/gdb/solib.h
> +++ b/gdb/solib.h
> @@ -42,10 +42,10 @@ extern bool debug_solib;
>  #define SOLIB_SCOPED_DEBUG_START_END(fmt, ...) \
>    scoped_debug_start_end (debug_solib, "solib", fmt, ##__VA_ARGS__)
>  
> -/* Called when we free all symtabs, to free the shared library information
> -   as well.  */
> +/* Called when we free all symtabs of PSPACE, to free the shared library
> +   information as well.  */
>  
> -extern void clear_solib (void);
> +extern void clear_solib (program_space *pspace);
>  
>  /* Called to add symbols from a shared library to gdb's symbol table.  */
>  
> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index 3b067a77b3c8..ca5a5b0f7fde 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -2423,10 +2423,6 @@ extern bool find_line_pc_range (struct symtab_and_line, CORE_ADDR *,
>  
>  extern void resolve_sal_pc (struct symtab_and_line *);
>  
> -/* solib.c */
> -
> -extern void clear_solib (void);
> -
>  /* The reason we're calling into a completion match list collector
>     function.  */
>  enum class complete_symbol_mode
> -- 
> 2.43.0
  

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
  

Simon Marchi <simon.marchi@efficios.com> writes:

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

Thanks for adding this last part.  This was what worried me about the
idea of a pspace -> solib_ops link, multiple gdbarch within a single
program_space.

So I'm convinced that this is the right way to go.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew
  

On 2024-02-09 09:59, Andrew Burgess wrote:
> Thanks for adding this last part.  This was what worried me about the
> idea of a pspace -> solib_ops link, multiple gdbarch within a single
> program_space.
> 
> So I'm convinced that this is the right way to go.

Cool, thanks.

Just to be clear, we can have multiple gdbarches within a pspace today
(e.g. the now-removed SPU port), we just can't have them specify
different solib_ops.  Just moving to have a single pspace -> solib_ops
link wouldn't be a regression, it would just make what we have now more
explicit.

Simon