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

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 <[email protected]>
>
>> @@ -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.