Re: [rfa] delete 'force_return' from lookup_symbol_aux_minsyms

David Carlton
2002-12-19T19:44:52Z

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)
{

}
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