[Bug tapsets/12471] New: Support wait4 *status printing

[Bug tapsets/12471] New: Support wait4 *status printing

[jan.kratochvil at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=12471

           Summary: Support wait4 *status printing
           Product: systemtap
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: tapsets
        AssignedTo: systemtap@sources.redhat.com
        ReportedBy: jan.kratochvil@redhat.com


Created attachment 5232
  --> http://sourceware.org/bugzilla/attachment.cgi?id=5232
wait4 *status printing

probe syscall.wait4.return { printf ("%s(%s)=%s\n", name, argstr, retstr) }

then prints nice:
wait4(-1, N/A, WNOHANG|__WCLONE, 0x0)=-10 (ECHILD)
wait4(-1, WSTOPSIG=SIGTRAP, WNOHANG, 0x0)=9319

The problem is we need argstr from the .return function.
Could we start providing argstr in all the syscall.return tapset hook?

This patch is on top of Bug 12470.

I do not understand where everywhere the code should be, it works here but
there is a massive duplication of all the wait* syscall hooks.

-- 
Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug tapsets/12471] Support wait4 *status printing

[dsmith at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=12471

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-09 17:19:01 UTC ---
(In reply to comment #0)
> Created attachment 5232 [details]
> wait4 *status printing
> 
> probe syscall.wait4.return { printf ("%s(%s)=%s\n", name, argstr, retstr) }
> 
> then prints nice:
> wait4(-1, N/A, WNOHANG|__WCLONE, 0x0)=-10 (ECHILD)
> wait4(-1, WSTOPSIG=SIGTRAP, WNOHANG, 0x0)=9319
> 
> The problem is we need argstr from the .return function.
> Could we start providing argstr in all the syscall.return tapset hook?
> 
> This patch is on top of Bug 12470.
> 
> I do not understand where everywhere the code should be, it works here but
> there is a massive duplication of all the wait* syscall hooks.

I'm not too fond of this change: it changes the meaning of 'argstr' and we
don't really have access to entry arguments in return probes (it works, because
we add a hidden entry probe to cache the values).

Perhaps adding a new variable called 'statusstr' or 'status_str' that just gets
the value of _wait_status_str() is a better idea.

We could also provide the user with WIFEXITED/WEXITSTATUS/etc. functions if
anyone thinks that would be a good idea.

-- 
Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug tapsets/12471] Support wait4 *status printing

[jan.kratochvil at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=12471

--- Comment #2 from Jan Kratochvil <jan.kratochvil at redhat dot com> 2011-02-09 17:24:52 UTC ---
(In reply to comment #1)
> Perhaps adding a new variable called 'statusstr' or 'status_str' that just gets
> the value of _wait_status_str() is a better idea.

I used it before but such variable needs to be thread/SMP protected.  There
should be some primitives to have per_cpu() accessible from .stp.


> We could also provide the user with WIFEXITED/WEXITSTATUS/etc. functions if
> anyone thinks that would be a good idea.

That decoding is provided by this patch, isn't it?


Thanks.

-- 
Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug tapsets/12471] Support wait4 *status printing

[dsmith at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=12471

--- Comment #3 from David Smith <dsmith at redhat dot com> 2011-02-09 17:45:12 UTC ---
(In reply to comment #2)
> (In reply to comment #1)
> > Perhaps adding a new variable called 'statusstr' or 'status_str' that just gets
> > the value of _wait_status_str() is a better idea.
> 
> I used it before but such variable needs to be thread/SMP protected.  There
> should be some primitives to have per_cpu() accessible from .stp.

Here's what I was suggesting:

probe syscall.wait4.return = kernel.function("sys_wait4").return
{
  name = "wait4"
  status_str = ($stat_addr == 0) ? "NULL"
     : _wait_status_str(user_int($stat_addr))
  retstr = return_str(1, $return)
}

> > We could also provide the user with WIFEXITED/WEXITSTATUS/etc. functions if
> > anyone thinks that would be a good idea.
> 
> That decoding is provided by this patch, isn't it?

This patch provides the WIFEXITED/WEXITSTATUS data as a string.  My thought
above was that we could provide functions like this:

function WTERMSIG:long(status:long) {
 return (status & 0x7f)
}

-- 
Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug tapsets/12471] Support wait4 *status printing

[jan.kratochvil at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=12471

--- Comment #4 from Jan Kratochvil <jan.kratochvil at redhat dot com> 2011-02-09 18:13:52 UTC ---
(In reply to comment #3)
> probe syscall.wait4.return = kernel.function("sys_wait4").return
> {
>   name = "wait4"
>   status_str = ($stat_addr == 0) ? "NULL"
>      : _wait_status_str(user_int($stat_addr))
>   retstr = return_str(1, $return)
> }

OK, I can do it this way for now.


My longer plan were more adjustments for getting closer to the output of
strace, for strace reimplementation on top of systemtap, as discussed before:
http://jankratochvil.net/priv/staptrace-0.1.tar.xz 

strace prints the returned wait string already as part of the parameters.
wait4(-1, [{WIFSTOPPED(s) && WSTOPSIG(s) == SIGTRAP}], WNOHANG, NULL) = 8580

That may be resolved later, though.

-- 
Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug tapsets/12471] Support wait4 *status printing

[jan.kratochvil at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=12471

Jan Kratochvil <jan.kratochvil at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #5232|0                           |1
        is obsolete|                            |

--- Comment #5 from Jan Kratochvil <jan.kratochvil at redhat dot com> 2011-02-15 19:35:21 UTC ---
Created attachment 5244
  --> http://sourceware.org/bugzilla/attachment.cgi?id=5244
Fix.

(In reply to comment #3)
> Here's what I was suggesting:
> probe syscall.wait4.return = kernel.function("sys_wait4").return
> {
>   name = "wait4"
>   status_str = ($stat_addr == 0) ? "NULL"
>      : _wait_status_str(user_int($stat_addr))
>   retstr = return_str(1, $return)
> }

OK, done this way.


> This patch provides the WIFEXITED/WEXITSTATUS data as a string.  My thought
> above was that we could provide functions like this:
> 
> function WTERMSIG:long(status:long) {
>  return (status & 0x7f)
> }

Done this way, the code looks much better now.

-- 
Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug tapsets/12471] Support wait4 *status printing

[dsmith at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=12471

--- Comment #6 from David Smith <dsmith at redhat dot com> 2011-03-03 19:09:14 UTC ---
(In reply to comment #5)
> Done this way, the code looks much better now.

The code does look much better now.  A few final small notes:

- Could you add the new 'status_str' support to the 'nd_syscall.wait4.return'
probe (in tapset/nd_syscalls2.stp)?

- Could you print out the new 'status_str' variable in
testsuite/buildok/syscalls2-detailed.stp ('syscall.wait4.return' probe) and in
testsuite/buildok/nd_syscalls2-detailed.stp ('nd_syscall.wait4.return' probe)?

-- 
Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.