[DTrace-devel] [PATCH 5/8] sdt: retrieve probe arguments from CTF data (if possible)

[Nick Alcock]

On 4 Apr 2024, Eugene Loh via DTrace-devel stated:

> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
> though I don't mind if Nick also takes a look... particularly at the CTF stuff.  And a few more things...

Ack.

(Not that I ever got the original email, or half the others: thanks,
Outlook.)

> On 4/3/24 11:26, Kris Van Hees via DTrace-devel wrote:
>>   static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
>>   		      int *argcp, dt_argdesc_t **argvp)
>>   {
>> -	FILE		*f;
>> -	char		fn[256];
>> -	int		rc;
>> -	tp_probe_t	*tpp = prp->prv_data;
>> +	tp_probe_t		*tpp = prp->prv_data;
>> +	int			rc, i;
>> +	char			*str;
>> +	ctf_dict_t		*ctfp;
>
> There are a bunch of declarations here that should probably be wrapped in #ifdef HAVE_LIBCTF.  That should help with compiler
> warnings about unused variables but also presumably with unknown types like ctf_dict_t.

Agreed. You could just move the ctfp and it decls inside the HAVE_LIBCTF
block...

>> +#ifdef HAVE_LIBCTF
>> +	if (asprintf(&str, "struct trace_event_raw_%s", prp->desc->prb) == -1)
>> +		return dt_set_errno(dtp, 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_STRUCT)
>> +		goto use_alt;
>>   -	f = fopen(fn, "r");
>> -	if (!f)
>> -		return -ENOENT;
>> +	/*
>> +	 * Tracepoints have an extra member at the beginning and end of the
>> +	 * struct.  We need to skip those.  (We also handle the case where one
>> +	 * or both of those members are missing even though that is not
>> +	 * supposed to happen.)
>> +	 */

It might make sense to validate that the extra member is what we expect
it to be, so that if this behaviour silently changes in future we don't
end up skipping useful members.

>> +	ctfp = sym.dtt_ctfp;
>> +	type = sym.dtt_type;
>> +	rc = ctf_member_count(ctfp, type);
>> +	if (rc <= 2)
>> +		goto done;
>> +
>> +	rc--;
>> +	argc = rc - 1;
>> +	argv = dt_zalloc(dtp, argc * sizeof(dt_argdesc_t));
>> +	if (argv == NULL)
>> +		return dt_set_errno(dtp, EDT_NOMEM);
>> +	/* Skip first member. */
>> +	ctf_member_next(ctfp, type, &it, NULL, NULL, 0);
>> +	for (i = 0; i < argc; i++) {
>> +		ctf_id_t	mtyp;
>> +		char		n[DT_TYPE_NAMELEN];
>> +
>> +		if (ctf_member_next(ctfp, type, &it, NULL, &mtyp, 0) == CTF_ERR)
>> +			return dt_set_errno(dtp, EDT_CTF);

You should probably do a ctf_next_destroy(it) in this failure path (when
we exit with ECTF_NEXT_END, that's done for you, but otherwise, it's
not, because it's occasionally possible to fix whatever the problem was
and continue iterating.)

It's safe to call this on a NULL iterator, so you don't need any extra
conditionals. (You might want to check for !ECTF_NEXT_END and report an
error, but these are pretty unlikely errors and a loss of function
prototype handling for one function is sufficeiently non-disastrous that
you're probably right to just ignore it.)

-- 
NULL && (void)