Re: [rfa] more lookup_symbol_aux_minsyms futzing

David Carlton
2003-02-20T20:57:55Z

On Thu, 20 Feb 2003 15:08:19 -0500, Elena Zannoni <ezannoni@redhat.com> said:

> A few comments

Ack! I was considering this patch to be withdrawn, and I hadn't
noticed that it got added to GNATS. Right now, all I'm proposing is
the search_symbols part of this patch (with a comment added), as in PR
symtab/1049. (Incidentally, I'm curious if Daniel considers the patch
in symtab/1070 to still be active: if so, please review the e-mail
discussion that Daniel and I had about this. I actually have more to
say about that if you're considering approving that patch.)

Having said that, I'm glad that you're thinking about this, because I
am soon planning to propose a similar (or maybe identical) patch to
modify lookup_symbol_aux_minsyms. So here's a discussion of facts
relevant to a future version of this patch.

At the time that I proposed this patch originally, I didn't understand
the history of symbols and partial symbols very well. At the time I
proposed this patch, I had assumed that partial symbol were supposed
to be demangled, and that the fact that they weren't was a bug. But
then Peter Schauer kindly explained more of the history of partial
symbols and demangling; I have a _much_ clearer picture of the
situation now, and I believe I understand pretty much all of the
details of lookup_symbol_aux.

For a long time, partial symbols were intentionally left mangled,
because demangling partial symbols posed an unacceptable time cost
(and a less important but nontrivial memory cost). This led to a
problem in lookup_symbol: lookup_symbol has to get passed a demangled
name (after all, it usually derives from something the user types),
but the symbol that it's supposed to find might be in a psymtab that
hasn't yet been converted to a symtab, in which case we need access to
the mangled name to find it. So how can we find that symbol?

The solution is this: if a symbol's mangled name and demangled name
are different, then there must be a minimal symbol associated to it.
(After all, name mangling is only done in order to make the linker
happy: and that's exactly where minimal symbols come into play.) And,
while we weren't demangling names of partial symbols, we were
demangling names of minimal symbols. So the logic (once we get to the
"look for global symbols part" of lookup_symbol_aux) is:

1) Look in symtabs. Demangled names are okay here.

2) Now we want to look in psymtabs, but we can't: we have a demangled
name, and we need a mangled name to look in symtabs. The solution
then is:

3) Use the demangled name to find a minimal symbol, if we can. If we
do find a minimal symbol, then we might be able to find the symtab
directly (if the symbol is associated to a function), or we might
need to get the mangled name from the minimal symbol.

4) Once we have the mangled name from the minimal symbol, we can
proceed with partial symbol lookup; we do this via a recursive call
to lookup_symbol_aux.

5) If we didn't find a minimal symbol in step 3, then maybe we didn't
need a mangled name; in that case, just proceed along to the
partial symbol check, using the demangled name.

In particular, I'm now confident that the role for minimal symbols is
to do demangled->mangled name translation; any other use of minimal
symbols is either an optimization or an unintended side effect. Of
course, there weren't comments actually _explaining_ this, and the
resulting control flow depended on some unclear and subtle
assumptions. It broke code in the HP case; and everything broke when
it was no longer possible to pass lookup_symbol_aux a mangled name
instead of a demangled name.

But now partial symbols store demangled names in a pleasantly
efficient way, thanks to Daniel's recent patch. Which means that the
whole raison d'etre of using minimal symbols in lookup_symbol_aux is
no more. This has two consequences:

1) The second part of lookup_symbol_aux_minsyms should definitely go.
It's caused problems in the past, it's causing problems right now,
and it's entirely superfluous: whatever consequences it might have
now are entirely negative ones.

2) The rest of lookup_symbol_aux_minsyms is unnecessary. It may be an
optimization; it may be a pessimization. It's certainly a
pessimization in some circumstances; given that it complicates the
function in question, we should get rid of it unless we can find
situations where it makes a real difference to the good. My guess
is that there are no such situations, but that's only a guess; I
have some ideas about how to test this, so I'll do some timings and
come back with a recommendation.

So if, for now, you could just give PR symtab/1049 a scan and approve
it (it shouldn't be too controversial, I hope), I'd appreciate it.
And think about the above, just to make sure that I'm not sounding
crazy. Once it's approved (and once I have a spare moment!), I'll
then submit two patches:

1) A patch to correct a slight bug that remains in
lookup_partial_symbol. Basically, partial symbols are sorted via
strcmp but we want to use strcmp_iw as our matching criterion;
strcmp and strcmp_iw aren't _quite_ suitable to be used together in
this way.

2) A patch to either get rid of the second half of
lookup_symbol_aux_minsyms or to get rid of all of it, depending on
whether or not I can find a situation where it helps to leave it in
place.