Re: [PATCH 1/5] tracing: Don't show dynamic string internals in synthetic event description

[Masami Hiramatsu <mhiramat@kernel.org>]

Hi Tom,

On Fri,  9 Oct 2020 10:17:07 -0500
Tom Zanussi <zanussi@kernel.org> wrote:

> For synthetic event dynamic fields, the type contains "__data_loc",
> which is basically an internal part of the type which is only meant to
> be displayed in the format, not in the event description itself, which
> is confusing to users since they can't use __data_loc on the
> command-line to define an event field, which printing it would lead
> them to believe.
> 
> So filter it out from the description, while leaving it in the type.
> 

OK, I confirmed this removes __data_loc from synth_events interface.
However, I also found another issue.

  /sys/kernel/debug/tracing # echo "myevent char str[]; int v" >> synthetic_events  
  /sys/kernel/debug/tracing # cat synthetic_events 
  myevent	char[]; str; int v

It seems that the type "char[]" includes ";" as a type, this results


  /sys/kernel/debug/tracing # cat events/synthetic/myevent/format 
  name: myevent
  ID: 1220
  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:__data_loc char[]; str;	offset:8;	size:8;	signed:1;
	field:int v;	offset:16;	size:4;	signed:1;

  print fmt: "str=%.*s, v=%d", __get_str(str), REC->v


As you can see, the field type has ";" in format file too. This will prevent
parsing event information correctly.
I also try to remove ";" as below, it seems to work correctly.

  /sys/kernel/debug/tracing # echo "myevent char[] str; int v" >> synthetic_events 
  /sys/kernel/debug/tracing # cat events/synthetic/myevent/format 
  name: myevent
  ID: 1221
  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:__data_loc char[] str;	offset:8;	size:8;	signed:1;
	field:int v;	offset:16;	size:4;	signed:1;

  print fmt: "str=%.*s, v=%d", __get_str(str), REC->v


Thank you,

> Reported-by: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Tom Zanussi <zanussi@kernel.org>
> ---
>  kernel/trace/trace_events_synth.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
> index 3b2dcc42b8ee..b19e2f4159ab 100644
> --- a/kernel/trace/trace_events_synth.c
> +++ b/kernel/trace/trace_events_synth.c
> @@ -1867,14 +1867,22 @@ static int __synth_event_show(struct seq_file *m, struct synth_event *event)
>  {
>  	struct synth_field *field;
>  	unsigned int i;
> +	char *type, *t;
>  
>  	seq_printf(m, "%s\t", event->name);
>  
>  	for (i = 0; i < event->n_fields; i++) {
>  		field = event->fields[i];
>  
> +		type = field->type;
> +		t = strstr(type, "__data_loc");
> +		if (t) { /* __data_loc belongs in format but not event desc */
> +			t += sizeof("__data_loc");
> +			type = t;
> +		}
> +
>  		/* parameter values */
> -		seq_printf(m, "%s %s%s", field->type, field->name,
> +		seq_printf(m, "%s %s%s", type, field->name,
>  			   i == event->n_fields - 1 ? "" : "; ");
>  	}
>  
> -- 
> 2.17.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>