[DTrace-devel] [PATCH] cg, printf: allow storing [u]stack() into vars and printf using %k
Eugene Loh
eugene.loh at oracle.com
Fri Oct 3 05:26:16 UTC 2025
Wow. Amazing stuff. I wouldn't swear to every detail, but it generally
looks good to me.
Picky stuff below.
On 10/2/25 15:47, Kris Van Hees via DTrace-devel wrote:
> To support storing [u]stack() values in variables and as arguments for
> printf(), a type definition is introduced:
>
> typedef struct dt_stack {
> uint32_t frames;
> uint32_t strsz; // optional string blob size
> uint32_t type; // type of stack (kernel/user)
The name is fine. I can live with "type". But in pfprint_stack() you
call it "is_user", which seems much more descriptive. I wouldn't mind a
change to "is_user", but again I can live with "type". Plus, changing
to "is_user" means renaming the ref_type test.
> uint32_t pid; // process id (or 0 for kernel)
> uint64_t addrs[n]; // stack trace addresses
> } dt_stack_t;
>
> where 'n' is the maximum number of frames that can be retrieved (set
> with the 'maxframes' option).
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> @@ -2760,17 +2767,18 @@ dt_cg_act_stack_sub(dt_pcb_t *pcb, dt_node_t *dnp, int reg, int off,
> dt_regset_free_args(drp);
> /* mov32 %r0, %r0 effectively masks the lower 32 bits. */
> emit(dlp, BPF_MOV32_REG(BPF_REG_0, BPF_REG_0));
> - emit(dlp, BPF_STORE(BPF_DW, reg, off, BPF_REG_0));
> + emit(dlp, BPF_STORE(BPF_DW, reg, aloff + 12, BPF_REG_0));
> dt_regset_free(drp, BPF_REG_0);
> - }
> + } else
> + emit(dlp, BPF_STORE_IMM(BPF_W, reg, aloff + 12, 0));
Store BPF_DW at aloff+12? Shouldn't that be BPF_W? And then maybe
don't need BPF_MOV32_REG?
> diff --git a/libdtrace/dt_printf.c b/libdtrace/dt_printf.c
> @@ -569,18 +570,41 @@ dt_print_stack_user(dtrace_hdl_t *dtp, FILE *fp, const char *format,
> return err;
> }
>
> -/*ARGSUSED*/
> +/*
> + * The data at vaddr is structured as follows:
> + * uint32_t depth
> + * uint32_t strsz
> + * uint32_t type
> + * uint32_t pid
> + * uint64_t addrs[depth & 0x7fffffff]
> + *
> + * If 'depth' member provides the maximum number of addresses in the stack
s/If/The/? Also, "maximum" in this context is a little confusing to me
(since maxframes is a different concept and we're just talking about how
many frames we're making space for), but it's okay.
> + * trace.
> + * The 'strsz' member provides the size of the optional string blob that is
> + * appended to the stack trace data.
> + * The 'type' member identifies the stack trace as kernel space (0) or
> + * userspace (1).
Again, the code goes on to call this "is_user", which seems so much more
descriptive. But not a big deal.
> + * The 'pid' member provides the userspace process id for userspace stack
> + * traces and otherwise will be 0.
> + * The 'addrs' array provides the stack traces addresses. If the stack trace
> + * is shorter than 'depth', remaining addresses will be 0 and can be ignored.
> + */
> static int
> pfprint_stack(dtrace_hdl_t *dtp, FILE *fp, const char *format,
> const dt_pfargd_t *pfd, const void *vaddr, size_t size,
> uint64_t normal, uint64_t sig)
> {
> int width;
> - const dtrace_recdesc_t *rec = pfd->pfd_rec;
Well, if we no longer use pfd->pfd_rec, then the caller dt_print_stack()
no longer needs to set it.
> diff --git a/test/unittest/funcs/stack/tst.asgn_dvar.d b/test/unittest/funcs/stack/tst.asgn_dvar.d
> + assoc["a"] = stack(5);
> diff --git a/test/unittest/funcs/stack/tst.asgn_dvar.r.p b/test/unittest/funcs/stack/tst.asgn_dvar.r.p
> @@ -0,0 +1,35 @@
> +#!/usr/bin/gawk -f
> +
> +/ksys_write/ {
> + # check probe
> + if ( $1 != "ksys_write:entry" ) {
> + print "ERROR: expected fun:prb = ksys_write:entry";
> + exit 1;
> + }
> +
> + # check stack(3)
> + getline;
> + if (index($1, "`ksys_write+0x") == 0 &&
> + match($1, "`ksys_write$") == 0) {
> + print "ERROR: expected leaf frame to be ksys_write";
> + exit 1;
> + }
> + getline;
> + if (NF == 0) {
> + print "ERROR: missing second frame";
> + exit 1;
> + }
> + getline;
> + if (NF == 0) {
> + print "ERROR: missing third frame";
> + exit 1;
> + }
> + getline;
> + if (NF > 0) {
> + print "ERROR: expected stack(3) to have only three frames";
> + exit 1;
> + }
> +
> + print "success";
> + exit(0);
> +}
Hang on. The test uses stack(5) but the .r.p file twice tells me it
expects stack(3). What's going on? Well, I think the stack is short
and so we get stack(3) by accident, allowing the test to pass. So it
seems that we need to probe somewhere where the stack is deeper.
Also, fwiw, all that nice diagnostic output emitted by the .r.p file in
case of a failure will be lost. We won't see it in runtest.log or
runtest.sum. Maybe that's okay. Just fwiw.
> diff --git a/test/unittest/funcs/stack/tst.ref_pid.d b/test/unittest/funcs/stack/tst.ref_pid.d
> + * ASSERTION: Test accessing the 'pid' field of s kernel stack yields 0.
> diff --git a/test/unittest/funcs/stack/tst.ref_strsz.d b/test/unittest/funcs/stack/tst.ref_strsz.d
> + * ASSERTION: Test accessing the 'strsz' field of s kernel stack yields 0.
"of s kernel" =>
"of a kernel"
(Ditto for the ustack variants of these tests.)
> diff --git a/test/unittest/funcs/stack/tst.store.d b/test/unittest/funcs/stack/tst.store.d
> + v.depth = 2;
> + printf("%k", v);
Wow, cool. But how about changing this to
v.depth = 1;
printf("%k", v);
v.depth = 2;
printf("%k", v);
and then checking that
the first stack is a single frame
the second stack looks like the first one plus one more frame
Not hugely different from other testing being done, but since we're
checking storing to a field of a stack var, could do some correctness
checking and not just "well, it doesn't complain."
> diff --git a/test/unittest/funcs/ustack/tst.asgn_dvar.d b/test/unittest/funcs/ustack/tst.asgn_dvar.d
> + assoc["a"] = ustack(5);
> diff --git a/test/unittest/funcs/ustack/tst.asgn_dvar.r.p b/test/unittest/funcs/ustack/tst.asgn_dvar.r.p
> +#!/usr/bin/gawk -f
> +
> +/ksys_write/ {
> + # check probe
> + if ( $1 != "ksys_write:entry" ) {
> + print "ERROR: expected fun:prb = ksys_write:entry";
> + exit 1;
> + }
> +
> + # check stack(3)
> + getline;
> + if (index($1, "`ksys_write+0x") == 0 &&
> + match($1, "`ksys_write$") == 0) {
> + print "ERROR: expected leaf frame to be ksys_write";
> + exit 1;
> + }
> + getline;
> + if (NF == 0) {
> + print "ERROR: missing second frame";
> + exit 1;
> + }
> + getline;
> + if (NF == 0) {
> + print "ERROR: missing third frame";
> + exit 1;
> + }
> + getline;
> + if (NF > 0) {
> + print "ERROR: expected stack(3) to have only three frames";
> + exit 1;
> + }
> +
> + print "success";
> + exit(0);
> +}
Okay, same problems as before (asgn_* tests, but stack that time and
ustack this time).
One "problem" is that the nice diagnostic output from the .r.p is lost
when there is an error. Oh well. I guess that's okay.
The big problem, though, is that the test emits ustack(5) while the .r.p
file is emphatic that it is expecting ustack(3). Well, it says
"stack(3)" but that typo can be fixed and wouldn't change anything. In
the stack() case, the problem was that the stack was coincidentally too
small. That is certainly not the problem here. Here, the problem is
that we claim we're looking for ksys_write (which is the wrong frame to
look for) and if we do not find it then no checks are applied! So, the
.r.p file should be changed to look for a ustack frame and to complain
if it is not found.
These problems appear in the other ustack asgn_* tests as well.
> diff --git a/test/unittest/funcs/ustack/tst.ref_addrs.d b/test/unittest/funcs/ustack/tst.ref_addrs.d
> + trace("Expecting vmlinux`ksys_write, got ");
> diff --git a/test/unittest/funcs/ustack/tst.ref_addrs.r b/test/unittest/funcs/ustack/tst.ref_addrs.r
> + myfunc_z:entry Expecting vmlinux`ksys_write, got ustack-tst-basic`myfunc_z
Expecting vmlinux`ksys_write on the ustack? No, as demonstrated by the
expected output.
> diff --git a/test/unittest/printf/tst.ustack25_pid.d b/test/unittest/printf/tst.ustack25_pid.d
This test fails for me, but I think the issue is simple: leading white
space / indentation. It might suffice to strip out the leading spaces,
both in the .r file AND in the output that is fabricated in the .r.p file.
More information about the DTrace-devel
mailing list