Re: [RFC][PATCH] Replace solib_global_lookup with gdbarch_iterate_over_objfiles_in_search_order

["Christian Biesinger via gdb-patches" <gdb-patches at sourceware dot org>]

On a different note, given __attribute__((visibility("hidden))), we
may just want to always start the search in the current objfile?

On Thu, Aug 1, 2019 at 1:43 PM Christian Biesinger
<cbiesinger@google.com> wrote:
>
> Thoughts on this change? It does not compile (_initialize_svr4_solib does
> not have a gdbarch; comments welcome on the best way to handle this). But
> it this worthwhile?
>
> All implementations of either function use it for the same purpose (except
> Darwin, which is a no-op): to prefer a symbol in the current objfile over
> symbols with the same name in other objfiles. There does not seem to be a
> reason to have both mechanisms for that purpose.
> ---
>  gdb/solib-darwin.c |  9 --------
>  gdb/solib-spu.c    | 27 ++++++++++++-----------
>  gdb/solib-svr4.c   | 54 ++++++++++++++++++++++++++++++----------------
>  gdb/solib-svr4.h   |  5 +++++
>  gdb/solib.c        | 15 -------------
>  gdb/solist.h       | 11 ----------
>  gdb/symtab.c       |  6 +-----
>  7 files changed, 56 insertions(+), 71 deletions(-)
>
> diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
> index 443ebb64a4..b386cd35ed 100644
> --- a/gdb/solib-darwin.c
> +++ b/gdb/solib-darwin.c
> @@ -638,14 +638,6 @@ darwin_relocate_section_addresses (struct so_list *so,
>      so->addr_low = sec->addr;
>  }
>
> -static struct block_symbol
> -darwin_lookup_lib_symbol (struct objfile *objfile,
> -                         const char *name,
> -                         const domain_enum domain)
> -{
> -  return {};
> -}
> -
>  static gdb_bfd_ref_ptr
>  darwin_bfd_open (const char *pathname)
>  {
> @@ -688,6 +680,5 @@ _initialize_darwin_solib (void)
>    darwin_so_ops.current_sos = darwin_current_sos;
>    darwin_so_ops.open_symbol_file_object = open_symbol_file_object;
>    darwin_so_ops.in_dynsym_resolve_code = darwin_in_dynsym_resolve_code;
> -  darwin_so_ops.lookup_lib_global_symbol = darwin_lookup_lib_symbol;
>    darwin_so_ops.bfd_open = darwin_bfd_open;
>  }
> diff --git a/gdb/solib-spu.c b/gdb/solib-spu.c
> index 5b97b9bcf6..ed33a0adf2 100644
> --- a/gdb/solib-spu.c
> +++ b/gdb/solib-spu.c
> @@ -385,19 +385,21 @@ spu_bfd_open (const char *pathname)
>    return abfd;
>  }
>
> -/* Lookup global symbol in a SPE executable.  */
> -static struct block_symbol
> -spu_lookup_lib_symbol (struct objfile *objfile,
> -                      const char *name,
> -                      const domain_enum domain)
> +/* Search order prefers the current objfile for SPE executables.  */
> +static void
> +spu_iterate_over_objfiles_in_search_order
> +  (struct gdbarch *gdbarch,
> +   iterate_over_objfiles_in_search_order_cb_ftype *cb,
> +   void *cb_data, struct objfile *objfile)
>  {
> -  if (bfd_get_arch (objfile->obfd) == bfd_arch_spu)
> -    return lookup_global_symbol_from_objfile (objfile, GLOBAL_BLOCK, name,
> -                                             domain);
> +  if (objfile != nullptr && bfd_get_arch (objfile->obfd) == bfd_arch_spu)
> +    {
> +      if (cb (objfile, cb_data) != 0)
> +       return;
> +    }
>
> -  if (svr4_so_ops.lookup_lib_global_symbol != NULL)
> -    return svr4_so_ops.lookup_lib_global_symbol (objfile, name, domain);
> -  return {};
> +  return svr4_iterate_over_objfiles_in_search_order(
> +    gdbarch, cb, cb_data, objfile);
>  }
>
>  /* Enable shared library breakpoint.  */
> @@ -515,9 +517,10 @@ set_spu_solib_ops (struct gdbarch *gdbarch)
>        spu_so_ops.free_so = spu_free_so;
>        spu_so_ops.current_sos = spu_current_sos;
>        spu_so_ops.bfd_open = spu_bfd_open;
> -      spu_so_ops.lookup_lib_global_symbol = spu_lookup_lib_symbol;
>      }
>
> +  set_gdbarch_iterate_over_objfiles_in_search_order
> +    (gdbarch, spu_iterate_over_objfiles_in_search_order);
>    set_solib_ops (gdbarch, &spu_so_ops);
>  }
>
> diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
> index c0c505acaa..c92d60874f 100644
> --- a/gdb/solib-svr4.c
> +++ b/gdb/solib-svr4.c
> @@ -3202,32 +3202,46 @@ svr4_lp64_fetch_link_map_offsets (void)
>
>  struct target_so_ops svr4_so_ops;
>
> -/* Lookup global symbol for ELF DSOs linked with -Bsymbolic.  Those DSOs have a
> +/* Search order for ELF DSOs linked with -Bsymbolic.  Those DSOs have a
>     different rule for symbol lookup.  The lookup begins here in the DSO, not in
>     the main executable.  */
>
> -static struct block_symbol
> -elf_lookup_lib_symbol (struct objfile *objfile,
> -                      const char *name,
> -                      const domain_enum domain)
> +void
> +svr4_iterate_over_objfiles_in_search_order
> +  (struct gdbarch *gdbarch,
> +   iterate_over_objfiles_in_search_order_cb_ftype *cb,
> +   void *cb_data, struct objfile *current_objfile)
>  {
> -  bfd *abfd;
> -
> -  if (objfile == symfile_objfile)
> -    abfd = exec_bfd;
> -  else
> +  if (current_objfile != nullptr)
>      {
> -      /* OBJFILE should have been passed as the non-debug one.  */
> -      gdb_assert (objfile->separate_debug_objfile_backlink == NULL);
> +      bfd *abfd;
>
> -      abfd = objfile->obfd;
> -    }
> +      if (current_objfile == symfile_objfile)
> +       abfd = exec_bfd;
> +      else
> +       {
> +         /* OBJFILE should have been passed as the non-debug one.  */
> +         gdb_assert (current_objfile->separate_debug_objfile_backlink == nullptr);
>
> -  if (abfd == NULL || scan_dyntag (DT_SYMBOLIC, abfd, NULL, NULL) != 1)
> -    return {};
> +         abfd = current_objfile->obfd;
> +       }
>
> -  return lookup_global_symbol_from_objfile (objfile, GLOBAL_BLOCK, name,
> -                                           domain);
> +      if (abfd != nullptr &&
> +         scan_dyntag (DT_SYMBOLIC, abfd, nullptr, nullptr) == 1)
> +       {
> +         if (cb (current_objfile, cb_data) != 0)
> +           return;
> +       }
> +    }
> +
> +  for (objfile *objfile : current_program_space->objfiles ())
> +    {
> +      if (objfile != current_objfile)
> +       {
> +         if (cb (objfile, cb_data) != 0)
> +           return;
> +       }
> +    }
>  }
>
>  void
> @@ -3244,11 +3258,13 @@ _initialize_svr4_solib (void)
>    svr4_so_ops.open_symbol_file_object = open_symbol_file_object;
>    svr4_so_ops.in_dynsym_resolve_code = svr4_in_dynsym_resolve_code;
>    svr4_so_ops.bfd_open = solib_bfd_open;
> -  svr4_so_ops.lookup_lib_global_symbol = elf_lookup_lib_symbol;
>    svr4_so_ops.same = svr4_same;
>    svr4_so_ops.keep_data_in_core = svr4_keep_data_in_core;
>    svr4_so_ops.update_breakpoints = svr4_update_solib_event_breakpoints;
>    svr4_so_ops.handle_event = svr4_handle_solib_event;
>
>    gdb::observers::free_objfile.attach (svr4_free_objfile_observer);
> +
> +  set_gdbarch_iterate_over_objfiles_in_search_order
> +    (gdbarch, svr4_iterate_over_objfiles_in_search_order);
>  }
> diff --git a/gdb/solib-svr4.h b/gdb/solib-svr4.h
> index a051e70b79..b99b8e2e3a 100644
> --- a/gdb/solib-svr4.h
> +++ b/gdb/solib-svr4.h
> @@ -21,6 +21,7 @@
>  #define SOLIB_SVR4_H
>
>  #include "solist.h"
> +#include "gdbarch.h"
>
>  struct objfile;
>  struct target_so_ops;
> @@ -107,4 +108,8 @@ extern struct link_map_offsets *svr4_lp64_fetch_link_map_offsets (void);
>     SVR4 run time loader.  */
>  int svr4_in_dynsym_resolve_code (CORE_ADDR pc);
>
> +extern void svr4_iterate_over_objfiles_in_search_order (
> +  struct gdbarch *gdbarch, iterate_over_objfiles_in_search_order_cb_ftype *cb,
> +  void *cb_data, struct objfile *objfile);
> +
>  #endif /* solib-svr4.h */
> diff --git a/gdb/solib.c b/gdb/solib.c
> index 29a17ad5d4..ec3bf06c50 100644
> --- a/gdb/solib.c
> +++ b/gdb/solib.c
> @@ -1441,21 +1441,6 @@ show_auto_solib_add (struct ui_file *file, int from_tty,
>  }
>
>
> -/* Handler for library-specific lookup of global symbol NAME in OBJFILE.  Call
> -   the library-specific handler if it is installed for the current target.  */
> -
> -struct block_symbol
> -solib_global_lookup (struct objfile *objfile,
> -                    const char *name,
> -                    const domain_enum domain)
> -{
> -  const struct target_so_ops *ops = solib_ops (target_gdbarch ());
> -
> -  if (ops->lookup_lib_global_symbol != NULL)
> -    return ops->lookup_lib_global_symbol (objfile, name, domain);
> -  return {};
> -}
> -
>  /* Lookup the value for a specific symbol from dynamic symbol table.  Look
>     up symbol from ABFD.  MATCH_SYM is a callback function to determine
>     whether to pick up a symbol.  DATA is the input of this callback
> diff --git a/gdb/solist.h b/gdb/solist.h
> index e1410c2b79..ee99fc815b 100644
> --- a/gdb/solist.h
> +++ b/gdb/solist.h
> @@ -139,12 +139,6 @@ struct target_so_ops
>                               unsigned o_flags,
>                               gdb::unique_xmalloc_ptr<char> *temp_pathname);
>
> -  /* Hook for looking up global symbols in a library-specific way.  */
> -  struct block_symbol (*lookup_lib_global_symbol)
> -    (struct objfile *objfile,
> -     const char *name,
> -     const domain_enum domain);
> -
>    /* Given two so_list objects, one from the GDB thread list
>       and another from the list returned by current_sos, return 1
>       if they represent the same library.
> @@ -209,9 +203,4 @@ extern gdb_bfd_ref_ptr solib_bfd_open (const char *in_pathname);
>  /* FIXME: gdbarch needs to control this variable.  */
>  extern struct target_so_ops *current_target_so_ops;
>
> -/* Handler for library-specific global symbol lookup in solib.c.  */
> -struct block_symbol solib_global_lookup (struct objfile *objfile,
> -                                           const char *name,
> -                                           const domain_enum domain);
> -
>  #endif
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 87a0c8e4da..cd6a87581d 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -2669,11 +2669,7 @@ lookup_global_symbol (const char *name,
>        return result;
>      }
>
> -  /* Call library-specific lookup procedure.  */
> -  if (objfile != NULL)
> -    result = solib_global_lookup (objfile, name, domain);
> -
> -  /* If that didn't work go a global search (of global blocks, heh).  */
> +  /* Do a global search (of global blocks, heh).  */
>    if (result.symbol == NULL)
>      {
>        memset (&lookup_data, 0, sizeof (lookup_data));
> --
> 2.22.0.770.g0f2c4a37fd-goog
>