[PATCH] dtprobed: handle multiple providers in a single piece of DOF

[PATCH] dtprobed: handle multiple providers in a single piece of DOF

[Nick Alcock]

A given ELF object can of course contain as many providers as you like.  The
DOF stash format, DOF stash management code and DTrace itself are all set up
fine to handle DOF with multiple providers in: but, alas, the code that
reads the DOF from the parser child in response to an ioctl() assumes that
one provider is all it will ever see.

This is not ideal, but rejigging it introduces an interesting problem: when
do you *stop* reading providers from the parser stream?  Right now,
dof_parser.c loops over all of its providers and emits them until all are
done, then just stops: there is no indication of how many providers there
are or that this is the last provider or anything (though if there are none,
it does try to send an empty provider to avoid the caller blocking waiting
for a reply that will never come).

It's a bit annoying to try to figure out how many providers there are in
advance, so instead of that, take a route that's easier both to emit and
parse, drop the empty provider stuff, and simply add a DIT_EOF record which
indicates "no more providers expected", which is always sent last.  (We
stuff this in before DIT_ARGS_* in the enum because it makes more conceptual
sense right after DIT_ERR, and DIT_ARGS_* hasn't released anywhere yet.)

With that in place it's a simple matter to loop adding parsed DOF to the
stash's accumulator until we hit an EOF record.  Memory management is
complicated a little, because a single parsed buffer (reply from dof_parser)
is now owned by multiple accumulator records in the stash: but every one of
those is terminated by a DIT_EOF, which appears nowhere else, so we can just
not free a parsed buffer unless it's of type DIT_EOF.

Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
---
 dtprobed/dof_stash.c                        | 12 +++-
 dtprobed/dtprobed.c                         | 63 ++++++++++++++-------
 libcommon/dof_parser.c                      | 23 +++-----
 libcommon/dof_parser.h                      |  8 ++-
 test/triggers/Build                         |  6 +-
 test/triggers/usdt-tst-multiprovider-prov.d | 12 ++++
 test/triggers/usdt-tst-multiprovider.c      | 22 +++++++
 test/unittest/usdt/tst.multiprovider.r      |  3 +
 test/unittest/usdt/tst.multiprovider.r.p    |  2 +
 test/unittest/usdt/tst.multiprovider.sh     | 15 +++++
 10 files changed, 124 insertions(+), 42 deletions(-)
 create mode 100644 test/triggers/usdt-tst-multiprovider-prov.d
 create mode 100644 test/triggers/usdt-tst-multiprovider.c
 create mode 100644 test/unittest/usdt/tst.multiprovider.r
 create mode 100755 test/unittest/usdt/tst.multiprovider.r.p
 create mode 100755 test/unittest/usdt/tst.multiprovider.sh

diff --git a/dtprobed/dof_stash.c b/dtprobed/dof_stash.c
index 82fdd3174759d..296987ad41e93 100644
--- a/dtprobed/dof_stash.c
+++ b/dtprobed/dof_stash.c
@@ -360,7 +360,13 @@ dof_stash_free(dt_list_t *accum)
 	for (accump = dt_list_next(accum); accump != NULL;
 	     accump = dt_list_next(accump)) {
 		dt_list_delete(accum, accump);
-		free(accump->parsed);
+
+		/*
+		 * All parsed memory regions are terminated by an EOF, so once
+		 * we encounter the EOF, this region can safely be freed.
+		 */
+		if (accump->parsed->type == DIT_EOF)
+			free(accump->parsed);
 		free(last_accump);
 		last_accump = accump;
 	}
@@ -656,6 +662,10 @@ dof_stash_write_parsed(pid_t pid, dev_t dev, ino_t ino, dt_list_t *accum)
 			fuse_log(FUSE_LOG_ERR, "dtprobed: parser error: %s\n",
 				 accump->parsed->err.err);
 			goto err_provider_close;
+		/* EOF: nothing to do.  No need to record this parser-stream
+		 * implementation detail into the parsed representation.  */
+		case DIT_EOF:
+			break;
 
 		default:
 			/*
diff --git a/dtprobed/dtprobed.c b/dtprobed/dtprobed.c
index 2ca39b26f88a9..86865eb467b67 100644
--- a/dtprobed/dtprobed.c
+++ b/dtprobed/dtprobed.c
@@ -772,41 +772,60 @@ process_dof(pid_t pid, int out, int in, dev_t dev, ino_t inum, dev_t exec_dev,
 			dof_parser_tidy(1);
 			continue;
 		}
-		if (provider->type != DIT_PROVIDER)
+		if (provider->type != DIT_PROVIDER && provider->type != DIT_EOF)
 			goto err;
 		break;
 	} while (!provider);
 
-	if (dof_stash_push_parsed(&accum, provider) < 0)
-		goto oom;
-
-	for (i = 0; i < provider->provider.nprobes; i++) {
-		dof_parsed_t *probe = dof_read(pid, in);
-		size_t j;
-
-		errmsg = "no probes in this provider, or parse state corrupt";
-		if (!probe || probe->type != DIT_PROBE)
-			goto err;
-
-		if (dof_stash_push_parsed(&accum, probe) < 0)
+	while (provider->type != DIT_EOF) {
+		if (dof_stash_push_parsed(&accum, provider) < 0)
 			goto oom;
 
-		j = 0;
-		do {
-			dof_parsed_t *tp = dof_read(pid, in);
+		fuse_log(FUSE_LOG_DEBUG, "Parser read: provider %s, %i probes\n",
+			 provider->provider.name, provider->provider.nprobes);
 
-			errmsg = "no tracepoints in a probe, or parse state corrupt";
-			if (!tp || tp->type == DIT_PROVIDER || tp->type == DIT_PROBE)
+		for (i = 0; i < provider->provider.nprobes; i++) {
+			dof_parsed_t *probe = dof_read(pid, in);
+			size_t j;
+
+			errmsg = "no probes in this provider, or parse state corrupt";
+			if (!probe || probe->type != DIT_PROBE)
 				goto err;
 
-			if (dof_stash_push_parsed(&accum, tp) < 0)
+			if (dof_stash_push_parsed(&accum, probe) < 0)
 				goto oom;
 
-			if (tp->type == DIT_TRACEPOINT)
-				j++;
-		} while (j < probe->probe.ntp);
+			j = 0;
+			do {
+				dof_parsed_t *tp = dof_read(pid, in);
+
+				errmsg = "no tracepoints in a probe, or parse state corrupt";
+				if (!tp || tp->type == DIT_PROVIDER ||
+				    tp->type == DIT_PROBE || tp->type == DIT_EOF)
+					goto err;
+
+				fuse_log(FUSE_LOG_DEBUG, "Parser read: adding %s:%s to stash\n",
+					 provider->provider.name, probe->probe.name);
+
+				if (dof_stash_push_parsed(&accum, tp) < 0)
+					goto oom;
+
+				if (tp->type == DIT_TRACEPOINT)
+					j++;
+			} while (j < probe->probe.ntp);
+		}
+
+		errmsg = "subsequent provider read failed, or stream not properly terminated";
+		provider = dof_read(pid, in);
+		if (!provider)
+			goto err;
 	}
 
+	/* Push the EOF on.  */
+
+	if (dof_stash_push_parsed(&accum, provider) < 0)
+		goto oom;
+
 	if (!reparsing)
 		if ((gen = dof_stash_add(pid, dev, inum, exec_dev, exec_inum, dh,
 					 in_buf, in_bufsz)) < 0)
diff --git a/libcommon/dof_parser.c b/libcommon/dof_parser.c
index 1792a8bfcc849..ae5abe5220929 100644
--- a/libcommon/dof_parser.c
+++ b/libcommon/dof_parser.c
@@ -1145,7 +1145,7 @@ dof_parse(int out, dof_helper_t *dhp, dof_hdr_t *dof)
 {
 	int			i, rv;
 	uintptr_t		daddr = (uintptr_t)dof;
-	int			count = 0;
+	dof_parsed_t		eof;
 
 	dt_dbg_dof("DOF 0x%p from helper {'%s', %p, %p}...\n",
 		   dof, dhp ? dhp->dofhp_mod : "<none>", dhp, dof);
@@ -1157,8 +1157,7 @@ dof_parse(int out, dof_helper_t *dhp, dof_hdr_t *dof)
 	}
 
 	/*
-	 * Look for helper providers, validate their descriptions, and
-	 * parse them.
+	 * Look for providers, validate their descriptions, and parse them.
 	 */
 	if (dhp != NULL) {
 		dt_dbg_dof("  DOF 0x%p Validating and parsing providers...\n", dof);
@@ -1177,25 +1176,19 @@ dof_parse(int out, dof_helper_t *dhp, dof_hdr_t *dof)
 				dof_destroy(dhp, dof);
 				return;
 			}
-			count++;
 			emit_provider(out, dhp, dof, sec);
 		}
 	}
 
 	/*
-	 * If nothing was written, emit an empty result to wake up
-	 * the caller.
+	 * Always emit an EOF, to wake up the caller if nothing else, but also
+	 * to notify the caller that there are no more providers to read.
 	 */
-	if (count == 0) {
-		dof_parsed_t empty;
+	memset(&eof, 0, sizeof(dof_parsed_t));
 
-		memset(&empty, 0, sizeof(dof_parsed_t));
-
-		empty.size = offsetof(dof_parsed_t, provider.name);
-		empty.type = DIT_PROVIDER;
-		empty.provider.nprobes = 0;
-		dof_parser_write_one(out, &empty, empty.size);
-	}
+	eof.size = offsetof(dof_parsed_t, provider.nprobes);
+	eof.type = DIT_EOF;
+	dof_parser_write_one(out, &eof, eof.size);
 
 	dof_destroy(dhp, dof);
 }
diff --git a/libcommon/dof_parser.h b/libcommon/dof_parser.h
index 8f42d00551e3f..75aa3082161b7 100644
--- a/libcommon/dof_parser.h
+++ b/libcommon/dof_parser.h
@@ -24,6 +24,7 @@
  *     DIT_ARGS_XLAT (1, optional)
  *     DIT_ARGS_MAP (1, optional)
  *     DIT_TRACEPOINT (any number >= 1)
+ * DIT_EOF (no more providers, last record)
  *
  * The dof_parsed.provider.flags word indicates the presence of the
  * various optional args records in the following stream (you can rely on
@@ -37,9 +38,10 @@ typedef enum dof_parsed_info {
 	DIT_PROBE = 1,
 	DIT_TRACEPOINT = 2,
 	DIT_ERR = 3,
-	DIT_ARGS_NATIVE = 4,
-	DIT_ARGS_XLAT = 5,
-	DIT_ARGS_MAP = 6,
+	DIT_EOF = 4,
+	DIT_ARGS_NATIVE = 5,
+	DIT_ARGS_XLAT = 6,
+	DIT_ARGS_MAP = 7,
 } dof_parsed_info_t;
 
 /*
diff --git a/test/triggers/Build b/test/triggers/Build
index 107b3b4d1f2e8..c50c265ccec7d 100644
--- a/test/triggers/Build
+++ b/test/triggers/Build
@@ -13,7 +13,8 @@ EXTERNAL_64BIT_TRIGGERS = testprobe readwholedir mmap bogus-ioctl open delaydie
     ustack-tst-spin ustack-tst-mtspin \
     visible-constructor visible-constructor-static visible-constructor-static-unstripped
 
-EXTERNAL_64BIT_SDT_TRIGGERS = usdt-tst-argmap usdt-tst-args usdt-tst-forker usdt-tst-defer usdt-tst-special
+EXTERNAL_64BIT_SDT_TRIGGERS = usdt-tst-argmap usdt-tst-args usdt-tst-forker usdt-tst-defer \
+    usdt-tst-multiprovider usdt-tst-special
 EXTERNAL_64BIT_TRIGGERS += $(EXTERNAL_64BIT_SDT_TRIGGERS)
 
 EXTERNAL_32BIT_TRIGGERS := visible-constructor-32
@@ -206,6 +207,9 @@ usdt-tst-forker_PROV := usdt-tst-forker-prov.d
 # usdt-tst-defer calls USDT probes based on dtrace -h
 usdt-tst-defer_PROV := usdt-tst-defer-prov.d
 
+# usdt-tst-multiprovider calls USDT probes based on dtrace -h
+usdt-tst-multiprovider_PROV := usdt-tst-multiprovider-prov.d
+
 # usdt-tst-special calls USDT probes based on dtrace -h
 usdt-tst-special_CFLAGS := -fno-inline -O2
 usdt-tst-special_PROV := usdt-tst-special-prov.d
diff --git a/test/triggers/usdt-tst-multiprovider-prov.d b/test/triggers/usdt-tst-multiprovider-prov.d
new file mode 100644
index 0000000000000..cfa8fab23233e
--- /dev/null
+++ b/test/triggers/usdt-tst-multiprovider-prov.d
@@ -0,0 +1,12 @@
+/*
+ * 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.
+ */
+
+/* @@skip: provider declaration - not a test */
+
+provider prova { probe entrya(); };
+provider provb { probe entryb(); };
+provider provc { probe entryc(); };
diff --git a/test/triggers/usdt-tst-multiprovider.c b/test/triggers/usdt-tst-multiprovider.c
new file mode 100644
index 0000000000000..430c0cdd2cf4d
--- /dev/null
+++ b/test/triggers/usdt-tst-multiprovider.c
@@ -0,0 +1,22 @@
+/*
+ * 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.
+ */
+
+/*
+ * A trigger with multiple providers in it.
+ */
+#include <stdio.h>
+#include <unistd.h>
+#include "usdt-tst-multiprovider-prov.h"
+
+int main(int argc, char **argv)
+{
+	PROVA_ENTRYA();
+	PROVB_ENTRYB();
+	PROVC_ENTRYC();
+	usleep(5 * 1000 * 1000);
+	return 0;
+}
diff --git a/test/unittest/usdt/tst.multiprovider.r b/test/unittest/usdt/tst.multiprovider.r
new file mode 100644
index 0000000000000..b83f559a7e7ff
--- /dev/null
+++ b/test/unittest/usdt/tst.multiprovider.r
@@ -0,0 +1,3 @@
+ID provaPID usdt-tst-multiprovider main entrya
+ID provbPID usdt-tst-multiprovider main entryb
+ID provcPID usdt-tst-multiprovider main entryc
diff --git a/test/unittest/usdt/tst.multiprovider.r.p b/test/unittest/usdt/tst.multiprovider.r.p
new file mode 100755
index 0000000000000..ae8493e3d5be4
--- /dev/null
+++ b/test/unittest/usdt/tst.multiprovider.r.p
@@ -0,0 +1,2 @@
+#!/bin/sh
+grep -v '^ *ID' | sed 's,^[0-9]*,ID,; s,prov\(.\)[0-9]*,prov\1PID,; s,  *, ,g'
diff --git a/test/unittest/usdt/tst.multiprovider.sh b/test/unittest/usdt/tst.multiprovider.sh
new file mode 100755
index 0000000000000..d5b72c2be77ec
--- /dev/null
+++ b/test/unittest/usdt/tst.multiprovider.sh
@@ -0,0 +1,15 @@
+#!/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.
+#
+if [ $# != 1 ]; then
+	echo expected one argument: '<'dtrace-path'>'
+	exit 2
+fi
+
+dtrace=$1
+
+exec $dtrace $dt_flags -l -P 'prov*' -c `pwd`/test/triggers/usdt-tst-multiprovider

base-commit: 68d14f30b5ffaaea405354a2edf44279127d209a
-- 
2.46.0.278.g36e3a12567

Re: [PATCH] dtprobed: handle multiple providers in a single piece of DOF

[Eugene Loh]

I'm not familiar with this code, but here is some amateur feedback...

On 11/14/24 11:03, Nick Alcock wrote:
> A given ELF object can of course contain as many providers as you like.  The
> DOF stash format, DOF stash management code and DTrace itself are all set up
> fine to handle DOF with multiple providers in: but, alas, the code that
> reads the DOF from the parser child in response to an ioctl() assumes that
> one provider is all it will ever see.
>
> This is not ideal, but rejigging it introduces an interesting problem: when
> do you *stop* reading providers from the parser stream?  Right now,
> dof_parser.c loops over all of its providers and emits them until all are
> done, then just stops: there is no indication of how many providers there
> are or that this is the last provider or anything (though if there are none,
> it does try to send an empty provider to avoid the caller blocking waiting
> for a reply that will never come).
>
> It's a bit annoying to try to figure out how many providers there are in
> advance, so instead of that, take a route that's easier both to emit and
> parse, drop the empty provider stuff, and simply add a DIT_EOF record which
> indicates "no more providers expected", which is always sent last.  (We
> stuff this in before DIT_ARGS_* in the enum because it makes more conceptual
> sense right after DIT_ERR, and DIT_ARGS_* hasn't released anywhere yet.)
>
> With that in place it's a simple matter to loop adding parsed DOF to the

I'm not sure what this is saying, but how about adding a comma right 
after "loop"?

> stash's accumulator until we hit an EOF record.  Memory management is
> complicated a little, because a single parsed buffer (reply from dof_parser)
> is now owned by multiple accumulator records in the stash: but every one of
> those is terminated by a DIT_EOF, which appears nowhere else, so we can just
> not free a parsed buffer unless it's of type DIT_EOF.
>
> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
>
> diff --git a/libcommon/dof_parser.h b/libcommon/dof_parser.h
> @@ -24,6 +24,7 @@
>    *     DIT_ARGS_XLAT (1, optional)
>    *     DIT_ARGS_MAP (1, optional)
>    *     DIT_TRACEPOINT (any number >= 1)
> + * DIT_EOF (no more providers, last record)
>    *
>    * The dof_parsed.provider.flags word indicates the presence of the
>    * various optional args records in the following stream (you can rely on

The new line has indentation inconsistent with the pre-existing lines.

> diff --git a/test/triggers/Build b/test/triggers/Build
> @@ -13,7 +13,8 @@ EXTERNAL_64BIT_TRIGGERS = testprobe readwholedir mmap bogus-ioctl open delaydie
>       ustack-tst-spin ustack-tst-mtspin \
>       visible-constructor visible-constructor-static visible-constructor-static-unstripped
>   
> -EXTERNAL_64BIT_SDT_TRIGGERS = usdt-tst-argmap usdt-tst-args usdt-tst-forker usdt-tst-defer usdt-tst-special
> +EXTERNAL_64BIT_SDT_TRIGGERS = usdt-tst-argmap usdt-tst-args usdt-tst-forker usdt-tst-defer \
> +    usdt-tst-multiprovider usdt-tst-special
>   EXTERNAL_64BIT_TRIGGERS += $(EXTERNAL_64BIT_SDT_TRIGGERS)
>   
>   EXTERNAL_32BIT_TRIGGERS := visible-constructor-32

Okay.  Introducing a new test/triggers that is used by only one test 
is... well, I guess it's a judgment call.

> diff --git a/test/triggers/usdt-tst-multiprovider-prov.d b/test/triggers/usdt-tst-multiprovider-prov.d
> new file mode 100644
> @@ -0,0 +1,12 @@
> +/*
> + * 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.
> + */
> +
> +/* @@skip: provider declaration - not a test */

I don't think the @@skip is needed for something inside test/triggers.

> +
> +provider prova { probe entrya(); };
> +provider provb { probe entryb(); };
> +provider provc { probe entryc(); };

We should stress test more:
*)   multiple probes per provider
*)   some probes with same and different names as in other providers
*)   probe args (including probes in different providers handling args 
differently)


> diff --git a/test/triggers/usdt-tst-multiprovider.c b/test/triggers/usdt-tst-multiprovider.c
> new file mode 100644
> index 0000000000000..430c0cdd2cf4d
> --- /dev/null
> +++ b/test/triggers/usdt-tst-multiprovider.c
> @@ -0,0 +1,22 @@
> +/*
> + * 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.
> + */
> +
> +/*
> + * A trigger with multiple providers in it.
> + */
> +#include <stdio.h>

stdio.h not needed

> +#include <unistd.h>

only needed for usleep()

> +#include "usdt-tst-multiprovider-prov.h"
> +
> +int main(int argc, char **argv)
> +{
> +	PROVA_ENTRYA();
> +	PROVB_ENTRYB();
> +	PROVC_ENTRYC();
> +	usleep(5 * 1000 * 1000);

usleep not needed and just taking up test time.  It was originally in 
some tests that were checking /run/dtrace "manually".

> +	return 0;
> +}
> diff --git a/test/unittest/usdt/tst.multiprovider.r b/test/unittest/usdt/tst.multiprovider.r
> new file mode 100644
> index 0000000000000..b83f559a7e7ff
> --- /dev/null
> +++ b/test/unittest/usdt/tst.multiprovider.r
> @@ -0,0 +1,3 @@
> +ID provaPID usdt-tst-multiprovider main entrya
> +ID provbPID usdt-tst-multiprovider main entryb
> +ID provcPID usdt-tst-multiprovider main entryc
> diff --git a/test/unittest/usdt/tst.multiprovider.r.p b/test/unittest/usdt/tst.multiprovider.r.p
> new file mode 100755
> index 0000000000000..ae8493e3d5be4
> --- /dev/null
> +++ b/test/unittest/usdt/tst.multiprovider.r.p
> @@ -0,0 +1,2 @@
> +#!/bin/sh
> +grep -v '^ *ID' | sed 's,^[0-9]*,ID,; s,prov\(.\)[0-9]*,prov\1PID,; s,  *, ,g'

Okay, though the "grep -v" would be clearer to me if it checked  '^ *ID 
*PROVIDER *MODULE *FUNCTION *NAME$'.

This whole thing would be clearer to me as a single awk, but maybe 
that's just me.

Comments might be nice.  I'm not sure how comfortably people read 
regex...  I need to concentrate hard to figure it out.

> diff --git a/test/unittest/usdt/tst.multiprovider.sh b/test/unittest/usdt/tst.multiprovider.sh
> new file mode 100755
> index 0000000000000..d5b72c2be77ec
> --- /dev/null
> +++ b/test/unittest/usdt/tst.multiprovider.sh
> @@ -0,0 +1,15 @@
> +#!/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.
> +#
> +if [ $# != 1 ]; then
> +	echo expected one argument: '<'dtrace-path'>'
> +	exit 2
> +fi
> +
> +dtrace=$1
> +
> +exec $dtrace $dt_flags -l -P 'prov*' -c `pwd`/test/triggers/usdt-tst-multiprovider

Okay, but there should also be a test that actually fires probes.

Re: [PATCH] dtprobed: handle multiple providers in a single piece of DOF

[Nick Alcock]

On 14 Nov 2024, Eugene Loh told this:

> I'm not familiar with this code, but here is some amateur feedback...

:)

> On 11/14/24 11:03, Nick Alcock wrote:
>> diff --git a/libcommon/dof_parser.h b/libcommon/dof_parser.h
>> @@ -24,6 +24,7 @@
>>    *     DIT_ARGS_XLAT (1, optional)
>>    *     DIT_ARGS_MAP (1, optional)
>>    *     DIT_TRACEPOINT (any number >= 1)
>> + * DIT_EOF (no more providers, last record)
>>    *
>>    * The dof_parsed.provider.flags word indicates the presence of the
>>    * various optional args records in the following stream (you can rely on
>
> The new line has indentation inconsistent with the pre-existing lines.

Intentional. The full context might make this clearer:

 * DIT_PROVIDER (at least 1, which contains...)
 *   DIT_PROBE (at least 1, each of which has...)
 *     DIT_ARGS_NATIVE (1, optional)
 *     DIT_ARGS_XLAT (1, optional)
 *     DIT_ARGS_MAP (1, optional)
 *     DIT_TRACEPOINT (any number >= 1)
 * DIT_EOF (no more providers, last record)

i.e. this is showing that "this is at the level of the DIT_PROVIDER
(after all DIT_PROVIDERs) through indentation. It is *not* at the level
of the DIT_TRACEPOINT.

>> diff --git a/test/triggers/Build b/test/triggers/Build
>> @@ -13,7 +13,8 @@ EXTERNAL_64BIT_TRIGGERS = testprobe readwholedir mmap bogus-ioctl open delaydie
>>       ustack-tst-spin ustack-tst-mtspin \
>>       visible-constructor visible-constructor-static visible-constructor-static-unstripped
>>   -EXTERNAL_64BIT_SDT_TRIGGERS = usdt-tst-argmap usdt-tst-args usdt-tst-forker usdt-tst-defer usdt-tst-special
>> +EXTERNAL_64BIT_SDT_TRIGGERS = usdt-tst-argmap usdt-tst-args usdt-tst-forker usdt-tst-defer \
>> +    usdt-tst-multiprovider usdt-tst-special
>>   EXTERNAL_64BIT_TRIGGERS += $(EXTERNAL_64BIT_SDT_TRIGGERS)
>>     EXTERNAL_32BIT_TRIGGERS := visible-constructor-32
>
> Okay.  Introducing a new test/triggers that is used by only one test is... well, I guess it's a judgment call.

It makes the test way simpler :) and if we do add another test that
actually fires these probes, it won't be used by only one test.

>> diff --git a/test/triggers/usdt-tst-multiprovider-prov.d b/test/triggers/usdt-tst-multiprovider-prov.d
>> new file mode 100644
>> @@ -0,0 +1,12 @@
>> +/*
>> + * 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.
>> + */
>> +
>> +/* @@skip: provider declaration - not a test */
>
> I don't think the @@skip is needed for something inside test/triggers.

I copied it from other things in test/triggers just in case. You're
probably right. I'll submit another patch dropping it from everything in
test/triggers.

(I think it originally appeared in usdt-tst-forker-prov.d, and
replicated from there.)

>> +
>> +provider prova { probe entrya(); };
>> +provider provb { probe entryb(); };
>> +provider provc { probe entryc(); };
>
> We should stress test more:
> *)   multiple probes per provider

See v2 of this series! this had... er... fallout.

> *)   some probes with same and different names as in other providers
> *)   probe args (including probes in different providers handling args differently)

I don't really see how this could possibly go wrong -- nothing after the
mid-parser stage can possibly have cross-provider interference as far as
I can see, and that stage doesn't know or care about any of this stuff.

But ICBW...

>> diff --git a/test/triggers/usdt-tst-multiprovider.c b/test/triggers/usdt-tst-multiprovider.c
>> new file mode 100644
>> index 0000000000000..430c0cdd2cf4d
>> --- /dev/null
>> +++ b/test/triggers/usdt-tst-multiprovider.c
>> @@ -0,0 +1,22 @@
>> +/*
>> + * 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.
>> + */
>> +
>> +/*
>> + * A trigger with multiple providers in it.
>> + */
>> +#include <stdio.h>
>
> stdio.h not needed

Agreed.

>> +#include <unistd.h>
>
> only needed for usleep()

... ack.

>> +#include "usdt-tst-multiprovider-prov.h"
>> +
>> +int main(int argc, char **argv)
>> +{
>> +	PROVA_ENTRYA();
>> +	PROVB_ENTRYB();
>> +	PROVC_ENTRYC();
>> +	usleep(5 * 1000 * 1000);
>
> usleep not needed and just taking up test time.  It was originally in some tests that were checking /run/dtrace "manually".

Ahhh. Dropped.

>> +	return 0;
>> +}
>> diff --git a/test/unittest/usdt/tst.multiprovider.r b/test/unittest/usdt/tst.multiprovider.r
>> new file mode 100644
>> index 0000000000000..b83f559a7e7ff
>> --- /dev/null
>> +++ b/test/unittest/usdt/tst.multiprovider.r
>> @@ -0,0 +1,3 @@
>> +ID provaPID usdt-tst-multiprovider main entrya
>> +ID provbPID usdt-tst-multiprovider main entryb
>> +ID provcPID usdt-tst-multiprovider main entryc
>> diff --git a/test/unittest/usdt/tst.multiprovider.r.p b/test/unittest/usdt/tst.multiprovider.r.p
>> new file mode 100755
>> index 0000000000000..ae8493e3d5be4
>> --- /dev/null
>> +++ b/test/unittest/usdt/tst.multiprovider.r.p
>> @@ -0,0 +1,2 @@
>> +#!/bin/sh
>> +grep -v '^ *ID' | sed 's,^[0-9]*,ID,; s,prov\(.\)[0-9]*,prov\1PID,; s,  *, ,g'
>
> Okay, though the "grep -v" would be clearer to me if it checked  '^ *ID *PROVIDER *MODULE *FUNCTION *NAME$'.

Why? We're not dependent on the format of the header, we just want to
get rid of it. Having the test fail because the header we don't care
about changed layout seems wrong.

I might just drop the first line with tail -n +1...

> This whole thing would be clearer to me as a single awk, but maybe that's just me.

How? Piles of gmatches?

> Comments might be nice.  I'm not sure how comfortably people read regex...  I need to concentrate hard to figure it out.

Honestly this is just doing some "replace changing numbers" stuff like a
hundred other .p's in the testsuite. Looking at the .r makes it obvious
what it's up to.

>> diff --git a/test/unittest/usdt/tst.multiprovider.sh b/test/unittest/usdt/tst.multiprovider.sh
>> new file mode 100755
>> index 0000000000000..d5b72c2be77ec
>> --- /dev/null
>> +++ b/test/unittest/usdt/tst.multiprovider.sh
>> @@ -0,0 +1,15 @@
>> +#!/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.
>> +#
>> +if [ $# != 1 ]; then
>> +	echo expected one argument: '<'dtrace-path'>'
>> +	exit 2
>> +fi
>> +
>> +dtrace=$1
>> +
>> +exec $dtrace $dt_flags -l -P 'prov*' -c `pwd`/test/triggers/usdt-tst-multiprovider
>
> Okay, but there should also be a test that actually fires probes.

... why? What we care about is that DTrace can see the probes properly
(since we already know from other tests that once it can do that,
everything else works), and -l is an easy way to verify that (and in
general exercise the entire pathway up to USDT probe discovery in dtrace
itself) without having to get tangled up in even *more* possible bugs
around probe firing. I hit enough bugs in the parser writing this as it
was...

I mean I can write one but I don't see what it's going to tell us.

-- 
NULL && (void)

Re: [PATCH] dtprobed: handle multiple providers in a single piece of DOF

[Eugene Loh]

On 11/14/24 15:38, Nick Alcock wrote:

> On 14 Nov 2024, Eugene Loh told this:
>
>> On 11/14/24 11:03, Nick Alcock wrote:
>>> diff --git a/test/unittest/usdt/tst.multiprovider.r.p b/test/unittest/usdt/tst.multiprovider.r.p
>>> new file mode 100755
>>> index 0000000000000..ae8493e3d5be4
>>> --- /dev/null
>>> +++ b/test/unittest/usdt/tst.multiprovider.r.p
>>> @@ -0,0 +1,2 @@
>>> +#!/bin/sh
>>> +grep -v '^ *ID' | sed 's,^[0-9]*,ID,; s,prov\(.\)[0-9]*,prov\1PID,; s,  *, ,g'
>> Okay, though the "grep -v" would be clearer to me if it checked  '^ *ID *PROVIDER *MODULE *FUNCTION *NAME$'.
> Why? We're not dependent on the format of the header, we just want to
> get rid of it.

It just seemed to me easier to figure out what was going on.  It looked 
like a recognizable header.

What do you think of ' *PROVIDER *'?  The 'ID' is short and strikes me 
as especially confusing if one looks at the post-processed output, which 
has 'ID' all over the place.

> Having the test fail because the header we don't care
> about changed layout seems wrong.
>
> I might just drop the first line with tail -n +1...
>
>> This whole thing would be clearer to me as a single awk, but maybe that's just me.
> How? Piles of gmatches?

gsub() or something.  Anyhow, your choice.

>> Comments might be nice.  I'm not sure how comfortably people read regex...  I need to concentrate hard to figure it out.
> Honestly this is just doing some "replace changing numbers" stuff like a
> hundred other .p's in the testsuite. Looking at the .r makes it obvious
> what it's up to.

No big deal either way (e.g., adding one-line comment).

>>> diff --git a/test/unittest/usdt/tst.multiprovider.sh b/test/unittest/usdt/tst.multiprovider.sh
>>> new file mode 100755
>>> index 0000000000000..d5b72c2be77ec
>>> --- /dev/null
>>> +++ b/test/unittest/usdt/tst.multiprovider.sh
>>> @@ -0,0 +1,15 @@
>>> +#!/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.
>>> +#
>>> +if [ $# != 1 ]; then
>>> +	echo expected one argument: '<'dtrace-path'>'
>>> +	exit 2
>>> +fi
>>> +
>>> +dtrace=$1
>>> +
>>> +exec $dtrace $dt_flags -l -P 'prov*' -c `pwd`/test/triggers/usdt-tst-multiprovider
>> Okay, but there should also be a test that actually fires probes.
> ... why? What we care about is that DTrace can see the probes properly
> (since we already know from other tests that once it can do that,
> everything else works), and -l is an easy way to verify that (and in
> general exercise the entire pathway up to USDT probe discovery in dtrace
> itself) without having to get tangled up in even *more* possible bugs
> around probe firing. I hit enough bugs in the parser writing this as it
> was...
>
> I mean I can write one but I don't see what it's going to tell us.

I'm less concerned about the bugs we can foresee.  I'm more concerned 
about the bugs we don't foresee.

I'm even interested in the case where probes in different providers have 
the same name.

Different subject:  do we have a test for "dtrace -lv" on USDT typed args?

Re: [PATCH] dtprobed: handle multiple providers in a single piece of DOF

[Eugene Loh]

I'm losing track of versions, but at some point I think your patch fixed 
the (same) problem with a different script I was trying:


$ cat x.d
#pragma D attributes Evolving/Evolving/Common provider myprov provider
#pragma D attributes Private/Private/Unknown provider myprov module
#pragma D attributes Private/Private/Unknown provider myprov function
#pragma D attributes Evolving/Evolving/Common provider myprov name
#pragma D attributes Evolving/Evolving/Common provider myprov args
provider myprov
{
     probe entry();
};
$ build/run-dtrace -h -s x.d


I had been getting a segfault (like you).  Now it's simply:

dtrace: failed to compile script x.d: line 5: unused #pragma attributes

Re: [PATCH] dtprobed: handle multiple providers in a single piece of DOF

[Nick Alcock]

On 14 Nov 2024, Eugene Loh said:

> On 11/14/24 15:38, Nick Alcock wrote:
>
>> On 14 Nov 2024, Eugene Loh told this:
>>
>>> On 11/14/24 11:03, Nick Alcock wrote:
>>>> diff --git a/test/unittest/usdt/tst.multiprovider.r.p b/test/unittest/usdt/tst.multiprovider.r.p
>>>> new file mode 100755
>>>> index 0000000000000..ae8493e3d5be4
>>>> --- /dev/null
>>>> +++ b/test/unittest/usdt/tst.multiprovider.r.p
>>>> @@ -0,0 +1,2 @@
>>>> +#!/bin/sh
>>>> +grep -v '^ *ID' | sed 's,^[0-9]*,ID,; s,prov\(.\)[0-9]*,prov\1PID,; s,  *, ,g'
>>> Okay, though the "grep -v" would be clearer to me if it checked  '^ *ID *PROVIDER *MODULE *FUNCTION *NAME$'.
>> Why? We're not dependent on the format of the header, we just want to
>> get rid of it.
>
> It just seemed to me easier to figure out what was going on.  It looked like a recognizable header.

Oh I see!

> What do you think of ' *PROVIDER *'?  The 'ID' is short and strikes me as especially confusing if one looks at the post-processed
> output, which has 'ID' all over the place.

Yes, that's good :)

>> Having the test fail because the header we don't care
>> about changed layout seems wrong.
>>
>> I might just drop the first line with tail -n +1...
>>
>>> This whole thing would be clearer to me as a single awk, but maybe that's just me.
>> How? Piles of gmatches?
>
> gsub() or something.  Anyhow, your choice.

I always find gsub() sufficiently confusing that I have to check the
manual every time :(

>>> Comments might be nice.  I'm not sure how comfortably people read regex...  I need to concentrate hard to figure it out.
>> Honestly this is just doing some "replace changing numbers" stuff like a
>> hundred other .p's in the testsuite. Looking at the .r makes it obvious
>> what it's up to.
>
> No big deal either way (e.g., adding one-line comment).

Totally agree in re a comment :)

>>>> diff --git a/test/unittest/usdt/tst.multiprovider.sh b/test/unittest/usdt/tst.multiprovider.sh
>>>> new file mode 100755
>>>> index 0000000000000..d5b72c2be77ec
>>>> --- /dev/null
>>>> +++ b/test/unittest/usdt/tst.multiprovider.sh
>>>> @@ -0,0 +1,15 @@
>>>> +#!/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.
>>>> +#
>>>> +if [ $# != 1 ]; then
>>>> +	echo expected one argument: '<'dtrace-path'>'
>>>> +	exit 2
>>>> +fi
>>>> +
>>>> +dtrace=$1
>>>> +
>>>> +exec $dtrace $dt_flags -l -P 'prov*' -c `pwd`/test/triggers/usdt-tst-multiprovider
>>> Okay, but there should also be a test that actually fires probes.
>> ... why? What we care about is that DTrace can see the probes properly
>> (since we already know from other tests that once it can do that,
>> everything else works), and -l is an easy way to verify that (and in
>> general exercise the entire pathway up to USDT probe discovery in dtrace
>> itself) without having to get tangled up in even *more* possible bugs
>> around probe firing. I hit enough bugs in the parser writing this as it
>> was...
>>
>> I mean I can write one but I don't see what it's going to tell us.
>
> I'm less concerned about the bugs we can foresee.  I'm more concerned
> about the bugs we don't foresee.
>
> I'm even interested in the case where probes in different providers have the same name.

Seems reasonable! I'll add some tests...

> Different subject:  do we have a test for "dtrace -lv" on USDT typed args?

Yes: test/unittest/dtrace-util/tst.ListProbesArgsUSDT.sh

-- 
NULL && (void)