[PATCH 01/22] test: Handle dtrace:::ERROR arg3 specially

[PATCH 01/22] test: Handle dtrace:::ERROR arg3 specially

[eugene.loh]

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

The ERROR probe's arg3 reports the culprit PC, whose value can vary
with minor implementation changes.  On the one hand, we do not want
tests to be overly sensitive to this value.  On the other hand, we
do at least want to check the value is reasonable.

Therefore:

*)  Change tests that dump ERROR's args to omit arg3.

*)  Add a new test that checks that ERROR's arg3 is reasonable.

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 test/unittest/error/tst.DTRACEFLT_UNKNOWN.d   |  6 +-
 test/unittest/error/tst.DTRACEFLT_UNKNOWN.r   |  2 +-
 .../regression/tst.DTRACEFLT_BADADDR.d_path.d |  8 +-
 .../regression/tst.DTRACEFLT_BADADDR.d_path.r |  2 +-
 test/unittest/variables/bvar/tst.arg3-ERROR.d | 92 +++++++++++++++++++
 5 files changed, 101 insertions(+), 9 deletions(-)
 create mode 100644 test/unittest/variables/bvar/tst.arg3-ERROR.d

diff --git a/test/unittest/error/tst.DTRACEFLT_UNKNOWN.d b/test/unittest/error/tst.DTRACEFLT_UNKNOWN.d
index 001903ff..bfc77bf5 100644
--- a/test/unittest/error/tst.DTRACEFLT_UNKNOWN.d
+++ b/test/unittest/error/tst.DTRACEFLT_UNKNOWN.d
@@ -1,6 +1,6 @@
 /*
  * Oracle Linux DTrace.
- * Copyright (c) 2006, 2020, 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.
  */
@@ -19,8 +19,8 @@
 
 ERROR
 {
-	printf("The arguments are %u %u %u %u %u\n",
-		arg1, arg2, arg3, arg4, arg5);
+	printf("The arguments are %u %u PC %u %u\n",
+		arg1, arg2, arg4, arg5);
 	printf("The value of arg4 = %u\n", DTRACEFLT_UNKNOWN);
 	exit(0);
 }
diff --git a/test/unittest/error/tst.DTRACEFLT_UNKNOWN.r b/test/unittest/error/tst.DTRACEFLT_UNKNOWN.r
index b11f6c99..3e7caac4 100644
--- a/test/unittest/error/tst.DTRACEFLT_UNKNOWN.r
+++ b/test/unittest/error/tst.DTRACEFLT_UNKNOWN.r
@@ -1,4 +1,4 @@
-The arguments are 2 2 4 1 64
+The arguments are 2 2 PC 1 64
 The value of arg4 = 0
 
 -- @@stderr --
diff --git a/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.d b/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.d
index c23f9503..ec519f4f 100644
--- a/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.d
+++ b/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.d
@@ -1,10 +1,10 @@
 /*
  * Oracle Linux DTrace.
- * Copyright (c) 2015, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2015, 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.
  */
-/* @@xfail: dtv2 */
+/* @@xfail: dtv2 d_path */
 
 /*
  * ASSERTION:
@@ -18,8 +18,7 @@
 
 ERROR
 {
-	printf("The arguments are %u %u %u %u %u\n",
-		arg1, arg2, arg3, arg4, arg5);
+	printf("The arguments are %u %u PC %u %u\n", arg1, arg2, arg4, arg5);
 	printf("The value of arg4 should be %u\n", DTRACEFLT_BADADDR);
 	printf("The value of arg5 should be %u\n", 0x18);
 	exit(0);
@@ -29,4 +28,5 @@ BEGIN
 {
 	d = d_path((struct path *)0x18);
 	trace(d);
+	exit(1);
 }
diff --git a/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.r b/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.r
index 8c601a43..be1f6d5b 100644
--- a/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.r
+++ b/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.r
@@ -1,4 +1,4 @@
-The arguments are 2 1 28 1 24
+The arguments are 2 1 PC 1 24
 The value of arg4 should be 1
 The value of arg5 should be 24
 
diff --git a/test/unittest/variables/bvar/tst.arg3-ERROR.d b/test/unittest/variables/bvar/tst.arg3-ERROR.d
new file mode 100644
index 00000000..6c2f5206
--- /dev/null
+++ b/test/unittest/variables/bvar/tst.arg3-ERROR.d
@@ -0,0 +1,92 @@
+/*
+ * 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.
+ */
+
+/*
+ * ASSERTION: The 'arg3' variable can be accessed in the ERROR probe
+ *	and its value (PC) is reasonable.
+ *
+ * SECTION: Variables/Built-in Variables/arg3
+ */
+
+#pragma D option quiet
+
+BEGIN
+{
+	nerrs = 0;
+}
+
+BEGIN
+{
+	x = (int *)64;
+	y = *x;                       /* Error at PC[0] */
+	trace(y);
+}
+
+BEGIN
+{
+	x = (int *)64;
+	y = *x;                       /* Error at PC[1] */
+	trace(y);
+}
+
+BEGIN
+{
+	x = (int *)64;
+	y = *x;                       /* Error at PC[2] */
+	trace(y);
+}
+
+BEGIN
+{
+	x = (int *)64;
+	y = *x;                       /* Error at PC[3] */
+	trace(y);
+}
+
+ERROR
+{
+	/* Record the problematic PC and continue execution. */
+	PC[nerrs++] = arg3;
+}
+
+/*
+ * The problematic PCs are likely to satisfy the following
+ * reasonable checks, though it's possible that some radically
+ * different implementation in the future might violate one or
+ * more checks.
+ *
+ * The first problematic PC is expected in the some range.
+ *
+ * The next problematic PC is expected to be PC[0] plus some
+ * delta, including some special functions that are loaded.
+ *
+ * Then, the next PC is expected to be PC[1] plus some delta
+ * that is smaller and narrower since those special functions
+ * do not need to be reloaded.
+ *
+ * The last PC is expected to be PC[2] plus some predictable,
+ * small and narrow, delta PC[2]-PC[1].
+ */
+BEGIN
+/PC[0] >   100 &&
+ PC[0] <  2000 &&
+ PC[1] >  PC[0] +  100 &&
+ PC[1] <  PC[0] + 2000 &&
+ PC[2] >  PC[1] +   30 &&
+ PC[2] <  PC[1] +  500 &&
+ PC[3] == PC[2] + (PC[2] - PC[1])/
+{
+	printf("PCs: %d %d %d %d\n", PC[0], PC[1], PC[2], PC[3]);
+	exit(0);
+}
+
+BEGIN
+{
+	printf("ERROR!  PCs do not seem reasonable: %d %d %d %d\n",
+		   PC[0], PC[1], PC[2], PC[3]);
+	exit(1);
+}
-- 
2.43.5

[PATCH 02/22] test: Clean up tests still expecting obsolete "at DIF offset NN"

[eugene.loh]

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

Also, the numbering of EPIDs has changed.

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>
---
 test/demo/dtrace/error.d                                | 1 -
 test/demo/dtrace/error.r                                | 2 +-
 test/unittest/assocs/tst.invalidref.r                   | 4 ++--
 test/unittest/drops/drp.DTRACEDROP_DBLERROR.r           | 2 +-
 test/unittest/error/tst.DTRACEFLT_UNKNOWN.d             | 1 -
 test/unittest/error/tst.DTRACEFLT_UNKNOWN.r             | 4 ++--
 test/unittest/error/tst.DTRACEFLT_UNKNOWN.sparc64.r     | 5 -----
 test/unittest/pointers/err.BadAlign.d                   | 1 -
 test/unittest/pointers/err.BadAlign.r                   | 2 +-
 test/unittest/pointers/err.InvalidAddress2.d            | 1 -
 test/unittest/pointers/err.InvalidAddress2.r            | 2 +-
 test/unittest/pointers/err.InvalidAddress3.r            | 2 +-
 test/unittest/pointers/err.InvalidAddress4.d            | 1 -
 test/unittest/pointers/err.InvalidAddress4.r            | 2 +-
 test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.r | 4 ++--
 15 files changed, 12 insertions(+), 22 deletions(-)
 delete mode 100644 test/unittest/error/tst.DTRACEFLT_UNKNOWN.sparc64.r

diff --git a/test/demo/dtrace/error.d b/test/demo/dtrace/error.d
index 5700dd33..d55fb090 100644
--- a/test/demo/dtrace/error.d
+++ b/test/demo/dtrace/error.d
@@ -5,7 +5,6 @@
  * http://oss.oracle.com/licenses/upl.
  */
 
-/* @@xfail: dtv2 */
 /* @@trigger: none */
 
 BEGIN
diff --git a/test/demo/dtrace/error.r b/test/demo/dtrace/error.r
index d894776b..d3904f47 100644
--- a/test/demo/dtrace/error.r
+++ b/test/demo/dtrace/error.r
@@ -3,4 +3,4 @@
 
 -- @@stderr --
 dtrace: script 'test/demo/dtrace/error.d' matched 2 probes
-dtrace: error on enabled probe ID 1 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #1 at DIF offset 16 at BPF pc NNN
+dtrace: error on enabled probe ID 3 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #1 at BPF pc NNN
diff --git a/test/unittest/assocs/tst.invalidref.r b/test/unittest/assocs/tst.invalidref.r
index b050e436..20f131b4 100644
--- a/test/unittest/assocs/tst.invalidref.r
+++ b/test/unittest/assocs/tst.invalidref.r
@@ -1,4 +1,4 @@
 
 -- @@stderr --
-dtrace: error on enabled probe ID (ID: profile:::tick-1s): invalid address ({ptr}) in action #2 at DIF offset 64 at BPF pc NNN
-dtrace: error on enabled probe ID (ID: profile:::tick-1s): invalid address ({ptr}) in action #2 at DIF offset 64 at BPF pc NNN
+dtrace: error on enabled probe ID (ID: profile:::tick-1s): invalid address ({ptr}) in action #2 at BPF pc NNN
+dtrace: error on enabled probe ID (ID: profile:::tick-1s): invalid address ({ptr}) in action #2 at BPF pc NNN
diff --git a/test/unittest/drops/drp.DTRACEDROP_DBLERROR.r b/test/unittest/drops/drp.DTRACEDROP_DBLERROR.r
index 14654676..9fa54dd9 100644
--- a/test/unittest/drops/drp.DTRACEDROP_DBLERROR.r
+++ b/test/unittest/drops/drp.DTRACEDROP_DBLERROR.r
@@ -4,4 +4,4 @@
 -- @@stderr --
 dtrace: script 'test/unittest/drops/drp.DTRACEDROP_DBLERROR.d' matched 3 probes
 dtrace: [DTRACEDROP_DBLERROR] 1 error in ERROR probe enabling
-dtrace: error on enabled probe ID 1 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #1 at DIF offset 16 at BPF pc NNN
+dtrace: error on enabled probe ID 3 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #1 at BPF pc NNN
diff --git a/test/unittest/error/tst.DTRACEFLT_UNKNOWN.d b/test/unittest/error/tst.DTRACEFLT_UNKNOWN.d
index bfc77bf5..c74762ae 100644
--- a/test/unittest/error/tst.DTRACEFLT_UNKNOWN.d
+++ b/test/unittest/error/tst.DTRACEFLT_UNKNOWN.d
@@ -4,7 +4,6 @@
  * Licensed under the Universal Permissive License v 1.0 as shown at
  * http://oss.oracle.com/licenses/upl.
  */
-/* @@xfail: dtv2 */
 
 /*
  * ASSERTION:
diff --git a/test/unittest/error/tst.DTRACEFLT_UNKNOWN.r b/test/unittest/error/tst.DTRACEFLT_UNKNOWN.r
index 3e7caac4..1e4fdd64 100644
--- a/test/unittest/error/tst.DTRACEFLT_UNKNOWN.r
+++ b/test/unittest/error/tst.DTRACEFLT_UNKNOWN.r
@@ -1,5 +1,5 @@
-The arguments are 2 2 PC 1 64
+The arguments are 3 1 PC 1 64
 The value of arg4 = 0
 
 -- @@stderr --
-dtrace: error on enabled probe ID 2 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #2 at DIF offset 4 at BPF pc NNN
+dtrace: error on enabled probe ID 3 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #1 at BPF pc NNN
diff --git a/test/unittest/error/tst.DTRACEFLT_UNKNOWN.sparc64.r b/test/unittest/error/tst.DTRACEFLT_UNKNOWN.sparc64.r
deleted file mode 100644
index 3944c138..00000000
--- a/test/unittest/error/tst.DTRACEFLT_UNKNOWN.sparc64.r
+++ /dev/null
@@ -1,5 +0,0 @@
-The arguments are 2 2 4 1 0
-The value of arg4 = 0
-
--- @@stderr --
-dtrace: error on enabled probe ID 2 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #2 at DIF offset 4
diff --git a/test/unittest/pointers/err.BadAlign.d b/test/unittest/pointers/err.BadAlign.d
index cd4138ae..e859dd75 100644
--- a/test/unittest/pointers/err.BadAlign.d
+++ b/test/unittest/pointers/err.BadAlign.d
@@ -4,7 +4,6 @@
  * Licensed under the Universal Permissive License v 1.0 as shown at
  * http://oss.oracle.com/licenses/upl.
  */
-/* @@xfail: dtv2 */
 
 /*
  * ASSERTION: This test reproduces the alignment error.
diff --git a/test/unittest/pointers/err.BadAlign.r b/test/unittest/pointers/err.BadAlign.r
index 4328aac4..187543b6 100644
--- a/test/unittest/pointers/err.BadAlign.r
+++ b/test/unittest/pointers/err.BadAlign.r
@@ -1,3 +1,3 @@
 
 -- @@stderr --
-dtrace: error on enabled probe ID 1 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #2 at DIF offset 4
+dtrace: error on enabled probe ID 3 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #1 at BPF pc NNN
diff --git a/test/unittest/pointers/err.InvalidAddress2.d b/test/unittest/pointers/err.InvalidAddress2.d
index 682ad650..b22f08fb 100644
--- a/test/unittest/pointers/err.InvalidAddress2.d
+++ b/test/unittest/pointers/err.InvalidAddress2.d
@@ -4,7 +4,6 @@
  * Licensed under the Universal Permissive License v 1.0 as shown at
  * http://oss.oracle.com/licenses/upl.
  */
-/* @@xfail: dtv2 */
 
 /*
  * ASSERTION: D pointers do not allow invalid pointer accesses.
diff --git a/test/unittest/pointers/err.InvalidAddress2.r b/test/unittest/pointers/err.InvalidAddress2.r
index d866eae1..187543b6 100644
--- a/test/unittest/pointers/err.InvalidAddress2.r
+++ b/test/unittest/pointers/err.InvalidAddress2.r
@@ -1,3 +1,3 @@
 
 -- @@stderr --
-dtrace: error on enabled probe ID 1 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #4 at DIF offset 8
+dtrace: error on enabled probe ID 3 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #1 at BPF pc NNN
diff --git a/test/unittest/pointers/err.InvalidAddress3.r b/test/unittest/pointers/err.InvalidAddress3.r
index 069ee1de..187543b6 100644
--- a/test/unittest/pointers/err.InvalidAddress3.r
+++ b/test/unittest/pointers/err.InvalidAddress3.r
@@ -1,3 +1,3 @@
 
 -- @@stderr --
-dtrace: error on enabled probe ID 1 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #2 at DIF offset 8
+dtrace: error on enabled probe ID 3 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #1 at BPF pc NNN
diff --git a/test/unittest/pointers/err.InvalidAddress4.d b/test/unittest/pointers/err.InvalidAddress4.d
index 1e2b4f62..586cddf9 100644
--- a/test/unittest/pointers/err.InvalidAddress4.d
+++ b/test/unittest/pointers/err.InvalidAddress4.d
@@ -4,7 +4,6 @@
  * Licensed under the Universal Permissive License v 1.0 as shown at
  * http://oss.oracle.com/licenses/upl.
  */
-/* @@xfail: dtv2 */
 
 /*
  * ASSERTION: Demonstrating valid memory access.
diff --git a/test/unittest/pointers/err.InvalidAddress4.r b/test/unittest/pointers/err.InvalidAddress4.r
index d866eae1..187543b6 100644
--- a/test/unittest/pointers/err.InvalidAddress4.r
+++ b/test/unittest/pointers/err.InvalidAddress4.r
@@ -1,3 +1,3 @@
 
 -- @@stderr --
-dtrace: error on enabled probe ID 1 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #4 at DIF offset 8
+dtrace: error on enabled probe ID 3 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #1 at BPF pc NNN
diff --git a/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.r b/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.r
index be1f6d5b..b5435eef 100644
--- a/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.r
+++ b/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.r
@@ -1,6 +1,6 @@
-The arguments are 2 1 PC 1 24
+The arguments are 3 1 PC 1 24
 The value of arg4 should be 1
 The value of arg5 should be 24
 
 -- @@stderr --
-dtrace: error on enabled probe ID 2 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #1 at DIF offset 28 at BPF pc NNN
+dtrace: error on enabled probe ID 3 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #1 at BPF pc NNN
-- 
2.43.5

[PATCH 03/22] Action clear() should clear only one aggregation

[eugene.loh]

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

In dt_clear(), we:

*)  extract an aggregation ID (dtrace_aggid_t)

*)  increment the gen counter for this aggregation ID

*)  clear the consumer copy of the aggregation, using:

      /* Also clear our own copy of the data, in case it gets printed. */
      dtrace_aggregate_walk(dtp, dt_aggregate_clear_one, dtp);

However, this clears all the aggregations.  While the next snapshot of
the aggregations will refresh our copies of the aggregations -- after all,
dtrace_aggregate_snap() itself starts with a call to dtrace_aggregate_clear()
-- if we print aggregations before the next snapshot, we would display an
erroneously cleared aggregation.

Therefore, change dt_clear() to clear only those aggregations that
correspond to the specified aggregation ID.

In order that only a specific aggregation ID can be cleared, we
modify the last argument to dt_aggregate_clear_one() to be not
simply the dtp, but a pointer to a struct that has both the dtp
and the aggregation ID.  The previous "clear all" behavior can be
specified with an aggregation ID of DTRACE_AGGVARIDNONE.

Though trunc() appears not to be similarly afflicted, add a test for
it too.

Orabug: 36900180
Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 libdtrace/dt_aggregate.c           | 16 ++++++++++++---
 libdtrace/dt_aggregate.h           |  5 +++++
 libdtrace/dt_consume.c             |  4 +++-
 test/unittest/aggs/tst.clear-one.d | 33 ++++++++++++++++++++++++++++++
 test/unittest/aggs/tst.clear-one.r | 13 ++++++++++++
 test/unittest/aggs/tst.trunc-one.d | 33 ++++++++++++++++++++++++++++++
 test/unittest/aggs/tst.trunc-one.r | 10 +++++++++
 7 files changed, 110 insertions(+), 4 deletions(-)
 create mode 100644 test/unittest/aggs/tst.clear-one.d
 create mode 100644 test/unittest/aggs/tst.clear-one.r
 create mode 100644 test/unittest/aggs/tst.trunc-one.d
 create mode 100644 test/unittest/aggs/tst.trunc-one.r

diff --git a/libdtrace/dt_aggregate.c b/libdtrace/dt_aggregate.c
index 329b8ffa..8825739c 100644
--- a/libdtrace/dt_aggregate.c
+++ b/libdtrace/dt_aggregate.c
@@ -504,7 +504,9 @@ dt_aggregate_clear_one_percpu(const dtrace_aggdata_t *agd,
 int
 dt_aggregate_clear_one(const dtrace_aggdata_t *agd, void *arg)
 {
-	dtrace_hdl_t		*dtp = arg;
+	dt_clear_arg_t		*dca = arg;
+	dtrace_hdl_t		*dtp = dca->dtp;
+	dtrace_aggid_t		aid = dca->aid;
 	dtrace_aggdesc_t	*agg = agd->dtada_desc;
 	dtrace_recdesc_t	*rec = &agg->dtagd_drecs[DT_AGGDATA_RECORD];
 	int64_t			*vals = (int64_t *)
@@ -512,6 +514,9 @@ dt_aggregate_clear_one(const dtrace_aggdata_t *agd, void *arg)
 	uint64_t		agen;
 	int			max_cpus = dtp->dt_conf.max_cpuid + 1;
 
+	if (aid != DTRACE_AGGVARIDNONE && aid != agg->dtagd_varid)
+		return DTRACE_AGGWALK_NEXT;
+
 	/*
 	 * We can pass the entire key because we know that the first uint32_t
 	 * in the key is the aggregation ID we need.
@@ -609,7 +614,9 @@ dt_aggregate_snap_one(dtrace_hdl_t *dtp, int aggid, int cpu, const char *key,
 		 */
 		hgen = *(int64_t *)agd->dtada_data;
 		if (dgen > hgen) {
-			dt_aggregate_clear_one(agd, dtp);
+			dt_clear_arg_t arg = { dtp, DTRACE_AGGVARIDNONE };
+
+			dt_aggregate_clear_one(agd, &arg);
 			hgen = *(int64_t *)agd->dtada_data;
 		}
 
@@ -1859,10 +1866,13 @@ dtrace_aggregate_print(dtrace_hdl_t *dtp, FILE *fp,
 	return 0;
 }
 
+/* FIXME: dtrace_aggregate_clear() is called from only one site.  Should we inline it? */
 void
 dtrace_aggregate_clear(dtrace_hdl_t *dtp)
 {
-	dtrace_aggregate_walk(dtp, dt_aggregate_clear_one, dtp);
+	dt_clear_arg_t arg = { dtp, DTRACE_AGGVARIDNONE };
+
+	dtrace_aggregate_walk(dtp, dt_aggregate_clear_one, &arg);
 }
 
 void
diff --git a/libdtrace/dt_aggregate.h b/libdtrace/dt_aggregate.h
index 4f8d09bf..0dc126e9 100644
--- a/libdtrace/dt_aggregate.h
+++ b/libdtrace/dt_aggregate.h
@@ -12,6 +12,11 @@
 extern "C" {
 #endif
 
+typedef struct dt_clear_arg {
+	dtrace_hdl_t	*dtp;
+	dtrace_aggid_t	aid;
+} dt_clear_arg_t;
+
 typedef struct dt_aggregate dt_aggregate_t;
 
 #define DT_AGGDATA_COUNTER	0
diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
index 0e30ddbd..f2d5cb74 100644
--- a/libdtrace/dt_consume.c
+++ b/libdtrace/dt_consume.c
@@ -1552,6 +1552,7 @@ dt_clear(dtrace_hdl_t *dtp, caddr_t base, dtrace_recdesc_t *rec)
 	dtrace_aggid_t	aid;
 	uint64_t	gen;
 	caddr_t		addr;
+	dt_clear_arg_t	arg = { dtp, 0 };
 
 	/* We have just one record: the aggregation ID. */
 	addr = base + rec->dtrd_offset;
@@ -1568,7 +1569,8 @@ dt_clear(dtrace_hdl_t *dtp, caddr_t base, dtrace_recdesc_t *rec)
 		return -1;
 
 	/* Also clear our own copy of the data, in case it gets printed. */
-	dtrace_aggregate_walk(dtp, dt_aggregate_clear_one, dtp);
+	arg.aid = aid;
+	dtrace_aggregate_walk(dtp, dt_aggregate_clear_one, &arg);
 
 	return 0;
 }
diff --git a/test/unittest/aggs/tst.clear-one.d b/test/unittest/aggs/tst.clear-one.d
new file mode 100644
index 00000000..f96c2db0
--- /dev/null
+++ b/test/unittest/aggs/tst.clear-one.d
@@ -0,0 +1,33 @@
+/*
+ * 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.
+ */
+
+/*
+ * ASSERTION: Clearing one aggregation clears only that one.
+ *
+ * SECTION: Aggregations/Clearing aggregations
+ */
+/* @@nosort */
+
+#pragma D option quiet
+
+BEGIN
+{
+	@a[1] = sum(100);
+	@a[2] = sum( 20);
+	@a[3] = sum(  3);
+	@b[4] = sum(400);
+	@b[5] = sum( 50);
+	@b[6] = sum(  6);
+	@c[7] = sum(700);
+	@c[8] = sum( 80);
+	@c[9] = sum(  9);
+	clear(@b);
+	printa(@a);
+	printa(@b);
+	printa(@c);
+	exit(0);
+}
diff --git a/test/unittest/aggs/tst.clear-one.r b/test/unittest/aggs/tst.clear-one.r
new file mode 100644
index 00000000..101d173f
--- /dev/null
+++ b/test/unittest/aggs/tst.clear-one.r
@@ -0,0 +1,13 @@
+
+        3                3
+        2               20
+        1              100
+
+        4                0
+        5                0
+        6                0
+
+        9                9
+        8               80
+        7              700
+
diff --git a/test/unittest/aggs/tst.trunc-one.d b/test/unittest/aggs/tst.trunc-one.d
new file mode 100644
index 00000000..0556bb34
--- /dev/null
+++ b/test/unittest/aggs/tst.trunc-one.d
@@ -0,0 +1,33 @@
+/*
+ * 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.
+ */
+
+/*
+ * ASSERTION: Truncating one aggregation truncates only that one.
+ *
+ * SECTION: Aggregations/Clearing aggregations
+ */
+/* @@nosort */
+
+#pragma D option quiet
+
+BEGIN
+{
+	@a[1] = sum(100);
+	@a[2] = sum( 20);
+	@a[3] = sum(  3);
+	@b[4] = sum(400);
+	@b[5] = sum( 50);
+	@b[6] = sum(  6);
+	@c[7] = sum(700);
+	@c[8] = sum( 80);
+	@c[9] = sum(  9);
+	trunc(@b);
+	printa(@a);
+	printa(@b);
+	printa(@c);
+	exit(0);
+}
diff --git a/test/unittest/aggs/tst.trunc-one.r b/test/unittest/aggs/tst.trunc-one.r
new file mode 100644
index 00000000..a611026d
--- /dev/null
+++ b/test/unittest/aggs/tst.trunc-one.r
@@ -0,0 +1,10 @@
+
+        3                3
+        2               20
+        1              100
+
+
+        9                9
+        8               80
+        7              700
+
-- 
2.43.5

Re: [PATCH 03/22] Action clear() should clear only one aggregation

[Kris Van Hees]

On Thu, Aug 29, 2024 at 01:22:00AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> In dt_clear(), we:
> 
> *)  extract an aggregation ID (dtrace_aggid_t)
> 
> *)  increment the gen counter for this aggregation ID
> 
> *)  clear the consumer copy of the aggregation, using:
> 
>       /* Also clear our own copy of the data, in case it gets printed. */
>       dtrace_aggregate_walk(dtp, dt_aggregate_clear_one, dtp);
> 
> However, this clears all the aggregations.  While the next snapshot of
> the aggregations will refresh our copies of the aggregations -- after all,
> dtrace_aggregate_snap() itself starts with a call to dtrace_aggregate_clear()
> -- if we print aggregations before the next snapshot, we would display an
> erroneously cleared aggregation.
> 
> Therefore, change dt_clear() to clear only those aggregations that
> correspond to the specified aggregation ID.
> 
> In order that only a specific aggregation ID can be cleared, we
> modify the last argument to dt_aggregate_clear_one() to be not
> simply the dtp, but a pointer to a struct that has both the dtp
> and the aggregation ID.  The previous "clear all" behavior can be
> specified with an aggregation ID of DTRACE_AGGVARIDNONE.
> 
> Though trunc() appears not to be similarly afflicted, add a test for
> it too.
> 
> Orabug: 36900180
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

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

... though with minor comments/changes as listed below.

> ---
>  libdtrace/dt_aggregate.c           | 16 ++++++++++++---
>  libdtrace/dt_aggregate.h           |  5 +++++
>  libdtrace/dt_consume.c             |  4 +++-
>  test/unittest/aggs/tst.clear-one.d | 33 ++++++++++++++++++++++++++++++
>  test/unittest/aggs/tst.clear-one.r | 13 ++++++++++++
>  test/unittest/aggs/tst.trunc-one.d | 33 ++++++++++++++++++++++++++++++
>  test/unittest/aggs/tst.trunc-one.r | 10 +++++++++
>  7 files changed, 110 insertions(+), 4 deletions(-)
>  create mode 100644 test/unittest/aggs/tst.clear-one.d
>  create mode 100644 test/unittest/aggs/tst.clear-one.r
>  create mode 100644 test/unittest/aggs/tst.trunc-one.d
>  create mode 100644 test/unittest/aggs/tst.trunc-one.r
> 
> diff --git a/libdtrace/dt_aggregate.c b/libdtrace/dt_aggregate.c
> index 329b8ffa..8825739c 100644
> --- a/libdtrace/dt_aggregate.c
> +++ b/libdtrace/dt_aggregate.c
> @@ -504,7 +504,9 @@ dt_aggregate_clear_one_percpu(const dtrace_aggdata_t *agd,
>  int
>  dt_aggregate_clear_one(const dtrace_aggdata_t *agd, void *arg)
>  {
> -	dtrace_hdl_t		*dtp = arg;
> +	dt_clear_arg_t		*dca = arg;
> +	dtrace_hdl_t		*dtp = dca->dtp;
> +	dtrace_aggid_t		aid = dca->aid;
>  	dtrace_aggdesc_t	*agg = agd->dtada_desc;
>  	dtrace_recdesc_t	*rec = &agg->dtagd_drecs[DT_AGGDATA_RECORD];
>  	int64_t			*vals = (int64_t *)
> @@ -512,6 +514,9 @@ dt_aggregate_clear_one(const dtrace_aggdata_t *agd, void *arg)
>  	uint64_t		agen;
>  	int			max_cpus = dtp->dt_conf.max_cpuid + 1;
>  
> +	if (aid != DTRACE_AGGVARIDNONE && aid != agg->dtagd_varid)
> +		return DTRACE_AGGWALK_NEXT;
> +
>  	/*
>  	 * We can pass the entire key because we know that the first uint32_t
>  	 * in the key is the aggregation ID we need.
> @@ -609,7 +614,9 @@ dt_aggregate_snap_one(dtrace_hdl_t *dtp, int aggid, int cpu, const char *key,
>  		 */
>  		hgen = *(int64_t *)agd->dtada_data;
>  		if (dgen > hgen) {
> -			dt_aggregate_clear_one(agd, dtp);
> +			dt_clear_arg_t arg = { dtp, DTRACE_AGGVARIDNONE };
> +
> +			dt_aggregate_clear_one(agd, &arg);
>  			hgen = *(int64_t *)agd->dtada_data;
>  		}
>  
> @@ -1859,10 +1866,13 @@ dtrace_aggregate_print(dtrace_hdl_t *dtp, FILE *fp,
>  	return 0;
>  }
>  
> +/* FIXME: dtrace_aggregate_clear() is called from only one site.  Should we inline it? */

Removing the FIXME.  Yes, it needs to remain as-is because it is part of the
libdtrace API.

>  void
>  dtrace_aggregate_clear(dtrace_hdl_t *dtp)
>  {
> -	dtrace_aggregate_walk(dtp, dt_aggregate_clear_one, dtp);
> +	dt_clear_arg_t arg = { dtp, DTRACE_AGGVARIDNONE };
> +
> +	dtrace_aggregate_walk(dtp, dt_aggregate_clear_one, &arg);
>  }
>  
>  void
> diff --git a/libdtrace/dt_aggregate.h b/libdtrace/dt_aggregate.h
> index 4f8d09bf..0dc126e9 100644
> --- a/libdtrace/dt_aggregate.h
> +++ b/libdtrace/dt_aggregate.h
> @@ -12,6 +12,11 @@
>  extern "C" {
>  #endif
>  
> +typedef struct dt_clear_arg {
> +	dtrace_hdl_t	*dtp;
> +	dtrace_aggid_t	aid;
> +} dt_clear_arg_t;
> +
>  typedef struct dt_aggregate dt_aggregate_t;
>  
>  #define DT_AGGDATA_COUNTER	0
> diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
> index 0e30ddbd..f2d5cb74 100644
> --- a/libdtrace/dt_consume.c
> +++ b/libdtrace/dt_consume.c
> @@ -1552,6 +1552,7 @@ dt_clear(dtrace_hdl_t *dtp, caddr_t base, dtrace_recdesc_t *rec)
>  	dtrace_aggid_t	aid;
>  	uint64_t	gen;
>  	caddr_t		addr;
> +	dt_clear_arg_t	arg = { dtp, 0 };

0 -> DTRACE_AGGVARIDNONE

>  
>  	/* We have just one record: the aggregation ID. */
>  	addr = base + rec->dtrd_offset;
> @@ -1568,7 +1569,8 @@ dt_clear(dtrace_hdl_t *dtp, caddr_t base, dtrace_recdesc_t *rec)
>  		return -1;
>  
>  	/* Also clear our own copy of the data, in case it gets printed. */
> -	dtrace_aggregate_walk(dtp, dt_aggregate_clear_one, dtp);
> +	arg.aid = aid;
> +	dtrace_aggregate_walk(dtp, dt_aggregate_clear_one, &arg);
>  
>  	return 0;
>  }
> diff --git a/test/unittest/aggs/tst.clear-one.d b/test/unittest/aggs/tst.clear-one.d
> new file mode 100644
> index 00000000..f96c2db0
> --- /dev/null
> +++ b/test/unittest/aggs/tst.clear-one.d
> @@ -0,0 +1,33 @@
> +/*
> + * 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.
> + */
> +
> +/*
> + * ASSERTION: Clearing one aggregation clears only that one.
> + *
> + * SECTION: Aggregations/Clearing aggregations
> + */
> +/* @@nosort */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> +	@a[1] = sum(100);
> +	@a[2] = sum( 20);
> +	@a[3] = sum(  3);
> +	@b[4] = sum(400);
> +	@b[5] = sum( 50);
> +	@b[6] = sum(  6);
> +	@c[7] = sum(700);
> +	@c[8] = sum( 80);
> +	@c[9] = sum(  9);
> +	clear(@b);
> +	printa(@a);
> +	printa(@b);
> +	printa(@c);
> +	exit(0);
> +}
> diff --git a/test/unittest/aggs/tst.clear-one.r b/test/unittest/aggs/tst.clear-one.r
> new file mode 100644
> index 00000000..101d173f
> --- /dev/null
> +++ b/test/unittest/aggs/tst.clear-one.r
> @@ -0,0 +1,13 @@
> +
> +        3                3
> +        2               20
> +        1              100
> +
> +        4                0
> +        5                0
> +        6                0
> +
> +        9                9
> +        8               80
> +        7              700
> +
> diff --git a/test/unittest/aggs/tst.trunc-one.d b/test/unittest/aggs/tst.trunc-one.d
> new file mode 100644
> index 00000000..0556bb34
> --- /dev/null
> +++ b/test/unittest/aggs/tst.trunc-one.d
> @@ -0,0 +1,33 @@
> +/*
> + * 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.
> + */
> +
> +/*
> + * ASSERTION: Truncating one aggregation truncates only that one.
> + *
> + * SECTION: Aggregations/Clearing aggregations
> + */
> +/* @@nosort */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> +	@a[1] = sum(100);
> +	@a[2] = sum( 20);
> +	@a[3] = sum(  3);
> +	@b[4] = sum(400);
> +	@b[5] = sum( 50);
> +	@b[6] = sum(  6);
> +	@c[7] = sum(700);
> +	@c[8] = sum( 80);
> +	@c[9] = sum(  9);
> +	trunc(@b);
> +	printa(@a);
> +	printa(@b);
> +	printa(@c);
> +	exit(0);
> +}
> diff --git a/test/unittest/aggs/tst.trunc-one.r b/test/unittest/aggs/tst.trunc-one.r
> new file mode 100644
> index 00000000..a611026d
> --- /dev/null
> +++ b/test/unittest/aggs/tst.trunc-one.r
> @@ -0,0 +1,10 @@
> +
> +        3                3
> +        2               20
> +        1              100
> +
> +
> +        9                9
> +        8               80
> +        7              700
> +
> -- 
> 2.43.5
> 

[PATCH 04/22] Remove unused "next" arg from dt_flowindent()

[eugene.loh]

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

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

diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
index f2d5cb74..6aa9812e 100644
--- a/libdtrace/dt_consume.c
+++ b/libdtrace/dt_consume.c
@@ -433,11 +433,9 @@ static dt_htab_ops_t dt_spec_buf_htab_ops = {
 };
 
 static int
-dt_flowindent(dtrace_hdl_t *dtp, dtrace_probedata_t *data, dtrace_epid_t last,
-	      dtrace_epid_t next)
+dt_flowindent(dtrace_hdl_t *dtp, dtrace_probedata_t *data, dtrace_epid_t last)
 {
-	dtrace_probedesc_t	*pd = data->dtpda_pdesc, *npd;
-	dtrace_datadesc_t	*ndd;
+	dtrace_probedesc_t	*pd = data->dtpda_pdesc;
 	dtrace_flowkind_t	flow = DTRACEFLOW_NONE;
 	const char		*p = pd->prv;
 	const char		*n = pd->prb;
@@ -448,7 +446,6 @@ dt_flowindent(dtrace_hdl_t *dtp, dtrace_probedata_t *data, dtrace_epid_t last,
 	static const char	*ent = "entry", *ret = "return";
 	static int		entlen = 0, retlen = 0;
 	dtrace_epid_t		id = data->dtpda_epid;
-	int			rval;
 
 	if (entlen == 0) {
 		assert(retlen == 0);
@@ -486,21 +483,6 @@ dt_flowindent(dtrace_hdl_t *dtp, dtrace_probedata_t *data, dtrace_epid_t last,
 			flow = DTRACEFLOW_NONE;
 	}
 
-	/*
-	 * If we're going to unindent this, it's more difficult to see if
-	 * we don't actually want to unindent it -- we need to look at the
-	 * _next_ EPID.
-	 */
-	if (flow == DTRACEFLOW_RETURN && next != DTRACE_EPIDNONE &&
-	    next != id) {
-		rval = dt_epid_lookup(dtp, next, &ndd, &npd);
-		if (rval != 0)
-			return rval;
-
-		if (npd->id == pd->id)
-			flow = DTRACEFLOW_NONE;
-	}
-
 	if (flow == DTRACEFLOW_ENTRY || flow == DTRACEFLOW_RETURN)
 		data->dtpda_prefix = str;
 	else
@@ -2327,7 +2309,7 @@ dt_consume_one_probe(dtrace_hdl_t *dtp, FILE *fp, char *data, uint32_t size,
 
 	if (data_recording) {
 		if (flow)
-			dt_flowindent(dtp, pdat, *last, DTRACE_EPIDNONE);
+			dt_flowindent(dtp, pdat, *last);
 
 		rval = (*efunc)(pdat, arg);
 
-- 
2.43.5

Re: [PATCH 04/22] Remove unused "next" arg from dt_flowindent()

[Kris Van Hees]

On Thu, Aug 29, 2024 at 01:22:01AM -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 | 24 +++---------------------
>  1 file changed, 3 insertions(+), 21 deletions(-)
> 
> diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
> index f2d5cb74..6aa9812e 100644
> --- a/libdtrace/dt_consume.c
> +++ b/libdtrace/dt_consume.c
> @@ -433,11 +433,9 @@ static dt_htab_ops_t dt_spec_buf_htab_ops = {
>  };
>  
>  static int
> -dt_flowindent(dtrace_hdl_t *dtp, dtrace_probedata_t *data, dtrace_epid_t last,
> -	      dtrace_epid_t next)
> +dt_flowindent(dtrace_hdl_t *dtp, dtrace_probedata_t *data, dtrace_epid_t last)
>  {
> -	dtrace_probedesc_t	*pd = data->dtpda_pdesc, *npd;
> -	dtrace_datadesc_t	*ndd;
> +	dtrace_probedesc_t	*pd = data->dtpda_pdesc;
>  	dtrace_flowkind_t	flow = DTRACEFLOW_NONE;
>  	const char		*p = pd->prv;
>  	const char		*n = pd->prb;
> @@ -448,7 +446,6 @@ dt_flowindent(dtrace_hdl_t *dtp, dtrace_probedata_t *data, dtrace_epid_t last,
>  	static const char	*ent = "entry", *ret = "return";
>  	static int		entlen = 0, retlen = 0;
>  	dtrace_epid_t		id = data->dtpda_epid;
> -	int			rval;
>  
>  	if (entlen == 0) {
>  		assert(retlen == 0);
> @@ -486,21 +483,6 @@ dt_flowindent(dtrace_hdl_t *dtp, dtrace_probedata_t *data, dtrace_epid_t last,
>  			flow = DTRACEFLOW_NONE;
>  	}
>  
> -	/*
> -	 * If we're going to unindent this, it's more difficult to see if
> -	 * we don't actually want to unindent it -- we need to look at the
> -	 * _next_ EPID.
> -	 */
> -	if (flow == DTRACEFLOW_RETURN && next != DTRACE_EPIDNONE &&
> -	    next != id) {
> -		rval = dt_epid_lookup(dtp, next, &ndd, &npd);
> -		if (rval != 0)
> -			return rval;
> -
> -		if (npd->id == pd->id)
> -			flow = DTRACEFLOW_NONE;
> -	}
> -
>  	if (flow == DTRACEFLOW_ENTRY || flow == DTRACEFLOW_RETURN)
>  		data->dtpda_prefix = str;
>  	else
> @@ -2327,7 +2309,7 @@ dt_consume_one_probe(dtrace_hdl_t *dtp, FILE *fp, char *data, uint32_t size,
>  
>  	if (data_recording) {
>  		if (flow)
> -			dt_flowindent(dtp, pdat, *last, DTRACE_EPIDNONE);
> +			dt_flowindent(dtp, pdat, *last);
>  
>  		rval = (*efunc)(pdat, arg);
>  
> -- 
> 2.43.5
> 

[PATCH 05/22] Set the ERROR PRID in BPF code

[eugene.loh]

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

We use the fact that the ERROR PRID is always 3.

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 bpf/probe_error.c                         |  3 +++
 test/unittest/builtinvar/tst.id_ERROR.d   | 32 +++++++++++++++++++++++
 test/unittest/builtinvar/tst.id_ERROR.r   |  3 +++
 test/unittest/builtinvar/tst.id_ERROR.r.p |  4 +++
 4 files changed, 42 insertions(+)
 create mode 100644 test/unittest/builtinvar/tst.id_ERROR.d
 create mode 100644 test/unittest/builtinvar/tst.id_ERROR.r
 create mode 100755 test/unittest/builtinvar/tst.id_ERROR.r.p

diff --git a/bpf/probe_error.c b/bpf/probe_error.c
index c8ddcdfa..ee1a1793 100644
--- a/bpf/probe_error.c
+++ b/bpf/probe_error.c
@@ -26,6 +26,7 @@ noinline void dt_probe_error(const dt_dctx_t *dctx, uint64_t pc, uint64_t fault,
 			     uint64_t illval)
 {
 	dt_mstate_t	*mst = dctx->mst;
+	int		oldprid = mst->prid;
 
 	mst->argv[0] = 0;
 	mst->argv[1] = mst->epid;
@@ -34,7 +35,9 @@ noinline void dt_probe_error(const dt_dctx_t *dctx, uint64_t pc, uint64_t fault,
 	mst->argv[4] = fault;
 	mst->argv[5] = illval;
 
+	mst->prid = 3;
 	dt_error(dctx);
+	mst->prid = oldprid;
 
 	mst->fault = fault;
 }
diff --git a/test/unittest/builtinvar/tst.id_ERROR.d b/test/unittest/builtinvar/tst.id_ERROR.d
new file mode 100644
index 00000000..59021c60
--- /dev/null
+++ b/test/unittest/builtinvar/tst.id_ERROR.d
@@ -0,0 +1,32 @@
+/*
+ * 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.
+ */
+
+/*
+ * ASSERTION:
+ * The id in the ERROR probe is 3.
+ *
+ * SECTION: Variables/Built-in Variables
+ */
+
+#pragma D option quiet
+
+tick-1s
+{
+	/* trigger the ERROR probe */
+	trace(*((int*)0));
+}
+
+tick-2s
+{
+	exit(1);
+}
+
+ERROR
+{
+	printf("id of the ERROR probe = %d\n", id);
+	exit(0);
+}
diff --git a/test/unittest/builtinvar/tst.id_ERROR.r b/test/unittest/builtinvar/tst.id_ERROR.r
new file mode 100644
index 00000000..95974abe
--- /dev/null
+++ b/test/unittest/builtinvar/tst.id_ERROR.r
@@ -0,0 +1,3 @@
+id of the ERROR probe = 3
+
+-- @@stderr --
diff --git a/test/unittest/builtinvar/tst.id_ERROR.r.p b/test/unittest/builtinvar/tst.id_ERROR.r.p
new file mode 100755
index 00000000..884b43f4
--- /dev/null
+++ b/test/unittest/builtinvar/tst.id_ERROR.r.p
@@ -0,0 +1,4 @@
+#!/usr/bin/gawk -f
+
+# Drop the line with run-dependent PRID for profile probe.
+!/error on enabled probe ID/ { print }
-- 
2.43.5

Re: [PATCH 05/22] Set the ERROR PRID in BPF code

[Kris Van Hees]

On Thu, Aug 29, 2024 at 01:22:02AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> We use the fact that the ERROR PRID is always 3.

Where do we use it?  We certainly should never override the probe id that is
assigned by dtrace when providers provide probes.  Will it always be 3?  Almost
certainly, yes.  But the right way to do this is to actually make the ERROR
PRID available as a symbol that can be resolved at program load time, so that
the dt_probe_error function can get to its value.

Anyway, the need to restore the mst->prid value in this patch also highlights
that there is a much bigger problem...  A fault in one clause contaminates the
first 6 arguments of the probe, and since a fault only interrupts the execution
of the current clause, following clauses will no longer see the correct values
of the first 6 arguments but rather the ones that dt_probe_error() stored in
mst->argv[0 .. 5].  That is a bug for sure!

Here is a test that demonstrates this issue:

#pragma D option quiet

syscall::write*:entry
{
	self->arg0 = arg0;
	self->arg1 = arg1;
	self->arg2 = arg2;
	self->arg3 = arg3;
	self->arg4 = arg4;
	self->arg5 = arg5;

	printf("%d / %d / %d / %d / %d / %d\n",
	       arg0, arg1, arg2, arg3, arg4, arg5);
}

syscall::write*:entry
{
	trace(*(int *)0);
}

syscall::write*:entry,
ERROR {
	printf("%d / %d / %d / %d / %d / %d\n",
	       arg0, arg1, arg2, arg3, arg4, arg5);
}

syscall::write*:entry
{
	exit(self->arg0 != arg0 || self->arg1 != arg1 || self->arg2 != arg2 ||
	     self->arg3 != arg3 || self->arg4 != arg4 || self->arg5 != arg5
		? 1 : 0);
}

So, we need the ERROR prid value exposed as an external symbol so that it can
be used in dt_probe_error(), and a patch that fixes the contamination of the
arg values.

> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> ---
>  bpf/probe_error.c                         |  3 +++
>  test/unittest/builtinvar/tst.id_ERROR.d   | 32 +++++++++++++++++++++++
>  test/unittest/builtinvar/tst.id_ERROR.r   |  3 +++
>  test/unittest/builtinvar/tst.id_ERROR.r.p |  4 +++
>  4 files changed, 42 insertions(+)
>  create mode 100644 test/unittest/builtinvar/tst.id_ERROR.d
>  create mode 100644 test/unittest/builtinvar/tst.id_ERROR.r
>  create mode 100755 test/unittest/builtinvar/tst.id_ERROR.r.p
> 
> diff --git a/bpf/probe_error.c b/bpf/probe_error.c
> index c8ddcdfa..ee1a1793 100644
> --- a/bpf/probe_error.c
> +++ b/bpf/probe_error.c
> @@ -26,6 +26,7 @@ noinline void dt_probe_error(const dt_dctx_t *dctx, uint64_t pc, uint64_t fault,
>  			     uint64_t illval)
>  {
>  	dt_mstate_t	*mst = dctx->mst;
> +	int		oldprid = mst->prid;
>  
>  	mst->argv[0] = 0;
>  	mst->argv[1] = mst->epid;
> @@ -34,7 +35,9 @@ noinline void dt_probe_error(const dt_dctx_t *dctx, uint64_t pc, uint64_t fault,
>  	mst->argv[4] = fault;
>  	mst->argv[5] = illval;
>  
> +	mst->prid = 3;
>  	dt_error(dctx);
> +	mst->prid = oldprid;
>  
>  	mst->fault = fault;
>  }
> diff --git a/test/unittest/builtinvar/tst.id_ERROR.d b/test/unittest/builtinvar/tst.id_ERROR.d
> new file mode 100644
> index 00000000..59021c60
> --- /dev/null
> +++ b/test/unittest/builtinvar/tst.id_ERROR.d
> @@ -0,0 +1,32 @@
> +/*
> + * 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.
> + */
> +
> +/*
> + * ASSERTION:
> + * The id in the ERROR probe is 3.
> + *
> + * SECTION: Variables/Built-in Variables
> + */
> +
> +#pragma D option quiet
> +
> +tick-1s
> +{
> +	/* trigger the ERROR probe */
> +	trace(*((int*)0));
> +}
> +
> +tick-2s
> +{
> +	exit(1);
> +}
> +
> +ERROR
> +{
> +	printf("id of the ERROR probe = %d\n", id);
> +	exit(0);
> +}
> diff --git a/test/unittest/builtinvar/tst.id_ERROR.r b/test/unittest/builtinvar/tst.id_ERROR.r
> new file mode 100644
> index 00000000..95974abe
> --- /dev/null
> +++ b/test/unittest/builtinvar/tst.id_ERROR.r
> @@ -0,0 +1,3 @@
> +id of the ERROR probe = 3
> +
> +-- @@stderr --
> diff --git a/test/unittest/builtinvar/tst.id_ERROR.r.p b/test/unittest/builtinvar/tst.id_ERROR.r.p
> new file mode 100755
> index 00000000..884b43f4
> --- /dev/null
> +++ b/test/unittest/builtinvar/tst.id_ERROR.r.p
> @@ -0,0 +1,4 @@
> +#!/usr/bin/gawk -f
> +
> +# Drop the line with run-dependent PRID for profile probe.
> +!/error on enabled probe ID/ { print }
> -- 
> 2.43.5
> 

Re: [PATCH 05/22] Set the ERROR PRID in BPF code

[Eugene Loh]

On 9/6/24 20:20, Kris Van Hees wrote:

> On Thu, Aug 29, 2024 at 01:22:02AM -0400, eugene.loh@oracle.com wrote:
>> From: Eugene Loh <eugene.loh@oracle.com>
>>
>> We use the fact that the ERROR PRID is always 3.
> Where do we use it?  We certainly should never override the probe id that is
> assigned by dtrace when providers provide probes.  Will it always be 3?  Almost
> certainly, yes.  But the right way to do this is to actually make the ERROR
> PRID available as a symbol that can be resolved at program load time, so that
> the dt_probe_error function can get to its value.

I'm confused?  I thought I did this and you suggested simply hardwiring 
the value 3. 
https://oss.oracle.com/pipermail/dtrace-devel/2024-July/004989.html

> Anyway, the need to restore the mst->prid value in this patch also highlights
> that there is a much bigger problem...  A fault in one clause contaminates the
> first 6 arguments of the probe, and since a fault only interrupts the execution
> of the current clause, following clauses will no longer see the correct values
> of the first 6 arguments but rather the ones that dt_probe_error() stored in
> mst->argv[0 .. 5].  That is a bug for sure!

Good point, but... a separate issue/patch?

> Here is a test that demonstrates this issue:
>
> #pragma D option quiet
>
> syscall::write*:entry
> {
> 	self->arg0 = arg0;
> 	self->arg1 = arg1;
> 	self->arg2 = arg2;
> 	self->arg3 = arg3;
> 	self->arg4 = arg4;
> 	self->arg5 = arg5;
>
> 	printf("%d / %d / %d / %d / %d / %d\n",
> 	       arg0, arg1, arg2, arg3, arg4, arg5);
> }
>
> syscall::write*:entry
> {
> 	trace(*(int *)0);
> }
>
> syscall::write*:entry,
> ERROR {
> 	printf("%d / %d / %d / %d / %d / %d\n",
> 	       arg0, arg1, arg2, arg3, arg4, arg5);
> }
>
> syscall::write*:entry
> {
> 	exit(self->arg0 != arg0 || self->arg1 != arg1 || self->arg2 != arg2 ||
> 	     self->arg3 != arg3 || self->arg4 != arg4 || self->arg5 != arg5
> 		? 1 : 0);
> }
>
> So, we need the ERROR prid value exposed as an external symbol so that it can
> be used in dt_probe_error(), and a patch that fixes the contamination of the
> arg values.
>
>> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
>> ---
>>   bpf/probe_error.c                         |  3 +++
>>   test/unittest/builtinvar/tst.id_ERROR.d   | 32 +++++++++++++++++++++++
>>   test/unittest/builtinvar/tst.id_ERROR.r   |  3 +++
>>   test/unittest/builtinvar/tst.id_ERROR.r.p |  4 +++
>>   4 files changed, 42 insertions(+)
>>   create mode 100644 test/unittest/builtinvar/tst.id_ERROR.d
>>   create mode 100644 test/unittest/builtinvar/tst.id_ERROR.r
>>   create mode 100755 test/unittest/builtinvar/tst.id_ERROR.r.p
>>
>> diff --git a/bpf/probe_error.c b/bpf/probe_error.c
>> index c8ddcdfa..ee1a1793 100644
>> --- a/bpf/probe_error.c
>> +++ b/bpf/probe_error.c
>> @@ -26,6 +26,7 @@ noinline void dt_probe_error(const dt_dctx_t *dctx, uint64_t pc, uint64_t fault,
>>   			     uint64_t illval)
>>   {
>>   	dt_mstate_t	*mst = dctx->mst;
>> +	int		oldprid = mst->prid;
>>   
>>   	mst->argv[0] = 0;
>>   	mst->argv[1] = mst->epid;
>> @@ -34,7 +35,9 @@ noinline void dt_probe_error(const dt_dctx_t *dctx, uint64_t pc, uint64_t fault,
>>   	mst->argv[4] = fault;
>>   	mst->argv[5] = illval;
>>   
>> +	mst->prid = 3;
>>   	dt_error(dctx);
>> +	mst->prid = oldprid;
>>   
>>   	mst->fault = fault;
>>   }
>> diff --git a/test/unittest/builtinvar/tst.id_ERROR.d b/test/unittest/builtinvar/tst.id_ERROR.d
>> new file mode 100644
>> index 00000000..59021c60
>> --- /dev/null
>> +++ b/test/unittest/builtinvar/tst.id_ERROR.d
>> @@ -0,0 +1,32 @@
>> +/*
>> + * 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.
>> + */
>> +
>> +/*
>> + * ASSERTION:
>> + * The id in the ERROR probe is 3.
>> + *
>> + * SECTION: Variables/Built-in Variables
>> + */
>> +
>> +#pragma D option quiet
>> +
>> +tick-1s
>> +{
>> +	/* trigger the ERROR probe */
>> +	trace(*((int*)0));
>> +}
>> +
>> +tick-2s
>> +{
>> +	exit(1);
>> +}
>> +
>> +ERROR
>> +{
>> +	printf("id of the ERROR probe = %d\n", id);
>> +	exit(0);
>> +}
>> diff --git a/test/unittest/builtinvar/tst.id_ERROR.r b/test/unittest/builtinvar/tst.id_ERROR.r
>> new file mode 100644
>> index 00000000..95974abe
>> --- /dev/null
>> +++ b/test/unittest/builtinvar/tst.id_ERROR.r
>> @@ -0,0 +1,3 @@
>> +id of the ERROR probe = 3
>> +
>> +-- @@stderr --
>> diff --git a/test/unittest/builtinvar/tst.id_ERROR.r.p b/test/unittest/builtinvar/tst.id_ERROR.r.p
>> new file mode 100755
>> index 00000000..884b43f4
>> --- /dev/null
>> +++ b/test/unittest/builtinvar/tst.id_ERROR.r.p
>> @@ -0,0 +1,4 @@
>> +#!/usr/bin/gawk -f
>> +
>> +# Drop the line with run-dependent PRID for profile probe.
>> +!/error on enabled probe ID/ { print }
>> -- 
>> 2.43.5
>>

Re: [PATCH 05/22] Set the ERROR PRID in BPF code

[Kris Van Hees]

On Fri, Sep 06, 2024 at 09:25:12PM -0400, Eugene Loh wrote:
> On 9/6/24 20:20, Kris Van Hees wrote:
> 
> > On Thu, Aug 29, 2024 at 01:22:02AM -0400, eugene.loh@oracle.com wrote:
> > > From: Eugene Loh <eugene.loh@oracle.com>
> > > 
> > > We use the fact that the ERROR PRID is always 3.
> > Where do we use it?  We certainly should never override the probe id that is
> > assigned by dtrace when providers provide probes.  Will it always be 3?  Almost
> > certainly, yes.  But the right way to do this is to actually make the ERROR
> > PRID available as a symbol that can be resolved at program load time, so that
> > the dt_probe_error function can get to its value.
> 
> I'm confused?  I thought I did this and you suggested simply hardwiring the
> value 3. https://oss.oracle.com/pipermail/dtrace-devel/2024-July/004989.html

True, though I didn't mean to just hardwire the value 3.  At a minimum, I think
it ought to be a define to set the value of the BEGIN, END, and ERROR probe,
so that a symbolic name can be used in code.  And that probably would mean that
it is best to add a check in the dtrace provider to ensure that those are
indeed the probe ids assigned to BEGIN/END/ERROR (and if not, something is very
wrong).

> > Anyway, the need to restore the mst->prid value in this patch also highlights
> > that there is a much bigger problem...  A fault in one clause contaminates the
> > first 6 arguments of the probe, and since a fault only interrupts the execution
> > of the current clause, following clauses will no longer see the correct values
> > of the first 6 arguments but rather the ones that dt_probe_error() stored in
> > mst->argv[0 .. 5].  That is a bug for sure!
> 
> Good point, but... a separate issue/patch?

Yes, already written :)

> > Here is a test that demonstrates this issue:
> > 
> > #pragma D option quiet
> > 
> > syscall::write*:entry
> > {
> > 	self->arg0 = arg0;
> > 	self->arg1 = arg1;
> > 	self->arg2 = arg2;
> > 	self->arg3 = arg3;
> > 	self->arg4 = arg4;
> > 	self->arg5 = arg5;
> > 
> > 	printf("%d / %d / %d / %d / %d / %d\n",
> > 	       arg0, arg1, arg2, arg3, arg4, arg5);
> > }
> > 
> > syscall::write*:entry
> > {
> > 	trace(*(int *)0);
> > }
> > 
> > syscall::write*:entry,
> > ERROR {
> > 	printf("%d / %d / %d / %d / %d / %d\n",
> > 	       arg0, arg1, arg2, arg3, arg4, arg5);
> > }
> > 
> > syscall::write*:entry
> > {
> > 	exit(self->arg0 != arg0 || self->arg1 != arg1 || self->arg2 != arg2 ||
> > 	     self->arg3 != arg3 || self->arg4 != arg4 || self->arg5 != arg5
> > 		? 1 : 0);
> > }
> > 
> > So, we need the ERROR prid value exposed as an external symbol so that it can
> > be used in dt_probe_error(), and a patch that fixes the contamination of the
> > arg values.
> > 
> > > Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> > > ---
> > >   bpf/probe_error.c                         |  3 +++
> > >   test/unittest/builtinvar/tst.id_ERROR.d   | 32 +++++++++++++++++++++++
> > >   test/unittest/builtinvar/tst.id_ERROR.r   |  3 +++
> > >   test/unittest/builtinvar/tst.id_ERROR.r.p |  4 +++
> > >   4 files changed, 42 insertions(+)
> > >   create mode 100644 test/unittest/builtinvar/tst.id_ERROR.d
> > >   create mode 100644 test/unittest/builtinvar/tst.id_ERROR.r
> > >   create mode 100755 test/unittest/builtinvar/tst.id_ERROR.r.p
> > > 
> > > diff --git a/bpf/probe_error.c b/bpf/probe_error.c
> > > index c8ddcdfa..ee1a1793 100644
> > > --- a/bpf/probe_error.c
> > > +++ b/bpf/probe_error.c
> > > @@ -26,6 +26,7 @@ noinline void dt_probe_error(const dt_dctx_t *dctx, uint64_t pc, uint64_t fault,
> > >   			     uint64_t illval)
> > >   {
> > >   	dt_mstate_t	*mst = dctx->mst;
> > > +	int		oldprid = mst->prid;
> > >   	mst->argv[0] = 0;
> > >   	mst->argv[1] = mst->epid;
> > > @@ -34,7 +35,9 @@ noinline void dt_probe_error(const dt_dctx_t *dctx, uint64_t pc, uint64_t fault,
> > >   	mst->argv[4] = fault;
> > >   	mst->argv[5] = illval;
> > > +	mst->prid = 3;
> > >   	dt_error(dctx);
> > > +	mst->prid = oldprid;
> > >   	mst->fault = fault;
> > >   }
> > > diff --git a/test/unittest/builtinvar/tst.id_ERROR.d b/test/unittest/builtinvar/tst.id_ERROR.d
> > > new file mode 100644
> > > index 00000000..59021c60
> > > --- /dev/null
> > > +++ b/test/unittest/builtinvar/tst.id_ERROR.d
> > > @@ -0,0 +1,32 @@
> > > +/*
> > > + * 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.
> > > + */
> > > +
> > > +/*
> > > + * ASSERTION:
> > > + * The id in the ERROR probe is 3.
> > > + *
> > > + * SECTION: Variables/Built-in Variables
> > > + */
> > > +
> > > +#pragma D option quiet
> > > +
> > > +tick-1s
> > > +{
> > > +	/* trigger the ERROR probe */
> > > +	trace(*((int*)0));
> > > +}
> > > +
> > > +tick-2s
> > > +{
> > > +	exit(1);
> > > +}
> > > +
> > > +ERROR
> > > +{
> > > +	printf("id of the ERROR probe = %d\n", id);
> > > +	exit(0);
> > > +}
> > > diff --git a/test/unittest/builtinvar/tst.id_ERROR.r b/test/unittest/builtinvar/tst.id_ERROR.r
> > > new file mode 100644
> > > index 00000000..95974abe
> > > --- /dev/null
> > > +++ b/test/unittest/builtinvar/tst.id_ERROR.r
> > > @@ -0,0 +1,3 @@
> > > +id of the ERROR probe = 3
> > > +
> > > +-- @@stderr --
> > > diff --git a/test/unittest/builtinvar/tst.id_ERROR.r.p b/test/unittest/builtinvar/tst.id_ERROR.r.p
> > > new file mode 100755
> > > index 00000000..884b43f4
> > > --- /dev/null
> > > +++ b/test/unittest/builtinvar/tst.id_ERROR.r.p
> > > @@ -0,0 +1,4 @@
> > > +#!/usr/bin/gawk -f
> > > +
> > > +# Drop the line with run-dependent PRID for profile probe.
> > > +!/error on enabled probe ID/ { print }
> > > -- 
> > > 2.43.5
> > > 

Re: [PATCH 05/22] Set the ERROR PRID in BPF code

[Kris Van Hees]

In addition, I think that the test should compare the id reported by the
ERROR probe against the id given in dtrace -ln dtrace:::ERROR output.  Yes,
it will be 3 (and I think putting the following three defines in dt_dctx.h
would be helpful with that), but by doing the comparison against the dtrace -l
output avoids having to encode a constant value in the expected output (and
we cannot easily access the defines from the test).

/* Static probe IDs for the dtrace provider. */
#define DTRACE_BEGIN_ID		1
#define DTRACE_END_ID		2
#define DTRACE_ERROR_ID		3

On Fri, Sep 06, 2024 at 10:03:38PM -0400, Kris Van Hees wrote:
> On Fri, Sep 06, 2024 at 09:25:12PM -0400, Eugene Loh wrote:
> > On 9/6/24 20:20, Kris Van Hees wrote:
> > 
> > > On Thu, Aug 29, 2024 at 01:22:02AM -0400, eugene.loh@oracle.com wrote:
> > > > From: Eugene Loh <eugene.loh@oracle.com>
> > > > 
> > > > We use the fact that the ERROR PRID is always 3.
> > > Where do we use it?  We certainly should never override the probe id that is
> > > assigned by dtrace when providers provide probes.  Will it always be 3?  Almost
> > > certainly, yes.  But the right way to do this is to actually make the ERROR
> > > PRID available as a symbol that can be resolved at program load time, so that
> > > the dt_probe_error function can get to its value.
> > 
> > I'm confused?  I thought I did this and you suggested simply hardwiring the
> > value 3. https://oss.oracle.com/pipermail/dtrace-devel/2024-July/004989.html
> 
> True, though I didn't mean to just hardwire the value 3.  At a minimum, I think
> it ought to be a define to set the value of the BEGIN, END, and ERROR probe,
> so that a symbolic name can be used in code.  And that probably would mean that
> it is best to add a check in the dtrace provider to ensure that those are
> indeed the probe ids assigned to BEGIN/END/ERROR (and if not, something is very
> wrong).
> 
> > > Anyway, the need to restore the mst->prid value in this patch also highlights
> > > that there is a much bigger problem...  A fault in one clause contaminates the
> > > first 6 arguments of the probe, and since a fault only interrupts the execution
> > > of the current clause, following clauses will no longer see the correct values
> > > of the first 6 arguments but rather the ones that dt_probe_error() stored in
> > > mst->argv[0 .. 5].  That is a bug for sure!
> > 
> > Good point, but... a separate issue/patch?
> 
> Yes, already written :)
> 
> > > Here is a test that demonstrates this issue:
> > > 
> > > #pragma D option quiet
> > > 
> > > syscall::write*:entry
> > > {
> > > 	self->arg0 = arg0;
> > > 	self->arg1 = arg1;
> > > 	self->arg2 = arg2;
> > > 	self->arg3 = arg3;
> > > 	self->arg4 = arg4;
> > > 	self->arg5 = arg5;
> > > 
> > > 	printf("%d / %d / %d / %d / %d / %d\n",
> > > 	       arg0, arg1, arg2, arg3, arg4, arg5);
> > > }
> > > 
> > > syscall::write*:entry
> > > {
> > > 	trace(*(int *)0);
> > > }
> > > 
> > > syscall::write*:entry,
> > > ERROR {
> > > 	printf("%d / %d / %d / %d / %d / %d\n",
> > > 	       arg0, arg1, arg2, arg3, arg4, arg5);
> > > }
> > > 
> > > syscall::write*:entry
> > > {
> > > 	exit(self->arg0 != arg0 || self->arg1 != arg1 || self->arg2 != arg2 ||
> > > 	     self->arg3 != arg3 || self->arg4 != arg4 || self->arg5 != arg5
> > > 		? 1 : 0);
> > > }
> > > 
> > > So, we need the ERROR prid value exposed as an external symbol so that it can
> > > be used in dt_probe_error(), and a patch that fixes the contamination of the
> > > arg values.
> > > 
> > > > Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> > > > ---
> > > >   bpf/probe_error.c                         |  3 +++
> > > >   test/unittest/builtinvar/tst.id_ERROR.d   | 32 +++++++++++++++++++++++
> > > >   test/unittest/builtinvar/tst.id_ERROR.r   |  3 +++
> > > >   test/unittest/builtinvar/tst.id_ERROR.r.p |  4 +++
> > > >   4 files changed, 42 insertions(+)
> > > >   create mode 100644 test/unittest/builtinvar/tst.id_ERROR.d
> > > >   create mode 100644 test/unittest/builtinvar/tst.id_ERROR.r
> > > >   create mode 100755 test/unittest/builtinvar/tst.id_ERROR.r.p
> > > > 
> > > > diff --git a/bpf/probe_error.c b/bpf/probe_error.c
> > > > index c8ddcdfa..ee1a1793 100644
> > > > --- a/bpf/probe_error.c
> > > > +++ b/bpf/probe_error.c
> > > > @@ -26,6 +26,7 @@ noinline void dt_probe_error(const dt_dctx_t *dctx, uint64_t pc, uint64_t fault,
> > > >   			     uint64_t illval)
> > > >   {
> > > >   	dt_mstate_t	*mst = dctx->mst;
> > > > +	int		oldprid = mst->prid;
> > > >   	mst->argv[0] = 0;
> > > >   	mst->argv[1] = mst->epid;
> > > > @@ -34,7 +35,9 @@ noinline void dt_probe_error(const dt_dctx_t *dctx, uint64_t pc, uint64_t fault,
> > > >   	mst->argv[4] = fault;
> > > >   	mst->argv[5] = illval;
> > > > +	mst->prid = 3;
> > > >   	dt_error(dctx);
> > > > +	mst->prid = oldprid;
> > > >   	mst->fault = fault;
> > > >   }
> > > > diff --git a/test/unittest/builtinvar/tst.id_ERROR.d b/test/unittest/builtinvar/tst.id_ERROR.d
> > > > new file mode 100644
> > > > index 00000000..59021c60
> > > > --- /dev/null
> > > > +++ b/test/unittest/builtinvar/tst.id_ERROR.d
> > > > @@ -0,0 +1,32 @@
> > > > +/*
> > > > + * 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.
> > > > + */
> > > > +
> > > > +/*
> > > > + * ASSERTION:
> > > > + * The id in the ERROR probe is 3.
> > > > + *
> > > > + * SECTION: Variables/Built-in Variables
> > > > + */
> > > > +
> > > > +#pragma D option quiet
> > > > +
> > > > +tick-1s
> > > > +{
> > > > +	/* trigger the ERROR probe */
> > > > +	trace(*((int*)0));
> > > > +}
> > > > +
> > > > +tick-2s
> > > > +{
> > > > +	exit(1);
> > > > +}
> > > > +
> > > > +ERROR
> > > > +{
> > > > +	printf("id of the ERROR probe = %d\n", id);
> > > > +	exit(0);
> > > > +}
> > > > diff --git a/test/unittest/builtinvar/tst.id_ERROR.r b/test/unittest/builtinvar/tst.id_ERROR.r
> > > > new file mode 100644
> > > > index 00000000..95974abe
> > > > --- /dev/null
> > > > +++ b/test/unittest/builtinvar/tst.id_ERROR.r
> > > > @@ -0,0 +1,3 @@
> > > > +id of the ERROR probe = 3
> > > > +
> > > > +-- @@stderr --
> > > > diff --git a/test/unittest/builtinvar/tst.id_ERROR.r.p b/test/unittest/builtinvar/tst.id_ERROR.r.p
> > > > new file mode 100755
> > > > index 00000000..884b43f4
> > > > --- /dev/null
> > > > +++ b/test/unittest/builtinvar/tst.id_ERROR.r.p
> > > > @@ -0,0 +1,4 @@
> > > > +#!/usr/bin/gawk -f
> > > > +
> > > > +# Drop the line with run-dependent PRID for profile probe.
> > > > +!/error on enabled probe ID/ { print }
> > > > -- 
> > > > 2.43.5
> > > > 

Re: [PATCH 05/22] Set the ERROR PRID in BPF code

[Eugene Loh]

Okay.  I posted a v2.  I also posted a v5 of "Deprecate EPID" since it 
experiences a minor side effect.

On 9/12/24 16:33, Kris Van Hees wrote:
> In addition, I think that the test should compare the id reported by the
> ERROR probe against the id given in dtrace -ln dtrace:::ERROR output.  Yes,
> it will be 3 (and I think putting the following three defines in dt_dctx.h
> would be helpful with that), but by doing the comparison against the dtrace -l
> output avoids having to encode a constant value in the expected output (and
> we cannot easily access the defines from the test).
>
> /* Static probe IDs for the dtrace provider. */
> #define DTRACE_BEGIN_ID		1
> #define DTRACE_END_ID		2
> #define DTRACE_ERROR_ID		3
>
> On Fri, Sep 06, 2024 at 10:03:38PM -0400, Kris Van Hees wrote:
>> On Fri, Sep 06, 2024 at 09:25:12PM -0400, Eugene Loh wrote:
>>> On 9/6/24 20:20, Kris Van Hees wrote:
>>>
>>>> On Thu, Aug 29, 2024 at 01:22:02AM -0400, eugene.loh@oracle.com wrote:
>>>>> From: Eugene Loh <eugene.loh@oracle.com>
>>>>>
>>>>> We use the fact that the ERROR PRID is always 3.
>>>> Where do we use it?  We certainly should never override the probe id that is
>>>> assigned by dtrace when providers provide probes.  Will it always be 3?  Almost
>>>> certainly, yes.  But the right way to do this is to actually make the ERROR
>>>> PRID available as a symbol that can be resolved at program load time, so that
>>>> the dt_probe_error function can get to its value.
>>> I'm confused?  I thought I did this and you suggested simply hardwiring the
>>> value 3. https://oss.oracle.com/pipermail/dtrace-devel/2024-July/004989.html
>> True, though I didn't mean to just hardwire the value 3.  At a minimum, I think
>> it ought to be a define to set the value of the BEGIN, END, and ERROR probe,
>> so that a symbolic name can be used in code.  And that probably would mean that
>> it is best to add a check in the dtrace provider to ensure that those are
>> indeed the probe ids assigned to BEGIN/END/ERROR (and if not, something is very
>> wrong).
>>
>>>> Anyway, the need to restore the mst->prid value in this patch also highlights
>>>> that there is a much bigger problem...  A fault in one clause contaminates the
>>>> first 6 arguments of the probe, and since a fault only interrupts the execution
>>>> of the current clause, following clauses will no longer see the correct values
>>>> of the first 6 arguments but rather the ones that dt_probe_error() stored in
>>>> mst->argv[0 .. 5].  That is a bug for sure!
>>> Good point, but... a separate issue/patch?
>> Yes, already written :)
>>
>>>> Here is a test that demonstrates this issue:
>>>>
>>>> #pragma D option quiet
>>>>
>>>> syscall::write*:entry
>>>> {
>>>> 	self->arg0 = arg0;
>>>> 	self->arg1 = arg1;
>>>> 	self->arg2 = arg2;
>>>> 	self->arg3 = arg3;
>>>> 	self->arg4 = arg4;
>>>> 	self->arg5 = arg5;
>>>>
>>>> 	printf("%d / %d / %d / %d / %d / %d\n",
>>>> 	       arg0, arg1, arg2, arg3, arg4, arg5);
>>>> }
>>>>
>>>> syscall::write*:entry
>>>> {
>>>> 	trace(*(int *)0);
>>>> }
>>>>
>>>> syscall::write*:entry,
>>>> ERROR {
>>>> 	printf("%d / %d / %d / %d / %d / %d\n",
>>>> 	       arg0, arg1, arg2, arg3, arg4, arg5);
>>>> }
>>>>
>>>> syscall::write*:entry
>>>> {
>>>> 	exit(self->arg0 != arg0 || self->arg1 != arg1 || self->arg2 != arg2 ||
>>>> 	     self->arg3 != arg3 || self->arg4 != arg4 || self->arg5 != arg5
>>>> 		? 1 : 0);
>>>> }
>>>>
>>>> So, we need the ERROR prid value exposed as an external symbol so that it can
>>>> be used in dt_probe_error(), and a patch that fixes the contamination of the
>>>> arg values.
>>>>
>>>>> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
>>>>> ---
>>>>>    bpf/probe_error.c                         |  3 +++
>>>>>    test/unittest/builtinvar/tst.id_ERROR.d   | 32 +++++++++++++++++++++++
>>>>>    test/unittest/builtinvar/tst.id_ERROR.r   |  3 +++
>>>>>    test/unittest/builtinvar/tst.id_ERROR.r.p |  4 +++
>>>>>    4 files changed, 42 insertions(+)
>>>>>    create mode 100644 test/unittest/builtinvar/tst.id_ERROR.d
>>>>>    create mode 100644 test/unittest/builtinvar/tst.id_ERROR.r
>>>>>    create mode 100755 test/unittest/builtinvar/tst.id_ERROR.r.p
>>>>>
>>>>> diff --git a/bpf/probe_error.c b/bpf/probe_error.c
>>>>> index c8ddcdfa..ee1a1793 100644
>>>>> --- a/bpf/probe_error.c
>>>>> +++ b/bpf/probe_error.c
>>>>> @@ -26,6 +26,7 @@ noinline void dt_probe_error(const dt_dctx_t *dctx, uint64_t pc, uint64_t fault,
>>>>>    			     uint64_t illval)
>>>>>    {
>>>>>    	dt_mstate_t	*mst = dctx->mst;
>>>>> +	int		oldprid = mst->prid;
>>>>>    	mst->argv[0] = 0;
>>>>>    	mst->argv[1] = mst->epid;
>>>>> @@ -34,7 +35,9 @@ noinline void dt_probe_error(const dt_dctx_t *dctx, uint64_t pc, uint64_t fault,
>>>>>    	mst->argv[4] = fault;
>>>>>    	mst->argv[5] = illval;
>>>>> +	mst->prid = 3;
>>>>>    	dt_error(dctx);
>>>>> +	mst->prid = oldprid;
>>>>>    	mst->fault = fault;
>>>>>    }
>>>>> diff --git a/test/unittest/builtinvar/tst.id_ERROR.d b/test/unittest/builtinvar/tst.id_ERROR.d
>>>>> new file mode 100644
>>>>> index 00000000..59021c60
>>>>> --- /dev/null
>>>>> +++ b/test/unittest/builtinvar/tst.id_ERROR.d
>>>>> @@ -0,0 +1,32 @@
>>>>> +/*
>>>>> + * 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.
>>>>> + */
>>>>> +
>>>>> +/*
>>>>> + * ASSERTION:
>>>>> + * The id in the ERROR probe is 3.
>>>>> + *
>>>>> + * SECTION: Variables/Built-in Variables
>>>>> + */
>>>>> +
>>>>> +#pragma D option quiet
>>>>> +
>>>>> +tick-1s
>>>>> +{
>>>>> +	/* trigger the ERROR probe */
>>>>> +	trace(*((int*)0));
>>>>> +}
>>>>> +
>>>>> +tick-2s
>>>>> +{
>>>>> +	exit(1);
>>>>> +}
>>>>> +
>>>>> +ERROR
>>>>> +{
>>>>> +	printf("id of the ERROR probe = %d\n", id);
>>>>> +	exit(0);
>>>>> +}
>>>>> diff --git a/test/unittest/builtinvar/tst.id_ERROR.r b/test/unittest/builtinvar/tst.id_ERROR.r
>>>>> new file mode 100644
>>>>> index 00000000..95974abe
>>>>> --- /dev/null
>>>>> +++ b/test/unittest/builtinvar/tst.id_ERROR.r
>>>>> @@ -0,0 +1,3 @@
>>>>> +id of the ERROR probe = 3
>>>>> +
>>>>> +-- @@stderr --
>>>>> diff --git a/test/unittest/builtinvar/tst.id_ERROR.r.p b/test/unittest/builtinvar/tst.id_ERROR.r.p
>>>>> new file mode 100755
>>>>> index 00000000..884b43f4
>>>>> --- /dev/null
>>>>> +++ b/test/unittest/builtinvar/tst.id_ERROR.r.p
>>>>> @@ -0,0 +1,4 @@
>>>>> +#!/usr/bin/gawk -f
>>>>> +
>>>>> +# Drop the line with run-dependent PRID for profile probe.
>>>>> +!/error on enabled probe ID/ { print }
>>>>> -- 
>>>>> 2.43.5
>>>>>

[PATCH 06/22] Fix provider lookup to use prv not prb

[eugene.loh]

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

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

diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
index 09a9ae58..cbde6a48 100644
--- a/libdtrace/dt_prov_uprobe.c
+++ b/libdtrace/dt_prov_uprobe.c
@@ -247,7 +247,7 @@ static int provide_probe(dtrace_hdl_t *dtp, const pid_probespec_t *psp,
 	pd.prb = prb;
 
 	/* Get (or create) the provider for the PID of the probe. */
-	pvp = dt_provider_lookup(dtp, pd.prb);
+	pvp = dt_provider_lookup(dtp, pd.prv);
 	if (pvp == NULL) {
 		pvp = dt_provider_create(dtp, pd.prv, pvops, &pattr, NULL);
 		if (pvp == NULL)
-- 
2.43.5

Re: [PATCH 06/22] Fix provider lookup to use prv not prb

[Kris Van Hees]

On Thu, Aug 29, 2024 at 01:22:03AM -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_prov_uprobe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
> index 09a9ae58..cbde6a48 100644
> --- a/libdtrace/dt_prov_uprobe.c
> +++ b/libdtrace/dt_prov_uprobe.c
> @@ -247,7 +247,7 @@ static int provide_probe(dtrace_hdl_t *dtp, const pid_probespec_t *psp,
>  	pd.prb = prb;
>  
>  	/* Get (or create) the provider for the PID of the probe. */
> -	pvp = dt_provider_lookup(dtp, pd.prb);
> +	pvp = dt_provider_lookup(dtp, pd.prv);
>  	if (pvp == NULL) {
>  		pvp = dt_provider_create(dtp, pd.prv, pvops, &pattr, NULL);
>  		if (pvp == NULL)
> -- 
> 2.43.5
> 

[PATCH 07/22] Supply a default probe_info()

[eugene.loh]

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

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 libdtrace/dt_probe.c        |  4 +++-
 libdtrace/dt_prov_cpc.c     | 11 -----------
 libdtrace/dt_prov_dtrace.c  | 10 ----------
 libdtrace/dt_prov_fbt.c     | 10 ----------
 libdtrace/dt_prov_profile.c | 11 -----------
 5 files changed, 3 insertions(+), 43 deletions(-)

diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
index 0b53121a..ab90d2ed 100644
--- a/libdtrace/dt_probe.c
+++ b/libdtrace/dt_probe.c
@@ -892,8 +892,10 @@ dt_probe_args_info(dtrace_hdl_t *dtp, dt_probe_t *prp)
 	/* Only retrieve probe argument information once per probe. */
 	if (prp->argc != -1)
 		return 0;
-	if (!prp->prov->impl->probe_info)
+	if (!prp->prov->impl->probe_info) {
+		prp->argc = 0;
 		return 0;
+	}
 	rc = prp->prov->impl->probe_info(dtp, prp, &argc, &argv);
 	if (rc == -1)
 		return rc;
diff --git a/libdtrace/dt_prov_cpc.c b/libdtrace/dt_prov_cpc.c
index 08689b35..8f33cf58 100644
--- a/libdtrace/dt_prov_cpc.c
+++ b/libdtrace/dt_prov_cpc.c
@@ -451,16 +451,6 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
 	return nattach > 0 ? 0 : -1;
 }
 
-static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
-		      int *argcp, dt_argdesc_t **argvp)
-{
-	/* cpc-provider probe arguments are not typed */
-	*argcp = 0;
-	*argvp = NULL;
-
-	return 0;
-}
-
 static void detach(dtrace_hdl_t *dtp, const dt_probe_t *prp)
 {
 	cpc_probe_t	*datap = prp->prv_data;
@@ -504,7 +494,6 @@ dt_provimpl_t	dt_cpc = {
 	.load_prog	= &dt_bpf_prog_load,
 	.trampoline	= &trampoline,
 	.attach		= &attach,
-	.probe_info	= &probe_info,
 	.detach		= &detach,
 	.probe_destroy	= &probe_destroy,
 	.destroy	= &destroy,
diff --git a/libdtrace/dt_prov_dtrace.c b/libdtrace/dt_prov_dtrace.c
index bf87cb05..a9deccee 100644
--- a/libdtrace/dt_prov_dtrace.c
+++ b/libdtrace/dt_prov_dtrace.c
@@ -273,15 +273,6 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
 	return dt_tp_probe_attach(dtp, prp, bpf_fd);
 }
 
-static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
-		      int *argcp, dt_argdesc_t **argvp)
-{
-	*argcp = 0;			/* no arguments */
-	*argvp = NULL;
-
-	return 0;
-}
-
 /*
  * Try to clean up system resources that may have been allocated for this
  * probe.
@@ -317,7 +308,6 @@ dt_provimpl_t	dt_dtrace = {
 	.trampoline	= &trampoline,
 	.load_prog	= &dt_bpf_prog_load,
 	.attach		= &attach,
-	.probe_info	= &probe_info,
 	.detach		= &detach,
 	.probe_destroy	= &dt_tp_probe_destroy,
 };
diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
index 62c568ce..21f63ddf 100644
--- a/libdtrace/dt_prov_fbt.c
+++ b/libdtrace/dt_prov_fbt.c
@@ -411,15 +411,6 @@ static int kprobe_attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
 	return dt_tp_probe_attach(dtp, prp, bpf_fd);
 }
 
-static int kprobe_probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
-			     int *argcp, dt_argdesc_t **argvp)
-{
-	*argcp = 0;			/* no arguments by default */
-	*argvp = NULL;
-
-	return 0;
-}
-
 /*
  * Try to clean up system resources that may have been allocated for this
  * probe.
@@ -469,7 +460,6 @@ dt_provimpl_t	dt_fbt_kprobe = {
 	.load_prog	= &dt_bpf_prog_load,
 	.trampoline	= &kprobe_trampoline,
 	.attach		= &kprobe_attach,
-	.probe_info	= &kprobe_probe_info,
 	.detach		= &kprobe_detach,
 	.probe_destroy	= &dt_tp_probe_destroy,
 };
diff --git a/libdtrace/dt_prov_profile.c b/libdtrace/dt_prov_profile.c
index bc224348..e1369ca9 100644
--- a/libdtrace/dt_prov_profile.c
+++ b/libdtrace/dt_prov_profile.c
@@ -299,16 +299,6 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
 	return nattach > 0 ? 0 : -1;
 }
 
-static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
-		      int *argcp, dt_argdesc_t **argvp)
-{
-	/* profile-provider probe arguments are not typed */
-	*argcp = 0;
-	*argvp = NULL;
-
-	return 0;
-}
-
 static void detach(dtrace_hdl_t *dtp, const dt_probe_t *prp)
 {
 	profile_probe_t	*pp = prp->prv_data;
@@ -337,7 +327,6 @@ dt_provimpl_t	dt_profile = {
 	.load_prog	= &dt_bpf_prog_load,
 	.trampoline	= &trampoline,
 	.attach		= &attach,
-	.probe_info	= &probe_info,
 	.detach		= &detach,
 	.probe_destroy	= &probe_destroy,
 };
-- 
2.43.5

Re: [PATCH 07/22] Supply a default probe_info()

[Kris Van Hees]

Is this still needed after the fix we put in for dealing with argc == -1, i.e.
commit 0e3231a268 "ident: fix unsigned vs signed comparison" ?

On Thu, Aug 29, 2024 at 01:22:04AM -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        |  4 +++-
>  libdtrace/dt_prov_cpc.c     | 11 -----------
>  libdtrace/dt_prov_dtrace.c  | 10 ----------
>  libdtrace/dt_prov_fbt.c     | 10 ----------
>  libdtrace/dt_prov_profile.c | 11 -----------
>  5 files changed, 3 insertions(+), 43 deletions(-)
> 
> diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
> index 0b53121a..ab90d2ed 100644
> --- a/libdtrace/dt_probe.c
> +++ b/libdtrace/dt_probe.c
> @@ -892,8 +892,10 @@ dt_probe_args_info(dtrace_hdl_t *dtp, dt_probe_t *prp)
>  	/* Only retrieve probe argument information once per probe. */
>  	if (prp->argc != -1)
>  		return 0;
> -	if (!prp->prov->impl->probe_info)
> +	if (!prp->prov->impl->probe_info) {
> +		prp->argc = 0;
>  		return 0;
> +	}
>  	rc = prp->prov->impl->probe_info(dtp, prp, &argc, &argv);
>  	if (rc == -1)
>  		return rc;
> diff --git a/libdtrace/dt_prov_cpc.c b/libdtrace/dt_prov_cpc.c
> index 08689b35..8f33cf58 100644
> --- a/libdtrace/dt_prov_cpc.c
> +++ b/libdtrace/dt_prov_cpc.c
> @@ -451,16 +451,6 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
>  	return nattach > 0 ? 0 : -1;
>  }
>  
> -static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> -		      int *argcp, dt_argdesc_t **argvp)
> -{
> -	/* cpc-provider probe arguments are not typed */
> -	*argcp = 0;
> -	*argvp = NULL;
> -
> -	return 0;
> -}
> -
>  static void detach(dtrace_hdl_t *dtp, const dt_probe_t *prp)
>  {
>  	cpc_probe_t	*datap = prp->prv_data;
> @@ -504,7 +494,6 @@ dt_provimpl_t	dt_cpc = {
>  	.load_prog	= &dt_bpf_prog_load,
>  	.trampoline	= &trampoline,
>  	.attach		= &attach,
> -	.probe_info	= &probe_info,
>  	.detach		= &detach,
>  	.probe_destroy	= &probe_destroy,
>  	.destroy	= &destroy,
> diff --git a/libdtrace/dt_prov_dtrace.c b/libdtrace/dt_prov_dtrace.c
> index bf87cb05..a9deccee 100644
> --- a/libdtrace/dt_prov_dtrace.c
> +++ b/libdtrace/dt_prov_dtrace.c
> @@ -273,15 +273,6 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
>  	return dt_tp_probe_attach(dtp, prp, bpf_fd);
>  }
>  
> -static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> -		      int *argcp, dt_argdesc_t **argvp)
> -{
> -	*argcp = 0;			/* no arguments */
> -	*argvp = NULL;
> -
> -	return 0;
> -}
> -
>  /*
>   * Try to clean up system resources that may have been allocated for this
>   * probe.
> @@ -317,7 +308,6 @@ dt_provimpl_t	dt_dtrace = {
>  	.trampoline	= &trampoline,
>  	.load_prog	= &dt_bpf_prog_load,
>  	.attach		= &attach,
> -	.probe_info	= &probe_info,
>  	.detach		= &detach,
>  	.probe_destroy	= &dt_tp_probe_destroy,
>  };
> diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
> index 62c568ce..21f63ddf 100644
> --- a/libdtrace/dt_prov_fbt.c
> +++ b/libdtrace/dt_prov_fbt.c
> @@ -411,15 +411,6 @@ static int kprobe_attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
>  	return dt_tp_probe_attach(dtp, prp, bpf_fd);
>  }
>  
> -static int kprobe_probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> -			     int *argcp, dt_argdesc_t **argvp)
> -{
> -	*argcp = 0;			/* no arguments by default */
> -	*argvp = NULL;
> -
> -	return 0;
> -}
> -
>  /*
>   * Try to clean up system resources that may have been allocated for this
>   * probe.
> @@ -469,7 +460,6 @@ dt_provimpl_t	dt_fbt_kprobe = {
>  	.load_prog	= &dt_bpf_prog_load,
>  	.trampoline	= &kprobe_trampoline,
>  	.attach		= &kprobe_attach,
> -	.probe_info	= &kprobe_probe_info,
>  	.detach		= &kprobe_detach,
>  	.probe_destroy	= &dt_tp_probe_destroy,
>  };
> diff --git a/libdtrace/dt_prov_profile.c b/libdtrace/dt_prov_profile.c
> index bc224348..e1369ca9 100644
> --- a/libdtrace/dt_prov_profile.c
> +++ b/libdtrace/dt_prov_profile.c
> @@ -299,16 +299,6 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
>  	return nattach > 0 ? 0 : -1;
>  }
>  
> -static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> -		      int *argcp, dt_argdesc_t **argvp)
> -{
> -	/* profile-provider probe arguments are not typed */
> -	*argcp = 0;
> -	*argvp = NULL;
> -
> -	return 0;
> -}
> -
>  static void detach(dtrace_hdl_t *dtp, const dt_probe_t *prp)
>  {
>  	profile_probe_t	*pp = prp->prv_data;
> @@ -337,7 +327,6 @@ dt_provimpl_t	dt_profile = {
>  	.load_prog	= &dt_bpf_prog_load,
>  	.trampoline	= &trampoline,
>  	.attach		= &attach,
> -	.probe_info	= &probe_info,
>  	.detach		= &detach,
>  	.probe_destroy	= &probe_destroy,
>  };
> -- 
> 2.43.5
> 

Re: [PATCH 07/22] Supply a default probe_info()

[Kris Van Hees]

On Fri, Sep 13, 2024 at 08:31:16PM -0400, Kris Van Hees wrote:
> Is this still needed after the fix we put in for dealing with argc == -1, i.e.
> commit 0e3231a268 "ident: fix unsigned vs signed comparison" ?

Ah no, you posted one that is based on the ident fix.

Comments below...

> On Thu, Aug 29, 2024 at 01:22:04AM -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        |  4 +++-
> >  libdtrace/dt_prov_cpc.c     | 11 -----------
> >  libdtrace/dt_prov_dtrace.c  | 10 ----------
> >  libdtrace/dt_prov_fbt.c     | 10 ----------
> >  libdtrace/dt_prov_profile.c | 11 -----------
> >  5 files changed, 3 insertions(+), 43 deletions(-)
> > 
> > diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
> > index 0b53121a..ab90d2ed 100644
> > --- a/libdtrace/dt_probe.c
> > +++ b/libdtrace/dt_probe.c
> > @@ -892,8 +892,10 @@ dt_probe_args_info(dtrace_hdl_t *dtp, dt_probe_t *prp)
> >  	/* Only retrieve probe argument information once per probe. */
> >  	if (prp->argc != -1)
> >  		return 0;
> > -	if (!prp->prov->impl->probe_info)
> > +	if (!prp->prov->impl->probe_info) {
> > +		prp->argc = 0;
> >  		return 0;
> > +	}
> >  	rc = prp->prov->impl->probe_info(dtp, prp, &argc, &argv);
> >  	if (rc == -1)
> >  		return rc;

I think that it might be cleaner to do:

	if (prp->prov->impl->probe_info &&
	    prp->prov->impl->probe_info(dtp, prp, &argc, &argv) == -1)
		return -1;

because the code that follows will look at argc (initialized as 0) and argv
(initialized as NULL), so if there is no probe_info hook, those initial
values will result in prp->argc being set to 0.

> > diff --git a/libdtrace/dt_prov_cpc.c b/libdtrace/dt_prov_cpc.c
> > index 08689b35..8f33cf58 100644
> > --- a/libdtrace/dt_prov_cpc.c
> > +++ b/libdtrace/dt_prov_cpc.c
> > @@ -451,16 +451,6 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
> >  	return nattach > 0 ? 0 : -1;
> >  }
> >  
> > -static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> > -		      int *argcp, dt_argdesc_t **argvp)
> > -{
> > -	/* cpc-provider probe arguments are not typed */
> > -	*argcp = 0;
> > -	*argvp = NULL;
> > -
> > -	return 0;
> > -}
> > -
> >  static void detach(dtrace_hdl_t *dtp, const dt_probe_t *prp)
> >  {
> >  	cpc_probe_t	*datap = prp->prv_data;
> > @@ -504,7 +494,6 @@ dt_provimpl_t	dt_cpc = {
> >  	.load_prog	= &dt_bpf_prog_load,
> >  	.trampoline	= &trampoline,
> >  	.attach		= &attach,
> > -	.probe_info	= &probe_info,
> >  	.detach		= &detach,
> >  	.probe_destroy	= &probe_destroy,
> >  	.destroy	= &destroy,
> > diff --git a/libdtrace/dt_prov_dtrace.c b/libdtrace/dt_prov_dtrace.c
> > index bf87cb05..a9deccee 100644
> > --- a/libdtrace/dt_prov_dtrace.c
> > +++ b/libdtrace/dt_prov_dtrace.c
> > @@ -273,15 +273,6 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
> >  	return dt_tp_probe_attach(dtp, prp, bpf_fd);
> >  }
> >  
> > -static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> > -		      int *argcp, dt_argdesc_t **argvp)
> > -{
> > -	*argcp = 0;			/* no arguments */
> > -	*argvp = NULL;
> > -
> > -	return 0;
> > -}
> > -
> >  /*
> >   * Try to clean up system resources that may have been allocated for this
> >   * probe.
> > @@ -317,7 +308,6 @@ dt_provimpl_t	dt_dtrace = {
> >  	.trampoline	= &trampoline,
> >  	.load_prog	= &dt_bpf_prog_load,
> >  	.attach		= &attach,
> > -	.probe_info	= &probe_info,
> >  	.detach		= &detach,
> >  	.probe_destroy	= &dt_tp_probe_destroy,
> >  };
> > diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
> > index 62c568ce..21f63ddf 100644
> > --- a/libdtrace/dt_prov_fbt.c
> > +++ b/libdtrace/dt_prov_fbt.c
> > @@ -411,15 +411,6 @@ static int kprobe_attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
> >  	return dt_tp_probe_attach(dtp, prp, bpf_fd);
> >  }
> >  
> > -static int kprobe_probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> > -			     int *argcp, dt_argdesc_t **argvp)
> > -{
> > -	*argcp = 0;			/* no arguments by default */
> > -	*argvp = NULL;
> > -
> > -	return 0;
> > -}
> > -
> >  /*
> >   * Try to clean up system resources that may have been allocated for this
> >   * probe.
> > @@ -469,7 +460,6 @@ dt_provimpl_t	dt_fbt_kprobe = {
> >  	.load_prog	= &dt_bpf_prog_load,
> >  	.trampoline	= &kprobe_trampoline,
> >  	.attach		= &kprobe_attach,
> > -	.probe_info	= &kprobe_probe_info,
> >  	.detach		= &kprobe_detach,
> >  	.probe_destroy	= &dt_tp_probe_destroy,
> >  };
> > diff --git a/libdtrace/dt_prov_profile.c b/libdtrace/dt_prov_profile.c
> > index bc224348..e1369ca9 100644
> > --- a/libdtrace/dt_prov_profile.c
> > +++ b/libdtrace/dt_prov_profile.c
> > @@ -299,16 +299,6 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
> >  	return nattach > 0 ? 0 : -1;
> >  }
> >  
> > -static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> > -		      int *argcp, dt_argdesc_t **argvp)
> > -{
> > -	/* profile-provider probe arguments are not typed */
> > -	*argcp = 0;
> > -	*argvp = NULL;
> > -
> > -	return 0;
> > -}
> > -
> >  static void detach(dtrace_hdl_t *dtp, const dt_probe_t *prp)
> >  {
> >  	profile_probe_t	*pp = prp->prv_data;
> > @@ -337,7 +327,6 @@ dt_provimpl_t	dt_profile = {
> >  	.load_prog	= &dt_bpf_prog_load,
> >  	.trampoline	= &trampoline,
> >  	.attach		= &attach,
> > -	.probe_info	= &probe_info,
> >  	.detach		= &detach,
> >  	.probe_destroy	= &probe_destroy,
> >  };
> > -- 
> > 2.43.5
> > 

[PATCH 08/22] dtprobed: Fix comment typo

[eugene.loh]

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

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

diff --git a/dtprobed/dof_stash.c b/dtprobed/dof_stash.c
index 625572d5..0e639076 100644
--- a/dtprobed/dof_stash.c
+++ b/dtprobed/dof_stash.c
@@ -64,7 +64,7 @@
  *
  * /run/dtrace/probes/: Per-probe info, written by dtprobed, read by DTrace.
  *
- *    .../$pid/$prv$pid/$mod/$fun/$prb: Hardlink from $prv$pid:$mod:$fun:$prb
+ *    .../$pid/$prv$pid/$mod/$fun/$prb: Hardlink from $prv:$mod:$fun:$prb
  *    above; parsed representation of one probe in a given process. Removed by
  *    dtprobed when the process dies, or if all mappings containing the probe
  *    are unmmapped.  Used by DTrace for tracing by PID.
-- 
2.43.5

Re: [PATCH 08/22] dtprobed: Fix comment typo

[Kris Van Hees]

On Thu, Aug 29, 2024 at 01:22:05AM -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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/dtprobed/dof_stash.c b/dtprobed/dof_stash.c
> index 625572d5..0e639076 100644
> --- a/dtprobed/dof_stash.c
> +++ b/dtprobed/dof_stash.c
> @@ -64,7 +64,7 @@
>   *
>   * /run/dtrace/probes/: Per-probe info, written by dtprobed, read by DTrace.
>   *
> - *    .../$pid/$prv$pid/$mod/$fun/$prb: Hardlink from $prv$pid:$mod:$fun:$prb
> + *    .../$pid/$prv$pid/$mod/$fun/$prb: Hardlink from $prv:$mod:$fun:$prb
>   *    above; parsed representation of one probe in a given process. Removed by
>   *    dtprobed when the process dies, or if all mappings containing the probe
>   *    are unmmapped.  Used by DTrace for tracing by PID.
> -- 
> 2.43.5
> 

[PATCH 09/22] Clean up dtsd_* members

[eugene.loh]

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

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

diff --git a/libdtrace/dtrace.h b/libdtrace/dtrace.h
index 8f40a581..09a87977 100644
--- a/libdtrace/dtrace.h
+++ b/libdtrace/dtrace.h
@@ -146,10 +146,7 @@ extern void *dtrace_geterr_dof(dtrace_hdl_t *dtp);
 typedef struct dtrace_stmtdesc {
 	dtrace_ecbdesc_t *dtsd_ecbdesc;		/* ECB description */
 	struct dt_ident *dtsd_clause;		/* clause identifier */
-	void *dtsd_aggdata;			/* aggregation data */
-	void *dtsd_fmtdata;			/* type-specific output data */
-	void (*dtsd_callback)();		/* callback function for EPID */
-	void *dtsd_data;			/* callback data pointer */
+	void *dtsd_fmtdata;			/* type-specific output data */    /* FIXME: dead code */
 	dtrace_attribute_t dtsd_descattr;	/* probedesc attributes */
 	dtrace_attribute_t dtsd_stmtattr;	/* statement attributes */
 	int dtsd_clauseflags;			/* clause flags */
-- 
2.43.5

Re: [PATCH 09/22] Clean up dtsd_* members

[Kris Van Hees]

On Thu, Aug 29, 2024 at 01:22:06AM -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/dtrace.h | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/libdtrace/dtrace.h b/libdtrace/dtrace.h
> index 8f40a581..09a87977 100644
> --- a/libdtrace/dtrace.h
> +++ b/libdtrace/dtrace.h
> @@ -146,10 +146,7 @@ extern void *dtrace_geterr_dof(dtrace_hdl_t *dtp);
>  typedef struct dtrace_stmtdesc {
>  	dtrace_ecbdesc_t *dtsd_ecbdesc;		/* ECB description */
>  	struct dt_ident *dtsd_clause;		/* clause identifier */
> -	void *dtsd_aggdata;			/* aggregation data */
> -	void *dtsd_fmtdata;			/* type-specific output data */
> -	void (*dtsd_callback)();		/* callback function for EPID */
> -	void *dtsd_data;			/* callback data pointer */
> +	void *dtsd_fmtdata;			/* type-specific output data */    /* FIXME: dead code */
>  	dtrace_attribute_t dtsd_descattr;	/* probedesc attributes */
>  	dtrace_attribute_t dtsd_stmtattr;	/* statement attributes */
>  	int dtsd_clauseflags;			/* clause flags */
> -- 
> 2.43.5
> 

[PATCH 10/22] Simplify dtrace_stmt_create() attr init

[eugene.loh]

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

Even though dtrace_stmt_create() initializes dtsd_descattr and
dtsd_stmtattr, there is no point to doing so.  Its only caller
is dt_stmt_create(), which itself sets these members.

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

diff --git a/libdtrace/dt_program.c b/libdtrace/dt_program.c
index a4b052fc..bdb434e0 100644
--- a/libdtrace/dt_program.c
+++ b/libdtrace/dt_program.c
@@ -240,8 +240,6 @@ dtrace_stmt_create(dtrace_hdl_t *dtp, dtrace_ecbdesc_t *edp)
 
 	dt_ecbdesc_hold(edp);
 	sdp->dtsd_ecbdesc = edp;
-	sdp->dtsd_descattr = _dtrace_defattr;
-	sdp->dtsd_stmtattr = _dtrace_defattr;
 
 	return sdp;
 }
-- 
2.43.5

Re: [PATCH 10/22] Simplify dtrace_stmt_create() attr init

[Kris Van Hees]

On Thu, Aug 29, 2024 at 01:22:07AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> Even though dtrace_stmt_create() initializes dtsd_descattr and
> dtsd_stmtattr, there is no point to doing so.  Its only caller
> is dt_stmt_create(), which itself sets these members.

No, it is the other way around...  It should be done in dtrace_stmt_create()
and thus it is no longer needed in dt_stmt_create().  As dtrace_stmt_create()
is a libdtrace API function, it can be called from other code, and since it
is the function that actual creates the statement and initializes some of its
members, it is the logical place to retain setting the attr fields.

So, instead remove the assignments from dt_stmt_create().

> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> ---
>  libdtrace/dt_program.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/libdtrace/dt_program.c b/libdtrace/dt_program.c
> index a4b052fc..bdb434e0 100644
> --- a/libdtrace/dt_program.c
> +++ b/libdtrace/dt_program.c
> @@ -240,8 +240,6 @@ dtrace_stmt_create(dtrace_hdl_t *dtp, dtrace_ecbdesc_t *edp)
>  
>  	dt_ecbdesc_hold(edp);
>  	sdp->dtsd_ecbdesc = edp;
> -	sdp->dtsd_descattr = _dtrace_defattr;
> -	sdp->dtsd_stmtattr = _dtrace_defattr;
>  
>  	return sdp;
>  }
> -- 
> 2.43.5
> 

Re: [DTrace-devel] [PATCH 10/22] Simplify dtrace_stmt_create() attr init

[Kris Van Hees]

I adjusted the patch, and changed the commit message to be:

    Simplify dt_stmt_create() attr init
    
    Even though dt_stmt_create() initializes dtsd_descattr and dtsd_stmtattr,
    there is no point to doing so.  It calls dtrace_stmt_create(), which also
    sets these members.

and added my R-b.

On Sat, Sep 14, 2024 at 12:25:57PM -0400, Kris Van Hees via DTrace-devel wrote:
> On Thu, Aug 29, 2024 at 01:22:07AM -0400, eugene.loh@oracle.com wrote:
> > From: Eugene Loh <eugene.loh@oracle.com>
> > 
> > Even though dtrace_stmt_create() initializes dtsd_descattr and
> > dtsd_stmtattr, there is no point to doing so.  Its only caller
> > is dt_stmt_create(), which itself sets these members.
> 
> No, it is the other way around...  It should be done in dtrace_stmt_create()
> and thus it is no longer needed in dt_stmt_create().  As dtrace_stmt_create()
> is a libdtrace API function, it can be called from other code, and since it
> is the function that actual creates the statement and initializes some of its
> members, it is the logical place to retain setting the attr fields.
> 
> So, instead remove the assignments from dt_stmt_create().
> 
> > 
> > Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> > ---
> >  libdtrace/dt_program.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/libdtrace/dt_program.c b/libdtrace/dt_program.c
> > index a4b052fc..bdb434e0 100644
> > --- a/libdtrace/dt_program.c
> > +++ b/libdtrace/dt_program.c
> > @@ -240,8 +240,6 @@ dtrace_stmt_create(dtrace_hdl_t *dtp, dtrace_ecbdesc_t *edp)
> >  
> >  	dt_ecbdesc_hold(edp);
> >  	sdp->dtsd_ecbdesc = edp;
> > -	sdp->dtsd_descattr = _dtrace_defattr;
> > -	sdp->dtsd_stmtattr = _dtrace_defattr;
> >  
> >  	return sdp;
> >  }
> > -- 
> > 2.43.5
> > 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel

Re: [DTrace-devel] [PATCH 10/22] Simplify dtrace_stmt_create() attr init

[Eugene Loh]

On 9/14/24 12:32, Kris Van Hees wrote:

> I adjusted the patch, and changed the commit message to be:
>
>      Simplify dt_stmt_create() attr init
>      
>      Even though dt_stmt_create() initializes dtsd_descattr and dtsd_stmtattr,
>      there is no point to doing so.  It calls dtrace_stmt_create(), which also
>      sets these members.
>
> and added my R-b.

That doesn't seem to work.  In dtrace_stmt_create(), we have:
         sdp->dtsd_descattr = _dtrace_defattr;
         sdp->dtsd_stmtattr = _dtrace_defattr;

In dt_stmt_create(), we have:
         dtrace_stmtdesc_t *sdp = dtrace_stmt_create(dtp, edp);
         sdp->dtsd_descattr = descattr;
         sdp->dtsd_stmtattr = stmtattr;
which means that whatever dtrace_stmt_create() does, we overwrite it 
with the attributes passed in by dt_stmt_create()'s caller.  So, what 
dtrace_stmt_create() does is irrelevant and what dt_stmt_create()'s 
caller asks for matters.  So removing the assignments from 
dtrace_stmt_create() makes sense (except for the API issue) and removing 
the assignments from dt_stmt_create() is wrong.

Specifically, removing the attr assignments in dt_stmt_create() causes 
test/unittest/dtrace-util/tst.VerboseStabilityReport.d to fail.

Either we should go with the original version of the patch or, more 
likely, to preserve the API, the patch should be withdrawn.

> On Sat, Sep 14, 2024 at 12:25:57PM -0400, Kris Van Hees via DTrace-devel wrote:
>> On Thu, Aug 29, 2024 at 01:22:07AM -0400, eugene.loh@oracle.com wrote:
>>> From: Eugene Loh <eugene.loh@oracle.com>
>>>
>>> Even though dtrace_stmt_create() initializes dtsd_descattr and
>>> dtsd_stmtattr, there is no point to doing so.  Its only caller
>>> is dt_stmt_create(), which itself sets these members.
>> No, it is the other way around...  It should be done in dtrace_stmt_create()
>> and thus it is no longer needed in dt_stmt_create().  As dtrace_stmt_create()
>> is a libdtrace API function, it can be called from other code, and since it
>> is the function that actual creates the statement and initializes some of its
>> members, it is the logical place to retain setting the attr fields.
>>
>> So, instead remove the assignments from dt_stmt_create().
>>
>>> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
>>> ---
>>>   libdtrace/dt_program.c | 2 --
>>>   1 file changed, 2 deletions(-)
>>>
>>> diff --git a/libdtrace/dt_program.c b/libdtrace/dt_program.c
>>> index a4b052fc..bdb434e0 100644
>>> --- a/libdtrace/dt_program.c
>>> +++ b/libdtrace/dt_program.c
>>> @@ -240,8 +240,6 @@ dtrace_stmt_create(dtrace_hdl_t *dtp, dtrace_ecbdesc_t *edp)
>>>   
>>>   	dt_ecbdesc_hold(edp);
>>>   	sdp->dtsd_ecbdesc = edp;
>>> -	sdp->dtsd_descattr = _dtrace_defattr;
>>> -	sdp->dtsd_stmtattr = _dtrace_defattr;
>>>   
>>>   	return sdp;
>>>   }
>>> -- 
>>> 2.43.5
>>>
>> _______________________________________________
>> DTrace-devel mailing list
>> DTrace-devel@oss.oracle.com
>> https://oss.oracle.com/mailman/listinfo/dtrace-devel

Re: [DTrace-devel] [PATCH 10/22] Simplify dtrace_stmt_create() attr init

[Kris Van Hees]

Ah yes, good point.  I should have realized that.  Yes, then we should just drop the patch (for now).  Thanks for catching my mistake.

I'll have to do a revert since I already pushed to dev....  My bad - thanks for pointing it out.

________________________________________
From: Eugene Loh <eugene.loh@oracle.com>
Sent: Thursday, September 19, 2024 1:38 PM
To: dtrace@lists.linux.dev; dtrace-devel@oss.oracle.com
Subject: Re: [DTrace-devel] [PATCH 10/22] Simplify dtrace_stmt_create() attr init

On 9/14/24 12:32, Kris Van Hees wrote:

> I adjusted the patch, and changed the commit message to be:
>
>      Simplify dt_stmt_create() attr init
>
>      Even though dt_stmt_create() initializes dtsd_descattr and dtsd_stmtattr,
>      there is no point to doing so.  It calls dtrace_stmt_create(), which also
>      sets these members.
>
> and added my R-b.

That doesn't seem to work.  In dtrace_stmt_create(), we have:
         sdp->dtsd_descattr = _dtrace_defattr;
         sdp->dtsd_stmtattr = _dtrace_defattr;

In dt_stmt_create(), we have:
         dtrace_stmtdesc_t *sdp = dtrace_stmt_create(dtp, edp);
         sdp->dtsd_descattr = descattr;
         sdp->dtsd_stmtattr = stmtattr;
which means that whatever dtrace_stmt_create() does, we overwrite it
with the attributes passed in by dt_stmt_create()'s caller.  So, what
dtrace_stmt_create() does is irrelevant and what dt_stmt_create()'s
caller asks for matters.  So removing the assignments from
dtrace_stmt_create() makes sense (except for the API issue) and removing
the assignments from dt_stmt_create() is wrong.

Specifically, removing the attr assignments in dt_stmt_create() causes
test/unittest/dtrace-util/tst.VerboseStabilityReport.d to fail.

Either we should go with the original version of the patch or, more
likely, to preserve the API, the patch should be withdrawn.

> On Sat, Sep 14, 2024 at 12:25:57PM -0400, Kris Van Hees via DTrace-devel wrote:
>> On Thu, Aug 29, 2024 at 01:22:07AM -0400, eugene.loh@oracle.com wrote:
>>> From: Eugene Loh <eugene.loh@oracle.com>
>>>
>>> Even though dtrace_stmt_create() initializes dtsd_descattr and
>>> dtsd_stmtattr, there is no point to doing so.  Its only caller
>>> is dt_stmt_create(), which itself sets these members.
>> No, it is the other way around...  It should be done in dtrace_stmt_create()
>> and thus it is no longer needed in dt_stmt_create().  As dtrace_stmt_create()
>> is a libdtrace API function, it can be called from other code, and since it
>> is the function that actual creates the statement and initializes some of its
>> members, it is the logical place to retain setting the attr fields.
>>
>> So, instead remove the assignments from dt_stmt_create().
>>
>>> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
>>> ---
>>>   libdtrace/dt_program.c | 2 --
>>>   1 file changed, 2 deletions(-)
>>>
>>> diff --git a/libdtrace/dt_program.c b/libdtrace/dt_program.c
>>> index a4b052fc..bdb434e0 100644
>>> --- a/libdtrace/dt_program.c
>>> +++ b/libdtrace/dt_program.c
>>> @@ -240,8 +240,6 @@ dtrace_stmt_create(dtrace_hdl_t *dtp, dtrace_ecbdesc_t *edp)
>>>
>>>     dt_ecbdesc_hold(edp);
>>>     sdp->dtsd_ecbdesc = edp;
>>> -   sdp->dtsd_descattr = _dtrace_defattr;
>>> -   sdp->dtsd_stmtattr = _dtrace_defattr;
>>>
>>>     return sdp;
>>>   }
>>> --
>>> 2.43.5
>>>
>> _______________________________________________
>> DTrace-devel mailing list
>> DTrace-devel@oss.oracle.com
>> https://oss.oracle.com/mailman/listinfo/dtrace-devel

[PATCH 11/22] DTPPT_POST_OFFSETS is unused

[eugene.loh]

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

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

diff --git a/include/dtrace/pid.h b/include/dtrace/pid.h
index 1fd594b6..0129674a 100644
--- a/include/dtrace/pid.h
+++ b/include/dtrace/pid.h
@@ -22,7 +22,6 @@ typedef enum pid_probetype {
 	DTPPT_ENTRY,
 	DTPPT_RETURN,
 	DTPPT_OFFSETS,
-	DTPPT_POST_OFFSETS,
 	DTPPT_IS_ENABLED
 } pid_probetype_t;
 
-- 
2.43.5

Re: [PATCH 11/22] DTPPT_POST_OFFSETS is unused

[Kris Van Hees]

On Thu, Aug 29, 2024 at 01:22:08AM -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>

> ---
>  include/dtrace/pid.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/include/dtrace/pid.h b/include/dtrace/pid.h
> index 1fd594b6..0129674a 100644
> --- a/include/dtrace/pid.h
> +++ b/include/dtrace/pid.h
> @@ -22,7 +22,6 @@ typedef enum pid_probetype {
>  	DTPPT_ENTRY,
>  	DTPPT_RETURN,
>  	DTPPT_OFFSETS,
> -	DTPPT_POST_OFFSETS,
>  	DTPPT_IS_ENABLED
>  } pid_probetype_t;
>  
> -- 
> 2.43.5
> 

[PATCH 12/22] Remove apparently redundant assignment

[eugene.loh]

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

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

diff --git a/libdtrace/dt_aggregate.c b/libdtrace/dt_aggregate.c
index 8825739c..d87d49e8 100644
--- a/libdtrace/dt_aggregate.c
+++ b/libdtrace/dt_aggregate.c
@@ -621,7 +621,6 @@ dt_aggregate_snap_one(dtrace_hdl_t *dtp, int aggid, int cpu, const char *key,
 		}
 
 		/* Skip if data gen is older than hash gen.  */
-		hgen = *(int64_t *)agd->dtada_data;
 		if (dgen < hgen)
 			return 0;
 
-- 
2.43.5

Re: [PATCH 12/22] Remove apparently redundant assignment

[Kris Van Hees]

On Thu, Aug 29, 2024 at 01:22:09AM -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_aggregate.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/libdtrace/dt_aggregate.c b/libdtrace/dt_aggregate.c
> index 8825739c..d87d49e8 100644
> --- a/libdtrace/dt_aggregate.c
> +++ b/libdtrace/dt_aggregate.c
> @@ -621,7 +621,6 @@ dt_aggregate_snap_one(dtrace_hdl_t *dtp, int aggid, int cpu, const char *key,
>  		}
>  
>  		/* Skip if data gen is older than hash gen.  */
> -		hgen = *(int64_t *)agd->dtada_data;
>  		if (dgen < hgen)
>  			return 0;
>  
> -- 
> 2.43.5
> 

[PATCH 13/22] Eliminate unused args to dt_spec_buf_add_data()

[eugene.loh]

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

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

diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
index 6aa9812e..51eb6c80 100644
--- a/libdtrace/dt_consume.c
+++ b/libdtrace/dt_consume.c
@@ -2060,9 +2060,7 @@ oom:
 
 static dt_spec_buf_data_t *
 dt_spec_buf_add_data(dtrace_hdl_t *dtp, dt_spec_buf_t *dtsb,
-		     dtrace_epid_t epid, unsigned int cpu,
-		     dtrace_datadesc_t *datadesc, char *data,
-		     uint32_t size)
+		     unsigned int cpu, char *data, uint32_t size)
 {
 	dt_spec_buf_data_t *dsbd;
 
@@ -2265,8 +2263,7 @@ dt_consume_one_probe(dtrace_hdl_t *dtp, FILE *fp, char *data, uint32_t size,
 			}
 		}
 
-		if (dt_spec_buf_add_data(dtp, dtsb, epid, pdat->dtpda_cpu, epd,
-					 data, size) == NULL)
+		if (dt_spec_buf_add_data(dtp, dtsb, pdat->dtpda_cpu, data, size) == NULL)
 			dtp->dt_specdrops++;
 
 		return DTRACE_WORKSTATUS_OKAY;
-- 
2.43.5

Re: [PATCH 13/22] Eliminate unused args to dt_spec_buf_add_data()

[Kris Van Hees]

On Thu, Aug 29, 2024 at 01:22:10AM -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 | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
> index 6aa9812e..51eb6c80 100644
> --- a/libdtrace/dt_consume.c
> +++ b/libdtrace/dt_consume.c
> @@ -2060,9 +2060,7 @@ oom:
>  
>  static dt_spec_buf_data_t *
>  dt_spec_buf_add_data(dtrace_hdl_t *dtp, dt_spec_buf_t *dtsb,
> -		     dtrace_epid_t epid, unsigned int cpu,
> -		     dtrace_datadesc_t *datadesc, char *data,
> -		     uint32_t size)
> +		     unsigned int cpu, char *data, uint32_t size)
>  {
>  	dt_spec_buf_data_t *dsbd;
>  
> @@ -2265,8 +2263,7 @@ dt_consume_one_probe(dtrace_hdl_t *dtp, FILE *fp, char *data, uint32_t size,
>  			}
>  		}
>  
> -		if (dt_spec_buf_add_data(dtp, dtsb, epid, pdat->dtpda_cpu, epd,
> -					 data, size) == NULL)
> +		if (dt_spec_buf_add_data(dtp, dtsb, pdat->dtpda_cpu, data, size) == NULL)
>  			dtp->dt_specdrops++;
>  
>  		return DTRACE_WORKSTATUS_OKAY;
> -- 
> 2.43.5
> 

[PATCH 14/22] Both dted_uarg and dofe_uarg are unused

[eugene.loh]

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

I confirmed in a run of the test suite that they are unused.

But I do not understand how any of the dof_ecbdesc_t fields are
used: dofe_probes, dofe_actions, or dofe_pad.

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 include/dtrace/dof.h      | 1 -
 include/dtrace/enabling.h | 1 -
 libdtrace/dt_dof.c        | 1 -
 3 files changed, 3 deletions(-)

diff --git a/include/dtrace/dof.h b/include/dtrace/dof.h
index f5655fe2..f9b66e10 100644
--- a/include/dtrace/dof.h
+++ b/include/dtrace/dof.h
@@ -94,7 +94,6 @@ typedef struct dof_ecbdesc {
 	dof_secidx_t dofe_probes;	/* link to DOF_SECT_PROBEDESC */
 	dof_secidx_t dofe_actions;	/* link to DOF_SECT_ACTDESC */
 	uint32_t dofe_pad;		/* reserved for future use */
-	uint64_t dofe_uarg;		/* user-supplied library argument */
 } dof_ecbdesc_t;
 
 typedef struct dof_probedesc {
diff --git a/include/dtrace/enabling.h b/include/dtrace/enabling.h
index f1ec444c..55f67c03 100644
--- a/include/dtrace/enabling.h
+++ b/include/dtrace/enabling.h
@@ -53,7 +53,6 @@ typedef struct dtrace_actdesc {
 
 typedef struct dtrace_ecbdesc {
 	dtrace_probedesc_t dted_probe;		/* probe description */
-	uint64_t dted_uarg;			/* library argument */
 	int dted_refcnt;			/* reference count */
 } dtrace_ecbdesc_t;
 
diff --git a/libdtrace/dt_dof.c b/libdtrace/dt_dof.c
index be29f045..c89ad830 100644
--- a/libdtrace/dt_dof.c
+++ b/libdtrace/dt_dof.c
@@ -738,7 +738,6 @@ dtrace_dof_create(dtrace_hdl_t *dtp, dtrace_prog_t *pgp, uint_t flags)
 		dofe.dofe_probes = probesec;
 		dofe.dofe_actions = actsec;
 		dofe.dofe_pad = 0;
-		dofe.dofe_uarg = edp->dted_uarg;
 
 		dof_add_lsect(ddo, &dofe, DOF_SECT_ECBDESC,
 		    sizeof(uint64_t), 0, 0, sizeof(dof_ecbdesc_t));
-- 
2.43.5

Re: [PATCH 14/22] Both dted_uarg and dofe_uarg are unused

[Kris Van Hees]

On Thu, Aug 29, 2024 at 01:22:11AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> I confirmed in a run of the test suite that they are unused.
> 
> But I do not understand how any of the dof_ecbdesc_t fields are
> used: dofe_probes, dofe_actions, or dofe_pad.
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

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

> ---
>  include/dtrace/dof.h      | 1 -
>  include/dtrace/enabling.h | 1 -
>  libdtrace/dt_dof.c        | 1 -
>  3 files changed, 3 deletions(-)
> 
> diff --git a/include/dtrace/dof.h b/include/dtrace/dof.h
> index f5655fe2..f9b66e10 100644
> --- a/include/dtrace/dof.h
> +++ b/include/dtrace/dof.h
> @@ -94,7 +94,6 @@ typedef struct dof_ecbdesc {
>  	dof_secidx_t dofe_probes;	/* link to DOF_SECT_PROBEDESC */
>  	dof_secidx_t dofe_actions;	/* link to DOF_SECT_ACTDESC */
>  	uint32_t dofe_pad;		/* reserved for future use */
> -	uint64_t dofe_uarg;		/* user-supplied library argument */
>  } dof_ecbdesc_t;
>  
>  typedef struct dof_probedesc {
> diff --git a/include/dtrace/enabling.h b/include/dtrace/enabling.h
> index f1ec444c..55f67c03 100644
> --- a/include/dtrace/enabling.h
> +++ b/include/dtrace/enabling.h
> @@ -53,7 +53,6 @@ typedef struct dtrace_actdesc {
>  
>  typedef struct dtrace_ecbdesc {
>  	dtrace_probedesc_t dted_probe;		/* probe description */
> -	uint64_t dted_uarg;			/* library argument */
>  	int dted_refcnt;			/* reference count */
>  } dtrace_ecbdesc_t;
>  
> diff --git a/libdtrace/dt_dof.c b/libdtrace/dt_dof.c
> index be29f045..c89ad830 100644
> --- a/libdtrace/dt_dof.c
> +++ b/libdtrace/dt_dof.c
> @@ -738,7 +738,6 @@ dtrace_dof_create(dtrace_hdl_t *dtp, dtrace_prog_t *pgp, uint_t flags)
>  		dofe.dofe_probes = probesec;
>  		dofe.dofe_actions = actsec;
>  		dofe.dofe_pad = 0;
> -		dofe.dofe_uarg = edp->dted_uarg;
>  
>  		dof_add_lsect(ddo, &dofe, DOF_SECT_ECBDESC,
>  		    sizeof(uint64_t), 0, 0, sizeof(dof_ecbdesc_t));
> -- 
> 2.43.5
> 

[PATCH 15/22] test: Clean up the specsize tests

[eugene.loh]

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

The tests had actions like
    printf("%lld: Lots of data\n", x);
    printf("%lld: Has to be crammed into this buffer\n", x);
    printf("%lld: Until it overflows\n", x);
    printf("%lld: And causes flops\n", x);
suggesting that these strings were crowding the buffer, but these
strings are not passed from producer to consumer at all.

The tests also only tested one clause per speculation.  It would be
nice also to test multiple clauses per speculation.

There is much replicated code from one of the tests to the other, a
shortcoming that is amplified if we want to test more specsize values,
which is the case when we test multiple clauses per speculation.

Therefore, replace the multiple tests with a single test that checks
multiple clauses per speculation and more values of specsize.

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 .../speculation/tst.SpecSizeVariations.r      | 68 +++++++++++++++++
 .../speculation/tst.SpecSizeVariations.sh     | 74 +++++++++++++++++++
 .../speculation/tst.SpecSizeVariations4.d     | 66 -----------------
 .../speculation/tst.SpecSizeVariations4.r     |  5 --
 .../speculation/tst.SpecSizeVariations5.d     | 61 ---------------
 .../speculation/tst.SpecSizeVariations5.r     |  7 --
 6 files changed, 142 insertions(+), 139 deletions(-)
 create mode 100644 test/unittest/speculation/tst.SpecSizeVariations.r
 create mode 100755 test/unittest/speculation/tst.SpecSizeVariations.sh
 delete mode 100644 test/unittest/speculation/tst.SpecSizeVariations4.d
 delete mode 100644 test/unittest/speculation/tst.SpecSizeVariations4.r
 delete mode 100644 test/unittest/speculation/tst.SpecSizeVariations5.d
 delete mode 100644 test/unittest/speculation/tst.SpecSizeVariations5.r

diff --git a/test/unittest/speculation/tst.SpecSizeVariations.r b/test/unittest/speculation/tst.SpecSizeVariations.r
new file mode 100644
index 00000000..51f0596c
--- /dev/null
+++ b/test/unittest/speculation/tst.SpecSizeVariations.r
@@ -0,0 +1,68 @@
+Speculative buffer ID: 1
+counts: 0 1
+
+Speculative buffer ID: 1
+123456700
+123456701
+123456702
+123456703
+123456704
+123456705
+123456706
+counts: 1 1
+
+Speculative buffer ID: 1
+123456700
+123456701
+123456702
+123456703
+123456704
+123456705
+123456706
+counts: 1 1
+
+Speculative buffer ID: 1
+123456700
+123456701
+123456702
+123456703
+123456704
+123456705
+123456706
+counts: 2 1
+
+Speculative buffer ID: 1
+123456700
+123456701
+123456702
+123456703
+123456704
+123456705
+123456706
+counts: 2 1
+
+Speculative buffer ID: 1
+123456700
+123456701
+123456702
+123456703
+123456704
+123456705
+123456706
+123456800
+123456801
+123456802
+123456803
+123456804
+123456805
+123456806
+123456807
+123456808
+counts: 2 1
+
+-- @@stderr --
+dtrace: 2 speculative drops
+dtrace: 1 speculative drop
+dtrace: 1 speculative drop
+dtrace: 1 speculative drop
+dtrace: 1 speculative drop
diff --git a/test/unittest/speculation/tst.SpecSizeVariations.sh b/test/unittest/speculation/tst.SpecSizeVariations.sh
new file mode 100755
index 00000000..75e527d9
--- /dev/null
+++ b/test/unittest/speculation/tst.SpecSizeVariations.sh
@@ -0,0 +1,74 @@
+#!/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.
+#
+
+dtrace=$1
+
+for x in 63 64 79 80 143 144; do
+	$dtrace $dt_flags -xspecsize=$x -qn '
+	BEGIN
+	{
+		x = 123456700ll;
+		self->nspeculate = 0;
+		self->ncommit = 0;
+		self->spec = speculation();
+		printf("Speculative buffer ID: %d\n", self->spec);
+	}
+
+	/* 16 + 7 * 8 = 72 bytes */
+	BEGIN
+	{
+		speculate(self->spec);
+		printf("%lld\n", x++);
+		printf("%lld\n", x++);
+		printf("%lld\n", x++);
+		printf("%lld\n", x++);
+		printf("%lld\n", x++);
+		printf("%lld\n", x++);
+		printf("%lld\n", x++);
+		self->nspeculate++;
+	}
+
+	BEGIN
+	{
+		x = 123456800ll;
+	}
+
+	/* 16 + 9 * 8 = 88 bytes */
+	BEGIN
+	{
+		speculate(self->spec);
+		printf("%lld\n", x++);
+		printf("%lld\n", x++);
+		printf("%lld\n", x++);
+		printf("%lld\n", x++);
+		printf("%lld\n", x++);
+		printf("%lld\n", x++);
+		printf("%lld\n", x++);
+		printf("%lld\n", x++);
+		printf("%lld\n", x++);
+		self->nspeculate++;
+	}
+
+	BEGIN
+	{
+		commit(self->spec);
+		self->ncommit++;
+	}
+
+	BEGIN
+	{
+		printf("counts: %d %d\n", self->nspeculate, self->ncommit);
+		exit(0);
+	}
+
+	ERROR
+	{
+		exit(1);
+	}'
+done
diff --git a/test/unittest/speculation/tst.SpecSizeVariations4.d b/test/unittest/speculation/tst.SpecSizeVariations4.d
deleted file mode 100644
index 4221c89e..00000000
--- a/test/unittest/speculation/tst.SpecSizeVariations4.d
+++ /dev/null
@@ -1,66 +0,0 @@
-/*
- * Oracle Linux DTrace.
- * Copyright (c) 2006, 2023, 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.
- */
-
-/*
- * ASSERTION:
- * Verify the behavior of speculations with changes in specsize.
- *
- * SECTION: Speculative Tracing/Options and Tuning;
- *	Options and Tunables/specsize
- *
- */
-
-#pragma D option quiet
-#pragma D option specsize=39
-
-long long x;
-
-BEGIN
-{
-	x = 123456789;
-	self->speculateFlag = 0;
-	self->commitFlag = 0;
-	self->spec = speculation();
-	printf("Speculative buffer ID: %d\n", self->spec);
-}
-
-BEGIN
-{
-	speculate(self->spec);
-	printf("%lld: Lots of data\n", x);
-	printf("%lld: Has to be crammed into this buffer\n", x);
-	printf("%lld: Until it overflows\n", x);
-	printf("%lld: And causes flops\n", x);
-	self->speculateFlag++;
-
-}
-
-BEGIN
-/1 <= self->speculateFlag/
-{
-	commit(self->spec);
-	self->commitFlag++;
-}
-
-BEGIN
-/1 <= self->commitFlag/
-{
-	printf("Statement was executed\n");
-	exit(1);
-}
-
-BEGIN
-/1 > self->commitFlag/
-{
-	printf("Statement wasn't executed\n");
-	exit(0);
-}
-
-ERROR
-{
-	exit(1);
-}
diff --git a/test/unittest/speculation/tst.SpecSizeVariations4.r b/test/unittest/speculation/tst.SpecSizeVariations4.r
deleted file mode 100644
index 7c4bb3b7..00000000
--- a/test/unittest/speculation/tst.SpecSizeVariations4.r
+++ /dev/null
@@ -1,5 +0,0 @@
-Speculative buffer ID: 1
-Statement wasn't executed
-
--- @@stderr --
-dtrace: 1 speculative drop
diff --git a/test/unittest/speculation/tst.SpecSizeVariations5.d b/test/unittest/speculation/tst.SpecSizeVariations5.d
deleted file mode 100644
index fb71dfed..00000000
--- a/test/unittest/speculation/tst.SpecSizeVariations5.d
+++ /dev/null
@@ -1,61 +0,0 @@
-/*
- * Oracle Linux DTrace.
- * Copyright (c) 2006, 2021, 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.
- */
-
-/*
- * ASSERTION:
- * Verify the behavior of speculations with changes in specsize.
- *
- * SECTION: Speculative Tracing/Options and Tuning;
- *	Options and Tunables/specsize
- *
- */
-
-#pragma D option quiet
-#pragma D option specsize=40
-
-long long x;
-
-BEGIN
-{
-	x = 123456789;
-	self->speculateFlag = 0;
-	self->commitFlag = 0;
-	self->spec = speculation();
-	printf("Speculative buffer ID: %d\n", self->spec);
-}
-
-BEGIN
-{
-	speculate(self->spec);
-	printf("%lld: Lots of data\n", x);
-	printf("%lld: Has to be crammed into this buffer\n", x);
-	printf("%lld: Until it overflows\n", x);
-	printf("%lld: And causes flops\n", x);
-	self->speculateFlag++;
-
-}
-
-BEGIN
-/1 <= self->speculateFlag/
-{
-	commit(self->spec);
-	self->commitFlag++;
-}
-
-BEGIN
-/1 <= self->commitFlag/
-{
-	printf("Statement was executed\n");
-	exit(0);
-}
-
-BEGIN
-/1 > self->commitFlag/
-{
-	printf("Statement wasn't executed\n");
-	exit(1);
-}
diff --git a/test/unittest/speculation/tst.SpecSizeVariations5.r b/test/unittest/speculation/tst.SpecSizeVariations5.r
deleted file mode 100644
index d09013a2..00000000
--- a/test/unittest/speculation/tst.SpecSizeVariations5.r
+++ /dev/null
@@ -1,7 +0,0 @@
-Speculative buffer ID: 1
-123456789: Lots of data
-123456789: Has to be crammed into this buffer
-123456789: Until it overflows
-123456789: And causes flops
-Statement was executed
-
-- 
2.43.5

Re: [PATCH 15/22] test: Clean up the specsize tests

[Kris Van Hees]

On Thu, Aug 29, 2024 at 01:22:12AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> The tests had actions like
>     printf("%lld: Lots of data\n", x);
>     printf("%lld: Has to be crammed into this buffer\n", x);
>     printf("%lld: Until it overflows\n", x);
>     printf("%lld: And causes flops\n", x);
> suggesting that these strings were crowding the buffer, but these
> strings are not passed from producer to consumer at all.
> 
> The tests also only tested one clause per speculation.  It would be
> nice also to test multiple clauses per speculation.
> 
> There is much replicated code from one of the tests to the other, a
> shortcoming that is amplified if we want to test more specsize values,
> which is the case when we test multiple clauses per speculation.
> 
> Therefore, replace the multiple tests with a single test that checks
> multiple clauses per speculation and more values of specsize.
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

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

> ---
>  .../speculation/tst.SpecSizeVariations.r      | 68 +++++++++++++++++
>  .../speculation/tst.SpecSizeVariations.sh     | 74 +++++++++++++++++++
>  .../speculation/tst.SpecSizeVariations4.d     | 66 -----------------
>  .../speculation/tst.SpecSizeVariations4.r     |  5 --
>  .../speculation/tst.SpecSizeVariations5.d     | 61 ---------------
>  .../speculation/tst.SpecSizeVariations5.r     |  7 --
>  6 files changed, 142 insertions(+), 139 deletions(-)
>  create mode 100644 test/unittest/speculation/tst.SpecSizeVariations.r
>  create mode 100755 test/unittest/speculation/tst.SpecSizeVariations.sh
>  delete mode 100644 test/unittest/speculation/tst.SpecSizeVariations4.d
>  delete mode 100644 test/unittest/speculation/tst.SpecSizeVariations4.r
>  delete mode 100644 test/unittest/speculation/tst.SpecSizeVariations5.d
>  delete mode 100644 test/unittest/speculation/tst.SpecSizeVariations5.r
> 
> diff --git a/test/unittest/speculation/tst.SpecSizeVariations.r b/test/unittest/speculation/tst.SpecSizeVariations.r
> new file mode 100644
> index 00000000..51f0596c
> --- /dev/null
> +++ b/test/unittest/speculation/tst.SpecSizeVariations.r
> @@ -0,0 +1,68 @@
> +Speculative buffer ID: 1
> +counts: 0 1
> +
> +Speculative buffer ID: 1
> +123456700
> +123456701
> +123456702
> +123456703
> +123456704
> +123456705
> +123456706
> +counts: 1 1
> +
> +Speculative buffer ID: 1
> +123456700
> +123456701
> +123456702
> +123456703
> +123456704
> +123456705
> +123456706
> +counts: 1 1
> +
> +Speculative buffer ID: 1
> +123456700
> +123456701
> +123456702
> +123456703
> +123456704
> +123456705
> +123456706
> +counts: 2 1
> +
> +Speculative buffer ID: 1
> +123456700
> +123456701
> +123456702
> +123456703
> +123456704
> +123456705
> +123456706
> +counts: 2 1
> +
> +Speculative buffer ID: 1
> +123456700
> +123456701
> +123456702
> +123456703
> +123456704
> +123456705
> +123456706
> +123456800
> +123456801
> +123456802
> +123456803
> +123456804
> +123456805
> +123456806
> +123456807
> +123456808
> +counts: 2 1
> +
> +-- @@stderr --
> +dtrace: 2 speculative drops
> +dtrace: 1 speculative drop
> +dtrace: 1 speculative drop
> +dtrace: 1 speculative drop
> +dtrace: 1 speculative drop
> diff --git a/test/unittest/speculation/tst.SpecSizeVariations.sh b/test/unittest/speculation/tst.SpecSizeVariations.sh
> new file mode 100755
> index 00000000..75e527d9
> --- /dev/null
> +++ b/test/unittest/speculation/tst.SpecSizeVariations.sh
> @@ -0,0 +1,74 @@
> +#!/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.
> +#
> +
> +dtrace=$1
> +
> +for x in 63 64 79 80 143 144; do
> +	$dtrace $dt_flags -xspecsize=$x -qn '
> +	BEGIN
> +	{
> +		x = 123456700ll;
> +		self->nspeculate = 0;
> +		self->ncommit = 0;
> +		self->spec = speculation();
> +		printf("Speculative buffer ID: %d\n", self->spec);
> +	}
> +
> +	/* 16 + 7 * 8 = 72 bytes */
> +	BEGIN
> +	{
> +		speculate(self->spec);
> +		printf("%lld\n", x++);
> +		printf("%lld\n", x++);
> +		printf("%lld\n", x++);
> +		printf("%lld\n", x++);
> +		printf("%lld\n", x++);
> +		printf("%lld\n", x++);
> +		printf("%lld\n", x++);
> +		self->nspeculate++;
> +	}
> +
> +	BEGIN
> +	{
> +		x = 123456800ll;
> +	}
> +
> +	/* 16 + 9 * 8 = 88 bytes */
> +	BEGIN
> +	{
> +		speculate(self->spec);
> +		printf("%lld\n", x++);
> +		printf("%lld\n", x++);
> +		printf("%lld\n", x++);
> +		printf("%lld\n", x++);
> +		printf("%lld\n", x++);
> +		printf("%lld\n", x++);
> +		printf("%lld\n", x++);
> +		printf("%lld\n", x++);
> +		printf("%lld\n", x++);
> +		self->nspeculate++;
> +	}
> +
> +	BEGIN
> +	{
> +		commit(self->spec);
> +		self->ncommit++;
> +	}
> +
> +	BEGIN
> +	{
> +		printf("counts: %d %d\n", self->nspeculate, self->ncommit);
> +		exit(0);
> +	}
> +
> +	ERROR
> +	{
> +		exit(1);
> +	}'
> +done
> diff --git a/test/unittest/speculation/tst.SpecSizeVariations4.d b/test/unittest/speculation/tst.SpecSizeVariations4.d
> deleted file mode 100644
> index 4221c89e..00000000
> --- a/test/unittest/speculation/tst.SpecSizeVariations4.d
> +++ /dev/null
> @@ -1,66 +0,0 @@
> -/*
> - * Oracle Linux DTrace.
> - * Copyright (c) 2006, 2023, 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.
> - */
> -
> -/*
> - * ASSERTION:
> - * Verify the behavior of speculations with changes in specsize.
> - *
> - * SECTION: Speculative Tracing/Options and Tuning;
> - *	Options and Tunables/specsize
> - *
> - */
> -
> -#pragma D option quiet
> -#pragma D option specsize=39
> -
> -long long x;
> -
> -BEGIN
> -{
> -	x = 123456789;
> -	self->speculateFlag = 0;
> -	self->commitFlag = 0;
> -	self->spec = speculation();
> -	printf("Speculative buffer ID: %d\n", self->spec);
> -}
> -
> -BEGIN
> -{
> -	speculate(self->spec);
> -	printf("%lld: Lots of data\n", x);
> -	printf("%lld: Has to be crammed into this buffer\n", x);
> -	printf("%lld: Until it overflows\n", x);
> -	printf("%lld: And causes flops\n", x);
> -	self->speculateFlag++;
> -
> -}
> -
> -BEGIN
> -/1 <= self->speculateFlag/
> -{
> -	commit(self->spec);
> -	self->commitFlag++;
> -}
> -
> -BEGIN
> -/1 <= self->commitFlag/
> -{
> -	printf("Statement was executed\n");
> -	exit(1);
> -}
> -
> -BEGIN
> -/1 > self->commitFlag/
> -{
> -	printf("Statement wasn't executed\n");
> -	exit(0);
> -}
> -
> -ERROR
> -{
> -	exit(1);
> -}
> diff --git a/test/unittest/speculation/tst.SpecSizeVariations4.r b/test/unittest/speculation/tst.SpecSizeVariations4.r
> deleted file mode 100644
> index 7c4bb3b7..00000000
> --- a/test/unittest/speculation/tst.SpecSizeVariations4.r
> +++ /dev/null
> @@ -1,5 +0,0 @@
> -Speculative buffer ID: 1
> -Statement wasn't executed
> -
> --- @@stderr --
> -dtrace: 1 speculative drop
> diff --git a/test/unittest/speculation/tst.SpecSizeVariations5.d b/test/unittest/speculation/tst.SpecSizeVariations5.d
> deleted file mode 100644
> index fb71dfed..00000000
> --- a/test/unittest/speculation/tst.SpecSizeVariations5.d
> +++ /dev/null
> @@ -1,61 +0,0 @@
> -/*
> - * Oracle Linux DTrace.
> - * Copyright (c) 2006, 2021, 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.
> - */
> -
> -/*
> - * ASSERTION:
> - * Verify the behavior of speculations with changes in specsize.
> - *
> - * SECTION: Speculative Tracing/Options and Tuning;
> - *	Options and Tunables/specsize
> - *
> - */
> -
> -#pragma D option quiet
> -#pragma D option specsize=40
> -
> -long long x;
> -
> -BEGIN
> -{
> -	x = 123456789;
> -	self->speculateFlag = 0;
> -	self->commitFlag = 0;
> -	self->spec = speculation();
> -	printf("Speculative buffer ID: %d\n", self->spec);
> -}
> -
> -BEGIN
> -{
> -	speculate(self->spec);
> -	printf("%lld: Lots of data\n", x);
> -	printf("%lld: Has to be crammed into this buffer\n", x);
> -	printf("%lld: Until it overflows\n", x);
> -	printf("%lld: And causes flops\n", x);
> -	self->speculateFlag++;
> -
> -}
> -
> -BEGIN
> -/1 <= self->speculateFlag/
> -{
> -	commit(self->spec);
> -	self->commitFlag++;
> -}
> -
> -BEGIN
> -/1 <= self->commitFlag/
> -{
> -	printf("Statement was executed\n");
> -	exit(0);
> -}
> -
> -BEGIN
> -/1 > self->commitFlag/
> -{
> -	printf("Statement wasn't executed\n");
> -	exit(1);
> -}
> diff --git a/test/unittest/speculation/tst.SpecSizeVariations5.r b/test/unittest/speculation/tst.SpecSizeVariations5.r
> deleted file mode 100644
> index d09013a2..00000000
> --- a/test/unittest/speculation/tst.SpecSizeVariations5.r
> +++ /dev/null
> @@ -1,7 +0,0 @@
> -Speculative buffer ID: 1
> -123456789: Lots of data
> -123456789: Has to be crammed into this buffer
> -123456789: Until it overflows
> -123456789: And causes flops
> -Statement was executed
> -
> -- 
> 2.43.5
> 

[PATCH 16/22] test: Fix the speculative tests that checked bufsize

[eugene.loh]

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

Ever since speculations were reimplemented, these tests simply were
not testing what they claimed to test.  Among other things:

1)  There is a simple dependence on bufsize that is independent of
speculations.  So speculations have no special role in bufsize tests.

2)  The messages in the print statements were not taxing bufsize.
(A single default string would be enough to exhaust 60 bytes, but
the producer wasn't passing the consumer these strings at all.
The bufsize is being set by the implicit ERROR probe.)

3)  The various size ranges described in the comments do not exist
and were not being tested anyhow.

So remove these tests, and just rely on the other bufsize tests.

To be fair, there seemed to have been no other "negative bufsize"
test, but add a more straightforward such test and add it to where
the other bufsize tests are.

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 test/unittest/options/err.bufsize-negative.d  | 24 +++++++
 test/unittest/options/err.bufsize-negative.r  |  2 +
 .../speculation/err.BufSizeVariations1.d      | 67 -------------------
 .../speculation/err.BufSizeVariations2.d      | 67 -------------------
 .../speculation/err.NegativeBufSize.d         | 67 -------------------
 .../speculation/err.NegativeBufSize.r         |  2 -
 6 files changed, 26 insertions(+), 203 deletions(-)
 create mode 100644 test/unittest/options/err.bufsize-negative.d
 create mode 100644 test/unittest/options/err.bufsize-negative.r
 delete mode 100644 test/unittest/speculation/err.BufSizeVariations1.d
 delete mode 100644 test/unittest/speculation/err.BufSizeVariations2.d
 delete mode 100644 test/unittest/speculation/err.NegativeBufSize.d
 delete mode 100644 test/unittest/speculation/err.NegativeBufSize.r

diff --git a/test/unittest/options/err.bufsize-negative.d b/test/unittest/options/err.bufsize-negative.d
new file mode 100644
index 00000000..ac194dc8
--- /dev/null
+++ b/test/unittest/options/err.bufsize-negative.d
@@ -0,0 +1,24 @@
+/*
+ * 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.
+ */
+
+/*
+ * ASSERTION: The -xbufsize option sets the trace buffer size.
+ *
+ * SECTION: Options and Tunables/Consumer Options
+ */
+
+/* @@runtest-opts: -xbufsize=-1 */
+
+BEGIN
+{
+	exit(0);
+}
+
+ERROR
+{
+	exit(1);
+}
diff --git a/test/unittest/options/err.bufsize-negative.r b/test/unittest/options/err.bufsize-negative.r
new file mode 100644
index 00000000..ea3089e9
--- /dev/null
+++ b/test/unittest/options/err.bufsize-negative.r
@@ -0,0 +1,2 @@
+-- @@stderr --
+dtrace: failed to set -x bufsize: Invalid value for specified option
diff --git a/test/unittest/speculation/err.BufSizeVariations1.d b/test/unittest/speculation/err.BufSizeVariations1.d
deleted file mode 100644
index c6e29b78..00000000
--- a/test/unittest/speculation/err.BufSizeVariations1.d
+++ /dev/null
@@ -1,67 +0,0 @@
-/*
- * Oracle Linux DTrace.
- * Copyright (c) 2006, 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.
- */
-
-/*
- * ASSERTION:
- * Verify the behavior of variations in bufsize.
- *
- * SECTION: Speculative Tracing/Options and Tuning;
- *	Options and Tunables/bufsize
- *
- * NOTES: This test behaves differently depending on the values
- * assigned to bufsize.
- * 1. 0 > bufsize.
- * 2. 0 == bufsize.
- * 3. 0 < bufsize <= 7
- * 4. 8 <= bufsize <= 31
- * 5. 32 <= bufsize <= 47
- * 6. 48 <= bufsize <= 71
- * 7. 72 <= bufsize
- */
-
-#pragma D option quiet
-#pragma D option bufsize=41
-
-BEGIN
-{
-	self->speculateFlag = 0;
-	self->commitFlag = 0;
-	self->spec = speculation();
-	printf("Speculative buffer ID: %d\n", self->spec);
-}
-
-BEGIN
-{
-	speculate(self->spec);
-	printf("Lots of data\n");
-	printf("Has to be crammed into this buffer\n");
-	printf("Until it overflows\n");
-	printf("And causes flops\n");
-	self->speculateFlag++;
-
-}
-
-BEGIN
-/1 <= self->speculateFlag/
-{
-	commit(self->spec);
-	self->commitFlag++;
-}
-
-BEGIN
-/1 <= self->commitFlag/
-{
-	printf("Statement was executed\n");
-	exit(0);
-}
-
-BEGIN
-/1 > self->commitFlag/
-{
-	printf("Statement wasn't executed\n");
-	exit(1);
-}
diff --git a/test/unittest/speculation/err.BufSizeVariations2.d b/test/unittest/speculation/err.BufSizeVariations2.d
deleted file mode 100644
index 8094cd0b..00000000
--- a/test/unittest/speculation/err.BufSizeVariations2.d
+++ /dev/null
@@ -1,67 +0,0 @@
-/*
- * Oracle Linux DTrace.
- * Copyright (c) 2006, 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.
- */
-
-/*
- * ASSERTION:
- * Verify the behavior of variations in bufsize.
- *
- * SECTION: Speculative Tracing/Options and Tuning;
- *	    Options and Tunables/bufsize
- *
- * NOTES: This test behaves differently depending on the values
- * assigned to bufsize.
- * 1. 0 > bufsize.
- * 2. 0 == bufsize.
- * 3. 0 < bufsize <= 7
- * 4. 8 <= bufsize <= 31
- * 5. 32 <= bufsize <= 47
- * 6. 48 <= bufsize <= 71
- * 7. 72 <= bufsize
- */
-
-#pragma D option quiet
-#pragma D option bufsize=4
-
-BEGIN
-{
-	self->speculateFlag = 0;
-	self->commitFlag = 0;
-	self->spec = speculation();
-	printf("Speculative buffer ID: %d\n", self->spec);
-}
-
-BEGIN
-{
-	speculate(self->spec);
-	printf("Lots of data\n");
-	printf("Has to be crammed into this buffer\n");
-	printf("Until it overflows\n");
-	printf("And causes flops\n");
-	self->speculateFlag++;
-
-}
-
-BEGIN
-/1 <= self->speculateFlag/
-{
-	commit(self->spec);
-	self->commitFlag++;
-}
-
-BEGIN
-/1 <= self->commitFlag/
-{
-	printf("Statement was executed\n");
-	exit(0);
-}
-
-BEGIN
-/1 > self->commitFlag/
-{
-	printf("Statement wasn't executed\n");
-	exit(1);
-}
diff --git a/test/unittest/speculation/err.NegativeBufSize.d b/test/unittest/speculation/err.NegativeBufSize.d
deleted file mode 100644
index a0154ccb..00000000
--- a/test/unittest/speculation/err.NegativeBufSize.d
+++ /dev/null
@@ -1,67 +0,0 @@
-/*
- * Oracle Linux DTrace.
- * Copyright (c) 2006, 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.
- */
-
-/*
- * ASSERTION:
- * Verify the behavior of variations in bufsize.
- *
- * SECTION: Speculative Tracing/Options and Tuning;
- *	Options and Tunables/bufsize
- *
- * NOTES: This test behaves differently depending on the values
- * assigned to bufsize.
- * 1. 0 > bufsize.
- * 2. 0 == bufsize.
- * 3. 0 < bufsize <= 7
- * 4. 8 <= bufsize <= 31
- * 5. 32 <= bufsize <= 47
- * 6. 48 <= bufsize <= 71
- * 7. 72 <= bufsize
- */
-
-#pragma D option quiet
-#pragma D option bufsize=-72
-
-BEGIN
-{
-	self->speculateFlag = 0;
-	self->commitFlag = 0;
-	self->spec = speculation();
-	printf("Speculative buffer ID: %d\n", self->spec);
-}
-
-BEGIN
-{
-	speculate(self->spec);
-	printf("Lots of data\n");
-	printf("Has to be crammed into this buffer\n");
-	printf("Until it overflows\n");
-	printf("And causes flops\n");
-	self->speculateFlag++;
-
-}
-
-BEGIN
-/1 <= self->speculateFlag/
-{
-	commit(self->spec);
-	self->commitFlag++;
-}
-
-BEGIN
-/1 <= self->commitFlag/
-{
-	printf("Statement was executed\n");
-	exit(0);
-}
-
-BEGIN
-/1 > self->commitFlag/
-{
-	printf("Statement wasn't executed\n");
-	exit(1);
-}
diff --git a/test/unittest/speculation/err.NegativeBufSize.r b/test/unittest/speculation/err.NegativeBufSize.r
deleted file mode 100644
index 9d7be6c0..00000000
--- a/test/unittest/speculation/err.NegativeBufSize.r
+++ /dev/null
@@ -1,2 +0,0 @@
--- @@stderr --
-dtrace: failed to compile script test/unittest/speculation/err.NegativeBufSize.d: line 27: failed to set option 'bufsize' to '-72': Invalid value for specified option
-- 
2.43.5

Re: [PATCH 16/22] test: Fix the speculative tests that checked bufsize

[Kris Van Hees]

On Thu, Aug 29, 2024 at 01:22:13AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> Ever since speculations were reimplemented, these tests simply were
> not testing what they claimed to test.  Among other things:
> 
> 1)  There is a simple dependence on bufsize that is independent of
> speculations.  So speculations have no special role in bufsize tests.
> 
> 2)  The messages in the print statements were not taxing bufsize.
> (A single default string would be enough to exhaust 60 bytes, but
> the producer wasn't passing the consumer these strings at all.
> The bufsize is being set by the implicit ERROR probe.)
> 
> 3)  The various size ranges described in the comments do not exist
> and were not being tested anyhow.
> 
> So remove these tests, and just rely on the other bufsize tests.
> 
> To be fair, there seemed to have been no other "negative bufsize"
> test, but add a more straightforward such test and add it to where
> the other bufsize tests are.
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

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

> ---
>  test/unittest/options/err.bufsize-negative.d  | 24 +++++++
>  test/unittest/options/err.bufsize-negative.r  |  2 +
>  .../speculation/err.BufSizeVariations1.d      | 67 -------------------
>  .../speculation/err.BufSizeVariations2.d      | 67 -------------------
>  .../speculation/err.NegativeBufSize.d         | 67 -------------------
>  .../speculation/err.NegativeBufSize.r         |  2 -
>  6 files changed, 26 insertions(+), 203 deletions(-)
>  create mode 100644 test/unittest/options/err.bufsize-negative.d
>  create mode 100644 test/unittest/options/err.bufsize-negative.r
>  delete mode 100644 test/unittest/speculation/err.BufSizeVariations1.d
>  delete mode 100644 test/unittest/speculation/err.BufSizeVariations2.d
>  delete mode 100644 test/unittest/speculation/err.NegativeBufSize.d
>  delete mode 100644 test/unittest/speculation/err.NegativeBufSize.r
> 
> diff --git a/test/unittest/options/err.bufsize-negative.d b/test/unittest/options/err.bufsize-negative.d
> new file mode 100644
> index 00000000..ac194dc8
> --- /dev/null
> +++ b/test/unittest/options/err.bufsize-negative.d
> @@ -0,0 +1,24 @@
> +/*
> + * 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.
> + */
> +
> +/*
> + * ASSERTION: The -xbufsize option sets the trace buffer size.
> + *
> + * SECTION: Options and Tunables/Consumer Options
> + */
> +
> +/* @@runtest-opts: -xbufsize=-1 */
> +
> +BEGIN
> +{
> +	exit(0);
> +}
> +
> +ERROR
> +{
> +	exit(1);
> +}
> diff --git a/test/unittest/options/err.bufsize-negative.r b/test/unittest/options/err.bufsize-negative.r
> new file mode 100644
> index 00000000..ea3089e9
> --- /dev/null
> +++ b/test/unittest/options/err.bufsize-negative.r
> @@ -0,0 +1,2 @@
> +-- @@stderr --
> +dtrace: failed to set -x bufsize: Invalid value for specified option
> diff --git a/test/unittest/speculation/err.BufSizeVariations1.d b/test/unittest/speculation/err.BufSizeVariations1.d
> deleted file mode 100644
> index c6e29b78..00000000
> --- a/test/unittest/speculation/err.BufSizeVariations1.d
> +++ /dev/null
> @@ -1,67 +0,0 @@
> -/*
> - * Oracle Linux DTrace.
> - * Copyright (c) 2006, 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.
> - */
> -
> -/*
> - * ASSERTION:
> - * Verify the behavior of variations in bufsize.
> - *
> - * SECTION: Speculative Tracing/Options and Tuning;
> - *	Options and Tunables/bufsize
> - *
> - * NOTES: This test behaves differently depending on the values
> - * assigned to bufsize.
> - * 1. 0 > bufsize.
> - * 2. 0 == bufsize.
> - * 3. 0 < bufsize <= 7
> - * 4. 8 <= bufsize <= 31
> - * 5. 32 <= bufsize <= 47
> - * 6. 48 <= bufsize <= 71
> - * 7. 72 <= bufsize
> - */
> -
> -#pragma D option quiet
> -#pragma D option bufsize=41
> -
> -BEGIN
> -{
> -	self->speculateFlag = 0;
> -	self->commitFlag = 0;
> -	self->spec = speculation();
> -	printf("Speculative buffer ID: %d\n", self->spec);
> -}
> -
> -BEGIN
> -{
> -	speculate(self->spec);
> -	printf("Lots of data\n");
> -	printf("Has to be crammed into this buffer\n");
> -	printf("Until it overflows\n");
> -	printf("And causes flops\n");
> -	self->speculateFlag++;
> -
> -}
> -
> -BEGIN
> -/1 <= self->speculateFlag/
> -{
> -	commit(self->spec);
> -	self->commitFlag++;
> -}
> -
> -BEGIN
> -/1 <= self->commitFlag/
> -{
> -	printf("Statement was executed\n");
> -	exit(0);
> -}
> -
> -BEGIN
> -/1 > self->commitFlag/
> -{
> -	printf("Statement wasn't executed\n");
> -	exit(1);
> -}
> diff --git a/test/unittest/speculation/err.BufSizeVariations2.d b/test/unittest/speculation/err.BufSizeVariations2.d
> deleted file mode 100644
> index 8094cd0b..00000000
> --- a/test/unittest/speculation/err.BufSizeVariations2.d
> +++ /dev/null
> @@ -1,67 +0,0 @@
> -/*
> - * Oracle Linux DTrace.
> - * Copyright (c) 2006, 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.
> - */
> -
> -/*
> - * ASSERTION:
> - * Verify the behavior of variations in bufsize.
> - *
> - * SECTION: Speculative Tracing/Options and Tuning;
> - *	    Options and Tunables/bufsize
> - *
> - * NOTES: This test behaves differently depending on the values
> - * assigned to bufsize.
> - * 1. 0 > bufsize.
> - * 2. 0 == bufsize.
> - * 3. 0 < bufsize <= 7
> - * 4. 8 <= bufsize <= 31
> - * 5. 32 <= bufsize <= 47
> - * 6. 48 <= bufsize <= 71
> - * 7. 72 <= bufsize
> - */
> -
> -#pragma D option quiet
> -#pragma D option bufsize=4
> -
> -BEGIN
> -{
> -	self->speculateFlag = 0;
> -	self->commitFlag = 0;
> -	self->spec = speculation();
> -	printf("Speculative buffer ID: %d\n", self->spec);
> -}
> -
> -BEGIN
> -{
> -	speculate(self->spec);
> -	printf("Lots of data\n");
> -	printf("Has to be crammed into this buffer\n");
> -	printf("Until it overflows\n");
> -	printf("And causes flops\n");
> -	self->speculateFlag++;
> -
> -}
> -
> -BEGIN
> -/1 <= self->speculateFlag/
> -{
> -	commit(self->spec);
> -	self->commitFlag++;
> -}
> -
> -BEGIN
> -/1 <= self->commitFlag/
> -{
> -	printf("Statement was executed\n");
> -	exit(0);
> -}
> -
> -BEGIN
> -/1 > self->commitFlag/
> -{
> -	printf("Statement wasn't executed\n");
> -	exit(1);
> -}
> diff --git a/test/unittest/speculation/err.NegativeBufSize.d b/test/unittest/speculation/err.NegativeBufSize.d
> deleted file mode 100644
> index a0154ccb..00000000
> --- a/test/unittest/speculation/err.NegativeBufSize.d
> +++ /dev/null
> @@ -1,67 +0,0 @@
> -/*
> - * Oracle Linux DTrace.
> - * Copyright (c) 2006, 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.
> - */
> -
> -/*
> - * ASSERTION:
> - * Verify the behavior of variations in bufsize.
> - *
> - * SECTION: Speculative Tracing/Options and Tuning;
> - *	Options and Tunables/bufsize
> - *
> - * NOTES: This test behaves differently depending on the values
> - * assigned to bufsize.
> - * 1. 0 > bufsize.
> - * 2. 0 == bufsize.
> - * 3. 0 < bufsize <= 7
> - * 4. 8 <= bufsize <= 31
> - * 5. 32 <= bufsize <= 47
> - * 6. 48 <= bufsize <= 71
> - * 7. 72 <= bufsize
> - */
> -
> -#pragma D option quiet
> -#pragma D option bufsize=-72
> -
> -BEGIN
> -{
> -	self->speculateFlag = 0;
> -	self->commitFlag = 0;
> -	self->spec = speculation();
> -	printf("Speculative buffer ID: %d\n", self->spec);
> -}
> -
> -BEGIN
> -{
> -	speculate(self->spec);
> -	printf("Lots of data\n");
> -	printf("Has to be crammed into this buffer\n");
> -	printf("Until it overflows\n");
> -	printf("And causes flops\n");
> -	self->speculateFlag++;
> -
> -}
> -
> -BEGIN
> -/1 <= self->speculateFlag/
> -{
> -	commit(self->spec);
> -	self->commitFlag++;
> -}
> -
> -BEGIN
> -/1 <= self->commitFlag/
> -{
> -	printf("Statement was executed\n");
> -	exit(0);
> -}
> -
> -BEGIN
> -/1 > self->commitFlag/
> -{
> -	printf("Statement wasn't executed\n");
> -	exit(1);
> -}
> diff --git a/test/unittest/speculation/err.NegativeBufSize.r b/test/unittest/speculation/err.NegativeBufSize.r
> deleted file mode 100644
> index 9d7be6c0..00000000
> --- a/test/unittest/speculation/err.NegativeBufSize.r
> +++ /dev/null
> @@ -1,2 +0,0 @@
> --- @@stderr --
> -dtrace: failed to compile script test/unittest/speculation/err.NegativeBufSize.d: line 27: failed to set option 'bufsize' to '-72': Invalid value for specified option
> -- 
> 2.43.5
> 

[PATCH 17/22] test: Tweak spec sizes to bracket size jumps more narrowly

[eugene.loh]

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

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 .../speculation/tst.SpecSizeVariations.r      | 22 -------------------
 .../speculation/tst.SpecSizeVariations.sh     |  2 +-
 2 files changed, 1 insertion(+), 23 deletions(-)

diff --git a/test/unittest/speculation/tst.SpecSizeVariations.r b/test/unittest/speculation/tst.SpecSizeVariations.r
index 51f0596c..2748b307 100644
--- a/test/unittest/speculation/tst.SpecSizeVariations.r
+++ b/test/unittest/speculation/tst.SpecSizeVariations.r
@@ -11,26 +11,6 @@ Speculative buffer ID: 1
 123456706
 counts: 1 1
 
-Speculative buffer ID: 1
-123456700
-123456701
-123456702
-123456703
-123456704
-123456705
-123456706
-counts: 1 1
-
-Speculative buffer ID: 1
-123456700
-123456701
-123456702
-123456703
-123456704
-123456705
-123456706
-counts: 2 1
-
 Speculative buffer ID: 1
 123456700
 123456701
@@ -64,5 +44,3 @@ counts: 2 1
 dtrace: 2 speculative drops
 dtrace: 1 speculative drop
 dtrace: 1 speculative drop
-dtrace: 1 speculative drop
-dtrace: 1 speculative drop
diff --git a/test/unittest/speculation/tst.SpecSizeVariations.sh b/test/unittest/speculation/tst.SpecSizeVariations.sh
index 75e527d9..79995b59 100755
--- a/test/unittest/speculation/tst.SpecSizeVariations.sh
+++ b/test/unittest/speculation/tst.SpecSizeVariations.sh
@@ -9,7 +9,7 @@
 
 dtrace=$1
 
-for x in 63 64 79 80 143 144; do
+for x in 71 72 159 160; do
 	$dtrace $dt_flags -xspecsize=$x -qn '
 	BEGIN
 	{
-- 
2.43.5

Re: [PATCH 17/22] test: Tweak spec sizes to bracket size jumps more narrowly

[Kris Van Hees]

[-- Attachment #1: Type: text/plain, Size: 1737 bytes --]

This test gives me faqilures.

runtest.log in attachment

On Thu, Aug 29, 2024 at 01:22:14AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> ---
>  .../speculation/tst.SpecSizeVariations.r      | 22 -------------------
>  .../speculation/tst.SpecSizeVariations.sh     |  2 +-
>  2 files changed, 1 insertion(+), 23 deletions(-)
> 
> diff --git a/test/unittest/speculation/tst.SpecSizeVariations.r b/test/unittest/speculation/tst.SpecSizeVariations.r
> index 51f0596c..2748b307 100644
> --- a/test/unittest/speculation/tst.SpecSizeVariations.r
> +++ b/test/unittest/speculation/tst.SpecSizeVariations.r
> @@ -11,26 +11,6 @@ Speculative buffer ID: 1
>  123456706
>  counts: 1 1
>  
> -Speculative buffer ID: 1
> -123456700
> -123456701
> -123456702
> -123456703
> -123456704
> -123456705
> -123456706
> -counts: 1 1
> -
> -Speculative buffer ID: 1
> -123456700
> -123456701
> -123456702
> -123456703
> -123456704
> -123456705
> -123456706
> -counts: 2 1
> -
>  Speculative buffer ID: 1
>  123456700
>  123456701
> @@ -64,5 +44,3 @@ counts: 2 1
>  dtrace: 2 speculative drops
>  dtrace: 1 speculative drop
>  dtrace: 1 speculative drop
> -dtrace: 1 speculative drop
> -dtrace: 1 speculative drop
> diff --git a/test/unittest/speculation/tst.SpecSizeVariations.sh b/test/unittest/speculation/tst.SpecSizeVariations.sh
> index 75e527d9..79995b59 100755
> --- a/test/unittest/speculation/tst.SpecSizeVariations.sh
> +++ b/test/unittest/speculation/tst.SpecSizeVariations.sh
> @@ -9,7 +9,7 @@
>  
>  dtrace=$1
>  
> -for x in 63 64 79 80 143 144; do
> +for x in 71 72 159 160; do
>  	$dtrace $dt_flags -xspecsize=$x -qn '
>  	BEGIN
>  	{
> -- 
> 2.43.5
> 

[-- Attachment #2: runtest.log --]
[-- Type: text/plain, Size: 2033 bytes --]

dtrace: Oracle D 2.0
This is DTrace 2.0.1
dtrace(1) version-control ID: 76d327b2d33c377041b8f1b3dbd67de7f2cc7b65
libdtrace version-control ID: 76d327b2d33c377041b8f1b3dbd67de7f2cc7b65
Linux kvh-deb-bpf3 6.8.8 #1 SMP PREEMPT_DYNAMIC Mon Apr 29 14:26:32 EDT 2024 x86_64 GNU/Linux
testsuite version-control ID: 76d327b2d33c377041b8f1b3dbd67de7f2cc7b65

test/unittest/speculation/tst.SpecSizeVariations.sh: Running timeout --signal=TERM 41 test/unittest/speculation/tst.SpecSizeVariations.sh /scratch/dtrace-bpf-user/build/dtrace 
FAIL: expected results differ.
Speculative buffer ID: 1
123456700
123456701
123456702
123456703
123456704
123456705
123456706
counts: 1 1

Speculative buffer ID: 1
123456700
123456701
123456702
123456703
123456704
123456705
123456706
counts: 1 1

Speculative buffer ID: 1
123456700
123456701
123456702
123456703
123456704
123456705
123456706
123456800
123456801
123456802
123456803
123456804
123456805
123456806
123456807
123456808
counts: 2 1

Speculative buffer ID: 1
123456700
123456701
123456702
123456703
123456704
123456705
123456706
123456800
123456801
123456802
123456803
123456804
123456805
123456806
123456807
123456808
counts: 2 1

-- @@stderr --
dtrace: 1 speculative drop
dtrace: 1 speculative drop
Diff against expected:
--- test/unittest/speculation/tst.SpecSizeVariations.r	2024-09-14 14:04:10.560046706 -0400
+++ /tmp/runtest.27272/test.out	2024-09-14 14:06:03.926307171 -0400
@@ -1,5 +1,12 @@
 Speculative buffer ID: 1
-counts: 0 1
+123456700
+123456701
+123456702
+123456703
+123456704
+123456705
+123456706
+counts: 1 1
 
 Speculative buffer ID: 1
 123456700
@@ -19,6 +26,15 @@
 123456704
 123456705
 123456706
+123456800
+123456801
+123456802
+123456803
+123456804
+123456805
+123456806
+123456807
+123456808
 counts: 2 1
 
 Speculative buffer ID: 1
@@ -41,6 +57,5 @@
 counts: 2 1
 
 -- @@stderr --
-dtrace: 2 speculative drops
 dtrace: 1 speculative drop
 dtrace: 1 speculative drop
Cleaning took 0.022518012 (0 kprobes, 0 uprobes)

1 cases (0 PASS, 1 FAIL, 0 XPASS, 0 XFAIL, 0 SKIP)

Re: [PATCH 17/22] test: Tweak spec sizes to bracket size jumps more narrowly

[Eugene Loh]

Yeah.  I think this is due to patch ordering.  The sizes depend on the 
new "Deprecate EPID" patch;  that patch, in turn, needs new sizes, 
whether narrow or not.

I guess I'll withdraw this 17/22 patch and just squash it into the 
already large 03/19 "Deprecate enabled probe ID (epid)"... v6.

On 9/14/24 14:07, Kris Van Hees wrote:
> This test gives me faqilures.
>
> runtest.log in attachment
>
> On Thu, Aug 29, 2024 at 01:22:14AM -0400, eugene.loh@oracle.com wrote:
>> From: Eugene Loh <eugene.loh@oracle.com>
>>
>> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
>> ---
>>   .../speculation/tst.SpecSizeVariations.r      | 22 -------------------
>>   .../speculation/tst.SpecSizeVariations.sh     |  2 +-
>>   2 files changed, 1 insertion(+), 23 deletions(-)
>>
>> diff --git a/test/unittest/speculation/tst.SpecSizeVariations.r b/test/unittest/speculation/tst.SpecSizeVariations.r
>> index 51f0596c..2748b307 100644
>> --- a/test/unittest/speculation/tst.SpecSizeVariations.r
>> +++ b/test/unittest/speculation/tst.SpecSizeVariations.r
>> @@ -11,26 +11,6 @@ Speculative buffer ID: 1
>>   123456706
>>   counts: 1 1
>>   
>> -Speculative buffer ID: 1
>> -123456700
>> -123456701
>> -123456702
>> -123456703
>> -123456704
>> -123456705
>> -123456706
>> -counts: 1 1
>> -
>> -Speculative buffer ID: 1
>> -123456700
>> -123456701
>> -123456702
>> -123456703
>> -123456704
>> -123456705
>> -123456706
>> -counts: 2 1
>> -
>>   Speculative buffer ID: 1
>>   123456700
>>   123456701
>> @@ -64,5 +44,3 @@ counts: 2 1
>>   dtrace: 2 speculative drops
>>   dtrace: 1 speculative drop
>>   dtrace: 1 speculative drop
>> -dtrace: 1 speculative drop
>> -dtrace: 1 speculative drop
>> diff --git a/test/unittest/speculation/tst.SpecSizeVariations.sh b/test/unittest/speculation/tst.SpecSizeVariations.sh
>> index 75e527d9..79995b59 100755
>> --- a/test/unittest/speculation/tst.SpecSizeVariations.sh
>> +++ b/test/unittest/speculation/tst.SpecSizeVariations.sh
>> @@ -9,7 +9,7 @@
>>   
>>   dtrace=$1
>>   
>> -for x in 63 64 79 80 143 144; do
>> +for x in 71 72 159 160; do
>>   	$dtrace $dt_flags -xspecsize=$x -qn '
>>   	BEGIN
>>   	{
>> -- 
>> 2.43.5
>>

[PATCH 18/22] test: Remove tst.DTRACEFLT_BADADDR2.d dependency on specific PC

[eugene.loh]

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

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 test/unittest/error/tst.DTRACEFLT_BADADDR2.d | 12 +++---------
 test/unittest/error/tst.DTRACEFLT_BADADDR2.r |  4 ++--
 2 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/test/unittest/error/tst.DTRACEFLT_BADADDR2.d b/test/unittest/error/tst.DTRACEFLT_BADADDR2.d
index 23663f3c..a822e63a 100644
--- a/test/unittest/error/tst.DTRACEFLT_BADADDR2.d
+++ b/test/unittest/error/tst.DTRACEFLT_BADADDR2.d
@@ -1,26 +1,23 @@
 /*
  * Oracle Linux DTrace.
- * Copyright (c) 2006, 2020, 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.
  */
-/* @@xfail: dtv2 */
 
 /*
  * ASSERTION:
  *	To test DTRACEFLT_BADADDR error with non-NULL address
  *
  * SECTION: dtrace Provider
- *
  */
 
-
 #pragma D option quiet
 
 ERROR
 {
-	printf("The arguments are %u %u %u %u %u\n",
-		arg1, arg2, arg3, arg4, arg5);
+	printf("The arguments are %u %u %u %u\n",
+		arg1, arg2, arg4, arg5);
 	printf("The value of arg4 should be %u\n", DTRACEFLT_BADADDR);
 	printf("The value of arg5 should be %u\n", 0x4000);
 	exit(0);
@@ -28,9 +25,6 @@ ERROR
 
 BEGIN
 {
-/*
-	x = (int *)64;
- */
 	x = (int *)0x4000;
 	y = *x;
 	trace(y);
diff --git a/test/unittest/error/tst.DTRACEFLT_BADADDR2.r b/test/unittest/error/tst.DTRACEFLT_BADADDR2.r
index ada685d6..6c5fa119 100644
--- a/test/unittest/error/tst.DTRACEFLT_BADADDR2.r
+++ b/test/unittest/error/tst.DTRACEFLT_BADADDR2.r
@@ -1,6 +1,6 @@
-The arguments are 2 2 4 1 16384
+The arguments are 3 1 1 16384
 The value of arg4 should be 1
 The value of arg5 should be 16384
 
 -- @@stderr --
-dtrace: error on enabled probe ID 2 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #2 at BPF pc NNN
+dtrace: error on enabled probe ID 3 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #1 at BPF pc NNN
-- 
2.43.5

Re: [PATCH 18/22] test: Remove tst.DTRACEFLT_BADADDR2.d dependency on specific PC

[Kris Van Hees]

On Thu, Aug 29, 2024 at 01:22:15AM -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>

> ---
>  test/unittest/error/tst.DTRACEFLT_BADADDR2.d | 12 +++---------
>  test/unittest/error/tst.DTRACEFLT_BADADDR2.r |  4 ++--
>  2 files changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/test/unittest/error/tst.DTRACEFLT_BADADDR2.d b/test/unittest/error/tst.DTRACEFLT_BADADDR2.d
> index 23663f3c..a822e63a 100644
> --- a/test/unittest/error/tst.DTRACEFLT_BADADDR2.d
> +++ b/test/unittest/error/tst.DTRACEFLT_BADADDR2.d
> @@ -1,26 +1,23 @@
>  /*
>   * Oracle Linux DTrace.
> - * Copyright (c) 2006, 2020, 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.
>   */
> -/* @@xfail: dtv2 */
>  
>  /*
>   * ASSERTION:
>   *	To test DTRACEFLT_BADADDR error with non-NULL address
>   *
>   * SECTION: dtrace Provider
> - *
>   */
>  
> -
>  #pragma D option quiet
>  
>  ERROR
>  {
> -	printf("The arguments are %u %u %u %u %u\n",
> -		arg1, arg2, arg3, arg4, arg5);
> +	printf("The arguments are %u %u %u %u\n",
> +		arg1, arg2, arg4, arg5);
>  	printf("The value of arg4 should be %u\n", DTRACEFLT_BADADDR);
>  	printf("The value of arg5 should be %u\n", 0x4000);
>  	exit(0);
> @@ -28,9 +25,6 @@ ERROR
>  
>  BEGIN
>  {
> -/*
> -	x = (int *)64;
> - */
>  	x = (int *)0x4000;
>  	y = *x;
>  	trace(y);
> diff --git a/test/unittest/error/tst.DTRACEFLT_BADADDR2.r b/test/unittest/error/tst.DTRACEFLT_BADADDR2.r
> index ada685d6..6c5fa119 100644
> --- a/test/unittest/error/tst.DTRACEFLT_BADADDR2.r
> +++ b/test/unittest/error/tst.DTRACEFLT_BADADDR2.r
> @@ -1,6 +1,6 @@
> -The arguments are 2 2 4 1 16384
> +The arguments are 3 1 1 16384
>  The value of arg4 should be 1
>  The value of arg5 should be 16384
>  
>  -- @@stderr --
> -dtrace: error on enabled probe ID 2 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #2 at BPF pc NNN
> +dtrace: error on enabled probe ID 3 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #1 at BPF pc NNN
> -- 
> 2.43.5
> 

[PATCH 19/22] test: Fix tst.probestar.d trigger

[eugene.loh]

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

This test was relying on a trigger that uses the obsolete
dt_test, even though dt_test is irrelevant to the test.

Use a different trigger.

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 test/unittest/probes/tst.probestar.d | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/test/unittest/probes/tst.probestar.d b/test/unittest/probes/tst.probestar.d
index dad7741e..b6da42e1 100644
--- a/test/unittest/probes/tst.probestar.d
+++ b/test/unittest/probes/tst.probestar.d
@@ -1,11 +1,11 @@
 /*
  * Oracle Linux DTrace.
- * Copyright (c) 2011, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2011, 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.
  */
-/* @@xfail: dtv2 */
-/* @@trigger: testprobe */
+/* @@trigger: readwholedir */
+/* FIXME */
 
 /*
  * ASSERTION:
@@ -16,7 +16,6 @@
  *
  */
 
-
 #pragma D option quiet
 
 int i;
-- 
2.43.5

Re: [PATCH 19/22] test: Fix tst.probestar.d trigger

[Kris Van Hees]

Given that the patch also adds a FIXME, I think that it would be better to
resolve whatever that FIXME is meant to refer to before we merge this.

:On Thu, Aug 29, 2024 at 01:22:16AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> This test was relying on a trigger that uses the obsolete
> dt_test, even though dt_test is irrelevant to the test.
> 
> Use a different trigger.
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> ---
>  test/unittest/probes/tst.probestar.d | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/test/unittest/probes/tst.probestar.d b/test/unittest/probes/tst.probestar.d
> index dad7741e..b6da42e1 100644
> --- a/test/unittest/probes/tst.probestar.d
> +++ b/test/unittest/probes/tst.probestar.d
> @@ -1,11 +1,11 @@
>  /*
>   * Oracle Linux DTrace.
> - * Copyright (c) 2011, 2020, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2011, 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.
>   */
> -/* @@xfail: dtv2 */
> -/* @@trigger: testprobe */
> +/* @@trigger: readwholedir */
> +/* FIXME */
>  
>  /*
>   * ASSERTION:
> @@ -16,7 +16,6 @@
>   *
>   */
>  
> -
>  #pragma D option quiet
>  
>  int i;
> -- 
> 2.43.5
> 

Re: [PATCH 19/22] test: Fix tst.probestar.d trigger

[Eugene Loh]

Yeah, weird.  I have no idea what the FIXME meant.  The fixed test works 
now.  I'll post a v2 without the FIXME comment.

On 9/14/24 14:13, Kris Van Hees wrote:
> Given that the patch also adds a FIXME, I think that it would be better to
> resolve whatever that FIXME is meant to refer to before we merge this.
>
> :On Thu, Aug 29, 2024 at 01:22:16AM -0400, eugene.loh@oracle.com wrote:
>> From: Eugene Loh <eugene.loh@oracle.com>
>>
>> This test was relying on a trigger that uses the obsolete
>> dt_test, even though dt_test is irrelevant to the test.
>>
>> Use a different trigger.
>>
>> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
>> ---
>>   test/unittest/probes/tst.probestar.d | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/test/unittest/probes/tst.probestar.d b/test/unittest/probes/tst.probestar.d
>> index dad7741e..b6da42e1 100644
>> --- a/test/unittest/probes/tst.probestar.d
>> +++ b/test/unittest/probes/tst.probestar.d
>> @@ -1,11 +1,11 @@
>>   /*
>>    * Oracle Linux DTrace.
>> - * Copyright (c) 2011, 2020, Oracle and/or its affiliates. All rights reserved.
>> + * Copyright (c) 2011, 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.
>>    */
>> -/* @@xfail: dtv2 */
>> -/* @@trigger: testprobe */
>> +/* @@trigger: readwholedir */
>> +/* FIXME */
>>   
>>   /*
>>    * ASSERTION:
>> @@ -16,7 +16,6 @@
>>    *
>>    */
>>   
>> -
>>   #pragma D option quiet
>>   
>>   int i;
>> -- 
>> 2.43.5
>>

[PATCH 20/22] test: Annotate some XFAILs

[eugene.loh]

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

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 test/unittest/assocs/tst.invalidref.d         |  2 +-
 test/unittest/builtinvar/tst.ipl.d            |  2 +-
 test/unittest/builtinvar/tst.ipl1.d           |  2 +-
 test/unittest/builtinvar/tst.vtimestamp.d     |  2 +-
 test/unittest/builtinvar/tst.vtimestamp2.d    |  2 +-
 .../lquantize/tst.32bit-bug26268136.sh        | 29 ++++++++++++++++++-
 .../printa/err.D_PRINTF_ARG_TYPE.jstack.d     |  2 +-
 test/unittest/printa/tst.jstack.d             |  2 +-
 .../tst.jstack_unprintable-bug26045010.sh     |  2 +-
 test/unittest/variables/bvar/tst.ipl.d        |  2 +-
 test/unittest/variables/bvar/tst.vtimestamp.d |  2 +-
 11 files changed, 38 insertions(+), 11 deletions(-)

diff --git a/test/unittest/assocs/tst.invalidref.d b/test/unittest/assocs/tst.invalidref.d
index 0ed17959..34367c6b 100644
--- a/test/unittest/assocs/tst.invalidref.d
+++ b/test/unittest/assocs/tst.invalidref.d
@@ -4,7 +4,7 @@
  * Licensed under the Universal Permissive License v 1.0 as shown at
  * http://oss.oracle.com/licenses/upl.
  */
-/* @@xfail: dtv2 */
+/* @@xfail: dtv2 no support for index of assoc array; last_cmds[1][3]=0 dumps BPF */
 
 /*
  * Test to ensure that invalid stores to a global associative array
diff --git a/test/unittest/builtinvar/tst.ipl.d b/test/unittest/builtinvar/tst.ipl.d
index c2b9850f..41a89b00 100644
--- a/test/unittest/builtinvar/tst.ipl.d
+++ b/test/unittest/builtinvar/tst.ipl.d
@@ -4,7 +4,7 @@
  * Licensed under the Universal Permissive License v 1.0 as shown at
  * http://oss.oracle.com/licenses/upl.
  */
-/* @@xfail: dtv2 */
+/* @@xfail: dtv2: need ipl support */
 
 /*
  * ASSERTION:
diff --git a/test/unittest/builtinvar/tst.ipl1.d b/test/unittest/builtinvar/tst.ipl1.d
index 1b489229..654b16ce 100644
--- a/test/unittest/builtinvar/tst.ipl1.d
+++ b/test/unittest/builtinvar/tst.ipl1.d
@@ -4,7 +4,7 @@
  * Licensed under the Universal Permissive License v 1.0 as shown at
  * http://oss.oracle.com/licenses/upl.
  */
-/* @@xfail: dtv2 */
+/* @@xfail: dtv2: need ipl support */
 
 /*
  * ASSERTION:
diff --git a/test/unittest/builtinvar/tst.vtimestamp.d b/test/unittest/builtinvar/tst.vtimestamp.d
index c9e2a112..11ffae7b 100644
--- a/test/unittest/builtinvar/tst.vtimestamp.d
+++ b/test/unittest/builtinvar/tst.vtimestamp.d
@@ -4,7 +4,7 @@
  * Licensed under the Universal Permissive License v 1.0 as shown at
  * http://oss.oracle.com/licenses/upl.
  */
-/* @@xfail: dtv2 */
+/* @@xfail: dtv2: need vtimestamp support */
 
 /*
  * ASSERTION:
diff --git a/test/unittest/builtinvar/tst.vtimestamp2.d b/test/unittest/builtinvar/tst.vtimestamp2.d
index 087a11e3..68a57ea8 100644
--- a/test/unittest/builtinvar/tst.vtimestamp2.d
+++ b/test/unittest/builtinvar/tst.vtimestamp2.d
@@ -4,7 +4,7 @@
  * Licensed under the Universal Permissive License v 1.0 as shown at
  * http://oss.oracle.com/licenses/upl.
  */
-/* @@xfail: dtv2 */
+/* @@xfail: dtv2: need vtimestamp support */
 
 /*
  * ASSERTION:
diff --git a/test/unittest/lquantize/tst.32bit-bug26268136.sh b/test/unittest/lquantize/tst.32bit-bug26268136.sh
index d5f143f5..91f4b65c 100755
--- a/test/unittest/lquantize/tst.32bit-bug26268136.sh
+++ b/test/unittest/lquantize/tst.32bit-bug26268136.sh
@@ -5,7 +5,34 @@
 # Licensed under the Universal Permissive License v 1.0 as shown at
 # http://oss.oracle.com/licenses/upl.
 #
-# @@xfail: dtv2
+# @@xfail: dtv2 since BPF verifier fails, see test source code
+
+# This test passes with BEGIN or even proc:::start but BPF verifier fails with proc:::exit.
+#
+# When the BPF verifier dives into
+#     uint64_t *dt_get_agg(const dt_dctx_t *dctx, uint32_t id, const char *key, uint64_t ival, const char *dflt) {
+#         uint64_t *genp;
+#         uint64_t *valp;
+#
+#         /* get the gen value */
+#         genp = bpf_map_lookup_elem(&agggen, &id);
+#         if (genp == 0)
+#             return dt_no_agg();
+#
+#         /* place the variable ID at the beginning of the key */
+#         *(uint32_t *)key = id;
+#
+#         /* try to look up the key */
+#         valp = bpf_map_lookup_elem(dctx->agg, key);
+# it tries to look up the key.  For the first argument (dctx->agg, where DCTX_AGG==64),
+# the BPF verifier shows
+#     (79) r1 = *(u64 *)(r8 +64)      R1= invP(id=0)
+# Since nothing is known about the first argument, the BPF verifier complains about
+# bpf_map_lookup_elem():
+#     R1 type=inv expected=map_ptr
+# In contrast, with proc:::start, we get
+#     (79) r1 = *(u64 *)(r8 +64)      R1= map_ptr(id=0,off=0,ks=12,vs=56,imm=0)
+# and the BPF verifier is then happy with the bpf_map_lookup_elem() call.
 
 if [ $# != 1 ]; then
 	echo expected one argument: '<'dtrace-path'>'
diff --git a/test/unittest/printa/err.D_PRINTF_ARG_TYPE.jstack.d b/test/unittest/printa/err.D_PRINTF_ARG_TYPE.jstack.d
index f04f9778..d2fccff7 100644
--- a/test/unittest/printa/err.D_PRINTF_ARG_TYPE.jstack.d
+++ b/test/unittest/printa/err.D_PRINTF_ARG_TYPE.jstack.d
@@ -4,7 +4,7 @@
  * Licensed under the Universal Permissive License v 1.0 as shown at
  * http://oss.oracle.com/licenses/upl.
  */
-/* @@xfail: dtv2 */
+/* @@xfail: dtv2 jstack not implemented */
 BEGIN
 {
 	@[jstack()] = count();
diff --git a/test/unittest/printa/tst.jstack.d b/test/unittest/printa/tst.jstack.d
index 4ef1ed58..7a404a09 100644
--- a/test/unittest/printa/tst.jstack.d
+++ b/test/unittest/printa/tst.jstack.d
@@ -4,7 +4,7 @@
  * Licensed under the Universal Permissive License v 1.0 as shown at
  * http://oss.oracle.com/licenses/upl.
  */
-/* @@xfail: dtv2 */
+/* @@xfail: dtv2 jstack not implemented */
 
 BEGIN
 {
diff --git a/test/unittest/ustack/tst.jstack_unprintable-bug26045010.sh b/test/unittest/ustack/tst.jstack_unprintable-bug26045010.sh
index c5fca2a5..e6d8b43c 100755
--- a/test/unittest/ustack/tst.jstack_unprintable-bug26045010.sh
+++ b/test/unittest/ustack/tst.jstack_unprintable-bug26045010.sh
@@ -5,7 +5,7 @@
 # Licensed under the Universal Permissive License v 1.0 as shown at
 # http://oss.oracle.com/licenses/upl.
 #
-# @@xfail: dtv2
+# @@xfail: dtv2 jstack not implemented
 if [ $# != 1 ]; then
 	echo expected one argument: '<'dtrace-path'>'
 	exit 2
diff --git a/test/unittest/variables/bvar/tst.ipl.d b/test/unittest/variables/bvar/tst.ipl.d
index a61e3b5b..7e2a432f 100644
--- a/test/unittest/variables/bvar/tst.ipl.d
+++ b/test/unittest/variables/bvar/tst.ipl.d
@@ -4,7 +4,7 @@
  * Licensed under the Universal Permissive License v 1.0 as shown at
  * http://oss.oracle.com/licenses/upl.
  */
-/* @@xfail: dtv2 */
+/* @@xfail: dtv2 need ipl support */
 
 /*
  * ASSERTION: The 'ipl' variable can be accessed and is not -1.
diff --git a/test/unittest/variables/bvar/tst.vtimestamp.d b/test/unittest/variables/bvar/tst.vtimestamp.d
index dc985ad1..b72eb8a7 100644
--- a/test/unittest/variables/bvar/tst.vtimestamp.d
+++ b/test/unittest/variables/bvar/tst.vtimestamp.d
@@ -4,7 +4,7 @@
  * Licensed under the Universal Permissive License v 1.0 as shown at
  * http://oss.oracle.com/licenses/upl.
  */
-/* @@xfail: dtv2 */
+/* @@xfail: dtv2 need vtimestamp support */
 
 /*
  * ASSERTION: The 'vtimestamp' variable can be accessed and is not -1.
-- 
2.43.5

Re: [PATCH 20/22] test: Annotate some XFAILs

[Kris Van Hees]

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

I am applying this for all tests
EXCEPT test/unittest/lquantize/tst.32bit-bug26268136.sh

That test is supposed to exercise a reported bug concerning overflow.  But
the test commit message is being updated to indicate that the test does pass
with BEGIN or proc:::start as trigger probe, but fails with proc:::exit.
That would mean that the test verifies correctly that the bug has been fixed.
But it also indicates that there is another (almost certainly unrelated) bug 
that causes bad code to be generated if the trigger probe is proc:::exit.

So, this test should be adjusted to use a trigger probe that works and a *NEW*
test should be added to exercise the problem that is being seen with the
proc:::exit probe.

There, I'm applying the patch changes to all tests except that one, and a new
patch should be submitteed for review to address the issue with the one test.

On Thu, Aug 29, 2024 at 01:22:17AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> ---
>  test/unittest/assocs/tst.invalidref.d         |  2 +-
>  test/unittest/builtinvar/tst.ipl.d            |  2 +-
>  test/unittest/builtinvar/tst.ipl1.d           |  2 +-
>  test/unittest/builtinvar/tst.vtimestamp.d     |  2 +-
>  test/unittest/builtinvar/tst.vtimestamp2.d    |  2 +-
>  .../lquantize/tst.32bit-bug26268136.sh        | 29 ++++++++++++++++++-
>  .../printa/err.D_PRINTF_ARG_TYPE.jstack.d     |  2 +-
>  test/unittest/printa/tst.jstack.d             |  2 +-
>  .../tst.jstack_unprintable-bug26045010.sh     |  2 +-
>  test/unittest/variables/bvar/tst.ipl.d        |  2 +-
>  test/unittest/variables/bvar/tst.vtimestamp.d |  2 +-
>  11 files changed, 38 insertions(+), 11 deletions(-)
> 
> diff --git a/test/unittest/assocs/tst.invalidref.d b/test/unittest/assocs/tst.invalidref.d
> index 0ed17959..34367c6b 100644
> --- a/test/unittest/assocs/tst.invalidref.d
> +++ b/test/unittest/assocs/tst.invalidref.d
> @@ -4,7 +4,7 @@
>   * Licensed under the Universal Permissive License v 1.0 as shown at
>   * http://oss.oracle.com/licenses/upl.
>   */
> -/* @@xfail: dtv2 */
> +/* @@xfail: dtv2 no support for index of assoc array; last_cmds[1][3]=0 dumps BPF */
>  
>  /*
>   * Test to ensure that invalid stores to a global associative array
> diff --git a/test/unittest/builtinvar/tst.ipl.d b/test/unittest/builtinvar/tst.ipl.d
> index c2b9850f..41a89b00 100644
> --- a/test/unittest/builtinvar/tst.ipl.d
> +++ b/test/unittest/builtinvar/tst.ipl.d
> @@ -4,7 +4,7 @@
>   * Licensed under the Universal Permissive License v 1.0 as shown at
>   * http://oss.oracle.com/licenses/upl.
>   */
> -/* @@xfail: dtv2 */
> +/* @@xfail: dtv2: need ipl support */
>  
>  /*
>   * ASSERTION:
> diff --git a/test/unittest/builtinvar/tst.ipl1.d b/test/unittest/builtinvar/tst.ipl1.d
> index 1b489229..654b16ce 100644
> --- a/test/unittest/builtinvar/tst.ipl1.d
> +++ b/test/unittest/builtinvar/tst.ipl1.d
> @@ -4,7 +4,7 @@
>   * Licensed under the Universal Permissive License v 1.0 as shown at
>   * http://oss.oracle.com/licenses/upl.
>   */
> -/* @@xfail: dtv2 */
> +/* @@xfail: dtv2: need ipl support */
>  
>  /*
>   * ASSERTION:
> diff --git a/test/unittest/builtinvar/tst.vtimestamp.d b/test/unittest/builtinvar/tst.vtimestamp.d
> index c9e2a112..11ffae7b 100644
> --- a/test/unittest/builtinvar/tst.vtimestamp.d
> +++ b/test/unittest/builtinvar/tst.vtimestamp.d
> @@ -4,7 +4,7 @@
>   * Licensed under the Universal Permissive License v 1.0 as shown at
>   * http://oss.oracle.com/licenses/upl.
>   */
> -/* @@xfail: dtv2 */
> +/* @@xfail: dtv2: need vtimestamp support */
>  
>  /*
>   * ASSERTION:
> diff --git a/test/unittest/builtinvar/tst.vtimestamp2.d b/test/unittest/builtinvar/tst.vtimestamp2.d
> index 087a11e3..68a57ea8 100644
> --- a/test/unittest/builtinvar/tst.vtimestamp2.d
> +++ b/test/unittest/builtinvar/tst.vtimestamp2.d
> @@ -4,7 +4,7 @@
>   * Licensed under the Universal Permissive License v 1.0 as shown at
>   * http://oss.oracle.com/licenses/upl.
>   */
> -/* @@xfail: dtv2 */
> +/* @@xfail: dtv2: need vtimestamp support */
>  
>  /*
>   * ASSERTION:
> diff --git a/test/unittest/lquantize/tst.32bit-bug26268136.sh b/test/unittest/lquantize/tst.32bit-bug26268136.sh
> index d5f143f5..91f4b65c 100755
> --- a/test/unittest/lquantize/tst.32bit-bug26268136.sh
> +++ b/test/unittest/lquantize/tst.32bit-bug26268136.sh
> @@ -5,7 +5,34 @@
>  # Licensed under the Universal Permissive License v 1.0 as shown at
>  # http://oss.oracle.com/licenses/upl.
>  #
> -# @@xfail: dtv2
> +# @@xfail: dtv2 since BPF verifier fails, see test source code
> +
> +# This test passes with BEGIN or even proc:::start but BPF verifier fails with proc:::exit.
> +#
> +# When the BPF verifier dives into
> +#     uint64_t *dt_get_agg(const dt_dctx_t *dctx, uint32_t id, const char *key, uint64_t ival, const char *dflt) {
> +#         uint64_t *genp;
> +#         uint64_t *valp;
> +#
> +#         /* get the gen value */
> +#         genp = bpf_map_lookup_elem(&agggen, &id);
> +#         if (genp == 0)
> +#             return dt_no_agg();
> +#
> +#         /* place the variable ID at the beginning of the key */
> +#         *(uint32_t *)key = id;
> +#
> +#         /* try to look up the key */
> +#         valp = bpf_map_lookup_elem(dctx->agg, key);
> +# it tries to look up the key.  For the first argument (dctx->agg, where DCTX_AGG==64),
> +# the BPF verifier shows
> +#     (79) r1 = *(u64 *)(r8 +64)      R1= invP(id=0)
> +# Since nothing is known about the first argument, the BPF verifier complains about
> +# bpf_map_lookup_elem():
> +#     R1 type=inv expected=map_ptr
> +# In contrast, with proc:::start, we get
> +#     (79) r1 = *(u64 *)(r8 +64)      R1= map_ptr(id=0,off=0,ks=12,vs=56,imm=0)
> +# and the BPF verifier is then happy with the bpf_map_lookup_elem() call.
>  
>  if [ $# != 1 ]; then
>  	echo expected one argument: '<'dtrace-path'>'
> diff --git a/test/unittest/printa/err.D_PRINTF_ARG_TYPE.jstack.d b/test/unittest/printa/err.D_PRINTF_ARG_TYPE.jstack.d
> index f04f9778..d2fccff7 100644
> --- a/test/unittest/printa/err.D_PRINTF_ARG_TYPE.jstack.d
> +++ b/test/unittest/printa/err.D_PRINTF_ARG_TYPE.jstack.d
> @@ -4,7 +4,7 @@
>   * Licensed under the Universal Permissive License v 1.0 as shown at
>   * http://oss.oracle.com/licenses/upl.
>   */
> -/* @@xfail: dtv2 */
> +/* @@xfail: dtv2 jstack not implemented */
>  BEGIN
>  {
>  	@[jstack()] = count();
> diff --git a/test/unittest/printa/tst.jstack.d b/test/unittest/printa/tst.jstack.d
> index 4ef1ed58..7a404a09 100644
> --- a/test/unittest/printa/tst.jstack.d
> +++ b/test/unittest/printa/tst.jstack.d
> @@ -4,7 +4,7 @@
>   * Licensed under the Universal Permissive License v 1.0 as shown at
>   * http://oss.oracle.com/licenses/upl.
>   */
> -/* @@xfail: dtv2 */
> +/* @@xfail: dtv2 jstack not implemented */
>  
>  BEGIN
>  {
> diff --git a/test/unittest/ustack/tst.jstack_unprintable-bug26045010.sh b/test/unittest/ustack/tst.jstack_unprintable-bug26045010.sh
> index c5fca2a5..e6d8b43c 100755
> --- a/test/unittest/ustack/tst.jstack_unprintable-bug26045010.sh
> +++ b/test/unittest/ustack/tst.jstack_unprintable-bug26045010.sh
> @@ -5,7 +5,7 @@
>  # Licensed under the Universal Permissive License v 1.0 as shown at
>  # http://oss.oracle.com/licenses/upl.
>  #
> -# @@xfail: dtv2
> +# @@xfail: dtv2 jstack not implemented
>  if [ $# != 1 ]; then
>  	echo expected one argument: '<'dtrace-path'>'
>  	exit 2
> diff --git a/test/unittest/variables/bvar/tst.ipl.d b/test/unittest/variables/bvar/tst.ipl.d
> index a61e3b5b..7e2a432f 100644
> --- a/test/unittest/variables/bvar/tst.ipl.d
> +++ b/test/unittest/variables/bvar/tst.ipl.d
> @@ -4,7 +4,7 @@
>   * Licensed under the Universal Permissive License v 1.0 as shown at
>   * http://oss.oracle.com/licenses/upl.
>   */
> -/* @@xfail: dtv2 */
> +/* @@xfail: dtv2 need ipl support */
>  
>  /*
>   * ASSERTION: The 'ipl' variable can be accessed and is not -1.
> diff --git a/test/unittest/variables/bvar/tst.vtimestamp.d b/test/unittest/variables/bvar/tst.vtimestamp.d
> index dc985ad1..b72eb8a7 100644
> --- a/test/unittest/variables/bvar/tst.vtimestamp.d
> +++ b/test/unittest/variables/bvar/tst.vtimestamp.d
> @@ -4,7 +4,7 @@
>   * Licensed under the Universal Permissive License v 1.0 as shown at
>   * http://oss.oracle.com/licenses/upl.
>   */
> -/* @@xfail: dtv2 */
> +/* @@xfail: dtv2 need vtimestamp support */
>  
>  /*
>   * ASSERTION: The 'vtimestamp' variable can be accessed and is not -1.
> -- 
> 2.43.5
> 

[PATCH 21/22] test: Fix DIRNAME

[eugene.loh]

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

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 test/unittest/dtrace-util/tst.ListProbesNameUSDT.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/unittest/dtrace-util/tst.ListProbesNameUSDT.sh b/test/unittest/dtrace-util/tst.ListProbesNameUSDT.sh
index c5dfc72b..f9b1d8bf 100755
--- a/test/unittest/dtrace-util/tst.ListProbesNameUSDT.sh
+++ b/test/unittest/dtrace-util/tst.ListProbesNameUSDT.sh
@@ -26,7 +26,7 @@ dtrace=$1
 CC=/usr/bin/gcc
 CFLAGS=
 
-DIRNAME="$tmpdir/list-probes-func-usdt.$$.$RANDOM"
+DIRNAME="$tmpdir/list-probes-name-usdt.$$.$RANDOM"
 mkdir -p $DIRNAME
 cd $DIRNAME
 
-- 
2.43.5

Re: [DTrace-devel] [PATCH 21/22] test: Fix DIRNAME

[Sam James]

"eugene.loh--- via DTrace-devel" <dtrace-devel@oss.oracle.com> writes:

> From: Eugene Loh <eugene.loh@oracle.com>
>
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> ---
>  test/unittest/dtrace-util/tst.ListProbesNameUSDT.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/test/unittest/dtrace-util/tst.ListProbesNameUSDT.sh b/test/unittest/dtrace-util/tst.ListProbesNameUSDT.sh
> index c5dfc72b..f9b1d8bf 100755
> --- a/test/unittest/dtrace-util/tst.ListProbesNameUSDT.sh
> +++ b/test/unittest/dtrace-util/tst.ListProbesNameUSDT.sh
> @@ -26,7 +26,7 @@ dtrace=$1
>  CC=/usr/bin/gcc
>  CFLAGS=
>  
> -DIRNAME="$tmpdir/list-probes-func-usdt.$$.$RANDOM"
> +DIRNAME="$tmpdir/list-probes-name-usdt.$$.$RANDOM"
>  mkdir -p $DIRNAME
>  cd $DIRNAME

Feel like this is crying out for $(mktemp -d) instead in both this and
the test it was copied from...

Re: [PATCH 21/22] test: Fix DIRNAME

[Kris Van Hees]

On Thu, Aug 29, 2024 at 01:22:18AM -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>

> ---
>  test/unittest/dtrace-util/tst.ListProbesNameUSDT.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/test/unittest/dtrace-util/tst.ListProbesNameUSDT.sh b/test/unittest/dtrace-util/tst.ListProbesNameUSDT.sh
> index c5dfc72b..f9b1d8bf 100755
> --- a/test/unittest/dtrace-util/tst.ListProbesNameUSDT.sh
> +++ b/test/unittest/dtrace-util/tst.ListProbesNameUSDT.sh
> @@ -26,7 +26,7 @@ dtrace=$1
>  CC=/usr/bin/gcc
>  CFLAGS=
>  
> -DIRNAME="$tmpdir/list-probes-func-usdt.$$.$RANDOM"
> +DIRNAME="$tmpdir/list-probes-name-usdt.$$.$RANDOM"
>  mkdir -p $DIRNAME
>  cd $DIRNAME
>  
> -- 
> 2.43.5
> 

[PATCH 22/22] test: Update tst.newprobes.sh xfail message

[eugene.loh]

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

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 test/unittest/pid/tst.newprobes.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/unittest/pid/tst.newprobes.sh b/test/unittest/pid/tst.newprobes.sh
index a4944688..1a88c3a8 100755
--- a/test/unittest/pid/tst.newprobes.sh
+++ b/test/unittest/pid/tst.newprobes.sh
@@ -6,7 +6,7 @@
 # http://oss.oracle.com/licenses/upl.
 #
 
-# @@xfail: not working yet
+# @@xfail: we do not support wildcard pids for pid probes
 
 if [ $# != 1 ]; then
 	echo expected one argument: '<'dtrace-path'>'
-- 
2.43.5

Re: [PATCH 22/22] test: Update tst.newprobes.sh xfail message

[Kris Van Hees]

On Thu, Aug 29, 2024 at 01:22:19AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> ---
>  test/unittest/pid/tst.newprobes.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/test/unittest/pid/tst.newprobes.sh b/test/unittest/pid/tst.newprobes.sh
> index a4944688..1a88c3a8 100755
> --- a/test/unittest/pid/tst.newprobes.sh
> +++ b/test/unittest/pid/tst.newprobes.sh
> @@ -6,7 +6,7 @@
>  # http://oss.oracle.com/licenses/upl.
>  #
>  
> -# @@xfail: not working yet
> +# @@xfail: we do not support wildcard pids for pid probes

I think it would be more accurate to say that 'dtrace' does not support
wildcards pids for pid probe.  It is not our choice or lack of support in
our implementation - dtrace never did support it.

Updated it to be:

   # @@xfail: dtrace does not support wildcard pids for pid probes

>  
>  if [ $# != 1 ]; then
>  	echo expected one argument: '<'dtrace-path'>'
> -- 
> 2.43.5
> 

Re: [PATCH 01/22] test: Handle dtrace:::ERROR arg3 specially

[Kris Van Hees]

On Thu, Aug 29, 2024 at 01:21:58AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> The ERROR probe's arg3 reports the culprit PC, whose value can vary
> with minor implementation changes.  On the one hand, we do not want
> tests to be overly sensitive to this value.  On the other hand, we
> do at least want to check the value is reasonable.
> 
> Therefore:
> 
> *)  Change tests that dump ERROR's args to omit arg3.
> 
> *)  Add a new test that checks that ERROR's arg3 is reasonable.
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
>
> ---
>  test/unittest/error/tst.DTRACEFLT_UNKNOWN.d   |  6 +-
>  test/unittest/error/tst.DTRACEFLT_UNKNOWN.r   |  2 +-
>  .../regression/tst.DTRACEFLT_BADADDR.d_path.d |  8 +-
>  .../regression/tst.DTRACEFLT_BADADDR.d_path.r |  2 +-
>  test/unittest/variables/bvar/tst.arg3-ERROR.d | 92 +++++++++++++++++++
>  5 files changed, 101 insertions(+), 9 deletions(-)
>  create mode 100644 test/unittest/variables/bvar/tst.arg3-ERROR.d
> 
> diff --git a/test/unittest/error/tst.DTRACEFLT_UNKNOWN.d b/test/unittest/error/tst.DTRACEFLT_UNKNOWN.d
> index 001903ff..bfc77bf5 100644
> --- a/test/unittest/error/tst.DTRACEFLT_UNKNOWN.d
> +++ b/test/unittest/error/tst.DTRACEFLT_UNKNOWN.d
> @@ -1,6 +1,6 @@
>  /*
>   * Oracle Linux DTrace.
> - * Copyright (c) 2006, 2020, 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.
>   */
> @@ -19,8 +19,8 @@
>  
>  ERROR
>  {
> -	printf("The arguments are %u %u %u %u %u\n",
> -		arg1, arg2, arg3, arg4, arg5);
> +	printf("The arguments are %u %u PC %u %u\n",
> +		arg1, arg2, arg4, arg5);
>  	printf("The value of arg4 = %u\n", DTRACEFLT_UNKNOWN);
>  	exit(0);
>  }
> diff --git a/test/unittest/error/tst.DTRACEFLT_UNKNOWN.r b/test/unittest/error/tst.DTRACEFLT_UNKNOWN.r
> index b11f6c99..3e7caac4 100644
> --- a/test/unittest/error/tst.DTRACEFLT_UNKNOWN.r
> +++ b/test/unittest/error/tst.DTRACEFLT_UNKNOWN.r
> @@ -1,4 +1,4 @@
> -The arguments are 2 2 4 1 64
> +The arguments are 2 2 PC 1 64
>  The value of arg4 = 0
>  
>  -- @@stderr --
> diff --git a/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.d b/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.d
> index c23f9503..ec519f4f 100644
> --- a/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.d
> +++ b/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.d
> @@ -1,10 +1,10 @@
>  /*
>   * Oracle Linux DTrace.
> - * Copyright (c) 2015, 2020, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2015, 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.
>   */
> -/* @@xfail: dtv2 */
> +/* @@xfail: dtv2 d_path */
>  
>  /*
>   * ASSERTION:
> @@ -18,8 +18,7 @@
>  
>  ERROR
>  {
> -	printf("The arguments are %u %u %u %u %u\n",
> -		arg1, arg2, arg3, arg4, arg5);
> +	printf("The arguments are %u %u PC %u %u\n", arg1, arg2, arg4, arg5);
>  	printf("The value of arg4 should be %u\n", DTRACEFLT_BADADDR);
>  	printf("The value of arg5 should be %u\n", 0x18);
>  	exit(0);
> @@ -29,4 +28,5 @@ BEGIN
>  {
>  	d = d_path((struct path *)0x18);
>  	trace(d);
> +	exit(1);
>  }
> diff --git a/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.r b/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.r
> index 8c601a43..be1f6d5b 100644
> --- a/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.r
> +++ b/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.r
> @@ -1,4 +1,4 @@
> -The arguments are 2 1 28 1 24
> +The arguments are 2 1 PC 1 24
>  The value of arg4 should be 1
>  The value of arg5 should be 24
>  
> diff --git a/test/unittest/variables/bvar/tst.arg3-ERROR.d b/test/unittest/variables/bvar/tst.arg3-ERROR.d
> new file mode 100644
> index 00000000..6c2f5206
> --- /dev/null
> +++ b/test/unittest/variables/bvar/tst.arg3-ERROR.d
> @@ -0,0 +1,92 @@
> +/*
> + * 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.
> + */
> +
> +/*
> + * ASSERTION: The 'arg3' variable can be accessed in the ERROR probe
> + *	and its value (PC) is reasonable.
> + *
> + * SECTION: Variables/Built-in Variables/arg3
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> +	nerrs = 0;
> +}
> +
> +BEGIN
> +{
> +	x = (int *)64;
> +	y = *x;                       /* Error at PC[0] */
> +	trace(y);
> +}
> +
> +BEGIN
> +{
> +	x = (int *)64;
> +	y = *x;                       /* Error at PC[1] */
> +	trace(y);
> +}
> +
> +BEGIN
> +{
> +	x = (int *)64;
> +	y = *x;                       /* Error at PC[2] */
> +	trace(y);
> +}
> +
> +BEGIN
> +{
> +	x = (int *)64;
> +	y = *x;                       /* Error at PC[3] */
> +	trace(y);
> +}
> +
> +ERROR
> +{
> +	/* Record the problematic PC and continue execution. */
> +	PC[nerrs++] = arg3;
> +}
> +
> +/*
> + * The problematic PCs are likely to satisfy the following
> + * reasonable checks, though it's possible that some radically
> + * different implementation in the future might violate one or
> + * more checks.
> + *
> + * The first problematic PC is expected in the some range.

"in the same range" ??  Changing to "some range".

> + *
> + * The next problematic PC is expected to be PC[0] plus some
> + * delta, including some special functions that are loaded.
> + *
> + * Then, the next PC is expected to be PC[1] plus some delta
> + * that is smaller and narrower since those special functions
> + * do not need to be reloaded.
> + *
> + * The last PC is expected to be PC[2] plus some predictable,
> + * small and narrow, delta PC[2]-PC[1].
> + */
> +BEGIN
> +/PC[0] >   100 &&
> + PC[0] <  2000 &&
> + PC[1] >  PC[0] +  100 &&
> + PC[1] <  PC[0] + 2000 &&
> + PC[2] >  PC[1] +   30 &&
> + PC[2] <  PC[1] +  500 &&
> + PC[3] == PC[2] + (PC[2] - PC[1])/

These ranges seem quite big.  I realize that code generator changes and changes
to linked in code can cause these error locations to drift over time, but I
think I'd rather have smaller ranges with the downfall of needing to update the
test at some point rather than having ranges that are so big that the test
itself may not really verify much anymore.

For that matter, perhaps it would be better to rework the test to actually
compare the error locations with the disassembler output because we know the
mechanism that sets the PC for the error location and thus we know what
instructions to expect there.  That would allow exact verification of the PC
values.

> +{
> +	printf("PCs: %d %d %d %d\n", PC[0], PC[1], PC[2], PC[3]);
> +	exit(0);
> +}
> +
> +BEGIN
> +{
> +	printf("ERROR!  PCs do not seem reasonable: %d %d %d %d\n",
> +		   PC[0], PC[1], PC[2], PC[3]);
> +	exit(1);
> +}
> -- 
> 2.43.5
>