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

Simon Marchi simark@simark.ca
Sat Aug 3 23:41:00 GMT 2019


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



More information about the Gdb-patches mailing list