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