[DTrace-devel] [PATCH] Record clause id in mstate
Kris Van Hees
kris.van.hees at oracle.com
Tue Jan 19 17:41:38 PST 2021
On Wed, Jan 20, 2021 at 01:18:02AM +0000, Eugene Loh wrote:
> From: dtrace-devel-bounces at oss.oracle.com <dtrace-devel-bounces at oss.oracle.com>
> on behalf of Kris Van Hees <kris.van.hees at oracle.com>
> Sent: Saturday, January 16, 2021 12:21 AM
> To: dtrace-devel at oss.oracle.com <dtrace-devel at oss.oracle.com>
> Subject: [DTrace-devel] [PATCH] Record clause id in mstate
>
>
> Support for the ERROR probe requires a per-probe unique id to identify
> the clause the ERROR probe was triggered from. Add a clid member to
> the machine state, and initialize it in the prologue.
>
> diff --git a/libdtrace/dt_bpf.h b/libdtrace/dt_bpf.h
> @@ -20,7 +20,8 @@ extern "C" {
>
> #define DT_CONST_EPID 1
> #define DT_CONST_ARGC 2
> -#define DT_CONST_PRID 3
> +#define DT_CONST_CLID 3
> +#define DT_CONST_PRID 4
>
>
> I'm curious what dictates the ordering here? Why insert rather than append?
> Why not a more consistent ordering of these things -- e.g., here, dt_dlibs.c,
> in MST, etc. Even the DT_IDENT_SCALAR case in dt_link_construct(). Not a big
> deal, but uniformity would seem nice.
Hm, I must have made a mistake because the ordering was meant to be EPID, PRID,
CLID, and then ARGC. I'll fix that.
> diff --git a/libdtrace/dt_cc.c b/libdtrace/dt_cc.c
> @@ -2324,7 +2324,8 @@ dt_link_layout(dtrace_hdl_t *dtp, const dtrace_difo_t
> *dp, uint_t *pcp,
> static int
> dt_link_construct(dtrace_hdl_t *dtp, const dt_probe_t *prp, dtrace_difo_t
> *dp,
> const dtrace_difo_t *sdp, dt_strtab_t *stab,
> - uint_t *pcp, uint_t *rcp, uint_t *vcp, dtrace_epid_t
> epid)
> + uint_t *pcp, uint_t *rcp, uint_t *vcp, dtrace_epid_t
> epid,
> + uint_t clid)
> {
> uint_t pc = *pcp;
> uint_t rc = *rcp;
> @@ -2412,6 +2413,9 @@ dt_link_construct(dtrace_hdl_t *dtp, const dt_probe_t
> *prp, dtrace_difo_t *dp,
> case DT_CONST_PRID:
> nrp->dofr_data = prp->desc->id;
> break;
> + case DT_CONST_CLID:
> + nrp->dofr_data = clid;
> + break;
> case DT_CONST_ARGC:
> nrp->dofr_data = 0; /* FIXME */
> break;
> @@ -2427,13 +2431,14 @@ dt_link_construct(dtrace_hdl_t *dtp, const
> dt_probe_t *prp, dtrace_difo_t *dp,
> rdp = dt_dlib_get_func_difo(dtp, idp);
> if (rdp == NULL)
> return -1;
> - if (rdp->dtdo_ddesc != NULL)
> + if (rdp->dtdo_ddesc != NULL) {
> nepid = dt_epid_add(dtp, rdp->dtdo_ddesc,
> prp->desc->id);
> - else
> + clid++;
> + } else
> nepid = 0;
> ipc = dt_link_construct(dtp, prp, dp, rdp, stab,
> - pcp, rcp, vcp, nepid);
> + pcp, rcp, vcp, nepid,
> clid);
> if (ipc == -1)
> return -1;
>
> @@ -2543,7 +2548,8 @@ dt_link(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> dtrace_difo_t *dp)
> if (stab == NULL)
> goto nomem;
>
> - rc = dt_link_construct(dtp, prp, fdp, dp, stab, &insc, &relc, &
> varc, 0);
> + rc = dt_link_construct(dtp, prp, fdp, dp, stab, &insc, &relc, &
> varc,
> + 0, 0);
> dt_dlib_reset(dtp, B_FALSE);
> if (rc == -1)
> goto fail;
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>
> @@ -263,6 +263,7 @@ dt_cg_prologue(dt_pcb_t *pcb, dt_node_t *pred)
> dt_irlist_t *dlp = &pcb->pcb_ir;
> dt_ident_t *epid = dt_dlib_get_var(pcb->pcb_hdl, "EPID");
> dt_ident_t *prid = dt_dlib_get_var(pcb->pcb_hdl, "PRID");
> + dt_ident_t *clid = dt_dlib_get_var(pcb->pcb_hdl, "CLID");
>
> assert(epid != NULL);
> assert(prid != NULL);
>
>
> I cannot tell: should there now be a "+ assert(clid)"?
Yes, thanks.
> @@ -292,6 +293,7 @@ dt_cg_prologue(dt_pcb_t *pcb, dt_node_t *pred)
> * dctx->mst->tstamp = 0; // stdw [%r0 + DMST_TSTAMP], 0
> * dctx->mst->epid = EPID; // stw [%r0 + DMST_EPID], EPID
> * dctx->mst->prid = PRID; // stw [%r0 + DMST_PRID], PRID
> + * dctx->mst->prid = CLID; // stw [%r0 + DMST_CLID], CLID
>
>
> dctx->mst->clid? Or, just skip these comments altogether since they are no
> more readable than the actual code (and clearly the comments and code get out
> of sync).
Yes, forgot to make that update.
>
> * *((uint32_t *)&buf[0]) = EPID;
> * // stw [%r9 + 0], EPID
> */
> @@ -300,6 +302,7 @@ dt_cg_prologue(dt_pcb_t *pcb, dt_node_t *pred)
> emit(dlp, BPF_STORE_IMM(BPF_DW, BPF_REG_0, DMST_TSTAMP, 0));
> emite(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_0, DMST_EPID, -1), epid);
> emite(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_0, DMST_PRID, -1), prid);
> + emite(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_0, DMST_CLID, -1), clid);
> emite(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_9, 0, -1), epid);
>
> /*
> diff --git a/libdtrace/dt_dctx.h b/libdtrace/dt_dctx.h
> @@ -20,6 +20,7 @@
> typedef struct dt_mstate {
> uint32_t epid; /* Enabled probe ID */
> uint32_t prid; /* Probe ID */
> + uint32_t clid; /* Clause ID (unique per probe) */
> uint32_t tag; /* Tag (for future use) */
> uint64_t fault; /* DTrace fault flags */
> uint64_t tstamp; /* cached timestamp value */
> @@ -55,6 +56,7 @@ typedef struct dt_dctx {
>
> #define DMST_EPID offsetof(dt_mstate_t, epid)
> #define DMST_PRID offsetof(dt_mstate_t, prid)
> +#define DMST_CLID offsetof(dt_mstate_t, clid)
> #define DMST_TAG offsetof(dt_mstate_t, tag)
> #define DMST_FAULT offsetof(dt_mstate_t, fault)
> #define DMST_TSTAMP offsetof(dt_mstate_t, tstamp)
> diff --git a/libdtrace/dt_dlibs.c b/libdtrace/dt_dlibs.c
> @@ -74,6 +74,7 @@ static const dt_ident_t dt_bpf_symbols[] =
> {
> /* BPF internal identifiers */
> DT_BPF_SYMBOL_ID(EPID, DT_IDENT_SCALAR, DT_CONST_EPID),
> DT_BPF_SYMBOL_ID(PRID, DT_IDENT_SCALAR, DT_CONST_PRID),
> + DT_BPF_SYMBOL_ID(CLID, DT_IDENT_SCALAR, DT_CONST_CLID),
> DT_BPF_SYMBOL_ID(ARGC, DT_IDENT_SCALAR, DT_CONST_ARGC),
> /* End-of-list marker */
> { NULL, }
>
>
> I don't really follow the code but:
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
For the record... what this code does is actually set a per-probe unique ID
for each clause, assigned in the order that the clauses appear in the script(s),
so that we can do things like report which clause an error occured in.
More information about the DTrace-devel
mailing list