From: Sam James <sam@gentoo.org>
To: "eugene.loh--- via DTrace-devel" <dtrace-devel@oss.oracle.com>
Cc: dtrace@lists.linux.dev, eugene.loh@oracle.com
Subject: Re: [DTrace-devel] [PATCH 03/19] Widen the EPID to include the PRID
Date: Thu, 29 Aug 2024 21:28:12 +0100 [thread overview]
Message-ID: <87ttf3f50j.fsf@gentoo.org> (raw)
In-Reply-To: <20240829052558.3525-3-eugene.loh@oracle.com> (eugene loh's message of "Thu, 29 Aug 2024 01:25:42 -0400")
"eugene.loh--- via DTrace-devel" <dtrace-devel@oss.oracle.com> writes:
> From: Eugene Loh <eugene.loh@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@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?
> [...]
next prev parent reply other threads:[~2024-08-29 20:28 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-08-29 5:25 [PATCH 01/19] Change probes from having lists of clauses to lists of stmts eugene.loh
2024-08-29 5:25 ` [PATCH 02/19] Add a hook for a provider-specific "update" function eugene.loh
2024-08-29 5:25 ` [PATCH 03/19] Widen the EPID to include the PRID eugene.loh
2024-08-29 20:28 ` Sam James [this message]
2024-08-29 20:38 ` [DTrace-devel] " Kris Van Hees
2024-08-29 5:25 ` [PATCH 04/19] Eliminate dt_pdesc eugene.loh
2024-09-03 17:47 ` Eugene Loh
2024-08-29 5:25 ` [PATCH 05/19] Add flag to dt_pid_create_probes() eugene.loh
2024-09-18 20:33 ` Kris Van Hees
2024-09-24 20:24 ` Eugene Loh
2024-08-29 5:25 ` [PATCH 06/19] Allow for USDT wildcards eugene.loh
2024-09-17 17:34 ` Eugene Loh
2024-08-29 5:25 ` [PATCH 07/19] Create the BPF usdt_prids map eugene.loh
2024-08-29 5:25 ` [PATCH 08/19] Support multiple overlying probes in the uprobe trampoline eugene.loh
2024-10-24 2:42 ` Kris Van Hees
2024-10-24 13:52 ` [DTrace-devel] " Kris Van Hees
2024-10-24 23:30 ` Eugene Loh
2024-10-25 0:14 ` Kris Van Hees
2024-08-29 5:25 ` [PATCH 09/19] Use usdt_prids map to call clauses conditionally for USDT probes eugene.loh
2024-08-29 5:25 ` [PATCH 10/19] Remove the is-enabled provider eugene.loh
2024-10-24 15:18 ` Kris Van Hees
2024-10-26 1:13 ` Eugene Loh
2024-08-29 5:25 ` [PATCH 11/19] Support USDT wildcard provider descriptions eugene.loh
2024-08-29 5:25 ` [PATCH 12/19] Increase size of BPF probes map eugene.loh
2024-08-29 20:30 ` [DTrace-devel] " Sam James
2024-10-08 22:15 ` Eugene Loh
2024-08-29 5:25 ` [PATCH 13/19] Get rid of relocatable EPID, dt_nextepid, and dt_ddesc[] eugene.loh
2024-09-03 17:49 ` Eugene Loh
2024-08-29 5:25 ` [PATCH 14/19] Ignore clauses in USDT trampoline if we know they are impossible eugene.loh
2024-08-29 5:25 ` [PATCH 15/19] Ignore clauses: some clauses are impossible regardless of uprp eugene.loh
2024-08-29 20:31 ` [DTrace-devel] " Sam James
2024-09-03 19:54 ` Eugene Loh
2024-09-03 20:10 ` Kris Van Hees
2024-08-29 5:25 ` [PATCH 16/19] Ignore clauses: use underlying probe's function information eugene.loh
2024-10-24 16:52 ` Kris Van Hees
2024-08-29 5:25 ` [PATCH 17/19] test: Add a pid-USDT test eugene.loh
2024-08-29 20:32 ` [DTrace-devel] " Sam James
2024-10-04 4:49 ` Eugene Loh
2024-10-04 5:51 ` Sam James
2024-08-29 5:25 ` [PATCH 18/19] test: Add a lazy USDT test eugene.loh
2024-09-28 2:11 ` Eugene Loh
2024-08-29 5:25 ` [PATCH 19/19] test: Add another USDT open/close test eugene.loh
2024-10-24 17:01 ` Kris Van Hees
2024-09-18 14:18 ` [PATCH 01/19] Change probes from having lists of clauses to lists of stmts Kris Van Hees
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87ttf3f50j.fsf@gentoo.org \
--to=sam@gentoo.org \
--cc=dtrace-devel@oss.oracle.com \
--cc=dtrace@lists.linux.dev \
--cc=eugene.loh@oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox