(no subject)

[eugene.loh]

On June 4, I sent out a 14-patch series for systemwide USDT.  Thanks
for the feedback.  Here is a 38-patch series that incorporates some
of the feedback.  I'm not labeling the patches "v2" since there is no
longer a one-to-one correspondence between patches.

Big changes include tracking retained probe descriptions and widening
the EPID from 32 to 64 bits (and thereby including the PRID).  There
is no great change in how the BPF map (storing PRIDs) is populated,
but further discussion is welcome and hopefully ensuing changes will
be easy to integrate into this patch series.

There are still a few FIXMEs, but at this point I welcome some feedback.

[PATCH 01/38] Move comment closer to the code it describes

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>
---
 libdtrace/dt_prov_uprobe.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
index b712c589..afc1f628 100644
--- a/libdtrace/dt_prov_uprobe.c
+++ b/libdtrace/dt_prov_uprobe.c
@@ -337,15 +337,16 @@ static void enable(dtrace_hdl_t *dtp, dt_probe_t *prp, int is_usdt)
 
 	/*
 	 * We need to enable the underlying probes (if not enabled yet).
-	 *
-	 * If necessary, we need to enable is-enabled probes too (if they
-	 * exist).
 	 */
 	for (pup = prp->prv_data; pup != NULL; pup = dt_list_next(pup)) {
 		dt_probe_t *uprp = pup->probe;
 		dt_probe_enable(dtp, uprp);
 	}
 
+	/*
+	 * If necessary, we need to enable is-enabled probes too (if they
+	 * exist).
+	 */
 	if (is_usdt) {
 		dtrace_probedesc_t pd;
 		dt_probe_t *iep;
-- 
2.18.4

[PATCH 02/38] Move dt_spec_buf_data_t and dt_spec_buf_t into dt_consume.c

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 libdtrace/dt_consume.c | 18 ++++++++++++++++++
 libdtrace/dt_impl.h    | 18 ------------------
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
index dec2314b..5fb636fe 100644
--- a/libdtrace/dt_consume.c
+++ b/libdtrace/dt_consume.c
@@ -27,6 +27,24 @@
 
 #define	DT_MASK_LO 0x00000000FFFFFFFFULL
 
+typedef struct dt_spec_buf_data {
+	dt_list_t dsbd_list;		/* linked-list forward/back pointers */
+	unsigned int dsbd_cpu;		/* cpu for data */
+	char *dsbd_data;		/* data for later processing */
+	uint32_t dsbd_size;		/* size of data */
+} dt_spec_buf_data_t;
+
+typedef struct dt_spec_buf {
+	dtrace_hdl_t *dtsb_dtp;		/* backpointer to the dtrace instance */
+	int32_t dtsb_id;		/* speculation ID */
+	size_t dtsb_size;		/* size of all buffers in this spec */
+	int dtsb_committing;		/* when draining, nonzero if commit */
+	dt_bpf_specs_t dtsb_spec;	/* bpf-side specs record for this spec
+					   (buffer read/write counts).  */
+	dt_list_t dtsb_dsbd_list;	/* list of dt_spec_bufs */
+	struct dt_hentry dtsb_he;	/* htab links */
+} dt_spec_buf_t;
+
 /*
  * We declare this here because (1) we need it and (2) we want to avoid a
  * dependency on libm in libdtrace.
diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
index 01313ff3..b5408c1c 100644
--- a/libdtrace/dt_impl.h
+++ b/libdtrace/dt_impl.h
@@ -269,24 +269,6 @@ typedef struct dt_lib_depend {
 	dt_list_t dtld_dependents;	/* linked-list of lib dependents */
 } dt_lib_depend_t;
 
-typedef struct dt_spec_buf_data {
-	dt_list_t dsbd_list;		/* linked-list forward/back pointers */
-	unsigned int dsbd_cpu;		/* cpu for data */
-	char *dsbd_data;		/* data for later processing */
-	uint32_t dsbd_size;		/* size of data */
-} dt_spec_buf_data_t;
-
-typedef struct dt_spec_buf {
-	dtrace_hdl_t *dtsb_dtp;		/* backpointer to the dtrace instance */
-	int32_t dtsb_id;		/* speculation ID */
-	size_t dtsb_size;		/* size of all buffers in this spec */
-	int dtsb_committing;		/* when draining, nonzero if commit */
-	dt_bpf_specs_t dtsb_spec;	/* bpf-side specs record for this spec
-					   (buffer read/write counts).  */
-	dt_list_t dtsb_dsbd_list;	/* list of dt_spec_bufs */
-	struct dt_hentry dtsb_he;	/* htab links */
-} dt_spec_buf_t;
-
 typedef struct dt_percpu_drops {
 	uint64_t	buf;		/* principal buffer drops */
 	uint64_t	agg;		/* aggregate buffer drops */
-- 
2.18.4

[PATCH 03/38] Get rid of apparently orphaned status[2]

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

This has apparently been replaced by the dtp component
"dtrace_status_t dt_status[2]".

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 cmd/dtrace.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/cmd/dtrace.c b/cmd/dtrace.c
index 9c820686..ba1c22c5 100644
--- a/cmd/dtrace.c
+++ b/cmd/dtrace.c
@@ -1,6 +1,6 @@
 /*
  * Oracle Linux DTrace.
- * Copyright (c) 2006, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2006, 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.
  */
@@ -916,7 +916,6 @@ main(int argc, char *argv[])
 {
 	dtrace_bufdesc_t buf;
 	struct sigaction act, oact;
-	dtrace_status_t status[2];
 	dtrace_optval_t opt;
 	dtrace_cmd_t *dcp;
 
@@ -941,7 +940,6 @@ main(int argc, char *argv[])
 	g_argv[g_argc++] = argv[0];	/* propagate argv[0] to D as $0/$$0 */
 	argv[0] = g_pname;		/* rewrite argv[0] for getopt errors */
 
-	memset(status, 0, sizeof(status));
 	memset(&buf, 0, sizeof(buf));
 
 	/*
-- 
2.18.4

[PATCH 04/38] Get rid of apparently orphaned bufdesc stuff

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 cmd/dtrace.c                    |  3 ---
 include/dtrace/buffer.h         | 42 ---------------------------------
 include/dtrace/buffer_defines.h | 20 ----------------
 include/dtrace/dtrace.h         |  3 +--
 include/dtrace/ioctl.h          |  5 +---
 5 files changed, 2 insertions(+), 71 deletions(-)
 delete mode 100644 include/dtrace/buffer.h
 delete mode 100644 include/dtrace/buffer_defines.h

diff --git a/cmd/dtrace.c b/cmd/dtrace.c
index ba1c22c5..af354653 100644
--- a/cmd/dtrace.c
+++ b/cmd/dtrace.c
@@ -914,7 +914,6 @@ intr(int signo)
 int
 main(int argc, char *argv[])
 {
-	dtrace_bufdesc_t buf;
 	struct sigaction act, oact;
 	dtrace_optval_t opt;
 	dtrace_cmd_t *dcp;
@@ -940,8 +939,6 @@ main(int argc, char *argv[])
 	g_argv[g_argc++] = argv[0];	/* propagate argv[0] to D as $0/$$0 */
 	argv[0] = g_pname;		/* rewrite argv[0] for getopt errors */
 
-	memset(&buf, 0, sizeof(buf));
-
 	/*
 	 * Make an initial pass through argv[] processing any arguments that
 	 * affect our behavior mode (g_mode) and flags used for dtrace_open().
diff --git a/include/dtrace/buffer.h b/include/dtrace/buffer.h
deleted file mode 100644
index 6fa8c769..00000000
--- a/include/dtrace/buffer.h
+++ /dev/null
@@ -1,42 +0,0 @@
-/*
- * Licensed under the Universal Permissive License v 1.0 as shown at
- * http://oss.oracle.com/licenses/upl.
- *
- * Copyright (c) 2009, 2013, Oracle and/or its affiliates. All rights reserved.
- */
-
-/*
- * Note: The contents of this file are private to the implementation of the
- * DTrace subsystem and are subject to change at any time without notice.
- */
-
-#ifndef _DTRACE_BUFFER_H
-#define _DTRACE_BUFFER_H
-
-#include <dtrace/universal.h>
-#include <dtrace/actions_defines.h>
-#include <dtrace/buffer_defines.h>
-
-/*
- * In order to get a snapshot of the principal or aggregation buffer,
- * user-level passes a buffer description to the kernel with the dtrace_bufdesc
- * structure.  This describes which CPU user-level is interested in, and
- * where user-level wishes the kernel to snapshot the buffer to (the
- * dtbd_data field).  The kernel uses the same structure to pass back some
- * information regarding the buffer:  the size of data actually copied out, the
- * number of drops, the number of errors, and the offset of the oldest record.
- * If the buffer policy is a "switch" policy, taking a snapshot of the
- * principal buffer has the additional effect of switching the active and
- * inactive buffers.  Taking a snapshot of the aggregation buffer _always_ has
- * the additional effect of switching the active and inactive buffers.
- */
-typedef struct dtrace_bufdesc {
-	uint64_t dtbd_size;			/* size of buffer */
-	uint32_t dtbd_cpu;			/* CPU or DTRACE_CPUALL */
-	uint32_t dtbd_errors;			/* number of errors */
-	uint64_t dtbd_drops;			/* number of drops */
-	DTRACE_PTR(char, dtbd_data);		/* data */
-	uint64_t dtbd_oldest;			/* offset of oldest record */
-} dtrace_bufdesc_t;
-
-#endif /* _DTRACE_BUFFER_H */
diff --git a/include/dtrace/buffer_defines.h b/include/dtrace/buffer_defines.h
deleted file mode 100644
index f81c22f1..00000000
--- a/include/dtrace/buffer_defines.h
+++ /dev/null
@@ -1,20 +0,0 @@
-/*
- * Licensed under the Universal Permissive License v 1.0 as shown at
- * http://oss.oracle.com/licenses/upl.
- *
- * Copyright (c) 2009, 2013, Oracle and/or its affiliates. All rights reserved.
- */
-
-/*
- * Note: The contents of this file are private to the implementation of the
- * DTrace subsystem and are subject to change at any time without notice.
- */
-
-#ifndef _DTRACE_BUFFER_DEFINES_H
-#define _DTRACE_BUFFER_DEFINES_H
-
-#include <dtrace/universal.h>
-
-struct dtrace_bufdesc;
-
-#endif /* _DTRACE_BUFFER_DEFINES_H */
diff --git a/include/dtrace/dtrace.h b/include/dtrace/dtrace.h
index 8f28c9ba..7fc5f5f7 100644
--- a/include/dtrace/dtrace.h
+++ b/include/dtrace/dtrace.h
@@ -2,7 +2,7 @@
  * Licensed under the Universal Permissive License v 1.0 as shown at
  * http://oss.oracle.com/licenses/upl.
  *
- * Copyright (c) 2009, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2009, 2024, Oracle and/or its affiliates. All rights reserved.
  */
 
 /*
@@ -21,7 +21,6 @@
 #include <dtrace/enabling.h>
 #include <dtrace/metadesc.h>
 #include <dtrace/options.h>
-#include <dtrace/buffer.h>
 #include <dtrace/status.h>
 #include <dtrace/conf.h>
 #include <dtrace/faults.h>
diff --git a/include/dtrace/ioctl.h b/include/dtrace/ioctl.h
index 75d02464..a2a3a93b 100644
--- a/include/dtrace/ioctl.h
+++ b/include/dtrace/ioctl.h
@@ -2,7 +2,7 @@
  * Licensed under the Universal Permissive License v 1.0 as shown at
  * http://oss.oracle.com/licenses/upl.
  *
- * Copyright (c) 2009, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2009, 2024, Oracle and/or its affiliates. All rights reserved.
  */
 
 #ifndef _DTRACE_IOCTL_H_
@@ -10,7 +10,6 @@
 
 #include <linux/ioctl.h>
 #include <dtrace/arg.h>
-#include <dtrace/buffer.h>
 #include <dtrace/conf.h>
 #include <dtrace/dof.h>
 #include <dtrace/enabling.h>
@@ -22,10 +21,8 @@
 #define DTRACEIOC		0xd4
 #define DTRACEIOC_PROVIDER	_IOR(DTRACEIOC, 1, dtrace_providerdesc_t)
 #define DTRACEIOC_PROBES	_IOR(DTRACEIOC, 2, dtrace_probedesc_t)
-#define DTRACEIOC_BUFSNAP	_IOR(DTRACEIOC, 4, dtrace_bufdesc_t)
 #define DTRACEIOC_PROBEMATCH	_IOR(DTRACEIOC, 5, dtrace_probedesc_t)
 #define DTRACEIOC_ENABLE	_IOW(DTRACEIOC, 6, void *)
-#define DTRACEIOC_AGGSNAP	_IOR(DTRACEIOC, 7, dtrace_bufdesc_t)
 #define DTRACEIOC_EPROBE	_IOW(DTRACEIOC, 8, dtrace_eprobedesc_t)
 #define DTRACEIOC_PROBEARG	_IOR(DTRACEIOC, 9, dtrace_argdesc_t)
 #define DTRACEIOC_CONF		_IOR(DTRACEIOC, 10, dtrace_conf_t)
-- 
2.18.4

[PATCH 05/38] Get rid of unneeded enabling_defines.h

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 include/dtrace/enabling.h         |  3 +--
 include/dtrace/enabling_defines.h | 23 -----------------------
 2 files changed, 1 insertion(+), 25 deletions(-)
 delete mode 100644 include/dtrace/enabling_defines.h

diff --git a/include/dtrace/enabling.h b/include/dtrace/enabling.h
index 41e50cda..d36a73df 100644
--- a/include/dtrace/enabling.h
+++ b/include/dtrace/enabling.h
@@ -2,7 +2,7 @@
  * Licensed under the Universal Permissive License v 1.0 as shown at
  * http://oss.oracle.com/licenses/upl.
  *
- * Copyright (c) 2009, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2009, 2024, Oracle and/or its affiliates. All rights reserved.
  */
 
 /*
@@ -15,7 +15,6 @@
 
 #include <dtrace/universal.h>
 #include <dtrace/difo_defines.h>
-#include <dtrace/enabling_defines.h>
 
 /*
  * FIXME: Needs to be rewritten.
diff --git a/include/dtrace/enabling_defines.h b/include/dtrace/enabling_defines.h
deleted file mode 100644
index 875b49d0..00000000
--- a/include/dtrace/enabling_defines.h
+++ /dev/null
@@ -1,23 +0,0 @@
-/*
- * Licensed under the Universal Permissive License v 1.0 as shown at
- * http://oss.oracle.com/licenses/upl.
- *
- * Copyright (c) 2009, 2013, Oracle and/or its affiliates. All rights reserved.
- */
-
-/*
- * Note: The contents of this file are private to the implementation of the
- * DTrace subsystem and are subject to change at any time without notice.
- */
-
-#ifndef _DTRACE_ENABLING_DEFINES_H
-#define _DTRACE_ENABLING_DEFINES_H
-
-#include <dtrace/universal.h>
-
-struct dtrace_probedesc;
-struct dtrace_repldesc;
-struct dtrace_actdesc;
-struct dtrace_ecbdesc;
-
-#endif /* _DTRACE_ENABLING_DEFINES_H */
-- 
2.18.4

[PATCH 06/38] Get rid of unused dtrace_repldesc_t

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 include/dtrace/enabling.h | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/include/dtrace/enabling.h b/include/dtrace/enabling.h
index d36a73df..f1ec444c 100644
--- a/include/dtrace/enabling.h
+++ b/include/dtrace/enabling.h
@@ -43,11 +43,6 @@ typedef struct dtrace_probedesc {
 	const char	*prb;			/* probe name */
 } dtrace_probedesc_t;
 
-typedef struct dtrace_repldesc {
-	dtrace_probedesc_t dtrpd_match;		/* probe descr. to match */
-	dtrace_probedesc_t dtrpd_create;	/* probe descr. to create */
-} dtrace_repldesc_t;
-
 typedef struct dtrace_actdesc {
 	struct dtrace_difo *dtad_difo;		/* pointer to DIF object */
 	dtrace_actkind_t dtad_kind;		/* kind of action */
-- 
2.18.4

[PATCH 07/38] Clean up prp/pprp/uprp variable names

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 libdtrace/dt_prov_uprobe.c | 83 +++++++++++++++++++++++++-------------
 1 file changed, 55 insertions(+), 28 deletions(-)

diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
index afc1f628..c77063a8 100644
--- a/libdtrace/dt_prov_uprobe.c
+++ b/libdtrace/dt_prov_uprobe.c
@@ -6,6 +6,31 @@
  *
  * The uprobe-based provider for DTrace (implementing pid and USDT providers).
  */
+/*
+ * This file uses both overlying probes (specified by the user) as well as
+ * underlying probes (the uprobes recognized by the kernel).  To minimize
+ * confusion, this file should use consistent variable names:
+ *
+ *     dt_probe_t	*prp;   //  overlying probe
+ *     dt_probe_t	*uprp;  // underlying probe
+ *
+ *             Either one might be returned by dt_probe_lookup() or
+ *             dt_probe_insert() or added to dt_enablings[] or dt_probes[].
+ *             Further, uprp might be returned by create_underlying().
+ *
+ *     dt_uprobe_t	*upp;   // uprobe associated with an underlying probe
+ *
+ *     list_probe_t	*pop;   //  overlying probe list
+ *     list_probe_t	*pup;   // underlying probe list
+ *
+ * The provider-specific prv_data has these meanings:
+ *
+ *     prp->prv_data            // dt_list_t of associated underlying probes
+ *
+ *     uprp->prv_data           // upp (the associated uprobe)
+ *
+ * Finally, note that upp->probes is a dt_list_t of overlying probes.
+ */
 #include <sys/types.h>
 #include <assert.h>
 #include <errno.h>
@@ -118,7 +143,7 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
 	char			mod[DTRACE_MODNAMELEN];
 	char			prb[DTRACE_NAMELEN];
 	dtrace_probedesc_t	pd;
-	dt_probe_t		*prp;
+	dt_probe_t		*uprp;
 	dt_uprobe_t		*upp;
 	int			is_enabled = 0;
 
@@ -160,8 +185,8 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
 	pd.fun = psp->pps_fun;
 	pd.prb = prb;
 
-	prp = dt_probe_lookup(dtp, &pd);
-	if (prp == NULL) {
+	uprp = dt_probe_lookup(dtp, &pd);
+	if (uprp == NULL) {
 		dt_provider_t	*pvp;
 
 		/* Get the provider for underlying probes. */
@@ -182,12 +207,12 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
 		if (upp->tp == NULL)
 			goto fail;
 
-		prp = dt_probe_insert(dtp, pvp, pd.prv, pd.mod, pd.fun, pd.prb,
+		uprp = dt_probe_insert(dtp, pvp, pd.prv, pd.mod, pd.fun, pd.prb,
 				      upp);
-		if (prp == NULL)
+		if (uprp == NULL)
 			goto fail;
 	} else
-		upp = prp->prv_data;
+		upp = uprp->prv_data;
 
 	switch (psp->pps_type) {
 	case DTPPT_RETURN:
@@ -202,7 +227,7 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
 	     */
 	}
 
-	return prp;
+	return uprp;
 
 fail:
 	probe_destroy(dtp, upp);
@@ -259,7 +284,7 @@ static int provide_probe(dtrace_hdl_t *dtp, const pid_probespec_t *psp,
 	}
 
 	/*
-	 * Underlying and overlying probe list entries.
+	 * Overlying and underlying probe list entries.
 	 */
 	pop = dt_zalloc(dtp, sizeof(list_probe_t));
 	if (pop == NULL)
@@ -271,7 +296,10 @@ static int provide_probe(dtrace_hdl_t *dtp, const pid_probespec_t *psp,
 		return -1;
 	}
 
-	/* Add the pid probe, if we need to. */
+	/*
+	 * Add the underlying probe to the list of probes for the overlying probe,
+	 * adding the overlying probe if we need to.
+	 */
 
 	pup->probe = uprp;
 	if (prp == NULL)
@@ -286,11 +314,10 @@ static int provide_probe(dtrace_hdl_t *dtp, const pid_probespec_t *psp,
 		return -1;
 	}
 
-	pop->probe = prp;
-
 	/*
-	 * Add the pid probe to the list of probes for the underlying probe.
+	 * Add the overlying probe to the list of probes for the underlying probe.
 	 */
+	pop->probe = prp;
 	dt_list_append(&upp->probes, pop);
 
 	return 0;
@@ -394,8 +421,8 @@ static void enable_usdt(dtrace_hdl_t *dtp, dt_probe_t *prp)
 static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
 {
 	dt_irlist_t		*dlp = &pcb->pcb_ir;
-	const dt_probe_t	*prp = pcb->pcb_probe;
-	const dt_uprobe_t	*upp = prp->prv_data;
+	const dt_probe_t	*uprp = pcb->pcb_probe;
+	const dt_uprobe_t	*upp = uprp->prv_data;
 	const list_probe_t	*pop;
 	uint_t			lbl_exit = pcb->pcb_exitlbl;
 
@@ -441,15 +468,15 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
 	 */
 	for (pop = dt_list_next(&upp->probes); pop != NULL;
 	     pop = dt_list_next(pop)) {
-		const dt_probe_t	*pprp = pop->probe;
+		const dt_probe_t	*prp = pop->probe;
 		uint_t			lbl_next = dt_irlist_label(dlp);
 		pid_t			pid;
 		dt_ident_t		*idp;
 
-		pid = dt_pid_get_pid(pprp->desc, pcb->pcb_hdl, pcb, NULL);
+		pid = dt_pid_get_pid(prp->desc, pcb->pcb_hdl, pcb, NULL);
 		assert(pid != -1);
 
-		idp = dt_dlib_add_probe_var(pcb->pcb_hdl, pprp);
+		idp = dt_dlib_add_probe_var(pcb->pcb_hdl, prp);
 		assert(idp != NULL);
 
 		/*
@@ -457,8 +484,8 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
 		 * process, and emit a sequence of clauses for it when it does.
 		 */
 		emit(dlp,  BPF_BRANCH_IMM(BPF_JNE, BPF_REG_0, pid, lbl_next));
-		emite(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_7, DMST_PRID, pprp->desc->id), idp);
-		dt_cg_tramp_call_clauses(pcb, pprp, DT_ACTIVITY_ACTIVE);
+		emite(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_7, DMST_PRID, prp->desc->id), idp);
+		dt_cg_tramp_call_clauses(pcb, prp, DT_ACTIVITY_ACTIVE);
 		emit(dlp,  BPF_JUMP(lbl_exit));
 		emitl(dlp, lbl_next,
 			   BPF_NOP());
@@ -508,8 +535,8 @@ copyout_val(dt_pcb_t *pcb, uint_t lbl, uint32_t val, int arg)
 static int trampoline_is_enabled(dt_pcb_t *pcb, uint_t exitlbl)
 {
 	dt_irlist_t		*dlp = &pcb->pcb_ir;
-	const dt_probe_t	*prp = pcb->pcb_probe;
-	const dt_uprobe_t	*upp = prp->prv_data;
+	const dt_probe_t	*uprp = pcb->pcb_probe;
+	const dt_uprobe_t	*upp = uprp->prv_data;
 	const list_probe_t	*pop;
 	uint_t			lbl_assign = dt_irlist_label(dlp);
 	uint_t			lbl_exit = pcb->pcb_exitlbl;
@@ -561,14 +588,14 @@ static int trampoline_is_enabled(dt_pcb_t *pcb, uint_t exitlbl)
 	 */
 	for (pop = dt_list_next(&upp->probes); pop != NULL;
 	     pop = dt_list_next(pop)) {
-		const dt_probe_t	*pprp = pop->probe;
+		const dt_probe_t	*prp = pop->probe;
 		pid_t			pid;
 		dt_ident_t		*idp;
 
-		pid = dt_pid_get_pid(pprp->desc, pcb->pcb_hdl, pcb, NULL);
+		pid = dt_pid_get_pid(prp->desc, pcb->pcb_hdl, pcb, NULL);
 		assert(pid != -1);
 
-		idp = dt_dlib_add_probe_var(pcb->pcb_hdl, pprp);
+		idp = dt_dlib_add_probe_var(pcb->pcb_hdl, prp);
 		assert(idp != NULL);
 
 		/*
@@ -636,9 +663,9 @@ out:
 	return name;
 }
 
-static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
+static int attach(dtrace_hdl_t *dtp, const dt_probe_t *uprp, int bpf_fd)
 {
-	dt_uprobe_t	*upp = prp->prv_data;
+	dt_uprobe_t	*upp = uprp->prv_data;
 	tp_probe_t	*tpp = upp->tp;
 	FILE		*f;
 	char		*fn;
@@ -733,9 +760,9 @@ out:
  * probes that didn't get created.  If the removal fails for some reason we are
  * out of luck - fortunately it is not harmful to the system as a whole.
  */
-static void detach(dtrace_hdl_t *dtp, const dt_probe_t *prp)
+static void detach(dtrace_hdl_t *dtp, const dt_probe_t *uprp)
 {
-	dt_uprobe_t	*upp = prp->prv_data;
+	dt_uprobe_t	*upp = uprp->prv_data;
 	tp_probe_t	*tpp = upp->tp;
 
 	if (!dt_tp_has_info(tpp))
-- 
2.18.4

[PATCH 08/38] Fix comment in dt_probe.c

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 libdtrace/dt_probe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
index e1099d75..0b53121a 100644
--- a/libdtrace/dt_probe.c
+++ b/libdtrace/dt_probe.c
@@ -838,7 +838,7 @@ dt_probe_lookup(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp)
 		 * To avoid checking multiple times whether an element in the
 		 * probe specification is a glob pattern, we (ab)use the
 		 * desc->id value (unused at this point) to store this
-		 * information a a bitmap.
+		 * information as a bitmap.
 		 */
 		desc = *pdp;
 		desc.id = (p_is_glob << 3) | (m_is_glob << 2) |
-- 
2.18.4

[PATCH 09/38] Fix comments that hardwire DBUF_ offsets

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 libdtrace/dt_cg.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
index e166d2d8..0977406a 100644
--- a/libdtrace/dt_cg.c
+++ b/libdtrace/dt_cg.c
@@ -1057,8 +1057,8 @@ dt_cg_tramp_error(dt_pcb_t *pcb)
  *
  *	1. Store the base pointer to the output data buffer in %r9.
  *	2. Initialize the machine state (dctx->mst).
- *	3. Store the epid at [%r9 + 0].
- *	4. Store 0 to indicate no active speculation at [%r9 + 4].
+ *	3. Store the epid at [%r9 + DBUF_EPID].
+ *	4. Store 0 to indicate no active speculation at [%r9 + DBUF_SPECID].
  *	5. Evaluate the predicate expression and return if false.
  *
  * The dt_program() function will always return 0.
-- 
2.18.4

[PATCH 10/38] Fix comments in dt_cg.c

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 libdtrace/dt_cg.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
index 0977406a..4fd2d359 100644
--- a/libdtrace/dt_cg.c
+++ b/libdtrace/dt_cg.c
@@ -1150,9 +1150,9 @@ dt_cg_prologue(dt_pcb_t *pcb, dt_node_t *pred)
 
 /*
  * Generate the function epilogue:
- *	4. Submit the buffer to the perf event output buffer for the current
+ *	1. Submit the buffer to the perf event output buffer for the current
  *	   cpu, if this is a data recording action..
- *	5. Return 0
+ *	2. Return 0
  * }
  */
 static void
-- 
2.18.4

[PATCH 11/38] USDT module names may contain dots; but forbid "." and ".." names

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 dtprobed/dof_stash.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/dtprobed/dof_stash.c b/dtprobed/dof_stash.c
index 62418b66..625572d5 100644
--- a/dtprobed/dof_stash.c
+++ b/dtprobed/dof_stash.c
@@ -231,6 +231,20 @@ make_probespec_name(const char *prov, const char *mod, const char *fn,
 {
 	char *ret;
 
+	/*
+	 * Ban "." and ".." as name components.  Obviously names
+	 * containing dots are commonplace (shared libraries,
+	 * for instance), but allowing straight . and .. would
+	 * have obviously horrible consequences.  They can't be
+	 * filenames anyway, and you can't create them with
+	 * dtrace -h because they aren't valid C identifier names.
+	 */
+	if (strcmp(prov, ".") == 0 || strcmp(prov, "..") == 0 ||
+	    strcmp(mod, ".") == 0 || strcmp(mod, "..") == 0 ||
+	    strcmp(fn, ".") == 0 || strcmp(fn, "..") == 0 ||
+	    strcmp(prb, ".") == 0 || strcmp(prb, "..") == 0)
+		return NULL;
+
 	if (asprintf(&ret, "%s:%s:%s:%s", prov, mod, fn, prb) < 0) {
 		fuse_log(FUSE_LOG_ERR, "dtprobed: out of memory making probespec\n");
 		return NULL;
@@ -589,22 +603,6 @@ dof_stash_write_parsed(pid_t pid, dev_t dev, ino_t ino, dt_list_t *accum)
 							    mod, fun, prb)) == NULL)
 				goto err_provider;
 
-			/*
-			 * Ban "." and ".." as name components.  Obviously names
-			 * containing dots are commonplace (shared libraries,
-			 * for instance), but allowing straight . and .. would
-			 * have obviously horrible consequences.  They can't be
-			 * filenames anyway, and you can't create them with
-			 * dtrace -h because they aren't valid C identifier
-			 * names.
-			 */
-			op = "probe name validation";
-			probe_err = parsedfn;
-
-			if (strcmp(parsedfn, ".") == 0 ||
-			    strcmp(parsedfn, "..") == 0)
-				goto err_provider;
-
 			op = "probe module";
 
 			if ((mod_dir = make_state_dirat(prov_dir, mod, op, 0)) < 0)
-- 
2.18.4

[PATCH 12/38] USDT module names may contain dots; remove incorrect check

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 libdtrace/dt_pid.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/libdtrace/dt_pid.c b/libdtrace/dt_pid.c
index 7c7d7e30..996543b1 100644
--- a/libdtrace/dt_pid.c
+++ b/libdtrace/dt_pid.c
@@ -833,14 +833,6 @@ dt_pid_create_usdt_probes(dtrace_hdl_t *dtp, dt_proc_t *dpr, dtrace_probedesc_t
 
 	assert(pvp->impl != NULL && pvp->impl->provide_probe != NULL);
 
-	if (strchr(pdp->prv, '.') != NULL ||
-	    strchr(pdp->mod, '.') != NULL ||
-	    strchr(pdp->fun, '.') != NULL ||
-	    strchr(pdp->prb, '.') != NULL) {
-		dt_dprintf("Probe component contains dots: cannot be a USDT probe.\n");
-		return 0;
-	}
-
 	if (asprintf(&probepath, "%s/probes/%i/%s/%s/%s/%s", dtp->dt_dofstash_path,
 		     dpr->dpr_pid, pdp->prv[0] == '\0' ? "*" : pdp->prv,
 		     pdp->mod[0] == '\0' ? "*" : pdp->mod,
-- 
2.18.4

[PATCH 13/38] Hide dtrace_actdesc_t until it is needed

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 include/dtrace/enabling.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/dtrace/enabling.h b/include/dtrace/enabling.h
index f1ec444c..2562c87b 100644
--- a/include/dtrace/enabling.h
+++ b/include/dtrace/enabling.h
@@ -43,6 +43,8 @@ typedef struct dtrace_probedesc {
 	const char	*prb;			/* probe name */
 } dtrace_probedesc_t;
 
+#ifdef FIXME
+This type is used only in #ifdef FIXME code.
 typedef struct dtrace_actdesc {
 	struct dtrace_difo *dtad_difo;		/* pointer to DIF object */
 	dtrace_actkind_t dtad_kind;		/* kind of action */
@@ -50,6 +52,7 @@ typedef struct dtrace_actdesc {
 	uint64_t dtad_arg;			/* action argument */
 	uint64_t dtad_uarg;			/* user argument */
 } dtrace_actdesc_t;
+#endif
 
 typedef struct dtrace_ecbdesc {
 	dtrace_probedesc_t dted_probe;		/* probe description */
-- 
2.18.4

[PATCH 14/38] Remove orphaned dtrace_hdl_t component dt_maxformat

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 libdtrace/dt_impl.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
index b5408c1c..3d3f58d5 100644
--- a/libdtrace/dt_impl.h
+++ b/libdtrace/dt_impl.h
@@ -368,7 +368,6 @@ struct dtrace_hdl {
 	dtrace_probedesc_t **dt_pdesc; /* probe descriptions for enabled prbs */
 	size_t dt_maxagg;	/* max aggregation ID */
 	dtrace_aggdesc_t **dt_adesc; /* aggregation descriptions */
-	int dt_maxformat;	/* max format ID */
 	dt_aggregate_t dt_aggregate; /* aggregate */
 	struct dt_pebset *dt_pebset; /* perf event buffers set */
 	struct dt_pfdict *dt_pfdict; /* dictionary of printf conversions */
-- 
2.18.4

[PATCH 15/38] Remove orphaned dtrace_hdl_t component dt_prov_usdt

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

This component was introduced by commit 95ac5e2496d1
("usdt: DTrace userspace side").

Its use was removed by commit e71a20d8
("dt_pid, dtprobed: move uprobe creation to dtrace"),
but it was left in dt_impl.h.

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 libdtrace/dt_impl.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
index 3d3f58d5..d15c238c 100644
--- a/libdtrace/dt_impl.h
+++ b/libdtrace/dt_impl.h
@@ -349,7 +349,6 @@ struct dtrace_hdl {
 
 	dt_htab_t *dt_provs;	/* hash table of dt_provider_t's */
 	const struct dt_provider *dt_prov_pid; /* PID provider */
-	const struct dt_provider *dt_prov_usdt; /* USDT provider */
 	int dt_proc_signal;	/* signal used to interrupt monitoring threads */
 	struct sigaction dt_proc_oact;
 	dt_proc_hash_t *dt_procs; /* hash table of grabbed process handles */
-- 
2.18.4

[PATCH 16/38] Move dt_probe_clause_t to be available outside of dt_probe.c

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 libdtrace/dt_impl.h  | 5 +++++
 libdtrace/dt_probe.c | 5 -----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
index d15c238c..2132eda2 100644
--- a/libdtrace/dt_impl.h
+++ b/libdtrace/dt_impl.h
@@ -283,6 +283,11 @@ typedef struct dt_percpu_drops {
 
 typedef uint32_t dt_version_t;		/* encoded version (see below) */
 
+typedef struct dt_probe_clause {
+	dt_list_t	list;
+	dt_ident_t	*clause;
+} dt_probe_clause_t;
+
 struct dtrace_hdl {
 	const dtrace_vector_t *dt_vector; /* library vector, if vectored open */
 	void *dt_varg;	/* vector argument, if vectored open */
diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
index 0b53121a..d0ae63d1 100644
--- a/libdtrace/dt_probe.c
+++ b/libdtrace/dt_probe.c
@@ -24,11 +24,6 @@
 #include <dt_list.h>
 #include <dt_bpf.h>
 
-typedef struct dt_probe_clause {
-	dt_list_t	list;
-	dt_ident_t	*clause;
-} dt_probe_clause_t;
-
 typedef struct dt_probe_dependent {
 	dt_list_t	list;
 	dt_probe_t	*probe;
-- 
2.18.4

[PATCH 17/38] Add a provider-specific probe_add_clause handle

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 libdtrace/dt_probe.c    | 2 ++
 libdtrace/dt_provider.h | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
index d0ae63d1..48b02e11 100644
--- a/libdtrace/dt_probe.c
+++ b/libdtrace/dt_probe.c
@@ -1354,6 +1354,8 @@ dt_probe_add_clause(dtrace_hdl_t *dtp, dt_probe_t *prp, dt_ident_t *idp)
 		pcp->clause = idp;
 
 	dt_list_append(&prp->clauses, pcp);
+	if (prp->prov->impl->probe_add_clause)
+		return prp->prov->impl->probe_add_clause(dtp, prp, idp);
 
 	return 0;
 }
diff --git a/libdtrace/dt_provider.h b/libdtrace/dt_provider.h
index 17b1844c..b1b1b1b8 100644
--- a/libdtrace/dt_provider.h
+++ b/libdtrace/dt_provider.h
@@ -62,6 +62,9 @@ typedef struct dt_provimpl {
 	int (*probe_info)(dtrace_hdl_t *dtp,	/* get probe info */
 			  const struct dt_probe *prp,
 			  int *argcp, dt_argdesc_t **argvp);
+	int (*probe_add_clause)(dtrace_hdl_t *dtp, /* add clause to probe */
+				struct dt_probe *prp,
+				dt_ident_t *idp);
 	void (*detach)(dtrace_hdl_t *dtp,	/* probe cleanup */
 		       const struct dt_probe *prb);
 	void (*probe_destroy)(dtrace_hdl_t *dtp, /* free probe data */
-- 
2.18.4

[PATCH 18/38] Add a provider-specific probe_add_clause for underlying probes

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 libdtrace/dt_prov_uprobe.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
index c77063a8..5dbd75e3 100644
--- a/libdtrace/dt_prov_uprobe.c
+++ b/libdtrace/dt_prov_uprobe.c
@@ -129,6 +129,38 @@ static void probe_destroy(dtrace_hdl_t *dtp, void *datap)
 	free_probe_list(dtp, datap);
 }
 
+/*
+ * Add clause to probe.
+ */
+static int probe_add_clause(dtrace_hdl_t *dtp, dt_probe_t *prp, dt_ident_t *idp)
+{
+	const list_probe_t	*pup;
+
+	for (pup = prp->prv_data; pup != NULL; pup = dt_list_next(pup)) {
+		dt_probe_t		*uprp = pup->probe;
+		dt_probe_clause_t	*prev, *pcp;
+
+		/*
+		 * Check if the clause is already there.  Since the clauses
+		 * are added "in order," we only need to check the previous
+		 * entry.
+		 */
+		prev = dt_list_prev(&uprp->clauses);
+		if (prev && strcmp(prev->clause->di_name, idp->di_name) == 0)
+			continue;
+
+		/*
+		 * Add the clause.
+		 */
+		pcp = dt_zalloc(dtp, sizeof(dt_probe_clause_t));
+		if (pcp == NULL)
+			return -1;
+		pcp->clause = idp;
+		dt_list_append(&uprp->clauses, pcp);
+	}
+
+	return 0;
+}
 
 /*
  * Look up or create an underlying (real) probe, corresponding directly to a
@@ -811,6 +843,7 @@ dt_provimpl_t	dt_pid = {
 	.prog_type	= BPF_PROG_TYPE_UNSPEC,
 	.provide_probe	= &provide_pid_probe,
 	.enable		= &enable_pid,
+	.probe_add_clause = &probe_add_clause,
 	.probe_destroy	= &probe_destroy,
 };
 
@@ -822,5 +855,6 @@ dt_provimpl_t	dt_usdt = {
 	.prog_type	= BPF_PROG_TYPE_UNSPEC,
 	.provide_probe	= &provide_usdt_probe,
 	.enable		= &enable_usdt,
+	.probe_add_clause = &probe_add_clause,
 	.probe_destroy	= &probe_destroy,
 };
-- 
2.18.4

Re: [PATCH 02/38] Move dt_spec_buf_data_t and dt_spec_buf_t into dt_consume.c

[Kris Van Hees]

On Thu, Jun 27, 2024 at 01:34:19AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

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

> ---
>  libdtrace/dt_consume.c | 18 ++++++++++++++++++
>  libdtrace/dt_impl.h    | 18 ------------------
>  2 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
> index dec2314b..5fb636fe 100644
> --- a/libdtrace/dt_consume.c
> +++ b/libdtrace/dt_consume.c
> @@ -27,6 +27,24 @@
>  
>  #define	DT_MASK_LO 0x00000000FFFFFFFFULL
>  
> +typedef struct dt_spec_buf_data {
> +	dt_list_t dsbd_list;		/* linked-list forward/back pointers */
> +	unsigned int dsbd_cpu;		/* cpu for data */
> +	char *dsbd_data;		/* data for later processing */
> +	uint32_t dsbd_size;		/* size of data */
> +} dt_spec_buf_data_t;
> +
> +typedef struct dt_spec_buf {
> +	dtrace_hdl_t *dtsb_dtp;		/* backpointer to the dtrace instance */
> +	int32_t dtsb_id;		/* speculation ID */
> +	size_t dtsb_size;		/* size of all buffers in this spec */
> +	int dtsb_committing;		/* when draining, nonzero if commit */
> +	dt_bpf_specs_t dtsb_spec;	/* bpf-side specs record for this spec
> +					   (buffer read/write counts).  */
> +	dt_list_t dtsb_dsbd_list;	/* list of dt_spec_bufs */
> +	struct dt_hentry dtsb_he;	/* htab links */
> +} dt_spec_buf_t;
> +
>  /*
>   * We declare this here because (1) we need it and (2) we want to avoid a
>   * dependency on libm in libdtrace.
> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> index 01313ff3..b5408c1c 100644
> --- a/libdtrace/dt_impl.h
> +++ b/libdtrace/dt_impl.h
> @@ -269,24 +269,6 @@ typedef struct dt_lib_depend {
>  	dt_list_t dtld_dependents;	/* linked-list of lib dependents */
>  } dt_lib_depend_t;
>  
> -typedef struct dt_spec_buf_data {
> -	dt_list_t dsbd_list;		/* linked-list forward/back pointers */
> -	unsigned int dsbd_cpu;		/* cpu for data */
> -	char *dsbd_data;		/* data for later processing */
> -	uint32_t dsbd_size;		/* size of data */
> -} dt_spec_buf_data_t;
> -
> -typedef struct dt_spec_buf {
> -	dtrace_hdl_t *dtsb_dtp;		/* backpointer to the dtrace instance */
> -	int32_t dtsb_id;		/* speculation ID */
> -	size_t dtsb_size;		/* size of all buffers in this spec */
> -	int dtsb_committing;		/* when draining, nonzero if commit */
> -	dt_bpf_specs_t dtsb_spec;	/* bpf-side specs record for this spec
> -					   (buffer read/write counts).  */
> -	dt_list_t dtsb_dsbd_list;	/* list of dt_spec_bufs */
> -	struct dt_hentry dtsb_he;	/* htab links */
> -} dt_spec_buf_t;
> -
>  typedef struct dt_percpu_drops {
>  	uint64_t	buf;		/* principal buffer drops */
>  	uint64_t	agg;		/* aggregate buffer drops */
> -- 
> 2.18.4
> 

Re: [PATCH 03/38] Get rid of apparently orphaned status[2]

[Kris Van Hees]

On Thu, Jun 27, 2024 at 01:34:20AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> This has apparently been replaced by the dtp component
> "dtrace_status_t dt_status[2]".

Not sure...  Even OpenSolaris already shows it as an unsused local array.  So
it is certainly deprecated.

> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

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

> ---
>  cmd/dtrace.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/cmd/dtrace.c b/cmd/dtrace.c
> index 9c820686..ba1c22c5 100644
> --- a/cmd/dtrace.c
> +++ b/cmd/dtrace.c
> @@ -1,6 +1,6 @@
>  /*
>   * Oracle Linux DTrace.
> - * Copyright (c) 2006, 2022, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2006, 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.
>   */
> @@ -916,7 +916,6 @@ main(int argc, char *argv[])
>  {
>  	dtrace_bufdesc_t buf;
>  	struct sigaction act, oact;
> -	dtrace_status_t status[2];
>  	dtrace_optval_t opt;
>  	dtrace_cmd_t *dcp;
>  
> @@ -941,7 +940,6 @@ main(int argc, char *argv[])
>  	g_argv[g_argc++] = argv[0];	/* propagate argv[0] to D as $0/$$0 */
>  	argv[0] = g_pname;		/* rewrite argv[0] for getopt errors */
>  
> -	memset(status, 0, sizeof(status));
>  	memset(&buf, 0, sizeof(buf));
>  
>  	/*
> -- 
> 2.18.4
> 

Re: [PATCH 04/38] Get rid of apparently orphaned bufdesc stuff

[Kris Van Hees]

On Thu, Jun 27, 2024 at 01:34:21AM -0400, eugene.loh@oracle.com wrote:

Another case of code that was even unused in Solaris (even though I believe
the libdtrace code used it - but we don't).  Good to get rid of it.

> From: Eugene Loh <eugene.loh@oracle.com>
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

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

> ---
>  cmd/dtrace.c                    |  3 ---
>  include/dtrace/buffer.h         | 42 ---------------------------------
>  include/dtrace/buffer_defines.h | 20 ----------------
>  include/dtrace/dtrace.h         |  3 +--
>  include/dtrace/ioctl.h          |  5 +---
>  5 files changed, 2 insertions(+), 71 deletions(-)
>  delete mode 100644 include/dtrace/buffer.h
>  delete mode 100644 include/dtrace/buffer_defines.h
> 
> diff --git a/cmd/dtrace.c b/cmd/dtrace.c
> index ba1c22c5..af354653 100644
> --- a/cmd/dtrace.c
> +++ b/cmd/dtrace.c
> @@ -914,7 +914,6 @@ intr(int signo)
>  int
>  main(int argc, char *argv[])
>  {
> -	dtrace_bufdesc_t buf;
>  	struct sigaction act, oact;
>  	dtrace_optval_t opt;
>  	dtrace_cmd_t *dcp;
> @@ -940,8 +939,6 @@ main(int argc, char *argv[])
>  	g_argv[g_argc++] = argv[0];	/* propagate argv[0] to D as $0/$$0 */
>  	argv[0] = g_pname;		/* rewrite argv[0] for getopt errors */
>  
> -	memset(&buf, 0, sizeof(buf));
> -
>  	/*
>  	 * Make an initial pass through argv[] processing any arguments that
>  	 * affect our behavior mode (g_mode) and flags used for dtrace_open().
> diff --git a/include/dtrace/buffer.h b/include/dtrace/buffer.h
> deleted file mode 100644
> index 6fa8c769..00000000
> --- a/include/dtrace/buffer.h
> +++ /dev/null
> @@ -1,42 +0,0 @@
> -/*
> - * Licensed under the Universal Permissive License v 1.0 as shown at
> - * http://oss.oracle.com/licenses/upl.
> - *
> - * Copyright (c) 2009, 2013, Oracle and/or its affiliates. All rights reserved.
> - */
> -
> -/*
> - * Note: The contents of this file are private to the implementation of the
> - * DTrace subsystem and are subject to change at any time without notice.
> - */
> -
> -#ifndef _DTRACE_BUFFER_H
> -#define _DTRACE_BUFFER_H
> -
> -#include <dtrace/universal.h>
> -#include <dtrace/actions_defines.h>
> -#include <dtrace/buffer_defines.h>
> -
> -/*
> - * In order to get a snapshot of the principal or aggregation buffer,
> - * user-level passes a buffer description to the kernel with the dtrace_bufdesc
> - * structure.  This describes which CPU user-level is interested in, and
> - * where user-level wishes the kernel to snapshot the buffer to (the
> - * dtbd_data field).  The kernel uses the same structure to pass back some
> - * information regarding the buffer:  the size of data actually copied out, the
> - * number of drops, the number of errors, and the offset of the oldest record.
> - * If the buffer policy is a "switch" policy, taking a snapshot of the
> - * principal buffer has the additional effect of switching the active and
> - * inactive buffers.  Taking a snapshot of the aggregation buffer _always_ has
> - * the additional effect of switching the active and inactive buffers.
> - */
> -typedef struct dtrace_bufdesc {
> -	uint64_t dtbd_size;			/* size of buffer */
> -	uint32_t dtbd_cpu;			/* CPU or DTRACE_CPUALL */
> -	uint32_t dtbd_errors;			/* number of errors */
> -	uint64_t dtbd_drops;			/* number of drops */
> -	DTRACE_PTR(char, dtbd_data);		/* data */
> -	uint64_t dtbd_oldest;			/* offset of oldest record */
> -} dtrace_bufdesc_t;
> -
> -#endif /* _DTRACE_BUFFER_H */
> diff --git a/include/dtrace/buffer_defines.h b/include/dtrace/buffer_defines.h
> deleted file mode 100644
> index f81c22f1..00000000
> --- a/include/dtrace/buffer_defines.h
> +++ /dev/null
> @@ -1,20 +0,0 @@
> -/*
> - * Licensed under the Universal Permissive License v 1.0 as shown at
> - * http://oss.oracle.com/licenses/upl.
> - *
> - * Copyright (c) 2009, 2013, Oracle and/or its affiliates. All rights reserved.
> - */
> -
> -/*
> - * Note: The contents of this file are private to the implementation of the
> - * DTrace subsystem and are subject to change at any time without notice.
> - */
> -
> -#ifndef _DTRACE_BUFFER_DEFINES_H
> -#define _DTRACE_BUFFER_DEFINES_H
> -
> -#include <dtrace/universal.h>
> -
> -struct dtrace_bufdesc;
> -
> -#endif /* _DTRACE_BUFFER_DEFINES_H */
> diff --git a/include/dtrace/dtrace.h b/include/dtrace/dtrace.h
> index 8f28c9ba..7fc5f5f7 100644
> --- a/include/dtrace/dtrace.h
> +++ b/include/dtrace/dtrace.h
> @@ -2,7 +2,7 @@
>   * Licensed under the Universal Permissive License v 1.0 as shown at
>   * http://oss.oracle.com/licenses/upl.
>   *
> - * Copyright (c) 2009, 2019, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2009, 2024, Oracle and/or its affiliates. All rights reserved.
>   */
>  
>  /*
> @@ -21,7 +21,6 @@
>  #include <dtrace/enabling.h>
>  #include <dtrace/metadesc.h>
>  #include <dtrace/options.h>
> -#include <dtrace/buffer.h>
>  #include <dtrace/status.h>
>  #include <dtrace/conf.h>
>  #include <dtrace/faults.h>
> diff --git a/include/dtrace/ioctl.h b/include/dtrace/ioctl.h
> index 75d02464..a2a3a93b 100644
> --- a/include/dtrace/ioctl.h
> +++ b/include/dtrace/ioctl.h
> @@ -2,7 +2,7 @@
>   * Licensed under the Universal Permissive License v 1.0 as shown at
>   * http://oss.oracle.com/licenses/upl.
>   *
> - * Copyright (c) 2009, 2019, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2009, 2024, Oracle and/or its affiliates. All rights reserved.
>   */
>  
>  #ifndef _DTRACE_IOCTL_H_
> @@ -10,7 +10,6 @@
>  
>  #include <linux/ioctl.h>
>  #include <dtrace/arg.h>
> -#include <dtrace/buffer.h>
>  #include <dtrace/conf.h>
>  #include <dtrace/dof.h>
>  #include <dtrace/enabling.h>
> @@ -22,10 +21,8 @@
>  #define DTRACEIOC		0xd4
>  #define DTRACEIOC_PROVIDER	_IOR(DTRACEIOC, 1, dtrace_providerdesc_t)
>  #define DTRACEIOC_PROBES	_IOR(DTRACEIOC, 2, dtrace_probedesc_t)
> -#define DTRACEIOC_BUFSNAP	_IOR(DTRACEIOC, 4, dtrace_bufdesc_t)
>  #define DTRACEIOC_PROBEMATCH	_IOR(DTRACEIOC, 5, dtrace_probedesc_t)
>  #define DTRACEIOC_ENABLE	_IOW(DTRACEIOC, 6, void *)
> -#define DTRACEIOC_AGGSNAP	_IOR(DTRACEIOC, 7, dtrace_bufdesc_t)
>  #define DTRACEIOC_EPROBE	_IOW(DTRACEIOC, 8, dtrace_eprobedesc_t)
>  #define DTRACEIOC_PROBEARG	_IOR(DTRACEIOC, 9, dtrace_argdesc_t)
>  #define DTRACEIOC_CONF		_IOR(DTRACEIOC, 10, dtrace_conf_t)
> -- 
> 2.18.4
> 

Re: [PATCH 06/38] Get rid of unused dtrace_repldesc_t

[Kris Van Hees]

On Thu, Jun 27, 2024 at 01:34:23AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>

This was only used in the kernel code, so definitely no longer relevant for
us.  And it was tied to the DTRACEIOC_REPLICATE ioctl that apparently was
only used by mdb, so even less relevant to us.  I am not even sure what the
expected semantics were.

> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

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

> ---
>  include/dtrace/enabling.h | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/include/dtrace/enabling.h b/include/dtrace/enabling.h
> index d36a73df..f1ec444c 100644
> --- a/include/dtrace/enabling.h
> +++ b/include/dtrace/enabling.h
> @@ -43,11 +43,6 @@ typedef struct dtrace_probedesc {
>  	const char	*prb;			/* probe name */
>  } dtrace_probedesc_t;
>  
> -typedef struct dtrace_repldesc {
> -	dtrace_probedesc_t dtrpd_match;		/* probe descr. to match */
> -	dtrace_probedesc_t dtrpd_create;	/* probe descr. to create */
> -} dtrace_repldesc_t;
> -
>  typedef struct dtrace_actdesc {
>  	struct dtrace_difo *dtad_difo;		/* pointer to DIF object */
>  	dtrace_actkind_t dtad_kind;		/* kind of action */
> -- 
> 2.18.4
> 

Re: [PATCH 05/38] Get rid of unneeded enabling_defines.h

[Kris Van Hees]

On Thu, Jun 27, 2024 at 01:34:22AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>

Again, stuff from the (old) kernel side of things.

> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

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

> ---
>  include/dtrace/enabling.h         |  3 +--
>  include/dtrace/enabling_defines.h | 23 -----------------------
>  2 files changed, 1 insertion(+), 25 deletions(-)
>  delete mode 100644 include/dtrace/enabling_defines.h
> 
> diff --git a/include/dtrace/enabling.h b/include/dtrace/enabling.h
> index 41e50cda..d36a73df 100644
> --- a/include/dtrace/enabling.h
> +++ b/include/dtrace/enabling.h
> @@ -2,7 +2,7 @@
>   * Licensed under the Universal Permissive License v 1.0 as shown at
>   * http://oss.oracle.com/licenses/upl.
>   *
> - * Copyright (c) 2009, 2019, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2009, 2024, Oracle and/or its affiliates. All rights reserved.
>   */
>  
>  /*
> @@ -15,7 +15,6 @@
>  
>  #include <dtrace/universal.h>
>  #include <dtrace/difo_defines.h>
> -#include <dtrace/enabling_defines.h>
>  
>  /*
>   * FIXME: Needs to be rewritten.
> diff --git a/include/dtrace/enabling_defines.h b/include/dtrace/enabling_defines.h
> deleted file mode 100644
> index 875b49d0..00000000
> --- a/include/dtrace/enabling_defines.h
> +++ /dev/null
> @@ -1,23 +0,0 @@
> -/*
> - * Licensed under the Universal Permissive License v 1.0 as shown at
> - * http://oss.oracle.com/licenses/upl.
> - *
> - * Copyright (c) 2009, 2013, Oracle and/or its affiliates. All rights reserved.
> - */
> -
> -/*
> - * Note: The contents of this file are private to the implementation of the
> - * DTrace subsystem and are subject to change at any time without notice.
> - */
> -
> -#ifndef _DTRACE_ENABLING_DEFINES_H
> -#define _DTRACE_ENABLING_DEFINES_H
> -
> -#include <dtrace/universal.h>
> -
> -struct dtrace_probedesc;
> -struct dtrace_repldesc;
> -struct dtrace_actdesc;
> -struct dtrace_ecbdesc;
> -
> -#endif /* _DTRACE_ENABLING_DEFINES_H */
> -- 
> 2.18.4
> 

Re: [PATCH 07/38] Clean up prp/pprp/uprp variable names

[Kris Van Hees]

On Thu, Jun 27, 2024 at 01:34:24AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> ---
>  libdtrace/dt_prov_uprobe.c | 83 +++++++++++++++++++++++++-------------
>  1 file changed, 55 insertions(+), 28 deletions(-)
> 
> diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
> index afc1f628..c77063a8 100644
> --- a/libdtrace/dt_prov_uprobe.c
> +++ b/libdtrace/dt_prov_uprobe.c
> @@ -6,6 +6,31 @@
>   *
>   * The uprobe-based provider for DTrace (implementing pid and USDT providers).
>   */
> +/*
> + * This file uses both overlying probes (specified by the user) as well as
> + * underlying probes (the uprobes recognized by the kernel).  To minimize
> + * confusion, this file should use consistent variable names:
> + *
> + *     dt_probe_t	*prp;   //  overlying probe
> + *     dt_probe_t	*uprp;  // underlying probe
> + *
> + *             Either one might be returned by dt_probe_lookup() or
> + *             dt_probe_insert() or added to dt_enablings[] or dt_probes[].
> + *             Further, uprp might be returned by create_underlying().
> + *
> + *     dt_uprobe_t	*upp;   // uprobe associated with an underlying probe
> + *
> + *     list_probe_t	*pop;   //  overlying probe list
> + *     list_probe_t	*pup;   // underlying probe list
> + *
> + * The provider-specific prv_data has these meanings:
> + *
> + *     prp->prv_data            // dt_list_t of associated underlying probes
> + *
> + *     uprp->prv_data           // upp (the associated uprobe)
> + *
> + * Finally, note that upp->probes is a dt_list_t of overlying probes.
> + */

As mentioned in my earlier review, I truly believe that this comment block is
not necessary because the code changes includes in the patch accomplish what
is described here and I think that sets up enough of a precedent/example that
future changes in code should stick to that naming pattern.  No need for an
explicit comment block to highlight that.

>  #include <sys/types.h>
>  #include <assert.h>
>  #include <errno.h>
> @@ -118,7 +143,7 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
>  	char			mod[DTRACE_MODNAMELEN];
>  	char			prb[DTRACE_NAMELEN];
>  	dtrace_probedesc_t	pd;
> -	dt_probe_t		*prp;
> +	dt_probe_t		*uprp;
>  	dt_uprobe_t		*upp;
>  	int			is_enabled = 0;
>  
> @@ -160,8 +185,8 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
>  	pd.fun = psp->pps_fun;
>  	pd.prb = prb;
>  
> -	prp = dt_probe_lookup(dtp, &pd);
> -	if (prp == NULL) {
> +	uprp = dt_probe_lookup(dtp, &pd);
> +	if (uprp == NULL) {
>  		dt_provider_t	*pvp;
>  
>  		/* Get the provider for underlying probes. */
> @@ -182,12 +207,12 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
>  		if (upp->tp == NULL)
>  			goto fail;
>  
> -		prp = dt_probe_insert(dtp, pvp, pd.prv, pd.mod, pd.fun, pd.prb,
> +		uprp = dt_probe_insert(dtp, pvp, pd.prv, pd.mod, pd.fun, pd.prb,
>  				      upp);
> -		if (prp == NULL)
> +		if (uprp == NULL)
>  			goto fail;
>  	} else
> -		upp = prp->prv_data;
> +		upp = uprp->prv_data;
>  
>  	switch (psp->pps_type) {
>  	case DTPPT_RETURN:
> @@ -202,7 +227,7 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
>  	     */
>  	}
>  
> -	return prp;
> +	return uprp;
>  
>  fail:
>  	probe_destroy(dtp, upp);
> @@ -259,7 +284,7 @@ static int provide_probe(dtrace_hdl_t *dtp, const pid_probespec_t *psp,
>  	}
>  
>  	/*
> -	 * Underlying and overlying probe list entries.
> +	 * Overlying and underlying probe list entries.
>  	 */
>  	pop = dt_zalloc(dtp, sizeof(list_probe_t));
>  	if (pop == NULL)
> @@ -271,7 +296,10 @@ static int provide_probe(dtrace_hdl_t *dtp, const pid_probespec_t *psp,
>  		return -1;
>  	}
>  
> -	/* Add the pid probe, if we need to. */
> +	/*
> +	 * Add the underlying probe to the list of probes for the overlying probe,
> +	 * adding the overlying probe if we need to.
> +	 */
>  
>  	pup->probe = uprp;
>  	if (prp == NULL)
> @@ -286,11 +314,10 @@ static int provide_probe(dtrace_hdl_t *dtp, const pid_probespec_t *psp,
>  		return -1;
>  	}
>  
> -	pop->probe = prp;
> -
>  	/*
> -	 * Add the pid probe to the list of probes for the underlying probe.
> +	 * Add the overlying probe to the list of probes for the underlying probe.
>  	 */
> +	pop->probe = prp;
>  	dt_list_append(&upp->probes, pop);
>  
>  	return 0;
> @@ -394,8 +421,8 @@ static void enable_usdt(dtrace_hdl_t *dtp, dt_probe_t *prp)
>  static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
>  {
>  	dt_irlist_t		*dlp = &pcb->pcb_ir;
> -	const dt_probe_t	*prp = pcb->pcb_probe;
> -	const dt_uprobe_t	*upp = prp->prv_data;
> +	const dt_probe_t	*uprp = pcb->pcb_probe;
> +	const dt_uprobe_t	*upp = uprp->prv_data;
>  	const list_probe_t	*pop;
>  	uint_t			lbl_exit = pcb->pcb_exitlbl;
>  
> @@ -441,15 +468,15 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
>  	 */
>  	for (pop = dt_list_next(&upp->probes); pop != NULL;
>  	     pop = dt_list_next(pop)) {
> -		const dt_probe_t	*pprp = pop->probe;
> +		const dt_probe_t	*prp = pop->probe;
>  		uint_t			lbl_next = dt_irlist_label(dlp);
>  		pid_t			pid;
>  		dt_ident_t		*idp;
>  
> -		pid = dt_pid_get_pid(pprp->desc, pcb->pcb_hdl, pcb, NULL);
> +		pid = dt_pid_get_pid(prp->desc, pcb->pcb_hdl, pcb, NULL);
>  		assert(pid != -1);
>  
> -		idp = dt_dlib_add_probe_var(pcb->pcb_hdl, pprp);
> +		idp = dt_dlib_add_probe_var(pcb->pcb_hdl, prp);
>  		assert(idp != NULL);
>  
>  		/*
> @@ -457,8 +484,8 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
>  		 * process, and emit a sequence of clauses for it when it does.
>  		 */
>  		emit(dlp,  BPF_BRANCH_IMM(BPF_JNE, BPF_REG_0, pid, lbl_next));
> -		emite(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_7, DMST_PRID, pprp->desc->id), idp);
> -		dt_cg_tramp_call_clauses(pcb, pprp, DT_ACTIVITY_ACTIVE);
> +		emite(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_7, DMST_PRID, prp->desc->id), idp);
> +		dt_cg_tramp_call_clauses(pcb, prp, DT_ACTIVITY_ACTIVE);
>  		emit(dlp,  BPF_JUMP(lbl_exit));
>  		emitl(dlp, lbl_next,
>  			   BPF_NOP());
> @@ -508,8 +535,8 @@ copyout_val(dt_pcb_t *pcb, uint_t lbl, uint32_t val, int arg)
>  static int trampoline_is_enabled(dt_pcb_t *pcb, uint_t exitlbl)
>  {
>  	dt_irlist_t		*dlp = &pcb->pcb_ir;
> -	const dt_probe_t	*prp = pcb->pcb_probe;
> -	const dt_uprobe_t	*upp = prp->prv_data;
> +	const dt_probe_t	*uprp = pcb->pcb_probe;
> +	const dt_uprobe_t	*upp = uprp->prv_data;
>  	const list_probe_t	*pop;
>  	uint_t			lbl_assign = dt_irlist_label(dlp);
>  	uint_t			lbl_exit = pcb->pcb_exitlbl;
> @@ -561,14 +588,14 @@ static int trampoline_is_enabled(dt_pcb_t *pcb, uint_t exitlbl)
>  	 */
>  	for (pop = dt_list_next(&upp->probes); pop != NULL;
>  	     pop = dt_list_next(pop)) {
> -		const dt_probe_t	*pprp = pop->probe;
> +		const dt_probe_t	*prp = pop->probe;
>  		pid_t			pid;
>  		dt_ident_t		*idp;
>  
> -		pid = dt_pid_get_pid(pprp->desc, pcb->pcb_hdl, pcb, NULL);
> +		pid = dt_pid_get_pid(prp->desc, pcb->pcb_hdl, pcb, NULL);
>  		assert(pid != -1);
>  
> -		idp = dt_dlib_add_probe_var(pcb->pcb_hdl, pprp);
> +		idp = dt_dlib_add_probe_var(pcb->pcb_hdl, prp);
>  		assert(idp != NULL);
>  
>  		/*
> @@ -636,9 +663,9 @@ out:
>  	return name;
>  }
>  
> -static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
> +static int attach(dtrace_hdl_t *dtp, const dt_probe_t *uprp, int bpf_fd)
>  {
> -	dt_uprobe_t	*upp = prp->prv_data;
> +	dt_uprobe_t	*upp = uprp->prv_data;
>  	tp_probe_t	*tpp = upp->tp;
>  	FILE		*f;
>  	char		*fn;
> @@ -733,9 +760,9 @@ out:
>   * probes that didn't get created.  If the removal fails for some reason we are
>   * out of luck - fortunately it is not harmful to the system as a whole.
>   */
> -static void detach(dtrace_hdl_t *dtp, const dt_probe_t *prp)
> +static void detach(dtrace_hdl_t *dtp, const dt_probe_t *uprp)
>  {
> -	dt_uprobe_t	*upp = prp->prv_data;
> +	dt_uprobe_t	*upp = uprp->prv_data;
>  	tp_probe_t	*tpp = upp->tp;
>  
>  	if (!dt_tp_has_info(tpp))
> -- 
> 2.18.4
> 

Re: [PATCH 08/38] Fix comment in dt_probe.c

[Kris Van Hees]

On Thu, Jun 27, 2024 at 01:34:25AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

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

> ---
>  libdtrace/dt_probe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
> index e1099d75..0b53121a 100644
> --- a/libdtrace/dt_probe.c
> +++ b/libdtrace/dt_probe.c
> @@ -838,7 +838,7 @@ dt_probe_lookup(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp)
>  		 * To avoid checking multiple times whether an element in the
>  		 * probe specification is a glob pattern, we (ab)use the
>  		 * desc->id value (unused at this point) to store this
> -		 * information a a bitmap.
> +		 * information as a bitmap.
>  		 */
>  		desc = *pdp;
>  		desc.id = (p_is_glob << 3) | (m_is_glob << 2) |
> -- 
> 2.18.4
> 

Re: [PATCH 09/38] Fix comments that hardwire DBUF_ offsets

[Kris Van Hees]

On Thu, Jun 27, 2024 at 01:34:26AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

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

> ---
>  libdtrace/dt_cg.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index e166d2d8..0977406a 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -1057,8 +1057,8 @@ dt_cg_tramp_error(dt_pcb_t *pcb)
>   *
>   *	1. Store the base pointer to the output data buffer in %r9.
>   *	2. Initialize the machine state (dctx->mst).
> - *	3. Store the epid at [%r9 + 0].
> - *	4. Store 0 to indicate no active speculation at [%r9 + 4].
> + *	3. Store the epid at [%r9 + DBUF_EPID].
> + *	4. Store 0 to indicate no active speculation at [%r9 + DBUF_SPECID].
>   *	5. Evaluate the predicate expression and return if false.
>   *
>   * The dt_program() function will always return 0.
> -- 
> 2.18.4
> 

Re: [PATCH 11/38] USDT module names may contain dots; but forbid "." and ".." names

[Kris Van Hees]

On Thu, Jun 27, 2024 at 01:34:28AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

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

> ---
>  dtprobed/dof_stash.c | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/dtprobed/dof_stash.c b/dtprobed/dof_stash.c
> index 62418b66..625572d5 100644
> --- a/dtprobed/dof_stash.c
> +++ b/dtprobed/dof_stash.c
> @@ -231,6 +231,20 @@ make_probespec_name(const char *prov, const char *mod, const char *fn,
>  {
>  	char *ret;
>  
> +	/*
> +	 * Ban "." and ".." as name components.  Obviously names
> +	 * containing dots are commonplace (shared libraries,
> +	 * for instance), but allowing straight . and .. would
> +	 * have obviously horrible consequences.  They can't be
> +	 * filenames anyway, and you can't create them with
> +	 * dtrace -h because they aren't valid C identifier names.
> +	 */
> +	if (strcmp(prov, ".") == 0 || strcmp(prov, "..") == 0 ||
> +	    strcmp(mod, ".") == 0 || strcmp(mod, "..") == 0 ||
> +	    strcmp(fn, ".") == 0 || strcmp(fn, "..") == 0 ||
> +	    strcmp(prb, ".") == 0 || strcmp(prb, "..") == 0)
> +		return NULL;
> +
>  	if (asprintf(&ret, "%s:%s:%s:%s", prov, mod, fn, prb) < 0) {
>  		fuse_log(FUSE_LOG_ERR, "dtprobed: out of memory making probespec\n");
>  		return NULL;
> @@ -589,22 +603,6 @@ dof_stash_write_parsed(pid_t pid, dev_t dev, ino_t ino, dt_list_t *accum)
>  							    mod, fun, prb)) == NULL)
>  				goto err_provider;
>  
> -			/*
> -			 * Ban "." and ".." as name components.  Obviously names
> -			 * containing dots are commonplace (shared libraries,
> -			 * for instance), but allowing straight . and .. would
> -			 * have obviously horrible consequences.  They can't be
> -			 * filenames anyway, and you can't create them with
> -			 * dtrace -h because they aren't valid C identifier
> -			 * names.
> -			 */
> -			op = "probe name validation";
> -			probe_err = parsedfn;
> -
> -			if (strcmp(parsedfn, ".") == 0 ||
> -			    strcmp(parsedfn, "..") == 0)
> -				goto err_provider;
> -
>  			op = "probe module";
>  
>  			if ((mod_dir = make_state_dirat(prov_dir, mod, op, 0)) < 0)
> -- 
> 2.18.4
> 

Re: [PATCH 12/38] USDT module names may contain dots; remove incorrect check

[Kris Van Hees]

On Thu, Jun 27, 2024 at 01:34:29AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

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

> ---
>  libdtrace/dt_pid.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/libdtrace/dt_pid.c b/libdtrace/dt_pid.c
> index 7c7d7e30..996543b1 100644
> --- a/libdtrace/dt_pid.c
> +++ b/libdtrace/dt_pid.c
> @@ -833,14 +833,6 @@ dt_pid_create_usdt_probes(dtrace_hdl_t *dtp, dt_proc_t *dpr, dtrace_probedesc_t
>  
>  	assert(pvp->impl != NULL && pvp->impl->provide_probe != NULL);
>  
> -	if (strchr(pdp->prv, '.') != NULL ||
> -	    strchr(pdp->mod, '.') != NULL ||
> -	    strchr(pdp->fun, '.') != NULL ||
> -	    strchr(pdp->prb, '.') != NULL) {
> -		dt_dprintf("Probe component contains dots: cannot be a USDT probe.\n");
> -		return 0;
> -	}
> -
>  	if (asprintf(&probepath, "%s/probes/%i/%s/%s/%s/%s", dtp->dt_dofstash_path,
>  		     dpr->dpr_pid, pdp->prv[0] == '\0' ? "*" : pdp->prv,
>  		     pdp->mod[0] == '\0' ? "*" : pdp->mod,
> -- 
> 2.18.4
> 

Re: [PATCH 10/38] Fix comments in dt_cg.c

[Kris Van Hees]

On Thu, Jun 27, 2024 at 01:34:27AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> ---
>  libdtrace/dt_cg.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 0977406a..4fd2d359 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -1150,9 +1150,9 @@ dt_cg_prologue(dt_pcb_t *pcb, dt_node_t *pred)
>  
>  /*
>   * Generate the function epilogue:
> - *	4. Submit the buffer to the perf event output buffer for the current
> + *	1. Submit the buffer to the perf event output buffer for the current
>   *	   cpu, if this is a data recording action..
> - *	5. Return 0
> + *	2. Return 0

This numbering was meant to continue the numbering that is in the comment block
for dt_cg_prologue(), which now would make this be 6 and 7.  The thought behind
that numbering (which was correctly sequential until we added speculation stuff
to the prologue) is that 1 through 7 cover all the steps of code that is
generated for a function.

But if you think that is too comfusing, I am ok with going with 1 and 2 here.
It just changes what this was meant to convey.

>   * }
>   */
>  static void
> -- 
> 2.18.4
> 

Re: [PATCH 13/38] Hide dtrace_actdesc_t until it is needed

[Kris Van Hees]

On Thu, Jun 27, 2024 at 01:34:30AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

I think you should just get rid of it.  Worst case, we can always revert the
patch.  But this type is only used in code that we do not plan to revive since
we no longer do DOF the same way as the older version.  Even if we were to
re-implement the functionality to save compiled probe programs to file, it
would look very different and certainly not be action-based but rather BPF
code based.

> ---
>  include/dtrace/enabling.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/dtrace/enabling.h b/include/dtrace/enabling.h
> index f1ec444c..2562c87b 100644
> --- a/include/dtrace/enabling.h
> +++ b/include/dtrace/enabling.h
> @@ -43,6 +43,8 @@ typedef struct dtrace_probedesc {
>  	const char	*prb;			/* probe name */
>  } dtrace_probedesc_t;
>  
> +#ifdef FIXME
> +This type is used only in #ifdef FIXME code.
>  typedef struct dtrace_actdesc {
>  	struct dtrace_difo *dtad_difo;		/* pointer to DIF object */
>  	dtrace_actkind_t dtad_kind;		/* kind of action */
> @@ -50,6 +52,7 @@ typedef struct dtrace_actdesc {
>  	uint64_t dtad_arg;			/* action argument */
>  	uint64_t dtad_uarg;			/* user argument */
>  } dtrace_actdesc_t;
> +#endif
>  
>  typedef struct dtrace_ecbdesc {
>  	dtrace_probedesc_t dted_probe;		/* probe description */
> -- 
> 2.18.4
> 

Re: [PATCH 15/38] Remove orphaned dtrace_hdl_t component dt_prov_usdt

[Kris Van Hees]

On Thu, Jun 27, 2024 at 01:34:32AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> This component was introduced by commit 95ac5e2496d1
> ("usdt: DTrace userspace side").
> 
> Its use was removed by commit e71a20d8
> ("dt_pid, dtprobed: move uprobe creation to dtrace"),
> but it was left in dt_impl.h.
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

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

> ---
>  libdtrace/dt_impl.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> index 3d3f58d5..d15c238c 100644
> --- a/libdtrace/dt_impl.h
> +++ b/libdtrace/dt_impl.h
> @@ -349,7 +349,6 @@ struct dtrace_hdl {
>  
>  	dt_htab_t *dt_provs;	/* hash table of dt_provider_t's */
>  	const struct dt_provider *dt_prov_pid; /* PID provider */
> -	const struct dt_provider *dt_prov_usdt; /* USDT provider */
>  	int dt_proc_signal;	/* signal used to interrupt monitoring threads */
>  	struct sigaction dt_proc_oact;
>  	dt_proc_hash_t *dt_procs; /* hash table of grabbed process handles */
> -- 
> 2.18.4
> 

Re: [PATCH 14/38] Remove orphaned dtrace_hdl_t component dt_maxformat

[Kris Van Hees]

On Thu, Jun 27, 2024 at 01:34:31AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

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

> ---
>  libdtrace/dt_impl.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> index b5408c1c..3d3f58d5 100644
> --- a/libdtrace/dt_impl.h
> +++ b/libdtrace/dt_impl.h
> @@ -368,7 +368,6 @@ struct dtrace_hdl {
>  	dtrace_probedesc_t **dt_pdesc; /* probe descriptions for enabled prbs */
>  	size_t dt_maxagg;	/* max aggregation ID */
>  	dtrace_aggdesc_t **dt_adesc; /* aggregation descriptions */
> -	int dt_maxformat;	/* max format ID */
>  	dt_aggregate_t dt_aggregate; /* aggregate */
>  	struct dt_pebset *dt_pebset; /* perf event buffers set */
>  	struct dt_pfdict *dt_pfdict; /* dictionary of printf conversions */
> -- 
> 2.18.4
> 

Re: [PATCH 07/38] Clean up prp/pprp/uprp variable names

[Eugene Loh]

On 7/18/24 14:48, Kris Van Hees wrote:

> On Thu, Jun 27, 2024 at 01:34:24AM -0400, eugene.loh@oracle.com wrote:
>> diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
>> @@ -6,6 +6,31 @@
>> +/*
>> + * This file uses both overlying probes (specified by the user) as well as
>> + * underlying probes (the uprobes recognized by the kernel).  To minimize
>> + * confusion, this file should use consistent variable names:
>> + *
>> + *     dt_probe_t	*prp;   //  overlying probe
>> + *     dt_probe_t	*uprp;  // underlying probe
>> + *
>> + *             Either one might be returned by dt_probe_lookup() or
>> + *             dt_probe_insert() or added to dt_enablings[] or dt_probes[].
>> + *             Further, uprp might be returned by create_underlying().
>> + *
>> + *     dt_uprobe_t	*upp;   // uprobe associated with an underlying probe
>> + *
>> + *     list_probe_t	*pop;   //  overlying probe list
>> + *     list_probe_t	*pup;   // underlying probe list
>> + *
>> + * The provider-specific prv_data has these meanings:
>> + *
>> + *     prp->prv_data            // dt_list_t of associated underlying probes
>> + *
>> + *     uprp->prv_data           // upp (the associated uprobe)
>> + *
>> + * Finally, note that upp->probes is a dt_list_t of overlying probes.
>> + */
> As mentioned in my earlier review, I truly believe that this comment block is
> not necessary because the code changes includes in the patch accomplish what
> is described here and I think that sets up enough of a precedent/example that
> future changes in code should stick to that naming pattern.  No need for an
> explicit comment block to highlight that.

One more round of negotiation, but then I'll go with whatever you 
respond in your next reply.

I'm a fan of code that is so well written that there is no need for 
comments.

But at the same time, if we rely on maintainers to read between the 
lines what is intended, there is a risk of things going wrong. E.g., 
someone might come from another file where they're used to prp for every 
probe (which is what was going on in this file, making things somewhat 
confusing in my mind).  Also, the comment points out the important 
distinction that prv_data means different things for different probes.

The comment block seems to me to be a clear statement of what practice 
to employ in this file.  I've spent a bunch of time reverse-engineering 
code to figure out what was going on in some writer's mind.  It becomes 
clear once I figure it out, but an explanation would have been so welcome.

Again, though, I'll go with whichever way you say in your next reply.

Re: [PATCH 16/38] Move dt_probe_clause_t to be available outside of dt_probe.c

[Kris Van Hees]

On Thu, Jun 27, 2024 at 01:34:33AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>

This certainly needs some motivation in the commit message, because it is not
at all clear why this is needed.  Sure, the next patch in the series uses it
outside of dtrace_probe, but that patch also does not provide any motivation
on why that is needed (and same with the patch after that).  So, these three
patches (16-18) should be combined, and bear a commit message that explains
why this is needed.  That will help in reviewing this code.

> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> ---
>  libdtrace/dt_impl.h  | 5 +++++
>  libdtrace/dt_probe.c | 5 -----
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> index d15c238c..2132eda2 100644
> --- a/libdtrace/dt_impl.h
> +++ b/libdtrace/dt_impl.h
> @@ -283,6 +283,11 @@ typedef struct dt_percpu_drops {
>  
>  typedef uint32_t dt_version_t;		/* encoded version (see below) */
>  
> +typedef struct dt_probe_clause {
> +	dt_list_t	list;
> +	dt_ident_t	*clause;
> +} dt_probe_clause_t;
> +
>  struct dtrace_hdl {
>  	const dtrace_vector_t *dt_vector; /* library vector, if vectored open */
>  	void *dt_varg;	/* vector argument, if vectored open */
> diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
> index 0b53121a..d0ae63d1 100644
> --- a/libdtrace/dt_probe.c
> +++ b/libdtrace/dt_probe.c
> @@ -24,11 +24,6 @@
>  #include <dt_list.h>
>  #include <dt_bpf.h>
>  
> -typedef struct dt_probe_clause {
> -	dt_list_t	list;
> -	dt_ident_t	*clause;
> -} dt_probe_clause_t;
> -
>  typedef struct dt_probe_dependent {
>  	dt_list_t	list;
>  	dt_probe_t	*probe;
> -- 
> 2.18.4
> 

Re: [PATCH 10/38] Fix comments in dt_cg.c

[Eugene Loh]

On 7/18/24 15:28, Kris Van Hees wrote:
> On Thu, Jun 27, 2024 at 01:34:27AM -0400, eugene.loh@oracle.com wrote:
>> From: Eugene Loh <eugene.loh@oracle.com>
>>
>> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
>> ---
>>   libdtrace/dt_cg.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>> index 0977406a..4fd2d359 100644
>> --- a/libdtrace/dt_cg.c
>> +++ b/libdtrace/dt_cg.c
>> @@ -1150,9 +1150,9 @@ dt_cg_prologue(dt_pcb_t *pcb, dt_node_t *pred)
>>   
>>   /*
>>    * Generate the function epilogue:
>> - *	4. Submit the buffer to the perf event output buffer for the current
>> + *	1. Submit the buffer to the perf event output buffer for the current
>>    *	   cpu, if this is a data recording action..
>> - *	5. Return 0
>> + *	2. Return 0
> This numbering was meant to continue the numbering that is in the comment block
> for dt_cg_prologue(), which now would make this be 6 and 7.  The thought behind
> that numbering (which was correctly sequential until we added speculation stuff
> to the prologue) is that 1 through 7 cover all the steps of code that is
> generated for a function.
>
> But if you think that is too comfusing, I am ok with going with 1 and 2 here.
> It just changes what this was meant to convey.

Yeah.  I get the intention, but it's easy to imagine someone tweaking 
numbers in the comment for prologue() not realizing that they also have 
to update comments 100 lines away for a different function.

I'm actually a fan of getting rid of the numbers altogether.  All they 
mean is "in this order," but the order is already clear.  It's not like 
we refer again to these numbers.  So, they're just extra stuff to 
maintain, with no useful value.  (Also, why continue any numbering when 
we don't number the stuff between the prologue and epilogue?  Kind of a 
rhetorical question since that just heads off into some fruitless 
discussion.)

So:

*)  R-b for the patch as is?

*)  R-b for a version of the patch that changes from numbered to 
unnumbered bullets for dt_cg_prologue() and dt_cg_epilogue() comments?

>
>>    * }
>>    */
>>   static void
>> -- 
>> 2.18.4
>>

Re: [PATCH 17/38] Add a provider-specific probe_add_clause handle

[Kris Van Hees]

See comments for 16/38

On Thu, Jun 27, 2024 at 01:34:34AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> ---
>  libdtrace/dt_probe.c    | 2 ++
>  libdtrace/dt_provider.h | 3 +++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
> index d0ae63d1..48b02e11 100644
> --- a/libdtrace/dt_probe.c
> +++ b/libdtrace/dt_probe.c
> @@ -1354,6 +1354,8 @@ dt_probe_add_clause(dtrace_hdl_t *dtp, dt_probe_t *prp, dt_ident_t *idp)
>  		pcp->clause = idp;
>  
>  	dt_list_append(&prp->clauses, pcp);
> +	if (prp->prov->impl->probe_add_clause)
> +		return prp->prov->impl->probe_add_clause(dtp, prp, idp);
>  
>  	return 0;
>  }
> diff --git a/libdtrace/dt_provider.h b/libdtrace/dt_provider.h
> index 17b1844c..b1b1b1b8 100644
> --- a/libdtrace/dt_provider.h
> +++ b/libdtrace/dt_provider.h
> @@ -62,6 +62,9 @@ typedef struct dt_provimpl {
>  	int (*probe_info)(dtrace_hdl_t *dtp,	/* get probe info */
>  			  const struct dt_probe *prp,
>  			  int *argcp, dt_argdesc_t **argvp);
> +	int (*probe_add_clause)(dtrace_hdl_t *dtp, /* add clause to probe */
> +				struct dt_probe *prp,
> +				dt_ident_t *idp);
>  	void (*detach)(dtrace_hdl_t *dtp,	/* probe cleanup */
>  		       const struct dt_probe *prb);
>  	void (*probe_destroy)(dtrace_hdl_t *dtp, /* free probe data */
> -- 
> 2.18.4
> 

Re: [PATCH 18/38] Add a provider-specific probe_add_clause for underlying probes

[Kris Van Hees]

See comments in 16/38

On Thu, Jun 27, 2024 at 01:34:35AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> ---
>  libdtrace/dt_prov_uprobe.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
> index c77063a8..5dbd75e3 100644
> --- a/libdtrace/dt_prov_uprobe.c
> +++ b/libdtrace/dt_prov_uprobe.c
> @@ -129,6 +129,38 @@ static void probe_destroy(dtrace_hdl_t *dtp, void *datap)
>  	free_probe_list(dtp, datap);
>  }
>  
> +/*
> + * Add clause to probe.
> + */
> +static int probe_add_clause(dtrace_hdl_t *dtp, dt_probe_t *prp, dt_ident_t *idp)
> +{
> +	const list_probe_t	*pup;
> +
> +	for (pup = prp->prv_data; pup != NULL; pup = dt_list_next(pup)) {
> +		dt_probe_t		*uprp = pup->probe;
> +		dt_probe_clause_t	*prev, *pcp;
> +
> +		/*
> +		 * Check if the clause is already there.  Since the clauses
> +		 * are added "in order," we only need to check the previous
> +		 * entry.
> +		 */
> +		prev = dt_list_prev(&uprp->clauses);
> +		if (prev && strcmp(prev->clause->di_name, idp->di_name) == 0)
> +			continue;
> +
> +		/*
> +		 * Add the clause.
> +		 */
> +		pcp = dt_zalloc(dtp, sizeof(dt_probe_clause_t));
> +		if (pcp == NULL)
> +			return -1;
> +		pcp->clause = idp;
> +		dt_list_append(&uprp->clauses, pcp);
> +	}
> +
> +	return 0;
> +}
>  
>  /*
>   * Look up or create an underlying (real) probe, corresponding directly to a
> @@ -811,6 +843,7 @@ dt_provimpl_t	dt_pid = {
>  	.prog_type	= BPF_PROG_TYPE_UNSPEC,
>  	.provide_probe	= &provide_pid_probe,
>  	.enable		= &enable_pid,
> +	.probe_add_clause = &probe_add_clause,
>  	.probe_destroy	= &probe_destroy,
>  };
>  
> @@ -822,5 +855,6 @@ dt_provimpl_t	dt_usdt = {
>  	.prog_type	= BPF_PROG_TYPE_UNSPEC,
>  	.provide_probe	= &provide_usdt_probe,
>  	.enable		= &enable_usdt,
> +	.probe_add_clause = &probe_add_clause,
>  	.probe_destroy	= &probe_destroy,
>  };
> -- 
> 2.18.4
> 

Re: [PATCH 13/38] Hide dtrace_actdesc_t until it is needed

[Eugene Loh]

On 7/18/24 16:02, Kris Van Hees wrote:

> On Thu, Jun 27, 2024 at 01:34:30AM -0400, eugene.loh@oracle.com wrote:
>> From: Eugene Loh <eugene.loh@oracle.com>
>>
>> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> I think you should just get rid of it.  Worst case, we can always revert the
> patch.  But this type is only used in code that we do not plan to revive since
> we no longer do DOF the same way as the older version.  Even if we were to
> re-implement the functionality to save compiled probe programs to file, it
> would look very different and certainly not be action-based but rather BPF
> code based.

It would seem to me that the declaration (this patch) and the code that 
uses it should go together.  So, are you saying the inoperative code 
that references this declaration should also be removed?  It would seem 
funny to me to remove the declaration (this patch) while (admittedly 
inoperative) code that references it is left alone.

>
>> ---
>>   include/dtrace/enabling.h | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/include/dtrace/enabling.h b/include/dtrace/enabling.h
>> index f1ec444c..2562c87b 100644
>> --- a/include/dtrace/enabling.h
>> +++ b/include/dtrace/enabling.h
>> @@ -43,6 +43,8 @@ typedef struct dtrace_probedesc {
>>   	const char	*prb;			/* probe name */
>>   } dtrace_probedesc_t;
>>   
>> +#ifdef FIXME
>> +This type is used only in #ifdef FIXME code.
>>   typedef struct dtrace_actdesc {
>>   	struct dtrace_difo *dtad_difo;		/* pointer to DIF object */
>>   	dtrace_actkind_t dtad_kind;		/* kind of action */
>> @@ -50,6 +52,7 @@ typedef struct dtrace_actdesc {
>>   	uint64_t dtad_arg;			/* action argument */
>>   	uint64_t dtad_uarg;			/* user argument */
>>   } dtrace_actdesc_t;
>> +#endif
>>   
>>   typedef struct dtrace_ecbdesc {
>>   	dtrace_probedesc_t dted_probe;		/* probe description */
>> -- 
>> 2.18.4
>>

Re: [PATCH 13/38] Hide dtrace_actdesc_t until it is needed

[Kris Van Hees]

On Thu, Jul 18, 2024 at 05:06:00PM -0400, Eugene Loh wrote:
> On 7/18/24 16:02, Kris Van Hees wrote:
> 
> > On Thu, Jun 27, 2024 at 01:34:30AM -0400, eugene.loh@oracle.com wrote:
> > > From: Eugene Loh <eugene.loh@oracle.com>
> > > 
> > > Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> > I think you should just get rid of it.  Worst case, we can always revert the
> > patch.  But this type is only used in code that we do not plan to revive since
> > we no longer do DOF the same way as the older version.  Even if we were to
> > re-implement the functionality to save compiled probe programs to file, it
> > would look very different and certainly not be action-based but rather BPF
> > code based.
> 
> It would seem to me that the declaration (this patch) and the code that uses
> it should go together.  So, are you saying the inoperative code that
> references this declaration should also be removed?  It would seem funny to
> me to remove the declaration (this patch) while (admittedly inoperative)
> code that references it is left alone.

That is a valid point - in which case I think that (for this patch series),
this is not a relevant patch and thus should be dropped.  Instead, a work item
ought to be added to either get rid of (most of) dt_dof.c and related code, or
to actually implement it in a way that makes sense (i.e. do something useful
and be in line with the new implementation of DTrace).

> > 
> > > ---
> > >   include/dtrace/enabling.h | 3 +++
> > >   1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/include/dtrace/enabling.h b/include/dtrace/enabling.h
> > > index f1ec444c..2562c87b 100644
> > > --- a/include/dtrace/enabling.h
> > > +++ b/include/dtrace/enabling.h
> > > @@ -43,6 +43,8 @@ typedef struct dtrace_probedesc {
> > >   	const char	*prb;			/* probe name */
> > >   } dtrace_probedesc_t;
> > > +#ifdef FIXME
> > > +This type is used only in #ifdef FIXME code.
> > >   typedef struct dtrace_actdesc {
> > >   	struct dtrace_difo *dtad_difo;		/* pointer to DIF object */
> > >   	dtrace_actkind_t dtad_kind;		/* kind of action */
> > > @@ -50,6 +52,7 @@ typedef struct dtrace_actdesc {
> > >   	uint64_t dtad_arg;			/* action argument */
> > >   	uint64_t dtad_uarg;			/* user argument */
> > >   } dtrace_actdesc_t;
> > > +#endif
> > >   typedef struct dtrace_ecbdesc {
> > >   	dtrace_probedesc_t dted_probe;		/* probe description */
> > > -- 
> > > 2.18.4
> > > 

Re: [PATCH 13/38] Hide dtrace_actdesc_t until it is needed

[Eugene Loh]

On 7/18/24 17:28, Kris Van Hees wrote:
> On Thu, Jul 18, 2024 at 05:06:00PM -0400, Eugene Loh wrote:
>> On 7/18/24 16:02, Kris Van Hees wrote:
>>
>>> On Thu, Jun 27, 2024 at 01:34:30AM -0400, eugene.loh@oracle.com wrote:
>>>> From: Eugene Loh <eugene.loh@oracle.com>
>>>>
>>>> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
>>> I think you should just get rid of it.  Worst case, we can always revert the
>>> patch.  But this type is only used in code that we do not plan to revive since
>>> we no longer do DOF the same way as the older version.  Even if we were to
>>> re-implement the functionality to save compiled probe programs to file, it
>>> would look very different and certainly not be action-based but rather BPF
>>> code based.
>> It would seem to me that the declaration (this patch) and the code that uses
>> it should go together.  So, are you saying the inoperative code that
>> references this declaration should also be removed?  It would seem funny to
>> me to remove the declaration (this patch) while (admittedly inoperative)
>> code that references it is left alone.
> That is a valid point - in which case I think that (for this patch series),
> this is not a relevant patch and thus should be dropped.  Instead, a work item
> ought to be added to either get rid of (most of) dt_dof.c and related code, or
> to actually implement it in a way that makes sense (i.e. do something useful
> and be in line with the new implementation of DTrace).

Okay.  Patch withdrawn.  Issue filed. 
https://github.com/oracle/dtrace-utils/issues/73

>>>> ---
>>>>    include/dtrace/enabling.h | 3 +++
>>>>    1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/include/dtrace/enabling.h b/include/dtrace/enabling.h
>>>> index f1ec444c..2562c87b 100644
>>>> --- a/include/dtrace/enabling.h
>>>> +++ b/include/dtrace/enabling.h
>>>> @@ -43,6 +43,8 @@ typedef struct dtrace_probedesc {
>>>>    	const char	*prb;			/* probe name */
>>>>    } dtrace_probedesc_t;
>>>> +#ifdef FIXME
>>>> +This type is used only in #ifdef FIXME code.
>>>>    typedef struct dtrace_actdesc {
>>>>    	struct dtrace_difo *dtad_difo;		/* pointer to DIF object */
>>>>    	dtrace_actkind_t dtad_kind;		/* kind of action */
>>>> @@ -50,6 +52,7 @@ typedef struct dtrace_actdesc {
>>>>    	uint64_t dtad_arg;			/* action argument */
>>>>    	uint64_t dtad_uarg;			/* user argument */
>>>>    } dtrace_actdesc_t;
>>>> +#endif
>>>>    typedef struct dtrace_ecbdesc {
>>>>    	dtrace_probedesc_t dted_probe;		/* probe description */
>>>> -- 
>>>> 2.18.4
>>>>

Re: [PATCH 17/38] Add a provider-specific probe_add_clause handle

[Eugene Loh]

Patch withdrawn.  Its contents will be squashed into a new "v2 16/38" 
with a modified subject line.

On 7/18/24 16:49, Kris Van Hees wrote:
> See comments for 16/38
>
> On Thu, Jun 27, 2024 at 01:34:34AM -0400, eugene.loh@oracle.com wrote:
>> From: Eugene Loh <eugene.loh@oracle.com>
>>
>> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
>> ---
>>   libdtrace/dt_probe.c    | 2 ++
>>   libdtrace/dt_provider.h | 3 +++
>>   2 files changed, 5 insertions(+)
>>
>> diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
>> index d0ae63d1..48b02e11 100644
>> --- a/libdtrace/dt_probe.c
>> +++ b/libdtrace/dt_probe.c
>> @@ -1354,6 +1354,8 @@ dt_probe_add_clause(dtrace_hdl_t *dtp, dt_probe_t *prp, dt_ident_t *idp)
>>   		pcp->clause = idp;
>>   
>>   	dt_list_append(&prp->clauses, pcp);
>> +	if (prp->prov->impl->probe_add_clause)
>> +		return prp->prov->impl->probe_add_clause(dtp, prp, idp);
>>   
>>   	return 0;
>>   }
>> diff --git a/libdtrace/dt_provider.h b/libdtrace/dt_provider.h
>> index 17b1844c..b1b1b1b8 100644
>> --- a/libdtrace/dt_provider.h
>> +++ b/libdtrace/dt_provider.h
>> @@ -62,6 +62,9 @@ typedef struct dt_provimpl {
>>   	int (*probe_info)(dtrace_hdl_t *dtp,	/* get probe info */
>>   			  const struct dt_probe *prp,
>>   			  int *argcp, dt_argdesc_t **argvp);
>> +	int (*probe_add_clause)(dtrace_hdl_t *dtp, /* add clause to probe */
>> +				struct dt_probe *prp,
>> +				dt_ident_t *idp);
>>   	void (*detach)(dtrace_hdl_t *dtp,	/* probe cleanup */
>>   		       const struct dt_probe *prb);
>>   	void (*probe_destroy)(dtrace_hdl_t *dtp, /* free probe data */
>> -- 
>> 2.18.4
>>

Re: [PATCH 18/38] Add a provider-specific probe_add_clause for underlying probes

[Eugene Loh]

Patch withdrawn.  Its contents will be squashed into a new "v2 16/38" 
with a modified subject line.

On 7/18/24 16:50, Kris Van Hees wrote:
> See comments in 16/38
>
> On Thu, Jun 27, 2024 at 01:34:35AM -0400, eugene.loh@oracle.com wrote:
>> From: Eugene Loh <eugene.loh@oracle.com>
>>
>> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
>> ---
>>   libdtrace/dt_prov_uprobe.c | 34 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 34 insertions(+)
>>
>> diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
>> index c77063a8..5dbd75e3 100644
>> --- a/libdtrace/dt_prov_uprobe.c
>> +++ b/libdtrace/dt_prov_uprobe.c
>> @@ -129,6 +129,38 @@ static void probe_destroy(dtrace_hdl_t *dtp, void *datap)
>>   	free_probe_list(dtp, datap);
>>   }
>>   
>> +/*
>> + * Add clause to probe.
>> + */
>> +static int probe_add_clause(dtrace_hdl_t *dtp, dt_probe_t *prp, dt_ident_t *idp)
>> +{
>> +	const list_probe_t	*pup;
>> +
>> +	for (pup = prp->prv_data; pup != NULL; pup = dt_list_next(pup)) {
>> +		dt_probe_t		*uprp = pup->probe;
>> +		dt_probe_clause_t	*prev, *pcp;
>> +
>> +		/*
>> +		 * Check if the clause is already there.  Since the clauses
>> +		 * are added "in order," we only need to check the previous
>> +		 * entry.
>> +		 */
>> +		prev = dt_list_prev(&uprp->clauses);
>> +		if (prev && strcmp(prev->clause->di_name, idp->di_name) == 0)
>> +			continue;
>> +
>> +		/*
>> +		 * Add the clause.
>> +		 */
>> +		pcp = dt_zalloc(dtp, sizeof(dt_probe_clause_t));
>> +		if (pcp == NULL)
>> +			return -1;
>> +		pcp->clause = idp;
>> +		dt_list_append(&uprp->clauses, pcp);
>> +	}
>> +
>> +	return 0;
>> +}
>>   
>>   /*
>>    * Look up or create an underlying (real) probe, corresponding directly to a
>> @@ -811,6 +843,7 @@ dt_provimpl_t	dt_pid = {
>>   	.prog_type	= BPF_PROG_TYPE_UNSPEC,
>>   	.provide_probe	= &provide_pid_probe,
>>   	.enable		= &enable_pid,
>> +	.probe_add_clause = &probe_add_clause,
>>   	.probe_destroy	= &probe_destroy,
>>   };
>>   
>> @@ -822,5 +855,6 @@ dt_provimpl_t	dt_usdt = {
>>   	.prog_type	= BPF_PROG_TYPE_UNSPEC,
>>   	.provide_probe	= &provide_usdt_probe,
>>   	.enable		= &enable_usdt,
>> +	.probe_add_clause = &probe_add_clause,
>>   	.probe_destroy	= &probe_destroy,
>>   };
>> -- 
>> 2.18.4
>>