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

[Edgar E. Iglesias]

On Thu, Sep 27, 2012 at 03:41:34PM +0200, Petr Machata wrote:
> 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?

Sure, I'll send a v3 with only the last 3 patches.

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

AFAIK, not yet. But It's something to look into. I'm not on a super new
toolchain here (GCC 4.3.1, binutils 2.18.50).


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

I don't have a strong opinion on style but it would help to have
a checkpatch script. Usually mixed code & var declarations is
discouraged but I don't mind it. Working on various projects with
different coding styles (QEMU, linux, toolchain etc) makes it hard
to naturally adapt to the various styles...

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

np at all! Thanks for reviewing.

Cheers