[Ltrace-devel] [PATCH] add library path to stack trace output when using -w

[Petr Machata]

Luca Clementi <luca.clementi at gmail.com> 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