On Thu, 2013-12-12 at 22:11 +0100, Jan Kratochvil wrote:
> > +/* Resolve a function descriptor if addr points into the .odp section.
> > + The .odp section contains function descriptors consisting of 3
> > addresses.
> > + function, toc and chain. We are just interested in the first.
> > +
> > http://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi-1.9.html#FUNC-DES
> > +
> > + Returns true if the given address could be resolved, false otherwise.
> > +*/
> > +bool
> > +ppc64_resolve_sym_value (Ebl *ebl, GElf_Addr *addr)
> > +{
> > + if (ebl->fd_data != NULL && *addr >= ebl->fd_addr
>
> > + && *addr < ebl->fd_addr + ebl->fd_data->d_size)
>
> && *addr + sizeof (Elf64_Addr) <= ebl->fd_addr +
> ebl->fd_data->d_size)
To make sure the whole address is inside. OK, changed.
> > +++ b/libdw/libdw.map
> > @@ -284,6 +284,8 @@ ELFUTILS_0.158 {
> > dwfl_thread_getframes;
> > dwfl_frame_pc;
> >
> > - dwfl_module_addrsym_elf;
> > - dwfl_module_getsym_elf;
> > + dwfl_module_addrinfo;
> > + dwfl_module_addrinfo_elf;
> > + dwfl_module_getsym_info;
> > + dwfl_module_getsym_info_elf;
>
> FYI my opinion is that there is no benefit to have the functions
> dwfl_module_addrinfo and dwfl_module_getsym_info. One can use just the *_elf
> ones.
I think you are right. less is more. The user can just pass in NULL for
the extra information. I removed all _elf variants and just made the
_info ones take all arguments.
> > +/* Fetch one entry from the module's symbol table and the associated
> > + address value. On errors, returns NULL. If successful, fills in
> > + *SYM, *ADDR and returns the string for st_name. This works like
> > + gelf_getsym. *ADDR is set to the st_value adjusted to an absolute
> > + value based on the module's location, when the symbol is in an
> > + SHF_ALLOC section. For non-ET_REL files, if the arch uses function
> > + descriptors, and the st_value points to one, the value will be
> > + resolved to the actual function address. Note that since symbols
> > + can come from either the main, debug or auxiliary ELF symbol file
> > + (either dynsym or symtab) and the st_value itself isn't adjusted in
> > + any way (this is different from dwfl_module_getsym). */
>
> The sentence is unfinished.
And grammatically wrong, there is also a dangling since. I have reworded
and merged the description with the now gone _elf variant.
> > +/* Same as dwfl_module_getsym_info but also returns the section index,
> > + the ELF file and bias. If SHNDXP is non-null, it's set with the
> > + section index (whether from st_shndx or extended index table); in
> > + case of a symbol in a non-allocated section, *SHNDXP is instead set
> > + to -1. Fills in BIAS, if not NULL, with the difference between
>
> It is uint32_t so maybe the doc could say s/-1/0xffffffff/. But maybe it was
> intentional.
Yes, it was intentional, that is how other functions that report an
error using an unsigned type also say. I think it is obvious it is
actually (GElf_Word) -1.
> > +/* Find the symbol associated with ADDRESS, return its name, the
> > + OFFSET that ADDRESS is from the start and optionally the originally
>
> I cannot parse the sentence.
> s/from the start/from the address of returned SYM/ ?
> s/originally// ?
I changed it to not be one giant sentence, but just small sentences
describing each of the arguments and the return value separately. And I
merged the description with the now gone _elf variant.
> > /* Find the symbol that ADDRESS lies inside, and return detailed
> > - information as for dwfl_module_getsym (above). */
> > + information as for dwfl_module_getsym (above). Note that unlike
> > + dwfl_module_addrname and dwfl_module_addrinfo this matches ADDRESS
> > + against the adjusted SYM->ST_VALUE directly, so might depending on
> > + whether the architecture uses indirections like function descriptors,
> > + not match against actual function addresses. */
>
> I cannot parse the sentence, I hope it is clear it is formed incorrectly.
> It would be good to know whether it finds function entry addresses and whether
> it finds function descriptor addresses.
Yeah, again I made the sentence too big. I chopped it up in smaller
sentences. And made it more explicit.
> > +/* Returns true if the value can be resolved to an address in an
> > + allocated section, which will be returned in *SHNDXP.
> > + (e.g. function descriptor resolving). */
> |
> probably an excessive dot ^
Removed.
> > +/* Returns true if the value can be resolved to an address in an
> > + allocated section, which will be returned in *ADDR
> > + (e.g. function descriptor resolving). */
> |
> probably an excessive dot ^
Removed.
Thanks,
Mark
