Re: [PATCH v3] Replace solib_global_lookup with gdbarch_iterate_over_objfiles_in_search_order

["Christian Biesinger via gdb-patches" <gdb-patches@sourceware.org>]

On Fri, Sep 20, 2019 at 10:00 AM Simon Marchi <simark@simark.ca> wrote:
>
> On 2019-08-27 7:03 p.m., Christian Biesinger via gdb-patches wrote:
> > [Test solib-corrupted.exp is now fixed]
> >
> > 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.
>
> Hi Christian,
>
> The patch LGTM, I'm also confident that it doesn't change the current
> behavior.  I just have two very minor comments below.
>
> Note that SPU support is about to be removed [1].  If you wait until it is
> removed, it would simplify a bit this patch, as you wish.
>
> [1] https://sourceware.org/ml/gdb-patches/2019-09/msg00067.html

Will do.

> > @@ -3202,32 +3204,41 @@ 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->separate_debug_objfile_backlink != nullptr)
> > +        current_objfile = current_objfile->separate_debug_objfile_backlink;
> >
> > -  if (abfd == NULL || scan_dyntag (DT_SYMBOLIC, abfd, NULL, NULL) != 1)
> > -    return {};
> > +      if (current_objfile == symfile_objfile)
> > +     abfd = exec_bfd;
> > +      else
> > +     abfd = current_objfile->obfd;
> > +
> > +      if (abfd != nullptr &&
> > +       scan_dyntag (DT_SYMBOLIC, abfd, nullptr, nullptr) == 1)
> > +     {
> > +       if (cb (current_objfile, cb_data) != 0)
> > +         return;
> > +     }
> > +    }
> >
> > -  return lookup_global_symbol_from_objfile (objfile, GLOBAL_BLOCK, name,
> > -                                         domain);
> > +  for (objfile *objfile : current_program_space->objfiles ())
> > +    {
> > +      if (cb (objfile, cb_data) != 0)
> > +     return;
> > +    }
> >  }
>
> If we do search the current objfile in the first part of that
> function, we could skip it in the second part.

Done.

> > 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"
>
> Could this be a forward declaration of `struct gdbarch` instead?
>
> Though if you wait until SPU is removed, the function
> svr4_iterate_over_objfiles_in_search_order won't need to be exported, it should
> be made static.

Done (made static).

Will send a new version of the patch and push when SPU is removed.

Christian