[DTrace-devel] [PATCH 02/05] tp: handle tracepoint argument datatypes with symbolic array size

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
but I have some questions.  (You can decide if any of the questions 
warrants rescinding the R-b!)

The main question is why not use this approach for every [] field, even 
if the arrsize spec is strictly numerical... that is, even if it's [4] 
or something?  Using the same approach in each case could simplify this 
code a little: no alternate code path for !alpha, don't care if it is 
alpha, etc.

Also, is the name "alpha" right?  !isdigit() could also mean ' ' or '+' 
and I see some arrsize specs like "[32 + 2]", which would be handled by 
the new mechanism even though the name "alpha" does not quite make sense.

But basically looks fine.

On 5/5/23 11:30, Kris Van Hees via DTrace-devel wrote:
> Some kernels expose datatypes for tracepoint elements like this:
>
>    field:char oldcomm[TASK_COMM_LEN]; offset:12; size:16; signed:1;
>
> Since the type compiler and CTF type parser) cannot resolve the value
> of TASK_COMM_LEN, we synthesize the type ourselves based on the base
> type (char in this case) and the argument size (16 in this case).
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
>   libdtrace/dt_provider_tp.c | 41 ++++++++++++++++++++++++++++++--------
>   1 file changed, 33 insertions(+), 8 deletions(-)
>
> diff --git a/libdtrace/dt_provider_tp.c b/libdtrace/dt_provider_tp.c
> index a08550ab..ff4cc56b 100644
> --- a/libdtrace/dt_provider_tp.c
> +++ b/libdtrace/dt_provider_tp.c
> @@ -7,6 +7,7 @@
>    * Provider support code for tracepoint-based probes.
>    */
>   #include <errno.h>
> +#include <ctype.h>
>   #include <stdio.h>
>   #include <sys/ioctl.h>
>   #include <linux/perf_event.h>
> @@ -111,7 +112,7 @@ dt_tp_is_created(const tp_probe_t *tpp)
>    * the identifier isn't as easy because it may be suffixed by one or more
>    * array dimension specifiers (and those are part of the type).
>    *
> - * All events include a number of fields that we are not interested and that
> + * All events include a number of fields that we are not interested in and that
>    * need to be skipped (SKIP_FIELDS_COUNT).  Callers of this function can
>    * specify an additional number of fields to skip (using the 'skip' parameter)
>    * before we get to the actual arguments.
> @@ -187,8 +188,13 @@ dt_tp_event_info(dtrace_hdl_t *dtp, FILE *f, int skip, tp_probe_t *tpp,
>   	rewind(f);
>   	argc = -skip;
>   	while (getline(&buf, &bufsz, f) >= 0) {
> -		char	*p = buf;
> +		char	*p;
>   		size_t	l;
> +		size_t	size = 0;
> +
> +		p = strstr(buf, "size:");
> +		if (p != NULL)
> +			size = strtol(p + 5, NULL, 10);
>   
>   		if (sscanf(buf, " field:%[^;]", p) <= 0)
>   			continue;
> @@ -214,6 +220,7 @@ dt_tp_event_info(dtrace_hdl_t *dtp, FILE *f, int skip, tp_probe_t *tpp,
>   		} else {
>   			char	*s, *q;
>   			int	n;
> +			int	alpha = 0;
>   
>   			/*
>   			 * The identifier is followed by at least one array
> @@ -223,7 +230,10 @@ dt_tp_event_info(dtrace_hdl_t *dtp, FILE *f, int skip, tp_probe_t *tpp,
>   			 */
>   			s = p + l - 1;
>   			for (;;) {
> -				while (*(--s) != '[') ;
> +				while (*(--s) != '[') {
> +					if (!isdigit(*s))
> +						alpha = 1;
> +				}
>   				while (*(--s) == ' ') ;
>   				if (*s != ']')
>   					break;
> @@ -240,11 +250,26 @@ dt_tp_event_info(dtrace_hdl_t *dtp, FILE *f, int skip, tp_probe_t *tpp,
>   			if ((q = strrchr(p, ' ')))
>   				*q = '\0';
>   
> -			l = q - p;
> -			memcpy(strp, p, l);
> -			n = strlen(s);
> -			memcpy(strp + l, s, n);
> -			l += n;
> +			if (alpha) {
> +				ctf_file_t	*ctfp = dtp->dt_shared_ctf;
> +				ctf_id_t	type;
> +				size_t		esize = 1;
> +
> +				type = ctf_lookup_by_name(ctfp, p);
> +				if (type != CTF_ERR)
> +					esize = ctf_type_size(ctfp, type);
> +				if (esize != 0)
> +					size /= esize;
> +
> +				l = snprintf(strp, q - p + strlen(s), "%s[%lu]",
> +					     p, size);
> +			} else {
> +				l = q - p;
> +				memcpy(strp, p, l);
> +				n = strlen(s);
> +				memcpy(strp + l, s, n);
> +				l += n;
> +			}
>   		}
>   
>   		argv[argc].mapping = argc;