Messages in this thread | | | Date | Wed, 5 Jan 2011 03:58:50 +0100 | From | Frederic Weisbecker <> | Subject | Re: [RFC patch 3/5] ftrace trace event add missing semicolumn |
| |
On Tue, Jan 04, 2011 at 09:35:41PM -0500, Mathieu Desnoyers wrote: > * Frederic Weisbecker (fweisbec@gmail.com) wrote: > > On Tue, Jan 04, 2011 at 07:18:37PM -0500, Mathieu Desnoyers wrote: > > > * Frederic Weisbecker (fweisbec@gmail.com) wrote: > > > > On Tue, Jan 04, 2011 at 06:16:32PM -0500, Mathieu Desnoyers wrote: > > > > > Add a missing semicolumn at the end of a ftrace definition. > > > > > > > > > > We currently are not seeing any impact of this missing semicolumn because extra > > > > > semicolumns appear all over the place in the code generated from TRACE_EVENT > > > > > within ftrace stages. > > > > > > > > > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > > > > > CC: Steven Rostedt <rostedt@goodmis.org> > > > > > CC: Frederic Weisbecker <fweisbec@gmail.com> > > > > > CC: Ingo Molnar <mingo@elte.hu> > > > > > CC: Thomas Gleixner <tglx@linutronix.de> > > > > > --- > > > > > include/trace/ftrace.h | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > Index: linux-2.6-lttng/include/trace/ftrace.h > > > > > =================================================================== > > > > > --- linux-2.6-lttng.orig/include/trace/ftrace.h > > > > > +++ linux-2.6-lttng/include/trace/ftrace.h > > > > > @@ -69,7 +69,7 @@ > > > > > #undef DEFINE_EVENT > > > > > #define DEFINE_EVENT(template, name, proto, args) \ > > > > > static struct ftrace_event_call __used \ > > > > > - __attribute__((__aligned__(4))) event_##name > > > > > + __attribute__((__aligned__(4))) event_##name; > > > > > > > > But DEFINE_EVENT() calls are supposed to be ";" terminated, no? > > > > > > Currently yes, but if you look at the preprocessor output currently generated by > > > the current TRACE_EVENT()/DEFINE_EVENT() scheme, there are useless ";" added all > > > over the place. I have a patch later in my queue that proposes removal of these > > > extra ";" as a cleanup of the TRACE_EVENT() semantic, but I'm keeping it for > > > later because it removes the extra ";" at the end of each TRACE_EVENT() > > > instance (and thus is more intrusive code-wise). > > > > > > Adding this semicolumn here ensures that all Ftrace macros are consistent wrt > > > semicolumns. We can get away without consistency currently exactly because the > > > current scheme adds many useless semicolumns between each TRACE_EVENT(). > > > > Are you sure you want to put so much time on this? > > We are about to spend more time arguing about it that the time it takes cleaning > it up. But here we go.
Hardly, as the real cleanup requires dozens of patches to clean every trace events.
But dropping the requirement of a ";" after the macro calls still sounds sensible.
> > > > This will require a massive change for the sole win of removing double ";" > > in generated code. This won't optimize much the build, and it will make the things > > not so much more readable for very rare people who dare to have interest into the > > TRACE_EVENT generated code. That notwithstanding the obfuscation of that generated > > code resides more in the lack of indentation and newlines than in double > > semicolons that we barely notice. > > Building: > > make kernel/sched.o V=1 > > taking the gcc invokation, changing it to: > > gcc -Wp,-MD,kernel/.sched.o.d -nostdinc -isystem /usr/local/lib/gcc/x86_64-unknown-linux-gnu/4.5.1/include -I/home/compudj/git/linux-tip/arch/x86/include -Iinclude -include include/generated/autoconf.h -D__KERNEL__ -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -Werror-implicit-function-declaration -Wno-format-security -fno-delete-null-pointer-checks -O2 -m64 -march=core2 -mno-red-zone -mcmodel=kernel -funit-at-a-time -maccumulate-outgoing-args -DCONFIG_AS_CFI=1 -DCONFIG_AS_CFI_SIGNAL_FRAME=1 -DCONFIG_AS_CFI_SECTIONS=1 -pipe -Wno-sign-compare -fno-asynchronous-unwind-tables -mno-sse -mno-mmx -mno-sse2 -mno-3dnow -Wframe-larger-than=1024 -fno-stack-protector -fno-omit-frame-pointer -fno-optimize-sibling-calls -Wdeclaration-after-statement -Wno-pointer-sign -fno-strict-overflow -fconserve-stack -D"KBUILD_STR(s)=#s" -D"KBUILD_BASENAME=KBUILD_STR(sched)" -D"KBUILD_MODNAME=KBUILD_STR(sched)" -E -o kernel/sched.pp kernel/sched.c > > An exerpt of the output, fed through "indent" for readability: > > static inline __attribute__ ((always_inline)) > void > # 275 "include/trace/events/sched.h" > check_trace_callback_type_sched_process_fork > # 252 "include/trace/events/sched.h" > > (void (*cb) > (void *__data, struct task_struct * parent, > struct task_struct * child)) > { > } > # 275 "include/trace/events/sched.h" > ; > > > > > > > # 305 "include/trace/events/sched.h" > ; > > > As we can notice, a few extra ";" are added between each "entity" created by the > ftrace trace event phase. This works only as long as we declare > semicolumn-separated C structure elements, functions, and statements, because > the compiler just skips the extra semicolumns, but forbids creation of arrays of > events, which need to be comma-separated.
So what are these arrays of events you have in mind?
> These extra semicolumns we see here are simply polluting the compiler input, and > I don't see any reason why we should leave them there.
Sure they are useless, I just wonder if that alone justifies such a massive change, although actually I don't care much :)
| |