[Bug tapsets/12500] ptrace decoder

["dsmith at redhat dot com" <sourceware-bugzilla at sourceware dot org>]

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.