From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751255Ab1AEC64 (ORCPT ); Tue, 4 Jan 2011 21:58:56 -0500 Received: from mail-fx0-f46.google.com ([209.85.161.46]:38617 "EHLO mail-fx0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750895Ab1AEC6z (ORCPT ); Tue, 4 Jan 2011 21:58:55 -0500 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; b=PnfZGKE/UPKGnyrL+M5ybT6F+Y5L+KFdySBnh3NgT31mhUP94Dn5EtIL/k0U0w0JUo miBcNP3hXwjrCnKghTR06+qUpQWWQW99mZY6B5Iw9Pu4qkSBxT52FLqPJqPrFF8V5x7v Eb0a8A9aBRFuleB0NvEsZCZOFpNZ+fCcVJD2o= Date: Wed, 5 Jan 2011 03:58:50 +0100 From: Frederic Weisbecker To: Mathieu Desnoyers Cc: LKML , Steven Rostedt , Ingo Molnar , Thomas Gleixner Subject: Re: [RFC patch 3/5] ftrace trace event add missing semicolumn Message-ID: <20110105025848.GH2911@nowhere> References: <20110104231629.996422888@efficios.com> <20110104232419.441463699@efficios.com> <20110105000005.GE2911@nowhere> <20110105001837.GA9737@Krystal> <20110105020759.GG2911@nowhere> <20110105023541.GA12950@Krystal> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20110105023541.GA12950@Krystal> User-Agent: Mutt/1.5.20 (2009-06-14) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > > > > CC: Steven Rostedt > > > > > CC: Frederic Weisbecker > > > > > CC: Ingo Molnar > > > > > CC: Thomas Gleixner > > > > > --- > > > > > 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 :)