[DTrace-devel] [PATCH v5 4/6] usdt: typed args and arg mapping

[Nick Alcock]

[Forgot to send this last night, sorry. Probably not much use now.]

On 5 Nov 2024, Kris Van Hees verbalised:

> On Tue, Nov 05, 2024 at 12:06:06AM +0000, Nick Alcock wrote:
>> This change propagates the arg type and mapping info we just got into the
>> pid_probespec_t onward into the underlying dt_uprobe_t, shuffles the native
>> types before handing off a 1:1 mapping to the dt_probe_t, and calls
>> dt_cg_tramp_map_args() to apply the requested mappings.  With this, USDT
>> probes should now have visible types which obey the :-notation translators
>> specified in their .d fils (this is now tested).
>
> Not sure why I didn't catch this earlier but this is confusing (or possibly
> wrong?)...  Perhaps the problem is that you are trying to put too much into a
> single summary sentence.

There's no chance that I'd do something like that! (ha ha yes there is).

> There is no shuffling of native types.

Indeed not. It shuffles the *arguments*, but the types are not shuffled.
Nice catch.

> What happens (based on my reading of the code) is that:
>
> - arg data in pid_probespec_t is propagated into the underlying dt_uprobe_t
>   where it is stored as a dt_argdesc_t array
> - the trampoline shuffles the argument values according to the mapping data
>   in the underlying probe
> - calls to probe_info() retrieve arg data as a dt_argdesc_t array with 1:1
>   mapping (because the trace program can only access arguments after they
>   were already mapped)
>
> Also.... "fils" -> "file"

Rephrased thusly:

> This change propagates the arg type and mapping info we just got into the
> pid_probespec_t onward into the underlying dt_uprobe_t and shuffles the
> argument values by calling dt_cg_tramp_map_args().  With this, USDT probes
> should now have visible types which obey the :-notation translators
> specified in their .d file (this is now tested).

which is much simpler.

>> The probe info has been moved from the underlying probe to the overlying one
>
> "probe info" -> "probe info hook"

probe_info hook, and yes.

>> as part of this: underlying probes don't have any user-visible args to
>> get info for, and their probe_info functions are never called.  We do all
>> the arg reshuffling in a function ultimately called from USDT probe
>
> There is no arg reshuffling or shuffling... all that happens is that the
> dt_argdesc_t array is populated.

It used to be shuffled and I forgot it wasn't when I rephrased bits.

>> discovery via provide_underlying, so it predates trampoline setup let alone
>> probe_info on the overlying probe and all the args info is present before
>> it is needed.  (We intentionally do not shuffle the args in
>> dt_cg_tramp_map_args specifically so that we are not sensitive to the
>
> Actually, dt_cg_tramp_map_args() specifically *does* the arg shuffling (the
> mapping of args).  That is its sole purpose.

Yep. I was thinking in terms of mapping modifications, but of course
that is done by probe_info... sigh.

>> relative call order of trampoline() versus probe_info(), but there's no
>> escaping the requirement for USDT probe discovery to predate all of this:
>> but this was already a requirement in any case.)
>
> There is no call order relation in any way between trampoline and probe_info.
> In fact, except for some special providers that do not have a low-level probe
> implementation component, trampolines are not generated until all olauses have
> been copiled.

Yeah, I've just dropped this whole parenthetical, it's a load of
rubbish. (And we no longer care what relative order they're called in
anyway: indeed we can even cope with create_underlying being called
later nowadays, as long as create_underlying for a given USDT probe is
called before probe_info for that probe, but we necessarily always
depended on that *anyway* so there's no point mentioning that either.)

>> We make one assumption for simplicity, noted by a TODO in the source.
>> We assume that overlying probes all have the same native types and mappings,
>> so we don't need to look at more than one underlying probe to figure out what
>> they are.  DTrace always generates such a thing, but DOF specifying multiple
>> overlapping probes seems perfectly possible for other programs to generate.
>> This should probably be validated in dof_parser.c in future.
>
> Since you added validation that rejects multiple USDT probes using the same

I meant to take this out :( :( it's gone now. I did the squashing-back
late last night, clearly too late...

> If you feel the need to mention that other DOF generators could potentially
> create DOF that associates multiple overying probes with the same underlying
> probe. that's fine, but since your code will not accept that, I don't think
> there is a need to mention it.

It's gone gone gone. Anyone who does that gets a failure from DTrace and
will immediately realise they've done something wrong.

>> diff --git a/libdtrace/dt_pid.c b/libdtrace/dt_pid.c
>> index 71ac1754d343..a0b3d892fd23 100644
>> --- a/libdtrace/dt_pid.c
>> +++ b/libdtrace/dt_pid.c
>> @@ -950,6 +950,7 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, dt_proc_t *dpr,
>>  
>>  			p += args->size;
>>  			seen_size += args->size;
>> +			args = (dof_parsed_t *) p;
>
> This change belongs in patch 1/6...  without it that 1/6 patch is validating
> the wrong data.

Moved.

>> +/*
>> + * Populate args for an underlying probe.  This really relates to an overlying
>> + * USDT probe (all overlying probes associated with a given underlying probe
>> + * must have the same args), but the overlying probe doesn't exist at the point
>> + * this is populated, so we must put it in the underlying probe instead and pull
>> + * it out when the overlying probe info is called for.
>
> "all overlying probes associated with a given underlying probe" makes little
> sense when it is already established (ad code guarantees) that there will
> only be a single overlying USDT probe for any underlying probe - and any other
> overlying probe is therefore a pid probe and does not have defined arguments.

True!

> Pehaps better would be:
>
> /*
>  * Populate args for an underlying probe for use by the overlying USDT probe.
>  * The overlying probe does not exist yet at this point, so the arg data is
>  * stored in the underlying probe instead and will be accessed when probe_info
>  * is called in the overlying probe.

Much better.

>> + *
>> + * Move it into dt_argdesc_t's for use later on. The char *'s in that structure
>> + * are pointers into the argvbuf array, which is a straight concatenated copy of
>> + * the nargc/xargc in the pid_probespec_t.
>
> nargc/xargc -> nargv/xargv

gah! I don't *believe* how many times I made that mistake. Almost every
reference was backwards.

>> +	/*
>> +	 * Construct an array to allow accessing native args by index.
>> +	 */
>> +
>
> Remove blank line.

Ew, seriously?

... it looks like that is in fact the formatting. Adjusted.

>> +	/*
>> +	 * Only one USDT probe can correspond to each underlying probe.
>> +	 */
>> +	if (psp->pps_type == DTPPT_USDT && upp->flags == PP_IS_USDT) {
>> +		dt_dprintf("Found overlapping USDT probe at %lx/%lx/%lx/%s\n",
>> +			   upp->dev, upp->inum, upp->off, upp->fn);
>> +		goto fail;
>> +	}
>> +
>> +	if (populate_args(dtp, psp, upp) < 0)
>> +		goto fail;
>
> Why bother calling this for a non-USDT probe?

populate_args() doesn't care if a probe is a USDT probe. It populates
args if the probe has them. It so happens that only USDT probes do, but
populate_args() is not sensitive to that and will work regardless.

Almost the first line of populate_args() is

	/*
	 * If we have a nonzero number of args, we always have at least one narg
	 * and at least one xarg.  Double-check to be sure.  (These are not
	 * populated, and thus left 0/NULL, for non-USDT probes.)
	 */
	if (upp->argc == 0 || psp->pps_xargv == NULL || psp->pps_nargv == NULL
		|| psp->pps_xargvlen == 0 || psp->pps_nargvlen == 0)
		return 0;

which will always return unless the probe has args, no matter what type
it is. populate_args() doesn't care.

> I would use:
> 	if ( psp->pps_type == DTPPT_USD && populate_args(dtp, psp, upp) < 0)

I don't see any way in which that is clearer, but I could do that if you
actually wanted. Should future probe types turn up that have args that
need population, that would rust and need replacing, so that's a (tiny)
disadvantage and as far as I can tell no advantages.

>> +
>>  	switch (psp->pps_type) {
>>  	case DTPPT_RETURN:
>>  	    upp->flags |= PP_IS_RETURN;
>> @@ -604,15 +703,19 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
>>  	case DTPPT_IS_ENABLED:
>>  	    upp->flags |= PP_IS_ENABLED;
>>  	    break;
>> +	case DTPPT_USDT:
>> +	    upp->flags |= PP_IS_USDT;
>> +	    break;
>>  	default: ;
>>  	    /*
>>  	     * No flags needed for other types.
>>  	     */
>>  	}
>> -
>
> I would leave the blank line.

Accidental deletion :(

>> - * the firing of all dependent pid* probes and their clauses.
>> + * the firing of all dependent pid* and USDT probes and their clauses, or (in
>> + * the case of is-enabled probes), do the necessary copying (is-enabled probes
>> + * have no associated clauses and their behaviour is hardwired).
>> + *
>> + * Trampolines associated with USDT probes will also arrange for argument
>> + * shuffling if the argmap array calls for it.
>
> I would drop this since you mention the napping in a comment below already.

I was trying to describe what the function does, rather than only one
thing it does which happens to be the first thing that was implemented
(firing the dependent probes and their clauses). This is one of the
things it does.

> Here is is potentially confusing because trampolines are associated with the
> underlying probe, and that probe might have both pid probes and a USDT probe
> associated with it.

So if I add "... for argument shuffling before the USDT clauses are
invoked" in the appropriate place, is that OK?  (I assumed that was obvious.)

>> +	for (i = 0; i < argc; i++) {
>> +		argv[i].native = strdup(upp->args[i].native);
>> +		if (upp->args[i].xlate)
>> +			argv[i].xlate = strdup(upp->args[i].xlate);
>> +		argv[i].mapping = i;
>> +
>
> Perhaps add a comment here that this is code to handle an allocation failure?

... er, the == NULL right after an allocation makes that sort of obvious?

> Or move this to the end of the function with a fail: label, and goto to that
> if allocation fails (since that makes it more obvious that this is code to
> handle an allocation failure).

That's much clearer, though you need to initialize i to zero to avoid
compiler warnings in that case. Adjusted.

>>  provider test_prov {
>>  	probe place(int i, int j) : (int j, int i, int i, int j);
>> +	probe place2(int i, char *j) : (char *j, int i, int i, char *j);
>> +	probe place3(int i, char *j) : (char *j);
>> +	probe place4(int i, char *j) : ();
>
> It might be worth adding tests for -lvn for all these cases, as an extra
> check that probe info is being provided correctly, even in the case where we
> do not actually trace.

Good point! Added some.

-- 
NULL && (void)