[PATCH 01/25] ldv/unwind: improve stacktrace feature(v3)

[Masatake YAMATO]

> On Sun, Nov 10, 2013 at 03:41:57PM +0900, Masatake YAMATO wrote:
>> Based on the discussion on strace-devel, I improved following points:
> 
> Masatake, your patch contains several different changes and therefore is
> not easy to follow.  This is probably the reason why I failed to review it
> in November.  Please submit each logically separate change in a separate
> commit.

I see. I'll reflect the result of review and resubmit well-splitted patches.


Luca, I will work. I just remeber I am the person who needs the -k option
seriously in daily job:)

Masatake YAMATO

 
>> * use unwind_ as prefix for functions exported from unwind.c(v1)
> 
> The rename itself is fine, but the way how it's implemented is not.
> init_unwind_addr_space was renamed to unwind_init by creating a new
> function unwind_init which only purpose is to call init_unwind_addr_space.
> I wonder why not simply rename init_unwind_addr_space -> unwind_init.
> In the similar way, init_libunwind_ui was "renamed" to unwind_tcb_init,
> free_libunwind_ui to unwind_tcb_fin.
> 
> Also, your patch adds two trivial static functions (stacktrace_capture and
> stacktrace_print) that just call stacktrace_walk.  These functions are not
> callbacks, so why don't you call stacktrace_walk directly?
> 
>> * capture stacktrace of (_exit, exit_group and execve) at entering stage(v1, v3)
>> 
>>     About the most of system calls, stacktrace is captured and printed at
>>     the same time, the system call exiting.  However, this procedure makes
>>     unwanted result for some system calls.
>> 
>>     About execve, which is marked with STACKTRACE_CAPTURE_IN_ENTERING(CE),
>>     stacktrace is captured at the system call entering, and is printed at
>>     the exiting. By capturing in the system call entering, user can know
>>     the process context of execve being called(v1).
> 
> I'd rather name it STACKTRACE_CAPTURE_ON_ENTER (with shortcut name SE).
> 
>>     It is the same for _exit and exit_group. However, the timing of
>>     printing is different from execve: printing when associated tcb
>>     object(tcp) is dropped because there is no exiting stage for them(v3).
> 
> 1. Why do you call unwind_stacktrace_capture when hide_log_until_execve is set?
> 2. Is it safe to print stack trace from droptcb?
> 
> Also, I suppose you can rearrange the code you add to
> trace_syscall_entering so that unwind_cache_invalidate would be mentioned
> there only once.
> 
>>     struct queue_t and some manipulators are introduced for
>>     capturing-at-entering and printing-at-exiting.
> 
> One of them is mmap_cache_generation that used to implement lazy cache
> invalidation: when unwind_cache_invalidate is called, no cache is
> deallocated, instead all allocated cache becomes invalid.  Why this
> approach is better than direct cache deallocation?
> 
>> * control the maps cache with STACKTRACE_MAKE_CACHE_INVALID(CI) marker(v1)
>> 
>>     Currently stacktrace feature of strace needs the information of the
>>     memory mapping. Some of system calls like mmap change the memory
>>     mapping of the target process. Therefore unwind.c must track the
>>     modification of memory mapping.
>> 
>>     In older version the modification is tracked in print functions of
>>     each system call. In this patch STACKTRACE_MAKE_CACHE_INVALID(CI)
>>     marker is introduced. In stead of handling in print functions, the
>>     modification is tracked in system call entering only if current
>>     system call is marked with CI.
> 
> I'd rather name it STACKTRACE_INVALIDATE_CACHE (with shortcut name SI).
> 
>>     If new system call modifying the memory mapping is added, just
>>     mark it with CI.
>> 
>> * mark CI on brk, mremap, shmat, shmdt, and remap_file_pages(v1)
>> 
>> * separate arch dependent CI/CE marking code to different patches(v2)
> 
> You made ~25 patches to implement this, one per architecture, but
> reviewing it would be a herculean task.  I suppose you made this change
> using a simple script.  Adding this script to the commit message would
> help reviewer a lot.
> 
>> * simplify call_t data structure(v3)
>> 
>>     In older version output lines of captured elements are built when
>>     printing. In this version they are built when capturing the stack.
>>     As the result dynamic memory allocations are suppressed.
>>     Suggested by Luca Clementi <luca.clementi at gmail.com>.
> 
> Your patch adds a function called sprint_call_or_error.  What it
> essentially does is a asprintf(3) emulation using a realloc-snprintf loop.
> Why don't you call asprintf directly?
> 
>> * handle multiple threads(v3)
>> 
>>     Rebuild caches for mmaps of all target threads if a thread invokes
>>     CI makred syscalls. This is inefficient; whether an address space
>>     is shared or not between the threads is not considered.
>> 
>> TODO:
>> 
>> * move the memory mapping cache to libuwind and use API for it
>> 
>>     See http://lists.nongnu.org/archive/html/libunwind-devel/2013-10/msg00001.html
> 
> Is there any progress with this idea?
> 
> 
> -- 
> ldv