Re: [rfa] delete 'force_return' from lookup_symbol_aux_minsyms

Elena Zannoni
2002-12-09T21:15:13Z

David Carlton writes:
> This patch is a short one: it deletes the 'force_return' argument of
> lookup_symbol_aux_minsyms. That argument was there because, when the
> code in lookup_symbol_aux_minsyms was part of lookup_symbol_aux, the
> return statements would sometimes cause lookup_symbol_aux to return a
> NULL value without checking the psymtabs first.

I am tempted to accept it, but could you first look at the
archeological diggings below?

>
> I don't think that behavior was either intentional or desirable.
> There's not much rhyme or reason to when this happens: in particular,
> it doesn't happen every time lookup_symbol_aux_minsyms finds a minsym.
> Instead, it is necessary, in addition, either for there to be a symtab at
> the appropriate address or for the symtab to be NULL but for the
> following test to hold:
>
> else if (MSYMBOL_TYPE (msymbol) != mst_text
> && MSYMBOL_TYPE (msymbol) != mst_file_text
> && !STREQ (name, SYMBOL_NAME (msymbol)))
>
> (I actually experimented with trying to have lookup_symbol _always_
> return NULL if a minsym was found without a corresponding symbol, and
> that breaks GDB.) I can't imagine that there are callers of
> lookup_symbol that depend on having it return NULL in these particular
> circumstances; it's certainly not documented anywhere. And I suspect

I see that this bit:

+ 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 (SYMBOL_NAME (msymbol), block,
+ namespace, is_a_field_of_this, symtab);
+ }
+ /* There are no debug symbols for this file, or we are looking
+ for an unmangled variable.
+ Try to find a matching static symbol below. */

and this bit:

@@ -2629,13 +2684,20 @@ list_symbols (regexp, class, bpt, from_t
}
}

- /* Here, we search through the minimal symbol tables for functions that
- match, and call find_pc_symtab on them to force their symbols to
- be read. The symbol will then be found during the scan of symtabs
- below. If find_pc_symtab fails, set found_misc so that we will
- rescan to print any matching symbols without debug info. */
+ /* Here, we search through the minimal symbol tables for functions
+ and variables that match, and force their symbols to be read.
+ This is in particular necessary for demangled variable names,
+ which are no longer put into the partial symbol tables.
+ The symbol will then be found during the scan of symtabs below.
+
+ For functions, find_pc_symtab should succeed if we have debug info
+ for the function, for variables we have to call lookup_symbol
+ to determine if the variable has debug info.
+ If the lookup fails, set found_misc so that we will rescan to print
+ any matching symbols without debug info.
+ */

- if (class == 1)
+ if (class == 0 || class == 1)
{
ALL_MSYMBOLS (objfile, msymbol)
{
@@ -2648,7 +2710,12 @@ list_symbols (regexp, class, bpt, from_t
{
if (0 == find_pc_symtab (SYMBOL_VALUE_ADDRESS (msymbol)))
{
- found_misc = 1;
+ if (class == 1
+ || lookup_symbol (SYMBOL_NAME (msymbol),
+ (struct block *) NULL,
+ VAR_NAMESPACE,
+ 0, (struct symtab **) NULL) == NULL)
+ found_misc = 1;
}
}
}

were added together in 94. the changelog was:

date: 1994/10/08 11:54:20; author: schauer; state: Exp; lines: +87 -20
Speed up GDB startup time by not demangling partial symbols.
* symfile.h (ADD_PSYMBOL_VT_TO_LIST),
symfile.c (add_psymbol_to_list, add_psymbol_addr_to_list):
No longer demangle partial symbols.
* symtab.c (lookup_symbol, list_symbols): Handle mangled
variables, e.g. C++ static members, via the minimal symbols.

Don't know if it helps.

Elena

> that this issue is what caused the #ifdef HPUXHPPA to be added to
> lookup_symbol_aux: as a relevant comment says,
>
> For HP-generated symbol tables, this check was causing a premature
> exit from lookup_symbol with NULL return, and thus messing up symbol
> lookups of things like "c::f".
>
> When I asked about this issue earlier, Jim Blandy came to more or less
> the same conclusion, I think: see
> <http://sources.redhat.com/ml/gdb/2002-11/msg00045.html>.
>
> David Carlton
> carlton@math.stanford.edu
>
> 2002-12-05 David Carlton <carlton@math.stanford.edu>
>
> * symtab.c (lookup_symbol_aux): Delete 'force_return' variable.
> (lookup_symbol_aux_minsyms): Delete 'force_return' argument.
>
> Index: symtab.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/symtab.c,v
> retrieving revision 1.81
> diff -u -p -r1.81 symtab.c
> --- symtab.c 5 Dec 2002 21:26:57 -0000 1.81
> +++ symtab.c 5 Dec 2002 22:23:55 -0000
> @@ -117,8 +117,7 @@ struct symbol *lookup_symbol_aux_minsyms
> const char *mangled_name,
> const namespace_enum namespace,
> int *is_a_field_of_this,
> - struct symtab **symtab,
> - int *force_return);
> + struct symtab **symtab);
>
> static struct symbol *find_active_alias (struct symbol *sym, CORE_ADDR addr);
>
> @@ -805,14 +804,6 @@ lookup_symbol_aux (const char *name, con
> struct symbol *sym;
> const struct block *static_block;
>
> - /* FIXME: carlton/2002-11-05: This variable is here so that
> - lookup_symbol_aux will sometimes return NULL after receiving a
> - NULL return value from lookup_symbol_aux_minsyms, without
> - proceeding on to the partial symtab and static variable tests. I
> - suspect that that's a bad idea. */
> -
> - int force_return;
> -
> /* Search specified block and its superiors. Don't search
> STATIC_BLOCK or GLOBAL_BLOCK. */
>
> @@ -931,13 +922,11 @@ lookup_symbol_aux (const char *name, con
> a mangled variable that is stored in one of the minimal symbol tables.
> Eventually, all global symbols might be resolved in this way. */
>
> - force_return = 0;
> -
> sym = lookup_symbol_aux_minsyms (name, mangled_name,
> namespace, is_a_field_of_this,
> - symtab, &force_return);
> + symtab);
>
> - if (sym != NULL || force_return == 1)
> + if (sym != NULL)
> return sym;
>
> #endif
> @@ -981,13 +970,11 @@ lookup_symbol_aux (const char *name, con
> */
>
>
> - force_return = 0;
> -
> sym = lookup_symbol_aux_minsyms (name, mangled_name,
> namespace, is_a_field_of_this,
> - symtab, &force_return);
> + symtab);
>
> - if (sym != NULL || force_return == 1)
> + if (sym != NULL)
> return sym;
>
> #endif
> @@ -1172,13 +1159,20 @@ lookup_symbol_aux_psymtabs (int block_in
> 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. */
> +
> 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,
> - int *force_return)
> + struct symtab **symtab)
> {
> struct symbol *sym;
> struct blockvector *bv;
> @@ -1271,7 +1265,6 @@ lookup_symbol_aux_minsyms (const char *n
>
> if (symtab != NULL && sym != NULL)
> *symtab = s;
> - *force_return = 1;
> return fixup_symbol_section (sym, s->objfile);
> }
> else if (MSYMBOL_TYPE (msymbol) != mst_text
> @@ -1280,7 +1273,6 @@ lookup_symbol_aux_minsyms (const char *n
> {
> /* 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);