[DTrace-devel] [PATCH 1/2] Add clause flags
Kris Van Hees
kris.van.hees at oracle.com
Tue Sep 15 21:50:02 PDT 2020
On Tue, Sep 15, 2020 at 10:26:35PM -0400, eugene.loh at oracle.com wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
>
> There are compile-time rules to enforce regarding which actions are
> compatible with which others. E.g., data-recording actions cannot
> follow commits, etc. Introduce flags that track clause actions during
> compilation so that these rules can be enforced.
>
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> ---
> libdtrace/dt_cc.c | 1 +
> libdtrace/dt_cg.c | 60 +++++++++++++++++--
> libdtrace/dt_map.c | 4 ++
> libdtrace/dtrace.h | 8 +++
> .../err.D_COMM_COMM.CommitAftCommit.d | 1 -
> .../err.D_COMM_COMM.DisjointCommit.d | 1 -
> .../err.D_COMM_DREC.CommitAftDataRec.d | 1 -
> .../err.D_DREC_COMM.DataRecAftCommit.d | 1 -
> .../err.D_DREC_COMM.ExitAfterCommit.d | 1 -
> .../speculation/err.D_EXIT_SPEC.ExitAftSpec.d | 1 -
> .../err.D_SPEC_COMM.SpecAftCommit.d | 1 -
> .../err.D_SPEC_DREC.SpecAftDataRec.d | 1 -
> .../speculation/err.D_SPEC_SPEC.SpecAftSpec.d | 1 -
> 13 files changed, 68 insertions(+), 14 deletions(-)
>
> diff --git a/libdtrace/dt_cc.c b/libdtrace/dt_cc.c
> index f1ac0776..4776c356 100644
> --- a/libdtrace/dt_cc.c
> +++ b/libdtrace/dt_cc.c
> @@ -163,6 +163,7 @@ dt_stmt_append(dtrace_stmtdesc_t *sdp, const dt_node_t *dnp)
> #ifdef FIXME
> /*
> * Make sure that the new statement jibes with the rest of the ECB.
> + * FIXME: Eliminate this code once it's incorporated elsewhere.
> */
> for (ap = edp->dted_action; ap != NULL; ap = ap->dtad_next) {
> if (ap->dtad_kind == DTRACEACT_COMMIT) {
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 0d3463c1..30dd7e1a 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -483,7 +483,7 @@ dt_cg_act_clear(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> }
>
> aid = anp->dn_ident;
> - if (aid->di_gen == pcb->pcb_hdl->dt_gen &&
> + if (aid->di_gen == pcb->pcb_hdl->dt_gen &&
> !(aid->di_flags & DT_IDFLG_MOD)) {
> dnerror(dnp, D_CLEAR_AGGBAD,
> "undefined aggregation: @%s\n", aid->di_name);
> @@ -507,12 +507,23 @@ dt_cg_act_clear(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> static void
> dt_cg_act_commit(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> {
> - struct bpf_insn instr;
> - dt_irlist_t *dlp = &pcb->pcb_ir;
> - uint_t off;
> + struct bpf_insn instr;
> + dt_irlist_t *dlp = &pcb->pcb_ir;
> + uint_t off;
> + int *cfp = &pcb->pcb_stmt->dtsd_clauseflags;
> +
> + /* process clause flags */
> + if (*cfp & DT_CLSFLAG_COMMIT)
> + dnerror(dnp, D_COMM_COMM,
> + "commit( ) may not follow commit( )\n");
> + if (*cfp & DT_CLSFLAG_DATAREC)
> + dnerror(dnp, D_COMM_DREC,
> + "commit( ) may not follow data-recording action(s)\n");
> + *cfp |= DT_CLSFLAG_COMMIT;
>
> dt_cg_node(dnp->dn_args, &pcb->pcb_ir, pcb->pcb_regs);
>
> + /* FIXME: add DT_CLSFLAG_DATAREC? */
No, because the following code is merely a placeholder.
> off = dt_rec_add(pcb->pcb_hdl, dt_cg_fill_gap, DTRACEACT_COMMIT,
> sizeof(uint64_t), sizeof(uint64_t), NULL,
> DT_ACT_COMMIT);
> @@ -539,7 +550,7 @@ dt_cg_act_denormalize(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> }
>
> aid = anp->dn_ident;
> - if (aid->di_gen == pcb->pcb_hdl->dt_gen &&
> + if (aid->di_gen == pcb->pcb_hdl->dt_gen &&
> !(aid->di_flags & DT_IDFLG_MOD)) {
> dnerror(dnp, D_NORMALIZE_AGGBAD,
> "undefined aggregation: @%s\n", aid->di_name);
> @@ -569,6 +580,7 @@ dt_cg_act_discard(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
>
> dt_cg_node(dnp->dn_args, &pcb->pcb_ir, pcb->pcb_regs);
>
> + /* FIXME: add DT_CLSFLAG_DATAREC? */
No, because the following code is merely a placeholder.
> off = dt_rec_add(pcb->pcb_hdl, dt_cg_fill_gap, DTRACEACT_DISCARD,
> sizeof(uint64_t), sizeof(uint64_t), NULL,
> DT_ACT_DISCARD);
> @@ -589,6 +601,16 @@ dt_cg_act_exit(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> struct bpf_insn instr;
> dt_irlist_t *dlp = &pcb->pcb_ir;
> uint_t off;
> + int *cfp = &pcb->pcb_stmt->dtsd_clauseflags;
> +
> + /* process clause flags */
> + if (*cfp & DT_CLSFLAG_SPECULATE)
> + dnerror(dnp, D_EXIT_SPEC,
> + "exit( ) may not follow speculate( )\n");
> + if (*cfp & DT_CLSFLAG_COMMIT)
> + dnerror(dnp, D_DREC_COMM,
> + "data-recording actions may not follow commit( )\n");
> + *cfp |= DT_CLSFLAG_EXIT | DT_CLSFLAG_DATAREC;
This should only set DT_CLSFLAG_EXIT, and the DT_CLSFLAG_DATAREC should be set
in dt_cg_store_val() since that is the function that calls dt_add_rec() and
then also generates the instructions to actually write the value to the output
buffer.
> dt_cg_node(dnp->dn_args, &pcb->pcb_ir, pcb->pcb_regs);
>
> @@ -638,6 +660,13 @@ dt_cg_act_printf(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> dt_pfargv_t *pfp;
> char n[DT_TYPE_NAMELEN];
> char *str;
> + int *cfp = &pcb->pcb_stmt->dtsd_clauseflags;
> +
> + /* process clause flags */
> + if (*cfp & DT_CLSFLAG_COMMIT)
> + dnerror(dnp, D_DREC_COMM,
> + "data-recording actions may not follow commit( )\n");
> + *cfp |= DT_CLSFLAG_DATAREC;
See comment above about setting DT_CLS_FLAG_DATARC from dt_cg_store_val().
> /*
> * Ensure that the format string is a string constant.
> @@ -765,9 +794,23 @@ dt_cg_act_speculate(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> struct bpf_insn instr;
> dt_irlist_t *dlp = &pcb->pcb_ir;
> uint_t off;
> + int *cfp = &pcb->pcb_stmt->dtsd_clauseflags;
> +
> + /* process clause flags */
> + if (*cfp & DT_CLSFLAG_SPECULATE)
> + dnerror(dnp, D_SPEC_SPEC,
> + "speculate( ) may not follow speculate( )\n");
> + if (*cfp & DT_CLSFLAG_COMMIT)
> + dnerror(dnp, D_SPEC_COMM,
> + "speculate( ) may not follow commit( )\n");
> + if (*cfp & DT_CLSFLAG_DATAREC)
> + dnerror(dnp, D_SPEC_DREC,
> + "speculate( ) may not follow data-recording action(s)\n");
> + *cfp |= DT_CLSFLAG_SPECULATE;
>
> dt_cg_node(dnp->dn_args, &pcb->pcb_ir, pcb->pcb_regs);
>
> + /* FIXME: add DT_CLSFLAG_DATAREC? */
No, because the following code is merely a placeholder.
> off = dt_rec_add(pcb->pcb_hdl, dt_cg_fill_gap, DTRACEACT_SPECULATE,
> sizeof(uint64_t), sizeof(uint64_t), NULL,
> DT_ACT_SPECULATE);
> @@ -810,6 +853,13 @@ dt_cg_act_trace(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> char n[DT_TYPE_NAMELEN];
> dt_node_t *arg = dnp->dn_args;
> int type = 0;
> + int *cfp = &pcb->pcb_stmt->dtsd_clauseflags;
> +
> + /* process clause flags */
> + if (*cfp & DT_CLSFLAG_COMMIT)
> + dnerror(dnp, D_DREC_COMM,
> + "data-recording actions may not follow commit( )\n");
> + *cfp |= DT_CLSFLAG_DATAREC;
See comment above about setting DT_CLS_FLAG_DATARC from dt_cg_store_val().
> if (dt_node_is_void(arg)) {
> dnerror(arg, D_TRACE_VOID,
> diff --git a/libdtrace/dt_map.c b/libdtrace/dt_map.c
> index d92259d0..0ea77261 100644
> --- a/libdtrace/dt_map.c
> +++ b/libdtrace/dt_map.c
> @@ -193,6 +193,7 @@ dt_rec_add(dtrace_hdl_t *dtp, dt_cg_gap_f gapf, dtrace_actkind_t kind,
>
> assert(gapf);
>
> + /* make more space if necessary */
> cnt = ddp->dtdd_nrecs + 1;
> max = pcb->pcb_maxrecs;
> if (cnt >= max) {
> @@ -214,6 +215,7 @@ dt_rec_add(dtrace_hdl_t *dtp, dt_cg_gap_f gapf, dtrace_actkind_t kind,
> pcb->pcb_maxrecs = nmax;
> }
>
> + /* add the new record */
> rec = &ddp->dtdd_recs[ddp->dtdd_nrecs++];
> off = (pcb->pcb_bufoff + (alignment - 1)) & ~(alignment - 1);
> rec->dtrd_action = kind;
> @@ -223,10 +225,12 @@ dt_rec_add(dtrace_hdl_t *dtp, dt_cg_gap_f gapf, dtrace_actkind_t kind,
> rec->dtrd_format = pfp;
> rec->dtrd_arg = arg;
>
> + /* fill in the gap */
> gap = off - pcb->pcb_bufoff;
> if (gap > 0)
> (*gapf)(pcb, gap);
>
> + /* update offset */
> pcb->pcb_bufoff = off + size;
>
> return off;
> diff --git a/libdtrace/dtrace.h b/libdtrace/dtrace.h
> index 1353a1fc..080c8175 100644
> --- a/libdtrace/dtrace.h
> +++ b/libdtrace/dtrace.h
> @@ -140,9 +140,17 @@ typedef struct dtrace_stmtdesc {
> void *dtsd_data; /* callback data pointer */
> dtrace_attribute_t dtsd_descattr; /* probedesc attributes */
> dtrace_attribute_t dtsd_stmtattr; /* statement attributes */
> + int dtsd_clauseflags; /* clause flags */
> int dtsd_padding; /* work around GCC bug 36043 */
> } dtrace_stmtdesc_t;
>
> +/* dtsd clause flags */
> +#define DT_CLSFLAG_DATAREC 1 /* data-recording */
> +#define DT_CLSFLAG_SPECULATE 2 /* speculate */
> +#define DT_CLSFLAG_COMMIT 4 /* commit */
> +#define DT_CLSFLAG_EXIT 8 /* exit */
> +#define DT_CLSFLAG_DESTRUCT 16 /* destructive */
> +
> typedef int dtrace_stmt_f(dtrace_hdl_t *dtp, dtrace_prog_t *pgp,
> dtrace_stmtdesc_t *sdp, void *data);
>
> diff --git a/test/unittest/speculation/err.D_COMM_COMM.CommitAftCommit.d b/test/unittest/speculation/err.D_COMM_COMM.CommitAftCommit.d
> index eb2d5378..4b20022c 100644
> --- a/test/unittest/speculation/err.D_COMM_COMM.CommitAftCommit.d
> +++ b/test/unittest/speculation/err.D_COMM_COMM.CommitAftCommit.d
> @@ -4,7 +4,6 @@
> * Licensed under the Universal Permissive License v 1.0 as shown at
> * http://oss.oracle.com/licenses/upl.
> */
> -/* @@xfail: dtv2 */
>
> /*
> * ASSERTION:
> diff --git a/test/unittest/speculation/err.D_COMM_COMM.DisjointCommit.d b/test/unittest/speculation/err.D_COMM_COMM.DisjointCommit.d
> index 3d5e7533..24c7ae7d 100644
> --- a/test/unittest/speculation/err.D_COMM_COMM.DisjointCommit.d
> +++ b/test/unittest/speculation/err.D_COMM_COMM.DisjointCommit.d
> @@ -4,7 +4,6 @@
> * Licensed under the Universal Permissive License v 1.0 as shown at
> * http://oss.oracle.com/licenses/upl.
> */
> -/* @@xfail: dtv2 */
>
> /*
> * ASSERTION:
> diff --git a/test/unittest/speculation/err.D_COMM_DREC.CommitAftDataRec.d b/test/unittest/speculation/err.D_COMM_DREC.CommitAftDataRec.d
> index 6ec8279b..5030b40b 100644
> --- a/test/unittest/speculation/err.D_COMM_DREC.CommitAftDataRec.d
> +++ b/test/unittest/speculation/err.D_COMM_DREC.CommitAftDataRec.d
> @@ -4,7 +4,6 @@
> * Licensed under the Universal Permissive License v 1.0 as shown at
> * http://oss.oracle.com/licenses/upl.
> */
> -/* @@xfail: dtv2 */
>
> /*
> * ASSERTION:
> diff --git a/test/unittest/speculation/err.D_DREC_COMM.DataRecAftCommit.d b/test/unittest/speculation/err.D_DREC_COMM.DataRecAftCommit.d
> index 24dfd1bc..5954e8db 100644
> --- a/test/unittest/speculation/err.D_DREC_COMM.DataRecAftCommit.d
> +++ b/test/unittest/speculation/err.D_DREC_COMM.DataRecAftCommit.d
> @@ -4,7 +4,6 @@
> * Licensed under the Universal Permissive License v 1.0 as shown at
> * http://oss.oracle.com/licenses/upl.
> */
> -/* @@xfail: dtv2 */
>
> /*
> * ASSERTION:
> diff --git a/test/unittest/speculation/err.D_DREC_COMM.ExitAfterCommit.d b/test/unittest/speculation/err.D_DREC_COMM.ExitAfterCommit.d
> index 83de3b7a..79034d1b 100644
> --- a/test/unittest/speculation/err.D_DREC_COMM.ExitAfterCommit.d
> +++ b/test/unittest/speculation/err.D_DREC_COMM.ExitAfterCommit.d
> @@ -4,7 +4,6 @@
> * Licensed under the Universal Permissive License v 1.0 as shown at
> * http://oss.oracle.com/licenses/upl.
> */
> -/* @@xfail: dtv2 */
>
> /*
> * ASSERTION: Exit after commit should throw a D_DREC_COMM.
> diff --git a/test/unittest/speculation/err.D_EXIT_SPEC.ExitAftSpec.d b/test/unittest/speculation/err.D_EXIT_SPEC.ExitAftSpec.d
> index 40cd81e0..650c318a 100644
> --- a/test/unittest/speculation/err.D_EXIT_SPEC.ExitAftSpec.d
> +++ b/test/unittest/speculation/err.D_EXIT_SPEC.ExitAftSpec.d
> @@ -4,7 +4,6 @@
> * Licensed under the Universal Permissive License v 1.0 as shown at
> * http://oss.oracle.com/licenses/upl.
> */
> -/* @@xfail: dtv2 */
>
> /*
> * ASSERTION:
> diff --git a/test/unittest/speculation/err.D_SPEC_COMM.SpecAftCommit.d b/test/unittest/speculation/err.D_SPEC_COMM.SpecAftCommit.d
> index bf0a5112..53bc4ead 100644
> --- a/test/unittest/speculation/err.D_SPEC_COMM.SpecAftCommit.d
> +++ b/test/unittest/speculation/err.D_SPEC_COMM.SpecAftCommit.d
> @@ -4,7 +4,6 @@
> * Licensed under the Universal Permissive License v 1.0 as shown at
> * http://oss.oracle.com/licenses/upl.
> */
> -/* @@xfail: dtv2 */
>
> /*
> * ASSERTION:
> diff --git a/test/unittest/speculation/err.D_SPEC_DREC.SpecAftDataRec.d b/test/unittest/speculation/err.D_SPEC_DREC.SpecAftDataRec.d
> index c2f8205b..820ddb9c 100644
> --- a/test/unittest/speculation/err.D_SPEC_DREC.SpecAftDataRec.d
> +++ b/test/unittest/speculation/err.D_SPEC_DREC.SpecAftDataRec.d
> @@ -4,7 +4,6 @@
> * Licensed under the Universal Permissive License v 1.0 as shown at
> * http://oss.oracle.com/licenses/upl.
> */
> -/* @@xfail: dtv2 */
>
> /*
> * ASSERTION:
> diff --git a/test/unittest/speculation/err.D_SPEC_SPEC.SpecAftSpec.d b/test/unittest/speculation/err.D_SPEC_SPEC.SpecAftSpec.d
> index b87da500..a2663790 100644
> --- a/test/unittest/speculation/err.D_SPEC_SPEC.SpecAftSpec.d
> +++ b/test/unittest/speculation/err.D_SPEC_SPEC.SpecAftSpec.d
> @@ -4,7 +4,6 @@
> * Licensed under the Universal Permissive License v 1.0 as shown at
> * http://oss.oracle.com/licenses/upl.
> */
> -/* @@xfail: dtv2 */
>
> /*
> * ASSERTION:
> --
> 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