On Sun, 12 Feb 2023 15:13:03 +0000 Yafang Shao <laoar.shao@xxxxxxxxx> wrote: > After commit 3087c61ed2c4 ("tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN"), > the content of the format file under > /sys/kernel/debug/tracing/events/task/task_newtask was changed from > field:char comm[16]; offset:12; size:16; signed:0; > to > field:char comm[TASK_COMM_LEN]; offset:12; size:16; signed:0; > > John reported that this change breaks older versions of perfetto. > Then Mathieu pointed out that this behavioral change was caused by the > use of __stringify(_len), which happens to work on macros, but not on enum > labels. And he also gave the suggestion on how to fix it: > :One possible solution to make this more robust would be to extend > :struct trace_event_fields with one more field that indicates the length > :of an array as an actual integer, without storing it in its stringified > :form in the type, and do the formatting in f_show where it belongs. > > The result as follows after this change, > $ cat /sys/kernel/debug/tracing/events/task/task_newtask/format > field:char comm[16]; offset:12; size:16; signed:0; > > Fixes: 3087c61ed2c4 ("tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN") > Reported-by: John Stultz <jstultz@xxxxxxxxxx> > Debugged-by: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx> > Suggested-by: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx> > Suggested-by: Steven Rostedt <rostedt@xxxxxxxxxxx> > Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx> > Cc: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx> > Cc: Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> > Cc: Kajetan Puchalski <kajetan.puchalski@xxxxxxx> > Cc: Steven Rostedt <rostedt@xxxxxxxxxxx> > CC: Qais Yousef <qyousef@xxxxxxxxxxx> > Cc: John Stultz <jstultz@xxxxxxxxxx> > Cc: stable@xxxxxxxxxxxxxxx # v5.17+ This looks good to me. Reviewed-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> Thanks! > --- > include/linux/trace_events.h | 1 + > include/trace/stages/stage4_event_fields.h | 3 ++- > kernel/trace/trace.h | 1 + > kernel/trace/trace_events.c | 39 +++++++++++++++++++++++------- > kernel/trace/trace_export.c | 3 ++- > 5 files changed, 36 insertions(+), 11 deletions(-) > > diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h > index 4342e99..0e37322 100644 > --- a/include/linux/trace_events.h > +++ b/include/linux/trace_events.h > @@ -270,6 +270,7 @@ struct trace_event_fields { > const int align; > const int is_signed; > const int filter_type; > + const int len; > }; > int (*define_fields)(struct trace_event_call *); > }; > diff --git a/include/trace/stages/stage4_event_fields.h b/include/trace/stages/stage4_event_fields.h > index affd541..b6f679a 100644 > --- a/include/trace/stages/stage4_event_fields.h > +++ b/include/trace/stages/stage4_event_fields.h > @@ -26,7 +26,8 @@ > #define __array(_type, _item, _len) { \ > .type = #_type"["__stringify(_len)"]", .name = #_item, \ > .size = sizeof(_type[_len]), .align = ALIGN_STRUCTFIELD(_type), \ > - .is_signed = is_signed_type(_type), .filter_type = FILTER_OTHER }, > + .is_signed = is_signed_type(_type), .filter_type = FILTER_OTHER,\ > + .len = _len }, > > #undef __dynamic_array > #define __dynamic_array(_type, _item, _len) { \ > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h > index e46a492..19caf15 100644 > --- a/kernel/trace/trace.h > +++ b/kernel/trace/trace.h > @@ -1282,6 +1282,7 @@ struct ftrace_event_field { > int offset; > int size; > int is_signed; > + int len; > }; > > struct prog_entry; > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c > index 33e0b4f..6a46967 100644 > --- a/kernel/trace/trace_events.c > +++ b/kernel/trace/trace_events.c > @@ -114,7 +114,7 @@ struct ftrace_event_field * > > static int __trace_define_field(struct list_head *head, const char *type, > const char *name, int offset, int size, > - int is_signed, int filter_type) > + int is_signed, int filter_type, int len) > { > struct ftrace_event_field *field; > > @@ -133,6 +133,7 @@ static int __trace_define_field(struct list_head *head, const char *type, > field->offset = offset; > field->size = size; > field->is_signed = is_signed; > + field->len = len; > > list_add(&field->link, head); > > @@ -150,14 +151,28 @@ int trace_define_field(struct trace_event_call *call, const char *type, > > head = trace_get_fields(call); > return __trace_define_field(head, type, name, offset, size, > - is_signed, filter_type); > + is_signed, filter_type, 0); > } > EXPORT_SYMBOL_GPL(trace_define_field); > > +int trace_define_field_ext(struct trace_event_call *call, const char *type, > + const char *name, int offset, int size, int is_signed, > + int filter_type, int len) > +{ > + struct list_head *head; > + > + if (WARN_ON(!call->class)) > + return 0; > + > + head = trace_get_fields(call); > + return __trace_define_field(head, type, name, offset, size, > + is_signed, filter_type, len); > +} > + > #define __generic_field(type, item, filter_type) \ > ret = __trace_define_field(&ftrace_generic_fields, #type, \ > #item, 0, 0, is_signed_type(type), \ > - filter_type); \ > + filter_type, 0); \ > if (ret) \ > return ret; > > @@ -166,7 +181,7 @@ int trace_define_field(struct trace_event_call *call, const char *type, > "common_" #item, \ > offsetof(typeof(ent), item), \ > sizeof(ent.item), \ > - is_signed_type(type), FILTER_OTHER); \ > + is_signed_type(type), FILTER_OTHER, 0); \ > if (ret) \ > return ret; > > @@ -1588,12 +1603,17 @@ static int f_show(struct seq_file *m, void *v) > seq_printf(m, "\tfield:%s %s;\toffset:%u;\tsize:%u;\tsigned:%d;\n", > field->type, field->name, field->offset, > field->size, !!field->is_signed); > - else > - seq_printf(m, "\tfield:%.*s %s%s;\toffset:%u;\tsize:%u;\tsigned:%d;\n", > + else if (field->len) > + seq_printf(m, "\tfield:%.*s %s[%d];\toffset:%u;\tsize:%u;\tsigned:%d;\n", > (int)(array_descriptor - field->type), > field->type, field->name, > - array_descriptor, field->offset, > + field->len, field->offset, > field->size, !!field->is_signed); > + else > + seq_printf(m, "\tfield:%.*s %s[];\toffset:%u;\tsize:%u;\tsigned:%d;\n", > + (int)(array_descriptor - field->type), > + field->type, field->name, > + field->offset, field->size, !!field->is_signed); > > return 0; > } > @@ -2379,9 +2399,10 @@ static int ftrace_event_release(struct inode *inode, struct file *file) > } > > offset = ALIGN(offset, field->align); > - ret = trace_define_field(call, field->type, field->name, > + ret = trace_define_field_ext(call, field->type, field->name, > offset, field->size, > - field->is_signed, field->filter_type); > + field->is_signed, field->filter_type, > + field->len); > if (WARN_ON_ONCE(ret)) { > pr_err("error code is %d\n", ret); > break; > diff --git a/kernel/trace/trace_export.c b/kernel/trace/trace_export.c > index d960f6b..58f3946 100644 > --- a/kernel/trace/trace_export.c > +++ b/kernel/trace/trace_export.c > @@ -111,7 +111,8 @@ struct ____ftrace_##name { \ > #define __array(_type, _item, _len) { \ > .type = #_type"["__stringify(_len)"]", .name = #_item, \ > .size = sizeof(_type[_len]), .align = __alignof__(_type), \ > - is_signed_type(_type), .filter_type = FILTER_OTHER }, > + is_signed_type(_type), .filter_type = FILTER_OTHER, \ > + .len = _len }, > > #undef __array_desc > #define __array_desc(_type, _container, _item, _len) __array(_type, _item, _len) > -- > 1.8.3.1 > -- Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>