[rfa] clean up lookup_symbol_aux

[rfa] clean up lookup_symbol_aux

[David Carlton]

When trying to understand lookup_symbol_aux, I found that it helped to
pull chunks of the code into separate functions (e.g. the code to look
up a symbol in all the global symtabs); this way, I could separate
questions like "in what order do we look up symbols in various data
structures?" from questions like "what exactly do we do when we look
for a symbol in the partial symbol tables?".  And, once I'd done that,
I found that some of the resulting functions could be combined
(e.g. the code to look up a symbol in all the global symtabs is
essentially the same as the code to look up a symbol in all the static
symtabs).

I think that the result is enough clearer than the original that I'm
probably not the only person who would find it useful.  And combining
similar chunks of code should help maintenance: e.g. the non-HPUXHPPA
and HPUXHPPA cases are now combined (i.e. they're both calls to
lookup_symbol_aux_minsyms, albeit in different places), so you don't
have to remember to update them both.  (Some people seem to have
forgotten that in the past: see below.)

The resulting patch isn't easy to read: to be honest, to understand
what it's doing, you'll probably want to apply it to a copy of
symtab.c and compare the results, rather than try to read the patch
directly.  Here are the Cliff's Notes for reading it:

Changes that shouldn't affect the functioning of GDB at all:

* The part of lookup_symbol_aux that never gets run and that is
  prefaced by 17 lines of comments being bemused at its existence has
  been deleted.

* Other long, coherent chunks of lookup_symbol_aux have been moved
  into separate functions.

* When two of the new functions were essentially identical, those
  functions have been merged.  To be specific, there's a single
  function lookup_symbol_aux_symtabs that can either search global
  symbols or static symbols, and similarly with psymtab search.

Changes that might theoretically affect the function of GDB:

* The two cases for searching minimal symbols (i.e. whether or not
  you're in HPUXHPPA) turned out to be slightly different.  I went
  through those differences and picked whichever variant seemed to me
  to be better; as far as I can tell, there's no reason for the code
  to differ in the two cases.  I also threw in a bug fix:
  fixup_symbol_section had the wrong second argument in the variant
  where it was present.

* I decided to reorder the search order: in the non-HPUXHPPA case, it
  makes sense to me to search global psymtabs before searching minimal
  symbols.  The theory there is that you should look for symbols with
  debugging information before looking for symbols without debugging
  information; also it seems to me that the current code might, in
  some situations, find a static symbol via searching the minsyms
  before it finds a global symbol via searching the psymtabs, which is
  definitely a bad outcome.

I've tested this, and didn't turn up any new regressions.  It would be
nice if somebody could give it a spin on an HP machine to make sure I
didn't screw up that variant of the function.

David Carlton
carlton@math.stanford.edu

2002-10-02  David Carlton  <carlton@math.stanford.edu>

	* symtab.c (lookup_symbol_aux): Move chunks of code into separate
	functions.
	(lookup_symbol_aux_local): New function.
	(lookup_symbol_aux_symtabs): New function.
	(lookup_symbol_aux_psymtabs): New function.
	(lookup_symbol_aux_minsyms): New function.

Index: symtab.c
===================================================================
RCS file: /cvs/src/src/gdb/symtab.c,v
retrieving revision 1.70
diff -u -p -r1.70 symtab.c
--- symtab.c	20 Sep 2002 14:58:58 -0000	1.70
+++ symtab.c	2 Oct 2002 21:10:53 -0000
@@ -88,6 +88,32 @@ static struct symbol *lookup_symbol_aux 
 					 int *is_a_field_of_this,
 					 struct symtab **symtab);
 
+static struct symbol *lookup_symbol_aux_local (const char *name,
+					       const char *mangled_name,
+					       const struct block *block,
+					       const namespace_enum namespace,
+					       struct symtab **symtab);
+
+static
+struct symbol *lookup_symbol_aux_symtabs (int block_index,
+					  const char *name,
+					  const char *mangled_name,
+					  const namespace_enum namespace,
+					  struct symtab **symtab);
+
+static
+struct symbol *lookup_symbol_aux_psymtabs (int block_index,
+					   const char *name,
+					   const char *mangled_name,
+					   const namespace_enum namespace,
+					   struct symtab **symtab);
+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);
 
@@ -729,84 +755,14 @@ lookup_symbol_aux (const char *name, con
 		   const struct block *block, const namespace_enum namespace,
 		   int *is_a_field_of_this, struct symtab **symtab)
 {
-  register struct symbol *sym;
-  register struct symtab *s = NULL;
-  register struct partial_symtab *ps;
-  register struct blockvector *bv;
-  register struct objfile *objfile = NULL;
-  register struct block *b;
-  register struct minimal_symbol *msymbol;
-
+  struct symbol *sym;
 
   /* Search specified block and its superiors.  */
 
-  while (block != 0)
-    {
-      sym = lookup_block_symbol (block, name, mangled_name, namespace);
-      if (sym)
-	{
-	  block_found = block;
-	  if (symtab != NULL)
-	    {
-	      /* Search the list of symtabs for one which contains the
-	         address of the start of this block.  */
-	      ALL_SYMTABS (objfile, s)
-	      {
-		bv = BLOCKVECTOR (s);
-		b = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
-		if (BLOCK_START (b) <= BLOCK_START (block)
-		    && BLOCK_END (b) > BLOCK_START (block))
-		  goto found;
-	      }
-	    found:
-	      *symtab = s;
-	    }
-
-	  return fixup_symbol_section (sym, objfile);
-	}
-      block = BLOCK_SUPERBLOCK (block);
-    }
-
-  /* FIXME: this code is never executed--block is always NULL at this
-     point.  What is it trying to do, anyway?  We already should have
-     checked the STATIC_BLOCK above (it is the superblock of top-level
-     blocks).  Why is VAR_NAMESPACE special-cased?  */
-  /* Don't need to mess with the psymtabs; if we have a block,
-     that file is read in.  If we don't, then we deal later with
-     all the psymtab stuff that needs checking.  */
-  /* Note (RT): The following never-executed code looks unnecessary to me also.
-   * If we change the code to use the original (passed-in)
-   * value of 'block', we could cause it to execute, but then what
-   * would it do? The STATIC_BLOCK of the symtab containing the passed-in
-   * 'block' was already searched by the above code. And the STATIC_BLOCK's
-   * of *other* symtabs (those files not containing 'block' lexically)
-   * should not contain 'block' address-wise. So we wouldn't expect this
-   * code to find any 'sym''s that were not found above. I vote for 
-   * deleting the following paragraph of code.
-   */
-  if (namespace == VAR_NAMESPACE && block != NULL)
-    {
-      struct block *b;
-      /* Find the right symtab.  */
-      ALL_SYMTABS (objfile, s)
-      {
-	bv = BLOCKVECTOR (s);
-	b = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
-	if (BLOCK_START (b) <= BLOCK_START (block)
-	    && BLOCK_END (b) > BLOCK_START (block))
-	  {
-	    sym = lookup_block_symbol (b, name, mangled_name, VAR_NAMESPACE);
-	    if (sym)
-	      {
-		block_found = b;
-		if (symtab != NULL)
-		  *symtab = s;
-		return fixup_symbol_section (sym, objfile);
-	      }
-	  }
-      }
-    }
-
+  sym = lookup_symbol_aux_local (name, mangled_name, block, namespace,
+				 symtab);
+  if (sym != NULL)
+    return sym;
 
   /* C++: If requested to do so by the caller, 
      check to see if NAME is a field of `this'. */
@@ -829,128 +785,145 @@ lookup_symbol_aux (const char *name, con
      of the desired name as a global, then do psymtab-to-symtab
      conversion on the fly and return the found symbol. */
 
-  ALL_SYMTABS (objfile, s)
-  {
-    bv = BLOCKVECTOR (s);
-    block = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
-    sym = lookup_block_symbol (block, name, mangled_name, namespace);
-    if (sym)
-      {
-	block_found = block;
-	if (symtab != NULL)
-	  *symtab = s;
-	return fixup_symbol_section (sym, objfile);
-      }
-  }
+  sym = lookup_symbol_aux_symtabs (GLOBAL_BLOCK, name, mangled_name,
+				   namespace, symtab);
+  if (sym != NULL)
+    return sym;
+
+  sym = lookup_symbol_aux_psymtabs (GLOBAL_BLOCK, name, mangled_name,
+				    namespace, symtab);
+  if (sym != NULL)
+    return sym;
 
 #ifndef HPUXHPPA
 
-  /* 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.  */
+  /* 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.  */
+
+  sym = lookup_symbol_aux_minsyms (name, mangled_name,
+				   namespace, is_a_field_of_this,
+				   symtab);
+  if (sym != NULL)
+    return sym;
 
-  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)
-	    {
-	      /* This is a function which has a symtab for its address.  */
-	      bv = BLOCKVECTOR (s);
-	      block = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
+#endif
 
-              /* 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.  */
-	      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. */
-	      if (!sym)
-		{
-		  block = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
-		  sym = lookup_block_symbol (block, name,
-                                             mangled_name, namespace);
-		}
+  /* Now search all static file-level symbols.  Not strictly correct,
+     but more useful than an error.  Do the symtabs first, then check
+     the psymtabs.  If a psymtab indicates the existence of the
+     desired name as a file-level static, then do psymtab-to-symtab
+     conversion on the fly and return the found symbol. */
 
-	      /* sym == 0 if symbol was found in the minimal symbol table
-	         but not in the symtab.
-	         Return 0 to use the msymbol definition of "foo_".
+  sym = lookup_symbol_aux_symtabs (STATIC_BLOCK, name, mangled_name,
+				   namespace, symtab);
+  if (sym != NULL)
+    return sym;
+  
+  sym = lookup_symbol_aux_psymtabs (STATIC_BLOCK, name, mangled_name,
+				    namespace, symtab);
+  if (sym != NULL)
+    return sym;
 
-	         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:");
-	       */
+#ifdef HPUXHPPA
 
-	      if (symtab != NULL)
-		*symtab = s;
-	      return fixup_symbol_section (sym, objfile);
-	    }
-	  else if (MSYMBOL_TYPE (msymbol) != mst_text
-		   && MSYMBOL_TYPE (msymbol) != mst_file_text
-		   && !STREQ (name, SYMBOL_NAME (msymbol)))
+  /* 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 within
+     lookup_symbol_aux_minsyms 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;
+}
+
+/* Check to see if the symbol is defined in BLOCK or its
+   superiors.  */
+
+static struct symbol *
+lookup_symbol_aux_local (const char *name, const char *mangled_name,
+			 const struct block *block,
+			 const namespace_enum namespace,
+			 struct symtab **symtab)
+{
+  struct symbol *sym;
+  struct objfile *objfile = NULL;
+  struct blockvector *bv;
+  struct block *b;
+  struct symtab *s = NULL;
+  
+  while (block != 0)
+    {
+      sym = lookup_block_symbol (block, name, mangled_name, namespace);
+      if (sym)
+	{
+	  block_found = block;
+	  if (symtab != NULL)
 	    {
-	      /* This is a mangled variable, look it up by its
-	         mangled name.  */
-	      return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name, block,
-					namespace, is_a_field_of_this, symtab);
+	      /* Search the list of symtabs for one which contains the
+	         address of the start of this block.  */
+	      ALL_SYMTABS (objfile, s)
+	      {
+		bv = BLOCKVECTOR (s);
+		b = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
+		if (BLOCK_START (b) <= BLOCK_START (block)
+		    && BLOCK_END (b) > BLOCK_START (block))
+		  goto found;
+	      }
+	    found:
+	      *symtab = s;
 	    }
-	  /* There are no debug symbols for this file, or we are looking
-	     for an unmangled variable.
-	     Try to find a matching static symbol below. */
+
+	  return fixup_symbol_section (sym, objfile);
 	}
+      block = BLOCK_SUPERBLOCK (block);
     }
 
-#endif
+  return NULL;
+}
 
-  ALL_PSYMTABS (objfile, ps)
-  {
-    if (!ps->readin && lookup_partial_symbol (ps, name, 1, namespace))
-      {
-	s = PSYMTAB_TO_SYMTAB (ps);
-	bv = BLOCKVECTOR (s);
-	block = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
-	sym = lookup_block_symbol (block, name, mangled_name, namespace);
-	if (!sym)
-	  {
-	    /* This shouldn't be necessary, but as a last resort
-	     * try looking in the statics even though the psymtab
-	     * claimed the symbol was global. It's possible that
-	     * the psymtab gets it wrong in some cases.
-	     */
-	    block = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
-	    sym = lookup_block_symbol (block, name, mangled_name, namespace);
-	    if (!sym)
-	      error ("Internal: global symbol `%s' found in %s psymtab but not in symtab.\n\
-%s may be an inlined function, or may be a template function\n\
-(if a template, try specifying an instantiation: %s<type>).",
-		     name, ps->filename, name, name);
-	  }
-	if (symtab != NULL)
-	  *symtab = s;
-	return fixup_symbol_section (sym, objfile);
-      }
-  }
+/* Check to see if the symbol is defined in one of the symtabs.
+   BLOCK_INDEX should be either GLOBAL_BLOCK or STATIC_BLOCK,
+   depending on whether or not we want to search global symbols or
+   static symbols.  */
 
-  /* Now search all static file-level symbols.
-     Not strictly correct, but more useful than an error.
-     Do the symtabs first, then check the psymtabs.
-     If a psymtab indicates the existence
-     of the desired name as a file-level static, then do psymtab-to-symtab
-     conversion on the fly and return the found symbol. */
+static struct symbol *
+lookup_symbol_aux_symtabs (int block_index,
+			   const char *name, const char *mangled_name,
+			   const namespace_enum namespace,
+			   struct symtab **symtab)
+{
+  struct symbol *sym;
+  struct objfile *objfile;
+  struct blockvector *bv;
+  const struct block *block;
+  struct symtab *s;
 
   ALL_SYMTABS (objfile, s)
   {
     bv = BLOCKVECTOR (s);
-    block = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
+    block = BLOCKVECTOR_BLOCK (bv, block_index);
     sym = lookup_block_symbol (block, name, mangled_name, namespace);
     if (sym)
       {
@@ -961,27 +934,57 @@ lookup_symbol_aux (const char *name, con
       }
   }
 
+  return NULL;
+}
+
+/* Check to see if the symbol is defined in one of the partial
+   symtabs.  BLOCK_INDEX should be either GLOBAL_BLOCK or
+   STATIC_BLOCK, depending on whether or not we want to search global
+   symbols or static symbols.  */
+
+static struct symbol *
+lookup_symbol_aux_psymtabs (int block_index, const char *name,
+			    const char *mangled_name,
+			    const namespace_enum namespace,
+			    struct symtab **symtab)
+{
+  struct symbol *sym;
+  struct objfile *objfile;
+  struct blockvector *bv;
+  const struct block *block;
+  struct partial_symtab *ps;
+  struct symtab *s;
+  const int psymtab_index = (block_index == GLOBAL_BLOCK ? 1 : 0);
+
   ALL_PSYMTABS (objfile, ps)
   {
-    if (!ps->readin && lookup_partial_symbol (ps, name, 0, namespace))
+    if (!ps->readin
+	&& lookup_partial_symbol (ps, name, psymtab_index, namespace))
       {
 	s = PSYMTAB_TO_SYMTAB (ps);
 	bv = BLOCKVECTOR (s);
-	block = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
+	block = BLOCKVECTOR_BLOCK (bv, block_index);
 	sym = lookup_block_symbol (block, name, mangled_name, namespace);
 	if (!sym)
 	  {
-	    /* This shouldn't be necessary, but as a last resort
-	     * try looking in the globals even though the psymtab
-	     * claimed the symbol was static. It's possible that
-	     * the psymtab gets it wrong in some cases.
-	     */
-	    block = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
+	    /* This shouldn't be necessary, but as a last resort try
+	       looking in the statics even though the psymtab claimed
+	       the symbol was global, or vice-versa. It's possible
+	       that the psymtab gets it wrong in some cases.  */
+
+	    /* FIXME: carlton/2002-09-30: Should we really do that?
+	       If that happens, isn't it likely to be a GDB error, in
+	       which case we should fix the GDB error rather than
+	       silently dealing with it here?  So I'd vote for
+	       removing the check for the symbol in the other
+	       block.  */
+	    block = BLOCKVECTOR_BLOCK (bv,
+				       block_index == GLOBAL_BLOCK ?
+				       STATIC_BLOCK : GLOBAL_BLOCK);
 	    sym = lookup_block_symbol (block, name, mangled_name, namespace);
 	    if (!sym)
-	      error ("Internal: static symbol `%s' found in %s psymtab but not in symtab.\n\
-%s may be an inlined function, or may be a template function\n\
-(if a template, try specifying an instantiation: %s<type>).",
+	      error ("Internal: %s symbol `%s' found in %s psymtab but not in symtab.\n%s may be an inlined function, or may be a template function\n(if a template, try specifying an instantiation: %s<type>).",
+		     block_index == GLOBAL_BLOCK ? "global" : "static",
 		     name, ps->filename, name, name);
 	  }
 	if (symtab != NULL)
@@ -990,79 +993,73 @@ lookup_symbol_aux (const char *name, con
       }
   }
 
-#ifdef HPUXHPPA
+  return NULL;
+}
 
-  /* 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.
+/* 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.  */
 
-     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? 
-   */
+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;
+  struct blockvector *bv;
+  const struct block *block;
+  struct minimal_symbol *msymbol;
+  struct symtab *s;
 
   if (namespace == VAR_NAMESPACE)
     {
       msymbol = lookup_minimal_symbol (name, NULL, NULL);
       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_symtab()
-	   * has a side-effect of doing psymtab-to-symtab expansion,
-	   * for the found symtab.
-	   */
-	  s = find_pc_symtab (SYMBOL_VALUE_ADDRESS (msymbol));
+	  /* 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.  */
+	  s = find_pc_sect_symtab (SYMBOL_VALUE_ADDRESS (msymbol),
+				   SYMBOL_BFD_SECTION (msymbol));
 	  if (s != NULL)
 	    {
+	      /* This is a function which has a symtab for its address.  */
 	      bv = BLOCKVECTOR (s);
 	      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.  */
-	      sym = lookup_block_symbol (block, name, mangled_name, namespace);
+
+	      /* 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.  */
+	      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. */
 	      if (!sym)
 		{
 		  block = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
 		  sym = lookup_block_symbol (block, name,
-                                             mangled_name, namespace);
-		}
-	      /* If we found one, return it */
-	      if (sym)
-		{
-		  if (symtab != NULL)
-		    *symtab = s;
-		  return sym;
+					     mangled_name, namespace);
 		}
 
-	      /* If we get here with sym == 0, the symbol was 
-	         found in the minimal symbol table
+	      /* sym == 0 if 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).
+	         Return 0 to use the msymbol definition of "foo_".
 
 	         This happens for Fortran  "foo_" symbols,
 	         which are "foo" in the symtab.
@@ -1072,28 +1069,25 @@ lookup_symbol_aux (const char *name, con
 	         asm(".globl _main");
 	         asm("_main:");
 	       */
-	    }
 
-	  /* If the lookup-by-address fails, try repeating the
-	   * entire lookup process with the symbol name from
-	   * the msymbol (if different from the original symbol name).
-	   */
+	      if (symtab != NULL)
+		*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,
-					block, namespace, is_a_field_of_this,
+					NULL, namespace, is_a_field_of_this,
 					symtab);
 	    }
 	}
     }
 
-#endif
-
-  if (symtab != NULL)
-    *symtab = NULL;
-  return 0;
+  return NULL;
 }
 								
 /* Look, in partial_symtab PST, for symbol NAME.  Check the global
@@ -1335,10 +1329,9 @@ lookup_block_symbol (register const stru
 		     const char *mangled_name,
 		     const namespace_enum namespace)
 {
-  register int bot, top, inc;
+  register int bot, top;
   register struct symbol *sym;
   register struct symbol *sym_found = NULL;
-  register int do_linear_search = 1;
 
   if (BLOCK_HASHTABLE (block))
     {
@@ -1355,103 +1348,27 @@ lookup_block_symbol (register const stru
 	}
       return NULL;
     }
-
-  /* If the blocks's symbols were sorted, start with a binary search.  */
-
-  if (BLOCK_SHOULD_SORT (block))
-    {
-      /* Reset the linear search flag so if the binary search fails, we
-         won't do the linear search once unless we find some reason to
-         do so */
-
-      do_linear_search = 0;
-      top = BLOCK_NSYMS (block);
-      bot = 0;
-
-      /* Advance BOT to not far before the first symbol whose name is NAME. */
-
-      while (1)
-	{
-	  inc = (top - bot + 1);
-	  /* No need to keep binary searching for the last few bits worth.  */
-	  if (inc < 4)
-	    {
-	      break;
-	    }
-	  inc = (inc >> 1) + bot;
-	  sym = BLOCK_SYM (block, inc);
-	  if (!do_linear_search && (SYMBOL_LANGUAGE (sym) == language_java))
-	    {
-	      do_linear_search = 1;
-	    }
-	  if (SYMBOL_SOURCE_NAME (sym)[0] < name[0])
-	    {
-	      bot = inc;
-	    }
-	  else if (SYMBOL_SOURCE_NAME (sym)[0] > name[0])
-	    {
-	      top = inc;
-	    }
-	  else if (strcmp (SYMBOL_SOURCE_NAME (sym), name) < 0)
-	    {
-	      bot = inc;
-	    }
-	  else
-	    {
-	      top = inc;
-	    }
-	}
-
-      /* Now scan forward until we run out of symbols, find one whose
-         name is greater than NAME, or find one we want.  If there is
-         more than one symbol with the right name and namespace, we
-         return the first one; I believe it is now impossible for us
-         to encounter two symbols with the same name and namespace
-         here, because blocks containing argument symbols are no
-         longer sorted.  The exception is for C++, where multiple functions
-	 (cloned constructors / destructors, in particular) can have
-	 the same demangled name.  So if we have a particular
-	 mangled name to match, try to do so.  */
-
-      top = BLOCK_NSYMS (block);
-      while (bot < top)
-	{
-	  sym = BLOCK_SYM (block, bot);
-	  if (SYMBOL_NAMESPACE (sym) == namespace
-	      && (mangled_name
-		  ? strcmp (SYMBOL_NAME (sym), mangled_name) == 0
-		  : SYMBOL_MATCHES_NAME (sym, name)))
-	    {
-	      return sym;
-	    }
-          if (SYMBOL_SOURCE_NAME (sym)[0] > name[0])
-            {
-              break;
-            }
-	  bot++;
-	}
-    }
-
-  /* Here if block isn't sorted, or we fail to find a match during the
-     binary search above.  If during the binary search above, we find a
-     symbol which is a Java symbol, then we have re-enabled the linear
-     search flag which was reset when starting the binary search.
-
-     This loop is equivalent to the loop above, but hacked greatly for speed.
-
-     Note that parameter symbols do not always show up last in the
-     list; this loop makes sure to take anything else other than
-     parameter symbols first; it only uses parameter symbols as a
-     last resort.  Note that this only takes up extra computation
-     time on a match.  */
-
-  if (do_linear_search)
+  else
     {
+      /* Note that parameter symbols do not always show up last in the
+	 list.  This loop makes sure to take anything else other than
+	 parameter symbols first; it only uses parameter symbols as a
+	 last resort.  Note that this only takes up extra computation
+	 time on a match.  */
       top = BLOCK_NSYMS (block);
       bot = 0;
       while (bot < top)
 	{
 	  sym = BLOCK_SYM (block, bot);
+	  /* If there is more than one symbol with the right name and
+	     namespace, we return the first one; I believe it is now
+	     impossible for us to encounter two symbols with the same
+	     name and namespace here, because blocks containing
+	     argument symbols are no longer sorted.  The exception is
+	     for C++, where multiple functions (cloned constructors /
+	     destructors, in particular) can have the same demangled
+	     name.  So if we have a particular mangled name to match,
+	     try to do so.  */
 	  if (SYMBOL_NAMESPACE (sym) == namespace
 	      && (mangled_name
 		  ? strcmp (SYMBOL_NAME (sym), mangled_name) == 0
@@ -1493,8 +1410,8 @@ lookup_block_symbol (register const stru
 	    }
 	  bot++;
 	}
+      return (sym_found);		/* Will be NULL if not found. */
     }
-  return (sym_found);		/* Will be NULL if not found. */
 }
 
 /* Given a main symbol SYM and ADDR, search through the alias

Re: [rfa] clean up lookup_symbol_aux

[David Carlton]

On 02 Oct 2002 14:50:34 -0700, David Carlton <carlton@math.Stanford.EDU> said:

> * I decided to reorder the search order: in the non-HPUXHPPA case, it
>   makes sense to me to search global psymtabs before searching minimal
>   symbols.  The theory there is that you should look for symbols with
>   debugging information before looking for symbols without debugging
>   information; also it seems to me that the current code might, in
>   some situations, find a static symbol via searching the minsyms
>   before it finds a global symbol via searching the psymtabs, which is
>   definitely a bad outcome.

Actually, I didn't understand lookup_minimal_symbol well enough when I
wrote the above; now I no longer think that the existing code might
find a static symbol via minsyms before a global symbol via psymtabs.

I still think that it would be a good idea to search psymtabs before
searching minimal symbols, but I prefer that only because I think it's
slightly cleaner, not because of any correctness reasons.  (And I'm
not sure which side efficiency concerns land on in this situation;
it's kind of complicated.  For all I know, it might even be more
efficient to move the minimal symbol search before the symtab search!)

And, of course, I still think that the general idea of cleaning up
lookup_symbol_aux and combining duplicate code is a good one.  I'm
just more agnostic about what order the resulting patch should call
lookup_symbol_aux_symtabs, lookup_symbol_aux_psymtabs, and
lookup_symbol_aux_minsyms in.

David Carlton
carlton@math.stanford.edu

Re: [rfa] clean up lookup_symbol_aux

[David Carlton]

Here's a slightly different version of my lookup_symbol_aux cleanup
patch.  It's basically the same patch, except that it tries to mimic
the current flow of control of lookup_symbol_aux even more closely.
Specifically, it no longer moves the psymtab search before the minsym
search (I no longer am convinced that that's a particularly good
idea), and it follows the flow of control from within the minsym
search more closely (the flow of control would seem to be somewhat
bizarre, but that's an issue for a subsequent patch).  Other than
that, my comments from that earlier message still hold, so I'll repeat
them here:

On 02 Oct 2002 14:50:34 -0700, David Carlton
<carlton@math.Stanford.EDU> said:

> When trying to understand lookup_symbol_aux, I found that it helped
> to pull chunks of the code into separate functions (e.g. the code to
> look up a symbol in all the global symtabs); this way, I could
> separate questions like "in what order do we look up symbols in
> various data structures?" from questions like "what exactly do we do
> when we look for a symbol in the partial symbol tables?".  And, once
> I'd done that, I found that some of the resulting functions could be
> combined (e.g. the code to look up a symbol in all the global
> symtabs is essentially the same as the code to look up a symbol in
> all the static symtabs).

> I think that the result is enough clearer than the original that I'm
> probably not the only person who would find it useful.  And
> combining similar chunks of code should help maintenance: e.g. the
> non-HPUXHPPA and HPUXHPPA cases are now combined (i.e. they're both
> calls to lookup_symbol_aux_minsyms, albeit in different places), so
> you don't have to remember to update them both.  (Some people seem
> to have forgotten that in the past: see below.)

> The resulting patch isn't easy to read: to be honest, to understand
> what it's doing, you'll probably want to apply it to a copy of
> symtab.c and compare the results, rather than try to read the patch
> directly.  Here are the Cliff's Notes for reading it:

> Changes that shouldn't affect the functioning of GDB at all:

> * The part of lookup_symbol_aux that never gets run and that is
>   prefaced by 17 lines of comments being bemused at its existence
>   has been deleted.

> * Other long, coherent chunks of lookup_symbol_aux have been moved
>   into separate functions.

> * When two of the new functions were essentially identical, those
>   functions have been merged.  To be specific, there's a single
>   function lookup_symbol_aux_symtabs that can either search global
>   symbols or static symbols, and similarly with psymtab search.

> Changes that might theoretically affect the function of GDB:

> * The two cases for searching minimal symbols (i.e. whether or not
>   you're in HPUXHPPA) turned out to be slightly different.  I went
>   through those differences and picked whichever variant seemed to
>   me to be better; as far as I can tell, there's no reason for the
>   code to differ in the two cases.  I also threw in a bug fix:
>   fixup_symbol_section had the wrong second argument in the variant
>   where it was present.

> I've tested this, and didn't turn up any new regressions.  It would
> be nice if somebody could give it a spin on an HP machine to make
> sure I didn't screw up that variant of the function.

Once this patch is accepted, I'll probably submit more patches which
could actually change the behavior of lookup_symbol_aux, but it seemed
like a good idea to start with a patch that tries to preserve the
current behavior while reducing code duplication.  (And while making
it clear just what that current behavior is...)

David Carlton
carlton@math.stanford.edu

2002-10-30  David Carlton  <carlton@math.stanford.edu>

	* symtab.c (lookup_symbol_aux): Move chunks of code into separate
	functions.
	(lookup_symbol_aux_local): New function.
	(lookup_symbol_aux_symtabs): New function.
	(lookup_symbol_aux_psymtabs): New function.
	(lookup_symbol_aux_minsyms): New function.

Index: symtab.c
===================================================================
RCS file: /cvs/src/src/gdb/symtab.c,v
retrieving revision 1.75
diff -u -p -r1.75 symtab.c
--- symtab.c	28 Oct 2002 17:05:56 -0000	1.75
+++ symtab.c	30 Oct 2002 22:52:55 -0000
@@ -83,6 +83,32 @@ static struct symbol *lookup_symbol_aux 
 					 int *is_a_field_of_this,
 					 struct symtab **symtab);
 
+static struct symbol *lookup_symbol_aux_local (const char *name,
+					       const char *mangled_name,
+					       const struct block *block,
+					       const namespace_enum namespace,
+					       struct symtab **symtab);
+
+static
+struct symbol *lookup_symbol_aux_symtabs (int block_index,
+					  const char *name,
+					  const char *mangled_name,
+					  const namespace_enum namespace,
+					  struct symtab **symtab);
+
+static
+struct symbol *lookup_symbol_aux_psymtabs (int block_index,
+					   const char *name,
+					   const char *mangled_name,
+					   const namespace_enum namespace,
+					   struct symtab **symtab);
+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);
 
 static struct symbol *find_active_alias (struct symbol *sym, CORE_ADDR addr);
 
@@ -766,84 +792,23 @@ lookup_symbol_aux (const char *name, con
 		   const struct block *block, const namespace_enum namespace,
 		   int *is_a_field_of_this, struct symtab **symtab)
 {
-  register struct symbol *sym;
-  register struct symtab *s = NULL;
-  register struct partial_symtab *ps;
-  register struct blockvector *bv;
-  register struct objfile *objfile = NULL;
-  register struct block *b;
-  register struct minimal_symbol *msymbol;
+  struct symbol *sym;
 
+  /* FIXME: carlton/2002-10-30: 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
+     don't yet have an informed opinion as to the circumstances in
+     which that's a good idea (if ever).  */
+  
+  int force_return;
 
   /* Search specified block and its superiors.  */
 
-  while (block != 0)
-    {
-      sym = lookup_block_symbol (block, name, mangled_name, namespace);
-      if (sym)
-	{
-	  block_found = block;
-	  if (symtab != NULL)
-	    {
-	      /* Search the list of symtabs for one which contains the
-	         address of the start of this block.  */
-	      ALL_SYMTABS (objfile, s)
-	      {
-		bv = BLOCKVECTOR (s);
-		b = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
-		if (BLOCK_START (b) <= BLOCK_START (block)
-		    && BLOCK_END (b) > BLOCK_START (block))
-		  goto found;
-	      }
-	    found:
-	      *symtab = s;
-	    }
-
-	  return fixup_symbol_section (sym, objfile);
-	}
-      block = BLOCK_SUPERBLOCK (block);
-    }
-
-  /* FIXME: this code is never executed--block is always NULL at this
-     point.  What is it trying to do, anyway?  We already should have
-     checked the STATIC_BLOCK above (it is the superblock of top-level
-     blocks).  Why is VAR_NAMESPACE special-cased?  */
-  /* Don't need to mess with the psymtabs; if we have a block,
-     that file is read in.  If we don't, then we deal later with
-     all the psymtab stuff that needs checking.  */
-  /* Note (RT): The following never-executed code looks unnecessary to me also.
-   * If we change the code to use the original (passed-in)
-   * value of 'block', we could cause it to execute, but then what
-   * would it do? The STATIC_BLOCK of the symtab containing the passed-in
-   * 'block' was already searched by the above code. And the STATIC_BLOCK's
-   * of *other* symtabs (those files not containing 'block' lexically)
-   * should not contain 'block' address-wise. So we wouldn't expect this
-   * code to find any 'sym''s that were not found above. I vote for 
-   * deleting the following paragraph of code.
-   */
-  if (namespace == VAR_NAMESPACE && block != NULL)
-    {
-      struct block *b;
-      /* Find the right symtab.  */
-      ALL_SYMTABS (objfile, s)
-      {
-	bv = BLOCKVECTOR (s);
-	b = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
-	if (BLOCK_START (b) <= BLOCK_START (block)
-	    && BLOCK_END (b) > BLOCK_START (block))
-	  {
-	    sym = lookup_block_symbol (b, name, mangled_name, VAR_NAMESPACE);
-	    if (sym)
-	      {
-		block_found = b;
-		if (symtab != NULL)
-		  *symtab = s;
-		return fixup_symbol_section (sym, objfile);
-	      }
-	  }
-      }
-    }
-
+  sym = lookup_symbol_aux_local (name, mangled_name, block, namespace,
+				 symtab);
+  if (sym != NULL)
+    return sym;
 
   /* C++: If requested to do so by the caller, 
      check to see if NAME is a field of `this'. */
@@ -866,128 +831,151 @@ lookup_symbol_aux (const char *name, con
      of the desired name as a global, then do psymtab-to-symtab
      conversion on the fly and return the found symbol. */
 
-  ALL_SYMTABS (objfile, s)
-  {
-    bv = BLOCKVECTOR (s);
-    block = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
-    sym = lookup_block_symbol (block, name, mangled_name, namespace);
-    if (sym)
-      {
-	block_found = block;
-	if (symtab != NULL)
-	  *symtab = s;
-	return fixup_symbol_section (sym, objfile);
-      }
-  }
+  sym = lookup_symbol_aux_symtabs (GLOBAL_BLOCK, name, mangled_name,
+				   namespace, symtab);
+  if (sym != NULL)
+    return sym;
 
 #ifndef HPUXHPPA
 
-  /* 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.  */
+  /* 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.  */
+
+  force_return = 0;
+
+  sym = lookup_symbol_aux_minsyms (name, mangled_name,
+				   namespace, is_a_field_of_this,
+				   symtab, &force_return);
+  
+  if (sym != NULL || force_return == 1)
+    return sym;
 
-  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)
-	    {
-	      /* This is a function which has a symtab for its address.  */
-	      bv = BLOCKVECTOR (s);
-	      block = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
+#endif
 
-              /* 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.  */
-	      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. */
-	      if (!sym)
-		{
-		  block = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
-		  sym = lookup_block_symbol (block, name,
-                                             mangled_name, namespace);
-		}
+  sym = lookup_symbol_aux_psymtabs (GLOBAL_BLOCK, name, mangled_name,
+				    namespace, symtab);
+  if (sym != NULL)
+    return sym;
 
-	      /* sym == 0 if symbol was found in the minimal symbol table
-	         but not in the symtab.
-	         Return 0 to use the msymbol definition of "foo_".
+  /* Now search all static file-level symbols.  Not strictly correct,
+     but more useful than an error.  Do the symtabs first, then check
+     the psymtabs.  If a psymtab indicates the existence of the
+     desired name as a file-level static, then do psymtab-to-symtab
+     conversion on the fly and return the found symbol. */
 
-	         This happens for Fortran  "foo_" symbols,
-	         which are "foo" in the symtab.
+  sym = lookup_symbol_aux_symtabs (STATIC_BLOCK, name, mangled_name,
+				   namespace, symtab);
+  if (sym != NULL)
+    return sym;
+  
+  sym = lookup_symbol_aux_psymtabs (STATIC_BLOCK, name, mangled_name,
+				    namespace, symtab);
+  if (sym != NULL)
+    return sym;
 
-	         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:");
-	       */
 
-	      if (symtab != NULL)
-		*symtab = s;
-	      return fixup_symbol_section (sym, objfile);
-	    }
-	  else if (MSYMBOL_TYPE (msymbol) != mst_text
-		   && MSYMBOL_TYPE (msymbol) != mst_file_text
-		   && !STREQ (name, SYMBOL_NAME (msymbol)))
+#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 within
+     lookup_symbol_aux_minsyms which talks about FORTRAN routines
+     "foo_" though... is it saying we need to do the "minsym" check
+     before the static check in this case?  */
+
+  force_return = 0;
+
+  sym = lookup_symbol_aux_minsyms (name, mangled_name,
+				   namespace, is_a_field_of_this,
+				   symtab, &force_return);
+  
+  if (sym != NULL || force_return == 1)
+    return sym;
+
+#endif
+
+  if (symtab != NULL)
+    *symtab = NULL;
+  return NULL;
+}
+
+/* Check to see if the symbol is defined in BLOCK or its
+   superiors.  */
+
+static struct symbol *
+lookup_symbol_aux_local (const char *name, const char *mangled_name,
+			 const struct block *block,
+			 const namespace_enum namespace,
+			 struct symtab **symtab)
+{
+  struct symbol *sym;
+  struct objfile *objfile = NULL;
+  struct blockvector *bv;
+  struct block *b;
+  struct symtab *s = NULL;
+  
+  while (block != 0)
+    {
+      sym = lookup_block_symbol (block, name, mangled_name, namespace);
+      if (sym)
+	{
+	  block_found = block;
+	  if (symtab != NULL)
 	    {
-	      /* This is a mangled variable, look it up by its
-	         mangled name.  */
-	      return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name, block,
-					namespace, is_a_field_of_this, symtab);
+	      /* Search the list of symtabs for one which contains the
+	         address of the start of this block.  */
+	      ALL_SYMTABS (objfile, s)
+	      {
+		bv = BLOCKVECTOR (s);
+		b = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
+		if (BLOCK_START (b) <= BLOCK_START (block)
+		    && BLOCK_END (b) > BLOCK_START (block))
+		  goto found;
+	      }
+	    found:
+	      *symtab = s;
 	    }
-	  /* There are no debug symbols for this file, or we are looking
-	     for an unmangled variable.
-	     Try to find a matching static symbol below. */
+
+	  return fixup_symbol_section (sym, objfile);
 	}
+      block = BLOCK_SUPERBLOCK (block);
     }
 
-#endif
+  return NULL;
+}
 
-  ALL_PSYMTABS (objfile, ps)
-  {
-    if (!ps->readin && lookup_partial_symbol (ps, name, 1, namespace))
-      {
-	s = PSYMTAB_TO_SYMTAB (ps);
-	bv = BLOCKVECTOR (s);
-	block = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
-	sym = lookup_block_symbol (block, name, mangled_name, namespace);
-	if (!sym)
-	  {
-	    /* This shouldn't be necessary, but as a last resort
-	     * try looking in the statics even though the psymtab
-	     * claimed the symbol was global. It's possible that
-	     * the psymtab gets it wrong in some cases.
-	     */
-	    block = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
-	    sym = lookup_block_symbol (block, name, mangled_name, namespace);
-	    if (!sym)
-	      error ("Internal: global symbol `%s' found in %s psymtab but not in symtab.\n\
-%s may be an inlined function, or may be a template function\n\
-(if a template, try specifying an instantiation: %s<type>).",
-		     name, ps->filename, name, name);
-	  }
-	if (symtab != NULL)
-	  *symtab = s;
-	return fixup_symbol_section (sym, objfile);
-      }
-  }
+/* Check to see if the symbol is defined in one of the symtabs.
+   BLOCK_INDEX should be either GLOBAL_BLOCK or STATIC_BLOCK,
+   depending on whether or not we want to search global symbols or
+   static symbols.  */
 
-  /* Now search all static file-level symbols.
-     Not strictly correct, but more useful than an error.
-     Do the symtabs first, then check the psymtabs.
-     If a psymtab indicates the existence
-     of the desired name as a file-level static, then do psymtab-to-symtab
-     conversion on the fly and return the found symbol. */
+static struct symbol *
+lookup_symbol_aux_symtabs (int block_index,
+			   const char *name, const char *mangled_name,
+			   const namespace_enum namespace,
+			   struct symtab **symtab)
+{
+  struct symbol *sym;
+  struct objfile *objfile;
+  struct blockvector *bv;
+  const struct block *block;
+  struct symtab *s;
 
   ALL_SYMTABS (objfile, s)
   {
     bv = BLOCKVECTOR (s);
-    block = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
+    block = BLOCKVECTOR_BLOCK (bv, block_index);
     sym = lookup_block_symbol (block, name, mangled_name, namespace);
     if (sym)
       {
@@ -998,27 +986,57 @@ lookup_symbol_aux (const char *name, con
       }
   }
 
+  return NULL;
+}
+
+/* Check to see if the symbol is defined in one of the partial
+   symtabs.  BLOCK_INDEX should be either GLOBAL_BLOCK or
+   STATIC_BLOCK, depending on whether or not we want to search global
+   symbols or static symbols.  */
+
+static struct symbol *
+lookup_symbol_aux_psymtabs (int block_index, const char *name,
+			    const char *mangled_name,
+			    const namespace_enum namespace,
+			    struct symtab **symtab)
+{
+  struct symbol *sym;
+  struct objfile *objfile;
+  struct blockvector *bv;
+  const struct block *block;
+  struct partial_symtab *ps;
+  struct symtab *s;
+  const int psymtab_index = (block_index == GLOBAL_BLOCK ? 1 : 0);
+
   ALL_PSYMTABS (objfile, ps)
   {
-    if (!ps->readin && lookup_partial_symbol (ps, name, 0, namespace))
+    if (!ps->readin
+	&& lookup_partial_symbol (ps, name, psymtab_index, namespace))
       {
 	s = PSYMTAB_TO_SYMTAB (ps);
 	bv = BLOCKVECTOR (s);
-	block = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
+	block = BLOCKVECTOR_BLOCK (bv, block_index);
 	sym = lookup_block_symbol (block, name, mangled_name, namespace);
 	if (!sym)
 	  {
-	    /* This shouldn't be necessary, but as a last resort
-	     * try looking in the globals even though the psymtab
-	     * claimed the symbol was static. It's possible that
-	     * the psymtab gets it wrong in some cases.
-	     */
-	    block = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
+	    /* This shouldn't be necessary, but as a last resort try
+	       looking in the statics even though the psymtab claimed
+	       the symbol was global, or vice-versa. It's possible
+	       that the psymtab gets it wrong in some cases.  */
+
+	    /* FIXME: carlton/2002-09-30: Should we really do that?
+	       If that happens, isn't it likely to be a GDB error, in
+	       which case we should fix the GDB error rather than
+	       silently dealing with it here?  So I'd vote for
+	       removing the check for the symbol in the other
+	       block.  */
+	    block = BLOCKVECTOR_BLOCK (bv,
+				       block_index == GLOBAL_BLOCK ?
+				       STATIC_BLOCK : GLOBAL_BLOCK);
 	    sym = lookup_block_symbol (block, name, mangled_name, namespace);
 	    if (!sym)
-	      error ("Internal: static symbol `%s' found in %s psymtab but not in symtab.\n\
-%s may be an inlined function, or may be a template function\n\
-(if a template, try specifying an instantiation: %s<type>).",
+	      error ("Internal: %s symbol `%s' found in %s psymtab but not in symtab.\n%s may be an inlined function, or may be a template function\n(if a template, try specifying an instantiation: %s<type>).",
+		     block_index == GLOBAL_BLOCK ? "global" : "static",
 		     name, ps->filename, name, name);
 	  }
 	if (symtab != NULL)
@@ -1027,79 +1045,75 @@ lookup_symbol_aux (const char *name, con
       }
   }
 
-#ifdef HPUXHPPA
+  return NULL;
+}
 
-  /* 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.
+/* 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.  */
 
-     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? 
-   */
+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 symbol *sym;
+  struct blockvector *bv;
+  const struct block *block;
+  struct minimal_symbol *msymbol;
+  struct symtab *s;
 
   if (namespace == VAR_NAMESPACE)
     {
       msymbol = lookup_minimal_symbol (name, NULL, NULL);
+
       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_symtab()
-	   * has a side-effect of doing psymtab-to-symtab expansion,
-	   * for the found symtab.
-	   */
-	  s = find_pc_symtab (SYMBOL_VALUE_ADDRESS (msymbol));
+	  /* 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.  */
+	  s = find_pc_sect_symtab (SYMBOL_VALUE_ADDRESS (msymbol),
+				   SYMBOL_BFD_SECTION (msymbol));
 	  if (s != NULL)
 	    {
+	      /* This is a function which has a symtab for its address.  */
 	      bv = BLOCKVECTOR (s);
 	      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.  */
-	      sym = lookup_block_symbol (block, name, mangled_name, namespace);
+
+	      /* 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.  */
+	      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. */
 	      if (!sym)
 		{
 		  block = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
 		  sym = lookup_block_symbol (block, name,
-                                             mangled_name, namespace);
-		}
-	      /* If we found one, return it */
-	      if (sym)
-		{
-		  if (symtab != NULL)
-		    *symtab = s;
-		  return sym;
+					     mangled_name, namespace);
 		}
 
-	      /* If we get here with sym == 0, the symbol was 
-	         found in the minimal symbol table
+	      /* sym == 0 if 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).
+	         Return 0 to use the msymbol definition of "foo_".
 
 	         This happens for Fortran  "foo_" symbols,
 	         which are "foo" in the symtab.
@@ -1109,28 +1123,27 @@ lookup_symbol_aux (const char *name, con
 	         asm(".globl _main");
 	         asm("_main:");
 	       */
-	    }
 
-	  /* If the lookup-by-address fails, try repeating the
-	   * entire lookup process with the symbol name from
-	   * the msymbol (if different from the original symbol name).
-	   */
+	      if (symtab != NULL)
+		*symtab = s;
+	      *force_return = 1;
+	      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.  */
+	      *force_return = 1;
 	      return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name,
-					block, namespace, is_a_field_of_this,
+					NULL, namespace, is_a_field_of_this,
 					symtab);
 	    }
 	}
     }
 
-#endif
-
-  if (symtab != NULL)
-    *symtab = NULL;
-  return 0;
+  return NULL;
 }
 								
 /* Look, in partial_symtab PST, for symbol NAME.  Check the global

Re: [rfa] clean up lookup_symbol_aux

[Elena Zannoni]

David Carlton writes:
 > Here's a slightly different version of my lookup_symbol_aux cleanup
 > patch.  It's basically the same patch, except that it tries to mimic
 > the current flow of control of lookup_symbol_aux even more closely.
 > Specifically, it no longer moves the psymtab search before the minsym
 > search (I no longer am convinced that that's a particularly good
 > idea), and it follows the flow of control from within the minsym
 > search more closely (the flow of control would seem to be somewhat
 > bizarre, but that's an issue for a subsequent patch).  Other than
 > that, my comments from that earlier message still hold, so I'll repeat
 > them here:
 > 
 > On 02 Oct 2002 14:50:34 -0700, David Carlton
 > <carlton@math.Stanford.EDU> said:
 > 
 > > When trying to understand lookup_symbol_aux, I found that it helped
 > > to pull chunks of the code into separate functions (e.g. the code to
 > > look up a symbol in all the global symtabs); this way, I could
 > > separate questions like "in what order do we look up symbols in
 > > various data structures?" from questions like "what exactly do we do
 > > when we look for a symbol in the partial symbol tables?".  And, once
 > > I'd done that, I found that some of the resulting functions could be
 > > combined (e.g. the code to look up a symbol in all the global
 > > symtabs is essentially the same as the code to look up a symbol in
 > > all the static symtabs).
 > 
 > > I think that the result is enough clearer than the original that I'm
 > > probably not the only person who would find it useful.  And
 > > combining similar chunks of code should help maintenance: e.g. the
 > > non-HPUXHPPA and HPUXHPPA cases are now combined (i.e. they're both
 > > calls to lookup_symbol_aux_minsyms, albeit in different places), so
 > > you don't have to remember to update them both.  (Some people seem
 > > to have forgotten that in the past: see below.)
 > 
 > > The resulting patch isn't easy to read: to be honest, to understand
 > > what it's doing, you'll probably want to apply it to a copy of
 > > symtab.c and compare the results, rather than try to read the patch
 > > directly.  Here are the Cliff's Notes for reading it:
 > 
 > > Changes that shouldn't affect the functioning of GDB at all:
 > 
 > > * The part of lookup_symbol_aux that never gets run and that is
 > >   prefaced by 17 lines of comments being bemused at its existence
 > >   has been deleted.

Could it be if deffed 0 for a bit, to see if we get some weird behaviors?
We can delete it later. Make sure you add a comment with a date.

 > 
 > > * Other long, coherent chunks of lookup_symbol_aux have been moved
 > >   into separate functions.
 > 

Yes

 > > * When two of the new functions were essentially identical, those
 > >   functions have been merged.  To be specific, there's a single
 > >   function lookup_symbol_aux_symtabs that can either search global
 > >   symbols or static symbols, and similarly with psymtab search.
 > 

yes

 > > Changes that might theoretically affect the function of GDB:
 > 
 > > * The two cases for searching minimal symbols (i.e. whether or not
 > >   you're in HPUXHPPA) turned out to be slightly different.  I went
 > >   through those differences and picked whichever variant seemed to
 > >   me to be better; as far as I can tell, there's no reason for the
 > >   code to differ in the two cases.  I also threw in a bug fix:
 > >   fixup_symbol_section had the wrong second argument in the variant
 > >   where it was present.
 > 

This one I am a bit worried about.  For the bugfix, I would prefer
that a separate patch is submitted, I cannot spot the fix in the patch.

I would check in the functions except the minsyms one, ifdeffing
the code that was deleted. Separately submit a patch for the bugfix,
and one just for the minsyms changes so that it can be tested independentely.

 > > I've tested this, and didn't turn up any new regressions.  It would
 > > be nice if somebody could give it a spin on an HP machine to make
 > > sure I didn't screw up that variant of the function.
 > 

Maybe Joel can test the hppa patch?

 > Once this patch is accepted, I'll probably submit more patches which
 > could actually change the behavior of lookup_symbol_aux, but it seemed
 > like a good idea to start with a patch that tries to preserve the
 > current behavior while reducing code duplication.  (And while making
 > it clear just what that current behavior is...)
 > 

Yes, mechanical changes should be separated from more functional ones.

Elena


 > David Carlton
 > carlton@math.stanford.edu
 > 
 > 2002-10-30  David Carlton  <carlton@math.stanford.edu>
 > 
 > 	* symtab.c (lookup_symbol_aux): Move chunks of code into separate
 > 	functions.
 > 	(lookup_symbol_aux_local): New function.
 > 	(lookup_symbol_aux_symtabs): New function.
 > 	(lookup_symbol_aux_psymtabs): New function.
 > 	(lookup_symbol_aux_minsyms): New function.
 > 
 > Index: symtab.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/symtab.c,v
 > retrieving revision 1.75
 > diff -u -p -r1.75 symtab.c
 > --- symtab.c	28 Oct 2002 17:05:56 -0000	1.75
 > +++ symtab.c	30 Oct 2002 22:52:55 -0000
 > @@ -83,6 +83,32 @@ static struct symbol *lookup_symbol_aux 
 >  					 int *is_a_field_of_this,
 >  					 struct symtab **symtab);
 >  
 > +static struct symbol *lookup_symbol_aux_local (const char *name,
 > +					       const char *mangled_name,
 > +					       const struct block *block,
 > +					       const namespace_enum namespace,
 > +					       struct symtab **symtab);
 > +
 > +static
 > +struct symbol *lookup_symbol_aux_symtabs (int block_index,
 > +					  const char *name,
 > +					  const char *mangled_name,
 > +					  const namespace_enum namespace,
 > +					  struct symtab **symtab);
 > +
 > +static
 > +struct symbol *lookup_symbol_aux_psymtabs (int block_index,
 > +					   const char *name,
 > +					   const char *mangled_name,
 > +					   const namespace_enum namespace,
 > +					   struct symtab **symtab);
 > +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);
 >  
 >  static struct symbol *find_active_alias (struct symbol *sym, CORE_ADDR addr);
 >  
 > @@ -766,84 +792,23 @@ lookup_symbol_aux (const char *name, con
 >  		   const struct block *block, const namespace_enum namespace,
 >  		   int *is_a_field_of_this, struct symtab **symtab)
 >  {
 > -  register struct symbol *sym;
 > -  register struct symtab *s = NULL;
 > -  register struct partial_symtab *ps;
 > -  register struct blockvector *bv;
 > -  register struct objfile *objfile = NULL;
 > -  register struct block *b;
 > -  register struct minimal_symbol *msymbol;
 > +  struct symbol *sym;
 >  
 > +  /* FIXME: carlton/2002-10-30: 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
 > +     don't yet have an informed opinion as to the circumstances in
 > +     which that's a good idea (if ever).  */
 > +  
 > +  int force_return;
 >  
 >    /* Search specified block and its superiors.  */
 >  
 > -  while (block != 0)
 > -    {
 > -      sym = lookup_block_symbol (block, name, mangled_name, namespace);
 > -      if (sym)
 > -	{
 > -	  block_found = block;
 > -	  if (symtab != NULL)
 > -	    {
 > -	      /* Search the list of symtabs for one which contains the
 > -	         address of the start of this block.  */
 > -	      ALL_SYMTABS (objfile, s)
 > -	      {
 > -		bv = BLOCKVECTOR (s);
 > -		b = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
 > -		if (BLOCK_START (b) <= BLOCK_START (block)
 > -		    && BLOCK_END (b) > BLOCK_START (block))
 > -		  goto found;
 > -	      }
 > -	    found:
 > -	      *symtab = s;
 > -	    }
 > -
 > -	  return fixup_symbol_section (sym, objfile);
 > -	}
 > -      block = BLOCK_SUPERBLOCK (block);
 > -    }
 > -
 > -  /* FIXME: this code is never executed--block is always NULL at this
 > -     point.  What is it trying to do, anyway?  We already should have
 > -     checked the STATIC_BLOCK above (it is the superblock of top-level
 > -     blocks).  Why is VAR_NAMESPACE special-cased?  */
 > -  /* Don't need to mess with the psymtabs; if we have a block,
 > -     that file is read in.  If we don't, then we deal later with
 > -     all the psymtab stuff that needs checking.  */
 > -  /* Note (RT): The following never-executed code looks unnecessary to me also.
 > -   * If we change the code to use the original (passed-in)
 > -   * value of 'block', we could cause it to execute, but then what
 > -   * would it do? The STATIC_BLOCK of the symtab containing the passed-in
 > -   * 'block' was already searched by the above code. And the STATIC_BLOCK's
 > -   * of *other* symtabs (those files not containing 'block' lexically)
 > -   * should not contain 'block' address-wise. So we wouldn't expect this
 > -   * code to find any 'sym''s that were not found above. I vote for 
 > -   * deleting the following paragraph of code.
 > -   */
 > -  if (namespace == VAR_NAMESPACE && block != NULL)
 > -    {
 > -      struct block *b;
 > -      /* Find the right symtab.  */
 > -      ALL_SYMTABS (objfile, s)
 > -      {
 > -	bv = BLOCKVECTOR (s);
 > -	b = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
 > -	if (BLOCK_START (b) <= BLOCK_START (block)
 > -	    && BLOCK_END (b) > BLOCK_START (block))
 > -	  {
 > -	    sym = lookup_block_symbol (b, name, mangled_name, VAR_NAMESPACE);
 > -	    if (sym)
 > -	      {
 > -		block_found = b;
 > -		if (symtab != NULL)
 > -		  *symtab = s;
 > -		return fixup_symbol_section (sym, objfile);
 > -	      }
 > -	  }
 > -      }
 > -    }
 > -
 > +  sym = lookup_symbol_aux_local (name, mangled_name, block, namespace,
 > +				 symtab);
 > +  if (sym != NULL)
 > +    return sym;
 >  
 >    /* C++: If requested to do so by the caller, 
 >       check to see if NAME is a field of `this'. */
 > @@ -866,128 +831,151 @@ lookup_symbol_aux (const char *name, con
 >       of the desired name as a global, then do psymtab-to-symtab
 >       conversion on the fly and return the found symbol. */
 >  
 > -  ALL_SYMTABS (objfile, s)
 > -  {
 > -    bv = BLOCKVECTOR (s);
 > -    block = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
 > -    sym = lookup_block_symbol (block, name, mangled_name, namespace);
 > -    if (sym)
 > -      {
 > -	block_found = block;
 > -	if (symtab != NULL)
 > -	  *symtab = s;
 > -	return fixup_symbol_section (sym, objfile);
 > -      }
 > -  }
 > +  sym = lookup_symbol_aux_symtabs (GLOBAL_BLOCK, name, mangled_name,
 > +				   namespace, symtab);
 > +  if (sym != NULL)
 > +    return sym;
 >  
 >  #ifndef HPUXHPPA
 >  
 > -  /* 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.  */
 > +  /* 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.  */
 > +
 > +  force_return = 0;
 > +
 > +  sym = lookup_symbol_aux_minsyms (name, mangled_name,
 > +				   namespace, is_a_field_of_this,
 > +				   symtab, &force_return);
 > +  
 > +  if (sym != NULL || force_return == 1)
 > +    return sym;
 >  
 > -  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)
 > -	    {
 > -	      /* This is a function which has a symtab for its address.  */
 > -	      bv = BLOCKVECTOR (s);
 > -	      block = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
 > +#endif
 >  
 > -              /* 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.  */
 > -	      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. */
 > -	      if (!sym)
 > -		{
 > -		  block = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
 > -		  sym = lookup_block_symbol (block, name,
 > -                                             mangled_name, namespace);
 > -		}
 > +  sym = lookup_symbol_aux_psymtabs (GLOBAL_BLOCK, name, mangled_name,
 > +				    namespace, symtab);
 > +  if (sym != NULL)
 > +    return sym;
 >  
 > -	      /* sym == 0 if symbol was found in the minimal symbol table
 > -	         but not in the symtab.
 > -	         Return 0 to use the msymbol definition of "foo_".
 > +  /* Now search all static file-level symbols.  Not strictly correct,
 > +     but more useful than an error.  Do the symtabs first, then check
 > +     the psymtabs.  If a psymtab indicates the existence of the
 > +     desired name as a file-level static, then do psymtab-to-symtab
 > +     conversion on the fly and return the found symbol. */
 >  
 > -	         This happens for Fortran  "foo_" symbols,
 > -	         which are "foo" in the symtab.
 > +  sym = lookup_symbol_aux_symtabs (STATIC_BLOCK, name, mangled_name,
 > +				   namespace, symtab);
 > +  if (sym != NULL)
 > +    return sym;
 > +  
 > +  sym = lookup_symbol_aux_psymtabs (STATIC_BLOCK, name, mangled_name,
 > +				    namespace, symtab);
 > +  if (sym != NULL)
 > +    return sym;
 >  
 > -	         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:");
 > -	       */
 >  
 > -	      if (symtab != NULL)
 > -		*symtab = s;
 > -	      return fixup_symbol_section (sym, objfile);
 > -	    }
 > -	  else if (MSYMBOL_TYPE (msymbol) != mst_text
 > -		   && MSYMBOL_TYPE (msymbol) != mst_file_text
 > -		   && !STREQ (name, SYMBOL_NAME (msymbol)))
 > +#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 within
 > +     lookup_symbol_aux_minsyms which talks about FORTRAN routines
 > +     "foo_" though... is it saying we need to do the "minsym" check
 > +     before the static check in this case?  */
 > +
 > +  force_return = 0;
 > +
 > +  sym = lookup_symbol_aux_minsyms (name, mangled_name,
 > +				   namespace, is_a_field_of_this,
 > +				   symtab, &force_return);
 > +  
 > +  if (sym != NULL || force_return == 1)
 > +    return sym;
 > +
 > +#endif
 > +
 > +  if (symtab != NULL)
 > +    *symtab = NULL;
 > +  return NULL;
 > +}
 > +
 > +/* Check to see if the symbol is defined in BLOCK or its
 > +   superiors.  */
 > +
 > +static struct symbol *
 > +lookup_symbol_aux_local (const char *name, const char *mangled_name,
 > +			 const struct block *block,
 > +			 const namespace_enum namespace,
 > +			 struct symtab **symtab)
 > +{
 > +  struct symbol *sym;
 > +  struct objfile *objfile = NULL;
 > +  struct blockvector *bv;
 > +  struct block *b;
 > +  struct symtab *s = NULL;
 > +  
 > +  while (block != 0)
 > +    {
 > +      sym = lookup_block_symbol (block, name, mangled_name, namespace);
 > +      if (sym)
 > +	{
 > +	  block_found = block;
 > +	  if (symtab != NULL)
 >  	    {
 > -	      /* This is a mangled variable, look it up by its
 > -	         mangled name.  */
 > -	      return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name, block,
 > -					namespace, is_a_field_of_this, symtab);
 > +	      /* Search the list of symtabs for one which contains the
 > +	         address of the start of this block.  */
 > +	      ALL_SYMTABS (objfile, s)
 > +	      {
 > +		bv = BLOCKVECTOR (s);
 > +		b = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
 > +		if (BLOCK_START (b) <= BLOCK_START (block)
 > +		    && BLOCK_END (b) > BLOCK_START (block))
 > +		  goto found;
 > +	      }
 > +	    found:
 > +	      *symtab = s;
 >  	    }
 > -	  /* There are no debug symbols for this file, or we are looking
 > -	     for an unmangled variable.
 > -	     Try to find a matching static symbol below. */
 > +
 > +	  return fixup_symbol_section (sym, objfile);
 >  	}
 > +      block = BLOCK_SUPERBLOCK (block);
 >      }
 >  
 > -#endif
 > +  return NULL;
 > +}
 >  
 > -  ALL_PSYMTABS (objfile, ps)
 > -  {
 > -    if (!ps->readin && lookup_partial_symbol (ps, name, 1, namespace))
 > -      {
 > -	s = PSYMTAB_TO_SYMTAB (ps);
 > -	bv = BLOCKVECTOR (s);
 > -	block = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
 > -	sym = lookup_block_symbol (block, name, mangled_name, namespace);
 > -	if (!sym)
 > -	  {
 > -	    /* This shouldn't be necessary, but as a last resort
 > -	     * try looking in the statics even though the psymtab
 > -	     * claimed the symbol was global. It's possible that
 > -	     * the psymtab gets it wrong in some cases.
 > -	     */
 > -	    block = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
 > -	    sym = lookup_block_symbol (block, name, mangled_name, namespace);
 > -	    if (!sym)
 > -	      error ("Internal: global symbol `%s' found in %s psymtab but not in symtab.\n\
 > -%s may be an inlined function, or may be a template function\n\
 > -(if a template, try specifying an instantiation: %s<type>).",
 > -		     name, ps->filename, name, name);
 > -	  }
 > -	if (symtab != NULL)
 > -	  *symtab = s;
 > -	return fixup_symbol_section (sym, objfile);
 > -      }
 > -  }
 > +/* Check to see if the symbol is defined in one of the symtabs.
 > +   BLOCK_INDEX should be either GLOBAL_BLOCK or STATIC_BLOCK,
 > +   depending on whether or not we want to search global symbols or
 > +   static symbols.  */
 >  
 > -  /* Now search all static file-level symbols.
 > -     Not strictly correct, but more useful than an error.
 > -     Do the symtabs first, then check the psymtabs.
 > -     If a psymtab indicates the existence
 > -     of the desired name as a file-level static, then do psymtab-to-symtab
 > -     conversion on the fly and return the found symbol. */
 > +static struct symbol *
 > +lookup_symbol_aux_symtabs (int block_index,
 > +			   const char *name, const char *mangled_name,
 > +			   const namespace_enum namespace,
 > +			   struct symtab **symtab)
 > +{
 > +  struct symbol *sym;
 > +  struct objfile *objfile;
 > +  struct blockvector *bv;
 > +  const struct block *block;
 > +  struct symtab *s;
 >  
 >    ALL_SYMTABS (objfile, s)
 >    {
 >      bv = BLOCKVECTOR (s);
 > -    block = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
 > +    block = BLOCKVECTOR_BLOCK (bv, block_index);
 >      sym = lookup_block_symbol (block, name, mangled_name, namespace);
 >      if (sym)
 >        {
 > @@ -998,27 +986,57 @@ lookup_symbol_aux (const char *name, con
 >        }
 >    }
 >  
 > +  return NULL;
 > +}
 > +
 > +/* Check to see if the symbol is defined in one of the partial
 > +   symtabs.  BLOCK_INDEX should be either GLOBAL_BLOCK or
 > +   STATIC_BLOCK, depending on whether or not we want to search global
 > +   symbols or static symbols.  */
 > +
 > +static struct symbol *
 > +lookup_symbol_aux_psymtabs (int block_index, const char *name,
 > +			    const char *mangled_name,
 > +			    const namespace_enum namespace,
 > +			    struct symtab **symtab)
 > +{
 > +  struct symbol *sym;
 > +  struct objfile *objfile;
 > +  struct blockvector *bv;
 > +  const struct block *block;
 > +  struct partial_symtab *ps;
 > +  struct symtab *s;
 > +  const int psymtab_index = (block_index == GLOBAL_BLOCK ? 1 : 0);
 > +
 >    ALL_PSYMTABS (objfile, ps)
 >    {
 > -    if (!ps->readin && lookup_partial_symbol (ps, name, 0, namespace))
 > +    if (!ps->readin
 > +	&& lookup_partial_symbol (ps, name, psymtab_index, namespace))
 >        {
 >  	s = PSYMTAB_TO_SYMTAB (ps);
 >  	bv = BLOCKVECTOR (s);
 > -	block = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
 > +	block = BLOCKVECTOR_BLOCK (bv, block_index);
 >  	sym = lookup_block_symbol (block, name, mangled_name, namespace);
 >  	if (!sym)
 >  	  {
 > -	    /* This shouldn't be necessary, but as a last resort
 > -	     * try looking in the globals even though the psymtab
 > -	     * claimed the symbol was static. It's possible that
 > -	     * the psymtab gets it wrong in some cases.
 > -	     */
 > -	    block = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
 > +	    /* This shouldn't be necessary, but as a last resort try
 > +	       looking in the statics even though the psymtab claimed
 > +	       the symbol was global, or vice-versa. It's possible
 > +	       that the psymtab gets it wrong in some cases.  */
 > +
 > +	    /* FIXME: carlton/2002-09-30: Should we really do that?
 > +	       If that happens, isn't it likely to be a GDB error, in
 > +	       which case we should fix the GDB error rather than
 > +	       silently dealing with it here?  So I'd vote for
 > +	       removing the check for the symbol in the other
 > +	       block.  */
 > +	    block = BLOCKVECTOR_BLOCK (bv,
 > +				       block_index == GLOBAL_BLOCK ?
 > +				       STATIC_BLOCK : GLOBAL_BLOCK);
 >  	    sym = lookup_block_symbol (block, name, mangled_name, namespace);
 >  	    if (!sym)
 > -	      error ("Internal: static symbol `%s' found in %s psymtab but not in symtab.\n\
 > -%s may be an inlined function, or may be a template function\n\
 > -(if a template, try specifying an instantiation: %s<type>).",
 > +	      error ("Internal: %s symbol `%s' found in %s psymtab but not in symtab.\n%s may be an inlined function, or may be a template function\n(if a template, try specifying an instantiation: %s<type>).",
 > +		     block_index == GLOBAL_BLOCK ? "global" : "static",
 >  		     name, ps->filename, name, name);
 >  	  }
 >  	if (symtab != NULL)
 > @@ -1027,79 +1045,75 @@ lookup_symbol_aux (const char *name, con
 >        }
 >    }
 >  
 > -#ifdef HPUXHPPA
 > +  return NULL;
 > +}
 >  
 > -  /* 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.
 > +/* 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.  */
 >  
 > -     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? 
 > -   */
 > +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 symbol *sym;
 > +  struct blockvector *bv;
 > +  const struct block *block;
 > +  struct minimal_symbol *msymbol;
 > +  struct symtab *s;
 >  
 >    if (namespace == VAR_NAMESPACE)
 >      {
 >        msymbol = lookup_minimal_symbol (name, NULL, NULL);
 > +
 >        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_symtab()
 > -	   * has a side-effect of doing psymtab-to-symtab expansion,
 > -	   * for the found symtab.
 > -	   */
 > -	  s = find_pc_symtab (SYMBOL_VALUE_ADDRESS (msymbol));
 > +	  /* 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.  */
 > +	  s = find_pc_sect_symtab (SYMBOL_VALUE_ADDRESS (msymbol),
 > +				   SYMBOL_BFD_SECTION (msymbol));
 >  	  if (s != NULL)
 >  	    {
 > +	      /* This is a function which has a symtab for its address.  */
 >  	      bv = BLOCKVECTOR (s);
 >  	      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.  */
 > -	      sym = lookup_block_symbol (block, name, mangled_name, namespace);
 > +
 > +	      /* 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.  */
 > +	      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. */
 >  	      if (!sym)
 >  		{
 >  		  block = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
 >  		  sym = lookup_block_symbol (block, name,
 > -                                             mangled_name, namespace);
 > -		}
 > -	      /* If we found one, return it */
 > -	      if (sym)
 > -		{
 > -		  if (symtab != NULL)
 > -		    *symtab = s;
 > -		  return sym;
 > +					     mangled_name, namespace);
 >  		}
 >  
 > -	      /* If we get here with sym == 0, the symbol was 
 > -	         found in the minimal symbol table
 > +	      /* sym == 0 if 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).
 > +	         Return 0 to use the msymbol definition of "foo_".
 >  
 >  	         This happens for Fortran  "foo_" symbols,
 >  	         which are "foo" in the symtab.
 > @@ -1109,28 +1123,27 @@ lookup_symbol_aux (const char *name, con
 >  	         asm(".globl _main");
 >  	         asm("_main:");
 >  	       */
 > -	    }
 >  
 > -	  /* If the lookup-by-address fails, try repeating the
 > -	   * entire lookup process with the symbol name from
 > -	   * the msymbol (if different from the original symbol name).
 > -	   */
 > +	      if (symtab != NULL)
 > +		*symtab = s;
 > +	      *force_return = 1;
 > +	      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.  */
 > +	      *force_return = 1;
 >  	      return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name,
 > -					block, namespace, is_a_field_of_this,
 > +					NULL, namespace, is_a_field_of_this,
 >  					symtab);
 >  	    }
 >  	}
 >      }
 >  
 > -#endif
 > -
 > -  if (symtab != NULL)
 > -    *symtab = NULL;
 > -  return 0;
 > +  return NULL;
 >  }
 >  								
 >  /* Look, in partial_symtab PST, for symbol NAME.  Check the global

Re: [rfa] clean up lookup_symbol_aux

[Joel Brobecker]

> Maybe Joel can test the hppa patch?

Yes, sure. The thread is a bit longish, so to make sure I don't
misinterpret what needed to be tested, can David resend me the
patch that needs to be tested?

>  > David Carlton
>  > carlton@math.stanford.edu
>  > 
>  > 2002-10-30  David Carlton  <carlton@math.stanford.edu>
>  > 
>  > 	* symtab.c (lookup_symbol_aux): Move chunks of code into separate
>  > 	functions.
>  > 	(lookup_symbol_aux_local): New function.
>  > 	(lookup_symbol_aux_symtabs): New function.
>  > 	(lookup_symbol_aux_psymtabs): New function.
>  > 	(lookup_symbol_aux_minsyms): New function.

-- 
Joel

Re: [rfa] clean up lookup_symbol_aux

[David Carlton]

On Sun, 3 Nov 2002 19:01:23 -0500, Elena Zannoni <ezannoni@redhat.com> said:
> David Carlton writes:
>> On 02 Oct 2002 14:50:34 -0700, David Carlton
>> <carlton@math.Stanford.EDU> said:

>> > * The part of lookup_symbol_aux that never gets run and that is
>> >   prefaced by 17 lines of comments being bemused at its existence
>> >   has been deleted.

> Could it be if deffed 0 for a bit, to see if we get some weird
> behaviors?  We can delete it later. Make sure you add a comment with
> a date.

Sure, I can do that.  Having said that, I don't really think this is
necessary: it's pretty clear that the code isn't being run; it's
prefaced by multiple comments that point out that it isn't being run,
that can't figure out what the code is supposed to do, and that are, I
think, a few years old (the more recent one seems to date from the HP
merge).  So ifdeffing it out won't change the behavior of GDB at all,
it'll just preserve another one of GDB's mysteries for that much
longer.  But ifdeffing it won't hurt anything, either, so I can
certainly do it if you'd be happier.

>> > * The two cases for searching minimal symbols (i.e. whether or not
>> >   you're in HPUXHPPA) turned out to be slightly different.  I went
>> >   through those differences and picked whichever variant seemed to
>> >   me to be better; as far as I can tell, there's no reason for the
>> >   code to differ in the two cases.  I also threw in a bug fix:
>> >   fixup_symbol_section had the wrong second argument in the variant
>> >   where it was present.

> This one I am a bit worried about.  For the bugfix, I would prefer
> that a separate patch is submitted, I cannot spot the fix in the
> patch.

Yeah, I probably should have done that first.  I'll include a patch
just for that after my signature; the point of the patch is that the
minsym code refers to 'block' and 'objfile', but those variables don't
actually point at anything useful in the minsym code.  (They're
pointing at whatever they happened to point at at the end of the
symtab search.)  So the patch replaces 'objfile' by something correct,
and replaces 'block' by NULL.  (There are other possible bugfixes, but
they would all result from situations where the HP and non-HP
currently do different things: those can wait.)

> I would check in the functions except the minsyms one, ifdeffing the
> code that was deleted. Separately submit a patch for the bugfix, and
> one just for the minsyms changes so that it can be tested
> independentely.

Great, I'll do that.  If you can look at the bugfix one first, I'd
appreciate it, because I don't want to move out the
local/symtab/psymtab stuff before having that bugfix patch accepted.
(If the bugfix patch isn't accepted, moving out the other parts of the
code might lead to bizarre consequences.)

David Carlton
carlton@math.stanford.edu

2002-11-04  David Carlton  <carlton@math.stanford.edu>

	* symtab.c (lookup_symbol_aux): In minsym sections, don't use the
	previous values of 'objfile' and 'block'.

Index: symtab.c
===================================================================
RCS file: /cvs/src/src/gdb/symtab.c,v
retrieving revision 1.75
diff -u -p -r1.75 symtab.c
--- symtab.c	28 Oct 2002 17:05:56 -0000	1.75
+++ symtab.c	4 Nov 2002 23:59:05 -0000
@@ -929,7 +929,7 @@ lookup_symbol_aux (const char *name, con
 
 	      if (symtab != NULL)
 		*symtab = s;
-	      return fixup_symbol_section (sym, objfile);
+	      return fixup_symbol_section (sym, s->objfile);
 	    }
 	  else if (MSYMBOL_TYPE (msymbol) != mst_text
 		   && MSYMBOL_TYPE (msymbol) != mst_file_text
@@ -937,7 +937,7 @@ lookup_symbol_aux (const char *name, con
 	    {
 	      /* This is a mangled variable, look it up by its
 	         mangled name.  */
-	      return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name, block,
+	      return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name, NULL,
 					namespace, is_a_field_of_this, symtab);
 	    }
 	  /* There are no debug symbols for this file, or we are looking
@@ -1120,7 +1120,7 @@ lookup_symbol_aux (const char *name, con
 		   && !STREQ (name, SYMBOL_NAME (msymbol)))
 	    {
 	      return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name,
-					block, namespace, is_a_field_of_this,
+					NULL, namespace, is_a_field_of_this,
 					symtab);
 	    }
 	}

Re: [rfa] clean up lookup_symbol_aux

[Elena Zannoni]

David Carlton writes:
 > On Sun, 3 Nov 2002 19:01:23 -0500, Elena Zannoni <ezannoni@redhat.com> said:
 > > David Carlton writes:
 > >> On 02 Oct 2002 14:50:34 -0700, David Carlton
 > >> <carlton@math.Stanford.EDU> said:
 > 
 > >> > * The part of lookup_symbol_aux that never gets run and that is
 > >> >   prefaced by 17 lines of comments being bemused at its existence
 > >> >   has been deleted.
 > 
 > > Could it be if deffed 0 for a bit, to see if we get some weird
 > > behaviors?  We can delete it later. Make sure you add a comment with
 > > a date.
 > 
 > Sure, I can do that.  Having said that, I don't really think this is
 > necessary: it's pretty clear that the code isn't being run; it's
 > prefaced by multiple comments that point out that it isn't being run,
 > that can't figure out what the code is supposed to do, and that are, I
 > think, a few years old (the more recent one seems to date from the HP
 > merge).  So ifdeffing it out won't change the behavior of GDB at all,
 > it'll just preserve another one of GDB's mysteries for that much
 > longer.  But ifdeffing it won't hurt anything, either, so I can
 > certainly do it if you'd be happier.
 > 

Yes, please. It wouldn't be the first time that weird things happen
because of apparently insignificant changes.  (see the old proverbial
wait_for_inferior).

 > >> > * The two cases for searching minimal symbols (i.e. whether or not
 > >> >   you're in HPUXHPPA) turned out to be slightly different.  I went
 > >> >   through those differences and picked whichever variant seemed to
 > >> >   me to be better; as far as I can tell, there's no reason for the
 > >> >   code to differ in the two cases.  I also threw in a bug fix:
 > >> >   fixup_symbol_section had the wrong second argument in the variant
 > >> >   where it was present.
 > 
 > > This one I am a bit worried about.  For the bugfix, I would prefer
 > > that a separate patch is submitted, I cannot spot the fix in the
 > > patch.
 > 
 > Yeah, I probably should have done that first.  I'll include a patch
 > just for that after my signature; the point of the patch is that the
 > minsym code refers to 'block' and 'objfile', but those variables don't
 > actually point at anything useful in the minsym code.  (They're
 > pointing at whatever they happened to point at at the end of the
 > symtab search.)  So the patch replaces 'objfile' by something correct,

Right. s->objfile will have 's' always non-null.

 > and replaces 'block' by NULL.  (There are other possible bugfixes, but
 > they would all result from situations where the HP and non-HP
 > currently do different things: those can wait.)
 > 

Ah yes, these two would be in the 'else' which is taken if s == NULL
and block is not going to be assigned to.

 > > I would check in the functions except the minsyms one, ifdeffing the
 > > code that was deleted. Separately submit a patch for the bugfix, and
 > > one just for the minsyms changes so that it can be tested
 > > independentely.
 > 
 > Great, I'll do that.  If you can look at the bugfix one first, I'd
 > appreciate it, because I don't want to move out the
 > local/symtab/psymtab stuff before having that bugfix patch accepted.
 > (If the bugfix patch isn't accepted, moving out the other parts of the
 > code might lead to bizarre consequences.)
 > 

Ok approved. Good catch.

Elena

 > David Carlton
 > carlton@math.stanford.edu
 > 
 > 2002-11-04  David Carlton  <carlton@math.stanford.edu>
 > 
 > 	* symtab.c (lookup_symbol_aux): In minsym sections, don't use the
 > 	previous values of 'objfile' and 'block'.
 > 
 > Index: symtab.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/symtab.c,v
 > retrieving revision 1.75
 > diff -u -p -r1.75 symtab.c
 > --- symtab.c	28 Oct 2002 17:05:56 -0000	1.75
 > +++ symtab.c	4 Nov 2002 23:59:05 -0000
 > @@ -929,7 +929,7 @@ lookup_symbol_aux (const char *name, con
 >  
 >  	      if (symtab != NULL)
 >  		*symtab = s;
 > -	      return fixup_symbol_section (sym, objfile);
 > +	      return fixup_symbol_section (sym, s->objfile);
 >  	    }
 >  	  else if (MSYMBOL_TYPE (msymbol) != mst_text
 >  		   && MSYMBOL_TYPE (msymbol) != mst_file_text
 > @@ -937,7 +937,7 @@ lookup_symbol_aux (const char *name, con
 >  	    {
 >  	      /* This is a mangled variable, look it up by its
 >  	         mangled name.  */
 > -	      return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name, block,
 > +	      return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name, NULL,
 >  					namespace, is_a_field_of_this, symtab);
 >  	    }
 >  	  /* There are no debug symbols for this file, or we are looking
 > @@ -1120,7 +1120,7 @@ lookup_symbol_aux (const char *name, con
 >  		   && !STREQ (name, SYMBOL_NAME (msymbol)))
 >  	    {
 >  	      return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name,
 > -					block, namespace, is_a_field_of_this,
 > +					NULL, namespace, is_a_field_of_this,
 >  					symtab);
 >  	    }
 >  	}

Re: [rfa] clean up lookup_symbol_aux

[David Carlton]

On Mon, 4 Nov 2002 20:21:32 -0500, Elena Zannoni <ezannoni@redhat.com> said:
> David Carlton writes:

>> So ifdeffing it out won't change the behavior of GDB at all, it'll
>> just preserve another one of GDB's mysteries for that much longer.
>> But ifdeffing it won't hurt anything, either, so I can certainly do
>> it if you'd be happier.

> Yes, please. It wouldn't be the first time that weird things happen
> because of apparently insignificant changes.

Fair enough.

> (see the old proverbial wait_for_inferior).

?

>> If you can look at the bugfix one first, I'd appreciate it, because
>> I don't want to move out the local/symtab/psymtab stuff before
>> having that bugfix patch accepted.  (If the bugfix patch isn't
>> accepted, moving out the other parts of the code might lead to
>> bizarre consequences.)

> Ok approved. Good catch.

Thanks.  Then I'll apply those two tomorrow, and submit a new RFA for
a patch which does the minsym stuff, together with an analysis of the
differences between the two versions of the minsym code.  Joel has
been kind enough to run it on his machine (but I haven't yet looked at
the results yet!), so we'll see whether or not I got it right on HPUX.

David Carlton
carlton@math.stanford.edu

Re: [rfa] clean up lookup_symbol_aux

[David Carlton]

On 04 Nov 2002 20:39:57 -0800, David Carlton <carlton@math.Stanford.EDU> said:

> Then I'll apply those two tomorrow

I've applied those two; the patch for the non-minsym-bugfixes is below.

David Carlton
carlton@math.stanford.edu

2002-11-05  David Carlton  <carlton@math.stanford.edu>

	* symtab.c (lookup_symbol_aux): Move chunks of code into separate
	functions.
	(lookup_symbol_aux_local): New function.
	(lookup_symbol_aux_symtabs): New function.
	(lookup_symbol_aux_psymtabs): New function.

Index: symtab.c
===================================================================
RCS file: /cvs/src/src/gdb/symtab.c,v
retrieving revision 1.76
diff -u -p -r1.76 symtab.c
--- symtab.c	5 Nov 2002 16:59:57 -0000	1.76
+++ symtab.c	5 Nov 2002 17:13:02 -0000
@@ -83,6 +83,25 @@ static struct symbol *lookup_symbol_aux 
 					 int *is_a_field_of_this,
 					 struct symtab **symtab);
 
+static struct symbol *lookup_symbol_aux_local (const char *name,
+					       const char *mangled_name,
+					       const struct block *block,
+					       const namespace_enum namespace,
+					       struct symtab **symtab);
+
+static
+struct symbol *lookup_symbol_aux_symtabs (int block_index,
+					  const char *name,
+					  const char *mangled_name,
+					  const namespace_enum namespace,
+					  struct symtab **symtab);
+
+static
+struct symbol *lookup_symbol_aux_psymtabs (int block_index,
+					   const char *name,
+					   const char *mangled_name,
+					   const namespace_enum namespace,
+					   struct symtab **symtab);
 
 static struct symbol *find_active_alias (struct symbol *sym, CORE_ADDR addr);
 
@@ -766,43 +785,22 @@ lookup_symbol_aux (const char *name, con
 		   const struct block *block, const namespace_enum namespace,
 		   int *is_a_field_of_this, struct symtab **symtab)
 {
-  register struct symbol *sym;
-  register struct symtab *s = NULL;
-  register struct partial_symtab *ps;
-  register struct blockvector *bv;
-  register struct objfile *objfile = NULL;
-  register struct block *b;
-  register struct minimal_symbol *msymbol;
-
+  struct symbol *sym;
+  struct symtab *s = NULL;
+  struct blockvector *bv;
+  struct minimal_symbol *msymbol;
 
   /* Search specified block and its superiors.  */
 
-  while (block != 0)
-    {
-      sym = lookup_block_symbol (block, name, mangled_name, namespace);
-      if (sym)
-	{
-	  block_found = block;
-	  if (symtab != NULL)
-	    {
-	      /* Search the list of symtabs for one which contains the
-	         address of the start of this block.  */
-	      ALL_SYMTABS (objfile, s)
-	      {
-		bv = BLOCKVECTOR (s);
-		b = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
-		if (BLOCK_START (b) <= BLOCK_START (block)
-		    && BLOCK_END (b) > BLOCK_START (block))
-		  goto found;
-	      }
-	    found:
-	      *symtab = s;
-	    }
+  sym = lookup_symbol_aux_local (name, mangled_name, block, namespace,
+				 symtab);
+  if (sym != NULL)
+    return sym;
 
-	  return fixup_symbol_section (sym, objfile);
-	}
-      block = BLOCK_SUPERBLOCK (block);
-    }
+#if 0
+  /* NOTE: carlton/2002-11-05: At the time that this code was
+     #ifdeffed out, the value of 'block' was always NULL at this
+     point, hence the bemused comments below.  */
 
   /* FIXME: this code is never executed--block is always NULL at this
      point.  What is it trying to do, anyway?  We already should have
@@ -843,7 +841,7 @@ lookup_symbol_aux (const char *name, con
 	  }
       }
     }
-
+#endif /* 0 */
 
   /* C++: If requested to do so by the caller, 
      check to see if NAME is a field of `this'. */
@@ -866,19 +864,10 @@ lookup_symbol_aux (const char *name, con
      of the desired name as a global, then do psymtab-to-symtab
      conversion on the fly and return the found symbol. */
 
-  ALL_SYMTABS (objfile, s)
-  {
-    bv = BLOCKVECTOR (s);
-    block = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
-    sym = lookup_block_symbol (block, name, mangled_name, namespace);
-    if (sym)
-      {
-	block_found = block;
-	if (symtab != NULL)
-	  *symtab = s;
-	return fixup_symbol_section (sym, objfile);
-      }
-  }
+  sym = lookup_symbol_aux_symtabs (GLOBAL_BLOCK, name, mangled_name,
+				   namespace, symtab);
+  if (sym != NULL)
+    return sym;
 
 #ifndef HPUXHPPA
 
@@ -948,84 +937,26 @@ lookup_symbol_aux (const char *name, con
 
 #endif
 
-  ALL_PSYMTABS (objfile, ps)
-  {
-    if (!ps->readin && lookup_partial_symbol (ps, name, 1, namespace))
-      {
-	s = PSYMTAB_TO_SYMTAB (ps);
-	bv = BLOCKVECTOR (s);
-	block = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
-	sym = lookup_block_symbol (block, name, mangled_name, namespace);
-	if (!sym)
-	  {
-	    /* This shouldn't be necessary, but as a last resort
-	     * try looking in the statics even though the psymtab
-	     * claimed the symbol was global. It's possible that
-	     * the psymtab gets it wrong in some cases.
-	     */
-	    block = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
-	    sym = lookup_block_symbol (block, name, mangled_name, namespace);
-	    if (!sym)
-	      error ("Internal: global symbol `%s' found in %s psymtab but not in symtab.\n\
-%s may be an inlined function, or may be a template function\n\
-(if a template, try specifying an instantiation: %s<type>).",
-		     name, ps->filename, name, name);
-	  }
-	if (symtab != NULL)
-	  *symtab = s;
-	return fixup_symbol_section (sym, objfile);
-      }
-  }
+  sym = lookup_symbol_aux_psymtabs (GLOBAL_BLOCK, name, mangled_name,
+				    namespace, symtab);
+  if (sym != NULL)
+    return sym;
 
-  /* Now search all static file-level symbols.
-     Not strictly correct, but more useful than an error.
-     Do the symtabs first, then check the psymtabs.
-     If a psymtab indicates the existence
-     of the desired name as a file-level static, then do psymtab-to-symtab
+  /* Now search all static file-level symbols.  Not strictly correct,
+     but more useful than an error.  Do the symtabs first, then check
+     the psymtabs.  If a psymtab indicates the existence of the
+     desired name as a file-level static, then do psymtab-to-symtab
      conversion on the fly and return the found symbol. */
 
-  ALL_SYMTABS (objfile, s)
-  {
-    bv = BLOCKVECTOR (s);
-    block = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
-    sym = lookup_block_symbol (block, name, mangled_name, namespace);
-    if (sym)
-      {
-	block_found = block;
-	if (symtab != NULL)
-	  *symtab = s;
-	return fixup_symbol_section (sym, objfile);
-      }
-  }
-
-  ALL_PSYMTABS (objfile, ps)
-  {
-    if (!ps->readin && lookup_partial_symbol (ps, name, 0, namespace))
-      {
-	s = PSYMTAB_TO_SYMTAB (ps);
-	bv = BLOCKVECTOR (s);
-	block = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
-	sym = lookup_block_symbol (block, name, mangled_name, namespace);
-	if (!sym)
-	  {
-	    /* This shouldn't be necessary, but as a last resort
-	     * try looking in the globals even though the psymtab
-	     * claimed the symbol was static. It's possible that
-	     * the psymtab gets it wrong in some cases.
-	     */
-	    block = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
-	    sym = lookup_block_symbol (block, name, mangled_name, namespace);
-	    if (!sym)
-	      error ("Internal: static symbol `%s' found in %s psymtab but not in symtab.\n\
-%s may be an inlined function, or may be a template function\n\
-(if a template, try specifying an instantiation: %s<type>).",
-		     name, ps->filename, name, name);
-	  }
-	if (symtab != NULL)
-	  *symtab = s;
-	return fixup_symbol_section (sym, objfile);
-      }
-  }
+  sym = lookup_symbol_aux_symtabs (STATIC_BLOCK, name, mangled_name,
+				   namespace, symtab);
+  if (sym != NULL)
+    return sym;
+  
+  sym = lookup_symbol_aux_psymtabs (STATIC_BLOCK, name, mangled_name,
+				    namespace, symtab);
+  if (sym != NULL)
+    return sym;
 
 #ifdef HPUXHPPA
 
@@ -1130,9 +1061,147 @@ lookup_symbol_aux (const char *name, con
 
   if (symtab != NULL)
     *symtab = NULL;
-  return 0;
+  return NULL;
 }
-								
+
+/* Check to see if the symbol is defined in BLOCK or its
+   superiors.  */
+
+static struct symbol *
+lookup_symbol_aux_local (const char *name, const char *mangled_name,
+			 const struct block *block,
+			 const namespace_enum namespace,
+			 struct symtab **symtab)
+{
+  struct symbol *sym;
+  struct objfile *objfile = NULL;
+  struct blockvector *bv;
+  struct block *b;
+  struct symtab *s = NULL;
+  
+  while (block != 0)
+    {
+      sym = lookup_block_symbol (block, name, mangled_name, namespace);
+      if (sym)
+	{
+	  block_found = block;
+	  if (symtab != NULL)
+	    {
+	      /* Search the list of symtabs for one which contains the
+	         address of the start of this block.  */
+	      ALL_SYMTABS (objfile, s)
+	      {
+		bv = BLOCKVECTOR (s);
+		b = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
+		if (BLOCK_START (b) <= BLOCK_START (block)
+		    && BLOCK_END (b) > BLOCK_START (block))
+		  goto found;
+	      }
+	    found:
+	      *symtab = s;
+	    }
+
+	  return fixup_symbol_section (sym, objfile);
+	}
+      block = BLOCK_SUPERBLOCK (block);
+    }
+
+  return NULL;
+}
+
+/* Check to see if the symbol is defined in one of the symtabs.
+   BLOCK_INDEX should be either GLOBAL_BLOCK or STATIC_BLOCK,
+   depending on whether or not we want to search global symbols or
+   static symbols.  */
+
+static struct symbol *
+lookup_symbol_aux_symtabs (int block_index,
+			   const char *name, const char *mangled_name,
+			   const namespace_enum namespace,
+			   struct symtab **symtab)
+{
+  struct symbol *sym;
+  struct objfile *objfile;
+  struct blockvector *bv;
+  const struct block *block;
+  struct symtab *s;
+
+  ALL_SYMTABS (objfile, s)
+  {
+    bv = BLOCKVECTOR (s);
+    block = BLOCKVECTOR_BLOCK (bv, block_index);
+    sym = lookup_block_symbol (block, name, mangled_name, namespace);
+    if (sym)
+      {
+	block_found = block;
+	if (symtab != NULL)
+	  *symtab = s;
+	return fixup_symbol_section (sym, objfile);
+      }
+  }
+
+  return NULL;
+}
+
+/* Check to see if the symbol is defined in one of the partial
+   symtabs.  BLOCK_INDEX should be either GLOBAL_BLOCK or
+   STATIC_BLOCK, depending on whether or not we want to search global
+   symbols or static symbols.  */
+
+static struct symbol *
+lookup_symbol_aux_psymtabs (int block_index, const char *name,
+			    const char *mangled_name,
+			    const namespace_enum namespace,
+			    struct symtab **symtab)
+{
+  struct symbol *sym;
+  struct objfile *objfile;
+  struct blockvector *bv;
+  const struct block *block;
+  struct partial_symtab *ps;
+  struct symtab *s;
+  const int psymtab_index = (block_index == GLOBAL_BLOCK ? 1 : 0);
+
+  ALL_PSYMTABS (objfile, ps)
+  {
+    if (!ps->readin
+	&& lookup_partial_symbol (ps, name, psymtab_index, namespace))
+      {
+	s = PSYMTAB_TO_SYMTAB (ps);
+	bv = BLOCKVECTOR (s);
+	block = BLOCKVECTOR_BLOCK (bv, block_index);
+	sym = lookup_block_symbol (block, name, mangled_name, namespace);
+	if (!sym)
+	  {
+	    /* This shouldn't be necessary, but as a last resort try
+	       looking in the statics even though the psymtab claimed
+	       the symbol was global, or vice-versa. It's possible
+	       that the psymtab gets it wrong in some cases.  */
+
+	    /* FIXME: carlton/2002-09-30: Should we really do that?
+	       If that happens, isn't it likely to be a GDB error, in
+	       which case we should fix the GDB error rather than
+	       silently dealing with it here?  So I'd vote for
+	       removing the check for the symbol in the other
+	       block.  */
+	    block = BLOCKVECTOR_BLOCK (bv,
+				       block_index == GLOBAL_BLOCK ?
+				       STATIC_BLOCK : GLOBAL_BLOCK);
+	    sym = lookup_block_symbol (block, name, mangled_name, namespace);
+	    if (!sym)
+	      error ("Internal: %s symbol `%s' found in %s psymtab but not in symtab.\n%s may be an inlined function, or may be a template function\n(if a template, try specifying an instantiation: %s<type>).",
+		     block_index == GLOBAL_BLOCK ? "global" : "static",
+		     name, ps->filename, name, name);
+	  }
+	if (symtab != NULL)
+	  *symtab = s;
+	return fixup_symbol_section (sym, objfile);
+      }
+  }
+
+  return NULL;
+}
+
 /* Look, in partial_symtab PST, for symbol NAME.  Check the global
    symbols if GLOBAL, the static symbols if not */