[DTrace-devel] [PATCH 03/19] Widen the EPID to include the PRID
Kris Van Hees
kris.van.hees at oracle.com
Thu Aug 29 20:38:34 UTC 2024
On Thu, Aug 29, 2024 at 09:28:12PM +0100, Sam James wrote:
> "eugene.loh--- via DTrace-devel" <dtrace-devel at oss.oracle.com> writes:
>
> > From: Eugene Loh <eugene.loh at oracle.com>
> >
> > Each output record has an EPID associated with it. When the consumer
> > reads a record, it uses the EPID to look up both a PRID (so it can
> > report PRID and probe function and name) and a data description.
> > During code generation, the PRID and data description are known,
> > and so the EPID is set during relocation.
> >
> > However, we want to support underlying probes that trigger for PRIDs
> > that will be discovered at run time. So, expand the EPID to 64 bits:
> >
> > *) the lower half, known at code generation, will index into an
> > array dtp->dt_stmts[] of statements, which have information
> > about probe descriptions, data descriptions, and clauses to call
> >
> > *) the upper half will have the PRID, which will be discovered
> > at run time
> >
> > Statement descriptions include a new member dtsd_index, which is
> > the index to dtp->dt_stmts[].
> >
> > The consumer gets information about the firing probe from the PRID,
> > an index to dtp->dt_probes[].
> >
> > The lower half "EPID" depends only on the statement and no longer
> > on the specific probe that is firing. Its value is known at code
> > generation for a compiled clause. It no longer needs to be set in
> > relocation; that relocation will be removed in a future patch.
> >
> > The buffer memory pointed to by dctx->buf is rearranged so that EPID
> > will (still) fall on an 8-byte boundary but now take up 8 bytes.
> >
> > We do not need to expand:
> >
> > - the mstate (since it already had the PRID)
> >
> > Rename mstate's epid to be a statement ID, stid. When
> > the producer needs the epid, it can construct it from
> > mst->prid and mst->stid.
> >
> > - the output buffer (since it already had unused padding)
> >
> > The new, wider EPID is seen for the built-in variable epid as well
> > as the epid that the consumer reads.
> >
> > The probe descriptions dt_pdesc become superfluous and will be
> > eliminated in a future patch.
> >
> > Update test files:
> >
> > - Where the upper 32 bits of the EPID are known (since the PRID
> > corresponds to a dtrace::: probe), update the expected EPID in
> > the .r files.
> >
> > - Where the upper 32 bits of the EPID are not known, add .r.p
> > processing to filter those bits out before comparing to known
> > results in .r files.
> >
> > - Increase expected EPID width.
> >
> > Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> > ---
> > bpf/get_bvar.c | 6 ++-
> > bpf/probe_error.c | 2 +-
> > include/dtrace/universal.h | 2 +-
> > libdtrace/dt_bpf.c | 2 -
> > libdtrace/dt_cc.c | 13 ++---
> > libdtrace/dt_cg.c | 49 +++++++++++--------
> > libdtrace/dt_consume.c | 28 +++++++----
> > libdtrace/dt_dctx.h | 16 +++---
> > libdtrace/dt_handle.c | 9 ++--
> > libdtrace/dt_impl.h | 1 +
> > libdtrace/dt_map.c | 11 +++--
> > libdtrace/dt_open.c | 2 +-
> > libdtrace/dt_program.c | 7 +++
> > libdtrace/dtrace.h | 1 +
> > test/demo/dtrace/error.r | 2 +-
> > test/stress/buffering/tst.resize3-manual.r | 2 +-
> > test/stress/buffering/tst.resize3.r | 2 +-
> > test/unittest/actions/setopt/tst.badopt.r | 14 +++---
> > .../arrays/tst.declared-bounds.runtime_out.r | 2 +-
> > test/unittest/codegen/err.deref_0.r | 2 +-
> > test/unittest/codegen/err.deref_1.r | 2 +-
> > test/unittest/codegen/err.deref_i0.r | 2 +-
> > test/unittest/codegen/err.deref_i1.r | 2 +-
> > .../unittest/codegen/err.deref_string-assoc.r | 2 +-
> > test/unittest/codegen/err.deref_string-gvar.r | 2 +-
> > test/unittest/codegen/err.deref_string-lvar.r | 2 +-
> > test/unittest/codegen/err.deref_string-tvar.r | 2 +-
> > .../codegen/err.str_NULL_plus_offset-assoc.r | 2 +-
> > .../codegen/err.str_NULL_plus_offset-lvar.r | 2 +-
> > .../codegen/err.str_NULL_plus_offset-tvar.r | 2 +-
> > .../codegen/err.str_NULL_plus_offset.r | 2 +-
> > test/unittest/disasm/tst.vartab-bvar.r | 2 +-
> > test/unittest/drops/drp.DTRACEDROP_DBLERROR.r | 2 +-
> > .../tst.DTRACEFLT_BADADDR.null_ptr_field.r | 2 +-
> > test/unittest/error/tst.DTRACEFLT_BADADDR.r | 4 +-
> > test/unittest/error/tst.DTRACEFLT_BADADDR2.r | 4 +-
> > .../error/tst.DTRACEFLT_DIVZERO.div.r | 2 +-
> > .../error/tst.DTRACEFLT_DIVZERO.mod.r | 2 +-
> > test/unittest/error/tst.DTRACEFLT_UNKNOWN.r | 4 +-
> > .../error/tst.clause_scope-begin-ended.r | 2 +-
> > test/unittest/error/tst.clause_scope-begin.r | 2 +-
> > .../unittest/error/tst.clause_scope-regular.r | 2 +-
> > .../error/tst.clause_scope-regular.r.p | 12 ++++-
> > test/unittest/error/tst.error.r | 2 +-
> > test/unittest/error/tst.errorend.r | 2 +-
> > .../alloca/err.alloca-bcopy-before-beyond.r | 2 +-
> > .../alloca/err.alloca-bcopy-before-bottom.r | 2 +-
> > .../alloca/err.alloca-bcopy-beyond-top.r | 2 +-
> > .../alloca/err.alloca-bcopy-crossing-bottom.r | 2 +-
> > .../alloca/err.alloca-bcopy-crossing-top.r | 2 +-
> > .../alloca/err.alloca-crossing-clauses.r | 2 +-
> > .../alloca/err.alloca-load-before-bottom.r | 2 +-
> > .../funcs/alloca/err.alloca-load-beyond-top.r | 2 +-
> > .../alloca/err.alloca-load-crossing-bottom.r | 2 +-
> > .../alloca/err.alloca-null-deref-lvalue.r | 2 +-
> > .../funcs/alloca/err.alloca-null-deref.r | 2 +-
> > .../err.alloca-scratch-exceeding-bcopy.r | 2 +-
> > .../alloca/err.alloca-store-before-bottom.r | 2 +-
> > .../alloca/err.alloca-store-beyond-top.r | 2 +-
> > .../alloca/err.alloca-store-crossing-bottom.r | 2 +-
> > test/unittest/funcs/bcopy/err.badbcopy1.r | 2 +-
> > test/unittest/funcs/bcopy/err.badbcopy4.r | 2 +-
> > test/unittest/funcs/bcopy/err.badbcopy5.r | 2 +-
> > test/unittest/funcs/bcopy/err.badbcopy6.r | 2 +-
> > test/unittest/funcs/bcopy/err.badbcopy7.r | 2 +-
> > test/unittest/funcs/bcopy/err.badbcopy8.r | 2 +-
> > test/unittest/funcs/copyin/err.badaddr.r | 2 +-
> > test/unittest/funcs/copyin/err.null_arg1.r | 2 +-
> > test/unittest/funcs/copyinstr/err.badaddr.r | 2 +-
> > test/unittest/funcs/copyinstr/err.null_arg1.r | 2 +-
> > test/unittest/funcs/copyinto/err.badaddr.r | 2 +-
> > test/unittest/funcs/copyinto/err.badsize.r | 2 +-
> > test/unittest/funcs/copyinto/err.null_arg1.r | 2 +-
> > test/unittest/funcs/err.badalloca.r | 2 +-
> > test/unittest/funcs/err.badalloca.r.p | 12 ++++-
> > test/unittest/funcs/err.link_ntopbadaddr.r | 2 +-
> > test/unittest/funcs/err.link_ntopbadarg.r | 2 +-
> > .../inet_ntoa6/err.inet_ntoa6.arg1_null.r | 2 +-
> > .../err.inet_ntoa6.arg1_null_const.r | 2 +-
> > test/unittest/funcs/strlen/tst.null.r | 2 +-
> > test/unittest/funcs/strtok/tst.strtok_null.r | 2 +-
> > .../funcs/strtok/tst.strtok_nulldel.r | 2 +-
> > .../funcs/strtok/tst.strtok_nullstr.r | 2 +-
> > .../funcs/strtok/tst.strtok_nullstr2.r | 2 +-
> > .../funcs/substr/err.substr_null_arg1.r | 2 +-
> > test/unittest/pointers/err.AllocaOverrun.r | 2 +-
> > test/unittest/pointers/err.BadAlign.r | 2 +-
> > test/unittest/pointers/err.InvalidAddress2.r | 2 +-
> > test/unittest/pointers/err.InvalidAddress4.r | 2 +-
> > .../speculation/err.CommitWithInvalid.r | 2 +-
> > .../speculation/err.DiscardWithInvalid.r | 2 +-
> > test/unittest/speculation/tst.negcommit.r | 2 +-
> > 92 files changed, 197 insertions(+), 146 deletions(-)
> >
> > diff --git a/bpf/get_bvar.c b/bpf/get_bvar.c
> > index a0c04f3a..e9733bb1 100644
> > --- a/bpf/get_bvar.c
> > +++ b/bpf/get_bvar.c
> > @@ -48,8 +48,10 @@ noinline uint64_t dt_get_bvar(const dt_dctx_t *dctx, uint32_t id, uint32_t idx)
> > mst->tstamp = bpf_ktime_get_ns();
> >
> > return mst->tstamp;
> > - case DIF_VAR_EPID:
> > - return mst->epid;
> > + case DIF_VAR_EPID: {
> > + uint64_t val = mst->prid;
> > + return (val << 32) | mst->stid;
> > + }
> > case DIF_VAR_ID:
> > return mst->prid;
> > case DIF_VAR_ARG0: case DIF_VAR_ARG1: case DIF_VAR_ARG2:
> > diff --git a/bpf/probe_error.c b/bpf/probe_error.c
> > index ee1a1793..fba779a8 100644
> > --- a/bpf/probe_error.c
> > +++ b/bpf/probe_error.c
> > @@ -29,7 +29,7 @@ noinline void dt_probe_error(const dt_dctx_t *dctx, uint64_t pc, uint64_t fault,
> > int oldprid = mst->prid;
> >
> > mst->argv[0] = 0;
> > - mst->argv[1] = mst->epid;
> > + mst->argv[1] = (((uint64_t)mst->prid) << 32) | mst->stid;
> > mst->argv[2] = mst->clid;
> > mst->argv[3] = pc;
> > mst->argv[4] = fault;
> > diff --git a/include/dtrace/universal.h b/include/dtrace/universal.h
> > index d6562489..655ea772 100644
> > --- a/include/dtrace/universal.h
> > +++ b/include/dtrace/universal.h
> > @@ -37,7 +37,7 @@ typedef uint16_t dtrace_actkind_t; /* action kind */
> >
> > typedef uint32_t dtrace_aggid_t; /* aggregation identifier */
> > typedef uint32_t dtrace_cacheid_t; /* predicate cache identifier */
> > -typedef uint32_t dtrace_epid_t; /* enabled probe identifier */
> > +typedef uint64_t dtrace_epid_t; /* enabled probe identifier */
> > typedef uint32_t dtrace_optid_t; /* option identifier */
> > typedef uint32_t dtrace_specid_t; /* speculation identifier */
> >
> > diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> > index 70597d65..3f9c42ea 100644
> > --- a/libdtrace/dt_bpf.c
> > +++ b/libdtrace/dt_bpf.c
> > @@ -774,7 +774,6 @@ gmap_create_cpuinfo(dtrace_hdl_t *dtp)
> > * The size of the memory region is the sum of:
> > * - size of the DTrace machine state, rounded up to the nearest
> > * multiple of 8
> > - * - 8 bytes padding for trace buffer alignment purposes
> > * - maximum trace buffer record size, rounded up to the nearest
> > * multiple of 8
> > * - size of dctx->mem (see dt_dctx.h)
> > @@ -783,7 +782,6 @@ static int
> > gmap_create_mem(dtrace_hdl_t *dtp)
> > {
> > size_t sz = roundup(sizeof(dt_mstate_t), 8) +
> > - 8 +
> > roundup(dtp->dt_maxreclen, 8) +
> > DMEM_SIZE(dtp);
> >
> > diff --git a/libdtrace/dt_cc.c b/libdtrace/dt_cc.c
> > index d1ee3843..cf3c5504 100644
> > --- a/libdtrace/dt_cc.c
> > +++ b/libdtrace/dt_cc.c
> > @@ -123,6 +123,7 @@ dt_stmt_create(dtrace_hdl_t *dtp, dtrace_ecbdesc_t *edp,
> > if (sdp == NULL)
> > longjmp(yypcb->pcb_jmpbuf, EDT_NOMEM);
> >
> > + sdp->dtsd_index = dtp->dt_clause_nextid++;
> > assert(yypcb->pcb_stmt == NULL);
> > yypcb->pcb_stmt = sdp;
> > yypcb->pcb_maxrecs = 0;
> > @@ -133,8 +134,8 @@ dt_stmt_create(dtrace_hdl_t *dtp, dtrace_ecbdesc_t *edp,
> > return sdp;
> > }
> >
> > -static dt_ident_t *
> > -dt_clause_create(dtrace_hdl_t *dtp, dtrace_difo_t *dp)
> > +static void
> > +dt_clause_create(dtrace_hdl_t *dtp, dtrace_difo_t *dp, dtrace_stmtdesc_t *sdp)
> > {
> > char *name;
> > int len;
> > @@ -156,12 +157,12 @@ dt_clause_create(dtrace_hdl_t *dtp, dtrace_difo_t *dp)
> > /*
> > * Generate a symbol name.
> > */
> > - len = snprintf(NULL, 0, "dt_clause_%d", dtp->dt_clause_nextid) + 1;
> > + len = snprintf(NULL, 0, "dt_clause_%d", sdp->dtsd_index) + 1;
> > name = dt_alloc(dtp, len);
> > if (name == NULL)
> > longjmp(yypcb->pcb_jmpbuf, EDT_NOMEM);
> >
> > - snprintf(name, len, "dt_clause_%d", dtp->dt_clause_nextid++);
> > + snprintf(name, len, "dt_clause_%d", sdp->dtsd_index);
> >
> > /*
> > * Add the symbol to the BPF identifier table and associate the DIFO
> > @@ -174,7 +175,7 @@ dt_clause_create(dtrace_hdl_t *dtp, dtrace_difo_t *dp)
> >
> > dt_ident_set_data(idp, dp);
> >
> > - return idp;
> > + sdp->dtsd_clause = idp;
> > }
> >
> > static void
> > @@ -214,7 +215,7 @@ dt_compile_one_clause(dtrace_hdl_t *dtp, dt_node_t *cnp, dt_node_t *pnp)
> > * Compile the clause (predicate and action).
> > */
> > dt_cg(yypcb, cnp);
> > - sdp->dtsd_clause = dt_clause_create(dtp, dt_as(yypcb));
> > + dt_clause_create(dtp, dt_as(yypcb), sdp);
> >
> > assert(yypcb->pcb_stmt == sdp);
> > if (dtrace_stmt_add(yypcb->pcb_hdl, yypcb->pcb_prog, sdp) != 0)
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > index a7861829..2c559868 100644
> > --- a/libdtrace/dt_cg.c
> > +++ b/libdtrace/dt_cg.c
> > @@ -277,15 +277,9 @@ dt_cg_tramp_prologue_act(dt_pcb_t *pcb, dt_activity_t act)
> > * buf = rc + roundup(sizeof(dt_mstate_t), 8);
> > * // add %r0, roundup(
> > * sizeof(dt_mstate_t), 8)
> > - * *((uint64_t *)&buf[0]) = 0;
> > - * // stdw [%r0 + 0], 0
> > - * buf += 8; // add %r0, 8
> > - * // (%r0 = pointer to buffer space)
> > * dctx.buf = buf; // stdw [%r9 + DCTX_BUF], %r0
> > */
> > emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, roundup(sizeof(dt_mstate_t), 8)));
> > - emit(dlp, BPF_STORE_IMM(BPF_DW, BPF_REG_0, 0, 0));
> > - emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 8));
> > emit(dlp, BPF_STORE(BPF_DW, BPF_REG_9, DCTX_BUF, BPF_REG_0));
> >
> > /*
> > @@ -1057,10 +1051,8 @@ static void
> > 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 *clid = dt_dlib_get_var(pcb->pcb_hdl, "CLID");
> >
> > - assert(epid != NULL);
> > assert(clid != NULL);
> >
> > /*
> > @@ -1093,18 +1085,22 @@ dt_cg_prologue(dt_pcb_t *pcb, dt_node_t *pred)
> > * // stdw [%r0 + DMST_FAULT], 0
> > * dctx->mst->tstamp = 0; // stdw [%r0 + DMST_TSTAMP], 0
> > * dctx->mst->specsize = 0;// stdw [%r0 + DMST_SPECSIZE], 0
> > - * dctx->mst->epid = EPID; // stw [%r0 + DMST_EPID], EPID
> > + * dctx->mst->stid = STID; // stw [%r0 + DMST_STID], STID
> > * dctx->mst->clid = CLID; // stw [%r0 + DMST_CLID], CLID
> > - * *((uint32_t *)&buf[DBUF_EPID]) = EPID;
> > - * // stw [%r9 + DBUF_EPID], EPID
> > */
> > emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_0, DCTX_MST));
> > emit(dlp, BPF_STORE_IMM(BPF_DW, BPF_REG_0, DMST_FAULT, 0));
> > emit(dlp, BPF_STORE_IMM(BPF_DW, BPF_REG_0, DMST_TSTAMP, 0));
> > emit(dlp, BPF_STORE_IMM(BPF_DW, BPF_REG_0, DMST_SPECSIZE, 0));
> > - emite(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_0, DMST_EPID, -1), epid);
> > + emit(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_0, DMST_STID, pcb->pcb_stmt->dtsd_index));
> > emite(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_0, DMST_CLID, -1), clid);
> > - emite(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_9, DBUF_EPID, -1), epid);
> > +
> > + /*
> > + * Zero out the leading 4 bytes of the buffer.
> > + * *((uint32_t *)&buf[DBUF_PAD]) = 0;
> > + * // stw [%r9 + DBUF_PAD], 0
> > + */
> > + emit(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_9, DBUF_PAD, 0));
> >
> > /*
> > * Set the speculation ID field to zero to indicate no active
> > @@ -1114,6 +1110,18 @@ dt_cg_prologue(dt_pcb_t *pcb, dt_node_t *pred)
> > */
> > emit(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_9, DBUF_SPECID, 0));
> >
> > + /*
> > + * *((uint64_t *)&buf[DBUF_EPID]) = (dctx->mst->prid << 32) | stid;
> > + * // ld %r1, [%r0 + DMST_PRID]
> > + * // lsh %r1, 32
> > + * // or %r1, stid
> > + * // stdw [%r9 + DBUF_EPID], %r1
> > + */
> > + emit (dlp, BPF_LOAD(BPF_W, BPF_REG_1, BPF_REG_0, DMST_PRID));
> > + emit (dlp, BPF_ALU64_IMM(BPF_LSH, BPF_REG_1, 32));
> > + emit (dlp, BPF_ALU64_IMM(BPF_OR, BPF_REG_1, pcb->pcb_stmt->dtsd_index));
> > + emit (dlp, BPF_STORE(BPF_DW, BPF_REG_9, DBUF_EPID, BPF_REG_1));
> > +
> > /*
> > * If there is a predicate:
> > *
> > @@ -1132,10 +1140,9 @@ dt_cg_prologue(dt_pcb_t *pcb, dt_node_t *pred)
> > TRACE_REGSET("Prologue: End ");
> >
> > /*
> > - * Account for 32-bit EPID (at offset 0) and 32-bit speculation ID (at
> > - * offset 4).
> > + * Set the offset for the beginning of trace data.
> > */
> > - pcb->pcb_bufoff += 2 * sizeof(uint32_t);
> > + pcb->pcb_bufoff = DBUF_DATA;
> > }
> >
> > /*
> > @@ -1170,15 +1177,15 @@ dt_cg_epilogue(dt_pcb_t *pcb)
> > /*
> > * rc = bpf_perf_event_output(dctx->ctx, &buffers,
> > * BPF_F_CURRENT_CPU,
> > - * buf - 4, bufoff + 4);
> > + * 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
> > + * // add %r4, 4
> > * // mov %r5, pcb->pcb_bufoff
> > - * // add %r5, 4
> > + * // add %r5, -4
> > * // call bpf_perf_event_output
> > */
> > emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_DCTX));
> > @@ -1186,9 +1193,9 @@ dt_cg_epilogue(dt_pcb_t *pcb)
> > 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);
> > emit(dlp, BPF_MOV_REG(BPF_REG_4, BPF_REG_9));
> > - emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, -4));
> > + emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, 4));
> > emit(dlp, BPF_MOV_IMM(BPF_REG_5, pcb->pcb_bufoff));
> > - emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_5, 4));
> > + emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_5, -4));
> > emit(dlp, BPF_CALL_HELPER(BPF_FUNC_perf_event_output));
> >
> > /*
> > diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
> > index 51eb6c80..e3ce2d3b 100644
> > --- a/libdtrace/dt_consume.c
> > +++ b/libdtrace/dt_consume.c
> > @@ -15,9 +15,11 @@
> > #include <alloca.h>
> > #include <dt_impl.h>
> > #include <dt_aggregate.h>
> > +#include <dt_dctx.h>
> > #include <dt_module.h>
> > #include <dt_pcap.h>
> > #include <dt_peb.h>
> > +#include <dt_probe.h>
> > #include <dt_state.h>
> > #include <dt_string.h>
> > #include <libproc.h>
> > @@ -479,7 +481,7 @@ dt_flowindent(dtrace_hdl_t *dtp, dtrace_probedata_t *data, dtrace_epid_t last)
> > */
> > if (flow == DTRACEFLOW_ENTRY) {
> > if (last != DTRACE_EPIDNONE && id != last &&
> > - pd->id == dtp->dt_pdesc[last]->id)
> > + pd->id == dtp->dt_probes[last >> 32]->desc->id)
> > flow = DTRACEFLOW_NONE;
> > }
> >
> > @@ -2184,19 +2186,23 @@ dt_consume_one_probe(dtrace_hdl_t *dtp, FILE *fp, char *data, uint32_t size,
> > int peekflags, dtrace_epid_t *last, int committing,
> > void *arg)
> > {
> > + int specid;
> > dtrace_epid_t epid;
> > + uint32_t prid;
> > dtrace_datadesc_t *epd;
> > dt_spec_buf_t tmpl;
> > dt_spec_buf_t *dtsb;
> > - int specid;
> > int i;
> > int rval;
> > dtrace_workstatus_t ret;
> > int commit_discard_seen, only_commit_discards;
> > int data_recording = 1;
> >
> > - epid = ((uint32_t *)data)[0];
> > - specid = ((uint32_t *)data)[1];
> > + specid = *((uint32_t *)(data + DBUF_SPECID));
> > + epid = *((uint64_t *)(data + DBUF_EPID));
> > + prid = epid >> 32;
> > + if (prid > dtp->dt_probe_id)
> > + return dt_set_errno(dtp, EDT_BADEPID);
> >
> > /*
> > * Fill in the epid and address of the epid in the buffer. We need to
> > @@ -2209,6 +2215,7 @@ dt_consume_one_probe(dtrace_hdl_t *dtp, FILE *fp, char *data, uint32_t size,
> > &pdat->dtpda_pdesc);
> > if (rval != 0)
> > return dt_set_errno(dtp, EDT_BADEPID);
> > + pdat->dtpda_pdesc = (dtrace_probedesc_t *)dtp->dt_probes[prid]->desc;
> >
> > epd = pdat->dtpda_ddesc;
> > if (epd->dtdd_uarg != DT_ECB_DEFAULT) {
> > @@ -2639,9 +2646,8 @@ dt_consume_one(dtrace_hdl_t *dtp, FILE *fp, char *buf,
> > * struct {
> > * struct perf_event_header header;
> > * uint32_t size;
> > - * uint32_t pad;
> > - * uint32_t epid;
> > * uint32_t specid;
> > + * dtrace_epid_t epid;
> > * uint64_t data[n];
> > * }
> > * and 'data' points to the 'size' member at this point.
> > @@ -2651,13 +2657,17 @@ dt_consume_one(dtrace_hdl_t *dtp, FILE *fp, char *buf,
> > return dt_set_errno(dtp, EDT_DSIZE);
> >
> > size = *(uint32_t *)data;
> > - data += sizeof(size);
> > ptr += sizeof(size) + size;
> > if (ptr != buf + hdr->size)
> > return dt_set_errno(dtp, EDT_DSIZE);
> >
> > - data += sizeof(uint32_t); /* skip padding */
> > - size -= sizeof(uint32_t);
> > + /*
> > + * The "size" measures from specid to the end. But our buffer
> > + * offsets are relative to &size itself, to preserve 8-byte
> > + * alignment. So, we leave data pointing at &size, and we
> > + * increase size by 4 bytes.
> > + */
> > + size += 4;
> >
> > return dt_consume_one_probe(dtp, fp, data, size, pdat, efunc,
> > rfunc, flow, quiet, peekflags,
> > diff --git a/libdtrace/dt_dctx.h b/libdtrace/dt_dctx.h
> > index 1422ad24..6592bef1 100644
> > --- a/libdtrace/dt_dctx.h
> > +++ b/libdtrace/dt_dctx.h
> > @@ -18,7 +18,7 @@
> > * The DTrace machine state.
> > */
> > typedef struct dt_mstate {
> > - uint32_t epid; /* Enabled probe ID */
> > + uint32_t stid; /* Statement ID */
> > uint32_t prid; /* Probe ID */
> > uint32_t clid; /* Clause ID (unique per probe) */
> > uint32_t tag; /* Tag (for future use) */
> > @@ -33,7 +33,7 @@ typedef struct dt_mstate {
> > uint64_t saved_argv[10]; /* Saved probe arguments */
> > } dt_mstate_t;
> >
> > -#define DMST_EPID offsetof(dt_mstate_t, epid)
> > +#define DMST_STID offsetof(dt_mstate_t, stid)
> > #define DMST_PRID offsetof(dt_mstate_t, prid)
> > #define DMST_CLID offsetof(dt_mstate_t, clid)
> > #define DMST_TAG offsetof(dt_mstate_t, tag)
> > @@ -82,16 +82,20 @@ typedef struct dt_dctx {
> > * The dctx->buf pointer references a block of memory that contains:
> > *
> > * +----------------+
> > - * 0 -> | EPID |
> > + * 0 -> | pad |
> > * +----------------+
> > - * 4 -> | Speculation ID |
> > + * 4 -> | Speculation ID |
> > * +----------------+
> > - * | Trace Data |
> > + * 8 -> | EPID |
> > + * +----------------+
> > + * 16 -> | Trace Data |
> > * | ... |
> > * +----------------+
> > */
> > -#define DBUF_EPID 0
> > +#define DBUF_PAD 0
> > #define DBUF_SPECID 4
> > +#define DBUF_EPID 8
> > +#define DBUF_DATA 16
> >
> > /*
> > * The dctx->mem pointer references a block of memory that contains:
> > diff --git a/libdtrace/dt_handle.c b/libdtrace/dt_handle.c
> > index 4c9b9413..b1ba5f9f 100644
> > --- a/libdtrace/dt_handle.c
> > +++ b/libdtrace/dt_handle.c
> > @@ -14,6 +14,7 @@
> > #include <alloca.h>
> >
> > #include <dt_impl.h>
> > +#include <dt_probe.h>
> > #include <dt_program.h>
> >
> > static const char _dt_errprog[] =
> > @@ -147,11 +148,11 @@ dt_handle_err(dtrace_hdl_t *dtp, dtrace_probedata_t *data)
> > * This is an error. We have the following items here: EPID,
> > * faulting action, BPF pc, fault code and faulting address.
> > */
> > - epid = (uint32_t)DT_REC(uint64_t, 0);
> > + epid = DT_REC(uint64_t, 0);
> >
> > if (dt_epid_lookup(dtp, epid, &errdd, &errpd) != 0)
> > return dt_set_errno(dtp, EDT_BADERROR);
> > -
> > + errpd = (dtrace_probedesc_t *)dtp->dt_probes[epid>>32]->desc;
> > err.dteda_ddesc = errdd;
> > err.dteda_pdesc = errpd;
> > err.dteda_cpu = data->dtpda_cpu;
> > @@ -195,7 +196,7 @@ no_addr:
> > details[0] = 0;
> > }
> >
> > - snprintf(str, len, "error on enabled probe ID %u (ID %u: %s:%s:%s:%s): "
> > + snprintf(str, len, "error on enabled probe ID %lu (ID %u: %s:%s:%s:%s): "
> > "%s%s in %s%s",
> > epid, errpd->id, errpd->prv, errpd->mod, errpd->fun,
> > errpd->prb, dtrace_faultstr(dtp, err.dteda_fault), details,
> > @@ -256,7 +257,7 @@ dt_handle_liberr(dtrace_hdl_t *dtp, const dtrace_probedata_t *data,
> > str = alloca(len);
> >
> > snprintf(str, len,
> > - "error on enabled probe ID %u (ID %u: %s:%s:%s:%s): %s",
> > + "error on enabled probe ID %lu (ID %u: %s:%s:%s:%s): %s",
> > data->dtpda_epid, errpd->id, errpd->prv, errpd->mod,
> > errpd->fun, errpd->prb, faultstr);
> >
> > diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> > index 98fddc23..a2ae84f6 100644
> > --- a/libdtrace/dt_impl.h
> > +++ b/libdtrace/dt_impl.h
> > @@ -275,6 +275,7 @@ struct dtrace_hdl {
> > dt_list_t dt_programs; /* linked list of dtrace_prog_t's */
> > dt_list_t dt_xlators; /* linked list of dt_xlator_t's */
> > dt_list_t dt_enablings; /* list of (to be) enabled probes */
> > + dtrace_stmtdesc_t **dt_stmts; /* array of stmts */
> > struct dt_xlator **dt_xlatormap; /* dt_xlator_t's indexed by dx_id */
> > id_t dt_xlatorid; /* next dt_xlator_t id to assign */
> > dt_ident_t *dt_externs; /* linked list of external symbol identifiers */
> > diff --git a/libdtrace/dt_map.c b/libdtrace/dt_map.c
> > index 60a2eca2..87ce5707 100644
> > --- a/libdtrace/dt_map.c
> > +++ b/libdtrace/dt_map.c
> > @@ -138,14 +138,17 @@ int
> > dt_epid_lookup(dtrace_hdl_t *dtp, dtrace_epid_t epid, dtrace_datadesc_t **ddp,
> > dtrace_probedesc_t **pdp)
> > {
> > - if (epid >= dtp->dt_maxprobe ||
> > - dtp->dt_ddesc[epid] == NULL || dtp->dt_pdesc[epid] == NULL)
> > + /* Remove the PRID portion of the EPID. */
> > + epid &= 0xffffffff;
> > +
> > + if (epid >= dtp->dt_clause_nextid)
> > return -1;
> >
> > - *ddp = dtp->dt_ddesc[epid];
> > + dtrace_difo_t *rdp = dt_dlib_get_func_difo(dtp, dtp->dt_stmts[epid]->dtsd_clause);
> > + *ddp = dt_datadesc_hold(rdp->dtdo_ddesc); // FIXME what releases the hold?
> > *pdp = dtp->dt_pdesc[epid];
> >
> > - return 0;
> > + return (*ddp == NULL) ? -1 : 0;
> > }
> >
> > void
> > diff --git a/libdtrace/dt_open.c b/libdtrace/dt_open.c
> > index 77ffb6d2..8ae6cdfa 100644
> > --- a/libdtrace/dt_open.c
> > +++ b/libdtrace/dt_open.c
> > @@ -170,7 +170,7 @@ static const dt_ident_t _dtrace_globals[] = {
> > { "discard", DT_IDENT_ACTFUNC, 0, DT_ACT_DISCARD, DT_ATTR_STABCMN, DT_VERS_1_0,
> > &dt_idops_func, "void(int)" },
> > { "epid", DT_IDENT_SCALAR, 0, DIF_VAR_EPID, DT_ATTR_STABCMN, DT_VERS_1_0,
> > - &dt_idops_type, "uint_t" },
> > + &dt_idops_type, "uint64_t" },
> > { "errno", DT_IDENT_SCALAR, 0, DIF_VAR_ERRNO, DT_ATTR_STABCMN, DT_VERS_1_0,
> > &dt_idops_type, "int" },
> > { "execname", DT_IDENT_SCALAR, 0, DIF_VAR_EXECNAME,
> > diff --git a/libdtrace/dt_program.c b/libdtrace/dt_program.c
> > index afbf7265..ee743589 100644
> > --- a/libdtrace/dt_program.c
> > +++ b/libdtrace/dt_program.c
> > @@ -165,6 +165,13 @@ dt_prog_stmt(dtrace_hdl_t *dtp, dtrace_prog_t *pgp, dtrace_stmtdesc_t *sdp,
> > dtrace_probedesc_t *pdp = &sdp->dtsd_ecbdesc->dted_probe;
> > int rc;
> >
> > + if (dtp->dt_stmts == NULL) {
> > + dtp->dt_stmts = dt_calloc(dtp, dtp->dt_clause_nextid, sizeof(dtrace_stmtdesc_t *));
> > + if (dtp->dt_stmts == NULL)
> > + return dt_set_errno(dtp, EDT_NOMEM);
> > + }
> > + dtp->dt_stmts[sdp->dtsd_index] = sdp;
> > +
> > st.cnt = cnt;
> > st.sdp = sdp;
> > rc = dt_probe_iter(dtp, pdp, (dt_probe_f *)dt_stmt_probe, NULL, &st);
> > diff --git a/libdtrace/dtrace.h b/libdtrace/dtrace.h
> > index 09a87977..a23052e4 100644
> > --- a/libdtrace/dtrace.h
> > +++ b/libdtrace/dtrace.h
> > @@ -150,6 +150,7 @@ typedef struct dtrace_stmtdesc {
> > dtrace_attribute_t dtsd_descattr; /* probedesc attributes */
> > dtrace_attribute_t dtsd_stmtattr; /* statement attributes */
> > int dtsd_clauseflags; /* clause flags */
> > + int dtsd_index; /* index in dtp->dt_stmts */
> > } dtrace_stmtdesc_t;
> >
> > /* dtsd clause flags */
> > diff --git a/test/demo/dtrace/error.r b/test/demo/dtrace/error.r
> > index d3904f47..50b52370 100644
> > --- a/test/demo/dtrace/error.r
> > +++ b/test/demo/dtrace/error.r
> > @@ -3,4 +3,4 @@
> >
> > -- @@stderr --
> > dtrace: script 'test/demo/dtrace/error.d' matched 2 probes
> > -dtrace: error on enabled probe ID 3 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #1 at BPF pc NNN
> > +dtrace: error on enabled probe ID 4294967296 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #1 at BPF pc NNN
> > diff --git a/test/stress/buffering/tst.resize3-manual.r b/test/stress/buffering/tst.resize3-manual.r
> > index 43b647c7..5493783e 100644
> > --- a/test/stress/buffering/tst.resize3-manual.r
> > +++ b/test/stress/buffering/tst.resize3-manual.r
> > @@ -1,5 +1,5 @@
> > FUNCTION:NAME
> > - :BEGIN 3
> > + :BEGIN 4294967297
> > :BEGIN
> >
> > -- @@stderr --
> > diff --git a/test/stress/buffering/tst.resize3.r b/test/stress/buffering/tst.resize3.r
> > index 9c471158..807c4d1c 100644
> > --- a/test/stress/buffering/tst.resize3.r
> > +++ b/test/stress/buffering/tst.resize3.r
> > @@ -1,5 +1,5 @@
> > FUNCTION:NAME
> > - :BEGIN 3
> > + :BEGIN 4294967297
> > :BEGIN
> >
> > -- @@stderr --
> > diff --git a/test/unittest/actions/setopt/tst.badopt.r b/test/unittest/actions/setopt/tst.badopt.r
> > index 29e39fd4..9373951c 100644
> > --- a/test/unittest/actions/setopt/tst.badopt.r
> > +++ b/test/unittest/actions/setopt/tst.badopt.r
> > @@ -1,16 +1,16 @@
> >
> > -- @@stderr --
> > -dtrace: error on enabled probe ID 2 (ID 1: dtrace:::BEGIN): couldn't set option "Nixon" to "1": Invalid option name
> > +dtrace: error on enabled probe ID 4294967296 (ID 1: dtrace:::BEGIN): couldn't set option "Nixon" to "1": Invalid option name
> >
>
> I'm very much *not* familiar enough to say if this is intended but can I
> ask if it is?
>
> That starts to look kind of inelegant (to print such a large ID for a
> basic bad option diagnostic), where the probe wasn't doing anything
> fancy? What am I missing?
I asked Eugene to look at this from a different angle, because we are (in
essence) deprecating the concept of EPID as it existed in DTrace, and that
needs to be reflected more in the code.
Reporting enabled probe ID is not really useful anymore anyway, and probably
should be renamed but that needs a bit more attention on how to rename it so
that it actuallt makes sense. But there is no actual need here to make it
such a high value I think because it is meant to assist in identifying the
"function" or "clause" that triggered the failure and for that the raw EPID
(in its new meaning) is actually sufficient.
So, stay tuned for changes to this patch and others...
More information about the DTrace-devel
mailing list