[DTrace-devel] [PATCH] Add support for array/struct/union to trace()
Kris Van Hees
kris.van.hees at oracle.com
Fri Jan 28 04:01:56 UTC 2022
On Thu, Jan 27, 2022 at 05:11:18PM -0500, Eugene Loh via DTrace-devel wrote:
> On 1/26/22 11:26 AM, Kris Van Hees via DTrace-devel wrote:
>
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > @@ -1002,6 +1002,32 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
> > return 0;
> > }
> > + /* Handle tracing of by-ref values (arrays, struct, union). */
> > + if (kind == DTRACEACT_DIFEXPR && (arg & DT_NF_REF)) {
> > + off = dt_rec_add(dtp, dt_cg_fill_gap, kind,
> > + size, size, pfp, arg);
> > +
> > + TRACE_REGSET("store_val(): Begin ");
> > + dt_cg_check_notnull(dlp, drp, dnp->dn_reg);
> > +
> > + 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_MOV_IMM(BPF_REG_2, size));
> > + emit(dlp, BPF_MOV_REG(BPF_REG_3, dnp->dn_reg));
> > + dt_regset_free(drp, dnp->dn_reg);
> > + dt_regset_xalloc(drp, BPF_REG_0);
> > + emit(dlp, BPF_CALL_HELPER(BPF_FUNC_probe_read));
> > + dt_regset_free_args(drp);
> > +
> > + dt_regset_free(drp, BPF_REG_0);
> > + TRACE_REGSET("store_val(): End ");
> > +
> > + return 0;
> > + }
> > +
> > return -1;
> > }
>
> For dt_rec_add(), why is the alignment "size" rather than 1?
Good catch. Cut'n'paste error. I am not certain that 1 will be the best
alignment here because we may want a struct, union, or array at its proper
alignment based on the conte type(s), but for now I will go with it. We
can revisit this if/when we might consider providing annotated output.
> dt_cg_store_val() handles different cases (USYM, scalar, string, and now
> by-ref), in each case ending with "return 0" -- or if no such case then
> "return -1". There are two styles for expressing this:
>
> if (...) {
> [...]
> return 0;
> }
> if (...) {
> [...]
> return 0;
> }
> if (...) {
> [...]
> return 0;
> }
> return -1;
>
> or
>
> if (...) {
> [...]
> return 0;
> }
> else if (...) {
> [...]
> return 0;
> }
> else if (...) {
> [...]
> return 0;
> }
> return -1;
>
> Right now, we use both styles. I think it makes sense to use one,
> consistent style (I prefer the former), and I think it is reasonable in this
> patch to fix the pre-existing inconsistency.
As you can see, the conditionals are based on different things. The first is
based on the action kind, whereas the 2nd and 3rd are based on node kinds.
That is why they are separated.
I could move the new case (DTRACEACT_DIFEXPR) as an alternative branch for the
first case (based on action kind), but having it as last case seems to make
sense.
> Regarding the tests...
>
> We need to check the output of the new tests. For struct and union, this
> means .r files. For array, we do not care about differences past the first
> NUL char... so this may need a tst.array.r.p file as well.
Ah yes, I seem to have failed to add them to the patch. Mea culpa.
> The output of tst.union.d looks wrong. If I try
> # dtrace -qs test/unittest/actions/trace/tst.union.d
> with Solaris and DTv1 I get:
> xV4
> while with DTv2 I get:
> 0 1 2 3 4 5 6 7 8 9 a b c d e f
> 0123456789abcdef
> 0: 78 56 34 xV4
The DTrace v2 behaviour is correct - DTrace v1 does not have the fixes to trace
to better deal with various integer sizes. 3 bytes cannot be a valid integer,
and it is also not a string, so it is printed using the rawbytes support.
> > diff --git a/test/unittest/actions/trace/tst.array.d b/test/unittest/actions/trace/tst.array.d
> > new file mode 100644
> > index 00000000..104d42e1
> > --- /dev/null
> > +++ b/test/unittest/actions/trace/tst.array.d
> > @@ -0,0 +1,18 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2022, 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 an array correctly.
> > + *
> > + * SECTION: Actions and Subroutines/trace()
> > + */
> > +
> > +BEGIN
> > +{
> > + trace(curthread->comm);
> > + exit(0);
> > +}
> > diff --git a/test/unittest/actions/trace/tst.struct.d b/test/unittest/actions/trace/tst.struct.d
> > new file mode 100644
> > index 00000000..ec60ea11
> > --- /dev/null
> > +++ b/test/unittest/actions/trace/tst.struct.d
> > @@ -0,0 +1,27 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2022, 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 struct correctly.
> > + *
> > + * SECTION: Actions and Subroutines/trace()
> > + */
> > +
> > +struct {
> > + int a;
> > + char b;
> > + int c;
> > +} st;
> > +
> > +BEGIN
> > +{
> > + st.a = 0x1234;
> > + st.b = 0;
> > + st.c = 0x9876;
> > + trace(st);
> > + exit(0);
> > +}
> > diff --git a/test/unittest/actions/trace/tst.union.d b/test/unittest/actions/trace/tst.union.d
> > new file mode 100644
> > index 00000000..b9d107d3
> > --- /dev/null
> > +++ b/test/unittest/actions/trace/tst.union.d
> > @@ -0,0 +1,25 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2022, 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 union correctly.
> > + *
> > + * SECTION: Actions and Subroutines/trace()
> > + */
> > +
> > +union {
> > + int a;
> > + char b[3];
> > + short c;
> > +} u;
> > +
> > +BEGIN
> > +{
> > + u.a = 0x12345678;
> > + trace(u.b);
> > + exit(0);
> > +}
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list