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