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

[Kris Van Hees]

On Fri, May 05, 2023 at 03:11:21PM -0400, Eugene Loh via DTrace-devel wrote:
> 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.

The reason I am not doing that (yet) is that the logic needed is quite a bit
more complex because we'd need to handle quite a few more cases.  And then
there is the problem of multi-dimensional arrays.  I.e. how do you determine
X and Y in 'char [X][Y]' based on only the base size (size of char) and the
total size?

So for now, I am only covering what is necessary.

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

Yes, the variable name 'alpha' is not ideal but I think it covers the load.

> 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;
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel