This is the mail archive of the gdb-patches@sources.redhat.com mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [rfa] lookup_symbol_aux_minsym


David Carlton writes:
 > Here's the part of my previous lookup_symbol_aux patch where I combine
 > the two versions of the minsym code into a single function
 > lookup_symbol_aux_minsym.  I've tried to mimic the current flow of
 > control as exactly as possible: in particular, this code is run at a
 > different time in HPUXHPPA situations than in other situations, and I
 > preserved the weird circumstances in which the minsym check might lead
 > to a NULL return.  (I'm not sure either of those is a good idea, but
 > that's a matter for a separate patch.)
 > 
 > Here are the differences between the two versions of the minsym code
 > in the current version of lookup_symbol_aux.
 > 
 > * The comments differ, to some extent: I picked whichever version of
 >   the comment I liked more.
 > 
 > * The non-HP code has
 > 
 >     s = find_pc_sect_symtab (SYMBOL_VALUE_ADDRESS (msymbol),
 > 			     SYMBOL_BFD_SECTION (msymbol));
 > 
 >   where the HP code has
 > 
 >     s = find_pc_symtab (SYMBOL_VALUE_ADDRESS (msymbol));
 > 
 >   Both seem fine to me; I picked the former, but I don't have a strong
 >   feeling about this.
 > 

Some random comments....


I don't know about this. find_pc_symtab calls find_pc_sect_symtab with
NULL as section parameter, unless there are overlays involved.  I
don't know if it would make a difference, but it seems that it would
in case of overlays, because finding the section is a bit more involved.
Maybe we should adopt the other approach? I.e. use the 

s = find_pc_symtab (SYMBOL_VALUE_ADDRESS (msymbol));

What do you think?

 > * The non-HP code has
 > 
 >     if (symtab != NULL)
 >       *symtab = s;
 >     return fixup_symbol_section (sym, s->objfile);
 > 
 >   where the non-HP code has
 > 
 >     if (sym)
 >       {
 >         if (symtab != NULL)
 >           *symtab = s;
 >         return sym;
 >       }
 > 
 >   There are a few differences here.  The non-HP code does a
 >   fixup_symbol_section, which seems like a good idea.  The HP code
 >   sets *symtab to s only if both symtab and sym are non-NULL: that
 >   sounds like a good idea. 

The cases seem to be as follow (I am not trying to be pedantic, I am
having a hard time myself understanding this code):

s == null && sym == null
  non-HP--> fall through
  HP--> fall through

s == null && sym != null
  Can't happen. sym must be null as well because the previous searches have
  failed. (actually sym is uninitialized).

s != null && sym != null 
  non-HP--> search finished: *symtab set to s + returns fixed up sym
  HP--> search finished: *symtab set to s + returns sym


s != null && sym == null
  non-HP--> search finished: *symtab set to s + returns null sym
  HP--> fall through. This means that, because of the position 
        of the ifdef, *symtab set to null + returns null sym.


So, the behaviors differ for 3 and 4.  I think that for 3, we could
call fixup_section, which is called for all the non-hp cases.  And you
did that. For 4, I am not sure what behavior is more correct, set the
symtab to null or not? What do the callers expect?  Hmm, sadly enough,
almost none of the callers to lookup_symbol (which initiates all these
calls) seems to care about the symtab parameter at all. Almost all the
calls use NULL, and those that don't use a placeholder parameter. But
maybe I missed a few. Bottom line, I am not sure what is best to do.


 >   Finally, the HP code only returns sym if
 >   sym is non-NULL; that turns out not to make a difference since, if
 >   we're in the HP branch, this is right next to the end of
 >   lookup_symbol_aux, so we'll return NULL immediately anyways.
 > 
 > It seems to me that none of these differences should stand in the way
 > of merging the two code paths into a single function; here's a patch
 > which does that.
 > 
 > Joel has kindly tested this patch on an HP/UX machine, so I don't
 > think I screwed up anything in that case.

I don't particularly like the force_return thing. May I suggest to change
the signature of the new function to this:

static enum return_code (or similar)
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 **return_symbol)

and 

	      *force_return = 1;
	      return fixup_symbol_section (sym, s->objfile);

into

	      *return_symbol = fixup_symbol_section (sym, s->objfile);
	      return RETURN_NOW; (or something like that)


Elena



 > 
 > David Carlton
 > carlton@math.stanford.edu
 > 
 > 2002-11-05  David Carlton  <carlton@math.stanford.edu>
 > 
 > 	* symtab.c (lookup_symbol_aux): Move minsym code into a separate
 > 	function.
 > 	(lookup_symbol_aux_minsyms): New function.
 > 
 > Index: symtab.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/symtab.c,v
 > retrieving revision 1.77
 > diff -u -p -r1.77 symtab.c
 > --- symtab.c	5 Nov 2002 20:33:01 -0000	1.77
 > +++ symtab.c	5 Nov 2002 22:43:38 -0000
 > @@ -103,6 +103,14 @@ struct symbol *lookup_symbol_aux_psymtab
 >  					   const namespace_enum namespace,
 >  					   struct symtab **symtab);
 >  
 > +static
 > +struct symbol *lookup_symbol_aux_minsyms (const char *name,
 > +					  const char *mangled_name,
 > +					  const namespace_enum namespace,
 > +					  int *is_a_field_of_this,
 > +					  struct symtab **symtab,
 > +					  int *force_return);
 > +
 >  static struct symbol *find_active_alias (struct symbol *sym, CORE_ADDR addr);
 >  
 >  /* This flag is used in hppa-tdep.c, and set in hp-symtab-read.c */
 > @@ -786,9 +794,14 @@ lookup_symbol_aux (const char *name, con
 >  		   int *is_a_field_of_this, struct symtab **symtab)
 >  {
 >    struct symbol *sym;
 > -  struct symtab *s = NULL;
 > -  struct blockvector *bv;
 > -  struct minimal_symbol *msymbol;
 > +
 > +  /* 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.  */
 >  
 > @@ -875,65 +888,14 @@ 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.  */
 >  
 > -  if (namespace == VAR_NAMESPACE)
 > -    {
 > -      msymbol = lookup_minimal_symbol (name, NULL, NULL);
 > -      if (msymbol != NULL)
 > -	{
 > -	  s = find_pc_sect_symtab (SYMBOL_VALUE_ADDRESS (msymbol),
 > -				   SYMBOL_BFD_SECTION (msymbol));
 > -	  if (s != NULL)
 > -	    {
 > -	      /* This is a function which has a symtab for its address.  */
 > -	      bv = BLOCKVECTOR (s);
 > -	      block = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
 > -
 > -              /* This call used to pass `SYMBOL_NAME (msymbol)' as the
 > -                 `name' argument to lookup_block_symbol.  But the name
 > -                 of a minimal symbol is always mangled, so that seems
 > -                 to be clearly the wrong thing to pass as the
 > -                 unmangled name.  */
 > -	      sym = lookup_block_symbol (block, name, mangled_name, namespace);
 > -	      /* We kept static functions in minimal symbol table as well as
 > -	         in static scope. We want to find them in the symbol table. */
 > -	      if (!sym)
 > -		{
 > -		  block = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
 > -		  sym = lookup_block_symbol (block, name,
 > -                                             mangled_name, namespace);
 > -		}
 > -
 > -	      /* sym == 0 if symbol was found in the minimal symbol table
 > -	         but not in the symtab.
 > -	         Return 0 to use the msymbol definition of "foo_".
 > -
 > -	         This happens for Fortran  "foo_" symbols,
 > -	         which are "foo" in the symtab.
 > -
 > -	         This can also happen if "asm" is used to make a
 > -	         regular symbol but not a debugging symbol, e.g.
 > -	         asm(".globl _main");
 > -	         asm("_main:");
 > -	       */
 > +  force_return = 0;
 >  
 > -	      if (symtab != NULL)
 > -		*symtab = s;
 > -	      return fixup_symbol_section (sym, s->objfile);
 > -	    }
 > -	  else if (MSYMBOL_TYPE (msymbol) != mst_text
 > -		   && MSYMBOL_TYPE (msymbol) != mst_file_text
 > -		   && !STREQ (name, SYMBOL_NAME (msymbol)))
 > -	    {
 > -	      /* This is a mangled variable, look it up by its
 > -	         mangled name.  */
 > -	      return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name, NULL,
 > -					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. */
 > -	}
 > -    }
 > +  sym = lookup_symbol_aux_minsyms (name, mangled_name,
 > +				   namespace, is_a_field_of_this,
 > +				   symtab, &force_return);
 > +  
 > +  if (sym != NULL || force_return == 1)
 > +    return sym;
 >  
 >  #endif
 >  
 > @@ -975,87 +937,15 @@ lookup_symbol_aux (const char *name, con
 >       the static check in this case? 
 >     */
 >  
 > -  if (namespace == VAR_NAMESPACE)
 > -    {
 > -      msymbol = lookup_minimal_symbol (name, NULL, NULL);
 > -      if (msymbol != NULL)
 > -	{
 > -	  /* OK, we found a minimal symbol in spite of not
 > -	   * finding any symbol. There are various possible
 > -	   * explanations for this. One possibility is the symbol
 > -	   * exists in code not compiled -g. Another possibility
 > -	   * is that the 'psymtab' isn't doing its job.
 > -	   * A third possibility, related to #2, is that we were confused 
 > -	   * by name-mangling. For instance, maybe the psymtab isn't
 > -	   * doing its job because it only know about demangled
 > -	   * names, but we were given a mangled name...
 > -	   */
 > -
 > -	  /* We first use the address in the msymbol to try to
 > -	   * locate the appropriate symtab. Note that find_pc_symtab()
 > -	   * has a side-effect of doing psymtab-to-symtab expansion,
 > -	   * for the found symtab.
 > -	   */
 > -	  s = find_pc_symtab (SYMBOL_VALUE_ADDRESS (msymbol));
 > -	  if (s != NULL)
 > -	    {
 > -	      bv = BLOCKVECTOR (s);
 > -	      block = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
 > -              /* This call used to pass `SYMBOL_NAME (msymbol)' as the
 > -                 `name' argument to lookup_block_symbol.  But the name
 > -                 of a minimal symbol is always mangled, so that seems
 > -                 to be clearly the wrong thing to pass as the
 > -                 unmangled name.  */
 > -	      sym = lookup_block_symbol (block, name, mangled_name, namespace);
 > -	      /* We kept static functions in minimal symbol table as well as
 > -	         in static scope. We want to find them in the symbol table. */
 > -	      if (!sym)
 > -		{
 > -		  block = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
 > -		  sym = lookup_block_symbol (block, name,
 > -                                             mangled_name, namespace);
 > -		}
 > -	      /* If we found one, return it */
 > -	      if (sym)
 > -		{
 > -		  if (symtab != NULL)
 > -		    *symtab = s;
 > -		  return sym;
 > -		}
 > -
 > -	      /* If we get here with sym == 0, the symbol was 
 > -	         found in the minimal symbol table
 > -	         but not in the symtab.
 > -	         Fall through and return 0 to use the msymbol 
 > -	         definition of "foo_".
 > -	         (Note that outer code generally follows up a call
 > -	         to this routine with a call to lookup_minimal_symbol(),
 > -	         so a 0 return means we'll just flow into that other routine).
 >  
 > -	         This happens for Fortran  "foo_" symbols,
 > -	         which are "foo" in the symtab.
 > -
 > -	         This can also happen if "asm" is used to make a
 > -	         regular symbol but not a debugging symbol, e.g.
 > -	         asm(".globl _main");
 > -	         asm("_main:");
 > -	       */
 > -	    }
 > +  force_return = 0;
 >  
 > -	  /* If the lookup-by-address fails, try repeating the
 > -	   * entire lookup process with the symbol name from
 > -	   * the msymbol (if different from the original symbol name).
 > -	   */
 > -	  else if (MSYMBOL_TYPE (msymbol) != mst_text
 > -		   && MSYMBOL_TYPE (msymbol) != mst_file_text
 > -		   && !STREQ (name, SYMBOL_NAME (msymbol)))
 > -	    {
 > -	      return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name,
 > -					NULL, namespace, is_a_field_of_this,
 > -					symtab);
 > -	    }
 > -	}
 > -    }
 > +  sym = lookup_symbol_aux_minsyms (name, mangled_name,
 > +				   namespace, is_a_field_of_this,
 > +				   symtab, &force_return);
 > +  
 > +  if (sym != NULL || force_return == 1)
 > +    return sym;
 >  
 >  #endif
 >  
 > @@ -1198,6 +1088,104 @@ lookup_symbol_aux_psymtabs (int block_in
 >  	return fixup_symbol_section (sym, objfile);
 >        }
 >    }
 > +
 > +  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.  */
 > +
 > +static struct symbol *
 > +lookup_symbol_aux_minsyms (const char *name,
 > +			   const char *mangled_name,
 > +			   const namespace_enum namespace,
 > +			   int *is_a_field_of_this,
 > +			   struct symtab **symtab,
 > +			   int *force_return)
 > +{
 > +  struct symbol *sym;
 > +  struct blockvector *bv;
 > +  const struct block *block;
 > +  struct minimal_symbol *msymbol;
 > +  struct symtab *s;
 > +
 > +  if (namespace == VAR_NAMESPACE)
 > +    {
 > +      msymbol = lookup_minimal_symbol (name, NULL, NULL);
 > +
 > +      if (msymbol != NULL)
 > +	{
 > +	  /* OK, we found a minimal symbol in spite of not finding any
 > +	     symbol. There are various possible explanations for
 > +	     this. One possibility is the symbol exists in code not
 > +	     compiled -g. Another possibility is that the 'psymtab'
 > +	     isn't doing its job.  A third possibility, related to #2,
 > +	     is that we were confused by name-mangling. For instance,
 > +	     maybe the psymtab isn't doing its job because it only
 > +	     know about demangled names, but we were given a mangled
 > +	     name...  */
 > +
 > +	  /* We first use the address in the msymbol to try to locate
 > +	     the appropriate symtab. Note that find_pc_sect_symtab()
 > +	     has a side-effect of doing psymtab-to-symtab expansion,
 > +	     for the found symtab.  */
 > +	  s = find_pc_sect_symtab (SYMBOL_VALUE_ADDRESS (msymbol),
 > +				   SYMBOL_BFD_SECTION (msymbol));
 > +	  if (s != NULL)
 > +	    {
 > +	      /* This is a function which has a symtab for its address.  */
 > +	      bv = BLOCKVECTOR (s);
 > +	      block = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
 > +
 > +	      /* This call used to pass `SYMBOL_NAME (msymbol)' as the
 > +	         `name' argument to lookup_block_symbol.  But the name
 > +	         of a minimal symbol is always mangled, so that seems
 > +	         to be clearly the wrong thing to pass as the
 > +	         unmangled name.  */
 > +	      sym =
 > +		lookup_block_symbol (block, name, mangled_name, namespace);
 > +	      /* We kept static functions in minimal symbol table as well as
 > +	         in static scope. We want to find them in the symbol table. */
 > +	      if (!sym)
 > +		{
 > +		  block = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
 > +		  sym = lookup_block_symbol (block, name,
 > +					     mangled_name, namespace);
 > +		}
 > +
 > +	      /* sym == 0 if symbol was found in the minimal symbol table
 > +	         but not in the symtab.
 > +	         Return 0 to use the msymbol definition of "foo_".
 > +
 > +	         This happens for Fortran  "foo_" symbols,
 > +	         which are "foo" in the symtab.
 > +
 > +	         This can also happen if "asm" is used to make a
 > +	         regular symbol but not a debugging symbol, e.g.
 > +	         asm(".globl _main");
 > +	         asm("_main:");
 > +	       */
 > +
 > +	      if (symtab != NULL && sym != NULL)
 > +		*symtab = s;
 > +	      *force_return = 1;
 > +	      return fixup_symbol_section (sym, s->objfile);
 > +	    }
 > +	  else if (MSYMBOL_TYPE (msymbol) != mst_text
 > +		   && MSYMBOL_TYPE (msymbol) != mst_file_text
 > +		   && !STREQ (name, SYMBOL_NAME (msymbol)))
 > +	    {
 > +	      /* This is a mangled variable, look it up by its
 > +	         mangled name.  */
 > +	      *force_return = 1;
 > +	      return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name,
 > +					NULL, namespace, is_a_field_of_this,
 > +					symtab);
 > +	    }
 > +	}
 > +    }
 >  
 >    return NULL;
 >  }


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