Re: [rfa] delete 'force_return' from lookup_symbol_aux_minsyms

Elena Zannoni
2002-12-19T22:33:21Z

David Carlton writes:
> 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.
>

Yes, I agree with this interpretation.

> 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.
>

yes

> 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.
>

ok...

> Except that isn't right, either! Because SYMBOL_NAME (msymbol) would
> be the mangled name of 'msymbol',

yes,

> and lookup_symbol demangled it, so

ah right, it would become modified_name

> the 'name' argument to lookup_symbol_aux_minsyms would actually be
> demangled.

ok, I think I follow up to here.

> So, indeed, we might well be in a situation where
> force_return comes into play.
>

I am lost now.

> 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.
>

tempting

> 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>.
>

I think it's just broken. The call in search_symbols predates the
introduction of lookup_symbol_aux and the demangling logic. So I think
it just wasn't updated to reflect the new behavior.

> 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".
>

Yeah, I think it's just a coincidence that lookup_symbol is still
called. At the time that call was introduced, lookup_symbol was maybe
the only function available for this sort of things.
We could try to call the lookup_symbol_aux_minsyms function.

> 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.
>

Ok, whatever seems easier for you. Although I think we can try to fix
the parameter problem, at least, and see what breaks.

Elena

> David Carlton
> carlton@math.stanford.edu