[rfa] delete lookup_symbol_aux_minsyms

[rfa] delete lookup_symbol_aux_minsyms

[David Carlton]

As posted in
<http://sources.redhat.com/ml/gdb-patches/2003-02/msg00496.html>, I
believe that I have good reason to believe that:

* The code in lookup_symbol_aux_minsyms only existed because once
  partial symbols didn't contain demangled names.

* The 'else clause' in that function has been actively harmful (in
  correctness terms) in the past, and may still be so.

* The rest of the function isn't harmful in correctness terms; it
  could be either an optimization or a pessimization

So, it seems to me that, at the very least, the 'else clause' of
lookup_symbol_aux_minsyms should be deleted; quite possibly, the whole
function should be deleted.

So: when might it be an optimization, and when might it be a
pessimization?

1) If we're looking for a local symbol or a symbol that's in a symtab
   that's already been loaded, then lookup_symbol_aux_minsyms will
   never be reached, so it doesn't matter.

2) If we're looking for a function that's in a symtab that hasn't been
   loaded, then lookup_symbol_aux_minsyms will find it.  However, if
   we deleted the call to lookup_symbol_aux_minsyms, then
   lookup_symbol_aux_psymtabs would also find it.  So the question
   here is whether a minsym search is faster than a partial symbol
   table search.

3) If we're looking for something that's not in a loaded symtab
   (e.g. a type, a non-function variable, or a symbol that actually
   doesn't exist at all), then the call to lookup_symbol_aux_minsyms
   is a wasted of time, and is a pessimization.

So the questions are:

* Is case 2) an optimization or not?  If it is, how much of an
  optimization is it?

* How much of a pessimization is case 3)?

* In practice, what's the relative frequency of case 2 vs. case 3?

I wanted to test this; the obvious place to do this is by calling
'search_symbols', because that contains a lot of lookup_symbol calls.
Unfortunately, when I investigated further, it turns out that
search_symbols usually reads in all or almost all of the partial
symbol tables, so we're usually in case 1.  Oops.

The thing is, though, I don't know of any other good way to generate
lots of calls to lookup_symbol!  What this suggests is that, in
practice, performance issues won't play a key role here, so we should
base our decision on whatever is correct and simplest.

Just to see what would happen, I tried timing 'info functions' and
'info variables' (with the target program being mainline GDB); this
might get some situations where case 3 happens (because my C library
doesn't have debugging symbols).  I tried these tests on mainline GDB,
on a version of GDB with only the else clause removed, and on a
version of GDB with all of lookup_symbol_aux removed.  All three
versions produced the same output, which is good!  The results:

* Mainline GDB and GDB with only the else clause removed produced the
  same range of times.  (Full disclosure: removing the else clause
  might be making things ever so slightly slower, which I don't
  understand at all.  But the time ranges were really very similar
  indeed, so this may just be an artifact of random fluctuations.)

* The version with lookup_symbol_aux_minsyms removed entirely was
  consistently but slightly faster, along the lines of 1 or 2 percent.
  Not a bad thing, but by no means decisive: if we want to speed up
  search_symbols, I bet there are much better ways to do it.

So what's the conclusion?  Performance considerations don't seem to
give a clear answer.  So we should go with whatever's cleanest.  My
recommendation:

* Delete the 'else' clause: it might cause correctness problems.

* Comment out the remaining part of lookup_symbol_aux_minsyms: if
  somebody comes up with a situation where we spend lots of time
  searching for functions that aren't in a loaded symtab, we can
  consider uncommenting it and adding it back in.

On a side note, there's the question of what to do with the HP bit.
The comment in the HP bit says that the call to
lookup_symbol_aux_minsyms was moved to the end because it had been
forcing incorrect NULL returns.  I'm 95% sure that that's a
manifestation of the weird control flow that manifested itself in the
'force_return' variable that I had to introduce when creating
lookup_symbol_aux_minsyms.  If so, then deleting force_return (which I
did in December) meant that the HP special case could have gone away.
So the HP bit can go away, too.

Here's a patch that implements my recommendations.  Tested on
i686-pc-linux-gnu/GCC3.1/DWARF-2, as well as by checking that it
doesn't change the output of 'info symbols' or 'info variables' in one
particular case.  No new regressions; OK to commit?

David Carlton
carlton@math.stanford.edu

2003-02-22  David Carlton  <carlton@math.stanford.edu>

	* symtab.c (lookup_symbol_aux): Comment out non-HP call to
	lookup_symbol_aux_minsyms; delete HP call.
	(lookup_symbol_aux_minsyms): Delete 'else' branch; comment out the
	rest of the function.

Index: symtab.c
===================================================================
RCS file: /cvs/src/src/gdb/symtab.c,v
retrieving revision 1.92
diff -u -p -r1.92 symtab.c
--- symtab.c	21 Feb 2003 15:24:18 -0000	1.92
+++ symtab.c	22 Feb 2003 18:52:12 -0000
@@ -115,12 +115,14 @@ struct symbol *lookup_symbol_aux_psymtab
 					   const namespace_enum namespace,
 					   struct symtab **symtab);
 
+#if 0
 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);
+#endif
 
 static struct symbol *find_active_alias (struct symbol *sym, CORE_ADDR addr);
 
@@ -981,8 +983,7 @@ lookup_symbol_aux (const char *name, con
   if (sym != NULL)
     return sym;
 
-#ifndef HPUXHPPA
-
+#if 0
   /* 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.  */
@@ -993,7 +994,6 @@ lookup_symbol_aux (const char *name, con
   
   if (sym != NULL)
     return sym;
-
 #endif
 
   sym = lookup_symbol_aux_psymtabs (GLOBAL_BLOCK, name, mangled_name,
@@ -1017,33 +1017,6 @@ lookup_symbol_aux (const char *name, con
   if (sym != NULL)
     return sym;
 
-#ifdef HPUXHPPA
-
-  /* Check for the possibility of the symbol being a function or
-     a global variable that is stored in one of the minimal symbol tables.
-     The "minimal symbol table" is built from linker-supplied info.
-
-     RT: I moved this check to last, after the complete search of
-     the global (p)symtab's and static (p)symtab's. 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". It seems to me a check of the minimal
-     symbol table ought to be a last resort in any case. I'm vaguely
-     worried about the comment below which talks about FORTRAN routines "foo_"
-     though... is it saying we need to do the "minsym" check before
-     the static check in this case? 
-   */
-
-
-  sym = lookup_symbol_aux_minsyms (name, mangled_name,
-				   namespace, is_a_field_of_this,
-				   symtab);
-  
-  if (sym != NULL)
-    return sym;
-
-#endif
-
   if (symtab != NULL)
     *symtab = NULL;
   return NULL;
@@ -1219,6 +1192,7 @@ lookup_symbol_aux_psymtabs (int block_in
   return NULL;
 }
 
+#if 0
 /* 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
@@ -1232,6 +1206,13 @@ lookup_symbol_aux_psymtabs (int block_in
    some additional conditions held as well, and it caused problems
    with HP-generated symbol tables.  */
 
+/* NOTE: carlton/2003-02-22: As far as I can tell, this code only
+   existed to handle the fact that partial symbols once only contained
+   mangled names.  This is no longer the case; I've deleted the code
+   that is now potentially harmful.  What remains may be an
+   optimization or a pessimization; since there's no evidence that
+   it's an optimization, I'm commenting it out.  */
+
 static struct symbol *
 lookup_symbol_aux_minsyms (const char *name,
 			   const char *mangled_name,
@@ -1332,21 +1313,12 @@ 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);
-	    }
 	}
     }
 
   return NULL;
 }
+#endif
 
 /* Look, in partial_symtab PST, for symbol NAME.  Check the global
    symbols if GLOBAL, the static symbols if not */

Re: [rfa] delete lookup_symbol_aux_minsyms

[Daniel Jacobowitz]

On Sat, Feb 22, 2003 at 11:54:40AM -0800, David Carlton wrote:
> So what's the conclusion?  Performance considerations don't seem to
> give a clear answer.  So we should go with whatever's cleanest.  My
> recommendation:
> 
> * Delete the 'else' clause: it might cause correctness problems.
> 
> * Comment out the remaining part of lookup_symbol_aux_minsyms: if
>   somebody comes up with a situation where we spend lots of time
>   searching for functions that aren't in a loaded symtab, we can
>   consider uncommenting it and adding it back in.

I'm pretty sure the answer is "none at all" based on skimming the code,
but what affect does removing lookup_symbol_aux_minsyms have when
looking for something which turns out not to have debugging info? 
lookup_symbol would fail, so it doesn't matter - is that right?

I don't know if I like this comment-out-part-delete-part business; if
we don't want the function, let's kill it.


-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [rfa] delete lookup_symbol_aux_minsyms

[David Carlton]

On Sat, 22 Feb 2003 15:50:11 -0500, Daniel Jacobowitz <drow@mvista.com> said:
> On Sat, Feb 22, 2003 at 11:54:40AM -0800, David Carlton wrote:

>> So what's the conclusion?  Performance considerations don't seem to
>> give a clear answer.  So we should go with whatever's cleanest.  My
>> recommendation:
>> 
>> * Delete the 'else' clause: it might cause correctness problems.
>> 
>> * Comment out the remaining part of lookup_symbol_aux_minsyms: if
>> somebody comes up with a situation where we spend lots of time
>> searching for functions that aren't in a loaded symtab, we can
>> consider uncommenting it and adding it back in.

> I'm pretty sure the answer is "none at all" based on skimming the
> code, but what affect does removing lookup_symbol_aux_minsyms have
> when looking for something which turns out not to have debugging
> info?  lookup_symbol would fail, so it doesn't matter - is that
> right?

That's right: lookup_symbol tries to find symbols, so if a function
doesn't have debugging info, then lookup_symbol will return NULL
(unless we're in some sort of bizarre situation like having a static
function elsewhere with the same name), at which point it's the
caller's responsibility to call lookup_minimal_symbol if the caller is
in a position to deal with minimal symbols instead of symbols.  And
lookup_symbol would have returned NULL either before or after my
patch: the call to 'find_pc_sect_symtab' would have failed.

> I don't know if I like this comment-out-part-delete-part business;
> if we don't want the function, let's kill it.

It's not my first choice, either, but Elena has shown a preference in
the past for commenting out code like this instead of deleting it.
(C.f. the already commented-out bit in lookup_symbol_aux.)  This seems
to me a situation where it's not worth arguing about it.  However, if
Elena would rather delete it, I'd be happy to go along with that, too.

David Carlton
carlton@math.stanford.edu

Re: [rfa] delete lookup_symbol_aux_minsyms

[Elena Zannoni]

David Carlton writes:
 > On Sat, 22 Feb 2003 15:50:11 -0500, Daniel Jacobowitz <drow@mvista.com> said:
 > > On Sat, Feb 22, 2003 at 11:54:40AM -0800, David Carlton wrote:
 > 
 > >> So what's the conclusion?  Performance considerations don't seem to
 > >> give a clear answer.  So we should go with whatever's cleanest.  My
 > >> recommendation:
 > >> 
 > >> * Delete the 'else' clause: it might cause correctness problems.
 > >> 
 > >> * Comment out the remaining part of lookup_symbol_aux_minsyms: if
 > >> somebody comes up with a situation where we spend lots of time
 > >> searching for functions that aren't in a loaded symtab, we can
 > >> consider uncommenting it and adding it back in.
 > 
 > > I'm pretty sure the answer is "none at all" based on skimming the
 > > code, but what affect does removing lookup_symbol_aux_minsyms have
 > > when looking for something which turns out not to have debugging
 > > info?  lookup_symbol would fail, so it doesn't matter - is that
 > > right?
 > 
 > That's right: lookup_symbol tries to find symbols, so if a function
 > doesn't have debugging info, then lookup_symbol will return NULL
 > (unless we're in some sort of bizarre situation like having a static
 > function elsewhere with the same name), at which point it's the
 > caller's responsibility to call lookup_minimal_symbol if the caller is
 > in a position to deal with minimal symbols instead of symbols.  And
 > lookup_symbol would have returned NULL either before or after my
 > patch: the call to 'find_pc_sect_symtab' would have failed.
 > 
 > > I don't know if I like this comment-out-part-delete-part business;
 > > if we don't want the function, let's kill it.
 > 
 > It's not my first choice, either, but Elena has shown a preference in
 > the past for commenting out code like this instead of deleting it.
 > (C.f. the already commented-out bit in lookup_symbol_aux.)  This seems
 > to me a situation where it's not worth arguing about it.  However, if
 > Elena would rather delete it, I'd be happy to go along with that, too.
 > 
 > David Carlton
 > carlton@math.stanford.edu


Actually, I did some gcov on that function last night, running make check,
here is what I got.

About deleting the function. I think I would prefer if we delete the
'else' part first. Then comment out the function.

But I am still not convinced that we should delete it yet.

elena



		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)
		{
        1442      struct symbol *sym;
		  struct blockvector *bv;
		  const struct block *block;
		  struct minimal_symbol *msymbol;
		  struct symtab *s;
		
        1442      if (namespace == VAR_NAMESPACE)
		    {
        1332          msymbol = lookup_minimal_symbol (name, NULL, NULL);
		
        1332          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.  */
         329    	  s = find_pc_sect_symtab (SYMBOL_VALUE_ADDRESS (msymbol),
						   SYMBOL_BFD_SECTION (msymbol));
         329    	  if (s != NULL)
			    {
			      /* This is a function which has a symtab for its address.  */
         201    	      bv = BLOCKVECTOR (s);
         201    	      block = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
		
			      /* This call used to pass `SYMBOL_NAME (msymbol)' as the
			         `name' argument to lookup_block_symbol.  But the name
			         of a minimal symbol is always mangled, so that seems
			         to be clearly the wrong thing to pass as the
			         unmangled name.  */
         201    	      sym =
				lookup_block_symbol (block, name, mangled_name, namespace);
			      /* We kept static functions in minimal symbol table as well as
			         in static scope. We want to find them in the symbol table. */
         201    	      if (!sym)
				{
          39    		  block = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
          39    		  sym = lookup_block_symbol (block, name,
							     mangled_name, namespace);
         201    		}
		
			      /* NOTE: carlton/2002-12-04: The following comment was
				 taken from a time when two versions of this function
				 were part of the body of lookup_symbol_aux: this
				 comment was taken from the version of the function
				 that was #ifdef HPUXHPPA, and the comment was right
				 before the 'return NULL' part of lookup_symbol_aux.
				 (Hence the "Fall through and return 0" comment.)
				 Elena did some digging into the situation for
				 Fortran, and she reports:
		
				 "I asked around (thanks to Jeff Knaggs), and I think
				 the story for Fortran goes like this:
		
				 "Apparently, in older Fortrans, '_' was not part of
				 the user namespace.  g77 attached a final '_' to
				 procedure names as the exported symbols for linkage
				 (foo_) , but the symbols went in the debug info just
				 like 'foo'. The rationale behind this is not
				 completely clear, and maybe it was done to other
				 symbols as well, not just procedures."  */
		
			      /* If we get here with sym == 0, the symbol was 
			         found in the minimal symbol table
			         but not in the symtab.
			         Fall through and return 0 to use the msymbol 
			         definition of "foo_".
			         (Note that outer code generally follows up a call
			         to this routine with a call to lookup_minimal_symbol(),
			         so a 0 return means we'll just flow into that other routine).
		
			         This happens for Fortran  "foo_" symbols,
			         which are "foo" in the symtab.
		
			         This can also happen if "asm" is used to make a
			         regular symbol but not a debugging symbol, e.g.
			         asm(".globl _main");
			         asm("_main:");
			       */
		
         201    	      if (symtab != NULL && sym != NULL)
         155    		*symtab = s;
         201    	      return fixup_symbol_section (sym, s->objfile);
         128    	    }
         128    	  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.  */
           2    	      return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name,
							NULL, namespace, is_a_field_of_this,
           2    					symtab);
         126    	    }
        1129    	}
        1239        }
		
        1239      return NULL;
        1442    }

Re: [rfa] delete lookup_symbol_aux_minsyms

[David Carlton]

On Mon, 24 Feb 2003 13:21:55 -0500, Elena Zannoni <ezannoni@redhat.com> said:

> Actually, I did some gcov on that function last night, running make check,
> here is what I got.

> About deleting the function. I think I would prefer if we delete the
> 'else' part first. Then comment out the function.

> But I am still not convinced that we should delete it yet.

I'm not an expert on 'gcov': does the data you presented mean that, in
some situation where lookup_symbol_aux_minsyms was called 1442 times,
the else block was called 2 times?  So you're worried about those 2
times, that they produced correct information that wouldn't have been
reached if we deleted the 'else' clause?

If so, here's the situation with the 'else' clause.  That clause calls
lookup_symbol_aux with a mangled name as the 'name' argument.  But the
'name' argument to lookup_symbol_aux should never be mangled: calls to
lookup_symbol_aux always ultimately boil down to a call to
lookup_block_symbol.  And lookup_block_symbol needs a demangled name
as its 'name' argument, not a mangled name: blocks are hashed by their
demangled names, so passing in a mangled name is likely to result in
searching in the wrong bucket.  There might be times when
lookup_block_symbol returns the correct value when you pass in a
mangled name as the 'name' argument, but, if so, that's purely
accidental; and, in those situations, the same symbol would have been
found in the psymtab search that follows the call to
lookup_symbol_aux_minsyms.

Here's the current control flow, more or less:

* Search symtabs.

* Search minsyms.

** Maybe then call lookup_symbol_aux with a demangled name as the
   'name' argument, in which case we:

** Search symtabs again, with mangled name.

** Search minsyms again, with mangled name.

** Search psymtabs, with mangled name.

* Search psymtabs (with demangled name).


The 'else' clause is the stuff that I've labeled with two asterisks.
I don't see how the 'search symtabs again' or 'search minsyms again'
parts could ever return anything useful.  And the 'search psymtabs,
with mangled name', might accidentally sometimes find the variable
we're looking for; if so, however, the outer 'search psymtabs (with
demangled name)' would have found it as well, and would do so much
more reliably.

David Carlton
carlton@math.stanford.edu

Re: [rfa] delete lookup_symbol_aux_minsyms

[Elena Zannoni]

David Carlton writes:
 > On Mon, 24 Feb 2003 13:21:55 -0500, Elena Zannoni <ezannoni@redhat.com> said:
 > 
 > > Actually, I did some gcov on that function last night, running make check,
 > > here is what I got.
 > 
 > > About deleting the function. I think I would prefer if we delete the
 > > 'else' part first. Then comment out the function.
 > 
 > > But I am still not convinced that we should delete it yet.
 > 
 > I'm not an expert on 'gcov': does the data you presented mean that, in
 > some situation where lookup_symbol_aux_minsyms was called 1442 times,
 > the else block was called 2 times?  So you're worried about those 2
 > times, that they produced correct information that wouldn't have been
 > reached if we deleted the 'else' clause?
 > 

Yes, each number is the number of times that the line is executed.  I
am not particularly worried about the 'else' clause. I agree that it's
dumb. I think it's ok to delete that.
The numbers were more like 'food for thoughts'.

elena


 > If so, here's the situation with the 'else' clause.  That clause calls
 > lookup_symbol_aux with a mangled name as the 'name' argument.  But the
 > 'name' argument to lookup_symbol_aux should never be mangled: calls to
 > lookup_symbol_aux always ultimately boil down to a call to
 > lookup_block_symbol.  And lookup_block_symbol needs a demangled name
 > as its 'name' argument, not a mangled name: blocks are hashed by their
 > demangled names, so passing in a mangled name is likely to result in
 > searching in the wrong bucket.  There might be times when
 > lookup_block_symbol returns the correct value when you pass in a
 > mangled name as the 'name' argument, but, if so, that's purely
 > accidental; and, in those situations, the same symbol would have been
 > found in the psymtab search that follows the call to
 > lookup_symbol_aux_minsyms.
 > 
 > Here's the current control flow, more or less:
 > 
 > * Search symtabs.
 > 
 > * Search minsyms.
 > 
 > ** Maybe then call lookup_symbol_aux with a demangled name as the
 >    'name' argument, in which case we:
 > 
 > ** Search symtabs again, with mangled name.
 > 
 > ** Search minsyms again, with mangled name.
 > 
 > ** Search psymtabs, with mangled name.
 > 
 > * Search psymtabs (with demangled name).
 > 
 > 
 > The 'else' clause is the stuff that I've labeled with two asterisks.
 > I don't see how the 'search symtabs again' or 'search minsyms again'
 > parts could ever return anything useful.  And the 'search psymtabs,
 > with mangled name', might accidentally sometimes find the variable
 > we're looking for; if so, however, the outer 'search psymtabs (with
 > demangled name)' would have found it as well, and would do so much
 > more reliably.
 > 
 > David Carlton
 > carlton@math.stanford.edu

Re: [rfa] delete lookup_symbol_aux_minsyms

[David Carlton]

On Mon, 24 Feb 2003 13:56:54 -0500, Elena Zannoni <ezannoni@redhat.com> said:

> I am not particularly worried about the 'else' clause. I agree that
> it's dumb. I think it's ok to delete that.

Great.  So is my original patch okay, then, or are there any
modifications that you'd like me to make?  It sounded earlier like the
else clause was the main thing you were worried about, but maybe I
misread you.

David Carlton
carlton@math.stanford.edu