Re: [rfa] lookup_symbol_aux_minsym

[Elena Zannoni <ezannoni at redhat dot com>]

David Carlton writes:
 > On Mon, 11 Nov 2002 13:18:44 -0500, Elena Zannoni <ezannoni@redhat.com> said:
 > > David Carlton writes:
 > 
 > >> * 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?
 > 
 > I don't have an informed opinion about this.  It seems plausible to me
 > that, given that the msymbol is providing a value that fits as a
 > second argument to find_pc_sect_symtab, we might as well use it.  But
 > I'm quite willing to believe that the other option could be better.
 > 
 > I did just notice the following ChangeLog entry from the great HP
 > merge:
 > 
 > 	* symtab.c (lookup_symbol): Changed call to find_pc_sect_symtab,
 >  	to the original find_pc_symtab, in HP added fragment.
 > 
 > So maybe it would be safer to use find_pc_symtab.  I dunno.  It does
 > currently pass the testsuite on HPPA if you use find_pc_sect_symtab;
 > for all I know, the HP added fragment only differs in this situation
 > for historical reasons (i.e. because HP was working from a code base
 > before find_pc_sect_symtab existed).  I can easily imagine that the
 > people doing the merge were far too busy to try to figure out if it
 > differed for a particular HP-specific reason or not.
 > 

OK, I think we should use find_pc_sect_symtab after all.

 > >> * 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):
 > 
 > Yeah, me too: the first time I read this code, I completely missed
 > these issues, and upon reading what you have to say, I still wasn't as
 > careful as I could have been in thinking about the s != NULL, sym ==
 > NULL case.
 > 
 > > 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.
 > 
 > I agree with your analysis.
 > 
 > > 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.
 > 
 > Right, that seems clearly correct.
 > 
 > > 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.
 > 
 > I've gone through the code a few times, and as far as I can tell the
 > only callers that care about the symtab parameter are within
 > decode_line_1.  (I've been meaning to submit a patch to replace the
 > dummy parameters in other callers by NULL.)  And now, happily, I'm in
 > a position to be able to tell you what goes on there: the only place
 > where decode_line_1 actually uses sym_symtab is in the function
 > symbol_found that got moved out last week.  And that function, in
 > turn, is only called if sym != NULL.  So I can say with some degree of
 > confidence that, if lookup_symbol returns NULL, then it doesn't matter
 > what the value of its symtab argument gets set to.
 > 

Cool. This is good news. So, your pick.

 > > 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)
 > 
 > The reason why I'd rather not do that is because, once some version of
 > this patch is applied, I want to submit a patch to get rid of
 > force_return entirely.  The force_return stuff is only necessary in a
 > fairly specialized set of circumstances: it's only the case that
 > force_return != 0 but sym == NULL when a minsym without a
 > corresponding symbols turns up such that either:
 > 
 > * There's a symtab whose address is the address of the minsym,
 > 
 > or
 > 
 > * The minsym type isn't mst_text or mst_file_tex, where name is
 >   mangled, and where there isn't a corresponding symbol whether we
 >   look under the mangled name or the demangled name.
 > 

If you are planning to get rid of it soon, then ok. I was objecting
more from a stylistic point of view than anything else.

 > If it were something consistent, like "return NULL whenever a minimal
 > symbol is found without a symbol", then I would definitely want to
 > keep around force_return or its equivalent, and your way of doing
 > things would probably be better than mine.  But my guess is that it's
 > only accidental that GDB is doing things this way; I can't find
 > anything in the ChangeLogs that support the idea that force_return is
 > necessary.  Though there is the following comment in the code:
 > 
 >   /* 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:");
 >   */
 > 
 > I'm really not sure what to make of this comment: if it were the case
 > that lookup symbol returned NULL whenever a minimal symbol was found
 > without a symbol, this would make sense, but as it is I don't really
 > get that comment.




I found some info....

Here is the earliest version of the code I culd get my hands on (from 1991):

if (s)
  {
    bv = BLOCKVECTOR (s);
    block = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
    sym = lookup_block_symbol (block, misc_function_vector[ind].name,
			       namespace);
    /* sym == 0 if symbol was found in the misc_function_vector
       but not in the symtab.
       Return 0 to use the misc_function 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)
     *symtab = s;
     return sym;
  }

Not much changed except the new fixup_symbol_section (which still
returns null is sym is null).


>From the stabs manual:

@findex N_FUN, for functions
@findex N_FNAME
@findex N_STSYM, for functions (Sun acc)
@findex N_GSYM, for functions (Sun acc)
All of the following stabs normally use the @code{N_FUN} symbol type.
However, Sun's @code{acc} compiler on SunOS4 uses @code{N_GSYM} and
@code{N_STSYM}, which means that the value of the stab for the function
is useless and the debugger must get the address of the function from
the non-stab symbols instead.  On systems where non-stab symbols have
leading underscores, the stabs will lack underscores and the debugger
needs to know about the leading underscore to match up the stab and the
non-stab symbol.  BSD Fortran is said to use @code{N_FNAME} with the
same restriction; the value of the symbol is not useful (I'm not sure it
really does use this, because GDB doesn't handle this and no one has
complained).

I am not sure I understand it fully. Besides, this talks about
*leading* underscores. So it may be irrelevant.

I asked around (thanks to Jeff Knaggs), and I think the story for
Fortran goes like this:

Apparently, in older Fortrans, '_' was not part of the user namespace.
g77 attached a final '_' to procedure names as the exported symbols
for linkage (foo_) , but the symbols went in the debug info just like
'foo'. The rationale behind this is not completely clear, and maybe it
was done to other symbols as well, not just procedures.

So this would explain why there are msymbols that are not in the
symbol tables. And I think we should still keep those comments and
behavior in place.  I see that the HP version is a bit more verbose,
so it should be integrated. Maybe add the above explanation as well.

 > 
 > Jim Blandy also agrees with me that it would be good to get rid of
 > force_return, I think: see
 > <http://sources.redhat.com/ml/gdb/2002-11/msg00045.html>.
 > 
 > Anyways, that argument is for another patch, but that's the reason why
 > I wrote this patch the way I did.  I'll do it whichever way you want,
 > though.

Don't worry.

Approved, modulus the fortran, etc comments.

Sorry for the delay.
Elena


 > 
 > David Carlton
 > carlton@math.stanford.edu