On Mon, Oct 01, 2012 at 10:59:33AM +0200, Petr Machata wrote: > "Edgar E. Iglesias" <[email protected]> writes: > > The patches look good. I've tried to replace the TOPLT_GOTONLY approach > > used by MIPS with delayed syms and it works nicely. > > Great, thanks. I'll merge after I get your MIPS optimizations in. > > The patch looks fine content-wise, but what you sent seems like output > of git log -p. I don't think there is a straightforward way to apply > these (but feel free to educate me). It seems like passing > --pretty=email would work, or, obviously, using git format-patch > instead.
Yes, my bad. It was meant as an inlined RFC in response to the thread and I didn't think too much about the format. But lets try the following (rebased ontop of the mips patches from today and your -l patchset). Can you apply with git am --scissors? -- >8 -- Subject: [PATCH] mipsel: Replace LS_TOPLT_GOTONLY with delayed syms Signed-off-by: Edgar E. Iglesias <[email protected]> --- library.h | 1 - proc.c | 17 ++--------------- sysdeps/linux-gnu/mipsel/arch.h | 3 +++ sysdeps/linux-gnu/mipsel/plt.c | 38 +++++++++++++------------------------- 4 files changed, 18 insertions(+), 41 deletions(-) diff --git a/library.h b/library.h index cea9156..eb986ea 100644 --- a/library.h +++ b/library.h @@ -31,7 +31,6 @@ struct library; enum toplt { LS_TOPLT_NONE = 0, /* PLT not used for this symbol. */ - LS_TOPLT_GOTONLY, /* Has a GOT entry but no PLT. */ LS_TOPLT_EXEC, /* PLT for this symbol is executable. */ }; diff --git a/proc.c b/proc.c index 222e6b4..3dab1e2 100644 --- a/proc.c +++ b/proc.c @@ -635,25 +635,12 @@ breakpoint_for_symbol(struct library_symbol *libsym, struct Process *proc) arch_addr_t bp_addr; assert(proc->leader == proc); - bp_addr = sym2addr(proc, libsym); - - /* For external function pointers, MIPS brings in stub-less funcs - * that point to zero at startup. These symbols get resolved by - * the dynamic linker and are ready to use at arch_dynlink_done(). - * - * Allow the backend to add these into the process representation - * but don't put breakpoints at this point. Let the backend fix that - * up later. - * - * XXX This should be changed to delayed symbols. */ - if (bp_addr == 0 && libsym->plt_type == LS_TOPLT_GOTONLY) { - /* Don't add breakpoints yet. */ - return CBS_CONT; - } /* Don't enable latent or delayed symbols. */ if (libsym->latent || libsym->delayed) return 0; + bp_addr = sym2addr(proc, libsym); + /* If there is an artificial breakpoint on the same address, * its libsym will be NULL, and we can smuggle our libsym * there. That artificial breakpoint is there presumably for diff --git a/sysdeps/linux-gnu/mipsel/arch.h b/sysdeps/linux-gnu/mipsel/arch.h index 5f5a7dd..e9a8962 100644 --- a/sysdeps/linux-gnu/mipsel/arch.h +++ b/sysdeps/linux-gnu/mipsel/arch.h @@ -56,6 +56,9 @@ struct arch_library_symbol_data { enum mips_plt_type type; GElf_Addr resolved_addr; GElf_Addr stub_addr; + + /* Set for FUNCs that have GOT entries but not PLT entries. */ + int gotonly : 1; }; #endif /* LTRACE_MIPS_ARCH_H */ diff --git a/sysdeps/linux-gnu/mipsel/plt.c b/sysdeps/linux-gnu/mipsel/plt.c index 7513c28..6684867 100644 --- a/sysdeps/linux-gnu/mipsel/plt.c +++ b/sysdeps/linux-gnu/mipsel/plt.c @@ -70,7 +70,7 @@ void * sym2addr(Process *proc, struct library_symbol *sym) { long ret; - if (sym->plt_type == LS_TOPLT_NONE) { + if (!sym->arch.gotonly && sym->plt_type == LS_TOPLT_NONE) { return sym->enter_addr; } @@ -225,10 +225,9 @@ static enum callback_status cb_enable_breakpoint_sym(struct library_symbol *libsym, void *data) { struct Process *proc = data; - struct breakpoint *bp; arch_addr_t bp_addr; - if (libsym->plt_type != LS_TOPLT_GOTONLY) + if (!libsym->arch.gotonly) return CBS_CONT; /* Update state. */ @@ -243,28 +242,11 @@ cb_enable_breakpoint_sym(struct library_symbol *libsym, void *data) libsym->arch.type = MIPS_PLT_RESOLVED; - /* Add breakpoint. */ - bp = malloc(sizeof *bp); - if (bp == NULL - || breakpoint_init(bp, proc, bp_addr, 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; + /* Now, activate the symbol causing a breakpoint to be added. */ + if (proc_activate_delayed_symbol(proc, libsym) < 0) { + fprintf(stderr, "Failed to activate delayed sym %s\n", + libsym->name); } - - return CBS_CONT; -fail: - free(bp); - fprintf(stderr, "Failed to add breakpoint for %s\n", libsym->name); return CBS_CONT; } @@ -317,8 +299,13 @@ arch_elf_add_plt_entry(struct Process *proc, struct ltelf *lte, if (bp_addr == 0) { /* Function pointers without PLT entries. */ - libsym->plt_type = LS_TOPLT_GOTONLY; + libsym->plt_type = LS_TOPLT_NONE; + libsym->arch.gotonly = 1; libsym->arch.type = MIPS_PLT_UNRESOLVED; + + /* Delay breakpoint activation until the symbol gets + * resolved. */ + libsym->delayed = 1; } *ret = libsym; @@ -333,6 +320,7 @@ fail: int arch_library_symbol_init(struct library_symbol *libsym) { + libsym->arch.gotonly = 0; libsym->arch.type = MIPS_PLT_UNRESOLVED; if (libsym->plt_type == LS_TOPLT_NONE) { libsym->arch.type = MIPS_PLT_RESOLVED; -- 1.7.8.6 _______________________________________________ Ltrace-devel mailing list [email protected] http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel
