Re: [rfa] delete 'force_return' from lookup_symbol_aux_minsyms

[David Carlton <carlton@math.stanford.edu>]

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;
+		      }
 		  }
 	      }
 	  }