[v2] Factor out the common code in lookup_{static,global}_symbol

  The two functions are extremely similar; this factors out their code into
a shared _internal function.

gdb/ChangeLog:

2019-08-02  Christian Biesinger  <cbiesinger@google.com>

	* symtab.c (lookup_static_symbol): Call the new function (and move
	it down to be next to lookup_global_symbol).
	(struct global_sym_lookup_data): Add block_enum member.
	(lookup_symbol_global_iterator_cb): Pass block_index instead of
	GLOBAL_BLOCK to lookup_symbol_in_objfile.
	(lookup_global_symbol_internal): New function.
	(lookup_global_symbol): Call new function.
---
 gdb/symtab.c | 82 ++++++++++++++++++++++++----------------------------
 1 file changed, 37 insertions(+), 45 deletions(-)
  

On Mon, Aug 5, 2019 at 10:33 AM Christian Biesinger
<cbiesinger@google.com> wrote:
>
> The two functions are extremely similar; this factors out their code into
> a shared _internal function.
>
> gdb/ChangeLog:
>
> 2019-08-02  Christian Biesinger  <cbiesinger@google.com>
>
>         * symtab.c (lookup_static_symbol): Call the new function (and move
>         it down to be next to lookup_global_symbol).
>         (struct global_sym_lookup_data): Add block_enum member.
>         (lookup_symbol_global_iterator_cb): Pass block_index instead of
>         GLOBAL_BLOCK to lookup_symbol_in_objfile.
>         (lookup_global_symbol_internal): New function.
>         (lookup_global_symbol): Call new function.
> ---
>  gdb/symtab.c | 82 ++++++++++++++++++++++++----------------------------
>  1 file changed, 37 insertions(+), 45 deletions(-)
>
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 87a0c8e4da..1a5a6bf20e 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -2566,44 +2566,6 @@ lookup_symbol_in_objfile (struct objfile *objfile, enum block_enum block_index,
>    return result;
>  }
>
> -/* See symtab.h.  */
> -
> -struct block_symbol
> -lookup_static_symbol (const char *name, const domain_enum domain)
> -{
> -  struct symbol_cache *cache = get_symbol_cache (current_program_space);
> -  struct block_symbol result;
> -  struct block_symbol_cache *bsc;
> -  struct symbol_cache_slot *slot;
> -
> -  /* Lookup in STATIC_BLOCK is not current-objfile-dependent, so just pass
> -     NULL for OBJFILE_CONTEXT.  */
> -  result = symbol_cache_lookup (cache, NULL, STATIC_BLOCK, name, domain,
> -                               &bsc, &slot);
> -  if (result.symbol != NULL)
> -    {
> -      if (SYMBOL_LOOKUP_FAILED_P (result))
> -       return {};
> -      return result;
> -    }
> -
> -  for (objfile *objfile : current_program_space->objfiles ())
> -    {
> -      result = lookup_symbol_in_objfile (objfile, STATIC_BLOCK, name, domain);
> -      if (result.symbol != NULL)
> -       {
> -         /* Still pass NULL for OBJFILE_CONTEXT here.  */
> -         symbol_cache_mark_found (bsc, slot, NULL, result.symbol,
> -                                  result.block);
> -         return result;
> -       }
> -    }
> -
> -  /* Still pass NULL for OBJFILE_CONTEXT here.  */
> -  symbol_cache_mark_not_found (bsc, slot, NULL, name, domain);
> -  return {};
> -}
> -
>  /* Private data to be used with lookup_symbol_global_iterator_cb.  */
>
>  struct global_sym_lookup_data
> @@ -2614,6 +2576,9 @@ struct global_sym_lookup_data
>    /* The domain to use for our search.  */
>    domain_enum domain;
>
> +  /* The block index in which to search */
> +  enum block_enum block_index;
> +
>    /* The field where the callback should store the symbol if found.
>       It should be initialized to {NULL, NULL} before the search is started.  */
>    struct block_symbol result;
> @@ -2634,7 +2599,7 @@ lookup_symbol_global_iterator_cb (struct objfile *objfile,
>    gdb_assert (data->result.symbol == NULL
>               && data->result.block == NULL);
>
> -  data->result = lookup_symbol_in_objfile (objfile, GLOBAL_BLOCK,
> +  data->result = lookup_symbol_in_objfile (objfile, data->block_index,
>                                            data->name, data->domain);
>
>    /* If we found a match, tell the iterator to stop.  Otherwise,
> @@ -2642,12 +2607,15 @@ lookup_symbol_global_iterator_cb (struct objfile *objfile,
>    return (data->result.symbol != NULL);
>  }
>
> -/* See symtab.h.  */
> +/* This function contains the common code of lookup_{global,static}_symbol.
> +   BLOCK is only used if BLOCK_INDEX is GLOBAL_SCOPE, in which case it is
> +   used to retrieve the objfile to start the lookup in.  */
>
> -struct block_symbol
> -lookup_global_symbol (const char *name,
> -                     const struct block *block,
> -                     const domain_enum domain)
> +static struct block_symbol
> +lookup_global_symbol_internal (const char *name,
> +                              enum block_enum block_index,
> +                              const struct block *block,
> +                              const domain_enum domain)
>  {
>    struct symbol_cache *cache = get_symbol_cache (current_program_space);
>    struct block_symbol result;
> @@ -2656,11 +2624,14 @@ lookup_global_symbol (const char *name,
>    struct block_symbol_cache *bsc;
>    struct symbol_cache_slot *slot;
>
> +  gdb_assert (block_index == GLOBAL_BLOCK || block_index == STATIC_BLOCK);
> +  gdb_assert (block == nullptr || block_index == GLOBAL_BLOCK);
> +
>    objfile = lookup_objfile_from_block (block);
>
>    /* First see if we can find the symbol in the cache.
>       This works because we use the current objfile to qualify the lookup.  */
> -  result = symbol_cache_lookup (cache, objfile, GLOBAL_BLOCK, name, domain,
> +  result = symbol_cache_lookup (cache, objfile, block_index, name, domain,
>                                 &bsc, &slot);
>    if (result.symbol != NULL)
>      {
> @@ -2678,6 +2649,7 @@ lookup_global_symbol (const char *name,
>      {
>        memset (&lookup_data, 0, sizeof (lookup_data));
>        lookup_data.name = name;
> +      lookup_data.block_index = block_index;
>        lookup_data.domain = domain;
>        gdbarch_iterate_over_objfiles_in_search_order
>         (objfile != NULL ? get_objfile_arch (objfile) : target_gdbarch (),
> @@ -2693,6 +2665,26 @@ lookup_global_symbol (const char *name,
>    return result;
>  }
>
> +/* See symtab.h.  */
> +
> +struct block_symbol
> +lookup_static_symbol (const char *name, const domain_enum domain)
> +{
> +  /* TODO: Should static symbol lookup start with a block as well, so we can
> +     prefer a static symbol in the current CU?  */
> +  return lookup_global_symbol_internal (name, STATIC_BLOCK, nullptr, domain);
> +}
> +
> +/* See symtab.h.  */
> +
> +struct block_symbol
> +lookup_global_symbol (const char *name,
> +                     const struct block *block,
> +                     const domain_enum domain)
> +{
> +  return lookup_global_symbol_internal (name, GLOBAL_BLOCK, block, domain);
> +}
> +
>  int
>  symbol_matches_domain (enum language symbol_language,
>                        domain_enum symbol_domain,
> --
> 2.22.0.770.g0f2c4a37fd-goog
>
  

On 2019-08-05 11:33 a.m., Christian Biesinger via gdb-patches wrote:
> @@ -2614,6 +2576,9 @@ struct global_sym_lookup_data
>    /* The domain to use for our search.  */
>    domain_enum domain;
>  
> +  /* The block index in which to search */

Period and two spaces:

  /* The block index in which to search.  */

> -struct block_symbol
> -lookup_global_symbol (const char *name,
> -		      const struct block *block,
> -		      const domain_enum domain)
> +static struct block_symbol
> +lookup_global_symbol_internal (const char *name,
> +			       enum block_enum block_index,
> +			       const struct block *block,
> +			       const domain_enum domain)

I'd rename that to lookup_symbol_internal, since it's used both in lookup_global_symbol and
lookup_static_symbol.  If you think it's too general (the function doesn't look up local symbols),
Maybe "lookup_global_or_static_symbol"?  And global_sym_lookup_data should be renamed accordingly.

And since BLOCK is only used for looking up an objfile in the global case, I would suggest
making this function accept the objfile instead of a block.  Otherwise, it could be confused
as "lookup symbol in this block".  Also, if some code already has the objfile handy, they can
pass it down, in which case we avoid the cost of lookup_objfile_from_block.

> +/* See symtab.h.  */
> +
> +struct block_symbol
> +lookup_static_symbol (const char *name, const domain_enum domain)
> +{
> +  /* TODO: Should static symbol lookup start with a block as well, so we can
> +     prefer a static symbol in the current CU?  */
> +  return lookup_global_symbol_internal (name, STATIC_BLOCK, nullptr, domain);

I'd say that we can add another argument when the need comes, it's not a stable API.

Simon
  

On Sun, Aug 25, 2019 at 9:20 PM Simon Marchi <simark@simark.ca> wrote:
>
> On 2019-08-05 11:33 a.m., Christian Biesinger via gdb-patches wrote:
> > @@ -2614,6 +2576,9 @@ struct global_sym_lookup_data
> >    /* The domain to use for our search.  */
> >    domain_enum domain;
> >
> > +  /* The block index in which to search */
>
> Period and two spaces:
>
>   /* The block index in which to search.  */

Done

> > -struct block_symbol
> > -lookup_global_symbol (const char *name,
> > -                   const struct block *block,
> > -                   const domain_enum domain)
> > +static struct block_symbol
> > +lookup_global_symbol_internal (const char *name,
> > +                            enum block_enum block_index,
> > +                            const struct block *block,
> > +                            const domain_enum domain)
>
> I'd rename that to lookup_symbol_internal, since it's used both in lookup_global_symbol and
> lookup_static_symbol.  If you think it's too general (the function doesn't look up local symbols),
> Maybe "lookup_global_or_static_symbol"?  And global_sym_lookup_data should be renamed accordingly.
>
> And since BLOCK is only used for looking up an objfile in the global case, I would suggest
> making this function accept the objfile instead of a block.  Otherwise, it could be confused
> as "lookup symbol in this block".  Also, if some code already has the objfile handy, they can
> pass it down, in which case we avoid the cost of lookup_objfile_from_block.

Done

> > +/* See symtab.h.  */
> > +
> > +struct block_symbol
> > +lookup_static_symbol (const char *name, const domain_enum domain)
> > +{
> > +  /* TODO: Should static symbol lookup start with a block as well, so we can
> > +     prefer a static symbol in the current CU?  */
> > +  return lookup_global_symbol_internal (name, STATIC_BLOCK, nullptr, domain);
>
> I'd say that we can add another argument when the need comes, it's not a stable API.

Removed