[DTrace-devel] [PATCH] Use default action ONLY if the clause is empty
Kris Van Hees
kris.van.hees at oracle.com
Thu Aug 20 11:10:00 PDT 2020
On Thu, Aug 20, 2020 at 01:13:30PM -0400, eugene.loh at oracle.com wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
>
> If a clause is empty, use the default clause; this was working.
> On the other hand, if a clause is not empty, do not use the default
> clause, even if the clause traces no data. This was not working
> in DTv2. Arguably, the new behavior was more sensible, but it
> breaks precedent, including in some very important and common
> cases, such as clauses that simply manipulate D variables.
>
> Restore the legacy behavior.
>
> Specifically, split the epilogue into two pieces:
> - If we explicitly wrote any data or if the clause was empty,
> output the buffer.
> - Exit.
I am not sure whether this is the correct approach. Because of some of the
limitations in what we can do with BPF in the kernel, I anticipate that we
need to actually have some of these clauses that do not generate data still be
reported in the trace output buffer. So, I think instead of this approach we
need to somehow mark the clause (perhaps in the datadesc in some way) as one
that should not display output.
Can you look at ways we could do that? Maybe we can stick a flag somewhere
to signal this? It needs to be something that can be easily checked for in
the buffer processing in dt_consume.
> Do not modify the prologue. A few instructions in the prologue could
> be eliminated for clauses that will not write the output buffer, but
> the instructions are inconsequential and would be difficult to eliminate.
>
> To check if we "wrote any data" it is not enough to see how much data
> was written. E.g., a printf("format") with no formatting attributes
> will write no explicit data. So, we have to check the number of records.
>
> The test test/unittest/funcs/tst.default.d was too mild to catch
> this problem. Add test/unittest/actions/default/tst.default.d .
>
> Orabug: 31772287
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> ---
> libdtrace/dt_cg.c | 42 ++++++++++++++++-----
> test/unittest/actions/default/tst.default.d | 40 ++++++++++++++++++++
> test/unittest/actions/default/tst.default.r | 7 ++++
> 3 files changed, 79 insertions(+), 10 deletions(-)
> create mode 100644 test/unittest/actions/default/tst.default.d
> create mode 100644 test/unittest/actions/default/tst.default.r
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index d5532c55..821e5b18 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -259,15 +259,14 @@ dt_cg_prologue(dt_pcb_t *pcb, dt_node_t *pred)
> }
>
> /*
> - * Generate the function epilogue:
> + * Generate the function epilogue (output the buffer):
> * 4. If a fault was flagged, return 0.
> * 5. Submit the buffer to the perf event output buffer for the current
> * cpu.
> - * 6. Return 0
> * }
> */
> static void
> -dt_cg_epilogue(dt_pcb_t *pcb)
> +dt_cg_epilogue_output(dt_pcb_t *pcb)
> {
> dt_irlist_t *dlp = &pcb->pcb_ir;
> dt_ident_t *buffers = dt_dlib_get_map(pcb->pcb_hdl, "buffers");
> @@ -304,11 +303,6 @@ dt_cg_epilogue(dt_pcb_t *pcb)
> * // mov %r5, pcb->pcb_bufoff
> * // add %r4, 4
> * // call bpf_perf_event_output
> - *
> - * exit:
> - * return 0; // mov %r0, 0
> - * // exit
> - * }
> */
> instr = BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_DCTX);
> dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> @@ -326,11 +320,30 @@ dt_cg_epilogue(dt_pcb_t *pcb)
> dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> instr = BPF_CALL_HELPER(BPF_FUNC_perf_event_output);
> dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> + TRACE_REGSET("Epilogue: End ");
> +}
> +
> +/*
> + * Generate the function epilogue (exit):
> + * 6. Return 0
> + * }
> + */
> +static void
> +dt_cg_epilogue_exit(dt_pcb_t *pcb)
> +{
> + dt_irlist_t *dlp = &pcb->pcb_ir;
> + struct bpf_insn instr;
> +
> + /*
> + * exit:
> + * return 0; // mov %r0, 0
> + * // exit
> + * }
> + */
> instr = BPF_MOV_IMM(BPF_REG_0, 0);
> dt_irlist_append(dlp, dt_cg_node_alloc(pcb->pcb_exitlbl, instr));
> instr = BPF_RETURN();
> dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> - TRACE_REGSET("Epilogue: End ");
> }
>
> /*
> @@ -2968,8 +2981,10 @@ dt_cg(dt_pcb_t *pcb, dt_node_t *dnp)
> dt_cg_node(dnp, &pcb->pcb_ir, pcb->pcb_regs);
> } else if (dnp->dn_kind == DT_NODE_CLAUSE) {
> dt_irlist_t *dlp = &pcb->pcb_ir;
> + int start_nrecs;
>
> dt_cg_prologue(pcb, dnp->dn_pred);
> + start_nrecs = pcb->pcb_ddesc->dtdd_nrecs;
>
> for (act = dnp->dn_acts; act != NULL; act = act->dn_list) {
> pcb->pcb_dret = act->dn_expr;
> @@ -2992,7 +3007,14 @@ dt_cg(dt_pcb_t *pcb, dt_node_t *dnp)
> }
> }
>
> - dt_cg_epilogue(pcb);
> + /*
> + * If we put something in the buffer or we have the default
> + * action, output the buffer.
> + */
> + if (pcb->pcb_ddesc->dtdd_nrecs != start_nrecs ||
> + dnp->dn_acts == NULL)
> + dt_cg_epilogue_output(pcb);
> + dt_cg_epilogue_exit(pcb);
> } else if (dnp->dn_kind == DT_NODE_TRAMPOLINE) {
> assert(pcb->pcb_probe != NULL);
>
> diff --git a/test/unittest/actions/default/tst.default.d b/test/unittest/actions/default/tst.default.d
> new file mode 100644
> index 00000000..26469c40
> --- /dev/null
> +++ b/test/unittest/actions/default/tst.default.d
> @@ -0,0 +1,40 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2020, 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 default action is used for the empty clause but not
> + * for a non-empty clause, even if it does not trace anything.
> + * A special case is a printf() with no attributes, which is
> + * a trace record with no extra data in the tracing record;
> + * make sure the printf() occurs.
> + *
> + * SECTION: Actions and Subroutines/default
> + */
> +
> +/* should not trace, but should update n */
> +BEGIN
> +{ n = 1; }
> +
> +/* should trace, "hello world" should appear */
> +tick-14
> +{ printf("hello world"); }
> +
> +/* should not trace, but should update n */
> +tick-13
> +{ n += 2; }
> +
> +/* should trace */
> +tick-12
> +{ }
> +
> +/* should not trace, but should update n */
> +tick-11
> +{ n += 4; }
> +
> +/* should trace, and n should be 1+2+4=7 */
> +tick-10
> +{ trace(n); exit(0) }
> diff --git a/test/unittest/actions/default/tst.default.r b/test/unittest/actions/default/tst.default.r
> new file mode 100644
> index 00000000..ef0e5257
> --- /dev/null
> +++ b/test/unittest/actions/default/tst.default.r
> @@ -0,0 +1,7 @@
> + FUNCTION:NAME
> + :tick-14 hello world
> + :tick-12
> + :tick-10 7
> +
> +-- @@stderr --
> +dtrace: script 'test/unittest/actions/default/tst.default.d' matched 6 probes
> --
> 2.18.4
>
>
> _______________________________________________
> 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