[rfa] delete 'force_return' from lookup_symbol_aux_minsyms

[rfa] delete 'force_return' from lookup_symbol_aux_minsyms

[David Carlton]

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

I don't think that behavior was either intentional or desirable.
There's not much rhyme or reason to when this happens: in particular,
it doesn't happen every time lookup_symbol_aux_minsyms finds a minsym.
Instead, it is necessary, in addition, either for there to be a symtab at
the appropriate address or for the symtab to be NULL but for the
following test to hold:

          else if (MSYMBOL_TYPE (msymbol) != mst_text
                   && MSYMBOL_TYPE (msymbol) != mst_file_text
                   && !STREQ (name, SYMBOL_NAME (msymbol)))

(I actually experimented with trying to have lookup_symbol _always_
return NULL if a minsym was found without a corresponding symbol, and
that breaks GDB.)  I can't imagine that there are callers of
lookup_symbol that depend on having it return NULL in these particular
circumstances; it's certainly not documented anywhere.  And I suspect
that this issue is what caused the #ifdef HPUXHPPA to be added to
lookup_symbol_aux: as a relevant comment says,

  For HP-generated symbol tables, this check was causing a premature
  exit from lookup_symbol with NULL return, and thus messing up symbol
  lookups of things like "c::f".

When I asked about this issue earlier, Jim Blandy came to more or less
the same conclusion, I think: see
<http://sources.redhat.com/ml/gdb/2002-11/msg00045.html>.

David Carlton
carlton@math.stanford.edu

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

	* symtab.c (lookup_symbol_aux): Delete 'force_return' variable.
	(lookup_symbol_aux_minsyms): Delete 'force_return' argument.

Index: symtab.c
===================================================================
RCS file: /cvs/src/src/gdb/symtab.c,v
retrieving revision 1.81
diff -u -p -r1.81 symtab.c
--- symtab.c	5 Dec 2002 21:26:57 -0000	1.81
+++ symtab.c	5 Dec 2002 22:23:55 -0000
@@ -117,8 +117,7 @@ struct symbol *lookup_symbol_aux_minsyms
 					  const char *mangled_name,
 					  const namespace_enum namespace,
 					  int *is_a_field_of_this,
-					  struct symtab **symtab,
-					  int *force_return);
+					  struct symtab **symtab);
 
 static struct symbol *find_active_alias (struct symbol *sym, CORE_ADDR addr);
 
@@ -805,14 +804,6 @@ lookup_symbol_aux (const char *name, con
   struct symbol *sym;
   const struct block *static_block;
 
-  /* FIXME: carlton/2002-11-05: This variable is here so that
-     lookup_symbol_aux will sometimes return NULL after receiving a
-     NULL return value from lookup_symbol_aux_minsyms, without
-     proceeding on to the partial symtab and static variable tests.  I
-     suspect that that's a bad idea.  */
-  
-  int force_return;
-
   /* Search specified block and its superiors.  Don't search
      STATIC_BLOCK or GLOBAL_BLOCK.  */
 
@@ -931,13 +922,11 @@ lookup_symbol_aux (const char *name, con
      a mangled variable that is stored in one of the minimal symbol tables.
      Eventually, all global symbols might be resolved in this way.  */
 
-  force_return = 0;
-
   sym = lookup_symbol_aux_minsyms (name, mangled_name,
 				   namespace, is_a_field_of_this,
-				   symtab, &force_return);
+				   symtab);
   
-  if (sym != NULL || force_return == 1)
+  if (sym != NULL)
     return sym;
 
 #endif
@@ -981,13 +970,11 @@ lookup_symbol_aux (const char *name, con
    */
 
 
-  force_return = 0;
-
   sym = lookup_symbol_aux_minsyms (name, mangled_name,
 				   namespace, is_a_field_of_this,
-				   symtab, &force_return);
+				   symtab);
   
-  if (sym != NULL || force_return == 1)
+  if (sym != NULL)
     return sym;
 
 #endif
@@ -1172,13 +1159,20 @@ lookup_symbol_aux_psymtabs (int block_in
    tables.  Eventually, all global symbols might be resolved in this
    way.  */
 
+/* NOTE: carlton/2002-12-05: At one point, this function was part of
+   lookup_symbol_aux, and what are now 'return' statements within
+   lookup_symbol_aux_minsyms returned from lookup_symbol_aux, even if
+   sym was NULL.  As far as I can tell, this was basically accidental;
+   it didn't happen every time that msymbol was non-NULL, but only if
+   some additional conditions held as well, and it caused problems
+   with HP-generated symbol tables.  */
+
 static struct symbol *
 lookup_symbol_aux_minsyms (const char *name,
 			   const char *mangled_name,
 			   const namespace_enum namespace,
 			   int *is_a_field_of_this,
-			   struct symtab **symtab,
-			   int *force_return)
+			   struct symtab **symtab)
 {
   struct symbol *sym;
   struct blockvector *bv;
@@ -1271,7 +1265,6 @@ lookup_symbol_aux_minsyms (const char *n
 
 	      if (symtab != NULL && sym != NULL)
 		*symtab = s;
-	      *force_return = 1;
 	      return fixup_symbol_section (sym, s->objfile);
 	    }
 	  else if (MSYMBOL_TYPE (msymbol) != mst_text
@@ -1280,7 +1273,6 @@ lookup_symbol_aux_minsyms (const char *n
 	    {
 	      /* This is a mangled variable, look it up by its
 	         mangled name.  */
-	      *force_return = 1;
 	      return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name,
 					NULL, namespace, is_a_field_of_this,
 					symtab);

Re: [rfa] delete 'force_return' from lookup_symbol_aux_minsyms

[Elena Zannoni]

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


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

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

I see that this bit:

+         else if (MSYMBOL_TYPE (msymbol) != mst_text
+                  && MSYMBOL_TYPE (msymbol) != mst_file_text
+                  && !STREQ (name, SYMBOL_NAME (msymbol)))
+           {
+             /* This is a mangled variable, look it up by its
+                mangled name.  */
+             return lookup_symbol (SYMBOL_NAME (msymbol), block,
+                                   namespace, is_a_field_of_this, symtab);
+           }
+         /* There are no debug symbols for this file, or we are looking
+            for an unmangled variable.
+            Try to find a matching static symbol below. */

and this bit:

@@ -2629,13 +2684,20 @@ list_symbols (regexp, class, bpt, from_t
        }
     }
 
-  /* Here, we search through the minimal symbol tables for functions that
-     match, and call find_pc_symtab on them to force their symbols to
-     be read.  The symbol will then be found during the scan of symtabs
-     below.  If find_pc_symtab fails, set found_misc so that we will
-     rescan to print any matching symbols without debug info.  */
+  /* Here, we search through the minimal symbol tables for functions
+     and variables that match, and force their symbols to be read.
+     This is in particular necessary for demangled variable names,
+     which are no longer put into the partial symbol tables.
+     The symbol will then be found during the scan of symtabs below.
+
+     For functions, find_pc_symtab should succeed if we have debug info
+     for the function, for variables we have to call lookup_symbol
+     to determine if the variable has debug info.
+     If the lookup fails, set found_misc so that we will rescan to print
+     any matching symbols without debug info.
+  */
 
-  if (class == 1)
+  if (class == 0 || class == 1)
     {
       ALL_MSYMBOLS (objfile, msymbol)
        {
@@ -2648,7 +2710,12 @@ list_symbols (regexp, class, bpt, from_t
                {
                  if (0 == find_pc_symtab (SYMBOL_VALUE_ADDRESS (msymbol)))
                    {
-                     found_misc = 1;
+                     if (class == 1
+                         || lookup_symbol (SYMBOL_NAME (msymbol), 
+                                           (struct block *) NULL,
+                                           VAR_NAMESPACE,
+                                           0, (struct symtab **) NULL) == NULL)
+                       found_misc = 1;
                    }
                }
            }

were added together in 94. the changelog was:

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

Don't know if it helps.

Elena


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

Re: [rfa] delete 'force_return' from lookup_symbol_aux_minsyms

[David Carlton]

On Mon, 9 Dec 2002 16:10:46 -0500, Elena Zannoni <ezannoni@redhat.com> said:

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

Interesting.  How did you track that down?  I couldn't get CVS to go
back that far when I just tried it.

> I see that this bit:

> +         else if (MSYMBOL_TYPE (msymbol) != mst_text
> +                  && MSYMBOL_TYPE (msymbol) != mst_file_text
> +                  && !STREQ (name, SYMBOL_NAME (msymbol)))
> +           {
> +             /* This is a mangled variable, look it up by its
> +                mangled name.  */
> +             return lookup_symbol (SYMBOL_NAME (msymbol), block,
> +                                   namespace, is_a_field_of_this, symtab);
> +           }
> +         /* There are no debug symbols for this file, or we are looking
> +            for an unmangled variable.
> +            Try to find a matching static symbol below. */

> and this bit:

> @@ -2629,13 +2684,20 @@ list_symbols (regexp, class, bpt, from_t
>         }
>      }
 
> -  /* Here, we search through the minimal symbol tables for functions that
> -     match, and call find_pc_symtab on them to force their symbols to
> -     be read.  The symbol will then be found during the scan of symtabs
> -     below.  If find_pc_symtab fails, set found_misc so that we will
> -     rescan to print any matching symbols without debug info.  */
> +  /* Here, we search through the minimal symbol tables for functions
> +     and variables that match, and force their symbols to be read.
> +     This is in particular necessary for demangled variable names,
> +     which are no longer put into the partial symbol tables.
> +     The symbol will then be found during the scan of symtabs below.
> +
> +     For functions, find_pc_symtab should succeed if we have debug info
> +     for the function, for variables we have to call lookup_symbol
> +     to determine if the variable has debug info.
> +     If the lookup fails, set found_misc so that we will rescan to print
> +     any matching symbols without debug info.
> +  */
 
> -  if (class == 1)
> +  if (class == 0 || class == 1)
>      {
>        ALL_MSYMBOLS (objfile, msymbol)
>         {
> @@ -2648,7 +2710,12 @@ list_symbols (regexp, class, bpt, from_t
>                 {
>                   if (0 == find_pc_symtab (SYMBOL_VALUE_ADDRESS (msymbol)))
>                     {
> -                     found_misc = 1;
> +                     if (class == 1
> +                         || lookup_symbol (SYMBOL_NAME (msymbol), 
> +                                           (struct block *) NULL,
> +                                           VAR_NAMESPACE,
> +                                           0, (struct symtab **) NULL) == NULL)
> +                       found_misc = 1;
>                     }
>                 }
>             }

> were added together in 94. the changelog was:

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

Hmm.  What's 'class'?  Does that refer to a variable/function
distinction, or a mangled/demangled distinction?  'found_misc' also
seems to have gone away; maybe that was a hint that callers should
follow up a failed call to lookup_symbol by one to
lookup_minimal_symbol, whereas we now always expect callers to do so
if appropriate, even without the hint.  At any rate, I'm particularly
interested by this part of the comment:

> +     For functions, find_pc_symtab should succeed if we have debug info
> +     for the function, for variables we have to call lookup_symbol
> +     to determine if the variable has debug info.
> +     If the lookup fails, set found_misc so that we will rescan to print
> +     any matching symbols without debug info.

So, at some point, this code had explicit assumptions about when there
should be a symbol corresponding to a minimal symbol, and set
found_misc appropriately.  But then, at some point, 'found_misc' went
away; it left, as a legacy, a somewhat strange flow of control that
wasn't at all explicit until I had to introduce 'force_return' to
mimic it.  

Also, the line

> +     The symbol will then be found during the scan of symtabs below.

makes me wonder if, at this time, the minimal symbol search happened
before the symtab search; I'm not sure how relevant that is.

At any rate, this was certainly interesting in terms of giving an idea
as to how the current flow of control might have arisen.  I still
think my patch is okay, though.

David Carlton
carlton@math.stanford.edu

Re: [rfa] delete 'force_return' from lookup_symbol_aux_minsyms

[Daniel Jacobowitz]

On Tue, Dec 10, 2002 at 10:16:08AM -0800, David Carlton wrote:
> On Mon, 9 Dec 2002 16:10:46 -0500, Elena Zannoni <ezannoni@redhat.com> said:
> 
> > I am tempted to accept it, but could you first look at the
> > archeological diggings below?
> 
> Interesting.  How did you track that down?  I couldn't get CVS to go
> back that far when I just tried it.

They cheat :)  Red Hat's internal repository (formerly Cygnus's) goes
back considerably farther than the one on sources.redhat.com.


-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [rfa] delete 'force_return' from lookup_symbol_aux_minsyms

[Elena Zannoni]

Daniel Jacobowitz writes:
 > On Tue, Dec 10, 2002 at 10:16:08AM -0800, David Carlton wrote:
 > > On Mon, 9 Dec 2002 16:10:46 -0500, Elena Zannoni <ezannoni@redhat.com> said:
 > > 
 > > > I am tempted to accept it, but could you first look at the
 > > > archeological diggings below?
 > > 
 > > Interesting.  How did you track that down?  I couldn't get CVS to go
 > > back that far when I just tried it.
 > 
 > They cheat :)  Red Hat's internal repository (formerly Cygnus's) goes
 > back considerably farther than the one on sources.redhat.com.
 > 

Hey! That's the advantage! :-)
Yes, we go back to 1991.

Elena


 > 
 > -- 
 > Daniel Jacobowitz
 > MontaVista Software                         Debian GNU/Linux Developer

Re: [rfa] delete 'force_return' from lookup_symbol_aux_minsyms

[Elena Zannoni]

David Carlton writes:
 > On Mon, 9 Dec 2002 16:10:46 -0500, Elena Zannoni <ezannoni@redhat.com> said:
 > 
 > > I am tempted to accept it, but could you first look at the
 > > archeological diggings below?
 > 
 > Interesting.  How did you track that down?  I couldn't get CVS to go
 > back that far when I just tried it.
 > 
 > > I see that this bit:
 > 
 > > +         else if (MSYMBOL_TYPE (msymbol) != mst_text
 > > +                  && MSYMBOL_TYPE (msymbol) != mst_file_text
 > > +                  && !STREQ (name, SYMBOL_NAME (msymbol)))
 > > +           {
 > > +             /* This is a mangled variable, look it up by its
 > > +                mangled name.  */
 > > +             return lookup_symbol (SYMBOL_NAME (msymbol), block,
 > > +                                   namespace, is_a_field_of_this, symtab);
 > > +           }
 > > +         /* There are no debug symbols for this file, or we are looking
 > > +            for an unmangled variable.
 > > +            Try to find a matching static symbol below. */
 > 
 > > and this bit:
 > 
 > > @@ -2629,13 +2684,20 @@ list_symbols (regexp, class, bpt, from_t
 > >         }
 > >      }
 >  
 > > -  /* Here, we search through the minimal symbol tables for functions that
 > > -     match, and call find_pc_symtab on them to force their symbols to
 > > -     be read.  The symbol will then be found during the scan of symtabs
 > > -     below.  If find_pc_symtab fails, set found_misc so that we will
 > > -     rescan to print any matching symbols without debug info.  */
 > > +  /* Here, we search through the minimal symbol tables for functions
 > > +     and variables that match, and force their symbols to be read.
 > > +     This is in particular necessary for demangled variable names,
 > > +     which are no longer put into the partial symbol tables.
 > > +     The symbol will then be found during the scan of symtabs below.
 > > +
 > > +     For functions, find_pc_symtab should succeed if we have debug info
 > > +     for the function, for variables we have to call lookup_symbol
 > > +     to determine if the variable has debug info.
 > > +     If the lookup fails, set found_misc so that we will rescan to print
 > > +     any matching symbols without debug info.
 > > +  */
 >  
 > > -  if (class == 1)
 > > +  if (class == 0 || class == 1)
 > >      {
 > >        ALL_MSYMBOLS (objfile, msymbol)
 > >         {
 > > @@ -2648,7 +2710,12 @@ list_symbols (regexp, class, bpt, from_t
 > >                 {
 > >                   if (0 == find_pc_symtab (SYMBOL_VALUE_ADDRESS (msymbol)))
 > >                     {
 > > -                     found_misc = 1;
 > > +                     if (class == 1
 > > +                         || lookup_symbol (SYMBOL_NAME (msymbol), 
 > > +                                           (struct block *) NULL,
 > > +                                           VAR_NAMESPACE,
 > > +                                           0, (struct symtab **) NULL) == NULL)
 > > +                       found_misc = 1;
 > >                     }
 > >                 }
 > >             }
 > 
 > > were added together in 94. the changelog was:
 > 
 > > date: 1994/10/08 11:54:20;  author: schauer;  state: Exp;  lines: +87 -20
 > >         Speed up GDB startup time by not demangling partial symbols.
 > >         * symfile.h (ADD_PSYMBOL_VT_TO_LIST),
 > >         symfile.c (add_psymbol_to_list, add_psymbol_addr_to_list):
 > >         No longer demangle partial symbols.
 > >         * symtab.c (lookup_symbol, list_symbols): Handle mangled
 > >         variables, e.g. C++ static members, via the minimal symbols.
 > 
 > Hmm.  What's 'class'?  Does that refer to a variable/function
 > distinction, or a mangled/demangled distinction?


Class was a parameter for the function list_symbols which now is
called search_symbols, it has been renamed to 'kind' and cleaned up:

If CLASS is zero, list all symbols except functions, type names, and
constants (enums).
If CLASS is 1, list only functions.
If CLASS is 2, list only type names.
If CLASS is 3, list only method names.


 >  'found_misc' also seems to have gone away; 

Actually it is still there, the code I posted is for list/search_symbols.


 > maybe that was a hint that callers should
 > follow up a failed call to lookup_symbol by one to
 > lookup_minimal_symbol, whereas we now always expect callers to do so
 > if appropriate, even without the hint.  At any rate, I'm particularly
 > interested by this part of the comment:

I forgot to post the bit of code that used found_misc: This is the end
of that function, and it is still pretty much the same today.

if (found_misc || class != 1)
 {
   found_in_file = 0;
   ALL_MSYMBOLS (objfile, msymbol)
    {
     if (MSYMBOL_TYPE (msymbol) == ourtype ||
         MSYMBOL_TYPE (msymbol) == ourtype2 ||
         MSYMBOL_TYPE (msymbol) == ourtype3 ||
         MSYMBOL_TYPE (msymbol) == ourtype4)
      {
        if (regexp == NULL || SYMBOL_MATCHES_REGEXP (msymbol))
          {
            /* Functions:  Look up by address. */
            if (class != 1 ||
                (0 == find_pc_symtab (SYMBOL_VALUE_ADDRESS (msymbol))))
              {
                /* Variables/Absolutes:  Look up by name */
                if (lookup_symbol (SYMBOL_NAME (msymbol), 
                                  (struct block *) NULL, VAR_NAMESPACE,
                                  0, (struct symtab **) NULL) == NULL)
                  {
                    if (!found_in_file)
                      {
                       printf_filtered ("\nNon-debugging symbols:\n");
                       found_in_file = 1;
                      }
                    printf_filtered ("    %08lx  %s\n",
                                     (unsigned long) SYMBOL_VALUE_ADDRESS (msymbol),
                                     SYMBOL_SOURCE_NAME (msymbol));
                   }}}}}


 > 
 > > +     For functions, find_pc_symtab should succeed if we have debug info
 > > +     for the function, for variables we have to call lookup_symbol
 > > +     to determine if the variable has debug info.
 > > +     If the lookup fails, set found_misc so that we will rescan to print
 > > +     any matching symbols without debug info.
 > 
 > So, at some point, this code had explicit assumptions about when there
 > should be a symbol corresponding to a minimal symbol, and set
 > found_misc appropriately.  But then, at some point, 'found_misc' went
 > away; it left, as a legacy, a somewhat strange flow of control that
 > wasn't at all explicit until I had to introduce 'force_return' to
 > mimic it.  

Found_misc is set if lookup_symbol returns null, which then leads to
looking through the minsyms. The assumption is that found_misc is set
if there is no symbol/debuginfo for a name.  With your force_return
elimination, is this still the case?  I see that it is for the HPPA
case, but for the other. You wouldn't return NULL, right there, but
would continue searching. Would this matter? I am not sure. Probably
you would end up returning NULL anyway later?


 > 
 > Also, the line
 > 
 > > +     The symbol will then be found during the scan of symtabs below.
 > 
 > makes me wonder if, at this time, the minimal symbol search happened
 > before the symtab search; I'm not sure how relevant that is.
 > 
 > At any rate, this was certainly interesting in terms of giving an idea
 > as to how the current flow of control might have arisen.  I still
 > think my patch is okay, though.
 > 

See above for why I am not truely convinced yet.

Does it matter that we keep going with the search instead of stopping?

Elena


 > David Carlton
 > carlton@math.stanford.edu

Re: [rfa] delete 'force_return' from lookup_symbol_aux_minsyms

[David Carlton]

On Thu, 19 Dec 2002 11:50:36 -0500, Elena Zannoni <ezannoni@redhat.com> said:
> David Carlton writes:

>> 'found_misc' also seems to have gone away; 

> Actually it is still there, the code I posted is for
> list/search_symbols.

Oh, I apologize, I completely misunderstood what you were saying.  I
thought that the code you dug up was an earlier version of
lookup_symbol.

Now I understand your point: I was claiming that no callers of
lookup_symbol depended on this NULL return that I'm trying to get rid
of, but you suspect that search_symbols might be such a caller.  And,
indeed, I hadn't considered that particular caller.

Looking into it further, I think you might have reason to worry.
Here's the relevant section of search_symbols:

  if (0 == find_pc_symtab (SYMBOL_VALUE_ADDRESS (msymbol)))
    {
      if (kind == FUNCTIONS_NAMESPACE
	  || lookup_symbol (SYMBOL_NAME (msymbol),
                            (struct block *) NULL,
                            VAR_NAMESPACE,
                            0, (struct symtab **) NULL) == NULL)
        found_misc = 1;
    }

And here's an (abbreviated) version of all of the uses of force_return
in lookup_symbol_aux_minsyms:

  s = find_pc_sect_symtab (SYMBOL_VALUE_ADDRESS (msymbol),
			   SYMBOL_BFD_SECTION (msymbol));
  if (s != NULL)
    {
      <code deleted that might set *force_return to 1>
    }
  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,
                                NULL, namespace, is_a_field_of_this,
                                symtab);
    }

These two sections of code are remarkably parallel, for reasons that I
don't understand.  And they're clearly trying to investigate the same
minimal symbol: the point of that code in search_symbols it to
determine whether or not a particular minimal symbol has debugging
info, so the question at hand is whether or not lookup_symbol is
supposed to terminate when it hits the minimal symbol that
search_symbols is interested in.  So I think it's safe to assume that
we're only interested in comparing the two pieces of code when
'msymbol' has the same value in both.

In that case, the find_pc_symtab in search_symbols corresponds to the
find_pc_sect_symtab in lookup_symbol_aux_minsyms (and should probably
be changed to find_pc_sect_symtab, but never mind that for now).
We're assuming that that symtab is 0; that means that we're in the
else clause of the lookup_symbol_aux_minsyms call.

So when does that else clause happens?  It's guarded by a conditional:
that tests that the MSYMBOL_TYPE of the msymbol isn't text, and that
the name we're searching under isn't the SYMBOL_NAME of the msymbol.

The former condition is true: we're assuming that kind isn't
FUNCTIONS_NAMESPACE, so the minimal symbol shouldn't be text.  But
we've called lookup_symbol with the 'name' argument equal to
SYMBOL_NAME (msymbol).  And, in that case, the test for

  !STREQ (name, SYMBOL_NAME (msymbol)

should fail.

Except that isn't right, either!  Because SYMBOL_NAME (msymbol) would
be the mangled name of 'msymbol', and lookup_symbol demangled it, so
the 'name' argument to lookup_symbol_aux_minsyms would actually be
demangled.  So, indeed, we might well be in a situation where
force_return comes into play.

Phew.  Assuming that analysis is correct, I have two comments and a
suggestion.

Comment #1: This whole logic is hopelessly unclear.  I think I'd
rather turn the code into something possibly broken but clearer, and
then try to fix any possible breakage, than try to leave it as is.

Comment #2: Just what is up with the call to lookup_symbol_aux from
within lookup_symbol_aux_minsyms, anyways?  It's passing in a mangled
name as the first argument; but lookup_symbol_aux normally expects its
first argument to be demangled.  I'm not sure what's going on here:
that call might be broken, or there might be some part of
lookup_symbol_aux that does something with a mangled first argument.
If the latter is the case, then there should be comments making this
explicit, and the call to lookup_symbol_aux should be replaced by a
call to lookup_symbol_aux_<something>.

Suggestion: The whole purpose of the call to lookup_symbol from within
search_symbols is to try to track down information about one
particular minimal symbol: does it have debugging information, or
doesn't it?  Given that that's the case, doing that via lookup_symbol
is at best overkill and at worst wrong.  This suggests that we might
be able to get around the issue by replacing that call to
lookup_symbol by a call to lookup_symbol_aux_minsyms.  The only
question that I have is whether the first argument should then be the
mangled name of 'msymbol' or the demangled name; I'd be happier if we
understood the situation with respect to my "Comment #2".

A datum which may or may not be relevant: currently, partial symbols
never have their names demangled.  I'd assumed that this was a bug;
but I just noticed the following comment in search_symbols:

  This is in particular necessary for demangled variable names,
  which are no longer put into the partial symbol tables.

Sigh.  So I have another suggestion:

Suggestion #2: Maybe we should put this particular patch on hold and
come to some sort of consensus as to how to deal with
mangled/demangled names.  I'll post an RFC for that later today.

David Carlton
carlton@math.stanford.edu

Re: [rfa] delete 'force_return' from lookup_symbol_aux_minsyms

[Elena Zannoni]

David Carlton writes:
 > On Thu, 19 Dec 2002 11:50:36 -0500, Elena Zannoni <ezannoni@redhat.com> said:
 > > David Carlton writes:
 > 
 > >> 'found_misc' also seems to have gone away; 
 > 
 > > Actually it is still there, the code I posted is for
 > > list/search_symbols.
 > 
 > Oh, I apologize, I completely misunderstood what you were saying.  I
 > thought that the code you dug up was an earlier version of
 > lookup_symbol.
 > 
 > Now I understand your point: I was claiming that no callers of
 > lookup_symbol depended on this NULL return that I'm trying to get rid
 > of, but you suspect that search_symbols might be such a caller.  And,
 > indeed, I hadn't considered that particular caller.
 > 
 > Looking into it further, I think you might have reason to worry.
 > Here's the relevant section of search_symbols:
 > 
 >   if (0 == find_pc_symtab (SYMBOL_VALUE_ADDRESS (msymbol)))
 >     {
 >       if (kind == FUNCTIONS_NAMESPACE
 > 	  || lookup_symbol (SYMBOL_NAME (msymbol),
 >                             (struct block *) NULL,
 >                             VAR_NAMESPACE,
 >                             0, (struct symtab **) NULL) == NULL)
 >         found_misc = 1;
 >     }
 > 
 > And here's an (abbreviated) version of all of the uses of force_return
 > in lookup_symbol_aux_minsyms:
 > 
 >   s = find_pc_sect_symtab (SYMBOL_VALUE_ADDRESS (msymbol),
 > 			   SYMBOL_BFD_SECTION (msymbol));
 >   if (s != NULL)
 >     {
 >       <code deleted that might set *force_return to 1>
 >     }
 >   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,
 >                                 NULL, namespace, is_a_field_of_this,
 >                                 symtab);
 >     }
 > 
 > These two sections of code are remarkably parallel, for reasons that I
 > don't understand.  And they're clearly trying to investigate the same
 > minimal symbol: the point of that code in search_symbols it to
 > determine whether or not a particular minimal symbol has debugging
 > info, so the question at hand is whether or not lookup_symbol is
 > supposed to terminate when it hits the minimal symbol that
 > search_symbols is interested in.  So I think it's safe to assume that
 > we're only interested in comparing the two pieces of code when
 > 'msymbol' has the same value in both.
 > 

Yes, I agree with this interpretation.

 > In that case, the find_pc_symtab in search_symbols corresponds to the
 > find_pc_sect_symtab in lookup_symbol_aux_minsyms (and should probably
 > be changed to find_pc_sect_symtab, but never mind that for now).
 > We're assuming that that symtab is 0; that means that we're in the
 > else clause of the lookup_symbol_aux_minsyms call.
 > 

yes

 > So when does that else clause happens?  It's guarded by a conditional:
 > that tests that the MSYMBOL_TYPE of the msymbol isn't text, and that
 > the name we're searching under isn't the SYMBOL_NAME of the msymbol.
 > 
 > The former condition is true: we're assuming that kind isn't
 > FUNCTIONS_NAMESPACE, so the minimal symbol shouldn't be text.  But
 > we've called lookup_symbol with the 'name' argument equal to
 > SYMBOL_NAME (msymbol).  And, in that case, the test for
 > 
 >   !STREQ (name, SYMBOL_NAME (msymbol)
 > 
 > should fail.
 > 

ok...

 > Except that isn't right, either!  Because SYMBOL_NAME (msymbol) would
 > be the mangled name of 'msymbol', 

yes,

 > and lookup_symbol demangled it, so

ah right, it would become modified_name

 > the 'name' argument to lookup_symbol_aux_minsyms would actually be
 > demangled.  

ok, I think I follow up to here.

 > So, indeed, we might well be in a situation where
 > force_return comes into play.
 > 

I am lost now.

 > Phew.  Assuming that analysis is correct, I have two comments and a
 > suggestion.
 > 
 > Comment #1: This whole logic is hopelessly unclear.  I think I'd
 > rather turn the code into something possibly broken but clearer, and
 > then try to fix any possible breakage, than try to leave it as is.
 > 

tempting

 > Comment #2: Just what is up with the call to lookup_symbol_aux from
 > within lookup_symbol_aux_minsyms, anyways?  It's passing in a mangled
 > name as the first argument; but lookup_symbol_aux normally expects its
 > first argument to be demangled.  I'm not sure what's going on here:
 > that call might be broken, or there might be some part of
 > lookup_symbol_aux that does something with a mangled first argument.
 > If the latter is the case, then there should be comments making this
 > explicit, and the call to lookup_symbol_aux should be replaced by a
 > call to lookup_symbol_aux_<something>.
 > 

I think it's just broken. The call in search_symbols predates the
introduction of lookup_symbol_aux and the demangling logic. So I think
it just wasn't updated to reflect the new behavior.

 > Suggestion: The whole purpose of the call to lookup_symbol from within
 > search_symbols is to try to track down information about one
 > particular minimal symbol: does it have debugging information, or
 > doesn't it?  Given that that's the case, doing that via lookup_symbol
 > is at best overkill and at worst wrong.  This suggests that we might
 > be able to get around the issue by replacing that call to
 > lookup_symbol by a call to lookup_symbol_aux_minsyms.  The only
 > question that I have is whether the first argument should then be the
 > mangled name of 'msymbol' or the demangled name; I'd be happier if we
 > understood the situation with respect to my "Comment #2".
 > 

Yeah, I think it's just a coincidence that lookup_symbol is still
called.  At the time that call was introduced, lookup_symbol was maybe
the only function available for this sort of things.
We could try to call the lookup_symbol_aux_minsyms function.


 > A datum which may or may not be relevant: currently, partial symbols
 > never have their names demangled.  I'd assumed that this was a bug;
 > but I just noticed the following comment in search_symbols:
 > 
 >   This is in particular necessary for demangled variable names,
 >   which are no longer put into the partial symbol tables.
 > 
 > Sigh.  So I have another suggestion:
 > 
 > Suggestion #2: Maybe we should put this particular patch on hold and
 > come to some sort of consensus as to how to deal with
 > mangled/demangled names.  I'll post an RFC for that later today.
 > 

Ok, whatever seems easier for you. Although I think we can try to fix
the parameter problem, at least, and see what breaks.

Elena


 > David Carlton
 > carlton@math.stanford.edu

Re: [rfa] delete 'force_return' from lookup_symbol_aux_minsyms

[David Carlton]

On Thu, 19 Dec 2002 17:37:33 -0500, Elena Zannoni <ezannoni@redhat.com> said:
> David Carlton writes:

>> The former condition is true: we're assuming that kind isn't
>> FUNCTIONS_NAMESPACE, so the minimal symbol shouldn't be text.  But
>> we've called lookup_symbol with the 'name' argument equal to
>> SYMBOL_NAME (msymbol).  And, in that case, the test for
>> 
>> !STREQ (name, SYMBOL_NAME (msymbol)
>> 
>> should fail.

> ok...

>> Except that isn't right, either!  Because SYMBOL_NAME (msymbol) would
>> be the mangled name of 'msymbol', 

> yes,

>> and lookup_symbol demangled it, so

> ah right, it would become modified_name

>> the 'name' argument to lookup_symbol_aux_minsyms would actually be
>> demangled.  

> ok, I think I follow up to here.

>> So, indeed, we might well be in a situation where
>> force_return comes into play.

> I am lost now.

We're trying to see if we hit this code:

  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,
                                NULL, namespace, is_a_field_of_this,
                                symtab);
    }

We've decided that the MSYMBOL_TYPE isn't mst_text or mst_file_text.
And name is the _demangled_ version of SYMBOL_NAME (msymbol), so
indeed the two are not STREQ.

So force_return is set to 1, which means that force_return is having
an effect, so deleting it would change the flow of control.  If I'm
not missing something.

>> Comment #2: Just what is up with the call to lookup_symbol_aux from
>> within lookup_symbol_aux_minsyms, anyways?  It's passing in a mangled
>> name as the first argument; but lookup_symbol_aux normally expects its
>> first argument to be demangled.  I'm not sure what's going on here:
>> that call might be broken, or there might be some part of
>> lookup_symbol_aux that does something with a mangled first argument.
>> If the latter is the case, then there should be comments making this
>> explicit, and the call to lookup_symbol_aux should be replaced by a
>> call to lookup_symbol_aux_<something>.

> I think it's just broken. The call in search_symbols predates the
> introduction of lookup_symbol_aux and the demangling logic. So I
> think it just wasn't updated to reflect the new behavior.

Yeah, I think you're right.

>> Suggestion: The whole purpose of the call to lookup_symbol from within
>> search_symbols is to try to track down information about one
>> particular minimal symbol: does it have debugging information, or
>> doesn't it?  Given that that's the case, doing that via lookup_symbol
>> is at best overkill and at worst wrong.  This suggests that we might
>> be able to get around the issue by replacing that call to
>> lookup_symbol by a call to lookup_symbol_aux_minsyms.  The only
>> question that I have is whether the first argument should then be the
>> mangled name of 'msymbol' or the demangled name; I'd be happier if we
>> understood the situation with respect to my "Comment #2".

> Yeah, I think it's just a coincidence that lookup_symbol is still
> called.  At the time that call was introduced, lookup_symbol was maybe
> the only function available for this sort of things.

Exactly.

>> Suggestion #2: Maybe we should put this particular patch on hold and
>> come to some sort of consensus as to how to deal with
>> mangled/demangled names.  I'll post an RFC for that later today.

> Ok, whatever seems easier for you. Although I think we can try to
> fix the parameter problem, at least, and see what breaks.

I started to write an RFC, but actually I think now isn't the best
time for that: it'll affect GDB fairly broadly, and enough people are
away on holidays that I don't want to propose that right now.  So, in
early January, I'll see if I can get some sort of consensus towards
the right way to approach this.

In the mean time, though, it seems like we agree that deleting
force_return would only affect the flow of control in one place where
we don't understand the logic and where we suspect the logic is buggy.
So let's just delete it, since you seem to be leaning that way as
well.  I doubt it will break anything (at the very most it will break
some strange edge case), and if something does break then we'll have a
test case, so we should be able to fix it fairly easily once we have
an actual test case.

Here's a revised version of the patch: it's exactly the same as the
previous one, except that it changes the call to lookup_symbol in
search_symbols to call lookup_symbol_aux_minsyms instead.  Tested on
i686-pc-linux-gnu/GCC 3.1/DWARF 2 (Michael Chastain has been nagging
me to give such info, but don't worry, I always test my patches :-) );
no new regressions.

David Carlton
carlton@math.stanford.edu

2002-12-19  David Carlton  <carlton@math.stanford.edu>

	* symtab.c (lookup_symbol_aux): Delete 'force_return' variable.
	(lookup_symbol_aux_minsyms): Delete 'force_return' argument.
	(search_symbols): Call lookup_symbol_aux_minsyms to find debugging
	information associated to a minsym, not lookup_symbol.

Index: symtab.c
===================================================================
RCS file: /cvs/src/src/gdb/symtab.c,v
retrieving revision 1.82
diff -u -p -r1.82 symtab.c
--- symtab.c	17 Dec 2002 18:34:06 -0000	1.82
+++ symtab.c	19 Dec 2002 23:02:35 -0000
@@ -117,8 +117,7 @@ struct symbol *lookup_symbol_aux_minsyms
 					  const char *mangled_name,
 					  const namespace_enum namespace,
 					  int *is_a_field_of_this,
-					  struct symtab **symtab,
-					  int *force_return);
+					  struct symtab **symtab);
 
 static struct symbol *find_active_alias (struct symbol *sym, CORE_ADDR addr);
 
@@ -805,14 +804,6 @@ lookup_symbol_aux (const char *name, con
   struct symbol *sym;
   const struct block *static_block;
 
-  /* FIXME: carlton/2002-11-05: This variable is here so that
-     lookup_symbol_aux will sometimes return NULL after receiving a
-     NULL return value from lookup_symbol_aux_minsyms, without
-     proceeding on to the partial symtab and static variable tests.  I
-     suspect that that's a bad idea.  */
-  
-  int force_return;
-
   /* Search specified block and its superiors.  Don't search
      STATIC_BLOCK or GLOBAL_BLOCK.  */
 
@@ -931,13 +922,11 @@ lookup_symbol_aux (const char *name, con
      a mangled variable that is stored in one of the minimal symbol tables.
      Eventually, all global symbols might be resolved in this way.  */
 
-  force_return = 0;
-
   sym = lookup_symbol_aux_minsyms (name, mangled_name,
 				   namespace, is_a_field_of_this,
-				   symtab, &force_return);
+				   symtab);
   
-  if (sym != NULL || force_return == 1)
+  if (sym != NULL)
     return sym;
 
 #endif
@@ -981,13 +970,11 @@ lookup_symbol_aux (const char *name, con
    */
 
 
-  force_return = 0;
-
   sym = lookup_symbol_aux_minsyms (name, mangled_name,
 				   namespace, is_a_field_of_this,
-				   symtab, &force_return);
+				   symtab);
   
-  if (sym != NULL || force_return == 1)
+  if (sym != NULL)
     return sym;
 
 #endif
@@ -1172,13 +1159,20 @@ lookup_symbol_aux_psymtabs (int block_in
    tables.  Eventually, all global symbols might be resolved in this
    way.  */
 
+/* NOTE: carlton/2002-12-05: At one point, this function was part of
+   lookup_symbol_aux, and what are now 'return' statements within
+   lookup_symbol_aux_minsyms returned from lookup_symbol_aux, even if
+   sym was NULL.  As far as I can tell, this was basically accidental;
+   it didn't happen every time that msymbol was non-NULL, but only if
+   some additional conditions held as well, and it caused problems
+   with HP-generated symbol tables.  */
+
 static struct symbol *
 lookup_symbol_aux_minsyms (const char *name,
 			   const char *mangled_name,
 			   const namespace_enum namespace,
 			   int *is_a_field_of_this,
-			   struct symtab **symtab,
-			   int *force_return)
+			   struct symtab **symtab)
 {
   struct symbol *sym;
   struct blockvector *bv;
@@ -1271,7 +1265,6 @@ lookup_symbol_aux_minsyms (const char *n
 
 	      if (symtab != NULL && sym != NULL)
 		*symtab = s;
-	      *force_return = 1;
 	      return fixup_symbol_section (sym, s->objfile);
 	    }
 	  else if (MSYMBOL_TYPE (msymbol) != mst_text
@@ -1280,7 +1273,6 @@ lookup_symbol_aux_minsyms (const char *n
 	    {
 	      /* This is a mangled variable, look it up by its
 	         mangled name.  */
-	      *force_return = 1;
 	      return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name,
 					NULL, namespace, is_a_field_of_this,
 					symtab);
@@ -2904,12 +2896,31 @@ search_symbols (char *regexp, namespace_
 	      {
 		if (0 == find_pc_symtab (SYMBOL_VALUE_ADDRESS (msymbol)))
 		  {
-		    if (kind == FUNCTIONS_NAMESPACE
-			|| lookup_symbol (SYMBOL_NAME (msymbol),
-					  (struct block *) NULL,
-					  VAR_NAMESPACE,
-					0, (struct symtab **) NULL) == NULL)
-		      found_misc = 1;
+		    if (kind == FUNCTIONS_NAMESPACE)
+		      {
+			found_misc = 1;
+		      }
+		    else
+		      {
+			struct symbol *sym;
+
+			if (SYMBOL_DEMANGLED_NAME (msymbol) != NULL)
+			  sym
+			    = lookup_symbol_aux_minsyms (SYMBOL_DEMANGLED_NAME
+							 (msymbol),
+							 SYMBOL_NAME (msymbol),
+							 VAR_NAMESPACE,
+							 NULL, NULL);
+			else
+			  sym
+			    = lookup_symbol_aux_minsyms (SYMBOL_NAME (msymbol),
+							 NULL,
+							 VAR_NAMESPACE,
+							 NULL, NULL);
+
+			if (sym == NULL)
+			  found_misc = 1;
+		      }
 		  }
 	      }
 	  }

Re: [rfa] delete 'force_return' from lookup_symbol_aux_minsyms

[Elena Zannoni]

David Carlton writes:
 > On Thu, 19 Dec 2002 17:37:33 -0500, Elena Zannoni <ezannoni@redhat.com> said:
 > > David Carlton writes:
 > 
 > >> The former condition is true: we're assuming that kind isn't
 > >> FUNCTIONS_NAMESPACE, so the minimal symbol shouldn't be text.  But
 > >> we've called lookup_symbol with the 'name' argument equal to
 > >> SYMBOL_NAME (msymbol).  And, in that case, the test for
 > >> 
 > >> !STREQ (name, SYMBOL_NAME (msymbol)
 > >> 
 > >> should fail.
 > 
 > > ok...
 > 
 > >> Except that isn't right, either!  Because SYMBOL_NAME (msymbol) would
 > >> be the mangled name of 'msymbol', 
 > 
 > > yes,
 > 
 > >> and lookup_symbol demangled it, so
 > 
 > > ah right, it would become modified_name
 > 
 > >> the 'name' argument to lookup_symbol_aux_minsyms would actually be
 > >> demangled.  
 > 
 > > ok, I think I follow up to here.
 > 
 > >> So, indeed, we might well be in a situation where
 > >> force_return comes into play.
 > 
 > > I am lost now.
 > 
 > We're trying to see if we hit this code:
 > 
 >   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,
 >                                 NULL, namespace, is_a_field_of_this,
 >                                 symtab);
 >     }
 > 
 > We've decided that the MSYMBOL_TYPE isn't mst_text or mst_file_text.
 > And name is the _demangled_ version of SYMBOL_NAME (msymbol), so
 > indeed the two are not STREQ.
 > 

Ah yes. ok.

 > So force_return is set to 1, which means that force_return is having
 > an effect, so deleting it would change the flow of control.  If I'm
 > not missing something.
 > 

right

 > >> Comment #2: Just what is up with the call to lookup_symbol_aux from
 > >> within lookup_symbol_aux_minsyms, anyways?  It's passing in a mangled
 > >> name as the first argument; but lookup_symbol_aux normally expects its
 > >> first argument to be demangled.  I'm not sure what's going on here:
 > >> that call might be broken, or there might be some part of
 > >> lookup_symbol_aux that does something with a mangled first argument.
 > >> If the latter is the case, then there should be comments making this
 > >> explicit, and the call to lookup_symbol_aux should be replaced by a
 > >> call to lookup_symbol_aux_<something>.
 > 
 > > I think it's just broken. The call in search_symbols predates the
 > > introduction of lookup_symbol_aux and the demangling logic. So I
 > > think it just wasn't updated to reflect the new behavior.
 > 
 > Yeah, I think you're right.
 > 
 > >> Suggestion: The whole purpose of the call to lookup_symbol from within
 > >> search_symbols is to try to track down information about one
 > >> particular minimal symbol: does it have debugging information, or
 > >> doesn't it?  Given that that's the case, doing that via lookup_symbol
 > >> is at best overkill and at worst wrong.  This suggests that we might
 > >> be able to get around the issue by replacing that call to
 > >> lookup_symbol by a call to lookup_symbol_aux_minsyms.  The only
 > >> question that I have is whether the first argument should then be the
 > >> mangled name of 'msymbol' or the demangled name; I'd be happier if we
 > >> understood the situation with respect to my "Comment #2".
 > 
 > > Yeah, I think it's just a coincidence that lookup_symbol is still
 > > called.  At the time that call was introduced, lookup_symbol was maybe
 > > the only function available for this sort of things.
 > 
 > Exactly.
 > 
 > >> Suggestion #2: Maybe we should put this particular patch on hold and
 > >> come to some sort of consensus as to how to deal with
 > >> mangled/demangled names.  I'll post an RFC for that later today.
 > 
 > > Ok, whatever seems easier for you. Although I think we can try to
 > > fix the parameter problem, at least, and see what breaks.
 > 
 > I started to write an RFC, but actually I think now isn't the best
 > time for that: it'll affect GDB fairly broadly, and enough people are
 > away on holidays that I don't want to propose that right now.  So, in
 > early January, I'll see if I can get some sort of consensus towards
 > the right way to approach this.
 > 

Alternatively you can post something and wait a bit. Maybe MichaelC
will put it through his test harness.

 > In the mean time, though, it seems like we agree that deleting
 > force_return would only affect the flow of control in one place where
 > we don't understand the logic and where we suspect the logic is buggy.
 > So let's just delete it, since you seem to be leaning that way as
 > well.  I doubt it will break anything (at the very most it will break
 > some strange edge case), and if something does break then we'll have a
 > test case, so we should be able to fix it fairly easily once we have
 > an actual test case.
 > 

Ah, living dangerously!

 > Here's a revised version of the patch: it's exactly the same as the
 > previous one, except that it changes the call to lookup_symbol in
 > search_symbols to call lookup_symbol_aux_minsyms instead.  Tested on
 > i686-pc-linux-gnu/GCC 3.1/DWARF 2 (Michael Chastain has been nagging
 > me to give such info, but don't worry, I always test my patches :-) );
 > no new regressions.
 > 

Since all this code was written when stabs was king, maybe we'll find
differences with stabs testing.  Also, given that as you say, people
are on vacation, the entropy of gdb should be a bit lower these days,
and if something breaks, it's probably because of this change, so it
should be more easily trackable. :-)

OK.
Elena

PS. Michael, could you do some testsuite grinding?


 > David Carlton
 > carlton@math.stanford.edu
 > 
 > 2002-12-19  David Carlton  <carlton@math.stanford.edu>
 > 
 > 	* symtab.c (lookup_symbol_aux): Delete 'force_return' variable.
 > 	(lookup_symbol_aux_minsyms): Delete 'force_return' argument.
 > 	(search_symbols): Call lookup_symbol_aux_minsyms to find debugging
 > 	information associated to a minsym, not lookup_symbol.
 > 
 > Index: symtab.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/symtab.c,v
 > retrieving revision 1.82
 > diff -u -p -r1.82 symtab.c
 > --- symtab.c	17 Dec 2002 18:34:06 -0000	1.82
 > +++ symtab.c	19 Dec 2002 23:02:35 -0000
 > @@ -117,8 +117,7 @@ struct symbol *lookup_symbol_aux_minsyms
 >  					  const char *mangled_name,
 >  					  const namespace_enum namespace,
 >  					  int *is_a_field_of_this,
 > -					  struct symtab **symtab,
 > -					  int *force_return);
 > +					  struct symtab **symtab);
 >  
 >  static struct symbol *find_active_alias (struct symbol *sym, CORE_ADDR addr);
 >  
 > @@ -805,14 +804,6 @@ lookup_symbol_aux (const char *name, con
 >    struct symbol *sym;
 >    const struct block *static_block;
 >  
 > -  /* FIXME: carlton/2002-11-05: This variable is here so that
 > -     lookup_symbol_aux will sometimes return NULL after receiving a
 > -     NULL return value from lookup_symbol_aux_minsyms, without
 > -     proceeding on to the partial symtab and static variable tests.  I
 > -     suspect that that's a bad idea.  */
 > -  
 > -  int force_return;
 > -
 >    /* Search specified block and its superiors.  Don't search
 >       STATIC_BLOCK or GLOBAL_BLOCK.  */
 >  
 > @@ -931,13 +922,11 @@ lookup_symbol_aux (const char *name, con
 >       a mangled variable that is stored in one of the minimal symbol tables.
 >       Eventually, all global symbols might be resolved in this way.  */
 >  
 > -  force_return = 0;
 > -
 >    sym = lookup_symbol_aux_minsyms (name, mangled_name,
 >  				   namespace, is_a_field_of_this,
 > -				   symtab, &force_return);
 > +				   symtab);
 >    
 > -  if (sym != NULL || force_return == 1)
 > +  if (sym != NULL)
 >      return sym;
 >  
 >  #endif
 > @@ -981,13 +970,11 @@ lookup_symbol_aux (const char *name, con
 >     */
 >  
 >  
 > -  force_return = 0;
 > -
 >    sym = lookup_symbol_aux_minsyms (name, mangled_name,
 >  				   namespace, is_a_field_of_this,
 > -				   symtab, &force_return);
 > +				   symtab);
 >    
 > -  if (sym != NULL || force_return == 1)
 > +  if (sym != NULL)
 >      return sym;
 >  
 >  #endif
 > @@ -1172,13 +1159,20 @@ lookup_symbol_aux_psymtabs (int block_in
 >     tables.  Eventually, all global symbols might be resolved in this
 >     way.  */
 >  
 > +/* NOTE: carlton/2002-12-05: At one point, this function was part of
 > +   lookup_symbol_aux, and what are now 'return' statements within
 > +   lookup_symbol_aux_minsyms returned from lookup_symbol_aux, even if
 > +   sym was NULL.  As far as I can tell, this was basically accidental;
 > +   it didn't happen every time that msymbol was non-NULL, but only if
 > +   some additional conditions held as well, and it caused problems
 > +   with HP-generated symbol tables.  */
 > +
 >  static struct symbol *
 >  lookup_symbol_aux_minsyms (const char *name,
 >  			   const char *mangled_name,
 >  			   const namespace_enum namespace,
 >  			   int *is_a_field_of_this,
 > -			   struct symtab **symtab,
 > -			   int *force_return)
 > +			   struct symtab **symtab)
 >  {
 >    struct symbol *sym;
 >    struct blockvector *bv;
 > @@ -1271,7 +1265,6 @@ lookup_symbol_aux_minsyms (const char *n
 >  
 >  	      if (symtab != NULL && sym != NULL)
 >  		*symtab = s;
 > -	      *force_return = 1;
 >  	      return fixup_symbol_section (sym, s->objfile);
 >  	    }
 >  	  else if (MSYMBOL_TYPE (msymbol) != mst_text
 > @@ -1280,7 +1273,6 @@ lookup_symbol_aux_minsyms (const char *n
 >  	    {
 >  	      /* This is a mangled variable, look it up by its
 >  	         mangled name.  */
 > -	      *force_return = 1;
 >  	      return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name,
 >  					NULL, namespace, is_a_field_of_this,
 >  					symtab);
 > @@ -2904,12 +2896,31 @@ search_symbols (char *regexp, namespace_
 >  	      {
 >  		if (0 == find_pc_symtab (SYMBOL_VALUE_ADDRESS (msymbol)))
 >  		  {
 > -		    if (kind == FUNCTIONS_NAMESPACE
 > -			|| lookup_symbol (SYMBOL_NAME (msymbol),
 > -					  (struct block *) NULL,
 > -					  VAR_NAMESPACE,
 > -					0, (struct symtab **) NULL) == NULL)
 > -		      found_misc = 1;
 > +		    if (kind == FUNCTIONS_NAMESPACE)
 > +		      {
 > +			found_misc = 1;
 > +		      }
 > +		    else
 > +		      {
 > +			struct symbol *sym;
 > +
 > +			if (SYMBOL_DEMANGLED_NAME (msymbol) != NULL)
 > +			  sym
 > +			    = lookup_symbol_aux_minsyms (SYMBOL_DEMANGLED_NAME
 > +							 (msymbol),
 > +							 SYMBOL_NAME (msymbol),
 > +							 VAR_NAMESPACE,
 > +							 NULL, NULL);
 > +			else
 > +			  sym
 > +			    = lookup_symbol_aux_minsyms (SYMBOL_NAME (msymbol),
 > +							 NULL,
 > +							 VAR_NAMESPACE,
 > +							 NULL, NULL);
 > +
 > +			if (sym == NULL)
 > +			  found_misc = 1;
 > +		      }
 >  		  }
 >  	      }
 >  	  }

Re: [rfa] delete 'force_return' from lookup_symbol_aux_minsyms

[David Carlton]

On Thu, 19 Dec 2002 19:03:59 -0500, Elena Zannoni <ezannoni@redhat.com> said:
> David Carlton writes:
>> On Thu, 19 Dec 2002 17:37:33 -0500, Elena Zannoni <ezannoni@redhat.com> said:
>>> David Carlton writes:

>>>> Suggestion #2: Maybe we should put this particular patch on hold and
>>>> come to some sort of consensus as to how to deal with
>>>> mangled/demangled names.  I'll post an RFC for that later today.

>>> Ok, whatever seems easier for you. Although I think we can try to
>>> fix the parameter problem, at least, and see what breaks.

>> I started to write an RFC, but actually I think now isn't the best
>> time for that: it'll affect GDB fairly broadly, and enough people
>> are away on holidays that I don't want to propose that right now.
>> So, in early January, I'll see if I can get some sort of consensus
>> towards the right way to approach this.

> Alternatively you can post something and wait a bit. Maybe MichaelC
> will put it through his test harness.

I think that this issue is comprehensive enough both to deserve an RFC
and to require a series of patches to resolve.  (Maybe even another
branch...)

>> Here's a revised version of the patch: it's exactly the same as the
>> previous one, except that it changes the call to lookup_symbol in
>> search_symbols to call lookup_symbol_aux_minsyms instead.  Tested on
>> i686-pc-linux-gnu/GCC 3.1/DWARF 2 (Michael Chastain has been nagging
>> me to give such info, but don't worry, I always test my patches :-) );
>> no new regressions.

> Since all this code was written when stabs was king, maybe we'll find
> differences with stabs testing.  Also, given that as you say, people
> are on vacation, the entropy of gdb should be a bit lower these days,
> and if something breaks, it's probably because of this change, so it
> should be more easily trackable. :-)

> OK.

Great.  Then I'll commit it now, and I'll hold off on my next patch in
this series (which this was setting up ground work for) until some
time in January, so that Michael's tests have had a chance to see it
and other people will be back from vacation and had a chance to run
into it.  That way, it will be easy to back out if necessary.

And I'll start working on a function "lookup_symbol_minsym" which
tries to find the symbol associated to a minsym: that would probably
be the best way to handle that call to lookup_symbol from within
search_symbols that we're replacing by a call to
lookup_symbol_aux_minsyms.  I think it will be useful elsewhere, as
well: when I skimmed all the callers to lookup_symbol, I remember
finding some other places that wanted to match a symbol to a specific
mangled name, and they probably got that mangled name from a minsym.

David Carlton
carlton@math.stanford.edu

Re: [rfa] delete 'force_return' from lookup_symbol_aux_minsyms

[Michael Elizabeth Chastain]

I'm not following the thread so I'm not sure what patch you're referring
to.  But if you want to give me a URL for a patch, any time this weekend
or the beginning of next week is a good time for testbedding.

It takes roughly 5 hours to test all configurations on one version of
gdb now -- I am not in a position to testbed lots of patches.  The big
time sink is that I test (all gdb) * (all binutils) and there are lots
of each of them.  It's much faster if I run just one test script, but
for a symbol table lookup patch, I want to run the whole suite.

The build time for gcc is increasing even worse than Gate's Law, also.
(Gates' Law: every 18 months, the speed of software halves).

Michael C
Gripe, gripe.

Re: [rfa] delete 'force_return' from lookup_symbol_aux_minsyms

[Elena Zannoni]

Michael Elizabeth Chastain writes:
 > I'm not following the thread so I'm not sure what patch you're referring
 > to.  But if you want to give me a URL for a patch, any time this weekend
 > or the beginning of next week is a good time for testbedding.
 > 

I am not sure if David committed it yet, but the patch it at the
bottom of:
http://sources.redhat.com/ml/gdb-patches/2002-12/msg00560.html
Maybe you already tested it!

 > It takes roughly 5 hours to test all configurations on one version of
 > gdb now -- I am not in a position to testbed lots of patches.  The big
 > time sink is that I test (all gdb) * (all binutils) and there are lots
 > of each of them.  It's much faster if I run just one test script, but
 > for a symbol table lookup patch, I want to run the whole suite.
 > 

Yes, that's why i thought your system was the perfect one for this.

 > The build time for gcc is increasing even worse than Gate's Law, also.
 > (Gates' Law: every 18 months, the speed of software halves).
 > 

:-(

 > Michael C
 > Gripe, gripe.

Elena

Re: [rfa] delete 'force_return' from lookup_symbol_aux_minsyms

[David Carlton]

On Sat, 21 Dec 2002 13:58:44 -0500, Elena Zannoni <ezannoni@redhat.com> said:

> Michael Elizabeth Chastain writes:
>> I'm not following the thread so I'm not sure what patch you're referring
>> to.  But if you want to give me a URL for a patch, any time this weekend
>> or the beginning of next week is a good time for testbedding.
>> 

> I am not sure if David committed it yet, but the patch it at the
> bottom of:
> http://sources.redhat.com/ml/gdb-patches/2002-12/msg00560.html
> Maybe you already tested it!

I committed it yesterday.

Though I thought some more about this last night: I'm finally starting
to understand the minsym stuff.  My current opinion is that deleting
force_return is good, but maybe changing that lookup_symbol call
within search_symbols wasn't the best idea.  I'll send something more
specific on Monday.

David Carlton
carlton@math.stanford.edu

Re: [rfa] delete 'force_return' from lookup_symbol_aux_minsyms

[David Carlton]

On 21 Dec 2002 20:14:14 -0800, David Carlton <carlton@math.Stanford.EDU> said:
> On Sat, 21 Dec 2002 13:58:44 -0500, Elena Zannoni <ezannoni@redhat.com> said:

>> I am not sure if David committed it yet, but the patch it at the
>> bottom of:
>> http://sources.redhat.com/ml/gdb-patches/2002-12/msg00560.html
>> Maybe you already tested it!

> I committed it yesterday.

No, apparently, I didn't.  I just committed it now; but, as I said
here:

> Though I thought some more about this last night: I'm finally starting
> to understand the minsym stuff.  My current opinion is that deleting
> force_return is good, but maybe changing that lookup_symbol call
> within search_symbols wasn't the best idea.  I'll send something more
> specific on Monday.

there's one aspect of the patch that makes me uneasy.  So another
patch will soon be forthcoming.

David Carlton
carlton@math.stanford.edu

Re: [rfa] delete 'force_return' from lookup_symbol_aux_minsyms

[Michael Elizabeth Chastain]

David Carlton writes:
> I committed it yesterday.

Cool, I started another spin for my own purposes this morning.  If the
Perl gods smile on me it will finish tommorow.  I might as well go all
the way and write an analysis report for it.

In my dream world, developers add some "<Test-Request> ... </Test-Request>"
XML mumbo-jumbo to their gdb-patches mail, and 1000 gnutest@home
clients rip through the regression tests.

Michael C

Re: [rfa] delete 'force_return' from lookup_symbol_aux_minsyms

[Michael Elizabeth Chastain]

Elena Z wrote:

> I am not sure if David committed it yet, but the patch it at the
> bottom of:
> http://sources.redhat.com/ml/gdb-patches/2002-12/msg00560.html
> Maybe you already tested it!

I am having some problems analyzing this.  I see 3 regressions from
2002-12-18 to 2002-12-21, but I can't tell if they are gcc problems
or gdb problems.

I've got one set of runs with:

  gdb HEAD 2002-12-18
  gcc HEAD 2002-12-18

And another set of runs with:

  gdb HEAD 2002-12-21
  gcc HEAD 2002-12-21

It would be great if I kept the install directories from 2002-12-18
for a few days, but I already recycled the disk space.  :(  I do have
all the executable files from gdb/testsuite though, so I can try the
new gdb on all the executables built with gcc HEAD 2002-12-18.

For the curious, the 3 regressions are:

(1) gcc ICE when compiling gdb.c++/anon-union.exp with gcc HEAD 2002-12-21.
    and dwarf-2.  This is obviously a gcc problem and I will follow the
    gcc reporting procedures for it.  I think the problem is related to
    the special status of C++ "main", which must be return type int,
    but are not required to return a value (the compiler is required to
    synthesize a value if control falls off the end, which is special
    code in gcc, which has problems getting the debug information right).

(2) New problems near the end of gdb.c++/anon-union.exp with gcc HEAD
    and stabs+.  Probably gcc, might be gdb (unlikely).

(3) Problems with gdb.c++/casts.exp.

I'll go do some QA sleuthing and file bug reports and stuff.

Michael C

Re: [rfa] delete 'force_return' from lookup_symbol_aux_minsyms

[Michael Elizabeth Chastain]

More news ...

> I am not sure if David committed it yet, but the patch it at the
> bottom of:
> http://sources.redhat.com/ml/gdb-patches/2002-12/msg00560.html
> Maybe you already tested it!

After grubbing around in three unrelated regressions, I can say that
this patch is cool and not causing any visible regressions.  There are
no problems in gdb from 2002-12-18 to 2002-12-21.  gcc broke anonymous
unions though.

Next up is JimB's patch submission for gdb.c++/local.exp.

Michael C