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

[Petr Machata]

I missed a couple more nits the first time around.  Patches 1-5 are
already in my tree, can you please fix and resend just the remaining
three?

edgar.iglesias at gmail.com writes:
> --- a/proc.c
> +++ b/proc.c
> @@ -637,6 +637,19 @@ breakpoint_for_symbol(struct library_symbol *libsym, void *data)

[...]

> +	if (!bp_addr && libsym->plt_type == LS_TOPLT_GOTONLY) {

This should be bp_addr == 0.

> --- a/sysdeps/linux-gnu/mipsel/arch.h
> +++ b/sysdeps/linux-gnu/mipsel/arch.h

[...]

> +#define ARCH_HAVE_LIBRARY_SYMBOL_DATA
> +enum mips_plt_type
> +{
> +	UNRESOLVED,
> +	RESOLVED,

This should be MIPS_PLT_*, or MIPS_*.  Those are globally visible
symbols, and bare {UN,}RESOLVED is too prone to collision.

> +	name = lte->dynstr + sym->st_name;
> +	if (ELF64_ST_TYPE(sym->st_info) != STT_FUNC) {
> +		debug(2, "sym %s not a function", name);
> +		return -1;
> +	}

This reminds me... does MIPS use IFUNC symbols?  libc and libm are
likely users if there are any at all.  (They show as GNU_IFUNC in
eu-readelf output.)  We don't support IFUNC in ltrace currently, but I
should be able to get to this problem soonish.

> +static enum callback_status
> +cb_each_sym(struct library_symbol *libsym, void *data)

While I'm being pedantic, I would prefer this to be called differently,
like [cb_]enable_breakpoint_sym or some such.

> +static enum callback_status
> +cb_each_lib(struct Process *proc,

... and this [cb_]enable_breakpoints_lib or similar.

> +				 struct library *lib, void *data)

The indentation is wrong here.  You may be able to fold this into
previous line, depending on what the renamed identifier will be.

> +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));

You don't need separate libsym declaration with initialization up there.
Just make it:

+	struct library_symbol *libsym = malloc(sizeof(*libsym));

Personally, I'd also move bp_addr and name declaration down to where
it's initialized (you only goto fail after name is initialized), but I
don't really care either way (and your way is formally correct per the
Linux coding style, which we seem to gravitate towards).

> +	if (libsym == NULL)
> +		return plt_fail;
> +
> +	GElf_Addr addr = arch_plt_sym_val(lte, sym_index, 0);
> +
> +	name = strdup(a_name);

+	if (name == NULL) {
+		fprintf(stderr, "%s: failed %s(%#llx): %s\n", __func__,
+			name, addr, strerror(errno));
+		goto fail;
+	}

(Or something like that, plus appropriate includes.)

> +	/* XXX The double cast should be removed when
> +	 * arch_addr_t becomes integral type.  */
> +	if (library_symbol_init(libsym,
> +				(arch_addr_t) (uintptr_t) addr,
> +				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;

Hmm, this should have a cast-comment similar to the above.  Something
like /* XXX This cast should be removed when arch_addr_t becomes
integral type.  keywords: double cast.  */

I'm sorry for not noticing these the first time around.

Thanks,
PM