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

[Petr Machata]

Max Filippov <jcmvbkbc at gmail.com> 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