[PATCH v2 1/4] dtprobed: handle multiple providers in a single piece of DOF

[PATCH v2 1/4] 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      | 23 ++++++++
 test/unittest/usdt/tst.multiprovider.r      |  4 ++
 test/unittest/usdt/tst.multiprovider.r.p    |  2 +
 test/unittest/usdt/tst.multiprovider.sh     | 15 +++++
 10 files changed, 126 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

Changes since v1: test multiple probes.

This had fallout -- see later commits in this series!

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..530693ea71fe7
--- /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(); probe entryc(int a, char *b) : (char * b, int a); };
+provider provc { probe entryd(); };
diff --git a/test/triggers/usdt-tst-multiprovider.c b/test/triggers/usdt-tst-multiprovider.c
new file mode 100644
index 0000000000000..a954111379d9f
--- /dev/null
+++ b/test/triggers/usdt-tst-multiprovider.c
@@ -0,0 +1,23 @@
+/*
+ * 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();
+	PROVB_ENTRYC(666, "foo");
+	PROVC_ENTRYD();
+	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..60a15c01eeee3
--- /dev/null
+++ b/test/unittest/usdt/tst.multiprovider.r
@@ -0,0 +1,4 @@
+ID provaPID usdt-tst-multiprovider main entrya
+ID provbPID usdt-tst-multiprovider main entryb
+ID provbPID usdt-tst-multiprovider main entryc
+ID provcPID usdt-tst-multiprovider main entryd
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

[PATCH v2 2/4] runtest: detect coredumps in more cases

[Nick Alcock]

Exitcode 139 == signal 11 == a coredump that should be reported as such,
even if we can't find a core file anywhere.

Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
---
 runtest.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/runtest.sh b/runtest.sh
index 46b532d7e161f..94634f80c2ebf 100755
--- a/runtest.sh
+++ b/runtest.sh
@@ -1361,9 +1361,9 @@ for dt in $dtrace; do
                 want_all_output=t
             fi
 
-            if [[ -f core ]] || [[ "$(find $tmpdir -name core -print)" != "" ]]; then
+            if [[ -f core ]] || [[ "$(find $tmpdir -name core -print)" != "" ]] || [[ $exitcode = 139 ]]; then
                 # A coredump in the current directory or under the tmpdir.
-                # Preserve it in the logdir.
+                # Preserve it in the logdir, if possible.
 
                 mv core $logdir/$(echo $base | tr '/' '-').core 2>/dev/null || true
                 find $tmpdir -name core -type f -print0 | xargs -0r -I'{}' mv --backup=numbered '{}' $logdir/$(echo $base | tr '/' '-').core
-- 
2.46.0.278.g36e3a12567

[PATCH v2 3/4] probe: do not try to reify probes from uncooked providers

[Nick Alcock]

If there is a syntax error in the middle of a .d script processed
by -h, we will be left with a provider in question in an uncooked
state (where dt_provider_create() has been called on it with a NULL
provider by dt_node_provider(), but it has not had its provider
set yet by dt_cook_provider()).

In this case, the attempt to pop the parser stack on error will
fail because probe iteration is attempting to reify probes out
of providers without making sure they're cooked first:

    at libdtrace/dt_probe.c:1006
    argv=0x535830, fp=0x21e4630, s=0x0) at libdtrace/dt_cc.c:789
    fp=0x21e4630, s=0x0) at libdtrace/dt_cc.c:1419
    at libdtrace/dt_cc.c:1441

The fix is simple: treat providers with no provimpl just like we do
providers with no provide() method in their provimpl: they cannot provide
probes, so don't try. (Test coming in a later commit in this series.)

Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
---
 libdtrace/dt_probe.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
index 189bf7928e145..ccaa3081c5b23 100644
--- a/libdtrace/dt_probe.c
+++ b/libdtrace/dt_probe.c
@@ -1007,7 +1007,8 @@ dt_probe_iter(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp,
 	 * Loop over providers, allowing them to provide these probes.
 	 */
 	while ((pvp = dt_htab_next(dtp->dt_provs, &it)) != NULL) {
-		if (pvp->impl->provide == NULL ||
+		if (pvp->impl == NULL ||
+		    pvp->impl->provide == NULL ||
 		    !dt_gmatch(pvp->desc.dtvd_name, pdp->prv))
 			continue;
 		memcpy(&desc, pdp, sizeof(desc));
-- 
2.46.0.278.g36e3a12567

[PATCH v2 4/4] probe: make it possible to destroy probes in more cases

[Nick Alcock]

In particular, when the parser has failed (so we have no yypcb)
but we have successfully defined some interface-only probes but
not yet cooked them (so we have no provimpl), we can find that
*both* our sources for a dtp in dt_probe_destroy are unavailable.

Make one available in the dt_probe_t as well.

The test in this commit provides a .d like this:

provider prova { probe entrya(); };
provider provb { probe entryb(); probe entryc(int, char *) : (char *, int); };

prova is defined but not cooked because parsing failed in provb, so we get
this crash when trying to do final cleanup of prova, long after the parser is
gone:

1  0x00007ffff7cf408a in dt_iddtor_probe (idp=0x3cee000) at libdtrace/dt_ident.c:542
2  0x00007ffff7cf4d5a in dt_ident_destroy (idp=0x3cee000) at libdtrace/dt_ident.c:964
3  0x00007ffff7d0c226 in dt_node_free (dnp=0x21e05b0) at libdtrace/dt_parser.c:571
4  0x00007ffff7d17748 in dt_node_list_free (pnp=0x3cee1b8) at libdtrace/dt_parser.c:4851
5  0x00007ffff7d0c460 in dt_node_free (dnp=0x3cee180) at libdtrace/dt_parser.c:643
6  0x00007ffff7d177ab in dt_node_link_free (pnp=0x3cee278) at libdtrace/dt_parser.c:4865
7  0x00007ffff7d3df8b in dt_provider_del_prov (head=0x0, pvp=0x3cee1f0) at libdtrace/dt_provider.c:74
8  0x00007ffff7cf1d32 in dt_htab_destroy (dtp=0x53eb20, htab=0x187a8a0) at libdtrace/dt_htab.c:108
9  0x00007ffff7d0842e in dtrace_close (dtp=0x53eb20) at libdtrace/dt_open.c:1306
10 0x0000000000403f8e in dfatal (fmt=0x4d6a45 "failed to compile script %s") at cmd/dtrace.c:194
11 0x0000000000404b9b in compile_file (dcp=0x536990) at cmd/dtrace.c:480
12 0x00000000004073c9 in main (argc=8, argv=0x7fffffffe788) at cmd/dtrace.c:1351

Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
---
 libdtrace/dt_probe.c                          | 14 +++-----
 libdtrace/dt_probe.h                          |  1 +
 .../unittest/usdt/err.argmap-name-required.sh | 34 +++++++++++++++++++
 3 files changed, 40 insertions(+), 9 deletions(-)
 create mode 100755 test/unittest/usdt/err.argmap-name-required.sh

diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
index ccaa3081c5b23..31c78d6d987e1 100644
--- a/libdtrace/dt_probe.c
+++ b/libdtrace/dt_probe.c
@@ -140,8 +140,7 @@ alloc_arg_nodes(dtrace_hdl_t *dtp, dt_provider_t *pvp, int argc)
 static void
 dt_probe_alloc_args(dt_probe_t *prp, int nargc, int xargc)
 {
-	dt_provider_t	*pvp = prp->prov;
-	dtrace_hdl_t	*dtp = pvp->pv_hdl;
+	dtrace_hdl_t	*dtp = prp->dtp;
 	dt_node_t	*nargs = NULL, *xargs = NULL;
 	int		i;
 
@@ -252,6 +251,7 @@ dt_probe_create(dtrace_hdl_t *dtp, dt_ident_t *idp, int protoc,
 	prp->pr_inst = NULL;
 	prp->argv = dt_calloc(dtp, xargc, sizeof(dtrace_typeinfo_t));
 	prp->argc = xargc;
+	prp->dtp = dtp;
 
 	if ((prp->nargc != 0 && prp->nargv == NULL) ||
 	    (prp->xargc != 0 && prp->xargv == NULL) ||
@@ -323,12 +323,7 @@ dt_probe_destroy(dt_probe_t *prp)
 	dt_probe_stmt_t		*psp, *psp_next;
 	dt_probe_instance_t	*pip, *pip_next;
 	dt_probe_dependent_t	*dep, *dep_next;
-	dtrace_hdl_t		*dtp;
-
-	if (prp->prov != NULL)
-		dtp = prp->prov->pv_hdl;
-	else
-		dtp = yypcb->pcb_hdl;
+	dtrace_hdl_t		*dtp = prp->dtp;
 
 	if (prp->difo)
 		dt_difo_free(dtp, prp->difo);
@@ -473,7 +468,7 @@ dt_probe_define(dt_provider_t *pvp, dt_probe_t *prp, const char *fname,
 dt_node_t *
 dt_probe_tag(dt_probe_t *prp, uint_t argn, dt_node_t *dnp)
 {
-	dtrace_hdl_t *dtp = prp->prov->pv_hdl;
+	dtrace_hdl_t *dtp = prp->dtp;
 	dtrace_typeinfo_t dtt;
 	size_t len;
 	char *tag;
@@ -557,6 +552,7 @@ dt_probe_insert(dtrace_hdl_t *dtp, dt_provider_t *prov, const char *prv,
 	prp->prov = prov;
 	prp->prv_data = datap;
 	prp->argc = -1;
+	prp->dtp = dtp;
 
 	dt_htab_insert(dtp->dt_byprv, prp);
 	dt_htab_insert(dtp->dt_bymod, prp);
diff --git a/libdtrace/dt_probe.h b/libdtrace/dt_probe.h
index 2a78cb9ca4dae..da7a341354225 100644
--- a/libdtrace/dt_probe.h
+++ b/libdtrace/dt_probe.h
@@ -55,6 +55,7 @@ typedef struct dt_probe {
 	int argc;			/* output argument count */
 	dt_probe_instance_t *pr_inst;	/* list of functions and offsets */
 	dtrace_difo_t *difo;		/* BPF probe program */
+	dtrace_hdl_t *dtp;		/* pointer to containing dtrace_hdl */
 } dt_probe_t;
 
 extern dt_probe_t *dt_probe_lookup2(dt_provider_t *, const char *);
diff --git a/test/unittest/usdt/err.argmap-name-required.sh b/test/unittest/usdt/err.argmap-name-required.sh
new file mode 100755
index 0000000000000..3fdefb8130ac7
--- /dev/null
+++ b/test/unittest/usdt/err.argmap-name-required.sh
@@ -0,0 +1,34 @@
+#!/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.
+#
+# Make sure we get the right error when mappings are defined with no
+# argument.
+
+if [ $# != 1 ]; then
+	echo expected one argument: '<'dtrace-path'>'
+	exit 2
+fi
+
+dtrace=$1
+CC=/usr/bin/gcc
+CFLAGS="$test_cppflags"
+LDFLAGS="$test_ldflags"
+
+DIRNAME="$tmpdir/argmap-name-required.$$.$RANDOM"
+mkdir -p $DIRNAME
+cd $DIRNAME
+
+# We define multiple probes here to verify the absence of a potential error
+# where parser failures when previous probes have been successfully defined
+# causes crashes due to problems freeing still-uncooked probes.
+
+cat > prov.d <<EOF
+provider prova { probe entrya(); };
+provider provb { probe entryb(); probe entryc(int, char *) : (char *, int); };
+EOF
+
+exec $dtrace $dt_flags -h -s prov.d
-- 
2.46.0.278.g36e3a12567

Re: [PATCH v2 2/4] runtest: detect coredumps in more cases

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh@oracle.com>
though how about for the commit message (just guessing here) something like:

Exitcode 139 == 128 + SIGSEGV should be reported as a coredump,
even if we can't find a core file anywhere.

On 11/14/24 17:01, Nick Alcock wrote:
> Exitcode 139 == signal 11 == a coredump that should be reported as such,
> even if we can't find a core file anywhere.
>
> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
> ---
>   runtest.sh | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/runtest.sh b/runtest.sh
> index 46b532d7e161f..94634f80c2ebf 100755
> --- a/runtest.sh
> +++ b/runtest.sh
> @@ -1361,9 +1361,9 @@ for dt in $dtrace; do
>                   want_all_output=t
>               fi
>   
> -            if [[ -f core ]] || [[ "$(find $tmpdir -name core -print)" != "" ]]; then
> +            if [[ -f core ]] || [[ "$(find $tmpdir -name core -print)" != "" ]] || [[ $exitcode = 139 ]]; then
>                   # A coredump in the current directory or under the tmpdir.
> -                # Preserve it in the logdir.
> +                # Preserve it in the logdir, if possible.
>   
>                   mv core $logdir/$(echo $base | tr '/' '-').core 2>/dev/null || true
>                   find $tmpdir -name core -type f -print0 | xargs -0r -I'{}' mv --backup=numbered '{}' $logdir/$(echo $base | tr '/' '-').core

Re: [PATCH v2 1/4] dtprobed: handle multiple providers in a single piece of DOF

[Kris Van Hees]

On Thu, Nov 14, 2024 at 10:01:05PM +0000, 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
> 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>

Reviewed-by: Kris Van Hees <kris.van.hees@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      | 23 ++++++++
>  test/unittest/usdt/tst.multiprovider.r      |  4 ++
>  test/unittest/usdt/tst.multiprovider.r.p    |  2 +
>  test/unittest/usdt/tst.multiprovider.sh     | 15 +++++
>  10 files changed, 126 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
> 
> Changes since v1: test multiple probes.
> 
> This had fallout -- see later commits in this series!
> 
> 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..530693ea71fe7
> --- /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(); probe entryc(int a, char *b) : (char * b, int a); };
> +provider provc { probe entryd(); };
> diff --git a/test/triggers/usdt-tst-multiprovider.c b/test/triggers/usdt-tst-multiprovider.c
> new file mode 100644
> index 0000000000000..a954111379d9f
> --- /dev/null
> +++ b/test/triggers/usdt-tst-multiprovider.c
> @@ -0,0 +1,23 @@
> +/*
> + * 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();
> +	PROVB_ENTRYC(666, "foo");
> +	PROVC_ENTRYD();
> +	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..60a15c01eeee3
> --- /dev/null
> +++ b/test/unittest/usdt/tst.multiprovider.r
> @@ -0,0 +1,4 @@
> +ID provaPID usdt-tst-multiprovider main entrya
> +ID provbPID usdt-tst-multiprovider main entryb
> +ID provbPID usdt-tst-multiprovider main entryc
> +ID provcPID usdt-tst-multiprovider main entryd
> 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: [DTrace-devel] [PATCH v2 2/4] runtest: detect coredumps in more cases

[Kris Van Hees]

On Thu, Nov 14, 2024 at 10:01:06PM +0000, Nick Alcock via DTrace-devel wrote:
> Exitcode 139 == signal 11 == a coredump that should be reported as such,
> even if we can't find a core file anywhere.
> 
> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>

Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>

> ---
>  runtest.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/runtest.sh b/runtest.sh
> index 46b532d7e161f..94634f80c2ebf 100755
> --- a/runtest.sh
> +++ b/runtest.sh
> @@ -1361,9 +1361,9 @@ for dt in $dtrace; do
>                  want_all_output=t
>              fi
>  
> -            if [[ -f core ]] || [[ "$(find $tmpdir -name core -print)" != "" ]]; then
> +            if [[ -f core ]] || [[ "$(find $tmpdir -name core -print)" != "" ]] || [[ $exitcode = 139 ]]; then
>                  # A coredump in the current directory or under the tmpdir.
> -                # Preserve it in the logdir.
> +                # Preserve it in the logdir, if possible.
>  
>                  mv core $logdir/$(echo $base | tr '/' '-').core 2>/dev/null || true
>                  find $tmpdir -name core -type f -print0 | xargs -0r -I'{}' mv --backup=numbered '{}' $logdir/$(echo $base | tr '/' '-').core
> -- 
> 2.46.0.278.g36e3a12567
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel

Re: [PATCH v2 3/4] probe: do not try to reify probes from uncooked providers

[Kris Van Hees]

On Thu, Nov 14, 2024 at 10:01:07PM +0000, Nick Alcock wrote:
> If there is a syntax error in the middle of a .d script processed
> by -h, we will be left with a provider in question in an uncooked
> state (where dt_provider_create() has been called on it with a NULL
> provider by dt_node_provider(), but it has not had its provider
> set yet by dt_cook_provider()).
> 
> In this case, the attempt to pop the parser stack on error will
> fail because probe iteration is attempting to reify probes out
> of providers without making sure they're cooked first:
> 
>     at libdtrace/dt_probe.c:1006
>     argv=0x535830, fp=0x21e4630, s=0x0) at libdtrace/dt_cc.c:789
>     fp=0x21e4630, s=0x0) at libdtrace/dt_cc.c:1419
>     at libdtrace/dt_cc.c:1441
> 
> The fix is simple: treat providers with no provimpl just like we do
> providers with no provide() method in their provimpl: they cannot provide
> probes, so don't try. (Test coming in a later commit in this series.)

This entire commit message (and the one-liner subject) are actually not
quite correct in terms of what the problem is.  It has nothing to do with
cooked vs uncooked providers or probes.  The problem is simply that the
dt_probe_iter() functions calls the provide() hook in provider prior to
trying to match probe descriptions, and that cannot be done for a provider
without implementation.  That is all.

This also needs a test.

I'm posting an alternative patch (with my reviewed-by) with updated commit
message and 2 tests (signed-off-by for both of us since you wrote the code).

> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
> ---
>  libdtrace/dt_probe.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
> index 189bf7928e145..ccaa3081c5b23 100644
> --- a/libdtrace/dt_probe.c
> +++ b/libdtrace/dt_probe.c
> @@ -1007,7 +1007,8 @@ dt_probe_iter(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp,
>  	 * Loop over providers, allowing them to provide these probes.
>  	 */
>  	while ((pvp = dt_htab_next(dtp->dt_provs, &it)) != NULL) {
> -		if (pvp->impl->provide == NULL ||
> +		if (pvp->impl == NULL ||
> +		    pvp->impl->provide == NULL ||
>  		    !dt_gmatch(pvp->desc.dtvd_name, pdp->prv))
>  			continue;
>  		memcpy(&desc, pdp, sizeof(desc));
> -- 
> 2.46.0.278.g36e3a12567
> 

Re: [PATCH v2 4/4] probe: make it possible to destroy probes in more cases

[Kris Van Hees]

This patch is not needed.  I do not see any failures related to the changes
in this patch and the test case passes even without these code changes as
far as I can see.

The test case has been added to the 3/4 patch, and has been converted to a 
simple .d file (and .r for test verification).  No need to use a shell script
in this case.

On Thu, Nov 14, 2024 at 10:01:08PM +0000, Nick Alcock wrote:
> In particular, when the parser has failed (so we have no yypcb)
> but we have successfully defined some interface-only probes but
> not yet cooked them (so we have no provimpl), we can find that
> *both* our sources for a dtp in dt_probe_destroy are unavailable.
> 
> Make one available in the dt_probe_t as well.
> 
> The test in this commit provides a .d like this:
> 
> provider prova { probe entrya(); };
> provider provb { probe entryb(); probe entryc(int, char *) : (char *, int); };
> 
> prova is defined but not cooked because parsing failed in provb, so we get
> this crash when trying to do final cleanup of prova, long after the parser is
> gone:
> 
> 1  0x00007ffff7cf408a in dt_iddtor_probe (idp=0x3cee000) at libdtrace/dt_ident.c:542
> 2  0x00007ffff7cf4d5a in dt_ident_destroy (idp=0x3cee000) at libdtrace/dt_ident.c:964
> 3  0x00007ffff7d0c226 in dt_node_free (dnp=0x21e05b0) at libdtrace/dt_parser.c:571
> 4  0x00007ffff7d17748 in dt_node_list_free (pnp=0x3cee1b8) at libdtrace/dt_parser.c:4851
> 5  0x00007ffff7d0c460 in dt_node_free (dnp=0x3cee180) at libdtrace/dt_parser.c:643
> 6  0x00007ffff7d177ab in dt_node_link_free (pnp=0x3cee278) at libdtrace/dt_parser.c:4865
> 7  0x00007ffff7d3df8b in dt_provider_del_prov (head=0x0, pvp=0x3cee1f0) at libdtrace/dt_provider.c:74
> 8  0x00007ffff7cf1d32 in dt_htab_destroy (dtp=0x53eb20, htab=0x187a8a0) at libdtrace/dt_htab.c:108
> 9  0x00007ffff7d0842e in dtrace_close (dtp=0x53eb20) at libdtrace/dt_open.c:1306
> 10 0x0000000000403f8e in dfatal (fmt=0x4d6a45 "failed to compile script %s") at cmd/dtrace.c:194
> 11 0x0000000000404b9b in compile_file (dcp=0x536990) at cmd/dtrace.c:480
> 12 0x00000000004073c9 in main (argc=8, argv=0x7fffffffe788) at cmd/dtrace.c:1351
> 
> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
> ---
>  libdtrace/dt_probe.c                          | 14 +++-----
>  libdtrace/dt_probe.h                          |  1 +
>  .../unittest/usdt/err.argmap-name-required.sh | 34 +++++++++++++++++++
>  3 files changed, 40 insertions(+), 9 deletions(-)
>  create mode 100755 test/unittest/usdt/err.argmap-name-required.sh
> 
> diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
> index ccaa3081c5b23..31c78d6d987e1 100644
> --- a/libdtrace/dt_probe.c
> +++ b/libdtrace/dt_probe.c
> @@ -140,8 +140,7 @@ alloc_arg_nodes(dtrace_hdl_t *dtp, dt_provider_t *pvp, int argc)
>  static void
>  dt_probe_alloc_args(dt_probe_t *prp, int nargc, int xargc)
>  {
> -	dt_provider_t	*pvp = prp->prov;
> -	dtrace_hdl_t	*dtp = pvp->pv_hdl;
> +	dtrace_hdl_t	*dtp = prp->dtp;
>  	dt_node_t	*nargs = NULL, *xargs = NULL;
>  	int		i;
>  
> @@ -252,6 +251,7 @@ dt_probe_create(dtrace_hdl_t *dtp, dt_ident_t *idp, int protoc,
>  	prp->pr_inst = NULL;
>  	prp->argv = dt_calloc(dtp, xargc, sizeof(dtrace_typeinfo_t));
>  	prp->argc = xargc;
> +	prp->dtp = dtp;
>  
>  	if ((prp->nargc != 0 && prp->nargv == NULL) ||
>  	    (prp->xargc != 0 && prp->xargv == NULL) ||
> @@ -323,12 +323,7 @@ dt_probe_destroy(dt_probe_t *prp)
>  	dt_probe_stmt_t		*psp, *psp_next;
>  	dt_probe_instance_t	*pip, *pip_next;
>  	dt_probe_dependent_t	*dep, *dep_next;
> -	dtrace_hdl_t		*dtp;
> -
> -	if (prp->prov != NULL)
> -		dtp = prp->prov->pv_hdl;
> -	else
> -		dtp = yypcb->pcb_hdl;
> +	dtrace_hdl_t		*dtp = prp->dtp;
>  
>  	if (prp->difo)
>  		dt_difo_free(dtp, prp->difo);
> @@ -473,7 +468,7 @@ dt_probe_define(dt_provider_t *pvp, dt_probe_t *prp, const char *fname,
>  dt_node_t *
>  dt_probe_tag(dt_probe_t *prp, uint_t argn, dt_node_t *dnp)
>  {
> -	dtrace_hdl_t *dtp = prp->prov->pv_hdl;
> +	dtrace_hdl_t *dtp = prp->dtp;
>  	dtrace_typeinfo_t dtt;
>  	size_t len;
>  	char *tag;
> @@ -557,6 +552,7 @@ dt_probe_insert(dtrace_hdl_t *dtp, dt_provider_t *prov, const char *prv,
>  	prp->prov = prov;
>  	prp->prv_data = datap;
>  	prp->argc = -1;
> +	prp->dtp = dtp;
>  
>  	dt_htab_insert(dtp->dt_byprv, prp);
>  	dt_htab_insert(dtp->dt_bymod, prp);
> diff --git a/libdtrace/dt_probe.h b/libdtrace/dt_probe.h
> index 2a78cb9ca4dae..da7a341354225 100644
> --- a/libdtrace/dt_probe.h
> +++ b/libdtrace/dt_probe.h
> @@ -55,6 +55,7 @@ typedef struct dt_probe {
>  	int argc;			/* output argument count */
>  	dt_probe_instance_t *pr_inst;	/* list of functions and offsets */
>  	dtrace_difo_t *difo;		/* BPF probe program */
> +	dtrace_hdl_t *dtp;		/* pointer to containing dtrace_hdl */
>  } dt_probe_t;
>  
>  extern dt_probe_t *dt_probe_lookup2(dt_provider_t *, const char *);
> diff --git a/test/unittest/usdt/err.argmap-name-required.sh b/test/unittest/usdt/err.argmap-name-required.sh
> new file mode 100755
> index 0000000000000..3fdefb8130ac7
> --- /dev/null
> +++ b/test/unittest/usdt/err.argmap-name-required.sh
> @@ -0,0 +1,34 @@
> +#!/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.
> +#
> +# Make sure we get the right error when mappings are defined with no
> +# argument.
> +
> +if [ $# != 1 ]; then
> +	echo expected one argument: '<'dtrace-path'>'
> +	exit 2
> +fi
> +
> +dtrace=$1
> +CC=/usr/bin/gcc
> +CFLAGS="$test_cppflags"
> +LDFLAGS="$test_ldflags"
> +
> +DIRNAME="$tmpdir/argmap-name-required.$$.$RANDOM"
> +mkdir -p $DIRNAME
> +cd $DIRNAME
> +
> +# We define multiple probes here to verify the absence of a potential error
> +# where parser failures when previous probes have been successfully defined
> +# causes crashes due to problems freeing still-uncooked probes.
> +
> +cat > prov.d <<EOF
> +provider prova { probe entrya(); };
> +provider provb { probe entryb(); probe entryc(int, char *) : (char *, int); };
> +EOF
> +
> +exec $dtrace $dt_flags -h -s prov.d
> -- 
> 2.46.0.278.g36e3a12567
> 

Re: [PATCH v2 2/4] runtest: detect coredumps in more cases

[Nick Alcock]

On 15 Nov 2024, Eugene Loh spake thusly:

> Reviewed-by: Eugene Loh <eugene.loh@oracle.com>
> though how about for the commit message (just guessing here) something like:
>
> Exitcode 139 == 128 + SIGSEGV should be reported as a coredump,
> even if we can't find a core file anywhere.

Oh, I had honestly forgotten that that was the origin of 139 :)

(It's gone in now, anyway.)

Re: [PATCH v2 3/4] probe: do not try to reify probes from uncooked providers

[Nick Alcock]

On 15 Nov 2024, Kris Van Hees outgrape:

> On Thu, Nov 14, 2024 at 10:01:07PM +0000, Nick Alcock wrote:
>> If there is a syntax error in the middle of a .d script processed
>> by -h, we will be left with a provider in question in an uncooked
>> state (where dt_provider_create() has been called on it with a NULL
>> provider by dt_node_provider(), but it has not had its provider
>> set yet by dt_cook_provider()).
>> 
>> In this case, the attempt to pop the parser stack on error will
>> fail because probe iteration is attempting to reify probes out
>> of providers without making sure they're cooked first:
>> 
>>     at libdtrace/dt_probe.c:1006
>>     argv=0x535830, fp=0x21e4630, s=0x0) at libdtrace/dt_cc.c:789
>>     fp=0x21e4630, s=0x0) at libdtrace/dt_cc.c:1419
>>     at libdtrace/dt_cc.c:1441
>> 
>> The fix is simple: treat providers with no provimpl just like we do
>> providers with no provide() method in their provimpl: they cannot provide
>> probes, so don't try. (Test coming in a later commit in this series.)
>
> This entire commit message (and the one-liner subject) are actually not
> quite correct in terms of what the problem is.  It has nothing to do with
> cooked vs uncooked providers or probes.  The problem is simply that the
> dt_probe_iter() functions calls the provide() hook in provider prior to
> trying to match probe descriptions, and that cannot be done for a provider
> without implementation.  That is all.

Yes, but I was trying to say *why* that might happen.

> This also needs a test.

It's got one -- the parser-error test I posted in this series died in
two different ways if either of these bugs were present.

> I'm posting an alternative patch (with my reviewed-by) with updated commit
> message and 2 tests (signed-off-by for both of us since you wrote the code).

OK! Looking...

-- 
NULL && (void)

Re: [PATCH v2 4/4] probe: make it possible to destroy probes in more cases

[Nick Alcock]

On 15 Nov 2024, Kris Van Hees said:

> This patch is not needed.  I do not see any failures related to the changes
> in this patch and the test case passes even without these code changes as
> far as I can see.

It didn't when I ran it. Try under valgrind.

> The test case has been added to the 3/4 patch, and has been converted to a 
> simple .d file (and .r for test verification).  No need to use a shell script
> in this case.

True enough.

-- 
NULL && (void)

Re: [PATCH v2 4/4] probe: make it possible to destroy probes in more cases

[Nick Alcock]

On 15 Nov 2024, Nick Alcock outgrape:

> On 15 Nov 2024, Kris Van Hees said:
>
>> This patch is not needed.  I do not see any failures related to the changes
>> in this patch and the test case passes even without these code changes as
>> far as I can see.
>
> It didn't when I ran it. Try under valgrind.

... and now it doesn't for me either! I guess 3/4 when done right (as
you suggested, not as I originally did it) obviates the need for 4/4.
Still don't entirely understand *why*, but maybe the probes vanish
before the parser terminates, so they're not left lingering for a late
deletion.

-- 
NULL && (void)