Bug 18630

Created attachment 8425 [details]
proposed patch

The testcase comes with two tiny tapset fixes:

- powerpc/registers.stp update discussed in PR18650.
- s390/registers.stp update mentioned in PR18649. That check was
  introduced in commit eefd579b5e and after talking to dsmith
  via IRC, it should be safe to remove it.

I attempted to save testsuite time, so the testcase only calls stap
once. Due to this, the testcase is maybe not very nice to debug.

One thing I am not sure about is that when looking at pointers in
31bit s390x userspace binary (gcc -m31), I seem to see 32 bit pointers,
because when tracing this function call: 'fc6(&i, (int *)-1);' I seem
to get 0xffffffff as the second argument instead of 0x7fffffff.

Does the patch look acceptable?

(In reply to Martin Cermak from comment #1)
> Created attachment 8425 [details]
> proposed patch
> 
> The testcase comes with two tiny tapset fixes:
> 
> - powerpc/registers.stp update discussed in PR18650.
> - s390/registers.stp update mentioned in PR18649. That check was
>   introduced in commit eefd579b5e and after talking to dsmith
>   via IRC, it should be safe to remove it.
> 
> I attempted to save testsuite time, so the testcase only calls stap
> once. Due to this, the testcase is maybe not very nice to debug.

I think that's fine, as long as the full output appears in systemtap.log. If it doesn't, you'll need to add some 'verbose -log $foo" calls to the test case.

> One thing I am not sure about is that when looking at pointers in
> 31bit s390x userspace binary (gcc -m31), I seem to see 32 bit pointers,
> because when tracing this function call: 'fc6(&i, (int *)-1);' I seem
> to get 0xffffffff as the second argument instead of 0x7fffffff.

I'm not 100% sure what is going on there. Certainly I've seen a 32-bit value as a pointer when passing -1, while other times I haven't. That's why in the syscall test cases -1 pointer values for s390 look like "[7]?[f]+" - so that the 7 is optional.

I'm sure there is a reason somewhere. I do certainly believe that if you allocated some memory you'd never get a return value with more than 31-bits as the address.

> Does the patch look acceptable?

In general, the patch looks fine. I do have a question however. The probes look like this:

====
probe process(\"$test_exepath\").function(\"fc1\") {
    printf(\"%s;%x;%x;%x;%d\\n\", ppfunc(), int_arg(1), s32_arg(1), int_arg(2), int_arg(2))
}
====

Why print everything twice? The second set makes some sense because you are printing the value in hex and then decimal, but the 1st set you are printing in hex both times.

(In reply to David Smith from comment #2)
> > I attempted to save testsuite time, so the testcase only calls stap
> > once. Due to this, the testcase is maybe not very nice to debug.
> 
> I think that's fine, as long as the full output appears in systemtap.log. If
> it doesn't, you'll need to add some 'verbose -log $foo" calls to the test
> case.

With default verbosity, systemtap.log looks like this:

=======
Running ./systemtap.syscall/uprobe_nd_params.exp ...                            
Executing on host: gcc /root/mcermak-systemtap/systemtap/testsuite/test.c  -m64  -lm   -o /root/mcermak-systemtap/systemtap/testsuite/test    (timeout = 300)
spawn -ignore SIGHUP gcc /root/mcermak-systemtap/systemtap/testsuite/test.c -m64 -lm -o /root/mcermak-systemtap/systemtap/testsuite/test^M
PASS: compiling test.c compiler=gcc additional_flags=-m64                       
spawn stap /root/mcermak-systemtap/systemtap/testsuite/test.stp -c /root/mcermak-systemtap/systemtap/testsuite/test^M
fc1;7fffffff;7fffffff;ffffffffffffffff;-1^M                                     
fc2;ffffffff;ffffffff;ffffffff;4294967295^M                                     
fc3;7fffffffffffffff;ffffffffffffffff;-1^M                                      
fc4;ffffffffffffffff;ffffffffffffffff;18446744073709551615^M                    
fc5;12345678deadbeef;12345678deadbeef;12345678deadbeef;12345678deadbeef;ffffffffffffffff;18446744073709551615^M
fc6;0x7fff1092f2dc;0xffffffffffffffff^M                                         
PASS: uprobe_nd_params                                                          
Executing on host: gcc /root/mcermak-systemtap/systemtap/testsuite/test.c  -m32  -lm   -o /root/mcermak-systemtap/systemtap/testsuite/test    (timeout = 300)
spawn -ignore SIGHUP gcc /root/mcermak-systemtap/systemtap/testsuite/test.c -m32 -lm -o /root/mcermak-systemtap/systemtap/testsuite/test^M
PASS: compiling test.c compiler=gcc additional_flags=-m32                       
spawn stap /root/mcermak-systemtap/systemtap/testsuite/test.stp -c /root/mcermak-systemtap/systemtap/testsuite/test^M
fc1;7fffffff;7fffffff;ffffffffffffffff;-1^M                                     
fc2;ffffffff;ffffffff;ffffffff;4294967295^M                                     
fc3;7fffffff;ffffffffffffffff;-1^M                                              
fc4;ffffffff;ffffffff;4294967295^M                                              
fc5;12345678deadbeef;12345678deadbeef;12345678deadbeef;12345678deadbeef;ffffffffffffffff;18446744073709551615^M
fc6;0xffa5ae24;0xffffffff^M                                                     
PASS: uprobe_nd_params                                                          
testcase ./systemtap.syscall/uprobe_nd_params.exp completed in 13 seconds          
                                                                                
                === systemtap Summary ===                                       
                                                                                
# of expected passes            4
=======

With e.g. '--debug -v -v -v' one can even see how the expect regexp gates work, so I think it's relatively fine. What do you think?

> 
> > Does the patch look acceptable?
> 
> In general, the patch looks fine. I do have a question however. The probes
> look like this:
> 
> ====
> probe process(\"$test_exepath\").function(\"fc1\") {
>     printf(\"%s;%x;%x;%x;%d\\n\", ppfunc(), int_arg(1), s32_arg(1),
> int_arg(2), int_arg(2))
> }
> ====
> 
> Why print everything twice? The second set makes some sense because you are
> printing the value in hex and then decimal, but the 1st set you are printing
> in hex both times.

The userspace function in question is 'int fc1(int arg1, int arg2)' and its respective call is 'fc1(INT_MAX, -1);'. My thinking is that passing INT_MAX checks whether the max value can be acquired without distorsion, whereas passing -1 is meant to be limit testing. That's for the first and third int_arg() call in the probe. The s32_arg() call basically smoke checks this "alias" function presence (since s32_arg() simply wraps int_arg() in the tapset). And the last int_arg() call in combination with '%d' checks for correct sign extension. Does this make sense?

Created attachment 8431 [details]
proposed patch

(In reply to David Smith from comment #2)
> In general, the patch looks fine. I do have a question however. The probes
> look like this:
> 
> ====
> probe process(\"$test_exepath\").function(\"fc1\") {
>     printf(\"%s;%x;%x;%x;%d\\n\", ppfunc(), int_arg(1), s32_arg(1),
> int_arg(2), int_arg(2))
> }
> ====
> 
> Why print everything twice? The second set makes some sense because you are
> printing the value in hex and then decimal, but the 1st set you are printing
> in hex both times.

I've updated the testcase. Does it look good now?

(In reply to Martin Cermak from comment #4)
> Created attachment 8431 [details]
> proposed patch
> 
> (In reply to David Smith from comment #2)
> > In general, the patch looks fine. I do have a question however. The probes
> > look like this:
> > 
> > ====
> > probe process(\"$test_exepath\").function(\"fc1\") {
> >     printf(\"%s;%x;%x;%x;%d\\n\", ppfunc(), int_arg(1), s32_arg(1),
> > int_arg(2), int_arg(2))
> > }
> > ====
> > 
> > Why print everything twice? The second set makes some sense because you are
> > printing the value in hex and then decimal, but the 1st set you are printing
> > in hex both times.
> 
> I've updated the testcase. Does it look good now?

It looks fine to me. Thanks a bunch for the test.

Fixed in commit 546ac814c3b6cbf96cef9a0c2cabb2b39b296d20