| Message ID | 20230309221302.642e82d9@gandalf.local.home (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f82e7ca019dfad3b006fd3b772f7ac569672db55 |
| Headers | show |
| Series | tracing: Error if a trace event has an array for a __field() | expand |
Hi Steven,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v6.3-rc1]
[cannot apply to next-20230310]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Steven-Rostedt/tracing-Error-if-a-trace-event-has-an-array-for-a-__field/20230310-111456
patch link: https://lore.kernel.org/r/20230309221302.642e82d9%40gandalf.local.home
patch subject: [PATCH] tracing: Error if a trace event has an array for a __field()
config: x86_64-randconfig-a006 (https://download.01.org/0day-ci/archive/20230310/202303101645.28bnQoH2-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/4828e98216144ead91bbb26298aae865dac9f837
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Steven-Rostedt/tracing-Error-if-a-trace-event-has-an-array-for-a-__field/20230310-111456
git checkout 4828e98216144ead91bbb26298aae865dac9f837
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 olddefconfig
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash kernel/rcu/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303101645.28bnQoH2-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from include/trace/define_trace.h:102,
from include/trace/events/rcu.h:839,
from kernel/rcu/rcu.h:13,
from kernel/rcu/update.c:49:
include/trace/events/rcu.h: In function 'trace_event_get_offsets_rcu_torture_read':
>> include/trace/stages/stage5_get_offsets.h:23:53: error: expected ')' before '*' token
23 | { (void)sizeof(struct _test_no_array_##item *); }
| ~ ^
include/trace/trace_events.h:263:9: note: in definition of macro 'DECLARE_EVENT_CLASS'
263 | tstruct; \
| ^~~~~~~
include/trace/trace_events.h:43:30: note: in expansion of macro 'PARAMS'
43 | PARAMS(tstruct), \
| ^~~~~~
include/trace/events/rcu.h:11:25: note: in expansion of macro 'TRACE_EVENT'
11 | #define TRACE_EVENT_RCU TRACE_EVENT
| ^~~~~~~~~~~
include/trace/events/rcu.h:770:9: note: in expansion of macro 'TP_STRUCT__entry'
770 | TP_STRUCT__entry(
| ^~~~~~~~~~~~~~~~
include/trace/events/rcu.h:771:17: note: in expansion of macro '__field'
771 | __field(char, rcutorturename[RCUTORTURENAME_LEN])
| ^~~~~~~
>> include/trace/events/rcu.h:771:45: error: array type has incomplete element type 'struct _test_no_array_rcutorturename'
771 | __field(char, rcutorturename[RCUTORTURENAME_LEN])
| ^
include/trace/trace_events.h:263:9: note: in definition of macro 'DECLARE_EVENT_CLASS'
263 | tstruct; \
| ^~~~~~~
include/trace/trace_events.h:43:30: note: in expansion of macro 'PARAMS'
43 | PARAMS(tstruct), \
| ^~~~~~
include/trace/events/rcu.h:11:25: note: in expansion of macro 'TRACE_EVENT'
11 | #define TRACE_EVENT_RCU TRACE_EVENT
| ^~~~~~~~~~~
include/trace/events/rcu.h:770:9: note: in expansion of macro 'TP_STRUCT__entry'
770 | TP_STRUCT__entry(
| ^~~~~~~~~~~~~~~~
include/trace/events/rcu.h:771:17: note: in expansion of macro '__field'
771 | __field(char, rcutorturename[RCUTORTURENAME_LEN])
| ^~~~~~~
vim +23 include/trace/stages/stage5_get_offsets.h
11
12 /*
13 * Fields should never declare an array: i.e. __field(int, arr[5])
14 * If they do, it will cause issues in parsing and possibly corrupt the
15 * events. To prevent that from happening, test the sizeof() a fictitious
16 * type called "struct _test_no_array_##item" which will fail if "item"
17 * contains array elements (like "arr[5]").
18 *
19 * If you hit this, use __array(int, arr, 5) instead.
20 */
21 #undef __field
22 #define __field(type, item) \
> 23 { (void)sizeof(struct _test_no_array_##item *); }
24
On Fri, 10 Mar 2023 16:37:09 +0800 kernel test robot <lkp@intel.com> wrote: > include/trace/events/rcu.h:771:17: note: in expansion of macro '__field' > 771 | __field(char, rcutorturename[RCUTORTURENAME_LEN]) > | ^~~~~~~ Awesome, it found the bug that this patch is suppose to find! :-) The above needs to be changed to: __array(char, rcutorturename, RCUTORTURENAME_LEN) And my patch will fail builds that have arrays in __field() macros. Yes, I'm not going to apply this patch until the current bugs in the kernel are fixed, because this patch will cause the kernel not to build if it has this type of bug. -- Steve
On Thu, 9 Mar 2023 22:13:02 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > A __field() in the TRACE_EVENT() macro is used to set up the fields of the > trace event data. It is for single storage units (word, char, int, > pointer, etc) and not for complex structures or arrays. Unfortunately, > there's nothing preventing the build from accepting: > > __field(int, arr[5]); > > from building. It will turn into a array value. This use to work fine, as > the offset and size use to be determined by the macro using the field name, > but things have changed and the offset and size are now determined by the > type. So the above would only be size 4, and the next field will be > located 4 bytes from it (instead of 20). > > The proper way to declare static arrays is to use the __array() macro. > > Instead of __field(int, arr[5]) it should be __array(int, arr, 5). > > Add some macro tricks to the building of a trace event from the > TRACE_EVENT() macro such that __field(int, arr[5]) will fail to build. A > comment by the failure will explain why the build failed. > > Link: https://lore.kernel.org/lkml/20230306122549.236561-1-douglas.raillard@arm.com/ > Looks good to me. Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> Thank you! > Reported-by: Douglas RAILLARD <douglas.raillard@arm.com> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > --- > include/trace/stages/stage5_get_offsets.h | 21 +++++++++++++++++---- > 1 file changed, 17 insertions(+), 4 deletions(-) > > diff --git a/include/trace/stages/stage5_get_offsets.h b/include/trace/stages/stage5_get_offsets.h > index ac5c24d3beeb..e30a13be46ba 100644 > --- a/include/trace/stages/stage5_get_offsets.h > +++ b/include/trace/stages/stage5_get_offsets.h > @@ -9,17 +9,30 @@ > #undef __entry > #define __entry entry > > +/* > + * Fields should never declare an array: i.e. __field(int, arr[5]) > + * If they do, it will cause issues in parsing and possibly corrupt the > + * events. To prevent that from happening, test the sizeof() a fictitious > + * type called "struct _test_no_array_##item" which will fail if "item" > + * contains array elements (like "arr[5]"). > + * > + * If you hit this, use __array(int, arr, 5) instead. > + */ > #undef __field > -#define __field(type, item) > +#define __field(type, item) \ > + { (void)sizeof(struct _test_no_array_##item *); } > > #undef __field_ext > -#define __field_ext(type, item, filter_type) > +#define __field_ext(type, item, filter_type) \ > + { (void)sizeof(struct _test_no_array_##item *); } > > #undef __field_struct > -#define __field_struct(type, item) > +#define __field_struct(type, item) \ > + { (void)sizeof(struct _test_no_array_##item *); } > > #undef __field_struct_ext > -#define __field_struct_ext(type, item, filter_type) > +#define __field_struct_ext(type, item, filter_type) \ > + { (void)sizeof(struct _test_no_array_##item *); } > > #undef __array > #define __array(type, item, len) > -- > 2.39.1 >
diff --git a/include/trace/stages/stage5_get_offsets.h b/include/trace/stages/stage5_get_offsets.h index ac5c24d3beeb..e30a13be46ba 100644 --- a/include/trace/stages/stage5_get_offsets.h +++ b/include/trace/stages/stage5_get_offsets.h @@ -9,17 +9,30 @@ #undef __entry #define __entry entry +/* + * Fields should never declare an array: i.e. __field(int, arr[5]) + * If they do, it will cause issues in parsing and possibly corrupt the + * events. To prevent that from happening, test the sizeof() a fictitious + * type called "struct _test_no_array_##item" which will fail if "item" + * contains array elements (like "arr[5]"). + * + * If you hit this, use __array(int, arr, 5) instead. + */ #undef __field -#define __field(type, item) +#define __field(type, item) \ + { (void)sizeof(struct _test_no_array_##item *); } #undef __field_ext -#define __field_ext(type, item, filter_type) +#define __field_ext(type, item, filter_type) \ + { (void)sizeof(struct _test_no_array_##item *); } #undef __field_struct -#define __field_struct(type, item) +#define __field_struct(type, item) \ + { (void)sizeof(struct _test_no_array_##item *); } #undef __field_struct_ext -#define __field_struct_ext(type, item, filter_type) +#define __field_struct_ext(type, item, filter_type) \ + { (void)sizeof(struct _test_no_array_##item *); } #undef __array #define __array(type, item, len)