Re: [RFC patch 3/5] ftrace trace event add missing semicolumn

[Frederic Weisbecker <fweisbec@gmail.com>]

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 :)