Re: [PATCH v3] Replace solib_global_lookup with gdbarch_iterate_over_objfiles_in_search_order

Christian Biesinger via gdb-patches
2019-09-20T02:57:47Z

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