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

[Luca Clementi]

On Mon, Sep 30, 2013 at 5:17 AM, Petr Machata <pmachata at redhat.com> wrote:
> 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);
>>  }
>>

Dear Petr,
I also forgot to
#ifdef HAVE_LIBUNWIND

>> +
>> +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?
>

Honestly I don't know enough linker internals to say that.

I just did some empirical tests on Centos5/6 and on Ubuntu 12.x (that
uses the new Gold linker).

It would be safe to store the length of library from the base in the
library struct.
Although I think that at least theoretically, you could have several
PT_LOAD executable segments which are not adjacent, so if you really
want to be paranoid you should have a list of:
struct {
arch_addr_t base_segment;
size_t length
}

So it depends on how much compliant we want to be.
Unfortunately unwind lib does not have an API to get the lib name (I
think Masatake is working on it).

Thanks for the review.
I will post the fixes soon.

Luca

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