[Ltrace-devel] [PATCH 6/8] mipsel: Add mips specific symbol info loading

[Petr Machata]

edgar.iglesias at gmail.com writes:
> @@ -131,6 +163,9 @@ arch_elf_init(struct ltelf *lte, struct library *lib)
>  		}
>  	}
>  
> +	/* Tell the generic code how many dynamic trace:able symbols
> +	 * we've got.  */
> +	lte->relplt_count = lte->dynsym_count - lte->arch.mips_gotsym;

Hmm, I'm not too fond of backend code overwriting core values like that,
but I guess it's OK.  It's in the right hook anyway.  It's not worth it
to build a separate hooklet for this.

>  	return 0;
>  }
>  
> @@ -139,4 +174,128 @@ arch_elf_destroy(struct ltelf *lte)
>  {
>  }
>  
> +enum callback_status cb_each_sym(struct library_symbol *libsym, void *data)

This should be static.  cb_each_sym should start a new line.

> +{
> +	struct Process *proc = data;
> +	struct breakpoint *bp;
> +
> +	if (libsym->plt_type != LS_TOPLT_GOTONLY)
> +		return CBS_CONT;
> +
> +	/* Update state.  */
> +	libsym->arch.resolved_addr = (uintptr_t) sym2addr(proc, libsym);
> +
> +	if (!libsym->arch.resolved_addr)

Use libsym->arch.resolved_addr == 0.  It's an address, not a boolean.  I
know this idiom is occasionally used in ltrace, but I can't stand it.

> +		/* FIXME: What does this mean?  */
> +		return CBS_CONT;
> +
> +	libsym->arch.type = RESOLVED;
> +
> +	/* Add breakpoint.  */
> +	bp = malloc(sizeof *bp);
> +	if (!bp

Use bp == NULL.

> +	    || breakpoint_init(bp, proc,
> +			       (void *) (uintptr_t) libsym->arch.resolved_addr,

The double cast shouldn't be necessary here.  uintptr_t already is
void*, and when we convert it to integral, breakpoint_init will be
adjusted as well.

> +			       libsym) < 0) {
> +		goto fail;
> +	}
> +
> +	if (proc_add_breakpoint(proc, bp) < 0) {
> +		breakpoint_destroy(bp);
> +		goto fail;
> +	}
> +
> +	if (breakpoint_turn_on(bp, proc) < 0) {
> +		proc_remove_breakpoint(proc, bp);
> +		breakpoint_destroy(bp);
> +		goto fail;
> +	}
> +
> +	return CBS_CONT;
> +fail:
> +	free(bp);
> +	fprintf(stderr, "Failed to add breakpoint for %s\n", libsym->name);
> +	return CBS_FAIL;
> +}
> +
> +enum callback_status cb_each_lib(struct Process *proc,
> +				 struct library *lib, void *data)

Static, newline.

> +{
> +	struct library_symbol *libsym = NULL;
> +	while ((libsym = library_each_symbol(lib, libsym,
> +					     cb_each_sym, proc)) != NULL) {

Hmm, above you could just return CBS_CONT instead of CBS_FAIL, and
change this into a simple call.

> +	}
> +	return CBS_CONT;
> +}
> +
> +void arch_dynlink_done(struct Process *proc)
> +{
> +	proc_each_library(proc, NULL, cb_each_lib, NULL);
> +}
> +
> +enum plt_status
> +arch_elf_add_plt_entry(struct Process *proc, struct ltelf *lte,
> +                       const char *a_name, GElf_Rela *rela, size_t ndx,
> +                       struct library_symbol **ret)
> +{
> +	struct library_symbol *libsym = NULL;
> +	arch_addr_t bp_addr;
> +	char *name = NULL;
> +	int sym_index = ndx + lte->arch.mips_gotsym;
> +
> +	libsym = malloc(sizeof(*libsym));
> +	if (!libsym)
> +		return plt_fail;

if (libsym == NULL)

> +
> +	GElf_Addr addr = arch_plt_sym_val(lte, sym_index, 0);
> +
> +	name = strdup(a_name);
> +	if (library_symbol_init(libsym,
> +				(arch_addr_t) (uintptr_t) addr,

Please add a comment like this:

		/* XXX The double cast should be removed when
		 * arch_addr_t becomes integral type.  */

When I get around to it, I'll grep for "double cast" to find individual
cases.

> +				name, 1, LS_TOPLT_EXEC) < 0) {
> +		fprintf(stderr, "%s: failed %s : %llx\n", __func__, name, addr);
> +		goto fail;
> +	}
> +
> +	bp_addr = sym2addr(proc, libsym);
> +	libsym->arch.stub_addr = (uintptr_t) bp_addr;
> +
> +	if (!bp_addr) {
> +		/* Function pointers without PLT entries.  */
> +		libsym->plt_type = LS_TOPLT_GOTONLY;
> +		libsym->arch.type = UNRESOLVED;
> +	}

if (bp_addr == 0)

With these nits fixed, I'll apply this patch.

Thanks,
PM