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

[Dmitry V. Levin]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20140411/2f5bc5a6/attachment.bin>