[PATCH 1/4] Remove orphaned dtrace_recdesc_t component dtrd_uarg

[PATCH 1/4] Remove orphaned dtrace_recdesc_t component dtrd_uarg

[eugene.loh]

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

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

diff --git a/include/dtrace/metadesc.h b/include/dtrace/metadesc.h
index 8a4add255..b0f789932 100644
--- a/include/dtrace/metadesc.h
+++ b/include/dtrace/metadesc.h
@@ -2,7 +2,7 @@
  * Licensed under the Universal Permissive License v 1.0 as shown at
  * http://oss.oracle.com/licenses/upl.
  *
- * Copyright (c) 2009, 2024, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2009, 2025, Oracle and/or its affiliates. All rights reserved.
  */
 
 /*
@@ -39,7 +39,6 @@ typedef struct dtrace_recdesc {
 	uint16_t dtrd_alignment;		/* required alignment */
 	void *dtrd_format;			/* format, if any */
 	uint64_t dtrd_arg;			/* action argument */
-	uint64_t dtrd_uarg;			/* user argument */
 } dtrace_recdesc_t;
 
 typedef struct dtrace_datadesc {
-- 
2.43.5

[PATCH 2/4] test: Skip trace() of a 1-byte struct

[eugene.loh]

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

With commit 3a551bfd ("trace: fix char-array handling"), this test
started to FAIL.  Meanwhile, the behavior of trace() on a 1-byte
struct is poorly defined.  Users wishing clear semantics should use
print() or other actions.

Skip the test.

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 .../actions/trace/tst.struct-1-byte.d         | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/test/unittest/actions/trace/tst.struct-1-byte.d b/test/unittest/actions/trace/tst.struct-1-byte.d
index 36de485f8..8bfcbdd53 100644
--- a/test/unittest/actions/trace/tst.struct-1-byte.d
+++ b/test/unittest/actions/trace/tst.struct-1-byte.d
@@ -5,6 +5,30 @@
  * http://oss.oracle.com/licenses/upl.
  */
 
+/*
+ * With Solaris or legacy DTrace on Linux, the script gives
+ *       52
+ * That is, contents are dumped as an integer because the trace()
+ * argument is 1, 2, 4, or 8 bytes -- specifically, it is 1 byte.
+ *
+ * Before commit 3a551bfd ("trace: fix char-array handling"), we got
+ *           0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f  0123456789abcdef
+ *       0: 34                                               4
+ * That is, contents were dumped as raw bytes.
+ *
+ * With that patch, we get:
+ *       4
+ * That is, contents are dumped as the string "4" since the contents
+ * are a printable char followed optionally by NUL bytes.
+ *
+ * The truth is that the semantics of trace() are poorly defined.
+ * So we are hard-pressed to say what is correct.  The test has
+ * little value.  Skip it.
+ *
+ * Users who want type-aware printing can use the print() action.
+ */
+/* @@skip: poorly defined semantics */
+
 /*
  * ASSERTION: The trace() action prints a struct { int8_t } correctly.
  *
-- 
2.43.5

[PATCH 3/4] test: Update some char-array results files

[eugene.loh]

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

A few tests started failing with commit 3a551bfd
("trace: fix char-array handling").  Update the results files to
reflect older behavior, whether on Solaris or with the legacy
Linux DTrace implementation.

Notice that test/unittest/funcs/substr/tst.substr-large-idx.d
specifies strsize=256.  The older behavior would result in 256
chars being shown.  We add a results file that includes a 257th
char.

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 .../funcs/substr/tst.substr-large-idx.r       | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/test/unittest/funcs/substr/tst.substr-large-idx.r b/test/unittest/funcs/substr/tst.substr-large-idx.r
index 8b1378917..79fecbd32 100644
--- a/test/unittest/funcs/substr/tst.substr-large-idx.r
+++ b/test/unittest/funcs/substr/tst.substr-large-idx.r
@@ -1 +1,20 @@
 
+             0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f  0123456789abcdef
+         0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
+        10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
+        20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
+        30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
+        40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
+        50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
+        60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
+        70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
+        80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
+        90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
+        a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
+        b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
+        c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
+        d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
+        e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
+        f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
+       100: 00                                               .
+
-- 
2.43.5

[PATCH 4/4] Pad strings in the output buffer with NUL bytes after terminating byte

[eugene.loh]

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

The consumer checks if there are non-NUL bytes after the terminating
NUL byte to decide whether to print the contents of the output buffer
as a string or as raw bytes.  So, for strings, make sure that the
string is padded with NUL bytes.

A robust test that shows the problem can be hard to devise.  Some tests
encounter the problem.  This patch adds another test that might show
the problem.

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 libdtrace/dt_cg.c                      | 46 ++++++++++++++++++--------
 test/unittest/error/tst.trace_string.d | 26 +++++++++++++++
 2 files changed, 59 insertions(+), 13 deletions(-)
 create mode 100644 test/unittest/error/tst.trace_string.d

diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
index 9b3592b9c..6dcf4cd3d 100644
--- a/libdtrace/dt_cg.c
+++ b/libdtrace/dt_cg.c
@@ -1596,6 +1596,22 @@ dt_cg_check_ptr_arg(dt_irlist_t *dlp, dt_regset_t *drp, dt_node_t *dnp,
 
 static void dt_cg_setx(dt_irlist_t *dlp, int reg, uint64_t x);
 
+/*
+ * Store a pointer to the 'memory block of zeros' in reg.
+ */
+static void
+dt_cg_zerosptr(int reg, dt_irlist_t *dlp, dt_regset_t *drp)
+{
+	dtrace_hdl_t	*dtp = yypcb->pcb_hdl;
+	dt_ident_t	*zero_off = dt_dlib_get_var(dtp, "ZERO_OFF");
+
+	dt_cg_access_dctx(reg, dlp, drp, DCTX_STRTAB);
+	emite(dlp, BPF_ALU64_IMM(BPF_ADD, reg, -1), zero_off);
+}
+
+/*
+ * Store a value to the output buffer.
+ */
 static int
 dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
 		dt_pfargv_t *pfp, int arg)
@@ -1676,6 +1692,7 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
 
 		goto ok;
 	} else if (dt_node_is_string(dnp)) {
+		uint_t	lbl_ok = dt_irlist_label(dlp);
 		size_t	strsize = dtp->dt_options[DTRACEOPT_STRSIZE];
 
 		if (!not_null)
@@ -1702,6 +1719,22 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
 		dt_regset_xalloc(drp, BPF_REG_0);
 		emit(dlp, BPF_CALL_HELPER(BPF_FUNC_probe_read_str));
 		dt_regset_free_args(drp);
+
+		/*
+		 * Pad the rest with zeroes, if necessary.
+		 */
+		emit(dlp,  BPF_BRANCH_IMM(BPF_JGE, BPF_REG_0, strsize + 1, lbl_ok));
+		if (dt_regset_xalloc_args(drp) == -1)
+			longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
+		emit(dlp,  BPF_MOV_REG(BPF_REG_1, BPF_REG_9));
+		emit(dlp,  BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, off));
+		emit(dlp,  BPF_ALU64_REG(BPF_ADD, BPF_REG_1, BPF_REG_0));
+		emit(dlp,  BPF_MOV_IMM(BPF_REG_2, strsize + 1));
+		emit(dlp,  BPF_ALU64_REG(BPF_SUB, BPF_REG_2, BPF_REG_0));
+		dt_cg_zerosptr(BPF_REG_3, dlp, drp);
+		emit(dlp,  BPF_CALL_HELPER(dtp->dt_bpfhelper[BPF_FUNC_probe_read_kernel]));
+		dt_regset_free_args(drp);
+		emitl(dlp, lbl_ok, BPF_NOP());
 		dt_regset_free(drp, BPF_REG_0);
 		TRACE_REGSET("store_val(): End   ");
 
@@ -3042,19 +3075,6 @@ dt_cg_pop_stack(int reg, dt_irlist_t *dlp, dt_regset_t *drp)
 	dt_regset_free(drp, treg);
 }
 
-/*
- * Store a pointer to the 'memory block of zeros' in reg.
- */
-static void
-dt_cg_zerosptr(int reg, dt_irlist_t *dlp, dt_regset_t *drp)
-{
-	dtrace_hdl_t	*dtp = yypcb->pcb_hdl;
-	dt_ident_t	*zero_off = dt_dlib_get_var(dtp, "ZERO_OFF");
-
-	dt_cg_access_dctx(reg, dlp, drp, DCTX_STRTAB);
-	emite(dlp, BPF_ALU64_IMM(BPF_ADD, reg, -1), zero_off);
-}
-
 /*
  * Generate code to promote signed scalars (size < 64 bits) to native register
  * size (64 bits).
diff --git a/test/unittest/error/tst.trace_string.d b/test/unittest/error/tst.trace_string.d
new file mode 100644
index 000000000..4b06aef88
--- /dev/null
+++ b/test/unittest/error/tst.trace_string.d
@@ -0,0 +1,26 @@
+/*
+ * Oracle Linux DTrace.
+ * Copyright (c) 2025, 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: Test ERROR probe firing.
+ *
+ * SECTION: dtrace Provider
+ */
+
+#pragma D option quiet
+
+ERROR
+{
+	trace("Error fired");
+	exit(0);
+}
+
+BEGIN
+{
+	*(char *)NULL;
+	exit(1);
+}
-- 
2.43.5

Re: [PATCH 1/4] Remove orphaned dtrace_recdesc_t component dtrd_uarg

[Kris Van Hees]

On Tue, Mar 25, 2025 at 06:25:18PM -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/metadesc.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/include/dtrace/metadesc.h b/include/dtrace/metadesc.h
> index 8a4add255..b0f789932 100644
> --- a/include/dtrace/metadesc.h
> +++ b/include/dtrace/metadesc.h
> @@ -2,7 +2,7 @@
>   * Licensed under the Universal Permissive License v 1.0 as shown at
>   * http://oss.oracle.com/licenses/upl.
>   *
> - * Copyright (c) 2009, 2024, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2009, 2025, Oracle and/or its affiliates. All rights reserved.
>   */
>  
>  /*
> @@ -39,7 +39,6 @@ typedef struct dtrace_recdesc {
>  	uint16_t dtrd_alignment;		/* required alignment */
>  	void *dtrd_format;			/* format, if any */
>  	uint64_t dtrd_arg;			/* action argument */
> -	uint64_t dtrd_uarg;			/* user argument */
>  } dtrace_recdesc_t;
>  
>  typedef struct dtrace_datadesc {
> -- 
> 2.43.5
> 

Re: [PATCH 2/4] test: Skip trace() of a 1-byte struct

[Nick Alcock]

On 25 Mar 2025, eugene loh outgrape:

> From: Eugene Loh <eugene.loh@oracle.com>
>
> With commit 3a551bfd ("trace: fix char-array handling"), this test
> started to FAIL.  Meanwhile, the behavior of trace() on a 1-byte
> struct is poorly defined.  Users wishing clear semantics should use
> print() or other actions.

This makes trace() much, much less useful. I'd say NAK, if this means
we're going to not come up with any useful behaviour. Why not define
something, then use it?

Nobody is going to say "look at the size of something and then do a
print() rather than a trace() if it's too small". Even looking at this
commit I'm not sure what "too small" is (one byte? four bytes?
sizeof(int)? sizeof(long)? A cacheline? what?) so I don't see how our
users can be expected to.

Re: [PATCH 3/4] test: Update some char-array results files

[Nick Alcock]

On 25 Mar 2025, eugene loh said:

> From: Eugene Loh <eugene.loh@oracle.com>
>
> A few tests started failing with commit 3a551bfd
> ("trace: fix char-array handling").  Update the results files to
> reflect older behavior, whether on Solaris or with the legacy
> Linux DTrace implementation.

I think this commit didn't get reviewed because nobody understood what
this meant. If tests started failing, why adjust the results to reflect
*older* behaviour (presumably "before that change"?)

Or do you mean that commit 3a551bfd adjusted trace() to work like it
historically did, but one test was missed and needs correspondingly
adjusting?

> Notice that test/unittest/funcs/substr/tst.substr-large-idx.d
> specifies strsize=256.  The older behavior would result in 256
> chars being shown.  We add a results file that includes a 257th
> char.

(the trailing NUL, presumably.)

Re: [PATCH 4/4] Pad strings in the output buffer with NUL bytes after terminating byte

[Nick Alcock]

On 25 Mar 2025, eugene loh uttered the following:

> From: Eugene Loh <eugene.loh@oracle.com>
>
> The consumer checks if there are non-NUL bytes after the terminating
> NUL byte to decide whether to print the contents of the output buffer
> as a string or as raw bytes.  So, for strings, make sure that the
> string is padded with NUL bytes.

Ugh. I guess there's no choice if we want trace() to work properly.

A use for strncpy()! except that this is BPF :)

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

Reviewed-by: Nick Alcock <nick.alcock@oracle.com>

because basing decisions on uninitialized data is bad.

(Modulo nit below.)

> ---
>  libdtrace/dt_cg.c                      | 46 ++++++++++++++++++--------
>  test/unittest/error/tst.trace_string.d | 26 +++++++++++++++
>  2 files changed, 59 insertions(+), 13 deletions(-)
>  create mode 100644 test/unittest/error/tst.trace_string.d
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 9b3592b9c..6dcf4cd3d 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -1596,6 +1596,22 @@ dt_cg_check_ptr_arg(dt_irlist_t *dlp, dt_regset_t *drp, dt_node_t *dnp,
>  
>  static void dt_cg_setx(dt_irlist_t *dlp, int reg, uint64_t x);
>  
> +/*
> + * Store a pointer to the 'memory block of zeros' in reg.
> + */
> +static void
> +dt_cg_zerosptr(int reg, dt_irlist_t *dlp, dt_regset_t *drp)
> +{
> +	dtrace_hdl_t	*dtp = yypcb->pcb_hdl;
> +	dt_ident_t	*zero_off = dt_dlib_get_var(dtp, "ZERO_OFF");
> +
> +	dt_cg_access_dctx(reg, dlp, drp, DCTX_STRTAB);
> +	emite(dlp, BPF_ALU64_IMM(BPF_ADD, reg, -1), zero_off);
> +}
> +
> +/*
> + * Store a value to the output buffer.
> + */
>  static int
>  dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
>  		dt_pfargv_t *pfp, int arg)
> @@ -1676,6 +1692,7 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
>  
>  		goto ok;
>  	} else if (dt_node_is_string(dnp)) {
> +		uint_t	lbl_ok = dt_irlist_label(dlp);
>  		size_t	strsize = dtp->dt_options[DTRACEOPT_STRSIZE];
>  
>  		if (!not_null)
> @@ -1702,6 +1719,22 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
>  		dt_regset_xalloc(drp, BPF_REG_0);
>  		emit(dlp, BPF_CALL_HELPER(BPF_FUNC_probe_read_str));
>  		dt_regset_free_args(drp);
> +
> +		/*
> +		 * Pad the rest with zeroes, if necessary.
> +		 */
> +		emit(dlp,  BPF_BRANCH_IMM(BPF_JGE, BPF_REG_0, strsize + 1, lbl_ok));
> +		if (dt_regset_xalloc_args(drp) == -1)
> +			longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> +		emit(dlp,  BPF_MOV_REG(BPF_REG_1, BPF_REG_9));
> +		emit(dlp,  BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, off));
> +		emit(dlp,  BPF_ALU64_REG(BPF_ADD, BPF_REG_1, BPF_REG_0));
> +		emit(dlp,  BPF_MOV_IMM(BPF_REG_2, strsize + 1));
> +		emit(dlp,  BPF_ALU64_REG(BPF_SUB, BPF_REG_2, BPF_REG_0));
> +		dt_cg_zerosptr(BPF_REG_3, dlp, drp);
> +		emit(dlp,  BPF_CALL_HELPER(dtp->dt_bpfhelper[BPF_FUNC_probe_read_kernel]));

Much faster than using a loop in all but the smallest cases, I suppose.
(And the zero block is presumably always big enough... yes, it is.)

> diff --git a/test/unittest/error/tst.trace_string.d b/test/unittest/error/tst.trace_string.d
> new file mode 100644
> index 000000000..4b06aef88
> --- /dev/null
> +++ b/test/unittest/error/tst.trace_string.d
> @@ -0,0 +1,26 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2025, 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: Test ERROR probe firing.

... is that really what you want to describe this test as?

> + * SECTION: dtrace Provider
> + */
> +
> +#pragma D option quiet
> +
> +ERROR
> +{
> +	trace("Error fired");

.... given that its stated purpose is quite different, why not describe
that purpose?

Re: [PATCH 3/4] test: Update some char-array results files

[Eugene Loh]

On 7/22/25 09:51, Nick Alcock wrote:

> On 25 Mar 2025, eugene loh said:
>> From: Eugene Loh <eugene.loh@oracle.com>
>>
>> A few tests started failing with commit 3a551bfd
>> ("trace: fix char-array handling").  Update the results files to
>> reflect older behavior, whether on Solaris or with the legacy
>> Linux DTrace implementation.
> I think this commit didn't get reviewed because nobody understood what
> this meant.

There might be other issues.  There may be broader issues here about how 
we should handle strings.  But, I guess here we can focus on this patch.

> If tests started failing, why adjust the results to reflect
> *older* behaviour (presumably "before that change"?)
>
> Or do you mean that commit 3a551bfd adjusted trace() to work like it
> historically did, but one test was missed and needs correspondingly
> adjusting?

The 3a551bf patch made this test fail, but changing the .r file as 
indicated works both for historical behavior and for this new patch 
(modulo the 256/257 issue).  So, "yes" to your "Or do you mean?" 
question.  Would a change of commit message wording get your R-b? I'm 
not sure what to change since I thought it was evident from the patch 
what was going on.

>> Notice that test/unittest/funcs/substr/tst.substr-large-idx.d
>> specifies strsize=256.  The older behavior would result in 256
>> chars being shown.  We add a results file that includes a 257th
>> char.
> (the trailing NUL, presumably.)

Re: [PATCH 2/4] test: Skip trace() of a 1-byte struct

[Eugene Loh]

This patch is rescinded in deference to another patch that simply 
changes the .r file to match the current output.

On 7/22/25 09:46, Nick Alcock wrote:
> On 25 Mar 2025, eugene loh outgrape:
>
>> From: Eugene Loh <eugene.loh@oracle.com>
>>
>> With commit 3a551bfd ("trace: fix char-array handling"), this test
>> started to FAIL.  Meanwhile, the behavior of trace() on a 1-byte
>> struct is poorly defined.  Users wishing clear semantics should use
>> print() or other actions.
> This makes trace() much, much less useful. I'd say NAK, if this means
> we're going to not come up with any useful behaviour. Why not define
> something, then use it?
>
> Nobody is going to say "look at the size of something and then do a
> print() rather than a trace() if it's too small". Even looking at this
> commit I'm not sure what "too small" is (one byte? four bytes?
> sizeof(int)? sizeof(long)? A cacheline? what?) so I don't see how our
> users can be expected to.