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

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

* Christian Biesinger <cbiesinger@google.com> [2019-09-21 13:31:30 +0900]:

> On Sat, Sep 21, 2019 at 4:20 AM Tom Tromey <tromey@adacore.com> wrote:
> > From: Andrew Burgess <andrew.burgess@embecosm.com>
> >
> > This changes lookup_global_symbol 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 further extends the situations where lookup_global_symbols checks
> a local scope first (currently only objfile) but lookup_static_symbol
> doesn't.

[ Note, in the below I talk about "my patch", I really mean mine and
  Tom's as my suggested rework was based on his original patch. ]

I didn't fully understand this for two reasons, first, you say
"further extends", however, both lookup_global_symbol and
lookup_static_symbol pre-patch, both just call
lookup_global_or_static_symbol.  And I couldn't see any code in
lookup_global_or_static_symbol that was conditional on global or
static lookup.  So my question is where is the existing situation in
which lookup_global_symbol searches a local scope first?  I ask only
so I can try to understand your question and my change in relation to
the existing code.

Second, I wanted to better understand what you mean by "local scope".
My understanding of lookup_global_symbol is that it searches every
global block from every object file in some particular order.  The
change here is simply that it makes more sense to search the global
scope relating to the current compilation unit before searching other
global scopes.  I can fully understand that this can be referred to as
a "local scope", I just wanted to make sure we're all agreed on what's
happening here.

> Is that really correct?

I think it makes sense.  If we have multiple global symbols with the
same name, we should find the one corresponding to the current scope I
think.  That feels like what I'd expect to happen.  And especially as
when we do a FILE::VAR lookup we end up in lookup_global_symbol with
the block corresponding to FILE.  If we then ignore that block and
search all global scopes in the predefined order then we will always
find the same global symbol, which is certainly wrong.

> I would think that
> lookup_static_symbol is even more likely to need that check, since
> static symbols are probably (?) more likely to share the same name. Is
> my interpretation wrong?

No, I think you are very right....

But...

There are currently 5 uses of lookup_static_symbol that I could find,
2 of these are in:

  d-namespace.c:find_symbol_in_baseclass
  cp-namespace.c:cp_lookup_nested_symbol_1

These are interesting because almost immediately before calling
lookup_static_symbol we call lookup_symbol_in_static_block, which I
think is solving the same problem as this proposed patch.

Then we have a call to lookup_static_symbol in:

  symtab.c:lookup_symbol_aux

This calls lookup_static_symbol as the last fallback action after
calling the language specific hook la_lookup_symbol_nonlocal.  I've
only audited some languages, but for those I've looked at they all
call lookup_symbol_in_static_block as one of their early actions:

  symtab.c:basic_lookup_symbol_nonlocal		(for C, Pascal)
  cp-namespace.c:cp_lookup_bare_symbol		(for C++, Fortran)
  cp-namespace.c:cp_lookup_bare_symbol		(for Fortran)
  d-namespace.c:d_lookup_symbol			(for D)
  rust-lang.c:rust_lookup_symbol_nonlocal	(for Rust)

That leaves two uses of lookup_static_symbol, these are:

  python/py-symbol.c:gdbpy_lookup_static_symbol
  d-namespace.c:d_lookup_nested_symbol

In these cases there is no call to lookup_symbol_in_static_block. I
would need to investigate more to see if these are working as expected
or not.  I suspect the python use case might not always do what a user
expects, as it searches static symbols in a predefined order, if we
have multiple with the same name we would always find the same one
first, but we'd probably expect to find one from the current object
file before one from a different object file.  As for the D use case,
I don't know D, so don't feel qualified to judge.

I think my conclusion is that you're right, but, refactoring this code
to have lookup_static_symbol call lookup_symbol_in_static_block (or
equivalent in all cases) seems a pretty scary change.  I'd ideally
like to see that refactoring separated from this patch series.

My vote would be to merge this, and then, possibly we can look at
reworking symbol lookup inline with your suggestion.  What do you
think?

Thanks,
Andrew