[DTrace-devel] [PATCH v2 04/04] rawtp: retrieve probe arguments from CTF data

[Kris Van Hees]

On Thu, Nov 30, 2023 at 08:26:33PM +0000, Nick Alcock wrote:
> On 30 Nov 2023, Kris Van Hees via DTrace-devel verbalised:
> 
> > We were determining the number of arguments using a trial-and-error
> > approach with minimal BPF programs.  If there is CTF type data for
> > the raw tracepoints (in the form of a function prototype), we use
> > that because it also gives us the types of the arguments.
> 
> Nice!
> 
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> 
> Reviewed-by: Nick Alcock <nick.alcock at oracle.com>
> 
> with one memory leak fixed (see below).
> 
> > -static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> > -		      int *argcp, dt_argdesc_t **argvp)
> > +/*
> > + * If there is no btf_trace_* prototype available in CTF, we can still probe
> > + * the number of available argument for a raw tracepoint by means of a trial
> > + * and error loop to see what the highest argument index is that the BPF
> > + * verifier allows us to load from.
> 
> Ew! :) but it's what we did before and it's a useful fallback in the
> absence of enough type info (OL7, right? dwarf2ctf doesn't emit function
> prototypes at all...)
> 
> > +static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> > +		      int *argcp, dt_argdesc_t **argvp)
> > +{
> > +#ifdef HAVE_LIBCTF
> 
> ... yeah, OL7 :) it amazes me that Solaris libctf had CTF_K_FUNCTION but
> no way to get any actual *info* on CTF_K_FUNCTIONs...
> 
> > +	int			rc, i;
> > +	char			*str;
> > +	ctf_dict_t		*ctfp;
> > +	ctf_id_t		type;
> > +	int			argc = 0;
> > +	dt_argdesc_t		*argv = NULL;
> > +	ctf_funcinfo_t		fi;
> > +	dtrace_typeinfo_t	sym;
> > +	ctf_id_t		*argt;
> > +
> > +	if (asprintf(&str, "btf_trace_%s", prp->desc->prb) == -1)
> > +		longjmp(yypcb->pcb_jmpbuf, EDT_NOMEM);
> > +	rc = dtrace_lookup_by_type(dtp, DTRACE_OBJ_EVERY, str, &sym);
> > +	free(str);
> > +	if (rc ||
> > +	    ctf_type_kind(sym.dtt_ctfp, sym.dtt_type) != CTF_K_TYPEDEF)
> > +		goto use_alt;
> > +	ctfp = sym.dtt_ctfp;
> > +	type = ctf_type_reference(ctfp, sym.dtt_type);
> > +	if (ctf_type_kind(ctfp, type) != CTF_K_POINTER)
> > +		goto use_alt;
> > +	type = ctf_type_reference(ctfp, type);
> > +	if (ctf_type_kind(ctfp, type) != CTF_K_FUNCTION)
> > +		goto use_alt;
> 
> ... I guess that handles -1 just as well, and no matter what the error
> (or if it's just not a pointer) you want to switch to the other
> resolution method :)
> 
> > +	if (ctf_func_type_info(ctfp, type, &fi) == -1)
> > +		goto use_alt;
> > +
> > +	/*
> > +	 * Raw tracepoints have an extra first argument for the context, so we
> > +	 * need to skip that.  (We also handle the case where fi.ctc_argc is 0
> > +	 * even though that is not supposed to happen.)
> > +	 */
> 
> It could perfectly well happen if there is some *other* function pointer
> called btf_trace_foobar that isn't actually a tracepoint (unlikely,
> sure, but no more unlikely than non-function-pointer-typedef
> btf_trace_*s, I'd say.)
> 
> I agree that there's no point handling the case where it's a function
> pointer and the first arg is something else -- you could in theory
> verify that the first arg is a context, but if this gets common enough
> that we actually hit that case it'll be pretty obvious and we can add
> suitable code then.
> 
> > +	argc = fi.ctc_argc;
> > +	argt = dt_calloc(dtp, argc, sizeof(ctf_id_t));
> > +	ctf_func_type_args(ctfp, type, argc, argt);
> > +
> > +	argc--;
> > +	argv = dt_zalloc(dtp, argc * sizeof(dt_argdesc_t));
> 
> I guess if there's an allocation failure we just crash? dt_[cz]alloc
> return NULL on failure... (I know we don't handle this very well
> anywhere, so I'm happy to ignore this.)

I'll add a test to handle that.  We should.

> > +	for (i = 0; i < argc; i++) {
> > +		char	n[DT_TYPE_NAMELEN];
> 
> #define        DT_TYPE_NAMELEN        128
> 
> ugh. I'm adding extirpating *that* to my todo list. :)
> 
> (for now it's all you can do though.)
> 
> > +		ctf_type_name(ctfp, argt[i + 1], n, sizeof(n));
> > +		argv[i].mapping = i;
> > +		argv[i].native = strdup(n);
> 
> Hm. So this is assigned to the dt_probe_t's argv element, which is freed
> in dt_probe_destroy() -- but unless it's done in an earlier patch in
> this series, nothing frees dt_probe_t.argv[i].native, only the argv
> array itself...
> 
> ... with that caveat, this looks good.

That is a pre-existing issue with argument type data for probes.  Fixing that
is not in the scope of this patch.  I'll work on a more generic solution for
that because it is (1) a long-standing issue in DTrace (pre-dating the Linux
version), and (2) an sisue at the level of probes in general - not a particular
provider.