[Ltrace-devel] [PATCH v2] xtensa: add xtensa support

Max Filippov jcmvbkbc at gmail.com
Sat Jan 3 01:36:25 UTC 2015


Hi Petr,

On Fri, Jan 2, 2015 at 4:32 AM, Petr Machata <pmachata at redhat.com> wrote:
>> +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

No, it wasn't tested on a prelinked system, since AFAIK xtensa is not
supported by the prelink.

> 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?

The intention was to calculate the size of loadable part of a library, to be
able to tell later, given library base address and a pointer, if it
points inside
the library or outside of it.

>> +     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?

Ok.

> 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.

Ok.

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

Thanks for the review. I'll post an updated version.

-- 
Thanks.
-- Max



More information about the Ltrace-devel mailing list