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?
[email protected] 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 _______________________________________________ Ltrace-devel mailing list [email protected] http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel
