Re: [PATCH v7 5/6] usdt: new tests for USDT arg sanity with overlapping pid probes

[Nick Alcock <nick.alcock@oracle.com>]

On 7 Nov 2024, Eugene Loh uttered the following:

> On 11/6/24 06:29, Nick Alcock wrote:
>
>> This generalizes the existing tst.pidprobes.sh to check the args
>> reported by the probe both with and without arg mapping in place.
>>
>> Everything seems to work.
>
> That comment seems to be unnecessary.

I think it's worth noting that all this arg-mapping infrastructure
(mostly entirely untested for fifteen years) still works :)

> Also, is there a "dtrace -lv" test for USDT args?  (I'm not even sure if that question makes sense.)

Yep. See the previous patch. (Multiple, actually, Kris had me add a few
more.)

>> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
>> ---
>>   .../usdt/{tst.pidprobes.sh => pidprobes.sh}   | 56 +++++++++++++++----
>
> Why the renaming?  And, will "./runtest.sh" run pidprobes.sh without any command-line args?

Nope! It's a library now, called by tst.pid*sh, including, uh, the new
tst.pidprobes.sh I meant to keep around and call it. Oops!

>> --- a/test/unittest/usdt/tst.pidprobes.sh
>> +++ b/test/unittest/usdt/pidprobes.sh
>> @@ -5,9 +5,12 @@
>>   # Licensed under the Universal Permissive License v 1.0 as shown at
>>   # http://oss.oracle.com/licenses/upl.
>>   #
>> -# This test verifies that USDT and pid probes can share underlying probes.
>> +# This test verifies various properties of USDT and pid probes sharing
>> +# underlying probes.
>>     dtrace=$1
>> +usdt=$2
>
> Why is there a usdt=$2?  It seems that usdt is always "t" and you'd expect that in test/unittest/usdt.

Oh crap. Sorry sorry this is a bug -- I lost the tst.pidprobes.sh I
meant to add back. Will fix and repush this patch.

-- which reads, in entirety

exec $(dirname $_test)/pidprobes.sh $1 "" ""

>> @@ -75,7 +86,7 @@ fi
>>   # Check that the program output is 0 when the USDT probe is not enabled.
>>   # That is, the PYRAMID_ENTRY_ENABLED() is-enabled checks should not pass.
>>   -./main > main.out
>> +./main standalone > main.out
>
> I'd vote against this change.  Introducing a command-line arg that is not used?  Or, add it as a comment?  But there is already a
> comment.

I... think that was fallout from my attempt to add deferred support that
I forgot to clean out again. Dropped.

>>   echo "my result: 10" > main.out2.expected
>>     if ! diff -q main.out2 main.out2.expected > /dev/null; then
>> -	echo '"my result"' looks wrong when using DTrace
>> +	echo '"my result"' looks wrong
>
> Why?  In particular, "when using DTrace" stands in contrast to the earlier message 'echo '"my result"' looks wrong when not using
> DTrace'.

Sorry! I didn't notice that the previous stanza existed and thought
"well of course we're using DTrace, that's redundant". Dropped.

>>   -# Sort and check.
>> +# Sort and check (dropping any wake-up firings from deferred probing).
>
> I guess I do not get this.  Where does any "dropping" take place?

In the deferred patch that caused massive ructions here and then I tried
to drop by hand very late at night and clearly didn't do as good a job
at as I thought I did. Dropped this reference to, uh, dropping.

>>     sort dtrace.out          > dtrace.out.sorted
>>   sort dtrace.out.expected > dtrace.out.expected.sorted
>> diff --git a/test/unittest/usdt/tst.pidargmap.sh b/test/unittest/usdt/tst.pidargmap.sh
>> new file mode 100755
>> index 000000000000..0c83f8703539
>> --- /dev/null
>> +++ b/test/unittest/usdt/tst.pidargmap.sh
>> @@ -0,0 +1,11 @@
>> +#!/bin/bash
>> +#
>> +# Oracle Linux DTrace.
>> +# Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
>> +# Licensed under the Universal Permissive License v 1.0 as shown at
>> +# http://oss.oracle.com/licenses/upl.
>> +#
>> +# This test verifies that USDT and pid probes that share underlying probes
>> +# do not apply arg mappings (incorrectly) to the pid probes.
>
> How do we verify that mappings do not apply (incorrectly) to pid probes?  Is there supposed to be a check on pid probe args?

Yeah, that's back now, apologies :) verified that it still works, too.
Will send as a followup to this reply in a mo.

-- 
NULL && (void)