public inbox for dtrace@lists.linux.dev
 help / color / mirror / Atom feed
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?

> [...]

  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