[DTrace-devel] [PATCH 04/14] Track uprobe provider descriptions

[Eugene Loh]

On 6/4/24 17:10, Kris Van Hees wrote:

> On Tue, Jun 04, 2024 at 02:11:03PM -0400, eugene.loh--- via DTrace-devel wrote:
>> From: Eugene Loh <eugene.loh at oracle.com>
>>
>> A uprobe will have to decide which of its clauses to run for a
>> given pid.  For now, keep the provider description for each clause.
>> There are many shortcomings to the way this is done in this patch,
>> but it is quick and dirty and helps us bootstrap real support.
>> Clean this up later, probably turning it into a growable array.
> This is the wrong way to look at it, I think.  The underlying concept is that
> DTrace supports specifying probes that do not match anything yet (-Z).  Those
> are called retained enablings and they can exist for more than just uprobes.
> So this needs to be more generic.

Okay.  I looked at legacy-DTrace retained enablings.  They seem to be a 
linked list -- (mainly) of arrays of dtrace_ecbdesc pointers. And 
dtrace_ecbdesc has dtrace_probedesc, dtrace_preddesc, and 
dtrace_actdesc.  In DTrace on eBPF, dtrace_ecbdesc is much simpler -- 
it's really just a dtrace_probedesc.  So it seems to me we could just 
make our retained enablings be an array of probe desc pointers.  That 
would be a small change to this (small) patch.

> We also shouldn't do this non-discriminately for all clauses all the time.
> Obviously, when -Z is not used, there is no point in doing this because there
> cannot be a match-after-start.

FWIW, that's not how legacy DTrace seems to work:  there *can* be 
match-after-start without -Z.  The -Z option only addresses whether 
there is an initial match.  If there is, then -Z is not needed, not even 
for subsequent matches to work.

> Even when -Z is specified, you might be able
> to determine whether the probe specification for a probe matches perfectly
> or whether it needs to be retained for match-after-start.

Maybe.  But what if you have pid1234:a.out:main:go, but pid 1234 has not 
yet started up?  (Yes, that's an extremely weird example.)

Mostly (going back to your "shouldn't non-discriminately" comment), 
trying to decide whether to keep a probe desc seems unnecessary. Just 
keep them all.  They will likely be dwarfed by dtp->dt_probes[] anyhow.

> So, let's put some thought into designing this in the more generic case, so
> that we can avoid going down a rabbit hole that gets tough to recover from.

What do you think of:

     struct dtrace_hdl {
         [...]
         dt_list_t dt_enablings; /* list of (to be) enabled probes */
+       dtrace_probedesc_t **dt_retained;
+       int dt_nretained;
+       int dt_maxretained;
         struct dt_xlator **dt_xlatormap; /* dt_xlator_t's indexed by 
dx_id */
         [...]
     }


> (And whatever we store the retained enablings in probably should be added to
>   dtrace_hdl_t after dt_enablings.)

No problem.

>> diff --git a/libdtrace/dt_cc.c b/libdtrace/dt_cc.c
>> @@ -215,6 +215,7 @@ dt_compile_one_clause(dtrace_hdl_t *dtp, dt_node_t *cnp, dt_node_t *pnp)
>>   	 */
>>   	dt_cg(yypcb, cnp);
>>   	sdp->dtsd_clause = dt_clause_create(dtp, dt_as(yypcb));
>> +	dtp->dt_uprovdescs[dtp->dt_clause_nextid - 1] = strdup(pnp->dn_desc->prv);
>>   
>>   	assert(yypcb->pcb_stmt == sdp);
>>   	if (dtrace_stmt_add(yypcb->pcb_hdl, yypcb->pcb_prog, sdp) != 0)
>> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
>> @@ -310,6 +310,7 @@ struct dtrace_hdl {
>>   	dt_pcb_t *dt_pcb;	/* pointer to current parsing control block */
>>   	ulong_t dt_gen;		/* compiler generation number */
>>   	uint_t dt_clause_nextid; /* next ID to use for programs */
>> +	char *dt_uprovdescs[256]; /* uprobe provider descriptor per clause... FIXME turn this into a growable array */
>>   	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 */