Overlay support broken (Re: [patch] [2/2] Discontiguous PSYMTABs (psymtabs->symtabs by addrmap))

[Pedro Alves]

A Tuesday 13 May 2008 22:12:20, Ulrich Weigand wrote:
> Pedro Alves wrote:
> > +  struct minimal_symbol *msym = NULL;
> > +  if (addr != ~(CORE_ADDR) 0)
> > +    /* If we have an address to lookup, use it.  */
> > +    msym = lookup_minimal_symbol_by_pc (addr);
> > +
> > +  if (!msym
> > +      || addr != SYMBOL_VALUE_ADDRESS (msym)
> > +      || strcmp (DEPRECATED_SYMBOL_NAME (msym), ginfo->name) != 0)
> > +    /* Try by looking up by name.  Not perfect, since it can match the
> > +       wrong symbol.  */
> > +    msym = lookup_minimal_symbol (ginfo->name, NULL, objfile);
>
> Hmm, I guess there is the possibility that even though there is
> a msymbol at ADDR with name NAME, both
>   lookup_minimal_symbol_by_pc (ADDR)
> and
>   lookup_minimal_symbol (NAME)
>
> might fail to find it ...   E.g. if there are minimal symbols
>   ADDR' NAME
>   ADDR  NAME'
>   ADDR  NAME
> lookup by pc might find NAME', but lookup by name might find ADDR'.
>
> Maybe we need a lookup_minimal_symbol_by_pc_name or so?
>

Ah, good point.  Looks like that would be the best practical
approach, yes.

> > +  /* We either have an OBJFILE, or we can get at it from the sym's
> > +     symtab.  Anything else is a bug.  */
> > +  gdb_assert (objfile || (sym->symtab && sym->symtab->objfile));
> > +
> > +  if (objfile == NULL)
> > +    objfile = sym->symtab->objfile;
>
> Huh.  If that's true, why does fixup_symbol_section even have an
> OBJFILE argument?  Is there ever a situation where we cannot use
> sym->symtab->objfile?
>

Yes, while still reading the debug info, you can get here with
a sym->symtab == NULL, but you'll have a valid objfile to pass
down.  The symtab is then set at the end of end_symtab.  E.g.:

(gdb) p sym->symtab
$2 = (struct symtab *) 0x0

(gdb) bt
#0  fixup_symbol_section (sym=0xb583e0, objfile=0xb336d0) 
at ../../src/gdb/symtab.c:1090
#1  0x0000000000552a1a in var_decode_location (attr=0xb51380, sym=0xb583e0, 
cu=0xb4c7a0)
    at ../../src/gdb/dwarf2read.c:7326
#2  0x00000000005530ce in new_symbol (die=0xb512f0, type=0x0, cu=0xb4c7a0) 
at ../../src/gdb/dwarf2read.c:7462
#3  0x00000000005491a7 in process_die (die=0xb512f0, cu=0xb4c7a0) 
at ../../src/gdb/dwarf2read.c:2777
#4  0x00000000005495a9 in read_file_scope (die=0xb4a630, cu=0xb4c7a0) 
at ../../src/gdb/dwarf2read.c:2895
#5  0x00000000005490c4 in process_die (die=0xb4a630, cu=0xb4c7a0) 
at ../../src/gdb/dwarf2read.c:2713
#6  0x0000000000548fcd in process_full_comp_unit (per_cu=0xb418d0) 
at ../../src/gdb/dwarf2read.c:2680
#7  0x0000000000548a3a in process_queue (objfile=0xb336d0) 
at ../../src/gdb/dwarf2read.c:2476
#8  0x0000000000548c56 in psymtab_to_symtab_1 (pst=0xb41cd0) 
at ../../src/gdb/dwarf2read.c:2556
...

That was reading the the symbol "main" while stopping at "main".

You can also get here with a NULL objfile, for example, by 
means of lookup_symbol_aux_block, when you pass
a NULL symtab output parameter around, fixup_symbol_section
is always called with a NULL objfile, but it's OK, since at
the time lookup_symbol* are called, the symbols should already
have a sym->symtab and a sym->symtab->objfile.

(I actually have no idea why that output *symtab arg is needed
in the lookup_symbol* functions, if a symbol has a symtab
pointer.  Legacy?)

> > +  switch (SYMBOL_CLASS (sym))
> > +    {
> > +    case LOC_UNRESOLVED:
> > +      addr = ~(CORE_ADDR) 0;
> > +      break;
>
> Why do we need to fixup the section for an LOC_UNRESOLVED symbol?
>
> I understand that every time we want to use the address of a
> LOC_UNRESOLVED, the user needs to look up the msymbol anyway.
> They should then use the section from the msymbol too, right?
>

Ah, you're right, from read_var_value:

    case LOC_UNRESOLVED:
      {
	struct minimal_symbol *msym;

	msym = lookup_minimal_symbol (DEPRECATED_SYMBOL_NAME (var), NULL, NULL);
	if (msym == NULL)
	  return 0;
	if (overlay_debugging)
	  addr = symbol_overlayed_address (SYMBOL_VALUE_ADDRESS (msym),
					   SYMBOL_BFD_SECTION (msym));
	else
	  addr = SYMBOL_VALUE_ADDRESS (msym);
      }
      break;


> > +  switch (SYMBOL_CLASS (psym))
> > +    {
> > +    case LOC_STATIC:
> > +    case LOC_BLOCK:
> > +      addr = SYMBOL_VALUE_ADDRESS (psym);
> > +      break;
> > +    default:
> > +      /* Nothing else will be listed in the minsyms -- no use looking
> > +	 it up.  */
> > +      return psym;
> > +    }
>
> Any reason for not supporting LOC_LABEL or LOC_INDIRECT for psymbols?
>
> (Well, except from the fact that apparently none of the symbol readers
> left in GDB will ever generate LOC_INDIRECT ...  But at least mdebugread.c
> will generate LOC_LABEL psymbols, it seems.)
>

But that comes from debug info.  I didn't think LOC_LABEL's would ever
be listed in the minsyms.  Can they?  There's certainly no harm in
adding it to the switch, though.
As for LOC_INDIRECT, I remember finding out no reader records it, and
meaning to garbage collect it instead, but defered that to
submission time.  :-/   If we want to keep it, it looks like, yes,
we should be fixing up its section too.

-- 
Pedro Alves