Re: [patch] Resolve ppc64 func descriptors as .func (via .opd)

[Mark Wielaard <mjw at redhat dot com>]

On Fri, 2012-12-07 at 13:39 +0100, Jan Kratochvil wrote:
> On Thu, 06 Dec 2012 16:35:24 +0100, Mark Wielaard wrote:
> > Like how you would use in a backtrace (it matches the
> > "DWARF way" of doing backtraces in gdb).
> 
> "DWARF way" is irrelevant here.  elfutils currently do not provide any such
> high level functionality of decoding DWARF.  Let's keep talking about ELF.

libdwfl provides both. I was just pointing out that in this case,
associating addresses to code names even on ppc64 are done without any
dot prefixing.

> > >   The facts are:
> > > 
> > > * Any current caller of dwfl_module_addrsym wants to call the "new" function
> > >   providing even the synthetic symbols.
> 
> 'addr2line -S'.
> 
> 
> > > * There is no use/caller of non-synthetic dwfl_module_addrsym.
> > 
> > Could you give examples of these?
> 
> See above.  I do not know well other elfutils applications besides
> elfutils/src/*.c.

OK, eu-addr2line -S is a nice example that we would indeed like to
handle on all arches when possible. So the code that does this looks
like:

static void
print_addrsym (Dwfl_Module *mod, GElf_Addr addr)
{
  GElf_Sym s;
  GElf_Word shndx;
  const char *name = dwfl_module_addrsym (mod, addr, &s, &shndx);
  if (name == NULL)
    {
      /* No symbol name.  Get a section name instead.  */
      int i = dwfl_module_relocate_address (mod, &addr);
      if (i >= 0)
        name = dwfl_module_relocation_info (mod, i, NULL);
      if (name == NULL)
        puts ("??");
      else
        printf ("(%s)+%#" PRIx64 "\n", name, addr);
    }
  else if (addr == s.st_value)
    puts (name);
  else
    printf ("%s+%#" PRIx64 "\n", name, addr - s.st_value);
}

So, it currently doesn't work on architectures that have st_value point
into a indirection table, like odp on ppc64. Then dwfl_module_addrsym ()
either doesn't match anything, or it matches something not really in the
neighborhood of the requested address. Providing the backend an
opportunity to match and set the st_value through indirection seems a
good idea here.

> > > > but just return the data from the symbol table that
> > > 
> > > Why?  The caller wants more than just the data from the symbol table.
> > 
> > OK, then I am confused about your use case. Could you give an example of
> > how you want to use this function?
> 
> ../src/addr2line -S -e testfile66 0x250
> from jankratochvil/ppc64-opd .

Could you be a bit more specific?

> > > > The current dwfl_module_addrname () and dwfl_module_addrsym () return
> > > > the name of that symbol and that seems as good a name as any to return,
> > > > so we should not tweak it. Nor should we tweak any other symbol values.
> > > 
> > > Why?  The caller wants them tweaked.
> > 
> > Because that is the contract of these functions, it seems we disagree on
> > that then?
> 
> Existing callers of dwfl_module_addrsym want to get the address 0x250
> resolved.  As it is not possible with in-file ELF symtab the returned ELF
> symbol needs to be tweaked some way.

Could you give an example that you think isn't covered by giving the
backend an opportunity to match and tweak st_value?

> > > > I don't agree with you that we should generate some synthetic symbols,
> > > > names, etc, that is just not what these functions do.
> > > 
> > > It is not my idea, it is a requirement of existing callers, such as addr2line,
> > > my backtracer and in fact also any other caller.
> > 
> > I think the name of the symbol as is is fine for these callers.
> 
> Skipping this as off-topic for the first part of the discussion what should
> dwfl_module_addrsym do except for the returned name.

OK, I don't want to see the name changed at all, so if we can resolve it
without discussing the name change that is fine with me.

> > > > Except for adjusting the st_value as we already do that is just
> > > > too painful and messy IMHO.
> > > 
> > > One could find adjusting st_value also messy in such case, bias could be
> > > applied by the caller.
> > 
> > But it is what is done now.
> 
> When we are designing new API and from the current discussion it seems to me
> we must not change saint dwfl_module_addrsym no matter what it does then the
> new API can do anything we want, if you want it just for pure in-file ELF
> symbols then even st_value must not be changed.

We are discussing just fixing dwfl_module_addrsym () so it works on all
architectures don't we? We aren't discussing designing new API again are
we? Since the current dwfl_module_addrsym () just doesn't work on
architectures that use st_value to point to an indirect function
description I wouldn't worry too much about it. Of course on
architectures where everything already works fine we should keep things
as they are.

> But then the new API becomes only a very simple (and primarily accelerated)
> interface to dwfl_module_getsymtab and dwfl_module_getsym.  I do not think
> anyone asks for that functionality, dwfl_module_getsymtab and
> dwfl_module_getsym are fine on their own for the callers needing them.

Yes, I think dwfl_module_getsymtab () and dwfl_module_getsym () are fine
as is since they don't do address matching on st_value. Sadly I don't
think we can really help the user there.

> > You said you liked me to be explicit and provide a
> > specification of what I would like to see implemented. That was this:
> > 
> > > So to be explicit. If we are using the current interfaces
> > > dwfl_module_addrname () and by extension dwfl_module_addrsym () for
> > > returning a symbol name associated with an address then I think we
> > > should be as conservative as possible. So that means not fabricating a
> > > "synthetic" symbol, but just return the data from the symbol table that
> > > we have at hand, where only the st_value is adjusted as documented for
> > > the module load address to match the given address. To make that work
> > > for architectures that use function descriptors add a backend hook that
> > > translates the st_value when appropriate if the original value doesn't
> > > match directly. And that IMHO is really all that is needed.
> > 
> > Everything else was just to explain why I think other options are not
> > appropriate. The above IMHO provides the simplest semantics for these
> > functions that are consistent with how they work now and give meaningful
> > results when used with architectures that happen to use function
> > descriptors in the symbol table.
> 
> OK, so I hope you agree now with the original patch post except the name it
> returns.

If that implements the specification as I wrote above then we agree.
Please post it again so we can finally review the implementation
details.

> If we agree on that part we can move to the separate second phase of
> discussion, about the name.

The name should just be as it is.

Thanks,

Mark