(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.

(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.

(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)
}

(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.

Created attachment 5244 [details]
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.

(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)?

Created attachment 5275 [details]
Fix.

Done.

Also included forgotten PTRACE_EVENT_* decoding and printing unknown values above the low 16 bits.

Tested it does not complain with systemtap.pass1-4/buildok.exp .

(In reply to comment #7)
> Created attachment 5275 [details]
> Fix.
> 
> Done.
> 
> Also included forgotten PTRACE_EVENT_* decoding and printing unknown values
> above the low 16 bits.
> 
> Tested it does not complain with systemtap.pass1-4/buildok.exp .

Looks great to me.  Go ahead and check this in.

Thanks, checked in.

commit 5d2f95b856501e9c9da090523feae16e28c23ef0
Author: Jan Kratochvil <jan.kratochvil@redhat.com>
Date:   Mon Mar 7 16:18:13 2011 +0100

    Decode wait4.return wait status.
    http://sourceware.org/bugzilla/show_bug.cgi?id=12471
    
        * tapset/aux_syscalls.stp (WIFEXITED, WEXITSTATUS, WIFSIGNALED)
        (WCOREDUMP, WTERMSIG, WIFSTOPPED, WSTOPSIG, WIFCONTINUED)
        (_ptrace_event_name, _wait_status_str): New functions.
        * tapset/nd_syscalls2.stp (nd_syscall.wait4): New variables
        status_uaddr and status_str, call _wait_status_str.
        * tapset/syscalls2.stp (syscall.wait4.return): New variable
        status_str, call _wait_status_str.
        * testsuite/buildok/nd_syscalls2-detailed.stp
        (nd_syscall.wait4.return): Print also status_str.
        * testsuite/buildok/syscalls2-detailed.stp (syscall.wait4.return):
        Likewise.