Re: [rfa] delete 'force_return' from lookup_symbol_aux_minsyms

[Elena Zannoni <ezannoni@redhat.com>]

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

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