Re: [PATCH v6 1/3] gdb: make lookup_minimal_symbol_linkage work with linker namespaces

[Andrew Burgess <aburgess@redhat.com>]

Guinevere Larsen <guinevere@redhat.com> writes:

> When a symbol found by GDB may have copy relocation, GDB calculates the
> address of the symbol by checking all the objfiles in the current
> program space for another instance of the same symbol, and returns the
> address of the first instance found.
>
> This works well when linker namespaces are not involved, but fails when
> a symbol is present in more than one namespace. That's because copy
> relocations respect linker namespace boundaries, and so if we're trying
> to find the symbol in namespace N, the symbol from a different namespace
> M may be returned, if the SO containing the symbol was loaded in M
> before N.
>
> To make the search work correctly, lookup_minimal_symbol_linkage would
> need to also respect linker namespace boundaries. However, to avoid
> leaking solib knowledge to minsyms, and because of how little
> information is passed to the function, this commit instead makes the
> function take a vector of objfiles to search, in place of the program
> space. This makes it so symbol::get_maybe_copied_address (and the
> equivalent of minimal_symbol) need to request the objfiles for the
> correct namespace, since they have enough context to figure out the
> namespace. Creating the vector is left as a function in solib.c
>
> Since minimal_symbol::get_maybe_copied_address only searches main_file
> objfiles, if we can guarantee that only one is loaded for each program
> space, we could avoid the work of constructing an std::vector that will
> be mostly ignored. However, I couldn't convince myself that that is the
> case, so I decided to keep the behavior exactly as is.
>
> Ideally, lookup_minimal_symbol_linkage would receive an iterator instead
> of a vector, but as of now it isn't possible to do forward declarations
> of nested types (we'd need program_space::objfiles_range) and
> progspace.h can't be included into minsyms.h, so we're stuck
> constructing a vector for it.

I think the wording of this paragraph is unchanged since v5.  You're
saying: I'd like to do <stuff> but the current GDB code prevents me.
Based on this, I asked, my not rework GDB.  You then explained that
there's a deeper reason for why you cannot do what you want.  I would
suggest you update the commit message in cases like this as future
readers might have the same questions I had...

... except in this case, I have some deeper thoughts on this patch.

>
> Finally, as some minor refactoring, find_solib_for_objfile is moved to
> solib.c to simplify looking for which namespace contains the objfile
> where we found the symbol the user was looking for.

I'll not leave this comment inline with the diff, as it's a more general
observation.  But I'm not super keen on find_solib_for_objfile moving
out of solib-svr4.c, at least not in its current form.

The reason: the function is broken and really shouldn't be used at all.

The relationship between objfiles and solibs is 1 to many.  The obvious
example here is the runtime linker, this will be mapped multiple times,
but only appear in memory once.  There will be many solibs, but only a
single objfile.

Prior to this patch, find_solib_for_objfile was used in one place, in
svr4_solib_ops::iterate_over_objfiles_in_search_order.  If we look at
how it is used, the function takes a CURRENT_OBJFILE and limits symbols
searches to objfiles in the same namespace.  But what if the dynamic
linker is the CURRENT_OBJFILE?  Right now we'll just search the first
namespace that we find, when we probably should be searching all
namespaces.

I got thinking about this because, I'm really keen on the build a
vector prior to calling lookup_minimal_symbol_linkage approach.  I
cannot really put my finger on why I don't like this, but it just
doesn't feel great.

I think my problem is that we already have a list of objfiles, and all
we really want to do is ask for each one; is this in the right linker
namespace?  The whole pre-build a vector business is necessitated
because, given an objfile, we cannot easily answer the question; is this
in the same namespace as some other objfile.  If we could ask that
question easily, then the change here would be simple, right?  In
minsyms.c, in lookup_minimal_symbol_linkage, we'd do this:

  const solib_ops *ops = pspace->solib_ops ();
  for (objfile &objfile : pspace->objfiles ())
    if (ops->in_same_linker_namespace (objfile, original_objfile))
      {
        ... existing code ...
      }

I did some hacking, and gives the same answer as the current approach,
but it isn't efficient as (currently) this requires looping over all
solib within the in_same_linker_namespace method.  And we need that loop
because find_solib_for_objfile is broken.

I wonder if we should first try to fix find_solib_for_objfile?  If this
could both return all solib in some way (more below), and maybe not
require a loop over all solib to search for the objfile, then this would
make what you want to do really straight forward.

On returning all solib, in order to avoid building a vector, I wondered
if we should consider linking all "sibling" solib into some kind of
linked list (sibling solib being solib that reference the same objfile),
then find_solib_for_objfile would return a single solib*, but users
would then need to iterate over all sibling solib.

> ---
>  gdb/dwarf2/ada-imported.c |  8 +++-
>  gdb/minsyms.c             | 15 ++++---
>  gdb/minsyms.h             |  9 ++--
>  gdb/solib-svr4.c          | 20 ---------
>  gdb/solib.c               | 86 ++++++++++++++++++++++++++++++++++++++-
>  gdb/solib.h               | 19 +++++++++
>  gdb/symtab.c              | 13 +++++-
>  7 files changed, 136 insertions(+), 34 deletions(-)
>
> diff --git a/gdb/dwarf2/ada-imported.c b/gdb/dwarf2/ada-imported.c
> index 48e6fccdb73..15ecfdaf6ce 100644
> --- a/gdb/dwarf2/ada-imported.c
> +++ b/gdb/dwarf2/ada-imported.c
> @@ -35,9 +35,13 @@ static struct value *
>  ada_imported_read_variable (struct symbol *symbol, const frame_info_ptr &frame)
>  {
>    const char *name = get_imported_name (symbol);
> +
> +  std::vector<objfile *> objfiles_to_search;
> +  for (objfile &objf : symbol->objfile ()->pspace ()->objfiles ())
> +    objfiles_to_search.push_back (&objf);
> +
>    bound_minimal_symbol minsym
> -    = lookup_minimal_symbol_linkage (symbol->objfile ()->pspace (), name,
> -				     true, false);
> +    = lookup_minimal_symbol_linkage (objfiles_to_search, name, true, false);

I would have expected some discussion in the commit message about why
this use of lookup_minimal_symbol_linkage doesn't require the same
namespace handling.  I haven't looked at the code to try and figure it
out, so maybe it is obvious.  But still, I think it's worth mentioning.

>    if (minsym.minsym == nullptr)
>      error (_("could not find imported name %s"), name);
>    return value_at (symbol->type (), minsym.value_address ());
> diff --git a/gdb/minsyms.c b/gdb/minsyms.c
> index 90a0c601c59..7e4f47b8c3f 100644
> --- a/gdb/minsyms.c
> +++ b/gdb/minsyms.c
> @@ -54,6 +54,7 @@
>  #include "gdbsupport/cxx-thread.h"
>  #include "gdbsupport/parallel-for.h"
>  #include "inferior.h"
> +#include "solib.h"
>  
>  /* Return true if MINSYM is a cold clone symbol.
>     Recognize f.i. these symbols (mangled/demangled):
> @@ -584,19 +585,21 @@ lookup_minimal_symbol_linkage (const char *name, struct objfile *objf,
>  /* See minsyms.h.  */
>  
>  bound_minimal_symbol
> -lookup_minimal_symbol_linkage (program_space *pspace, const char *name,
> -			       bool match_static_type, bool only_main)
> +lookup_minimal_symbol_linkage (gdb::array_view<objfile *> objfiles_to_search,
> +			       const char *name, bool match_static_type,
> +			       bool only_main)
>  {
> -  for (objfile &objfile : pspace->objfiles ())
> +  for (objfile *objfile : objfiles_to_search)
>      {
> -      if (objfile.separate_debug_objfile_backlink != nullptr)
> +      if (objfile == nullptr
> +	  || objfile->separate_debug_objfile_backlink != nullptr)
>  	continue;
>  
> -      if (only_main && (objfile.flags & OBJF_MAINLINE) == 0)
> +      if (only_main && (objfile->flags & OBJF_MAINLINE) == 0)
>  	continue;
>  
>        bound_minimal_symbol minsym
> -	= lookup_minimal_symbol_linkage (name, &objfile, match_static_type);
> +	= lookup_minimal_symbol_linkage (name, objfile, match_static_type);
>        if (minsym.minsym != nullptr)
>  	return minsym;
>      }
> diff --git a/gdb/minsyms.h b/gdb/minsyms.h
> index ed38044a38c..4aa8a428470 100644
> --- a/gdb/minsyms.h
> +++ b/gdb/minsyms.h
> @@ -237,11 +237,14 @@ extern bound_minimal_symbol lookup_minimal_symbol_linkage
>  
>  /* A variant of lookup_minimal_symbol_linkage that iterates over all
>     objfiles of PSPACE.  If ONLY_MAIN is true, then only an objfile with
> -   OBJF_MAINLINE will be considered.  */
> +   OBJF_MAINLINE will be considered.  This function should receive a
> +   program_space::objfile_ranges instead, but we can't include progspace.h
> +   here, nor can we do forward declarations of nested types, so std::vector
> +   will waste a bit of memory to work around that issue.  */

The reference to PSPACE here is now out of date.

Thanks,
Andrew

>  
>  extern bound_minimal_symbol lookup_minimal_symbol_linkage
> -  (program_space *pspace, const char *name, bool match_static_type,
> -   bool only_main) ATTRIBUTE_NONNULL (1);
> +  (gdb::array_view<objfile *> objfiles_to_search, const char *name,
> +   bool match_static_type, bool only_main);
>  
>  /* Look through all the current minimal symbol tables and find the
>     first minimal symbol that matches NAME and PC.  If OBJF is non-NULL,
> diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
> index c91e0b2c37b..49d2b207c36 100644
> --- a/gdb/solib-svr4.c
> +++ b/gdb/solib-svr4.c
> @@ -3595,26 +3595,6 @@ lp64_svr4_solib_ops::fetch_link_map_offsets () const
>  }
>  \f
>  
> -/* Return the DSO matching OBJFILE or nullptr if none can be found.  */
> -
> -static const solib *
> -find_solib_for_objfile (struct objfile *objfile)
> -{
> -  if (objfile == nullptr)
> -    return nullptr;
> -
> -  /* If OBJFILE is a separate debug object file, look for the original
> -     object file.  */
> -  if (objfile->separate_debug_objfile_backlink != nullptr)
> -    objfile = objfile->separate_debug_objfile_backlink;
> -
> -  for (const solib &so : current_program_space->solibs ())
> -    if (so.objfile == objfile)
> -      return &so;
> -
> -  return nullptr;
> -}
> -
>  /* Return the address of the r_debug object for the namespace containing
>     SOLIB or zero if it cannot be found.  This may happen when symbol files
>     are added manually, for example, or with the main executable.
> diff --git a/gdb/solib.c b/gdb/solib.c
> index dade2a76a48..34b0d6b3f9c 100644
> --- a/gdb/solib.c
> +++ b/gdb/solib.c
> @@ -1825,8 +1825,92 @@ solib_linker_namespace_count (program_space *pspace)
>    return 0;
>  }
>  
> -/* Implementation of the linker_namespace convenience variable.
> +/* See solib.h.  */
> +
> +const solib *
> +find_solib_for_objfile (struct objfile *objfile)
> +{
> +  if (objfile == nullptr)
> +    return nullptr;
> +
> +  /* If OBJFILE is a separate debug object file, look for the original
> +     object file.  */
> +  if (objfile->separate_debug_objfile_backlink != nullptr)
> +    objfile = objfile->separate_debug_objfile_backlink;
> +
> +  for (const solib &so : objfile->pspace ()->solibs ())
> +    if (so.objfile == objfile)
> +      return &so;
> +
> +  return nullptr;
> +}
> +
> +/* See solib.h.  */
> +
> +std::vector<objfile *>
> +get_objfiles_in_linker_namespace (int nsid, program_space *pspace)
> +{
> +  std::vector<objfile *> objfiles_in_ns;
> +  const solib_ops *ops = pspace->solib_ops ();
> +
> +  gdb_assert (ops->supports_namespaces ());
> +
> +  /* If we're looking at the default namespace, we also need
> +     to add the mainline objfiles by hand, since there is no
> +     solib associated with those files.  We add them first
> +     because if we're searching for copy relocations, they
> +     should be in the main file, so we'll find it faster.  */
> +  if (nsid == 0)
> +    for (objfile &objf : pspace->objfiles ())
> +      if ((objf.flags & OBJF_MAINLINE) != 0)
> +	objfiles_in_ns.push_back (&objf);
> +
> +  std::vector<const solib *> solibs = ops->get_solibs_in_ns (nsid);
> +  /* Reserve for efficiency.  */
> +  objfiles_in_ns.reserve (solibs.size () + objfiles_in_ns.size ());
> +  for (const solib *so : solibs)
> +    objfiles_in_ns.push_back (so->objfile);
> +
> +  return objfiles_in_ns;
> +}
> +
> +/* See solib.h.  */
> +
> +std::vector<objfile *>
> +get_objfiles_in_linker_namespace (objfile *objfile)
> +{
> +  program_space *pspace = objfile->pspace ();
> +  const solib_ops *ops = pspace->solib_ops ();
> +  const solib *so = find_solib_for_objfile (objfile);
> +
> +  /* If the inferior hasn't started yet, the solib_ops won't be
> +     set for the program space.  Since namespaces only make sense
> +     when the inferior has already created some, we can just skip
> +     it here.  */
> +  if (ops != nullptr && ops->supports_namespaces ()
> +      /* If we're searching for a symbol from the linker, we'll reach here
> +	 before having any namespaces.  Return all objfiles since the
> +	 boundaries haven't been setup yet.  */
> +      && ops->num_active_namespaces () > 0
> +      /* When trying to load libthread_db, we can search for a symbol in an
> +	 objfile with no associated solib.  In that case, again, we should
> +	 return all objfiles.  */
> +      && so != nullptr)
> +    {
> +      return get_objfiles_in_linker_namespace (ops->find_solib_ns (*so),
> +					       pspace);
> +    }
> +
> +  /* If any of the previous conditions isn't satisfied, we return
> +     the full list of objfiles in the inferior.  */
> +  std::vector<struct objfile *> found_objfiles;
> +  for (struct objfile &objf : objfile->pspace ()->objfiles ())
> +    found_objfiles.push_back (&objf);
>  
> +  return found_objfiles;
> +}
> +
> +/* Implementation of the linker_namespace convenience variable.
>     This returns the GDB internal identifier of the linker namespace,
>     for the selected frame, as an integer.  If the inferior doesn't support
>     linker namespaces, this always returns 0.  */
> diff --git a/gdb/solib.h b/gdb/solib.h
> index 85ea6675b79..81389da6267 100644
> --- a/gdb/solib.h
> +++ b/gdb/solib.h
> @@ -20,6 +20,9 @@
>  #ifndef GDB_SOLIB_H
>  #define GDB_SOLIB_H
>  
> +/* Forward decl's for prototypes */
> +struct objfile;
> +
>  #include "gdb_bfd.h"
>  #include "gdbsupport/function-view.h"
>  #include "gdbsupport/intrusive_list.h"
> @@ -409,4 +412,20 @@ extern void handle_solib_event (void);
>  
>  extern int solib_linker_namespace_count (program_space *pspace);
>  
> +/* Return a vector with pointers of all objfiles in the namespace
> +   NSID.  This version assumes that the inferior supports namespaces.  */
> +
> +std::vector<objfile *> get_objfiles_in_linker_namespace
> +  (int nsid, program_space *pspace);
> +
> +/* Return a vector with pointers of all objfiles in the same namespace
> +   as OBJFILE.  If the inferior doesn't support namespaces, return all
> +   objfiles in the program_space.  */
> +
> +std::vector<objfile *> get_objfiles_in_linker_namespace (objfile *objfile);
> +
> +/* Return the DSO matching OBJFILE or nullptr if none can be found.  */
> +
> +const solib *find_solib_for_objfile (struct objfile *objfile);
> +
>  #endif /* GDB_SOLIB_H */
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 3b0687c0750..786dd9f56ba 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -74,6 +74,7 @@
>  #include "gdbsupport/common-utils.h"
>  #include <optional>
>  #include "gdbsupport/unordered_set.h"
> +#include "solib.h"
>  
>  /* Forward declarations for local functions.  */
>  
> @@ -6575,8 +6576,12 @@ symbol::get_maybe_copied_address () const
>    gdb_assert (this->loc_class () == LOC_STATIC);
>  
>    const char *linkage_name = this->linkage_name ();
> +
> +  std::vector<struct objfile *> objfiles_to_search
> +    (get_objfiles_in_linker_namespace (this->objfile ()));
> +
>    bound_minimal_symbol minsym
> -    = lookup_minimal_symbol_linkage (this->objfile ()->pspace (), linkage_name,
> +    = lookup_minimal_symbol_linkage (objfiles_to_search, linkage_name,
>  				     false, false);
>    if (minsym.minsym != nullptr)
>      return minsym.value_address ();
> @@ -6593,8 +6598,12 @@ minimal_symbol::get_maybe_copied_address (objfile *objf) const
>    gdb_assert ((objf->flags & OBJF_MAINLINE) == 0);
>  
>    const char *linkage_name = this->linkage_name ();
> +
> +  std::vector<struct objfile *> objfiles_to_search
> +    (get_objfiles_in_linker_namespace (objf));
> +
>    bound_minimal_symbol found
> -    = lookup_minimal_symbol_linkage (objf->pspace (), linkage_name,
> +    = lookup_minimal_symbol_linkage (objfiles_to_search, linkage_name,
>  				     false, true);
>    if (found.minsym != nullptr)
>      return found.value_address ();
> -- 
> 2.51.0