Re: [PATCH v3] Replace solib_global_lookup with gdbarch_iterate_over_objfiles_in_search_order

[Simon Marchi <simark@simark.ca>]

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