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

[Nick Alcock]

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

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

-- 
NULL && (void)