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