Re: [PATCH 19/38] Retain probe descriptions

[Kris Van Hees <kris.van.hees@oracle.com>]

I think this needs an actual description of what you are trying to do, because
retaining pdescs in and of itself does not seem meaningful.  They do not
contain a reference to the clause they were associated with when the code was
parsed and compiled) and it seems to me that you are doing a sort-of reverse
association, and are actually (indirectly) retaining clauses by keeping a
link of pdescs, one per clause.  And you identify the clause by its numeric
id as obtained from its identifier name (which is dynamically assigned during
compilation).  That seems fragile and rather non-intuitive.

If memory serves me right, the intent was to retain enablings.  Of course, in
the current implementation of DTrace, enablings are nothing more than probes
because the probes have BPF programs associated with them (unless they are
overlying probes, in which case the actual code that gets loaded is part of the
BPF program of the underlying probe (which will be in the enablings list).

When we are retaining enablings, it seems to me that it would make more sense
to keep a list of objects { pdesc, clause } where clause would be the
identifier name of the compiled clause (similar to a function name).  Using a
pointer to the actual clause instead is (I think) not going to work becauase
AFAIK the clause does not store a pointer to its own identiifier.

In the end, this will largely do the same thing as your code (here and when it
gets used later), but it removes the implied dependency on how identifier names
for clauses are generated.  That could change in the future, and doing so would
completely break this imlementation.

On Thu, Jun 27, 2024 at 01:38:10AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> To support matching probes after dtrace has started, we retain
> probe descriptions.
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> ---
>  libdtrace/dt_impl.h    |  2 ++
>  libdtrace/dt_program.c | 37 ++++++++++++++++++++++++++++++++++++-
>  2 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> index 2132eda2..445cd602 100644
> --- a/libdtrace/dt_impl.h
> +++ b/libdtrace/dt_impl.h
> @@ -300,6 +300,8 @@ 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_probedesc_t **dt_retained; /* array of retained pdescs */
> +	int dt_maxretained;	/* number of retained pdescs allocated */
>  	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_program.c b/libdtrace/dt_program.c
> index a4b052fc..7106c249 100644
> --- a/libdtrace/dt_program.c
> +++ b/libdtrace/dt_program.c
> @@ -163,7 +163,42 @@ dt_prog_stmt(dtrace_hdl_t *dtp, dtrace_prog_t *pgp, dtrace_stmtdesc_t *sdp,
>  {
>  	pi_state_t		st;
>  	dtrace_probedesc_t	*pdp = &sdp->dtsd_ecbdesc->dted_probe;
> -	int			rc;
> +	char			*sclause = sdp->dtsd_clause->di_name;
> +	int			rc, nclause;
> +
> +	/* Get the clause number from the clause name "dt_clause_"$n. */
> +	assert(strncmp(sclause, "dt_clause_", strlen("dt_clause_")) == 0);    // FIXME can probably drop this check (make sure the name starts "dt_clause_")
> +	nclause = atoi(sclause + strlen("dt_clause_"));
> +	assert(nclause < 65536);	// FIXME are we okay here with some arbitrary limit?
> +
> +	/* Grow the list of retained pdescs, if necessary. */
> +	if (nclause >= dtp->dt_maxretained) {
> +		int			newm;
> +		dtrace_probedesc_t	**newr;
> +
> +		/* Figure how big to make the array. */
> +		if (dtp->dt_maxretained > 0)
> +			newm = 2 * dtp->dt_maxretained;
> +		else
> +			newm = 8;
> +		while (newm <= nclause)
> +			newm *= 2;
> +
> +		/* Allocate the bigger array. */
> +		newr = dt_zalloc(dtp, newm * sizeof(dtrace_probedesc_t *));
> +		if (newr == NULL)
> +			return 0;
> +
> +		/* Copy data and reset the parameters. */
> +		memcpy(newr, dtp->dt_retained, dtp->dt_maxretained * sizeof(dtrace_probedesc_t *));
> +		dt_free(dtp, dtp->dt_retained);
> +		dtp->dt_retained = newr;
> +		dtp->dt_maxretained = newm;
> +	}
> +
> +	/* Add probe description to retained pdescs. */
> +	assert(dtp->dt_retained[nclause] == NULL);  // FIXME can probably drop this
> +	dtp->dt_retained[nclause] = pdp;        // FIXME I think just pointing to pdp is okay.  No need to cache my own copy.
>  
>  	st.cnt = cnt;
>  	st.idp = sdp->dtsd_clause;
> -- 
> 2.18.4
>