Re: [PATCH] Simplify block_find_symbol

[Alexandra Petlanova Hajkova <ahajkova@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 8149 bytes --]

On Sat, Sep 2, 2023 at 1:29 AM Tom Tromey <tom@tromey.com> wrote:

> block_find_symbol takes a callback function, but only two callbacks
> are ever passed to it -- and they are similar enough that it seems
> cleaner to just have block_find_symbol do the work itself.  Also,
> block_find_symbol can take a lookup_name_info as an argument,
> following the general idea of pushing the construction of these
> objects as high in the call chain as feasible.
>
> Regression tested on x86-64 Fedora 38.
> ---
>  gdb/block.c         | 43 +++++++++++--------------------------------
>  gdb/block.h         | 36 ++++++------------------------------
>  gdb/symfile-debug.c | 10 +++-------
>  gdb/symtab.c        | 13 +++++++------
>  4 files changed, 27 insertions(+), 75 deletions(-)
>
> diff --git a/gdb/block.c b/gdb/block.c
> index 8e1b6ec88d4..6ada69c388f 100644
> --- a/gdb/block.c
> +++ b/gdb/block.c
> @@ -778,46 +778,25 @@ block_lookup_symbol_primary (const struct block
> *block, const char *name,
>  /* See block.h.  */
>
>  struct symbol *
> -block_find_symbol (const struct block *block, const char *name,
> -                  const domain_enum domain,
> -                  block_symbol_matcher_ftype *matcher, void *data)
> +block_find_symbol (const struct block *block, const lookup_name_info
> &name,
> +                  const domain_enum domain, struct symbol **stub)
>  {
> -  lookup_name_info lookup_name (name, symbol_name_match_type::FULL);
> -
>    /* Verify BLOCK is STATIC_BLOCK or GLOBAL_BLOCK.  */
>    gdb_assert (block->superblock () == NULL
>               || block->superblock ()->superblock () == NULL);
>
> -  for (struct symbol *sym : block_iterator_range (block, &lookup_name))
> +  for (struct symbol *sym : block_iterator_range (block, &name))
>      {
> -      /* MATCHER is deliberately called second here so that it never sees
> -        a non-domain-matching symbol.  */
> -      if (sym->matches (domain)
> -         && matcher (sym, data))
> -       return sym;
> -    }
> -  return NULL;
> -}
> -
> -/* See block.h.  */
> +      if (!sym->matches (domain))
> +       continue;
>
> -int
> -block_find_non_opaque_type (struct symbol *sym, void *data)
> -{
> -  return !TYPE_IS_OPAQUE (sym->type ());
> -}
> -
> -/* See block.h.  */
> -
> -int
> -block_find_non_opaque_type_preferred (struct symbol *sym, void *data)
> -{
> -  struct symbol **best = (struct symbol **) data;
> +      if (!TYPE_IS_OPAQUE (sym->type ()))
> +       return sym;
>
> -  if (!TYPE_IS_OPAQUE (sym->type ()))
> -    return 1;
> -  *best = sym;
> -  return 0;
> +      if (stub != nullptr)
> +       *stub = sym;
> +    }
> +  return nullptr;
>  }
>
>  /* See block.h.  */
> diff --git a/gdb/block.h b/gdb/block.h
> index f132d351bb6..3a197e63754 100644
> --- a/gdb/block.h
> +++ b/gdb/block.h
> @@ -556,39 +556,15 @@ extern struct symbol *block_lookup_symbol_primary
> (const struct block *block,
>                                                    const char *name,
>                                                    const domain_enum
> domain);
>
> -/* The type of the MATCHER argument to block_find_symbol.  */
> -
> -typedef int (block_symbol_matcher_ftype) (struct symbol *, void *);
> -
> -/* Find symbol NAME in BLOCK and in DOMAIN that satisfies MATCHER.
> -   DATA is passed unchanged to MATCHER.
> -   BLOCK must be STATIC_BLOCK or GLOBAL_BLOCK.  */
> +/* Find symbol NAME in BLOCK and in DOMAIN.  This will return a
> +   matching symbol whose type is not a "opaque", see TYPE_IS_OPAQUE.
> +   If STUB is non-NULL, an otherwise matching symbol whose type is a
> +   opaque will be stored here.  */
>
>  extern struct symbol *block_find_symbol (const struct block *block,
> -                                        const char *name,
> +                                        const lookup_name_info &name,
>                                          const domain_enum domain,
> -                                        block_symbol_matcher_ftype
> *matcher,
> -                                        void *data);
> -
> -/* A matcher function for block_find_symbol to find only symbols with
> -   non-opaque types.  */
> -
> -extern int block_find_non_opaque_type (struct symbol *sym, void *data);
> -
> -/* A matcher function for block_find_symbol to prefer symbols with
> -   non-opaque types.  The way to use this function is as follows:
> -
> -   struct symbol *with_opaque = NULL;
> -   struct symbol *sym
> -     = block_find_symbol (block, name, domain,
> -                         block_find_non_opaque_type_preferred,
> &with_opaque);
> -
> -   At this point if SYM is non-NULL then a non-opaque type has been found.
> -   Otherwise, if WITH_OPAQUE is non-NULL then an opaque type has been
> found.
> -   Otherwise, the symbol was not found.  */
> -
> -extern int block_find_non_opaque_type_preferred (struct symbol *sym,
> -                                                void *data);
> +                                        struct symbol **stub);
>
>  /* Given a vector of pairs, allocate and build an obstack allocated
>     blockranges struct for a block.  */
> diff --git a/gdb/symfile-debug.c b/gdb/symfile-debug.c
> index 9db5c47a8ce..233cf3390e5 100644
> --- a/gdb/symfile-debug.c
> +++ b/gdb/symfile-debug.c
> @@ -260,23 +260,19 @@ objfile::lookup_symbol (block_enum kind, const char
> *name, domain_enum domain)
>      const struct blockvector *bv = stab->blockvector ();
>      const struct block *block = bv->block (kind);
>
> -    sym = block_find_symbol (block, name, domain,
> -                            block_find_non_opaque_type_preferred,
> -                            &with_opaque);
> +    sym = block_find_symbol (block, lookup_name, domain, &with_opaque);
>
>      /* Some caution must be observed with overloaded functions
>         and methods, since the index will not contain any overload
>         information (but NAME might contain it).  */
>
> -    if (sym != NULL
> -       && symbol_matches_search_name (sym, lookup_name))
> +    if (sym != nullptr)
>        {
>         retval = stab;
>         /* Found it.  */
>         return false;
>        }
> -    if (with_opaque != NULL
> -       && symbol_matches_search_name (with_opaque, lookup_name))
> +    if (with_opaque != nullptr)
>        retval = stab;
>
>      /* Keep looking through other psymtabs.  */
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index d8ce2bf8482..29ca418d1e9 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -2675,9 +2675,10 @@ basic_lookup_transparent_type_quick (struct objfile
> *objfile,
>
>    bv = cust->blockvector ();
>    block = bv->block (block_index);
> -  sym = block_find_symbol (block, name, STRUCT_DOMAIN,
> -                          block_find_non_opaque_type, NULL);
> -  if (sym == NULL)
> +
> +  lookup_name_info lookup_name (name, symbol_name_match_type::FULL);
> +  sym = block_find_symbol (block, lookup_name, STRUCT_DOMAIN, nullptr);
> +  if (sym == nullptr)
>      error_in_psymtab_expansion (block_index, name, cust);
>    gdb_assert (!TYPE_IS_OPAQUE (sym->type ()));
>    return sym->type ();
> @@ -2696,13 +2697,13 @@ basic_lookup_transparent_type_1 (struct objfile
> *objfile,
>    const struct block *block;
>    const struct symbol *sym;
>
> +  lookup_name_info lookup_name (name, symbol_name_match_type::FULL);
>    for (compunit_symtab *cust : objfile->compunits ())
>      {
>        bv = cust->blockvector ();
>        block = bv->block (block_index);
> -      sym = block_find_symbol (block, name, STRUCT_DOMAIN,
> -                              block_find_non_opaque_type, NULL);
> -      if (sym != NULL)
> +      sym = block_find_symbol (block, lookup_name, STRUCT_DOMAIN,
> nullptr);
> +      if (sym != nullptr)
>         {
>           gdb_assert (!TYPE_IS_OPAQUE (sym->type ()));
>           return sym->type ();
> --
> 2.41.0
>
>
Things are simpler and nicer this way indeed. I tested this on ppc64le
Fedora Rawhide and I can see no regressions.