[rfa] more lookup_symbol_aux_minsyms futzing

[rfa] more lookup_symbol_aux_minsyms futzing

[David Carlton]

Here's another patch involving lookup_symbol_aux_minsyms.  It reverts
the search_symbols part of my last patch, and deletes the last "else
if" part of lookup_symbol_aux_minsyms.  (And it deletes the
'is_a_field_of_this' argument, since that's was only used in that
"else if" part.)

I'd been ascribing more power to lookup_symbol_aux_minsyms than it
actually had: I had assumed that it did a reasonable job of finding a
symbol corresponding to a minimal symbol whenever there was one, but
now I think that it only does that in the function case.  Furthermore,
it seems to me that whatever it tries to do in the non-function case
is more likely to be hazardous than helpful.  (Or at least was more
likely before 'force_return' got deleted.)

This has two implications:

1) Don't count on lookup_symbol_aux_minsyms doing anything useful in
   the non-function case.  In particular, my search_symbols patch from
   before was bad.

2) Get rid of the non-function case of lookup_symbol_aux_minsyms: it
   just confuses the issue and slows down symbol lookup.

Let me analyze lookup_symbol_aux_minsyms in a bit more detail, to back
this up.  It looks like this, stripped down:

  if (namespace == VAR_NAMESPACE)
    {
      msymbol = lookup_minimal_symbol (name, NULL, NULL);

      if (msymbol != NULL)
        {
          s = find_pc_sect_symtab (SYMBOL_VALUE_ADDRESS (msymbol),
                                   SYMBOL_BFD_SECTION (msymbol));
          if (s != NULL)
            {
               /* Find the correct symbol, and return it. */
            }
          else if (MSYMBOL_TYPE (msymbol) != mst_text
                   && MSYMBOL_TYPE (msymbol) != mst_file_text
                   && !STREQ (name, SYMBOL_NAME (msymbol)))
            {
              /* This is a mangled variable, look it up by its
                 mangled name.  */
              return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name,
                                        NULL, namespace, is_a_field_of_this,
                                        symtab);
            }
        }
    }

  return NULL;

Now, find_pc_sect_symtab only works if the symbol in question
corresponds to a function, loosely speaking: it finds the right
minimal symbol (which will be 'msymbol' above) and then immediately
returns NULL if that symbol doesn't have type mst_text or
mst_file_text.  So the whole bit that I've marked by "Find the correct
symbol, and return it" only happens if we're looking for a function.

So what happens in the "else if" case?  This checks that we're in the
non-function case, and that the name we were passed isn't the
SYMBOL_NAME of msymbol, which means that msymbol has a mangled name
while we were passed the demangled name.  In this case, it calles
lookup_symbol_aux with a mangled name as the 'name' argument.  So what
happens in this nested call to lookup_symbol_aux?

1) lookup_symbol_aux_local.  This doesn't happen because 'block' is
   NULL.

2) The 'is_a_field_of_this' check.  It makes no sense to do this now:
   we already did it once!  So we should have passed NULL in place of
   'is_a_field_of_this': oops.

3) The static block check; again, 'block' is NULL, so this doesn't
   happen.

4) The 'lookup_symbol_aux_symtabs' check.  The symtab search
   eventually comes down to calling lookup_block_symbol a bunch of
   times, with the mangled name as its 'name' argument.  But, as Jim
   Blandy noted in
   <http://sources.redhat.com/ml/gdb-patches/2002-10/msg00040.html>,
   doing that makes no sense: it won't find a match in the hash
   table.  So that's not useful.

5) The 'lookup_symbol_aux_minsyms' check.  That's not going to uncover
   anything the second time that it didn't uncover the first time it
   was called.

6) The 'lookup_symbol_aux_psymtabs' check.  This tries to find the
   right partial symbol table, read in the corresponding symbol table,
   and then search it with lookup_block_symbol.  But, again, calling
   lookup_block_symbol with the mangled name as the 'name' argument
   makes no sense.

So in no case is it correct for lookup_symbol_aux_minsyms to call
lookup_symbol_aux with the demangled name as the 'name' argument.  So
I think we should just delete that call.

There is one thing that bothers me: right now, partial symbols don't
include demangled names.  So that call to lookup_symbol_aux_psymtabs
with the demangled name might actually cause the correct symtab to be
read in, even if the lookup_block_symbol fails.  The correct thing to
do here is to store demangled names in partial symbols, however, not
to depend on an undocumented side effect of an incorrect psymtab
search.  I'll submit a patch for that next.

David Carlton
carlton@math.stanford.edu

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

	* symtab.c (search_symbols): Change call to
	lookup_symbol_aux_minsyms back to cal to lookup_symbol, reverting
	previous patch.
	(lookup_symbol_aux_minsyms): Don't search on mangled names; delete
	'is_a_field_of_this' argument.
	(lookup_symbol_aux): Don't pass 'is_a_field_of_this' to
	lookup_symbol_aux_minsyms.

Index: symtab.c
===================================================================
RCS file: /cvs/src/src/gdb/symtab.c,v
retrieving revision 1.83
diff -u -p -r1.83 symtab.c
--- symtab.c	23 Dec 2002 16:43:18 -0000	1.83
+++ symtab.c	23 Dec 2002 18:03:03 -0000
@@ -116,7 +116,6 @@ static
 struct symbol *lookup_symbol_aux_minsyms (const char *name,
 					  const char *mangled_name,
 					  const namespace_enum namespace,
-					  int *is_a_field_of_this,
 					  struct symtab **symtab);
 
 static struct symbol *find_active_alias (struct symbol *sym, CORE_ADDR addr);
@@ -923,8 +922,7 @@ lookup_symbol_aux (const char *name, con
      Eventually, all global symbols might be resolved in this way.  */
 
   sym = lookup_symbol_aux_minsyms (name, mangled_name,
-				   namespace, is_a_field_of_this,
-				   symtab);
+				   namespace, symtab);
   
   if (sym != NULL)
     return sym;
@@ -971,8 +969,7 @@ lookup_symbol_aux (const char *name, con
 
 
   sym = lookup_symbol_aux_minsyms (name, mangled_name,
-				   namespace, is_a_field_of_this,
-				   symtab);
+				   namespace, symtab);
   
   if (sym != NULL)
     return sym;
@@ -1154,24 +1151,20 @@ lookup_symbol_aux_psymtabs (int block_in
   return NULL;
 }
 
-/* Check for the possibility of the symbol being a function or a
-   mangled variable that is stored in one of the minimal symbol
-   tables.  Eventually, all global symbols might be resolved in this
-   way.  */
-
-/* NOTE: carlton/2002-12-05: At one point, this function was part of
-   lookup_symbol_aux, and what are now 'return' statements within
-   lookup_symbol_aux_minsyms returned from lookup_symbol_aux, even if
-   sym was NULL.  As far as I can tell, this was basically accidental;
-   it didn't happen every time that msymbol was non-NULL, but only if
-   some additional conditions held as well, and it caused problems
-   with HP-generated symbol tables.  */
+/* Check for the possibility of the symbol being a function that is
+   stored in one of the minimal symbol tables.  */
+
+/* NOTE: carlton/2002-12-23: At one point, when this function was part
+   of lookup_symbol_aux, its behavior was different, for reasons that
+   are unclear: it forced a return from lookup_symbol_aux at times
+   even without checking the partial symbol tables, and it tried to do
+   something strange involving mangled names in the non-function
+   case.  */
 
 static struct symbol *
 lookup_symbol_aux_minsyms (const char *name,
 			   const char *mangled_name,
 			   const namespace_enum namespace,
-			   int *is_a_field_of_this,
 			   struct symtab **symtab)
 {
   struct symbol *sym;
@@ -1186,20 +1179,15 @@ lookup_symbol_aux_minsyms (const char *n
 
       if (msymbol != NULL)
 	{
-	  /* OK, we found a minimal symbol in spite of not finding any
-	     symbol. There are various possible explanations for
-	     this. One possibility is the symbol exists in code not
-	     compiled -g. Another possibility is that the 'psymtab'
-	     isn't doing its job.  A third possibility, related to #2,
-	     is that we were confused by name-mangling. For instance,
-	     maybe the psymtab isn't doing its job because it only
-	     know about demangled names, but we were given a mangled
-	     name...  */
-
-	  /* We first use the address in the msymbol to try to locate
-	     the appropriate symtab. Note that find_pc_sect_symtab()
-	     has a side-effect of doing psymtab-to-symtab expansion,
-	     for the found symtab.  */
+	  /* We found a minimal symbol in spite of not finding a
+	     symbol.  This probably means that the symbol in question
+	     is in a partial symbol table that hasn't been loaded, but
+	     it may mean that it's from code compiled without -g.
+	     Partial symbol table searches are kind of expensive; if
+	     the symbol corresponds to a function, we can find the
+	     appropriate symtab more quickly by calling
+	     find_pc_sect_symtab, so let's try that.  */
+
 	  s = find_pc_sect_symtab (SYMBOL_VALUE_ADDRESS (msymbol),
 				   SYMBOL_BFD_SECTION (msymbol));
 	  if (s != NULL)
@@ -1267,16 +1255,6 @@ lookup_symbol_aux_minsyms (const char *n
 		*symtab = s;
 	      return fixup_symbol_section (sym, s->objfile);
 	    }
-	  else if (MSYMBOL_TYPE (msymbol) != mst_text
-		   && MSYMBOL_TYPE (msymbol) != mst_file_text
-		   && !STREQ (name, SYMBOL_NAME (msymbol)))
-	    {
-	      /* This is a mangled variable, look it up by its
-	         mangled name.  */
-	      return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name,
-					NULL, namespace, is_a_field_of_this,
-					symtab);
-	    }
 	}
     }
 
@@ -2896,31 +2874,12 @@ search_symbols (char *regexp, namespace_
 	      {
 		if (0 == find_pc_symtab (SYMBOL_VALUE_ADDRESS (msymbol)))
 		  {
-		    if (kind == FUNCTIONS_NAMESPACE)
-		      {
-			found_misc = 1;
-		      }
-		    else
-		      {
-			struct symbol *sym;
-
-			if (SYMBOL_DEMANGLED_NAME (msymbol) != NULL)
-			  sym
-			    = lookup_symbol_aux_minsyms (SYMBOL_DEMANGLED_NAME
-							 (msymbol),
-							 SYMBOL_NAME (msymbol),
-							 VAR_NAMESPACE,
-							 NULL, NULL);
-			else
-			  sym
-			    = lookup_symbol_aux_minsyms (SYMBOL_NAME (msymbol),
-							 NULL,
-							 VAR_NAMESPACE,
-							 NULL, NULL);
-
-			if (sym == NULL)
-			  found_misc = 1;
-		      }
+		    if (kind == FUNCTIONS_NAMESPACE
+			|| lookup_symbol (SYMBOL_NAME (msymbol),
+					  (struct block *) NULL,
+					  VAR_NAMESPACE,
+					0, (struct symtab **) NULL) == NULL)
+		      found_misc = 1;
 		  }
 	      }
 	  }

Re: [rfa] more lookup_symbol_aux_minsyms futzing

[Jason Molenda]

Hi David,

On Mon, Dec 23, 2002 at 10:21:12AM -0800, David Carlton wrote:
> Here's another patch involving lookup_symbol_aux_minsyms.  

We (Apple) shipped our latest devtools release based on the Octoberish
gdb top-of-tree and I spent a good bit of today tracking over this
same exactly section of code for a customer's problem.  I was going
to send out a note once I understood how it'd all happened, but it
sounds exactly like what you describe here.

It's been a good while since I've looked at this code, and my notes
from today's debugging are all at work, so please excuse any especially
dumb misstatements. :)  

lookup_symbol_aux is called with a C++ unmanged+mangled name,
lookup_symbol_aux_symtabs doesn't get a match because we've only
read in the psymtab at this point; lookup_symbol_aux_minsyms calls
lookup_symbol_aux with the mangled+mangled names as a last resort,
and something lame happened from there on (but I don't remember
exactly...) -- and eventually we call lookup_symbol_aux_psymtabs
with the mangled+mangled names -- lookup_partial_symbol gets passed
a mangled name, says there is a match because psymtabs have only
mangled names.  We parse in the symtab, then we do lookup_block_symbol
with the mangled name, but the lookup_block_symbol hash of the mangled
name doesn't match that of the unmangled name, so we issue the error

              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",

It also seemed incorrect for lookup_symbol_aux to call
lookup_symbol_aux_minsyms before lookup_symbol_aux_psymtabs, but 
I'm guessing this is done on purpose based on this comment in your
patch -

> +	  /* We found a minimal symbol in spite of not finding a
> +	     symbol.  This probably means that the symbol in question
> +	     is in a partial symbol table that hasn't been loaded, but
> +	     it may mean that it's from code compiled without -g.
> +	     Partial symbol table searches are kind of expensive; if
> +	     the symbol corresponds to a function, we can find the
> +	     appropriate symtab more quickly by calling
> +	     find_pc_sect_symtab, so let's try that.  */
> +


The test case I have from the customer is a nice little thing but
it pulls in a big gop of Apple system libraries so I don't know
how easy it'd be to make a portable version.  At the very least,
I'll be looking at this tomorrow and should know better how we can
fix it up.

For what it's worth, everyone at Apple has been really excited by
the work you've been doing, David.  Anyone willing to wade into
decode_line_1 and do something other than increase entropy has my
respect. :-)

Jason

Re: [rfa] more lookup_symbol_aux_minsyms futzing

[David Carlton]

On Mon, 23 Dec 2002 22:59:39 -0800, Jason Molenda <jason-swarelist@molenda.com> said:

> lookup_symbol_aux is called with a C++ unmanged+mangled name,
> lookup_symbol_aux_symtabs doesn't get a match because we've only
> read in the psymtab at this point; lookup_symbol_aux_minsyms calls
> lookup_symbol_aux with the mangled+mangled names as a last resort,
> and something lame happened from there on (but I don't remember
> exactly...) -- and eventually we call lookup_symbol_aux_psymtabs
> with the mangled+mangled names -- lookup_partial_symbol gets passed
> a mangled name, says there is a match because psymtabs have only
> mangled names.  We parse in the symtab, then we do lookup_block_symbol
> with the mangled name, but the lookup_block_symbol hash of the mangled
> name doesn't match that of the unmangled name, so we issue the error

>               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",

That sounds like an entirely plausible scenario to me.  I didn't spend
the time coming up with a test case triggering that code path (it's
really tedious to write test cases for partial symbol tests,
at least if they interact with hashing like this), but that code path
is there, and it shouldn't be.

> It also seemed incorrect for lookup_symbol_aux to call
> lookup_symbol_aux_minsyms before lookup_symbol_aux_psymtabs, but 
> I'm guessing this is done on purpose based on this comment in your
> patch -

>> +	  /* We found a minimal symbol in spite of not finding a
>> +	     symbol.  This probably means that the symbol in question
>> +	     is in a partial symbol table that hasn't been loaded, but
>> +	     it may mean that it's from code compiled without -g.
>> +	     Partial symbol table searches are kind of expensive; if
>> +	     the symbol corresponds to a function, we can find the
>> +	     appropriate symtab more quickly by calling
>> +	     find_pc_sect_symtab, so let's try that.  */
>> +

Keep in mind while reading that comment that it's purely a guess
trying to justify existing code years after it was written.  (Or
really, after it's been accreting layers of not-entirely-compatible
changes for years.)  I think it's a fairly well-informed guess; on the
other hand, when writing the comment, my frame of mind was more
"assuming that there's a reason for the code to be done this way, what
is that reason" rather than "should the code _really_ be done this
way?"

I do think that it's reasonable for the code to have a minimal symbol
check before the partial symbol check - the partial symbol check
doesn't use the fastest algorithm in the world, so why not speed it up
if it's easy to do so?  On the other hand, if I were writing the code
from scratch, I'd probably omit the minsym check entirely; if
optimization proved desirable, I'd rather tackle that issue head-on.
I don't think I'd move the minsym check after the psymtab check: it
seems to me that if the psymtab check succeeds but the minsym check
fails then that's a bug in the partial symbols, and I'd rather know
about such bugs than have them papered over like that.  (I can't think
of any situations where the minsym check should succeed but the psymtab
check should fail, modulo issues like (lack of) mangling in the
psymtabs.)

> The test case I have from the customer is a nice little thing but it
> pulls in a big gop of Apple system libraries so I don't know how
> easy it'd be to make a portable version.  At the very least, I'll be
> looking at this tomorrow and should know better how we can fix it
> up.

With luck, my patch in the message you're replying to and my other
patch from yesterday demangling partial symbol names will be more or
less what you're looking for.  Let me know how it turns out!

David Carlton
carlton@math.stanford.edu

Re: [rfa] more lookup_symbol_aux_minsyms futzing

[Jason Molenda]

On Tue, Dec 24, 2002 at 09:53:53AM -0800, David Carlton wrote:

> With luck, my patch in the message you're replying to and my other
> patch from yesterday demangling partial symbol names will be more or
> less what you're looking for.  Let me know how it turns out!

The patch to demangle the psymtabs fixes the problem, thanks!  
FWIW I think the symtab.c::lookup_symbol_aux_minsyms change also
makes sense, but it doesn't have any affect on this particular
failing case.

Jason

Re: [rfa] more lookup_symbol_aux_minsyms futzing

[Elena Zannoni]

A few comments

David Carlton writes:
 > Here's another patch involving lookup_symbol_aux_minsyms.  It reverts
 > the search_symbols part of my last patch, and deletes the last "else
 > if" part of lookup_symbol_aux_minsyms.  (And it deletes the
 > 'is_a_field_of_this' argument, since that's was only used in that
 > "else if" part.)
 > 
 > I'd been ascribing more power to lookup_symbol_aux_minsyms than it
 > actually had: I had assumed that it did a reasonable job of finding a
 > symbol corresponding to a minimal symbol whenever there was one, but
 > now I think that it only does that in the function case.  Furthermore,
 > it seems to me that whatever it tries to do in the non-function case
 > is more likely to be hazardous than helpful.  (Or at least was more
 > likely before 'force_return' got deleted.)
 > 
 > This has two implications:
 > 
 > 1) Don't count on lookup_symbol_aux_minsyms doing anything useful in
 >    the non-function case.  In particular, my search_symbols patch from
 >    before was bad.
 > 
 > 2) Get rid of the non-function case of lookup_symbol_aux_minsyms: it
 >    just confuses the issue and slows down symbol lookup.
 > 
 > Let me analyze lookup_symbol_aux_minsyms in a bit more detail, to back
 > this up.  It looks like this, stripped down:
 > 
 >   if (namespace == VAR_NAMESPACE)
 >     {
 >       msymbol = lookup_minimal_symbol (name, NULL, NULL);
 > 
 >       if (msymbol != NULL)
 >         {
 >           s = find_pc_sect_symtab (SYMBOL_VALUE_ADDRESS (msymbol),
 >                                    SYMBOL_BFD_SECTION (msymbol));
 >           if (s != NULL)
 >             {
 >                /* Find the correct symbol, and return it. */
 >             }
 >           else if (MSYMBOL_TYPE (msymbol) != mst_text
 >                    && MSYMBOL_TYPE (msymbol) != mst_file_text
 >                    && !STREQ (name, SYMBOL_NAME (msymbol)))
 >             {
 >               /* This is a mangled variable, look it up by its
 >                  mangled name.  */
 >               return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name,
 >                                         NULL, namespace, is_a_field_of_this,
 >                                         symtab);
 >             }
 >         }
 >     }
 > 
 >   return NULL;
 > 
 > Now, find_pc_sect_symtab only works if the symbol in question
 > corresponds to a function, loosely speaking: it finds the right
 > minimal symbol (which will be 'msymbol' above) and then immediately
 > returns NULL if that symbol doesn't have type mst_text or
 > mst_file_text.  So the whole bit that I've marked by "Find the correct
 > symbol, and return it" only happens if we're looking for a function.
 > 


I think this deserves a clearer comment. It took me a while to refigure
this out.  Also, I suggest changing the name of the function to
reflect this. lookup_symbol_aux_func?

 > So what happens in the "else if" case?  This checks that we're in the
 > non-function case, and that the name we were passed isn't the
 > SYMBOL_NAME of msymbol, which means that msymbol has a mangled name
 > while we were passed the demangled name.  In this case, it calles
 > lookup_symbol_aux with a mangled name as the 'name' argument.  So what
 > happens in this nested call to lookup_symbol_aux?
 > 
 > 1) lookup_symbol_aux_local.  This doesn't happen because 'block' is
 >    NULL.

Micro optimization: should we conditionalize the call on 'block', and
initialize 'static_block' to NULL. Just like the call to
lookup_symbol_aux_block is.

 > 
 > 2) The 'is_a_field_of_this' check.  It makes no sense to do this now:
 >    we already did it once!  So we should have passed NULL in place of
 >    'is_a_field_of_this': oops.
 > 

however, because of the oddities in this recursive call, in the first
invocation the is_a_field_of_this check was done using 'name' (ie the
demangled name), while now we redo it using SYMBOL_NAME(msymbol) which
would be the mangled one. does it matter?

 > 3) The static block check; again, 'block' is NULL, so this doesn't
 >    happen.
 > 

ok

 > 4) The 'lookup_symbol_aux_symtabs' check.  The symtab search
 >    eventually comes down to calling lookup_block_symbol a bunch of
 >    times, with the mangled name as its 'name' argument.  But, as Jim
 >    Blandy noted in
 >    <http://sources.redhat.com/ml/gdb-patches/2002-10/msg00040.html>,
 >    doing that makes no sense: it won't find a match in the hash
 >    table.  So that's not useful.
 > 

what happened to that patch? did it ever get committed? I wonder if it
would have helped in Jason's case.

 > 5) The 'lookup_symbol_aux_minsyms' check.  That's not going to uncover
 >    anything the second time that it didn't uncover the first time it
 >    was called.
 > 
 > 6) The 'lookup_symbol_aux_psymtabs' check.  This tries to find the
 >    right partial symbol table, read in the corresponding symbol table,
 >    and then search it with lookup_block_symbol.  But, again, calling
 >    lookup_block_symbol with the mangled name as the 'name' argument
 >    makes no sense.
 >

right, in this invocation of lookup_symbol_aux the two parameters name
and mangled_name are the same.
 
 > So in no case is it correct for lookup_symbol_aux_minsyms to call
 > lookup_symbol_aux with the demangled name as the 'name' argument.  So
 > I think we should just delete that call.
 > 
 > There is one thing that bothers me: right now, partial symbols don't
 > include demangled names.  So that call to lookup_symbol_aux_psymtabs
 > with the demangled name might actually cause the correct symtab to be
 > read in, even if the lookup_block_symbol fails.  The correct thing to
 > do here is to store demangled names in partial symbols, however, not
 > to depend on an undocumented side effect of an incorrect psymtab
 > search.  I'll submit a patch for that next.
 > 
 > David Carlton
 > carlton@math.stanford.edu
 > 
 > 2002-12-23  David Carlton  <carlton@math.stanford.edu>
 > 
 > 	* symtab.c (search_symbols): Change call to
 > 	lookup_symbol_aux_minsyms back to cal to lookup_symbol, reverting
 > 	previous patch.
 > 	(lookup_symbol_aux_minsyms): Don't search on mangled names; delete
 > 	'is_a_field_of_this' argument.
 > 	(lookup_symbol_aux): Don't pass 'is_a_field_of_this' to
 > 	lookup_symbol_aux_minsyms.
 > 
 > Index: symtab.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/symtab.c,v
 > retrieving revision 1.83
 > diff -u -p -r1.83 symtab.c
 > --- symtab.c	23 Dec 2002 16:43:18 -0000	1.83
 > +++ symtab.c	23 Dec 2002 18:03:03 -0000
 > @@ -116,7 +116,6 @@ static
 >  struct symbol *lookup_symbol_aux_minsyms (const char *name,
 >  					  const char *mangled_name,
 >  					  const namespace_enum namespace,
 > -					  int *is_a_field_of_this,
 >  					  struct symtab **symtab);
 >  
 >  static struct symbol *find_active_alias (struct symbol *sym, CORE_ADDR addr);
 > @@ -923,8 +922,7 @@ lookup_symbol_aux (const char *name, con
 >       Eventually, all global symbols might be resolved in this way.  */
 >  
 >    sym = lookup_symbol_aux_minsyms (name, mangled_name,
 > -				   namespace, is_a_field_of_this,
 > -				   symtab);
 > +				   namespace, symtab);
 >    
 >    if (sym != NULL)
 >      return sym;
 > @@ -971,8 +969,7 @@ lookup_symbol_aux (const char *name, con
 >  
 >  
 >    sym = lookup_symbol_aux_minsyms (name, mangled_name,
 > -				   namespace, is_a_field_of_this,
 > -				   symtab);
 > +				   namespace, symtab);
 >    
 >    if (sym != NULL)
 >      return sym;
 > @@ -1154,24 +1151,20 @@ lookup_symbol_aux_psymtabs (int block_in
 >    return NULL;
 >  }
 >  
 > -/* Check for the possibility of the symbol being a function or a
 > -   mangled variable that is stored in one of the minimal symbol
 > -   tables.  Eventually, all global symbols might be resolved in this
 > -   way.  */
 > -
 > -/* NOTE: carlton/2002-12-05: At one point, this function was part of
 > -   lookup_symbol_aux, and what are now 'return' statements within
 > -   lookup_symbol_aux_minsyms returned from lookup_symbol_aux, even if
 > -   sym was NULL.  As far as I can tell, this was basically accidental;
 > -   it didn't happen every time that msymbol was non-NULL, but only if
 > -   some additional conditions held as well, and it caused problems
 > -   with HP-generated symbol tables.  */
 > +/* Check for the possibility of the symbol being a function that is
 > +   stored in one of the minimal symbol tables.  */
 > +
 > +/* NOTE: carlton/2002-12-23: At one point, when this function was part
 > +   of lookup_symbol_aux, its behavior was different, for reasons that
 > +   are unclear: it forced a return from lookup_symbol_aux at times
 > +   even without checking the partial symbol tables, and it tried to do
 > +   something strange involving mangled names in the non-function
 > +   case.  */
 >  
 >  static struct symbol *
 >  lookup_symbol_aux_minsyms (const char *name,
 >  			   const char *mangled_name,
 >  			   const namespace_enum namespace,
 > -			   int *is_a_field_of_this,
 >  			   struct symtab **symtab)
 >  {
 >    struct symbol *sym;
 > @@ -1186,20 +1179,15 @@ lookup_symbol_aux_minsyms (const char *n
 >  
 >        if (msymbol != NULL)
 >  	{
 > -	  /* OK, we found a minimal symbol in spite of not finding any
 > -	     symbol. There are various possible explanations for
 > -	     this. One possibility is the symbol exists in code not
 > -	     compiled -g. Another possibility is that the 'psymtab'
 > -	     isn't doing its job.  A third possibility, related to #2,
 > -	     is that we were confused by name-mangling. For instance,
 > -	     maybe the psymtab isn't doing its job because it only
 > -	     know about demangled names, but we were given a mangled
 > -	     name...  */
 > -
 > -	  /* We first use the address in the msymbol to try to locate
 > -	     the appropriate symtab. Note that find_pc_sect_symtab()
 > -	     has a side-effect of doing psymtab-to-symtab expansion,
 > -	     for the found symtab.  */
 > +	  /* We found a minimal symbol in spite of not finding a
 > +	     symbol.  This probably means that the symbol in question
 > +	     is in a partial symbol table that hasn't been loaded, but
 > +	     it may mean that it's from code compiled without -g.
 > +	     Partial symbol table searches are kind of expensive; if
 > +	     the symbol corresponds to a function, we can find the
 > +	     appropriate symtab more quickly by calling
 > +	     find_pc_sect_symtab, so let's try that.  */
 > +
 >  	  s = find_pc_sect_symtab (SYMBOL_VALUE_ADDRESS (msymbol),
 >  				   SYMBOL_BFD_SECTION (msymbol));
 >  	  if (s != NULL)
 > @@ -1267,16 +1255,6 @@ lookup_symbol_aux_minsyms (const char *n
 >  		*symtab = s;
 >  	      return fixup_symbol_section (sym, s->objfile);
 >  	    }
 > -	  else if (MSYMBOL_TYPE (msymbol) != mst_text
 > -		   && MSYMBOL_TYPE (msymbol) != mst_file_text
 > -		   && !STREQ (name, SYMBOL_NAME (msymbol)))
 > -	    {
 > -	      /* This is a mangled variable, look it up by its
 > -	         mangled name.  */
 > -	      return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name,
 > -					NULL, namespace, is_a_field_of_this,
 > -					symtab);
 > -	    }
 >  	}
 >      }
 >  
 > @@ -2896,31 +2874,12 @@ search_symbols (char *regexp, namespace_
 >  	      {
 >  		if (0 == find_pc_symtab (SYMBOL_VALUE_ADDRESS (msymbol)))
 >  		  {
 > -		    if (kind == FUNCTIONS_NAMESPACE)
 > -		      {
 > -			found_misc = 1;
 > -		      }
 > -		    else
 > -		      {
 > -			struct symbol *sym;
 > -
 > -			if (SYMBOL_DEMANGLED_NAME (msymbol) != NULL)
 > -			  sym
 > -			    = lookup_symbol_aux_minsyms (SYMBOL_DEMANGLED_NAME
 > -							 (msymbol),
 > -							 SYMBOL_NAME (msymbol),
 > -							 VAR_NAMESPACE,
 > -							 NULL, NULL);
 > -			else
 > -			  sym
 > -			    = lookup_symbol_aux_minsyms (SYMBOL_NAME (msymbol),
 > -							 NULL,
 > -							 VAR_NAMESPACE,
 > -							 NULL, NULL);
 > -
 > -			if (sym == NULL)
 > -			  found_misc = 1;
 > -		      }
 > +		    if (kind == FUNCTIONS_NAMESPACE
 > +			|| lookup_symbol (SYMBOL_NAME (msymbol),
 > +					  (struct block *) NULL,
 > +					  VAR_NAMESPACE,
 > +					0, (struct symtab **) NULL) == NULL)
 > +		      found_misc = 1;
 >  		  }
 >  	      }
 >  	  }

Re: [rfa] more lookup_symbol_aux_minsyms futzing

[David Carlton]

On Thu, 20 Feb 2003 15:08:19 -0500, Elena Zannoni <ezannoni@redhat.com> said:

> A few comments

Ack! I was considering this patch to be withdrawn, and I hadn't
noticed that it got added to GNATS.  Right now, all I'm proposing is
the search_symbols part of this patch (with a comment added), as in PR
symtab/1049.  (Incidentally, I'm curious if Daniel considers the patch
in symtab/1070 to still be active: if so, please review the e-mail
discussion that Daniel and I had about this.  I actually have more to
say about that if you're considering approving that patch.)

Having said that, I'm glad that you're thinking about this, because I
am soon planning to propose a similar (or maybe identical) patch to
modify lookup_symbol_aux_minsyms.  So here's a discussion of facts
relevant to a future version of this patch.


At the time that I proposed this patch originally, I didn't understand
the history of symbols and partial symbols very well.  At the time I
proposed this patch, I had assumed that partial symbol were supposed
to be demangled, and that the fact that they weren't was a bug.  But
then Peter Schauer kindly explained more of the history of partial
symbols and demangling; I have a _much_ clearer picture of the
situation now, and I believe I understand pretty much all of the
details of lookup_symbol_aux.

For a long time, partial symbols were intentionally left mangled,
because demangling partial symbols posed an unacceptable time cost
(and a less important but nontrivial memory cost).  This led to a
problem in lookup_symbol: lookup_symbol has to get passed a demangled
name (after all, it usually derives from something the user types),
but the symbol that it's supposed to find might be in a psymtab that
hasn't yet been converted to a symtab, in which case we need access to
the mangled name to find it.  So how can we find that symbol?

The solution is this: if a symbol's mangled name and demangled name
are different, then there must be a minimal symbol associated to it.
(After all, name mangling is only done in order to make the linker
happy: and that's exactly where minimal symbols come into play.)  And,
while we weren't demangling names of partial symbols, we were
demangling names of minimal symbols.  So the logic (once we get to the
"look for global symbols part" of lookup_symbol_aux) is:

1) Look in symtabs.  Demangled names are okay here.

2) Now we want to look in psymtabs, but we can't: we have a demangled
   name, and we need a mangled name to look in symtabs.  The solution
   then is:

3) Use the demangled name to find a minimal symbol, if we can.  If we
   do find a minimal symbol, then we might be able to find the symtab
   directly (if the symbol is associated to a function), or we might
   need to get the mangled name from the minimal symbol.

4) Once we have the mangled name from the minimal symbol, we can
   proceed with partial symbol lookup; we do this via a recursive call
   to lookup_symbol_aux.

5) If we didn't find a minimal symbol in step 3, then maybe we didn't
   need a mangled name; in that case, just proceed along to the
   partial symbol check, using the demangled name.

In particular, I'm now confident that the role for minimal symbols is
to do demangled->mangled name translation; any other use of minimal
symbols is either an optimization or an unintended side effect.  Of
course, there weren't comments actually _explaining_ this, and the
resulting control flow depended on some unclear and subtle
assumptions.  It broke code in the HP case; and everything broke when
it was no longer possible to pass lookup_symbol_aux a mangled name
instead of a demangled name.


But now partial symbols store demangled names in a pleasantly
efficient way, thanks to Daniel's recent patch.  Which means that the
whole raison d'etre of using minimal symbols in lookup_symbol_aux is
no more.  This has two consequences:

1) The second part of lookup_symbol_aux_minsyms should definitely go.
   It's caused problems in the past, it's causing problems right now,
   and it's entirely superfluous: whatever consequences it might have
   now are entirely negative ones.

2) The rest of lookup_symbol_aux_minsyms is unnecessary.  It may be an
   optimization; it may be a pessimization.  It's certainly a
   pessimization in some circumstances; given that it complicates the
   function in question, we should get rid of it unless we can find
   situations where it makes a real difference to the good.  My guess
   is that there are no such situations, but that's only a guess; I
   have some ideas about how to test this, so I'll do some timings and
   come back with a recommendation.


So if, for now, you could just give PR symtab/1049 a scan and approve
it (it shouldn't be too controversial, I hope), I'd appreciate it.
And think about the above, just to make sure that I'm not sounding
crazy.  Once it's approved (and once I have a spare moment!), I'll
then submit two patches:

1) A patch to correct a slight bug that remains in
   lookup_partial_symbol.  Basically, partial symbols are sorted via
   strcmp but we want to use strcmp_iw as our matching criterion;
   strcmp and strcmp_iw aren't _quite_ suitable to be used together in
   this way.

2) A patch to either get rid of the second half of
   lookup_symbol_aux_minsyms or to get rid of all of it, depending on
   whether or not I can find a situation where it helps to leave it in
   place.

David Carlton
carlton@math.stanford.edu

Re: [rfa] more lookup_symbol_aux_minsyms futzing

[Daniel Jacobowitz]

On Thu, Feb 20, 2003 at 12:57:37PM -0800, David Carlton wrote:
> On Thu, 20 Feb 2003 15:08:19 -0500, Elena Zannoni <ezannoni@redhat.com> said:
> 
> > A few comments
> 
> Ack! I was considering this patch to be withdrawn, and I hadn't
> noticed that it got added to GNATS.  Right now, all I'm proposing is
> the search_symbols part of this patch (with a comment added), as in PR
> symtab/1049.  (Incidentally, I'm curious if Daniel considers the patch
> in symtab/1070 to still be active: if so, please review the e-mail
> discussion that Daniel and I had about this.  I actually have more to
> say about that if you're considering approving that patch.)

Yes, I never withdrew it; that's why I forwarded it to GNATS.

> 1) A patch to correct a slight bug that remains in
>    lookup_partial_symbol.  Basically, partial symbols are sorted via
>    strcmp but we want to use strcmp_iw as our matching criterion;
>    strcmp and strcmp_iw aren't _quite_ suitable to be used together in
>    this way.

Should they be sorted via strcmp_iw instead?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [rfa] more lookup_symbol_aux_minsyms futzing

[David Carlton]

On Thu, 20 Feb 2003 16:16:42 -0500, Daniel Jacobowitz <drow@mvista.com> said:
> On Thu, Feb 20, 2003 at 12:57:37PM -0800, David Carlton wrote:

>> (Incidentally, I'm curious if Daniel considers the patch in
>> symtab/1070 to still be active: if so, please review the e-mail
>> discussion that Daniel and I had about this.  I actually have more
>> to say about that if you're considering approving that patch.)

> Yes, I never withdrew it; that's why I forwarded it to GNATS.

Ah, I wasn't sure if you'd forwarded it or if Andrew and his nefarious
gang of shell scripts had forwarded it.

>> 1) A patch to correct a slight bug that remains in
>> lookup_partial_symbol.  Basically, partial symbols are sorted via
>> strcmp but we want to use strcmp_iw as our matching criterion;
>> strcmp and strcmp_iw aren't _quite_ suitable to be used together in
>> this way.

> Should they be sorted via strcmp_iw instead?

That would be the obvious thing to do; unfortunately, strcmp_iw
doesn't define an ordering.  (It defines an asymmetric equality
relationship, but doesn't give you a way to order names that aren't
strcmp_iw.)  So you need to come up with an ordering function that can
be passed to qsort that is compatible with strcmp_iw's quirks.

Not too hard; I've done it, I just need to submit it as a patch.
It'll happen as soon as I've submitted my first namespace patch, no
later than Monday.

(And, as you said elsewhere, we should just hash partial symbols, like
we do with everything else, but that takes more work; eventually, one
of us will find some time to do it.)

David Carlton
carlton@math.stanford.edu