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

[Kris Van Hees]

I plan to merge this patch with my r-b with changes outlined below if you are
OK with it.  No need for a new series.

On Wed, Nov 06, 2024 at 11:16:41AM +0000, Nick Alcock wrote:
> >> - * 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.)

No, honestly, because you already made the case that adding a conditional to
only populate args for USDT is not wanted because other types of userspace
probes might have arguments also.  And that means they may also have argument
mapping data, and thus this is again not USDT-specific.  The comment in the
code itself is better.

> >> +	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.

Hm, why would you need to init i to 0?  The compiler should be able to see
that you cannot reach the oom label except after i is already initialized by
the for loop.

Which compiler version flagged a warning on this when i is not initialized.

Also, you put a size_t j; declaration at the oom: label rather than at the
beginning of a code block (beginning of the function in this case).  I'll
change that in my copy before merging.

Also, in your patch you added a space before the label (I removed it because
we put labels at the left margin), and you introduced space indentation where
there should be a tab before the dt_free().

> >>  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)