[PATCH 1/3] Clean up unused argsz

[PATCH 1/3] Clean up unused argsz

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 libdtrace/dt_provider_tp.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/libdtrace/dt_provider_tp.c b/libdtrace/dt_provider_tp.c
index 4531a88a8..33821f2c7 100644
--- a/libdtrace/dt_provider_tp.c
+++ b/libdtrace/dt_provider_tp.c
@@ -141,7 +141,6 @@ dt_tp_event_info(dtrace_hdl_t *dtp, FILE *f, int skip, tp_probe_t *tpp,
 	char		*buf = NULL;
 	size_t		bufsz;
 	int		argc;
-	size_t		argsz = 0;
 	dt_argdesc_t	*argv = NULL;
 
 	tpp->id = 0;
@@ -153,8 +152,7 @@ dt_tp_event_info(dtrace_hdl_t *dtp, FILE *f, int skip, tp_probe_t *tpp,
 
 	/*
 	 * Pass 1:
-	 * Determine the event id and the number of arguments (along with the
-	 * total size of all type strings together).
+	 * Determine the event id and the number of arguments.
 	 */
 	argc = -skip;
 	while (getline(&buf, &bufsz, f) >= 0) {
@@ -170,12 +168,6 @@ dt_tp_event_info(dtrace_hdl_t *dtp, FILE *f, int skip, tp_probe_t *tpp,
 		/* We found a field: description - see if we should skip it. */
 		if (argc++ < 0)
 			continue;
-
-		/*
-		 * We over-estimate the space needed (pass 2 will strip off the
-		 * identifier name).
-		 */
-		argsz += strlen(p) + 1;
 	}
 	free(buf);
 	buf = NULL;
-- 
2.47.3

[PATCH 2/3] Defer stripping out "__data_loc" until we know that is useful

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 libdtrace/dt_provider_tp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libdtrace/dt_provider_tp.c b/libdtrace/dt_provider_tp.c
index 33821f2c7..a6f978e3f 100644
--- a/libdtrace/dt_provider_tp.c
+++ b/libdtrace/dt_provider_tp.c
@@ -208,12 +208,13 @@ dt_tp_event_info(dtrace_hdl_t *dtp, FILE *f, int skip, tp_probe_t *tpp,
 		p = buf;
 		if (sscanf(buf, " field:%[^;]", p) <= 0)
 			continue;
-		sscanf(p, "__data_loc %[^;]", p);
 
 		/* We found a field: description - see if we should skip it. */
 		if (argc < 0)
 			goto skip;
 
+		sscanf(p, "__data_loc %[^;]", p);
+
 		/*
 		 * If the last character is not ']', the last token must be the
 		 * identifier name.  Get rid of it.
-- 
2.47.3

[PATCH v2 3/3] Add more robust mechanism to skip tracepoint common fields

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

In dt_tp_event_info(), we parse tracepoint format files -- e.g.,
/sys/kernel/debug/tracing/events/*/*/format.  Specifically, we
are interested in the argument descriptions found in the file,
but we are interested in skipping over "common fields."

The mechanism we used for this purpose was, in dt_provider_tp.c,
to hardwire the number of fields to skip to SKIP_FIELDS_COUNT=4,
assuming the common fields would always be type, flags, pid, and
preempt_count.

This is hardly a robust mechanism.  For example, consider
    https://kernel.googlesource.com/pub/scm/linux/kernel/git/rt/
        linux-rt-devel/+/refs/tags/v5.9.1-rt20-patches/patches/
        preempt-lazy-support.patch
which introduces a preempt_lazy_count common field (on top of
others).  Recent dtrace testing on RHCK 5.14 indicates widespread
test failures due to this problem.

Implement a more robust mechanism.

Specifically, instead of skipping a hardwired (SKIP_FIELDS_COUNT=4)
number of common fields, look for "common_" names.  E.g., in
kernel/trace/trace_events.c in trace_define_common_fields(), we
see the macro __common_field() is used to define common fields,
and the names are prepended with "common_".

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 libdtrace/dt_provider_tp.c | 83 ++++++++++++++++++++++++++++----------
 1 file changed, 61 insertions(+), 22 deletions(-)

diff --git a/libdtrace/dt_provider_tp.c b/libdtrace/dt_provider_tp.c
index a6f978e3f..13f4528fc 100644
--- a/libdtrace/dt_provider_tp.c
+++ b/libdtrace/dt_provider_tp.c
@@ -17,13 +17,6 @@
 #include "dt_probe.h"
 #include "dt_impl.h"
 
-/*
- * All tracing events (tracepoints) include a number of fields that we need to
- * skip in the tracepoint format description.  These fields are: common_type,
- * common_flags, common_preempt_coint, and common_pid.
- */
-#define SKIP_FIELDS_COUNT	4
-
 /*
  * Tracepoint-specific probe data.  This is allocated for every tracepoint
  * based probe.  Since 0 is not a valid tracepoint event id, and given that BTF
@@ -129,10 +122,10 @@ dt_tp_attach_raw(dtrace_hdl_t *dtp, tp_probe_t *tpp, const char *name,
  * 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 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.
+ * All events include a number of common fields that we are not interested
+ * in and that need to be skipped.  Callers of this function can specify an
+ * additional number of fields to skip (using the 'skip' parameter) before
+ * we get to the actual arguments.
  */
 int
 dt_tp_event_info(dtrace_hdl_t *dtp, FILE *f, int skip, tp_probe_t *tpp,
@@ -140,19 +133,16 @@ dt_tp_event_info(dtrace_hdl_t *dtp, FILE *f, int skip, tp_probe_t *tpp,
 {
 	char		*buf = NULL;
 	size_t		bufsz;
-	int		argc;
+	int		argc, common = 1;
 	dt_argdesc_t	*argv = NULL;
 
 	tpp->id = 0;
 
-	/*
-	 * Let skip be the total number of fields to skip.
-	 */
-	skip += SKIP_FIELDS_COUNT;
-
 	/*
 	 * Pass 1:
 	 * Determine the event id and the number of arguments.
+	 * Skip over how ever many arguments the caller asks us to skip.
+	 * We will skip initial "common fields" as well.
 	 */
 	argc = -skip;
 	while (getline(&buf, &bufsz, f) >= 0) {
@@ -160,14 +150,63 @@ dt_tp_event_info(dtrace_hdl_t *dtp, FILE *f, int skip, tp_probe_t *tpp,
 
 		if (sscanf(buf, "ID: %u\n", &tpp->id) == 1)
 			continue;
-
 		if (sscanf(buf, " field:%[^;]", p) <= 0)
 			continue;
-		sscanf(p, "__data_loc %[^;]", p);
 
-		/* We found a field: description - see if we should skip it. */
-		if (argc++ < 0)
-			continue;
+		/*
+		 * If we have only seen common fields to date, keep
+		 * looking for a non-common field.
+		 */
+		if (common == 1) {
+			char	*s = p + strlen(p) - 1;
+
+			/*
+			 * Strip off any [] array size specifications at the end.
+			 */
+			while (*s == ']') {
+				/* From ']' hunt back to '['.  They are not nested. */
+				while (s > p && *(--s) != '[') ;
+
+				/* Then remove any spaces. */
+				while (s > p && *(--s) == ' ') ;
+			}
+			*(++s) = '\0';
+
+			/*
+			 * Go to the beginning of the identifier.
+			 */
+			p = strrchr(p, ' ');
+			if (p == NULL)
+				return -EINVAL;
+			p++;
+
+			/*
+			 * Check if it is a common field.
+			 *
+			 * In kernel source file kernel/trace/trace_events.c
+			 * in trace_define_common_fields(), the macro
+			 * __common_field() is used to define common fields,
+			 * prepending names with "common_".
+			 */
+			if (strncmp(p, "common_", 7) == 0) {
+				/*
+				 * For Pass 2, we will not bother checking
+				 * for "common" fields;  we will just pretend
+				 * the caller asked us to skip more arguments.
+				 */
+				skip++;
+
+				continue;
+			}
+
+			/*
+			 * No more need to check for common fields.
+			 */
+			common = 0;
+		}
+
+		/* We found a non-common "field:" description. */
+		argc++;
 	}
 	free(buf);
 	buf = NULL;
-- 
2.47.3

Re: [DTrace-devel] [PATCH 1/3] Clean up unused argsz

[Nick Alcock]

On 13 Sep 2025, eugene loh stated:

> From: Eugene Loh <eugene.loh@oracle.com>
>
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

Reviewed-by: Nick Alcock <nick.alcock@oracle.com>

Re: [PATCH 2/3] Defer stripping out "__data_loc" until we know that is useful

[Nick Alcock]

On 13 Sep 2025, eugene loh uttered the following:

> From: Eugene Loh <eugene.loh@oracle.com>
>
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

Reviewed-by: Nick Alcock <nick.alcock@oracle.com>

with the tiny caveat below.

> ---
>  libdtrace/dt_provider_tp.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/libdtrace/dt_provider_tp.c b/libdtrace/dt_provider_tp.c
> index 33821f2c7..a6f978e3f 100644
> --- a/libdtrace/dt_provider_tp.c
> +++ b/libdtrace/dt_provider_tp.c
> @@ -208,12 +208,13 @@ dt_tp_event_info(dtrace_hdl_t *dtp, FILE *f, int skip, tp_probe_t *tpp,
>  		p = buf;
>  		if (sscanf(buf, " field:%[^;]", p) <= 0)
>  			continue;
> -		sscanf(p, "__data_loc %[^;]", p);
>  
>  		/* We found a field: description - see if we should skip it. */
>  		if (argc < 0)
>  			goto skip;
>  
> +		sscanf(p, "__data_loc %[^;]", p);
> +

... why not defer the sscanf above it too? The manipulation of argc at
this point looks like this:

        argc = -skip;

(in getline loop)

                /* We found a field: description - see if we should skip it. */
                if (argc < 0)
                        goto skip;
(at loop end)
	skip:
                argc++;
        }


So if argc is < 0, it's going to *stay* like that until a given number
of getlines have happened. Doing more work than just skipping and
getlining is pointless. (This affects the next patch as much as this
one. More generally, it might be clearer to move skipping to a separate
getline loop entirely, above this one that also does all the other work.)

-- 
NULL && (void)

Re: [DTrace-devel] [PATCH v2 3/3] Add more robust mechanism to skip tracepoint common fields

[Nick Alcock]

On 13 Sep 2025, eugene loh stated:
> Implement a more robust mechanism.
>
> Specifically, instead of skipping a hardwired (SKIP_FIELDS_COUNT=4)
> number of common fields, look for "common_" names.  E.g., in
> kernel/trace/trace_events.c in trace_define_common_fields(), we
> see the macro __common_field() is used to define common fields,
> and the names are prepended with "common_".

(If this turns out not to work, maybe we can hunt for the blank line
that always seems to follow common_* fields?)

>  	while (getline(&buf, &bufsz, f) >= 0) {

This is much clearer than it was before!

Reviewed-by: Nick Alcock <nick.alcock@oracle.com>

> @@ -160,14 +150,63 @@ dt_tp_event_info(dtrace_hdl_t *dtp, FILE *f, int skip, tp_probe_t *tpp,
>  
>  		if (sscanf(buf, "ID: %u\n", &tpp->id) == 1)
>  			continue;
> -
>  		if (sscanf(buf, " field:%[^;]", p) <= 0)
>  			continue;
> -		sscanf(p, "__data_loc %[^;]", p);
>  
> -		/* We found a field: description - see if we should skip it. */
> -		if (argc++ < 0)
> -			continue;
> +		/*
> +		 * If we have only seen common fields to date, keep
> +		 * looking for a non-common field.
> +		 */
> +		if (common == 1) {

... ok, I take back the thing I said in the previous mail about moving
things to a separate loop: the ID/field scan is necessary anyway, given
the first two lines:

ID: 94
format:
	field:unsigned short common_type;	offset:0;	size:2;	signed:0;

Given the overall format of e.g.

name: sys_enter_iopl
ID: 94
format:
	field:unsigned short common_type;	offset:0;	size:2;	signed:0;
	field:unsigned char common_flags;	offset:2;	size:1;	signed:0;
	field:unsigned char common_preempt_count;	offset:3;	size:1;	signed:0;
	field:int common_pid;	offset:4;	size:4;	signed:1;

	field:int __syscall_nr;	offset:8;	size:4;	signed:1;

an alternative approach might be to exploit the format, and always skip
all lines that do not start with a tab, and/or simply all lines until
the first entirely blank line? Maybe? (Looking at the kernel source, the
common fields always seem to be followed by a blank one.)

But this approach will work for now, though it appears to be pure
coincidence that all common fields start with common_. We definitely
have workable alternative approaches if this stops working :)

Hmm...

+			/*
+			 * Strip off any [] array size specifications at the end.
+			 */
+			while (*s == ']') {
+				/* From ']' hunt back to '['.  They are not nested. */
+				while (s > p && *(--s) != '[') ;
+
+				/* Then remove any spaces. */
+				while (s > p && *(--s) == ' ') ;
+			}
+			*(++s) = '\0';

Why is this necessary? We never look at the end of the identifier in the
common_ search, only the start...

Re: [PATCH 2/3] Defer stripping out "__data_loc" until we know that is useful

[Eugene Loh]

On 9/14/25 09:17, Nick Alcock wrote:

> On 13 Sep 2025, eugene loh uttered the following:
>
>> From: Eugene Loh <eugene.loh@oracle.com>
>>
>> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> Reviewed-by: Nick Alcock <nick.alcock@oracle.com>

Great, thanks.

> with the tiny caveat below.
>
>> ---
>>   libdtrace/dt_provider_tp.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/libdtrace/dt_provider_tp.c b/libdtrace/dt_provider_tp.c
>> index 33821f2c7..a6f978e3f 100644
>> --- a/libdtrace/dt_provider_tp.c
>> +++ b/libdtrace/dt_provider_tp.c
>> @@ -208,12 +208,13 @@ dt_tp_event_info(dtrace_hdl_t *dtp, FILE *f, int skip, tp_probe_t *tpp,
>>   		p = buf;
>>   		if (sscanf(buf, " field:%[^;]", p) <= 0)
>>   			continue;
>> -		sscanf(p, "__data_loc %[^;]", p);
>>   
>>   		/* We found a field: description - see if we should skip it. */
>>   		if (argc < 0)
>>   			goto skip;
>>   
>> +		sscanf(p, "__data_loc %[^;]", p);
>> +
> ... why not defer the sscanf above it too?

Because it's watching for "field:" lines.  We're not counting lines;  
we're counting lines with "field:" in them.

> The manipulation of argc at
> this point looks like this:
>
>          argc = -skip;
>
> (in getline loop)
>
>                  /* We found a field: description - see if we should skip it. */
>                  if (argc < 0)
>                          goto skip;
> (at loop end)
> 	skip:
>                  argc++;
>          }
>
>
> So if argc is < 0, it's going to *stay* like that until a given number
> of getlines have happened. Doing more work than just skipping and
> getlining is pointless.

Again, we're counting lines that have "field:" in them.

Now, I suppose we could count lines, period, whether they have "field:" 
in them or not (e.g., the ID: line), but I'm okay with forgoing that 
change until some future patch.  I get that this "__data_loc" change is 
no more relevant than what you propose.  But, if folks are okay with it, 
I wouldn't mind just pushing ahead with the patch as it is.

But, yeah, you make a reasonable suggestion.  Thanks.

> (This affects the next patch as much as this
> one. More generally, it might be clearer to move skipping to a separate
> getline loop entirely, above this one that also does all the other work.)

Re: [DTrace-devel] [PATCH v2 3/3] Add more robust mechanism to skip tracepoint common fields

[Eugene Loh]

On 9/14/25 09:26, Nick Alcock wrote:

> On 13 Sep 2025, eugene loh stated:
>> Implement a more robust mechanism.
>>
>> Specifically, instead of skipping a hardwired (SKIP_FIELDS_COUNT=4)
>> number of common fields, look for "common_" names.  E.g., in
>> kernel/trace/trace_events.c in trace_define_common_fields(), we
>> see the macro __common_field() is used to define common fields,
>> and the names are prepended with "common_".
> (If this turns out not to work, maybe we can hunt for the blank line
> that always seems to follow common_* fields?)
>
>>   	while (getline(&buf, &bufsz, f) >= 0) {
> This is much clearer than it was before!
>
> Reviewed-by: Nick Alcock <nick.alcock@oracle.com>
>
>> @@ -160,14 +150,63 @@ dt_tp_event_info(dtrace_hdl_t *dtp, FILE *f, int skip, tp_probe_t *tpp,
>>   
>>   		if (sscanf(buf, "ID: %u\n", &tpp->id) == 1)
>>   			continue;
>> -
>>   		if (sscanf(buf, " field:%[^;]", p) <= 0)
>>   			continue;
>> -		sscanf(p, "__data_loc %[^;]", p);
>>   
>> -		/* We found a field: description - see if we should skip it. */
>> -		if (argc++ < 0)
>> -			continue;
>> +		/*
>> +		 * If we have only seen common fields to date, keep
>> +		 * looking for a non-common field.
>> +		 */
>> +		if (common == 1) {
> ... ok, I take back the thing I said in the previous mail about moving
> things to a separate loop: the ID/field scan is necessary anyway, given
> the first two lines:
>
> ID: 94
> format:
> 	field:unsigned short common_type;	offset:0;	size:2;	signed:0;
>
> Given the overall format of e.g.
>
> name: sys_enter_iopl
> ID: 94
> format:
> 	field:unsigned short common_type;	offset:0;	size:2;	signed:0;
> 	field:unsigned char common_flags;	offset:2;	size:1;	signed:0;
> 	field:unsigned char common_preempt_count;	offset:3;	size:1;	signed:0;
> 	field:int common_pid;	offset:4;	size:4;	signed:1;
>
> 	field:int __syscall_nr;	offset:8;	size:4;	signed:1;
>
> an alternative approach might be to exploit the format, and always skip
> all lines that do not start with a tab, and/or simply all lines until
> the first entirely blank line? Maybe? (Looking at the kernel source, the
> common fields always seem to be followed by a blank one.)
>
> But this approach will work for now, though it appears to be pure
> coincidence that all common fields start with common_.

Coincidence?  As the patch notes, commons fields are inserted via a 
macro that prepends "common_".  Not a coincidence!

> We definitely
> have workable alternative approaches if this stops working :)
>
> Hmm...
>
> +			/*
> +			 * Strip off any [] array size specifications at the end.
> +			 */
> +			while (*s == ']') {
> +				/* From ']' hunt back to '['.  They are not nested. */
> +				while (s > p && *(--s) != '[') ;
> +
> +				/* Then remove any spaces. */
> +				while (s > p && *(--s) == ' ') ;
> +			}
> +			*(++s) = '\0';
>
> Why is this necessary? We never look at the end of the identifier in the
> common_ search, only the start...

I used to look simply for "common_".  Kris was concerned that a type 
might have this string, messing things up.  So, we want to make sure, 
specifically, that it's the identifier that starts with "common_".  So, 
how do we find the start of the identifier?  It's the last token... 
*after* any trailing [] are stripped off.