Re: [rfa] delete 'force_return' from lookup_symbol_aux_minsyms

[David Carlton <carlton at math dot stanford dot edu>]

On Thu, 19 Dec 2002 11:50:36 -0500, Elena Zannoni <ezannoni@redhat.com> said:
> David Carlton writes:

>> 'found_misc' also seems to have gone away; 

> Actually it is still there, the code I posted is for
> list/search_symbols.

Oh, I apologize, I completely misunderstood what you were saying.  I
thought that the code you dug up was an earlier version of
lookup_symbol.

Now I understand your point: I was claiming that no callers of
lookup_symbol depended on this NULL return that I'm trying to get rid
of, but you suspect that search_symbols might be such a caller.  And,
indeed, I hadn't considered that particular caller.

Looking into it further, I think you might have reason to worry.
Here's the relevant section of search_symbols:

  if (0 == find_pc_symtab (SYMBOL_VALUE_ADDRESS (msymbol)))
    {
      if (kind == FUNCTIONS_NAMESPACE
	  || lookup_symbol (SYMBOL_NAME (msymbol),
                            (struct block *) NULL,
                            VAR_NAMESPACE,
                            0, (struct symtab **) NULL) == NULL)
        found_misc = 1;
    }

And here's an (abbreviated) version of all of the uses of force_return
in lookup_symbol_aux_minsyms:

  s = find_pc_sect_symtab (SYMBOL_VALUE_ADDRESS (msymbol),
			   SYMBOL_BFD_SECTION (msymbol));
  if (s != NULL)
    {
      <code deleted that might set *force_return to 1>
    }
  else if (MSYMBOL_TYPE (msymbol) != mst_text
           && MSYMBOL_TYPE (msymbol) != mst_file_text
           && !STREQ (name, SYMBOL_NAME (msymbol)))
    {
      /* This is a mangled variable, look it up by its
         mangled name.  */
      *force_return = 1;
      return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name,
                                NULL, namespace, is_a_field_of_this,
                                symtab);
    }

These two sections of code are remarkably parallel, for reasons that I
don't understand.  And they're clearly trying to investigate the same
minimal symbol: the point of that code in search_symbols it to
determine whether or not a particular minimal symbol has debugging
info, so the question at hand is whether or not lookup_symbol is
supposed to terminate when it hits the minimal symbol that
search_symbols is interested in.  So I think it's safe to assume that
we're only interested in comparing the two pieces of code when
'msymbol' has the same value in both.

In that case, the find_pc_symtab in search_symbols corresponds to the
find_pc_sect_symtab in lookup_symbol_aux_minsyms (and should probably
be changed to find_pc_sect_symtab, but never mind that for now).
We're assuming that that symtab is 0; that means that we're in the
else clause of the lookup_symbol_aux_minsyms call.

So when does that else clause happens?  It's guarded by a conditional:
that tests that the MSYMBOL_TYPE of the msymbol isn't text, and that
the name we're searching under isn't the SYMBOL_NAME of the msymbol.

The former condition is true: we're assuming that kind isn't
FUNCTIONS_NAMESPACE, so the minimal symbol shouldn't be text.  But
we've called lookup_symbol with the 'name' argument equal to
SYMBOL_NAME (msymbol).  And, in that case, the test for

  !STREQ (name, SYMBOL_NAME (msymbol)

should fail.

Except that isn't right, either!  Because SYMBOL_NAME (msymbol) would
be the mangled name of 'msymbol', and lookup_symbol demangled it, so
the 'name' argument to lookup_symbol_aux_minsyms would actually be
demangled.  So, indeed, we might well be in a situation where
force_return comes into play.

Phew.  Assuming that analysis is correct, I have two comments and a
suggestion.

Comment #1: This whole logic is hopelessly unclear.  I think I'd
rather turn the code into something possibly broken but clearer, and
then try to fix any possible breakage, than try to leave it as is.

Comment #2: Just what is up with the call to lookup_symbol_aux from
within lookup_symbol_aux_minsyms, anyways?  It's passing in a mangled
name as the first argument; but lookup_symbol_aux normally expects its
first argument to be demangled.  I'm not sure what's going on here:
that call might be broken, or there might be some part of
lookup_symbol_aux that does something with a mangled first argument.
If the latter is the case, then there should be comments making this
explicit, and the call to lookup_symbol_aux should be replaced by a
call to lookup_symbol_aux_<something>.

Suggestion: The whole purpose of the call to lookup_symbol from within
search_symbols is to try to track down information about one
particular minimal symbol: does it have debugging information, or
doesn't it?  Given that that's the case, doing that via lookup_symbol
is at best overkill and at worst wrong.  This suggests that we might
be able to get around the issue by replacing that call to
lookup_symbol by a call to lookup_symbol_aux_minsyms.  The only
question that I have is whether the first argument should then be the
mangled name of 'msymbol' or the demangled name; I'd be happier if we
understood the situation with respect to my "Comment #2".

A datum which may or may not be relevant: currently, partial symbols
never have their names demangled.  I'd assumed that this was a bug;
but I just noticed the following comment in search_symbols:

  This is in particular necessary for demangled variable names,
  which are no longer put into the partial symbol tables.

Sigh.  So I have another suggestion:

Suggestion #2: Maybe we should put this particular patch on hold and
come to some sort of consensus as to how to deal with
mangled/demangled names.  I'll post an RFC for that later today.

David Carlton
carlton@math.stanford.edu