[DTrace-devel] [PATCH v2 2/2] Use default action ONLY if the clause is empty
Kris Van Hees
kris.van.hees at oracle.com
Tue Sep 15 23:08:52 PDT 2020
Reviewed-by: Kris Van Hees <kris.van.hees at oracle.com>
On Wed, Sep 16, 2020 at 01:37:14AM -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.
>
> 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.
>
> 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 | 101 +++++++++++---------
> test/unittest/actions/default/tst.default.d | 44 +++++++++
> test/unittest/actions/default/tst.default.r | 8 ++
> 3 files changed, 109 insertions(+), 44 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 62b10fa4..d948fcfa 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -270,62 +270,75 @@ static void
> dt_cg_epilogue(dt_pcb_t *pcb)
> {
> dt_irlist_t *dlp = &pcb->pcb_ir;
> - dt_ident_t *buffers = dt_dlib_get_map(pcb->pcb_hdl, "buffers");
> struct bpf_insn instr;
>
> - assert(buffers != NULL);
> + TRACE_REGSET("Epilogue: Begin");
>
> /*
> - * rc = dctx->mst->fault; // lddw %r0, [%fp + DT_STK_DCTX]
> - * // lddw %r0, [%r0 + DCTX_MST]
> - * // lddw %r0, [%r0 + DMST_FAULT]
> - * if (rc != 0)
> - * goto exit; // jne %r0, 0, pcb->pcb_exitlbl
> + * Output the buffer if:
> + * - data-recording action, or
> + * - default action (no clause specified)
> */
> - TRACE_REGSET("Epilogue: Begin");
> - instr = BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_FP, DT_STK_DCTX);
> - dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> - instr = BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_0, DCTX_MST);
> - dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> - instr = BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_0, DMST_FAULT);
> - dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> - instr = BPF_BRANCH_IMM(BPF_JNE, BPF_REG_0, 0, pcb->pcb_exitlbl);
> - dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> + if (pcb->pcb_stmt->dtsd_clauseflags & DT_CLSFLAG_DATAREC ||
> + pcb->pcb_dret->dn_acts == NULL) {
> + dt_ident_t *buffers = dt_dlib_get_map(pcb->pcb_hdl, "buffers");
> +
> + assert(buffers != NULL);
> +
> + /*
> + * rc = dctx->mst->fault; // lddw %r0, [%fp + DT_STK_DCTX]
> + * // lddw %r0, [%r0 + DCTX_MST]
> + * // lddw %r0, [%r0 + DMST_FAULT]
> + * if (rc != 0)
> + * goto exit; // jne %r0, 0, pcb->pcb_exitlbl
> + */
> + instr = BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_FP, DT_STK_DCTX);
> + dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> + instr = BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_0, DCTX_MST);
> + dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> + instr = BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_0, DMST_FAULT);
> + dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> + instr = BPF_BRANCH_IMM(BPF_JNE, BPF_REG_0, 0, pcb->pcb_exitlbl);
> + dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> +
> + /*
> + * bpf_perf_event_output(dctx->ctx, &buffers, BPF_F_CURRENT_CPU,
> + * buf - 4, bufoff + 4);
> + * // lddw %r1, [%fp + DT_STK_DCTX]
> + * // lddw %r1, [%r1 + DCTX_CTX]
> + * // lddw %r2, &buffers
> + * // lddw %r3, BPF_F_CURRENT_CPU
> + * // mov %r4, %r9
> + * // add %r4, -4
> + * // mov %r5, pcb->pcb_bufoff
> + * // add %r4, 4
> + * // call bpf_perf_event_output
> + *
> + */
> + 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));
> + instr = BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_1, DCTX_CTX);
> + dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> + dt_cg_xsetx(dlp, buffers, DT_LBL_NONE, BPF_REG_2, buffers->di_id);
> + dt_cg_xsetx(dlp, NULL, DT_LBL_NONE, BPF_REG_3, BPF_F_CURRENT_CPU);
> + instr = BPF_MOV_REG(BPF_REG_4, BPF_REG_9);
> + dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> + instr = BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, -4);
> + dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> + instr = BPF_MOV_IMM(BPF_REG_5, pcb->pcb_bufoff);
> + dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> + instr = BPF_ALU64_IMM(BPF_ADD, BPF_REG_5, 4);
> + 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));
> + }
>
> /*
> - * bpf_perf_event_output(dctx->ctx, &buffers, BPF_F_CURRENT_CPU,
> - * buf - 4, bufoff + 4);
> - * // lddw %r1, [%fp + DT_STK_DCTX]
> - * // lddw %r1, [%r1 + DCTX_CTX]
> - * // lddw %r2, &buffers
> - * // lddw %r3, BPF_F_CURRENT_CPU
> - * // mov %r4, %r9
> - * // add %r4, -4
> - * // 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));
> - instr = BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_1, DCTX_CTX);
> - dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> - dt_cg_xsetx(dlp, buffers, DT_LBL_NONE, BPF_REG_2, buffers->di_id);
> - dt_cg_xsetx(dlp, NULL, DT_LBL_NONE, BPF_REG_3, BPF_F_CURRENT_CPU);
> - instr = BPF_MOV_REG(BPF_REG_4, BPF_REG_9);
> - dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> - instr = BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, -4);
> - dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> - instr = BPF_MOV_IMM(BPF_REG_5, pcb->pcb_bufoff);
> - dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> - instr = BPF_ALU64_IMM(BPF_ADD, BPF_REG_5, 4);
> - 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));
> instr = BPF_MOV_IMM(BPF_REG_0, 0);
> dt_irlist_append(dlp, dt_cg_node_alloc(pcb->pcb_exitlbl, instr));
> instr = BPF_RETURN();
> diff --git a/test/unittest/actions/default/tst.default.d b/test/unittest/actions/default/tst.default.d
> new file mode 100644
> index 00000000..6cb8be27
> --- /dev/null
> +++ b/test/unittest/actions/default/tst.default.d
> @@ -0,0 +1,44 @@
> +/*
> + * 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); }
> +
> +/* should trace */
> +tick-10
> +{ 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..943981bf
> --- /dev/null
> +++ b/test/unittest/actions/default/tst.default.r
> @@ -0,0 +1,8 @@
> + FUNCTION:NAME
> + :tick-14 hello world
> + :tick-12
> + :tick-10 7
> + :tick-10
> +
> +-- @@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