Re: [patch] Bug #5001 - ctime() vs _stp_ctime() duplication

[Mark Wielaard <mwielaard at redhat dot com>]

Hi Jim,

On Wed, 2008-05-21 at 11:55 -0700, Jim Keniston wrote:
> On Wed, 2008-05-21 at 14:57 +0200, Mark Wielaard wrote:
> > Hi,
> > 
> > On Wed, 2008-05-21 at 14:01 +0200, Mark Wielaard wrote:
> > > I am not sure how I should fix this. Is the idea that the arguments are
> > > now always referred to by number?
> 
> There's been no consensus or fiat to convert tapsets from named args to
> numbered args so they can be used in dwarfless probing.  But there's
> been some background work on doing just that for the syscalls tapset --
> see nd_syscalls.stp -- and I have a CS student who's willing to continue
> that conversion once spring term ends.

aha, nice. This was in syscall2.stp which doesn't have a nd_ equivalent
yet.

Having all this duplicated does seem a little wasteful though. Have
there been any thoughts on some kind of "combined syntax" to indicate
both the argument number and argument name so these tapsets don't have
to be completely duplicated?

> In this case, the fault may be with the nodw*.stp tests.  There's no
> particular reason nodwf02.stp and nodwf05.stp have to probe exactly that
> set of system calls.  If your changes to syscalls*.stp make sense
> otherwise, then we should just tweak the list in nodwf0{2,5}.stp.

I think the change is OK. I don't see these functions particularly
benefiting from explicitly referring to the function arguments by name.

> Sorry, I haven't been following the 5001 thread.  Could you send me
> before and after tapsets so I can figure out what went wrong wrt
> dwarfless?

The full patch is git show fc5a2d42b6cc46a9d4f7f3919ddc74ce70ad2a66
Just the syscall2.stp change is attached. Reverting to the changeset
before that does make both nodwf02.stp and nodwf05.stp pass. Adding the
patch from git show e483d9dfa614ee17b488df7224ee22a0f7dc9386 makes them
pass again. The thing I don't understand is why it passed before my
patch (on irc it was suggested that maybe for some reason it was still
using the dwarf debuginfo by accident).

Thanks,

Mark

commit fc5a2d42b6cc46a9d4f7f3919ddc74ce70ad2a66
Author: Mark Wielaard <mwielaard@redhat.com>
Date:   Tue May 20 21:58:04 2008 +0200

    PR5001: Remove _stp_ctime and always use ctime.

diff --git a/tapset/syscalls2.stp b/tapset/syscalls2.stp
index 558e89b..31e1830 100644
--- a/tapset/syscalls2.stp
+++ b/tapset/syscalls2.stp
@@ -2900,8 +2900,10 @@ probe syscall.utime = kernel.function("sys_utime") ? {
 	filename_uaddr = $filename
 	filename = user_string($filename)
 	buf_uaddr = $times
-	buf_str = _struct_utimbuf_u($times)
-	argstr = sprintf("%s, %s", user_string_quoted($filename), buf_str)
+	actime = _struct_utimbuf_actime(buf_uaddr)
+	modtime = _struct_utimbuf_modtime(buf_uaddr)
+	argstr = sprintf("%s, [%s, %s]", user_string_quoted($filename),
+	       	 	 ctime(actime), ctime(modtime))
 }
 probe syscall.utime.return = kernel.function("sys_utime").return ? {
 	name = "utime"
@@ -2914,8 +2916,10 @@ probe syscall.compat_utime = kernel.function("compat_sys_utime") ? {
 	filename_uaddr = $filename
 	filename = user_string($filename)
 	buf_uaddr = $t
-	buf_str = _struct_compat_utimbuf_u($t)
-	argstr = sprintf("%s, %s", user_string_quoted($filename), _struct_compat_utimbuf_u($t))
+	actime = _struct_compat_utimbuf_actime(buf_uaddr)
+	modtime = _struct_compat_utimbuf_modtime(buf_uaddr)
+	argstr = sprintf("%s, [%s, %s]", user_string_quoted($filename),
+	       	 	 ctime(actime), ctime(modtime))
 }
 probe syscall.compat_utime.return = kernel.function("compat_sys_utime").return ? {
 	name = "utime"