On Thu, Sep 27, 2012 at 01:31:53AM +0200, Petr Machata wrote:
> [email protected] writes:
> > @@ -131,6 +163,9 @@ arch_elf_init(struct ltelf *lte, struct library *lib)
> > }
> > }
> >
> > + /* Tell the generic code how many dynamic trace:able symbols
> > + * we've got. */
> > + lte->relplt_count = lte->dynsym_count - lte->arch.mips_gotsym;
>
> Hmm, I'm not too fond of backend code overwriting core values like that,
> but I guess it's OK. It's in the right hook anyway. It's not worth it
> to build a separate hooklet for this.
>
> > return 0;
> > }
> >
> > @@ -139,4 +174,128 @@ arch_elf_destroy(struct ltelf *lte)
> > {
> > }
> >
> > +enum callback_status cb_each_sym(struct library_symbol *libsym, void *data)
>
> This should be static. cb_each_sym should start a new line.
>
> > +{
> > + struct Process *proc = data;
> > + struct breakpoint *bp;
> > +
> > + if (libsym->plt_type != LS_TOPLT_GOTONLY)
> > + return CBS_CONT;
> > +
> > + /* Update state. */
> > + libsym->arch.resolved_addr = (uintptr_t) sym2addr(proc, libsym);
> > +
> > + if (!libsym->arch.resolved_addr)
>
> Use libsym->arch.resolved_addr == 0. It's an address, not a boolean. I
> know this idiom is occasionally used in ltrace, but I can't stand it.
>
> > + /* FIXME: What does this mean? */
> > + return CBS_CONT;
> > +
> > + libsym->arch.type = RESOLVED;
> > +
> > + /* Add breakpoint. */
> > + bp = malloc(sizeof *bp);
> > + if (!bp
>
> Use bp == NULL.
>
> > + || breakpoint_init(bp, proc,
> > + (void *) (uintptr_t) libsym->arch.resolved_addr,
>
> The double cast shouldn't be necessary here. uintptr_t already is
> void*, and when we convert it to integral, breakpoint_init will be
> adjusted as well.
Hi,
It seems to be that you have confused uintptr_t with arch_addr_t i your
response?
The issue in this case is that resolved_addr is a GElf_addr, 64bit integral.
breakpoint init wants a pointer type, so I need to first transform the
64bit intergral value into a 32bit integral value and then cast it to
t. By using uintptr_t as the intermediate integral type, I think the code
will be reusable for both 32 and 64 bit mips archs.
I've added the same comment at you suggest a bit further down this email,
it's actually the exact same types being involved.
>
> > + 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;
> > + }
> > +
> > + return CBS_CONT;
> > +fail:
> > + free(bp);
> > + fprintf(stderr, "Failed to add breakpoint for %s\n", libsym->name);
> > + return CBS_FAIL;
> > +}
> > +
> > +enum callback_status cb_each_lib(struct Process *proc,
> > + struct library *lib, void *data)
>
> Static, newline.
>
> > +{
> > + struct library_symbol *libsym = NULL;
> > + while ((libsym = library_each_symbol(lib, libsym,
> > + cb_each_sym, proc)) != NULL) {
>
> Hmm, above you could just return CBS_CONT instead of CBS_FAIL, and
> change this into a simple call.
Nice cleanup, thanks.
> > + }
> > + return CBS_CONT;
> > +}
> > +
> > +void arch_dynlink_done(struct Process *proc)
> > +{
> > + proc_each_library(proc, NULL, cb_each_lib, NULL);
> > +}
> > +
> > +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));
> > + if (!libsym)
> > + return plt_fail;
>
> if (libsym == NULL)
>
> > +
> > + GElf_Addr addr = arch_plt_sym_val(lte, sym_index, 0);
> > +
> > + name = strdup(a_name);
> > + if (library_symbol_init(libsym,
> > + (arch_addr_t) (uintptr_t) addr,
>
> Please add a comment like this:
>
> /* XXX The double cast should be removed when
> * arch_addr_t becomes integral type. */
>
> When I get around to it, I'll grep for "double cast" to find individual
> cases.
>
> > + 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;
> > +
> > + if (!bp_addr) {
> > + /* Function pointers without PLT entries. */
> > + libsym->plt_type = LS_TOPLT_GOTONLY;
> > + libsym->arch.type = UNRESOLVED;
> > + }
>
> if (bp_addr == 0)
>
> With these nits fixed, I'll apply this patch.
>
> Thanks,
> PM
_______________________________________________
Ltrace-devel mailing list
[email protected]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel
