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]

[rfa] delete lookup_symbol_aux_minsyms


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 at math dot stanford dot edu

2003-02-22  David Carlton  <carlton at math dot stanford dot 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 */


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