changelog files, %( %) idioms

changelog files, %( %) idioms

[Frank Ch. Eigler]

Hi -

Please remember to add ChangeLog file entries for patches you commit.
You committed several important fixes that only show up on careful
search of cvs/git logs.  Please rescan src/HACKING for guidelines, and
consider retroactively adding the ChangeLog entries.

Thanks for the bug #5772 patch.  It is possible to make the %( kernel
%) conditionals look more compact by realizing that they don't operate
at the statement but at the token level.  That means one can use them
around just the smallest bit of code that needs to be changed for
different versions/architectures, so that instead of:

   %( kernel_vr > "2.6.24" %?
   argstr = sprintf("%d, %p, %s, %p", $upid, $stat_addr, _wait4_opt_str($options), $ru)
   %:
   argstr = sprintf("%d, %p, %s, %p", $pid, $stat_addr, _wait4_opt_str($options), $ru)
   %)

and

   %( kernel_vr > "2.6.24" %?
      pid = $upid
   %:
      pid = $pid
   %)

one could write ...

   argstr = sprintf("%d, %p, %s, %p",
     %( kernel_vr > "2.6.24" %? $upid %: $pid %),
     $stat_addr, _wait4_opt_str($options), $ru)

and

   pid = %( kernel_vr > "2.6.24" %? $upid %: $pid %)


- FChE

Re: changelog files, %( %) idioms

[Ananth N Mavinakayanahalli]

On Mon, Feb 25, 2008 at 09:59:15AM -0500, Frank Ch. Eigler wrote:
> 
> Thanks for the bug #5772 patch.  It is possible to make the %( kernel
> %) conditionals look more compact by realizing that they don't operate
> at the statement but at the token level.  That means one can use them
> around just the smallest bit of code that needs to be changed for
> different versions/architectures, so that instead of:
> 
>    %( kernel_vr > "2.6.24" %?
>    argstr = sprintf("%d, %p, %s, %p", $upid, $stat_addr, _wait4_opt_str($options), $ru)
>    %:
>    argstr = sprintf("%d, %p, %s, %p", $pid, $stat_addr, _wait4_opt_str($options), $ru)
>    %)
> 
> and
> 
>    %( kernel_vr > "2.6.24" %?
>       pid = $upid
>    %:
>       pid = $pid
>    %)
> 
> one could write ...
> 
>    argstr = sprintf("%d, %p, %s, %p",
>      %( kernel_vr > "2.6.24" %? $upid %: $pid %),
>      $stat_addr, _wait4_opt_str($options), $ru)
> 
> and
> 
>    pid = %( kernel_vr > "2.6.24" %? $upid %: $pid %)

We have a similar issue with the x86_32 syscalls tapset. sys_signalstack
has a reference to ebx, while with register unification, its now bx. The
tapset is thus broken for kernel_vr > 2.6.24.

Following the compact method, the patch looks thus:

---
 tapset/i686/syscalls.stp |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Index: systemtap-3mar/tapset/i686/syscalls.stp
===================================================================
--- systemtap-3mar.orig/tapset/i686/syscalls.stp
+++ systemtap-3mar/tapset/i686/syscalls.stp
@@ -119,8 +119,8 @@ probe syscall.set_zone_reclaim.return =
 #
 probe syscall.sigaltstack = kernel.function("sys_sigaltstack") {
 	name = "sigaltstack"
-	ebx = $ebx
-	argstr = sprintf("%p", $ebx)
+	bx = %( kernel_vr > "2.6.24" %? $bx %: $ebx %)
+	argstr = sprintf("%p", %( kernel_vr > "2.6.24" %? $bx %: $ebx %) )
 }
 probe syscall.sigaltstack.return = kernel.function("sys_sigaltstack").return {
 	name = "sigaltstack"

Notice that the LHS has changed from ebx to bx.

However, if we need to have a 100% backward compatibility, I'd prefer
the older approach thus:

---
 tapset/i686/syscalls.stp |    5 +++++
 1 files changed, 5 insertions(+)

Index: systemtap-3mar/tapset/i686/syscalls.stp
===================================================================
--- systemtap-3mar.orig/tapset/i686/syscalls.stp
+++ systemtap-3mar/tapset/i686/syscalls.stp
@@ -119,8 +119,13 @@ probe syscall.set_zone_reclaim.return =
 #
 probe syscall.sigaltstack = kernel.function("sys_sigaltstack") {
 	name = "sigaltstack"
+%( kernel_vr > "2.6.24" %?
+	bx = $bx
+	argstr = sprintf("%p", $bx)
+%:
 	ebx = $ebx
 	argstr = sprintf("%p", $ebx)
+%)
 }
 probe syscall.sigaltstack.return = kernel.function("sys_sigaltstack").return {
 	name = "sigaltstack"

Comments?

Ananth

Re: changelog files, %( %) idioms

[Frank Ch. Eigler]

Hi -

On Mon, Mar 03, 2008 at 05:48:41PM +0530, Ananth N Mavinakayanahalli wrote:
> [...]
>  probe syscall.sigaltstack = kernel.function("sys_sigaltstack") {
>  	name = "sigaltstack"
> -	ebx = $ebx
> -	argstr = sprintf("%p", $ebx)
> +	bx = %( kernel_vr > "2.6.24" %? $bx %: $ebx %)
> +	argstr = sprintf("%p", %( kernel_vr > "2.6.24" %? $bx %: $ebx %) )

I suggest picking a single more informative variable name than "ebx"
or "bx" for that parameter.  Then that variable could be used as the
plain sprintf value.

- FChE

Re: changelog files, %( %) idioms

[William Cohen]

Frank Ch. Eigler wrote:
> Hi -
> 
> On Mon, Mar 03, 2008 at 05:48:41PM +0530, Ananth N Mavinakayanahalli wrote:
>> [...]
>>  probe syscall.sigaltstack = kernel.function("sys_sigaltstack") {
>>  	name = "sigaltstack"
>> -	ebx = $ebx
>> -	argstr = sprintf("%p", $ebx)
>> +	bx = %( kernel_vr > "2.6.24" %? $bx %: $ebx %)
>> +	argstr = sprintf("%p", %( kernel_vr > "2.6.24" %? $bx %: $ebx %) )
> 
> I suggest picking a single more informative variable name than "ebx"
> or "bx" for that parameter.  Then that variable could be used as the
> plain sprintf value.
> 
> - FChE

I saw this this cause failure on F-9 i686. There could be 2.6.24.1, so probably 
want to check that the kernel is earlier than 2.6.25. Something like the following:

bx = %( kernel_vr < "2.6.25" %? $bx %: $ebx %)

Could the use of $bx be factored out of argstr, so there is only one check for 
the kernel version for $ebx/$bx rather than two?

-Will

Re: changelog files, %( %) idioms

[Ananth N Mavinakayanahalli]

On Mon, Mar 03, 2008 at 11:48:13AM -0500, William Cohen wrote:
> Frank Ch. Eigler wrote:
>> Hi -
>>
>> On Mon, Mar 03, 2008 at 05:48:41PM +0530, Ananth N Mavinakayanahalli wrote:
>>> [...]
>>>  probe syscall.sigaltstack = kernel.function("sys_sigaltstack") {
>>>  	name = "sigaltstack"
>>> -	ebx = $ebx
>>> -	argstr = sprintf("%p", $ebx)
>>> +	bx = %( kernel_vr > "2.6.24" %? $bx %: $ebx %)
>>> +	argstr = sprintf("%p", %( kernel_vr > "2.6.24" %? $bx %: $ebx %) )
>>
>> I suggest picking a single more informative variable name than "ebx"
>> or "bx" for that parameter.  Then that variable could be used as the
>> plain sprintf value.
>>
>> - FChE
>
> I saw this this cause failure on F-9 i686. There could be 2.6.24.1, so 
> probably want to check that the kernel is earlier than 2.6.25. Something 
> like the following:

Actually, this went into 2.6.25-rc

> bx = %( kernel_vr < "2.6.25" %? $bx %: $ebx %)
>
> Could the use of $bx be factored out of argstr, so there is only one check 
> for the kernel version for $ebx/$bx rather than two?

How does this look?

---
 tapset/i686/syscalls.stp |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Index: systemtap-4mar/tapset/i686/syscalls.stp
===================================================================
--- systemtap-4mar.orig/tapset/i686/syscalls.stp
+++ systemtap-4mar/tapset/i686/syscalls.stp
@@ -119,8 +119,8 @@ probe syscall.set_zone_reclaim.return =
 #
 probe syscall.sigaltstack = kernel.function("sys_sigaltstack") {
 	name = "sigaltstack"
-	ebx = $ebx
-	argstr = sprintf("%p", $ebx)
+	ussp = %( kernel_vr < "2.6.25" %? $ebx %: $bx %)
+	argstr = sprintf("%p", ussp)
 }
 probe syscall.sigaltstack.return = kernel.function("sys_sigaltstack").return {
 	name = "sigaltstack"

examples/wait4time.stp

[Wenji Huang]

Hi,

In previous threads, there is the discussion about syscall.wait4 due to 
renamed parameter in kernel 2.6.25.

	  pid = %( kernel_vr > "2.6.24" %? $upid %: $pid %)

So the related script examples/wait4time.stp need to reflect that.

    t = gettimeofday_us(); p = pid()
    entry_wait4[p] = t
-  wait4_pid[p]=$pid
+  wait4_pid[p]=pid
  }

Regards,
Wenji

Re: changelog files, %( %) idioms

[William Cohen]

Ananth N Mavinakayanahalli wrote:
> On Mon, Mar 03, 2008 at 11:48:13AM -0500, William Cohen wrote:
>> Frank Ch. Eigler wrote:
>>> Hi -
>>>
>>> On Mon, Mar 03, 2008 at 05:48:41PM +0530, Ananth N Mavinakayanahalli wrote:
>>>> [...]
>>>>  probe syscall.sigaltstack = kernel.function("sys_sigaltstack") {
>>>>  	name = "sigaltstack"
>>>> -	ebx = $ebx
>>>> -	argstr = sprintf("%p", $ebx)
>>>> +	bx = %( kernel_vr > "2.6.24" %? $bx %: $ebx %)
>>>> +	argstr = sprintf("%p", %( kernel_vr > "2.6.24" %? $bx %: $ebx %) )
>>> I suggest picking a single more informative variable name than "ebx"
>>> or "bx" for that parameter.  Then that variable could be used as the
>>> plain sprintf value.
>>>
>>> - FChE
>> I saw this this cause failure on F-9 i686. There could be 2.6.24.1, so 
>> probably want to check that the kernel is earlier than 2.6.25. Something 
>> like the following:
> 
> Actually, this went into 2.6.25-rc
> 
>> bx = %( kernel_vr < "2.6.25" %? $bx %: $ebx %)
>>
>> Could the use of $bx be factored out of argstr, so there is only one check 
>> for the kernel version for $ebx/$bx rather than two?
> 
> How does this look?
> 
> ---
>  tapset/i686/syscalls.stp |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> Index: systemtap-4mar/tapset/i686/syscalls.stp
> ===================================================================
> --- systemtap-4mar.orig/tapset/i686/syscalls.stp
> +++ systemtap-4mar/tapset/i686/syscalls.stp
> @@ -119,8 +119,8 @@ probe syscall.set_zone_reclaim.return =
>  #
>  probe syscall.sigaltstack = kernel.function("sys_sigaltstack") {
>  	name = "sigaltstack"
> -	ebx = $ebx
> -	argstr = sprintf("%p", $ebx)
> +	ussp = %( kernel_vr < "2.6.25" %? $ebx %: $bx %)
> +	argstr = sprintf("%p", ussp)
>  }
>  probe syscall.sigaltstack.return = kernel.function("sys_sigaltstack").return {
>  	name = "sigaltstack"

That looks okay.

-Will

Re: changelog files, %( %) idioms

[Ananth N Mavinakayanahalli]

On Thu, Mar 06, 2008 at 09:48:30AM -0500, William Cohen wrote:
> Ananth N Mavinakayanahalli wrote:
>> On Mon, Mar 03, 2008 at 11:48:13AM -0500, William Cohen wrote:
>>> Frank Ch. Eigler wrote:
>>>> Hi -
>>>>
>>>> On Mon, Mar 03, 2008 at 05:48:41PM +0530, Ananth N Mavinakayanahalli wrote:
>>>>> [...]
>>>>>  probe syscall.sigaltstack = kernel.function("sys_sigaltstack") {
>>>>>  	name = "sigaltstack"
>>>>> -	ebx = $ebx
>>>>> -	argstr = sprintf("%p", $ebx)
>>>>> +	bx = %( kernel_vr > "2.6.24" %? $bx %: $ebx %)
>>>>> +	argstr = sprintf("%p", %( kernel_vr > "2.6.24" %? $bx %: $ebx %) )
>>>> I suggest picking a single more informative variable name than "ebx"
>>>> or "bx" for that parameter.  Then that variable could be used as the
>>>> plain sprintf value.
>>>>
>>>> - FChE
>>> I saw this this cause failure on F-9 i686. There could be 2.6.24.1, so 
>>> probably want to check that the kernel is earlier than 2.6.25. Something 
>>> like the following:
>>
>> Actually, this went into 2.6.25-rc
>>
>>> bx = %( kernel_vr < "2.6.25" %? $bx %: $ebx %)
>>>
>>> Could the use of $bx be factored out of argstr, so there is only one 
>>> check for the kernel version for $ebx/$bx rather than two?
>>
>> How does this look?
>>
>> ---
>>  tapset/i686/syscalls.stp |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> Index: systemtap-4mar/tapset/i686/syscalls.stp
>> ===================================================================
>> --- systemtap-4mar.orig/tapset/i686/syscalls.stp
>> +++ systemtap-4mar/tapset/i686/syscalls.stp
>> @@ -119,8 +119,8 @@ probe syscall.set_zone_reclaim.return =
>>  #
>>  probe syscall.sigaltstack = kernel.function("sys_sigaltstack") {
>>  	name = "sigaltstack"
>> -	ebx = $ebx
>> -	argstr = sprintf("%p", $ebx)
>> +	ussp = %( kernel_vr < "2.6.25" %? $ebx %: $bx %)
>> +	argstr = sprintf("%p", ussp)
>>  }
>>  probe syscall.sigaltstack.return = kernel.function("sys_sigaltstack").return {
>>  	name = "sigaltstack"
>
> That looks okay.

Patch committed.

Thanks,
Ananth