Re: [rfa] lookup_symbol_aux_minsym

[David Carlton <carlton at math dot stanford dot edu>]

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.

>> * 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.

> 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 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.

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.

David Carlton
carlton@math.stanford.edu