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

[Masatake YAMATO]

First of all, thank you for reviewing.

I've rearranged my patch set; I splinted the original
patch into smaller patches and reflected your comments.
I'll submit them.

I'll reply to your review result with quoting the new
patches.

Masatake YAMATO

On Fri, 11 Apr 2014 02:58:15 +0400, "Dmitry V. Levin" <ldv at altlinux.org> wrote:
> 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.
> 
>> * 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