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

Wed Apr 16 07:20:15 UTC 2014

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.

I'm sorry. I've well separated patches are submitted.
Could you review again?

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

I was thinking about extendin my code more. I removed all such indirect calls
(wrapper functions) are removed. 

[PATCH v4 04/37] unwind: add unwind_ as prefix to functions exported from unwind.c
[PATCH v4 06/37] unwind: introduce stacktrace_walker
[PATCH v4 08/37] unwind: introduce queue_t for capturing stacktrace

>> * 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).

Renamed.
[PATCH v4 13/37] unwind: introduce markers specifying the needs of special care in unwinding

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

This is my misunderstanding of meaning of `hide_log_until_execve'.
Fixed in

[PATCH v4 37/37] unwind: stacktrace capturing and mmap cache invalidating at trace_syscall_entering.

> 2. Is it safe to print stack trace from droptcb?

As you say, it is dangerous. I rearranged to make printing safely.
[PATCH v4 10/37] unwind: call unwind_tcb_fin before printing detached message
[PATCH v4 08/37] unwind: introduce queue_t for capturing stacktrace

> 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.

I moved `unwind_cache_invalidate' from entering stage to exiting stage.
Much simplified.
[PATCH v4 37/37] unwind: stacktrace capturing and mmap cache invalidating at trace_syscall_entering.

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

Consider there are many threads shared the same memmory mapping.
In current implementation mmap cache is not shared between
tcbs associated with the threads.

When one thread calls mmap, tcb associated with the thread have
a chance to invalidate the cache. About this thread deallocation 
at the time is simple. However, we have to think about the mmap 
caches of annother threads. I guessed deallocation all of them at 
the time may make the code complex; all tcb must be traversed.

In the future I should make the data structure of mmap cache should 
be more multi-threading aware. I've documented related topics in
log of 

    [PATCH v4 09/37] unwind: consider multi-threading when rebuilding mmap cache

>> * 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).

Renamed.
[PATCH v4 13/37] unwind: introduce markers specifying the needs of special care in unwinding

>>     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.

I'm sorry but I have no such intelligent script.
In my experience, it is not so obvious to know which system call must
be marked SI and/or SE. I think someone familiar with the target
CPU type should work on marking.

I'm using GNU/Linux running on x86_64. So SI and/or SE maker of the
CPU type is reliable. However, I don't know and have an access 
the other CPU types. I marked SI on bsd43_sstk of mips. However,
I wonder the marking is really needed or not.

As far as I remember Luca asked me to prepare the patches for 
other CPU types last year. So I prepared without serious consideration.
However, now I think we should prepare stractrace feature only tested
CPU types, here x86_64.

So my opinion is patches from [PATCH v4 15/37] to [PATCH v4 36/37]
should be throw away.

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

Thanks. I should use asprintf. Rewrited based on asprintf.
[PATCH v4 08/37] unwind: introduce queue_t for capturing stacktrace

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

I'm sorry but no progress here.
To hack libunwind, more deeper knowledge about linking and elf than I 
have now.

Thank you again.

Masatake YAMATO

> 
> -- 
> ldv