[DTrace-devel] [PATCH] Add support for array/struct/union to trace()
Kris Van Hees
kris.van.hees at oracle.com
Fri Jan 28 18:48:47 UTC 2022
On Thu, Jan 27, 2022 at 11:01:56PM -0500, Kris Van Hees via DTrace-devel wrote:
> 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.
Going back on this... Setting the alignment to 1 is not right either because
that will cause the consumer to try to interpret the data as a possible string.
I am working on a more comprehensive solution that takes into account the type
of the data being passed to trace().
v2 to come later today.
> > 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
>
> _______________________________________________
> 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