Re: [rfa] more lookup_symbol_aux_minsyms futzing

Elena Zannoni
2003-02-20T20:04:18Z

A few comments

David Carlton writes:
> Here's another patch involving lookup_symbol_aux_minsyms. It reverts
> the search_symbols part of my last patch, and deletes the last "else
> if" part of lookup_symbol_aux_minsyms. (And it deletes the
> 'is_a_field_of_this' argument, since that's was only used in that
> "else if" part.)
>
> I'd been ascribing more power to lookup_symbol_aux_minsyms than it
> actually had: I had assumed that it did a reasonable job of finding a
> symbol corresponding to a minimal symbol whenever there was one, but
> now I think that it only does that in the function case. Furthermore,
> it seems to me that whatever it tries to do in the non-function case
> is more likely to be hazardous than helpful. (Or at least was more
> likely before 'force_return' got deleted.)
>
> This has two implications:
>
> 1) Don't count on lookup_symbol_aux_minsyms doing anything useful in
> the non-function case. In particular, my search_symbols patch from
> before was bad.
>
> 2) Get rid of the non-function case of lookup_symbol_aux_minsyms: it
> just confuses the issue and slows down symbol lookup.
>
> Let me analyze lookup_symbol_aux_minsyms in a bit more detail, to back
> this up. It looks like this, stripped down:
>
> if (namespace == VAR_NAMESPACE)
> {
> msymbol = lookup_minimal_symbol (name, NULL, NULL);
>
> if (msymbol != NULL)
> {
> s = find_pc_sect_symtab (SYMBOL_VALUE_ADDRESS (msymbol),
> SYMBOL_BFD_SECTION (msymbol));
> if (s != NULL)
> {
> /* Find the correct symbol, and return it. */
> }
> 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. */
> return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name,
> NULL, namespace, is_a_field_of_this,
> symtab);
> }
> }
> }
>
> return NULL;
>
> Now, find_pc_sect_symtab only works if the symbol in question
> corresponds to a function, loosely speaking: it finds the right
> minimal symbol (which will be 'msymbol' above) and then immediately
> returns NULL if that symbol doesn't have type mst_text or
> mst_file_text. So the whole bit that I've marked by "Find the correct
> symbol, and return it" only happens if we're looking for a function.
>

I think this deserves a clearer comment. It took me a while to refigure
this out. Also, I suggest changing the name of the function to
reflect this. lookup_symbol_aux_func?

> So what happens in the "else if" case? This checks that we're in the
> non-function case, and that the name we were passed isn't the
> SYMBOL_NAME of msymbol, which means that msymbol has a mangled name
> while we were passed the demangled name. In this case, it calles
> lookup_symbol_aux with a mangled name as the 'name' argument. So what
> happens in this nested call to lookup_symbol_aux?
>
> 1) lookup_symbol_aux_local. This doesn't happen because 'block' is
> NULL.

Micro optimization: should we conditionalize the call on 'block', and
initialize 'static_block' to NULL. Just like the call to
lookup_symbol_aux_block is.

>
> 2) The 'is_a_field_of_this' check. It makes no sense to do this now:
> we already did it once! So we should have passed NULL in place of
> 'is_a_field_of_this': oops.
>

however, because of the oddities in this recursive call, in the first
invocation the is_a_field_of_this check was done using 'name' (ie the
demangled name), while now we redo it using SYMBOL_NAME(msymbol) which
would be the mangled one. does it matter?

> 3) The static block check; again, 'block' is NULL, so this doesn't
> happen.
>

ok

> 4) The 'lookup_symbol_aux_symtabs' check. The symtab search
> eventually comes down to calling lookup_block_symbol a bunch of
> times, with the mangled name as its 'name' argument. But, as Jim
> Blandy noted in
> <http://sources.redhat.com/ml/gdb-patches/2002-10/msg00040.html>,
> doing that makes no sense: it won't find a match in the hash
> table. So that's not useful.
>

what happened to that patch? did it ever get committed? I wonder if it
would have helped in Jason's case.

> 5) The 'lookup_symbol_aux_minsyms' check. That's not going to uncover
> anything the second time that it didn't uncover the first time it
> was called.
>
> 6) The 'lookup_symbol_aux_psymtabs' check. This tries to find the
> right partial symbol table, read in the corresponding symbol table,
> and then search it with lookup_block_symbol. But, again, calling
> lookup_block_symbol with the mangled name as the 'name' argument
> makes no sense.
>

right, in this invocation of lookup_symbol_aux the two parameters name
and mangled_name are the same.

> So in no case is it correct for lookup_symbol_aux_minsyms to call
> lookup_symbol_aux with the demangled name as the 'name' argument. So
> I think we should just delete that call.
>
> There is one thing that bothers me: right now, partial symbols don't
> include demangled names. So that call to lookup_symbol_aux_psymtabs
> with the demangled name might actually cause the correct symtab to be
> read in, even if the lookup_block_symbol fails. The correct thing to
> do here is to store demangled names in partial symbols, however, not
> to depend on an undocumented side effect of an incorrect psymtab
> search. I'll submit a patch for that next.
>
> David Carlton
> carlton@math.stanford.edu
>
> 2002-12-23 David Carlton <carlton@math.stanford.edu>
>
> * symtab.c (search_symbols): Change call to
> lookup_symbol_aux_minsyms back to cal to lookup_symbol, reverting
> previous patch.
> (lookup_symbol_aux_minsyms): Don't search on mangled names; delete
> 'is_a_field_of_this' argument.
> (lookup_symbol_aux): Don't pass 'is_a_field_of_this' to
> lookup_symbol_aux_minsyms.
>
> Index: symtab.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/symtab.c,v
> retrieving revision 1.83
> diff -u -p -r1.83 symtab.c
> --- symtab.c 23 Dec 2002 16:43:18 -0000 1.83
> +++ symtab.c 23 Dec 2002 18:03:03 -0000
> @@ -116,7 +116,6 @@ static
> struct symbol *lookup_symbol_aux_minsyms (const char *name,
> const char *mangled_name,
> const namespace_enum namespace,
> - int *is_a_field_of_this,
> struct symtab **symtab);
>
> static struct symbol *find_active_alias (struct symbol *sym, CORE_ADDR addr);
> @@ -923,8 +922,7 @@ lookup_symbol_aux (const char *name, con
> Eventually, all global symbols might be resolved in this way. */
>
> sym = lookup_symbol_aux_minsyms (name, mangled_name,
> - namespace, is_a_field_of_this,
> - symtab);
> + namespace, symtab);
>
> if (sym != NULL)
> return sym;
> @@ -971,8 +969,7 @@ lookup_symbol_aux (const char *name, con
>
>
> sym = lookup_symbol_aux_minsyms (name, mangled_name,
> - namespace, is_a_field_of_this,
> - symtab);
> + namespace, symtab);
>
> if (sym != NULL)
> return sym;
> @@ -1154,24 +1151,20 @@ lookup_symbol_aux_psymtabs (int block_in
> return NULL;
> }
>
> -/* Check for the possibility of the symbol being a function or a
> - mangled variable that is stored in one of the minimal symbol
> - tables. Eventually, all global symbols might be resolved in this
> - way. */
> -
> -/* NOTE: carlton/2002-12-05: At one point, this function was part of
> - lookup_symbol_aux, and what are now 'return' statements within
> - lookup_symbol_aux_minsyms returned from lookup_symbol_aux, even if
> - sym was NULL. As far as I can tell, this was basically accidental;
> - it didn't happen every time that msymbol was non-NULL, but only if
> - some additional conditions held as well, and it caused problems
> - with HP-generated symbol tables. */
> +/* Check for the possibility of the symbol being a function that is
> + stored in one of the minimal symbol tables. */
> +
> +/* NOTE: carlton/2002-12-23: At one point, when this function was part
> + of lookup_symbol_aux, its behavior was different, for reasons that
> + are unclear: it forced a return from lookup_symbol_aux at times
> + even without checking the partial symbol tables, and it tried to do
> + something strange involving mangled names in the non-function
> + case. */
>
> static struct symbol *
> lookup_symbol_aux_minsyms (const char *name,
> const char *mangled_name,
> const namespace_enum namespace,
> - int *is_a_field_of_this,
> struct symtab **symtab)
> {
> struct symbol *sym;
> @@ -1186,20 +1179,15 @@ lookup_symbol_aux_minsyms (const char *n
>
> if (msymbol != NULL)
> {
> - /* OK, we found a minimal symbol in spite of not finding any
> - symbol. There are various possible explanations for
> - this. One possibility is the symbol exists in code not
> - compiled -g. Another possibility is that the 'psymtab'
> - isn't doing its job. A third possibility, related to #2,
> - is that we were confused by name-mangling. For instance,
> - maybe the psymtab isn't doing its job because it only
> - know about demangled names, but we were given a mangled
> - name... */
> -
> - /* We first use the address in the msymbol to try to locate
> - the appropriate symtab. Note that find_pc_sect_symtab()
> - has a side-effect of doing psymtab-to-symtab expansion,
> - for the found symtab. */
> + /* We found a minimal symbol in spite of not finding a
> + symbol. This probably means that the symbol in question
> + is in a partial symbol table that hasn't been loaded, but
> + it may mean that it's from code compiled without -g.
> + Partial symbol table searches are kind of expensive; if
> + the symbol corresponds to a function, we can find the
> + appropriate symtab more quickly by calling
> + find_pc_sect_symtab, so let's try that. */
> +
> s = find_pc_sect_symtab (SYMBOL_VALUE_ADDRESS (msymbol),
> SYMBOL_BFD_SECTION (msymbol));
> if (s != NULL)
> @@ -1267,16 +1255,6 @@ lookup_symbol_aux_minsyms (const char *n
> *symtab = s;
> return fixup_symbol_section (sym, s->objfile);
> }
> - 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. */
> - return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name,
> - NULL, namespace, is_a_field_of_this,
> - symtab);
> - }
> }
> }
>
> @@ -2896,31 +2874,12 @@ search_symbols (char *regexp, namespace_
> {
> if (0 == find_pc_symtab (SYMBOL_VALUE_ADDRESS (msymbol)))
> {
> - if (kind == FUNCTIONS_NAMESPACE)
> - {
> - found_misc = 1;
> - }
> - else
> - {
> - struct symbol *sym;
> -
> - if (SYMBOL_DEMANGLED_NAME (msymbol) != NULL)
> - sym
> - = lookup_symbol_aux_minsyms (SYMBOL_DEMANGLED_NAME
> - (msymbol),
> - SYMBOL_NAME (msymbol),
> - VAR_NAMESPACE,
> - NULL, NULL);
> - else
> - sym
> - = lookup_symbol_aux_minsyms (SYMBOL_NAME (msymbol),
> - NULL,
> - VAR_NAMESPACE,
> - NULL, NULL);
> -
> - if (sym == NULL)
> - found_misc = 1;
> - }
> + if (kind == FUNCTIONS_NAMESPACE
> + || lookup_symbol (SYMBOL_NAME (msymbol),
> + (struct block *) NULL,
> + VAR_NAMESPACE,
> + 0, (struct symtab **) NULL) == NULL)
> + found_misc = 1;
> }
> }
> }