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

[Luca Clementi]

On Sun, Oct 6, 2013 at 11:01 PM, Petr Machata <pmachata at redhat.com> wrote:
> Luca Clementi <luca.clementi at gmail.com> writes:
>
>> For each stack frame 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.:
>>  > /bin/ls(_init+0x19be) [0x40398e]
>>  > /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xed) [0x7f50cbc3676d]
>>  > /bin/ls(_init+0x25fd) [0x4045cd]
>> ---
>>  output.c |   34 ++++++++++++++++++++++++++++++----
>>  1 file changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/output.c b/output.c
>> index da8c419..f89b287 100644
>> --- a/output.c
>> +++ b/output.c
>> @@ -548,6 +548,7 @@ free_stringp_cb(const char **stringp, void *data)
>>       free((char *)*stringp);
>>  }
>>
>> +
>>  void
>>  output_right(enum tof type, struct process *proc, struct library_symbol *libsym)
>>  {
>> @@ -670,16 +671,41 @@ again:
>>           && proc->unwind_priv != NULL
>>           && proc->unwind_as != NULL) {
>>               unw_cursor_t cursor;
>> -             unw_word_t ip, sp;
>> +             arch_addr_t ip;
>> +             unw_word_t function_offset, sp;
>> +             struct library *lib = NULL;
>>               int unwind_depth = options.bt_depth;
>>               char fn_name[100];
>> +             const char *lib_name;
>> +             size_t distance;
>>
>>               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_IP, (unw_word_t *) &ip);
>
> That cast is problematic.  unw_word_t is a uint64_t typedef, but
> arch_addr_t is a void* typedef, and may be 32-bit.

Hey Petr,
I had the same doubt but then when I looked at the code:

 $ grep -r unw_word_t include/* | grep typedef
include/libunwind-x86_64.h:typedef uint64_t unw_word_t;
include/libunwind-x86.h:typedef uint32_t unw_word_t;
[...]

I had the impression that this should be fine, since depending on the
platform you are compiling it might be either uint32 or uint64, which
should be the same as void* (32 or x86 and 64 on x86_64).
Is this correct?

I will send soon a patch with the other fixes.

Luca


>
>>                       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, sizeof(fn_name),
>> +                                     &function_offset);
>> +
>> +                     /* we are looking for the for the library with the
>> +                      * closest base address to the current ip */
>
> This should be /* We are looking for the library with the base address
> closest to the current ip.  */ (Word order, duplicate words, capital
> letter, period and two spaces at the end of sentence.)
>
>> +                     lib_name = NULL;
>> +                     distance = (size_t) -1;
>> +                     lib = proc->libraries;
>> +                     while (lib != NULL) {
>> +                             if ((ip > lib->base) &&
>> +                                         ((size_t)(ip - lib->base)
>> +                                         < distance)) {
>
> That should still be ip >= lib->base (though it's still unlikely to make
> difference in practice).
>
> That cast to size_t will become problematic in the future.
> Conceptually, arch_addr_t may be 64-bit even in a 32-bit tracer to allow
> tracing 64-bit processes.  (Currently it never is, as it's a void*
> typedef.)  I think it's fair to leave the code as-is now, but please
> annotate it like this (or similar):
>
>         /* N.B.: Assumes sizeof(size_t) == sizeof(arch_addr_t).
>            Keyword: double cast.  */
>
> The double cast bit is to tie this comment to a large bunch of other
> code that was already annotated and will need attention when the switch
> is done.
>
>> +                                     distance = ip - lib->base;
>> +                                     lib_name = lib->pathname;
>> +                             }
>> +                             lib = lib->next;
>> +                     }
>> +
>> +                     if (!lib_name)
>
> That should be if (lib_name == NULL).  In fact, just initialize lib_name
> with "unmapped_area" and drop this if.
>
>> +                             lib_name = "unmapped_area";
>> +                     fprintf(options.output, " > %s(%s+0x%lx) [0x%lx]\n",
>> +                                     lib_name, fn_name, function_offset, (long)ip);
>
> Make this [%p] and remove the cast at IP.  That will do the right thing
> now, and produce a warning in the future.
>
>> +
>>                       if (unw_step(&cursor) <= 0)
>>                               break;
>>                       unwind_depth--;
>
> Thanks,
> PM