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