From: Eugene Loh <eugene.loh@oracle.com>
To: Kris Van Hees <kris.van.hees@oracle.com>,
dtrace@lists.linux.dev, dtrace-devel@oss.oracle.com
Subject: Re: [PATCH] trace: fix char-array handling
Date: Fri, 7 Feb 2025 01:04:53 -0500 [thread overview]
Message-ID: <5629b0db-a8ed-5b75-4bcf-e1ffc7e6daca@oracle.com> (raw)
In-Reply-To: <PH8PR10MB6337447CA1EB2716CC4CE522C2F62@PH8PR10MB6337.namprd10.prod.outlook.com>
I don't understand the relevant code very well, but
Reviewed-by: Eugene Loh <eugene.loh@oracle.com>
Incidentally, it looks funny to have so many "new" files have 2022
copyrights on them, but I suppose they're inheriting a lot from
pre-existing tests... so, okay.
Also...
On 2/6/25 11:46, Kris Van Hees wrote:
> The special handling of strings in the consumer's trace implementation
> did not correctly account for char-arrays that do not necessarily are
s/do not necessarily are/are not necessarily/
> intended to hold string data. Thr trace action is documented to print
s/Thr/The/
> data as an ASCII string if it can be represented as such, i.e. if it
> comprises only printable characters, optionally terminated by one or
> more 0-bytes.
That doesn't read quite right to me. E.g., 'a' 'b' 0 'c' 0 looks to me
like "only printable chars, terminated by one or more 0 bytes" yet I
think it's supposed to be dumped (not printed as a string). I'm not sure
and maybe it doesn't matter.
Another possible test is all bytes are 0, but I'm not worried about
whether such a test is included.
> The implementation for trace on both producer and consumer side was
> using the data alignment as a marker to determine whether data should
> be printed as a string (alignment == 1) or not. The real alignment
> of the data should be used, reverting code back to the necessary
> heuristics in dt_print_bytes().
>
> The dtrace_diftype struct has its dtdt_pad member renamed as dtdt_align.
>
> Added several new test cases to cover various possibilities.
>
> Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> ---
> include/dtrace/dif.h | 2 +-
> libdtrace/dt_cg.c | 16 ++++++----
> libdtrace/dt_consume.c | 20 ++-----------
> libdtrace/dt_parser.c | 2 +-
> .../actions/trace/tst.array-char-multi-nul.d | 30 +++++++++++++++++++
> .../actions/trace/tst.array-char-multi-nul.r | 8 +++++
> .../trace/tst.array-char-str-multi-nul.d | 30 +++++++++++++++++++
> .../trace/tst.array-char-str-multi-nul.r | 5 ++++
> .../actions/trace/tst.array-char-str-no-nul.d | 30 +++++++++++++++++++
> .../actions/trace/tst.array-char-str-no-nul.r | 5 ++++
> .../actions/trace/tst.array-char-str.d | 30 +++++++++++++++++++
> .../actions/trace/tst.array-char-str.r | 5 ++++
> .../trace/tst.array-char-unprintable.d | 30 +++++++++++++++++++
> .../trace/tst.array-char-unprintable.r | 8 +++++
> test/unittest/actions/trace/tst.array.d | 11 +++++--
> test/unittest/actions/trace/tst.array.r | 2 +-
> test/unittest/actions/trace/tst.array.r.p | 6 ----
> 17 files changed, 206 insertions(+), 34 deletions(-)
> create mode 100644 test/unittest/actions/trace/tst.array-char-multi-nul.d
> create mode 100644 test/unittest/actions/trace/tst.array-char-multi-nul.r
> create mode 100644 test/unittest/actions/trace/tst.array-char-str-multi-nul.d
> create mode 100644 test/unittest/actions/trace/tst.array-char-str-multi-nul.r
> create mode 100644 test/unittest/actions/trace/tst.array-char-str-no-nul.d
> create mode 100644 test/unittest/actions/trace/tst.array-char-str-no-nul.r
> create mode 100644 test/unittest/actions/trace/tst.array-char-str.d
> create mode 100644 test/unittest/actions/trace/tst.array-char-str.r
> create mode 100644 test/unittest/actions/trace/tst.array-char-unprintable.d
> create mode 100644 test/unittest/actions/trace/tst.array-char-unprintable.r
> delete mode 100755 test/unittest/actions/trace/tst.array.r.p
>
> diff --git a/include/dtrace/dif.h b/include/dtrace/dif.h
> index 576bda2c..70257b2f 100644
> --- a/include/dtrace/dif.h
> +++ b/include/dtrace/dif.h
> @@ -33,7 +33,7 @@ typedef struct dtrace_diftype {
> uint8_t dtdt_kind;
> uint8_t dtdt_ckind;
> uint8_t dtdt_flags;
> - uint8_t dtdt_pad;
> + uint8_t dtdt_align;
> uint32_t dtdt_size;
> } dtrace_diftype_t;
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 6e74b4b0..a5c9aa09 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -1601,12 +1601,11 @@ static int
> dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
> dt_pfargv_t *pfp, int arg)
> {
> - dtrace_diftype_t vtype;
> dtrace_hdl_t *dtp = pcb->pcb_hdl;
> dt_irlist_t *dlp = &pcb->pcb_ir;
> dt_regset_t *drp = pcb->pcb_regs;
> uint_t off;
> - size_t size;
> + size_t size, align;
> int not_null = 0;
> int cflags = pcb->pcb_stmt->dtsd_clauseflags;
>
> @@ -1619,10 +1618,14 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
> longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> emit(dlp, BPF_MOV_IMM(dnp->dn_reg, dnp->dn_ident->di_id));
> size = sizeof(dnp->dn_ident->di_id);
> + align = size;
> } else {
> + dtrace_diftype_t vtype;
> +
> dt_cg_node(dnp, dlp, drp);
> dt_node_diftype(dtp, dnp, &vtype);
> size = vtype.dtdt_size;
> + align = vtype.dtdt_align;
>
> /*
> * A DEREF of a REF node does not get resolved in dt_cg_node()
> @@ -1664,7 +1667,7 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
>
> if (dt_node_is_scalar(dnp) || dt_node_is_float(dnp) ||
> dnp->dn_kind == DT_NODE_AGG) {
> - off = dt_rec_add(dtp, dt_cg_fill_gap, kind, size, size, pfp,
> + off = dt_rec_add(dtp, dt_cg_fill_gap, kind, size, align, pfp,
> arg);
>
> emit(dlp, BPF_STOREX(size, BPF_REG_9, off, dnp->dn_reg));
> @@ -1678,8 +1681,8 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
> dt_cg_check_ptr_arg(dlp, drp, dnp, NULL);
>
> TRACE_REGSET("store_val(): Begin ");
> - off = dt_rec_add(dtp, dt_cg_fill_gap, kind, size + 1,
> - 1, pfp, arg);
> + off = dt_rec_add(dtp, dt_cg_fill_gap, kind, size + 1, 1, pfp,
> + arg);
>
> /*
> * Copy the string data (no more than STRSIZE + 1 bytes) to the
> @@ -1706,7 +1709,8 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
>
> /* Handle tracing of by-ref values (arrays, struct, union). */
> if ((dnp->dn_flags & DT_NF_REF) || (arg & DT_NF_REF)) {
> - off = dt_rec_add(dtp, dt_cg_fill_gap, kind, size, 2, pfp, arg);
> + off = dt_rec_add(dtp, dt_cg_fill_gap, kind, size, align, pfp,
> + arg);
>
> TRACE_REGSET("store_val(): Begin ");
> if (!not_null)
> diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
> index 74004c69..58a2ead9 100644
> --- a/libdtrace/dt_consume.c
> +++ b/libdtrace/dt_consume.c
> @@ -1921,23 +1921,9 @@ dt_print_trace(dtrace_hdl_t *dtp, FILE *fp, dtrace_recdesc_t *rec,
> if (dtp->dt_options[DTRACEOPT_RAWBYTES] != DTRACEOPT_UNSET)
> return dt_print_rawbytes(dtp, fp, data, rec->dtrd_size);
>
> - /*
> - * String data can be recognized as a non-scalar data item with
> - * alignment == 1.
> - * Any other non-scalar data items are printed as a byte stream.
> - */
> - if (rec->dtrd_arg == DT_NF_REF) {
> - char *s = (char *)data;
> -
> - if (rec->dtrd_alignment > 1)
> - return dt_print_rawbytes(dtp, fp, data, rec->dtrd_size);
> -
> - /* We have a string. Print it. */
> - if (quiet)
> - return dt_printf(dtp, fp, "%s", s);
> - else
> - return dt_printf(dtp, fp, " %-33s", s);
> - }
> + /* Handle non-scalar data. */
> + if (rec->dtrd_arg == DT_NF_REF)
> + return dt_print_bytes(dtp, fp, data, rec->dtrd_size, 33, quiet);
>
> /*
> * Differentiate between signed and unsigned numeric values.
> diff --git a/libdtrace/dt_parser.c b/libdtrace/dt_parser.c
> index 325ba881..eefe8341 100644
> --- a/libdtrace/dt_parser.c
> +++ b/libdtrace/dt_parser.c
> @@ -4914,7 +4914,7 @@ dt_node_diftype(dtrace_hdl_t *dtp, const dt_node_t *dnp, dtrace_diftype_t *tp)
> }
>
> tp->dtdt_flags = (dnp->dn_flags & DT_NF_REF) ? DIF_TF_BYREF : 0;
> - tp->dtdt_pad = 0;
> + tp->dtdt_align = ctf_type_align(dnp->dn_ctfp, dnp->dn_type);
> tp->dtdt_size = ctf_type_size(dnp->dn_ctfp, dnp->dn_type);
> }
>
> diff --git a/test/unittest/actions/trace/tst.array-char-multi-nul.d b/test/unittest/actions/trace/tst.array-char-multi-nul.d
> new file mode 100644
> index 00000000..187fb711
> --- /dev/null
> +++ b/test/unittest/actions/trace/tst.array-char-multi-nul.d
> @@ -0,0 +1,30 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2022, 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: The trace() action prints a char-array of printable characters
> + * with multiple 0-bytes in its content.
> + *
> + * SECTION: Actions and Subroutines/trace()
> + */
> +
> +char n[9];
> +
> +BEGIN
> +{
> + n[0] = 'a';
> + n[1] = 'A';
> + n[2] = 0;
> + n[3] = 'B';
> + n[4] = 0;
> + n[5] = 'C';
> + n[6] = 0;
> + n[7] = 'D';
> + n[8] = 'e';
> + trace(n);
> + exit(0);
> +}
> diff --git a/test/unittest/actions/trace/tst.array-char-multi-nul.r b/test/unittest/actions/trace/tst.array-char-multi-nul.r
> new file mode 100644
> index 00000000..c1361e75
> --- /dev/null
> +++ b/test/unittest/actions/trace/tst.array-char-multi-nul.r
> @@ -0,0 +1,8 @@
> + FUNCTION:NAME
> + :BEGIN
> + 0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef
> + 0: 61 41 00 42 00 43 00 44 65 aA.B.C.De
> +
> +
> +-- @@stderr --
> +dtrace: script 'test/unittest/actions/trace/tst.array-char-multi-nul.d' matched 1 probe
> diff --git a/test/unittest/actions/trace/tst.array-char-str-multi-nul.d b/test/unittest/actions/trace/tst.array-char-str-multi-nul.d
> new file mode 100644
> index 00000000..75164cd3
> --- /dev/null
> +++ b/test/unittest/actions/trace/tst.array-char-str-multi-nul.d
> @@ -0,0 +1,30 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2022, 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: The trace() action prints a char-array of printable characters
> + * followed y by multiple 0-bytes correctly.
> + *
> + * SECTION: Actions and Subroutines/trace()
> + */
> +
> +char n[9];
> +
> +BEGIN
> +{
> + n[0] = 'a';
> + n[1] = 'A';
> + n[2] = 'b';
> + n[3] = 'B';
> + n[4] = 'c';
> + n[5] = 0;
> + n[6] = 0;
> + n[7] = 0;
> + n[8] = 0;
> + trace(n);
> + exit(0);
> +}
> diff --git a/test/unittest/actions/trace/tst.array-char-str-multi-nul.r b/test/unittest/actions/trace/tst.array-char-str-multi-nul.r
> new file mode 100644
> index 00000000..1951ae69
> --- /dev/null
> +++ b/test/unittest/actions/trace/tst.array-char-str-multi-nul.r
> @@ -0,0 +1,5 @@
> + FUNCTION:NAME
> + :BEGIN aAbBc
> +
> +-- @@stderr --
> +dtrace: script 'test/unittest/actions/trace/tst.array-char-str-multi-nul.d' matched 1 probe
> diff --git a/test/unittest/actions/trace/tst.array-char-str-no-nul.d b/test/unittest/actions/trace/tst.array-char-str-no-nul.d
> new file mode 100644
> index 00000000..532c3519
> --- /dev/null
> +++ b/test/unittest/actions/trace/tst.array-char-str-no-nul.d
> @@ -0,0 +1,30 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2022, 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: The trace() action prints a char-aarray of printable characters
> + * (not terminated) correctly.
> + *
> + * SECTION: Actions and Subroutines/trace()
> + */
> +
> +char n[9];
> +
> +BEGIN
> +{
> + n[0] = 'a';
> + n[1] = 'A';
> + n[2] = 'b';
> + n[3] = 'B';
> + n[4] = 'c';
> + n[5] = 'C';
> + n[6] = 'd';
> + n[7] = 'D';
> + n[8] = 'e';
> + trace(n);
> + exit(0);
> +}
> diff --git a/test/unittest/actions/trace/tst.array-char-str-no-nul.r b/test/unittest/actions/trace/tst.array-char-str-no-nul.r
> new file mode 100644
> index 00000000..3d56e520
> --- /dev/null
> +++ b/test/unittest/actions/trace/tst.array-char-str-no-nul.r
> @@ -0,0 +1,5 @@
> + FUNCTION:NAME
> + :BEGIN aAbBcCdDe
> +
> +-- @@stderr --
> +dtrace: script 'test/unittest/actions/trace/tst.array-char-str-no-nul.d' matched 1 probe
> diff --git a/test/unittest/actions/trace/tst.array-char-str.d b/test/unittest/actions/trace/tst.array-char-str.d
> new file mode 100644
> index 00000000..70f6371f
> --- /dev/null
> +++ b/test/unittest/actions/trace/tst.array-char-str.d
> @@ -0,0 +1,30 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2022, 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: The trace() action prints a char-array of printable characters
> + * (terminated) correctly.
> + *
> + * SECTION: Actions and Subroutines/trace()
> + */
> +
> +char n[9];
> +
> +BEGIN
> +{
> + n[0] = 'a';
> + n[1] = 'A';
> + n[2] = 'b';
> + n[3] = 'B';
> + n[4] = 'c';
> + n[5] = 'C';
> + n[6] = 'd';
> + n[7] = 'D';
> + n[8] = '\0';
> + trace(n);
> + exit(0);
> +}
> diff --git a/test/unittest/actions/trace/tst.array-char-str.r b/test/unittest/actions/trace/tst.array-char-str.r
> new file mode 100644
> index 00000000..9778ddd2
> --- /dev/null
> +++ b/test/unittest/actions/trace/tst.array-char-str.r
> @@ -0,0 +1,5 @@
> + FUNCTION:NAME
> + :BEGIN aAbBcCdD
> +
> +-- @@stderr --
> +dtrace: script 'test/unittest/actions/trace/tst.array-char-str.d' matched 1 probe
> diff --git a/test/unittest/actions/trace/tst.array-char-unprintable.d b/test/unittest/actions/trace/tst.array-char-unprintable.d
> new file mode 100644
> index 00000000..d88df369
> --- /dev/null
> +++ b/test/unittest/actions/trace/tst.array-char-unprintable.d
> @@ -0,0 +1,30 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2022, 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: The trace() action prints a char-array with a non-printable
> + * character correctly.
> + *
> + * SECTION: Actions and Subroutines/trace()
> + */
> +
> +char n[9];
> +
> +BEGIN
> +{
> + n[0] = 'a';
> + n[1] = 'A';
> + n[2] = 'b';
> + n[3] = 'B';
> + n[4] = 'c';
> + n[5] = 5;
> + n[6] = 'd';
> + n[7] = 'D';
> + n[8] = '\0';
> + trace(n);
> + exit(0);
> +}
> diff --git a/test/unittest/actions/trace/tst.array-char-unprintable.r b/test/unittest/actions/trace/tst.array-char-unprintable.r
> new file mode 100644
> index 00000000..1f84fcfa
> --- /dev/null
> +++ b/test/unittest/actions/trace/tst.array-char-unprintable.r
> @@ -0,0 +1,8 @@
> + FUNCTION:NAME
> + :BEGIN
> + 0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef
> + 0: 61 41 62 42 63 05 64 44 00 aAbBc.dD.
> +
> +
> +-- @@stderr --
> +dtrace: script 'test/unittest/actions/trace/tst.array-char-unprintable.d' matched 1 probe
> diff --git a/test/unittest/actions/trace/tst.array.d b/test/unittest/actions/trace/tst.array.d
> index 104d42e1..0779d90f 100644
> --- a/test/unittest/actions/trace/tst.array.d
> +++ b/test/unittest/actions/trace/tst.array.d
> @@ -1,6 +1,6 @@
> /*
> * Oracle Linux DTrace.
> - * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2022, 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.
> */
> @@ -11,8 +11,15 @@
> * SECTION: Actions and Subroutines/trace()
> */
>
> +short n[5];
> +
> BEGIN
> {
> - trace(curthread->comm);
> + n[0] = 0x1234;
> + n[1] = 0x5678;
> + n[2] = 0x0000;
> + n[3] = 0x8765;
> + n[4] = 0x4321;
> + trace(n);
> exit(0);
> }
> diff --git a/test/unittest/actions/trace/tst.array.r b/test/unittest/actions/trace/tst.array.r
> index fbb674b6..52ff28ec 100644
> --- a/test/unittest/actions/trace/tst.array.r
> +++ b/test/unittest/actions/trace/tst.array.r
> @@ -1,7 +1,7 @@
> FUNCTION:NAME
> :BEGIN
> 0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef
> - 0: 64 74 72 61 63 65 00 00 00 00 00 00 00 00 00 00 dtrace..........
> + 0: 34 12 78 56 00 00 65 87 21 43 4.xV..e.!C
>
>
> -- @@stderr --
> diff --git a/test/unittest/actions/trace/tst.array.r.p b/test/unittest/actions/trace/tst.array.r.p
> deleted file mode 100755
> index b8cc8daf..00000000
> --- a/test/unittest/actions/trace/tst.array.r.p
> +++ /dev/null
> @@ -1,6 +0,0 @@
> -#!/usr/bin/gawk -f
> -
> -# Some Linux kernel versions leave garbage at the end of the string.
> -{ sub(/( [0-9A-F]{2}){9} /, " 00 00 00 00 00 00 00 00 00 "); }
> -{ sub(/ dtrace\..{9}/, " dtrace.........."); }
> -{ print; }
next prev parent reply other threads:[~2025-02-07 6:05 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-02-06 16:46 [PATCH] trace: fix char-array handling Kris Van Hees
2025-02-07 6:04 ` Eugene Loh [this message]
2025-02-07 6:08 ` Kris Van Hees
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5629b0db-a8ed-5b75-4bcf-e1ffc7e6daca@oracle.com \
--to=eugene.loh@oracle.com \
--cc=dtrace-devel@oss.oracle.com \
--cc=dtrace@lists.linux.dev \
--cc=kris.van.hees@oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.