* [Bug tapsets/12500] ptrace decoder
2011-02-18 23:17 [Bug tapsets/12500] New: ptrace decoder jan.kratochvil at redhat dot com
@ 2011-02-24 19:06 ` dsmith at redhat dot com
0 siblings, 0 replies; 2+ messages in thread
From: dsmith at redhat dot com @ 2011-02-24 19:06 UTC (permalink / raw)
To: systemtap
http://sourceware.org/bugzilla/show_bug.cgi?id=12500
David Smith <dsmith at redhat dot com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |dsmith at redhat dot com
--- Comment #1 from David Smith <dsmith at redhat dot com> 2011-02-24 19:05:45 UTC ---
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.)
--
Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
^ permalink raw reply [flat|nested] 2+ messages in thread