Patchwork Factor out the common code in lookup_{static,global}_symbol

login
register
mail settings
Submitter Doug Evans via gdb-patches
Date Aug. 3, 2019, 1:04 a.m.
Message ID <20190803010414.34872-1-cbiesinger@google.com>
Download mbox | patch
Permalink /patch/33926/
State New
Headers show

Comments

Doug Evans via gdb-patches - Aug. 3, 2019, 1:04 a.m.
Thanks for the suggestion, that does seem better.

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 | 81 +++++++++++++++++++++++-----------------------------
 1 file changed, 36 insertions(+), 45 deletions(-)
Simon Marchi - Aug. 3, 2019, 11:41 p.m.
On 2019-08-02 9:04 p.m., Christian Biesinger via gdb-patches wrote:
> Thanks for the suggestion, that does seem better.
> 
> The two functions are extremely similar; this factors out their code into
> a shared _internal function.

I am reasonably convinced that this is correct.  lookup_static_symbol currently
iterates objfiles by simply iterating the linked list.  Now it will use
gdbarch_iterate_over_objfiles_in_search_order, with current_objfile == NULL.  The
only gdbarch that implements it is Windows (windows_iterate_over_objfiles_in_search_order)
and with current_objfile == NULL, it goes back to iterating the objfiles linked list directly.

So there shouldn't be any functional change.

> 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 | 81 +++++++++++++++++++++++-----------------------------
>  1 file changed, 36 insertions(+), 45 deletions(-)
> 
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 87a0c8e4da..55df92f3e0 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,14 @@ 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)
> +lookup_global_symbol_internal (const char *name,
> +			       enum block_enum block_index,
> +			       const struct block *block,
> +			       const domain_enum domain)

Make this function static.  And to be pedant, add a newline between the doc and function name.

>  {
>    struct symbol_cache *cache = get_symbol_cache (current_program_space);
>    struct block_symbol result;
> @@ -2656,11 +2623,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 +2648,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 +2664,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?  */

That's a good question.  So one of the things using lookup_static_symbol is the
gdb.lookup_static_symbol Python function you recently added.  Right now, it is not
context sensitive.  For example, here I have two files with each a static variable
`allo`, one with value 22 and the other with value 33:

    (gdb) python print(gdb.lookup_static_symbol('allo').value())
    22
    (gdb) print allo
    $1 = 22
    (gdb) s
    fonction () at test2.c:6
    6	    printf("allo = %d\n", allo);
    (gdb) python print(gdb.lookup_static_symbol('allo').value())
    22
    (gdb) print allo
    $2 = 33

As you can see, gdb.lookup_static_symbol will always find the same symbol,
regardless of the current context, unlike `print`, which ends up using lookup_symbol_aux.
Should functions like gdb.lookup_static_symbol implicitly prioritize the current context
(e.g. current CU)?  I think something like that makes sense for the command line of GDB,
used interactively.  But for an API, is it strange to get different results when calling
gdb.lookup_static_symbol with the same parameters at different times?

Simon
Doug Evans via gdb-patches - Aug. 5, 2019, 3:32 p.m.
On Sat, Aug 3, 2019 at 6:41 PM Simon Marchi <simark@simark.ca> wrote:
>
> On 2019-08-02 9:04 p.m., Christian Biesinger via gdb-patches wrote:
> > Thanks for the suggestion, that does seem better.
> >
> > The two functions are extremely similar; this factors out their code into
> > a shared _internal function.
>
> I am reasonably convinced that this is correct.  lookup_static_symbol currently
> iterates objfiles by simply iterating the linked list.  Now it will use
> gdbarch_iterate_over_objfiles_in_search_order, with current_objfile == NULL.  The
> only gdbarch that implements it is Windows (windows_iterate_over_objfiles_in_search_order)
> and with current_objfile == NULL, it goes back to iterating the objfiles linked list directly.
>
> So there shouldn't be any functional change.

Yes, that was my thinking too.

> > 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 | 81 +++++++++++++++++++++++-----------------------------
> >  1 file changed, 36 insertions(+), 45 deletions(-)
> >
> > diff --git a/gdb/symtab.c b/gdb/symtab.c
> > index 87a0c8e4da..55df92f3e0 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,14 @@ 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)
> > +lookup_global_symbol_internal (const char *name,
> > +                            enum block_enum block_index,
> > +                            const struct block *block,
> > +                            const domain_enum domain)
>
> Make this function static.  And to be pedant, add a newline between the doc and function name.

Oops, thanks. Will send a new patch in a moment.

> >  {
> >    struct symbol_cache *cache = get_symbol_cache (current_program_space);
> >    struct block_symbol result;
> > @@ -2656,11 +2623,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 +2648,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 +2664,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?  */
>
> That's a good question.  So one of the things using lookup_static_symbol is the
> gdb.lookup_static_symbol Python function you recently added.  Right now, it is not
> context sensitive.  For example, here I have two files with each a static variable
> `allo`, one with value 22 and the other with value 33:
>
>     (gdb) python print(gdb.lookup_static_symbol('allo').value())
>     22
>     (gdb) print allo
>     $1 = 22
>     (gdb) s
>     fonction () at test2.c:6
>     6       printf("allo = %d\n", allo);
>     (gdb) python print(gdb.lookup_static_symbol('allo').value())
>     22
>     (gdb) print allo
>     $2 = 33
>
> As you can see, gdb.lookup_static_symbol will always find the same symbol,
> regardless of the current context, unlike `print`, which ends up using lookup_symbol_aux.
> Should functions like gdb.lookup_static_symbol implicitly prioritize the current context
> (e.g. current CU)?  I think something like that makes sense for the command line of GDB,
> used interactively.  But for an API, is it strange to get different results when calling
> gdb.lookup_static_symbol with the same parameters at different times?

Yeah, I agree that the python one should not be context-sensitive. But
take, for example, the call to lookup_static_symbol at the end of
lookup_symbol_aux -- shouldn't that take the context into account? For
the one in cp_lookup_nested_symbol_1 I can't quite tell if it makes a
difference, though I think the code there only looks in the current CU
and probably should prefer the current objfile for static symbols as
well. Etc.

Christian
Doug Evans via gdb-patches - Aug. 9, 2019, 3:28 p.m.
On Mon, Aug 5, 2019 at 10:32 AM Christian Biesinger
<cbiesinger@google.com> wrote:
>
> On Sat, Aug 3, 2019 at 6:41 PM Simon Marchi <simark@simark.ca> wrote:
> >
> > On 2019-08-02 9:04 p.m., Christian Biesinger via gdb-patches wrote:
> > > Thanks for the suggestion, that does seem better.
> > >
> > > The two functions are extremely similar; this factors out their code into
> > > a shared _internal function.
> >
> > I am reasonably convinced that this is correct.  lookup_static_symbol currently
> > iterates objfiles by simply iterating the linked list.  Now it will use
> > gdbarch_iterate_over_objfiles_in_search_order, with current_objfile == NULL.  The
> > only gdbarch that implements it is Windows (windows_iterate_over_objfiles_in_search_order)
> > and with current_objfile == NULL, it goes back to iterating the objfiles linked list directly.
> >
> > So there shouldn't be any functional change.
>
> Yes, that was my thinking too.
>
> > > 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 | 81 +++++++++++++++++++++++-----------------------------
> > >  1 file changed, 36 insertions(+), 45 deletions(-)
> > >
> > > diff --git a/gdb/symtab.c b/gdb/symtab.c
> > > index 87a0c8e4da..55df92f3e0 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,14 @@ 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)
> > > +lookup_global_symbol_internal (const char *name,
> > > +                            enum block_enum block_index,
> > > +                            const struct block *block,
> > > +                            const domain_enum domain)
> >
> > Make this function static.  And to be pedant, add a newline between the doc and function name.
>
> Oops, thanks. Will send a new patch in a moment.
>
> > >  {
> > >    struct symbol_cache *cache = get_symbol_cache (current_program_space);
> > >    struct block_symbol result;
> > > @@ -2656,11 +2623,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 +2648,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 +2664,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?  */
> >
> > That's a good question.  So one of the things using lookup_static_symbol is the
> > gdb.lookup_static_symbol Python function you recently added.  Right now, it is not
> > context sensitive.  For example, here I have two files with each a static variable
> > `allo`, one with value 22 and the other with value 33:
> >
> >     (gdb) python print(gdb.lookup_static_symbol('allo').value())
> >     22
> >     (gdb) print allo
> >     $1 = 22
> >     (gdb) s
> >     fonction () at test2.c:6
> >     6       printf("allo = %d\n", allo);
> >     (gdb) python print(gdb.lookup_static_symbol('allo').value())
> >     22
> >     (gdb) print allo
> >     $2 = 33
> >
> > As you can see, gdb.lookup_static_symbol will always find the same symbol,
> > regardless of the current context, unlike `print`, which ends up using lookup_symbol_aux.
> > Should functions like gdb.lookup_static_symbol implicitly prioritize the current context
> > (e.g. current CU)?  I think something like that makes sense for the command line of GDB,
> > used interactively.  But for an API, is it strange to get different results when calling
> > gdb.lookup_static_symbol with the same parameters at different times?
>
> Yeah, I agree that the python one should not be context-sensitive. But
> take, for example, the call to lookup_static_symbol at the end of
> lookup_symbol_aux -- shouldn't that take the context into account? For
> the one in cp_lookup_nested_symbol_1 I can't quite tell if it makes a
> difference, though I think the code there only looks in the current CU
> and probably should prefer the current objfile for static symbols as
> well. Etc.

BTW, if that's the only thing holding up this patch, I'm happy to
remove the comment.

Christian

Patch

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 87a0c8e4da..55df92f3e0 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,14 @@  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)
+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 +2623,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 +2648,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 +2664,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,