Re: [PATCH v6 7/7] selftests/usdt: add is-enabled stapsdt tests using semaphores

[Kris Van Hees <kris.van.hees@oracle.com>]

On Tue, Jul 29, 2025 at 08:37:16AM +0100, Alan Maguire wrote:
> On 29/07/2025 03:07, Kris Van Hees wrote:
> > This test is flawed because you never initialize the semaphores.  If you change
> > the code to assign 0 to then, then the test fails because the probes never
> > fire.  That makes sense because your implementation does not actually do
> 
> Hmm, I don't see this behaviour. If I change the semaphores to be set to
> 0, i.e.
> 
> -unsigned short test_prov_zero_semaphore SEC(".probes");
> -unsigned short test_prov_one_semaphore SEC(".probes");
> -unsigned short test_prov_two_semaphore SEC(".probes");
> -unsigned short test_prov_three_semaphore SEC(".probes");
> -unsigned short test_prov_four_semaphore SEC(".probes");
> -unsigned short test_prov_five_semaphore SEC(".probes");
> -unsigned short test_prov_six_semaphore SEC(".probes");
> -unsigned short test_prov_seven_semaphore SEC(".probes");
> -unsigned short test_prov_eight_semaphore SEC(".probes");
> -unsigned short test_prov_nine_semaphore SEC(".probes");
> -unsigned short test_prov_ten_semaphore SEC(".probes");
> -unsigned short test_prov_eleven_semaphore SEC(".probes");
> -unsigned short test_prov_twelve_semaphore SEC(".probes");
> +unsigned short test_prov_zero_semaphore SEC(".probes") = 0;
> +unsigned short test_prov_one_semaphore SEC(".probes") = 0;
> +unsigned short test_prov_two_semaphore SEC(".probes") = 0;
> +unsigned short test_prov_three_semaphore SEC(".probes") = 0;
> +unsigned short test_prov_four_semaphore SEC(".probes") = 0;
> +unsigned short test_prov_five_semaphore SEC(".probes") = 0;
> +unsigned short test_prov_six_semaphore SEC(".probes") = 0;
> +unsigned short test_prov_seven_semaphore SEC(".probes") = 0;
> +unsigned short test_prov_eight_semaphore SEC(".probes") = 0;
> +unsigned short test_prov_nine_semaphore SEC(".probes") = 0;
> +unsigned short test_prov_ten_semaphore SEC(".probes") = 0;
> +unsigned short test_prov_eleven_semaphore SEC(".probes") = 0;
> +unsigned short test_prov_twelve_semaphore SEC(".probes") = 0;
> 
> ...the test still passes. Are you perhaps initializing the semaphores in
> main()? This has the potential to overwrite the kernel-updated values
> which are incremented when the probes are enabled.

I put the assignment before the SEC(...) which may have caused the semaphore
to perhaps not be created as expected.

Also, do you actually need the SEC(...) for this?  From the look of the code
in the kernel, any offset in the inode would work, and unnless I overlooked it
I also do not see anything in the macros for systemtap that require a specific
section for the semaphore variables.

> > anything with the semaphore, i.e. if the probe it relatees to is enabled, 
> > nothing happens to the semaphore, so while you do store the address of the
> > semaphore in patch 2, it is never used for anything, so this test passing is
> 
> The refcntr address is set in parentheses where the probe is set up -
> see uprobe_create():
> 
> 	if (refcntr_off) {
>                 if (asprintf(&spec, "%s:0x%lx(0x%lx)", mapping_fn, addr,
> refcntr_off) < 0)
>                         return NULL;
>         } else {
>                 if (asprintf(&spec, "%s:0x%lx", mapping_fn, addr) < 0)
>                         return NULL;
>         }
> 
> The parenthesized value is then handed off to the kernel to
> reference-count probe enablings.

Ah, overlooked that :)  My bad.  Sorry about the false alarm on that.

However, there definitely should be tests that also check that when the probe
is *not* enabled, the code is indeed also not executed.

> 
> > actually accidental because the semaphore initial value is (apparently) most
> > commonly *not* 0.
> > 
> > On Mon, Jul 28, 2025 at 05:36:11PM +0100, Alan Maguire wrote:
> >> Is-eanbled probes are implemented using semaphores where the
> >> semaphore address is specified in the ELF notes and passed
> >> in at probe creation time to have the kernel reference-count
> >> probes; this allows us to have argument assembly code that
> >> only gets executed when the stapsdt probe is in use.
> >>
> >> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> >> ---
> >>  .../usdt/tst.stapsdt-notes-isenabled.r        |  14 ++
> >>  .../usdt/tst.stapsdt-notes-isenabled.sh       | 151 ++++++++++++++++++
> >>  2 files changed, 165 insertions(+)
> >>  create mode 100644 test/unittest/usdt/tst.stapsdt-notes-isenabled.r
> >>  create mode 100755 test/unittest/usdt/tst.stapsdt-notes-isenabled.sh
> >>
> >> diff --git a/test/unittest/usdt/tst.stapsdt-notes-isenabled.r b/test/unittest/usdt/tst.stapsdt-notes-isenabled.r
> >> new file mode 100644
> >> index 00000000..db6d18cb
> >> --- /dev/null
> >> +++ b/test/unittest/usdt/tst.stapsdt-notes-isenabled.r
> >> @@ -0,0 +1,14 @@
> >> +test:main:zero
> >> +test:main:one:1
> >> +test:main:two:2:3
> >> +test:main:three:4:5:7
> >> +test:main:four:7:8:9:10
> >> +test:main:five:11:12:13:14:15
> >> +test:main:six:16:17:18:19:20:21
> >> +test:main:seven:22:23:24:25:26:27:28
> >> +test:main:eight:29:30:31:32:33:34:35:36
> >> +test:main:nine:37:38:39:40:41:42:43:44:45
> >> +test:main:ten:46:47:48:49:50:51:52:53:54:55
> >> +test:main:eleven:56:57:58:59:60:61:62:63:64:65
> >> +test:main:twelve:67:68:69:70:71:72:73:74:75:76
> >> +
> >> diff --git a/test/unittest/usdt/tst.stapsdt-notes-isenabled.sh b/test/unittest/usdt/tst.stapsdt-notes-isenabled.sh
> >> new file mode 100755
> >> index 00000000..99fcc844
> >> --- /dev/null
> >> +++ b/test/unittest/usdt/tst.stapsdt-notes-isenabled.sh
> >> @@ -0,0 +1,151 @@
> >> +#!/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 covers all stapsdt probes fired by the STAP_PROBEn macros.
> >> +# Arguments values are checked only for first 10 arguments because
> >> +# there is support for arg0 ... arg9 only at this moment.
> >> +
> >> +if [ $# != 1 ]; then
> >> +	echo expected one argument: '<'dtrace-path'>'
> >> +	exit 2
> >> +fi
> >> +
> >> +dtrace=$1
> >> +CC=/usr/bin/gcc
> >> +CFLAGS="-I${PWD}/test/unittest/usdt"
> >> +
> >> +DIRNAME="$tmpdir/usdt-notes.$$.$RANDOM"
> >> +mkdir -p $DIRNAME
> >> +cd $DIRNAME
> >> +
> >> +cat > test.c <<EOF
> >> +#define _SDT_HAS_SEMAPHORES 1
> >> +#include <sdt_notes.h>
> >> +
> >> +#define SEC(name) __attribute__((section(name), used))
> >> +
> >> +unsigned short test_prov_zero_semaphore SEC(".probes");
> >> +unsigned short test_prov_one_semaphore SEC(".probes");
> >> +unsigned short test_prov_two_semaphore SEC(".probes");
> >> +unsigned short test_prov_three_semaphore SEC(".probes");
> >> +unsigned short test_prov_four_semaphore SEC(".probes");
> >> +unsigned short test_prov_five_semaphore SEC(".probes");
> >> +unsigned short test_prov_six_semaphore SEC(".probes");
> >> +unsigned short test_prov_seven_semaphore SEC(".probes");
> >> +unsigned short test_prov_eight_semaphore SEC(".probes");
> >> +unsigned short test_prov_nine_semaphore SEC(".probes");
> >> +unsigned short test_prov_ten_semaphore SEC(".probes");
> >> +unsigned short test_prov_eleven_semaphore SEC(".probes");
> >> +unsigned short test_prov_twelve_semaphore SEC(".probes");
> >> +
> >> +int
> >> +main(int argc, char **argv)
> >> +{
> >> +	if (test_prov_zero_semaphore)
> >> +		STAP_PROBE(test_prov, zero);
> >> +	if (test_prov_one_semaphore)
> >> +		STAP_PROBE1(test_prov, one, argc);
> >> +	if (test_prov_two_semaphore)
> >> +		STAP_PROBE2(test_prov, two, 2, 3);
> >> +	if (test_prov_three_semaphore)
> >> +		STAP_PROBE3(test_prov, three, 4, 5, 7);
> >> +	if (test_prov_four_semaphore)
> >> +		STAP_PROBE4(test_prov, four, 7, 8, 9, 10);
> >> +	if (test_prov_five_semaphore)
> >> +		STAP_PROBE5(test_prov, five, 11, 12, 13, 14, 15);
> >> +	if (test_prov_six_semaphore)
> >> +		STAP_PROBE6(test_prov, six, 16, 17, 18, 19, 20, 21);
> >> +	if (test_prov_seven_semaphore)
> >> +		STAP_PROBE7(test_prov, seven, 22, 23, 24, 25, 26, 27, 28);
> >> +	if (test_prov_eight_semaphore)
> >> +		STAP_PROBE8(test_prov, eight, 29, 30, 31, 32, 33, 34, 35, 36);
> >> +	if (test_prov_nine_semaphore)
> >> +		STAP_PROBE9(test_prov, nine, 37, 38, 39, 40, 41, 42, 43, 44, 45);
> >> +	if (test_prov_ten_semaphore)
> >> +		STAP_PROBE10(test_prov, ten, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55);
> >> +	if (test_prov_eleven_semaphore)
> >> +		STAP_PROBE11(test_prov, eleven, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66);
> >> +	if (test_prov_twelve_semaphore)
> >> +		STAP_PROBE12(test_prov, twelve, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78);
> >> +}
> >> +EOF
> >> +
> >> +${CC} ${CFLAGS} -o test test.c
> >> +if [ $? -ne 0 ]; then
> >> +	echo "failed to compile test.c" >& 2
> >> +	exit 1
> >> +fi
> >> +
> >> +$dtrace -c ./test -qs /dev/stdin <<EOF
> >> +test_prov\$target:::zero
> >> +{
> >> +	printf("%s:%s:%s\n", probemod, probefunc, probename);
> >> +}
> >> +
> >> +test_prov\$target:::one
> >> +{
> >> +	printf("%s:%s:%s:%li\n", probemod, probefunc, probename, arg0);
> >> +}
> >> +
> >> +test_prov\$target:::two
> >> +{
> >> +	printf("%s:%s:%s:%li:%li\n", probemod, probefunc, probename, arg0, arg1);
> >> +}
> >> +
> >> +test_prov\$target:::three
> >> +{
> >> +	printf("%s:%s:%s:%li:%li:%li\n", probemod, probefunc, probename, arg0, arg1,
> >> +	       arg2);
> >> +}
> >> +
> >> +test_prov\$target:::four
> >> +{
> >> +	printf("%s:%s:%s:%li:%li:%li:%li\n", probemod, probefunc, probename, arg0, arg1,
> >> +	       arg2, arg3);
> >> +}
> >> +
> >> +test_prov\$target:::five
> >> +{
> >> +	printf("%s:%s:%s:%li:%li:%li:%li:%li\n", probemod, probefunc, probename,
> >> +	       arg0, arg1, arg2, arg3, arg4);
> >> +}
> >> +
> >> +test_prov\$target:::six
> >> +{
> >> +	printf("%s:%s:%s:%li:%li:%li:%li:%li:%li\n", probemod, probefunc, probename,
> >> +	       arg0, arg1, arg2, arg3, arg4, arg5);
> >> +}
> >> +
> >> +test_prov\$target:::seven
> >> +{
> >> +	printf("%s:%s:%s:%li:%li:%li:%li:%li:%li:%li\n", probemod, probefunc, probename,
> >> +	       arg0, arg1, arg2, arg3, arg4, arg5, arg6);
> >> +}
> >> +
> >> +test_prov\$target:::eight
> >> +{
> >> +	printf("%s:%s:%s:%li:%li:%li:%li:%li:%li:%li:%li\n", probemod, probefunc, probename,
> >> +	       arg0, arg1, arg2, arg3, arg4, arg5, arg6, arg7);
> >> +}
> >> +
> >> +test_prov\$target:::nine
> >> +{
> >> +	printf("%s:%s:%s:%li:%li:%li:%li:%li:%li:%li:%li:%li\n", probemod, probefunc, probename,
> >> +	       arg0, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8);
> >> +}
> >> +
> >> +test_prov\$target:::ten,
> >> +test_prov\$target:::eleven,
> >> +test_prov\$target:::twelve
> >> +{
> >> +	printf("%s:%s:%s:%li:%li:%li:%li:%li:%li:%li:%li:%li:%li\n", probemod, probefunc, probename,
> >> +	       arg0, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8, arg9);
> >> +}
> >> +EOF
> >> +status=$?
> >> +
> >> +exit $status
> >> -- 
> >> 2.39.3
> >>