Hi, this looks mostly fine, except for a couple mostly coding-style nits. I haven't tested yet though.
Mark Wielaard <[email protected]> writes: > diff --git a/output.c b/output.c > index f804cd0..0cbd651 100644 > --- a/output.c > +++ b/output.c [...] > +#if defined(HAVE_LIBDW) > +static int > +frame_callback (Dwfl_Frame *state, void *arg) > +{ > + Dwarf_Addr pc; > + bool isactivation; > + > + int *frames = (int *) arg; > + > + if (! dwfl_frame_pc (state, &pc, &isactivation)) Space before paren. > + return -1; > + > + if (! isactivation) > + pc--; > + > + Dwfl *dwfl = dwfl_thread_dwfl (dwfl_frame_thread (state)); Spaces. > + Dwfl_Module *mod = dwfl_addrmodule (dwfl, pc); And here. And many places below, I'll just stop here. > + const char *modname = NULL; > + const char *symname = NULL; > + GElf_Off off = 0; > + if (mod) { This should be mod != NULL. > + GElf_Sym sym; > + modname = dwfl_module_info(mod, NULL, NULL, NULL, NULL, > + NULL, NULL, NULL); > + symname = dwfl_module_addrinfo(mod, pc, &off, &sym, > + NULL, NULL, NULL); > + } > + > + /* This mimics the output produced by libunwind below. */ Two spaces after a period. > + fprintf(options.output, " > %s(%s+0x%" PRIx64 ") [%" PRIx64 "]\n", > + modname, symname, off, pc); > + > + /* If we wanted fancy we could try to extract the source line too... > + if (mod) { != NULL. > + Dwfl_Line *l = dwfl_module_getsrc(mod, pc); > + if (l) { != NULL. > + int line, col; > + const char* src; > + line = col = -1; > + src = dwfl_lineinfo (l, NULL, &line, &col, NULL, NULL); The definition of SRC can be merged with its initialization. > + if (src != NULL) { > + fprintf (options.output, "\n\t%s", src); > + if (line > 0) { > + fprintf (options.output, ":%d", line); > + if (col > 0) > + fprintf (options.output, > + ":%d", col); > + } > + fprintf (options.output, "\n"); > + } > + > + } > + } > + ... not fancy for now. */ I'm not against the fancy FWIW, though ltrace doesn't currently recognize existence of Dwarf. Any reason not to include the above code? Any reason not to drop it altogether then? > + > + /* Max number of frames to print reached? */ > + if ((*frames)-- == 0) > + return DWARF_CB_ABORT; > + > + return DWARF_CB_OK; > +} > +#endif /* defined(HAVE_LIBDW) */ > + > void > output_right(enum tof type, struct process *proc, struct library_symbol > *libsym, > struct timedelta *spent) > @@ -702,6 +766,17 @@ output_right(enum tof type, struct process *proc, struct > library_symbol *libsym, > } > #endif /* defined(HAVE_LIBUNWIND) */ > > +#if defined(HAVE_LIBDW) > + if (options.bt_depth > 0 && proc->leader->dwfl != NULL) { > + int frames = options.bt_depth; > + if (dwfl_getthread_frames(proc->leader->dwfl, proc->pid, > + frame_callback, &frames) < 0) Here you rely on the fact that DWARF_CB_ABORT is > 0, which is not obvious (though likely always true). But dwfl_getthread_frames doesn't care about particular value returned from the callback as long as it's non-zero, so why not change the DWARF_CB_ABORT above to 1? > + fprintf(stderr, "dwfl_getthread_frames tid %d: %s\n", > + proc->pid, dwfl_errmsg(-1)); > + fprintf(options.output, "\n"); > + } > +#endif /* defined(HAVE_LIBDW) */ > + > current_proc = NULL; > current_column = 0; > } > diff --git a/proc.c b/proc.c > index 97bcb60..e89da6e 100644 > --- a/proc.c > +++ b/proc.c [...] > static int > @@ -167,6 +172,10 @@ process_bare_init(struct process *proc, const char > *filename, > } > #endif /* defined(HAVE_LIBUNWIND) */ > > +#if defined(HAVE_LIBDW) > + proc->dwfl = NULL; /* Initialize for leader only on first library. */ Two spaces after period. [...] > + if (leader->dwfl == NULL) { > + int r; > + r = dwfl_linux_proc_attach (dwfl, leader->pid, > + true); You can merge the above two lines. Thanks, PM _______________________________________________ Ltrace-devel mailing list [email protected] http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel
