Max Filippov <[email protected]> writes:

> - drop gimme_arg and implement arch_fetch_* callbacks;

Cool.

> - add diagnostics to {get,set}_instruction_pointer, get_stack_pointer
>   and get_return_addr;
> - update CREDITS, NEWS and README.
>
> Changes RFC->PATCH:
> - adopt PLT unresolving algorithm used by PPC

Also cool.

> +int
> +arch_elf_init(struct ltelf *lte, struct library *lib)
> +{
> +     Elf_Scn *scn;
> +     GElf_Shdr shdr;
> +     GElf_Addr relplt_addr;
> +     GElf_Xword relplt_size;
> +     GElf_Phdr phdr;
> +     size_t i;
> +
> +     for (i = 0; gelf_getphdr(lte->elf, i, &phdr) != NULL; ++i) {
> +             if (phdr.p_type == PT_LOAD) {
> +                     lib->arch.loadable_sz =
> +                             phdr.p_vaddr + phdr.p_memsz - lte->bias;

Was this tested on a prelinked system?  It's confusing that you subtract
bias to get from ELF address to memory address.  Normally you would add
it.

Also, correct me if I'm wrong, but loadable_sz actually points to the
end of the region, and hence is not a size.  Would loadable_end be a
better name?

> +     if (elf_get_section_type(lte, SHT_DYNAMIC, &scn, &shdr) < 0
> +         || scn == NULL) {
> +     fail:
> +             error(0, 0, "Couldn't get SHT_DYNAMIC: %s",
> +                   elf_errmsg(-1));
> +             return -1;
> +     }
> +
> +     Elf_Data *data = elf_loaddata(scn, &shdr);
> +     if (data == NULL)
> +             goto fail;
> +
> +     for (i = 0; i < shdr.sh_size / shdr.sh_entsize; ++i) {
> +             GElf_Dyn dyn;
> +
> +             if (gelf_getdyn(data, i, &dyn) == NULL)
> +                     goto fail;
> +
> +             if (dyn.d_tag == DT_JMPREL) {
> +                     relplt_addr = dyn.d_un.d_ptr;
> +             } else if (dyn.d_tag == DT_PLTRELSZ) {
> +                     relplt_size = dyn.d_un.d_val;
> +             }
> +     }

Would calling elf_load_dynamic_entry twice be suitable instead of the
above block?

The two calls would end up looking up the section by type twice.  That
has negative performance implications.  I suspect they are negligible,
but should you care, would you be also willing to contribute a patch
that implements on-demand caching of SHT_DYNAMIC inside
elf_load_dynamic_entry?

> +     for (i = 1; i < lte->ehdr.e_shnum; ++i) {
> +             Elf_Scn *scn;
> +             GElf_Shdr shdr;
> +
> +             scn = elf_getscn(lte->elf, i);
> +             if (scn == NULL || gelf_getshdr(scn, &shdr) == NULL) {
> +                     fprintf(stderr, "Couldn't get section header: %s\n",
> +                             elf_errmsg(-1));
> +                     exit(EXIT_FAILURE);
> +             }
> +             if (shdr.sh_addr == relplt_addr
> +                 && shdr.sh_size == relplt_size) {

To get the section pointed to by DT_JMPREL, you could call
elf_get_section_covering and then check the size in the returned shdr.
My thinking is that if there is more than one section covering a given
address, the ELF is probably not structurally sound, and you can safely
bail out anyway.

FWIW, the ARM backend simply does this:

        GElf_Addr jmprel_addr;
        Elf_Scn *jmprel_sec;
        GElf_Shdr jmprel_shdr;
        if (elf_load_dynamic_entry(lte, DT_JMPREL, &jmprel_addr) < 0
            || elf_get_section_covering(lte, jmprel_addr,
                                        &jmprel_sec, &jmprel_shdr) < 0
            || jmprel_sec == NULL)
                return -1;

Other than these things, the code looks solid as far as I can tell (not
that I know anything about xtensa though ;) ).

Thanks,
Petr

_______________________________________________
Ltrace-devel mailing list
[email protected]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Reply via email to