Re: [PATCH v2 4/8] Search global block from basic_lookup_symbol_nonlocal

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Tom Tromey <tromey@adacore.com> [2019-08-01 11:04:08 -0600]:

> This changes basic_lookup_symbol_nonlocal to look in the global block
> of the passed-in block.  If no block was passed in, it reverts to the
> previous behavior.
> 
> This change is needed to ensure that 'FILENAME'::NAME lookups work
> properly.  As debugging Pedro's test case showed, this was not working
> properly in the case where multiple identical names could be found
> (the one situation where this feature is truly needed :-).
> 
> This also removes some old comments from basic_lookup_symbol_nonlocal
> that no longer apply once this patch goes in.

So I guess the tests for this are going to be in the
gdb.base/print-file-var.exp changes that are part of patch #8.  It
would be great if the commit message could mention this - it just
makes life easier later on.

I wonder if we need to update other *_lookup_symbol_nonlocal functions
in a similar way?  For example can the C tests be compiled as C++,
which should cause GDB to use cp_lookup_symbol_nonlocal.

Looking at both basic_lookup_symbol_nonlocal and
cp_lookup_symbol_nonlocal, I wonder if your fix could be moved into
lookup_global_symbol?  And just have 'block_global_block (block)'
checked before the search of all global blocks?

Some other languages provide their own *_lookup_symbol_nonlocal, I
don't know if these would also need fixing.

Thanks,
Andrew


> 
> gdb/ChangeLog
> 2019-08-01  Tom Tromey  <tromey@adacore.com>
> 
> 	* symtab.c (basic_lookup_symbol_nonlocal): Search global block.
> 	Remove old comments.
> ---
>  gdb/ChangeLog |  5 +++++
>  gdb/symtab.c  | 44 ++++++++++++++++----------------------------
>  2 files changed, 21 insertions(+), 28 deletions(-)
> 
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 0ff212e0d97..b8f33509c09 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -2417,34 +2417,6 @@ basic_lookup_symbol_nonlocal (const struct language_defn *langdef,
>  {
>    struct block_symbol result;
>  
> -  /* NOTE: carlton/2003-05-19: The comments below were written when
> -     this (or what turned into this) was part of lookup_symbol_aux;
> -     I'm much less worried about these questions now, since these
> -     decisions have turned out well, but I leave these comments here
> -     for posterity.  */
> -
> -  /* NOTE: carlton/2002-12-05: There is a question as to whether or
> -     not it would be appropriate to search the current global block
> -     here as well.  (That's what this code used to do before the
> -     is_a_field_of_this check was moved up.)  On the one hand, it's
> -     redundant with the lookup in all objfiles search that happens
> -     next.  On the other hand, if decode_line_1 is passed an argument
> -     like filename:var, then the user presumably wants 'var' to be
> -     searched for in filename.  On the third hand, there shouldn't be
> -     multiple global variables all of which are named 'var', and it's
> -     not like decode_line_1 has ever restricted its search to only
> -     global variables in a single filename.  All in all, only
> -     searching the static block here seems best: it's correct and it's
> -     cleanest.  */
> -
> -  /* NOTE: carlton/2002-12-05: There's also a possible performance
> -     issue here: if you usually search for global symbols in the
> -     current file, then it would be slightly better to search the
> -     current global block before searching all the symtabs.  But there
> -     are other factors that have a much greater effect on performance
> -     than that one, so I don't think we should worry about that for
> -     now.  */
> -
>    /* NOTE: dje/2014-10-26: The lookup in all objfiles search could skip
>       the current objfile.  Searching the current objfile first is useful
>       for both matching user expectations as well as performance.  */
> @@ -2453,6 +2425,22 @@ basic_lookup_symbol_nonlocal (const struct language_defn *langdef,
>    if (result.symbol != NULL)
>      return result;
>  
> +  /* If a block was passed in, we want to search the corresponding
> +     global block now.  This yields "more expected" behavior, and is
> +     needed to support 'FILENAME'::VARIABLE lookups.  */
> +  const struct block *global_block = block_global_block (block);
> +  if (global_block != nullptr)
> +    {
> +      result.symbol = lookup_symbol_in_block (name,
> +					      symbol_name_match_type::FULL,
> +					      global_block, domain);
> +      if (result.symbol != nullptr)
> +	{
> +	  result.block = global_block;
> +	  return result;
> +	}
> +    }
> +
>    /* If we didn't find a definition for a builtin type in the static block,
>       search for it now.  This is actually the right thing to do and can be
>       a massive performance win.  E.g., when debugging a program with lots of
> -- 
> 2.20.1
>