Luca Clementi <[email protected]> writes:
> For each stack frame this patch prints the library path containing the
> code pointed by the IP.
> The output format is similar to the return value of backtrace_symbols
> function found in glibc, e.g.:
> > ./a.out() [0x40063d]
> > ./a.out() [0x4006bb]
> > ./a.out() [0x4006c6]
> > /lib64/libc.so.6(__libc_start_main+0xed) [0x7fa2f8a5976d]
> > ./a.out() [0x400569]
I like the overall direction of this patch, but I have a number of
points that will need addressing before this goes in.
> ---
> output.c | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/output.c b/output.c
> index da8c419..d0bf80e 100644
> --- a/output.c
> +++ b/output.c
> @@ -548,6 +548,14 @@ free_stringp_cb(const char **stringp, void *data)
> free((char *)*stringp);
> }
>
> +
> +static enum callback_status
> +find_lib_ip(struct process *proc, struct library *lib, void *ip)
> +{
> + arch_addr_t * my_ip = (arch_addr_t *) ip;
No space after *. I believe the cast is redundant, as IP is void*.
> + return CBS_STOP_IF((*my_ip > lib->base) && (*my_ip < lib->dyn_addr));
This should be *my_ip >= lib->base, though in practice this is unlikely
to make difference.
In the other part of the equation, you assume that PT_DYNAMIC will be
mapped after any LOAD segments--do you think we can rely on that?
> +}
> +
> void
> output_right(enum tof type, struct process *proc, struct library_symbol
> *libsym)
> {
> @@ -671,15 +679,25 @@ again:
> && proc->unwind_as != NULL) {
> unw_cursor_t cursor;
> unw_word_t ip, sp;
> + unw_word_t function_off_set;
function_offset would be a better name.
> + struct library *lib = NULL;
> int unwind_depth = options.bt_depth;
> char fn_name[100];
> + char * temp_name;
No space after *.
>
> unw_init_remote(&cursor, proc->unwind_as, proc->unwind_priv);
> while (unwind_depth) {
> unw_get_reg(&cursor, UNW_REG_IP, &ip);
> unw_get_reg(&cursor, UNW_REG_SP, &sp);
> - unw_get_proc_name(&cursor, fn_name, 100, NULL);
> - fprintf(options.output, "\t\t\t%s (ip = 0x%lx)\n",
> fn_name, (long) ip);
> + unw_get_proc_name(&cursor, fn_name, 100,
> &function_off_set);
Please change the 100 here to sizeof fn_name. Also, this line is too
long (the limit is 80 characters).
> +
> + lib = proc_each_library(proc, NULL, find_lib_ip, &ip);
> + if ( lib )
There shouldn't be spaces around lib.
> + temp_name = (char *) lib->pathname;
> + else
> + temp_name = "unmapped_area";
I'm surprised your compiler doesn't warn about assigning a string
literal to char *. temp_name should be const char *, at which point you
can drop the cast at lib->pathname as well. Also, temp_name is not such
a good name--make it lib_name, map_name, or some such.
> + fprintf(options.output, " > %s(%s+0x%lx) [0x%lx]\n",
> temp_name, fn_name, function_off_set, (long) ip);
This line is too long.
> +
> if (unw_step(&cursor) <= 0)
> break;
> unwind_depth--;
Thanks,
PM
_______________________________________________
Ltrace-devel mailing list
[email protected]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel
