Created attachment 5252 [details] ptrace decoder. Tested on: F14 x86_64 RHEL-6.1 s390+s390x+ppc64 ia64 is untested There is some room for improvements but it works fine. probe syscall.ptrace.return // TODO: _arch_ptrace_return would need to create probe variables - how? - A function called from probe cannot create variables like `name'. s390x: // FIXME: ptrace of 32-bit debuggers is not caught by SystemTap, // use +4 and +8 offsets for them On RHEL-4/RHEL-5 there may be needed more #ifndef/#define's like now in tapset/powerpc/syscalls.stp but I did not have systemtap-1.4/HEAD on those boxes to test it.
I've taken a look at the patch. Here are some comments: 1) On x86_64 RHEL5 (2.6.18-238.1.1.el5), the following come up as undefined: PTRACE_SINGLEBLOCK, PTRACE_GETREGSET, and PTRACE_SETREGSET. On ia64 RHEL5 (2.6.18-194.26.1.el5), the following come up as undefined: PTRACE_GETREGSET and PTRACE_SETREGSET. It looks like on RHEL5, PTRACE_[GS]ETREGS is used instead of PTRACE_[GS]ETREGSET. 2) I'm not too fond of the following style of code in tapset/powerpc/syscalls.stp: ==== function _arch_ptrace(request,pid,addr,data) { if (request == %{ PTRACE_GETVRREGS %}) // TODO: Retrieve *data in .return return sprintf ("PTRACE_GETVRREGS, %d, data=%p", pid, data) # ... stuff deleted ... %{ #ifndef PPC_PTRACE_GETHWDBGINFO # define PPC_PTRACE_GETHWDBGINFO 0x89 #endif 1 %} if (request == %{ PPC_PTRACE_GETHWDBGINFO %}) // TODO: Retrieve *data in .return return sprintf ("PPC_PTRACE_GETHWDBGINFO, %d, data=%p", pid, data) # ... stuff deleted ... } ==== I'm not sure we envisioned using the inline embedded-C expressions that way. Instead I'd suggest something like the following (note that I haven't actually tried this): ==== %{ #ifndef PPC_PTRACE_GETHWDBGINFO # define PPC_PTRACE_GETHWDBGINFO 0x89 #endif %} function _arch_ptrace(request,pid,addr,data) { if (request == %{ PTRACE_GETVRREGS %}) // TODO: Retrieve *data in .return return sprintf ("PTRACE_GETVRREGS, %d, data=%p", pid, data) # ... stuff deleted ... if (request == %{ PPC_PTRACE_GETHWDBGINFO %}) // TODO: Retrieve *data in .return return sprintf ("PPC_PTRACE_GETHWDBGINFO, %d, data=%p", pid, data) # ... stuff deleted ... } ==== You lose a bit of locality, but gain a bit of readability (and don't have to use a fake return value from the inline embedded-C expression). 3) I'd probably move most of the new code in the syscall.ptrace probe itself to an auxiliary function, so that nd_syscall.ptrace could share it also. 4) Code like this: ==== probe syscall.ptrace = kernel.function("sys_ptrace").call ? { name = "ptrace" request = $request pid = $pid addr = $addr data = $data argstr=_arch_ptrace(request,pid,addr,data) ==== is better expressed as: probe syscall.ptrace = kernel.function("sys_ptrace").call ? { name = "ptrace" request = $request pid = $pid addr = $addr data = $data argstr=_arch_ptrace($request,$pid,$addr,$data) ==== By using the dwarf variables directly, the optimizer has an easier time getting rid of unused convenience variables. (Feel free to use a convenience variable when getting the convenience variable value is complicated.)
Created attachment 5308 [details] ptrace decoder #2. Addressed your points, thanks. Tested on F14.x86_64. For the other arches I can test them if you point me to some boxes (inside Red Hat), allocating each arch via Beaker for each patch submit/verification is very expensive. Possible typos etc. should be very easy to fix during the final package port/tests I hope. I see some problems ptrace return value is not being decoded. This bug is outside of the scope of this patch. probe syscall.ptrace { printf ("->%s(%s)\n", name, argstr) } probe syscall.ptrace.return { printf ("<-%s,%p,%p\n",retstr,geteventmsg_data,arch_prctl_addr) } ->ptrace(PTRACE_POKEDATA, 23768, addr=0x3699e0e580, data=0x2e6666666666c3f3) <-0,0x0,0x0 ->ptrace(PTRACE_PEEKTEXT, 23768, addr=0x3699e0e580) <-0,0x0,0x0 That `retstr' is returned as `0' but it definitely had the value 0x2e6666666666c3f3 - on patched systemtap-1.4-2.fc14.x86_64
(In reply to comment #1) > It looks like on RHEL5, PTRACE_[GS]ETREGS is used instead of > PTRACE_[GS]ETREGSET. In fact everywhere are used the older PTRACE_[GS]ETREGS. PTRACE_[GS]ETREGSET are a recent addition: http://sourceware.org/ml/archer/2010-q3/msg00193.html #ifndef+#defined+#endif should be suitable for this purpose. These new commands have their specific new assigned numbers and it does not hurt if systemtap supports decoding even unimplemented commands on those older platforms.
Created attachment 5309 [details] ptrace decoder #3. Fix probe nd_syscall.ptrace.return. - long_arg values are not available there.
(In reply to comment #4) > Created attachment 5309 [details] > ptrace decoder #3. > > Fix probe nd_syscall.ptrace.return. > - long_arg values are not available there. The new patch looks good to me. The only thing I can think of that needs doing is adding some compile testcases for the new functions in testsuite/buildok/aux_syscalls-embedded.stp and adding a new compile testcase for the arch-specific aux_syscalls.stp files.
Created attachment 5342 [details] ptrace decoder #4. (In reply to comment #5) > The only thing I can think of that needs doing > is adding some compile testcases for the new functions in > testsuite/buildok/aux_syscalls-embedded.stp Done. Only the functions not referenced otherwise (as called only from syscalls2.stp) were added. > and adding a new compile testcase > for the arch-specific aux_syscalls.stp files. Should the testsuite test all the arch code - even for other arches? Currently it tests only the testsuite host native arch. As the host-specific kernel include files are not present on different arch hosts the code would need to add many stubs +#ifndef PTRACE_foo +# define PTRACE_foo 42 +#endif which would be never needed on a native arch host.
(In reply to comment #6) > Created attachment 5342 [details] > ptrace decoder #4. > > (In reply to comment #5) > > The only thing I can think of that needs doing > > is adding some compile testcases for the new functions in > > testsuite/buildok/aux_syscalls-embedded.stp > > Done. Only the functions not referenced otherwise (as called only from > syscalls2.stp) were added. Thanks. > > and adding a new compile testcase > > for the arch-specific aux_syscalls.stp files. > > Should the testsuite test all the arch code - even for other arches? Currently > it tests only the testsuite host native arch. Sorry, I should have explained better. You'll have one new test file, something like buildok/aux_syscalls-arch-embedded.stp that would do something like: %( arch == "i386" %? # test i386 arch specific functions... %) %( arch == "x86_64" %? # test x86_64 arch specific functions... %) # ... and so on ... Looking at your patch again, you may not need that if your new tests in buildok/aux_syscalls-embedded.stp test all your new functions.
(In reply to comment #7) > Looking at your patch again, you may not need that if your new tests in > buildok/aux_syscalls-embedded.stp test all your new functions. Yes, it always tests all the new functions on the native arch as all get referenced some way. I have verified a typo in any of them gets reported. Still tapset/s390/aux_syscalls.stp is not tested on x86_64 box etc. This part should be done I hope (and I should start checking kernel.trace), thanks for the reviews.
Checked in as it seems to be approved, thanks. commit aa1d3e0aaf021b45f12b79f09d24e3935c54829c Author: Jan Kratochvil <jan.kratochvil@redhat.com> Date: Wed Mar 30 19:59:44 2011 +0200 Decode ptrace. http://sourceware.org/bugzilla/show_bug.cgi?id=12500 * tapset/aux_syscalls.stp (_ptrace_options_str, _ptrace_argstr) (_ptrace_return_geteventmsg_data): New functions. * tapset/i386/aux_syscalls.stp: New file. * tapset/ia64/aux_syscalls.stp: New file. * tapset/nd_syscalls2.stp (nd_syscall.ptrace): Set argstr using _ptrace_argstr. (nd_syscall.ptrace.return): Set geteventmsg_data and arch_prctl_addr. * tapset/powerpc/aux_syscalls.stp: New file. * tapset/s390/aux_syscalls.stp: New file. * tapset/syscalls2.stp (syscall.ptrace): Set argstr using _ptrace_argstr. (syscall.ptrace.return): Set geteventmsg_data and arch_prctl_addr. * tapset/x86_64/aux_syscalls.stp: New file. * testsuite/buildok/aux_syscalls-embedded.stp (begin): Call _ptrace_argstr, _ptrace_return_geteventmsg_data and _ptrace_return_arch_prctl_addr.