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 <[email protected]>. 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
pgpLaLh4exexU.pgp
Description: PGP signature
------------------------------------------------------------------------------ Put Bad Developers to Shame Dominate Development with Jenkins Continuous Integration Continuously Automate Build, Test & Deployment Start a new project now. Try Jenkins in the cloud. http://p.sf.net/sfu/13600_Cloudbees
_______________________________________________ Strace-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/strace-devel
