Re: [PATCH v3] Replace solib_global_lookup with gdbarch_iterate_over_objfiles_in_search_order

Simon Marchi
2019-09-20T01:01:02Z

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

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

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

Simon