This is the mail archive of the gdb-patches@sources.redhat.com mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [rfa] more lookup_symbol_aux_minsyms futzing



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 at math dot stanford dot edu
 > 
 > 2002-12-23  David Carlton  <carlton at math dot stanford dot 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;
 >  		  }
 >  	      }
 >  	  }


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]