From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: with ECARTIS (v1.0.0; list xfs); Tue, 21 Nov 2006 18:17:34 -0800 (PST) Received: from larry.melbourne.sgi.com (larry.melbourne.sgi.com [134.14.52.130]) by oss.sgi.com (8.12.10/8.12.10/SuSE Linux 0.7) with SMTP id kAM2HMaG007921 for ; Tue, 21 Nov 2006 18:17:24 -0800 Message-ID: <4563B2F1.6040603@melbourne.sgi.com> Date: Wed, 22 Nov 2006 13:16:17 +1100 From: David Chatterton Reply-To: chatz@melbourne.sgi.com MIME-Version: 1.0 Subject: Re: [PATCH 1/2] Make stuff static References: <20060929032856.8DA9C18001A5E@sandeen.net> <23F15D6AE8566A54B81188AC@timothy-shimmins-power-mac-g5.local> <45338DDE.8020903@sandeen.net> <4533FAEA.2080500@sandeen.net> <20061016232250.GM11034@melbourne.sgi.com> <1161042943.5723.117.camel@xenon.msp.redhat.com> <20061017005038.GN11034@melbourne.sgi.com> <20061017215706.GI8394166@melbourne.sgi.com> <1161125131.5723.158.camel@xenon.msp.redhat.com> <20061122004216.GT11034@melbourne.sgi.com> <1164157783.19915.46.camel@xenon.msp.redhat.com> In-Reply-To: <1164157783.19915.46.camel@xenon.msp.redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: xfs-bounce@oss.sgi.com Errors-to: xfs-bounce@oss.sgi.com List-Id: xfs To: Russell Cattelan Cc: David Chinner , Tim Shimmin , Eric Sandeen , xfs@oss.sgi.com Russell Cattelan wrote: > On Wed, 2006-11-22 at 11:42 +1100, David Chinner wrote: >> On Tue, Oct 17, 2006 at 05:45:31PM -0500, Russell Cattelan wrote: >>> On Wed, 2006-10-18 at 07:57 +1000, David Chinner wrote: >>>> On Tue, Oct 17, 2006 at 05:13:01PM +1000, Tim Shimmin wrote: >>>>> I thought that for debug, we could stop them from being inline >>>>> for easier debugging. We could have a STATIC_INLINE :-) >>>> We could, but I don't think it gains us anything. >>> I agree with Tim on this. >>> when I see STATIC in the code it's generally assumed to >>> be a way to toggle of static on/off. Adding static inline >>> to the #define STATIC starts to overload the the macro >>> and creates an obfuscation that isn't immediately obvious. >>> STATIC_INLINE should be fairly obvious. >> Ok, so I've had time to look at this again. Here's the definitions >> of STATIC and STATIC_INLINE for debug and nondebug from the >> patch (whitespace damaged): >> >> Index: 2.6.x-xfs-new/fs/xfs/support/debug.h >> =================================================================== >> --- 2.6.x-xfs-new.orig/fs/xfs/support/debug.h 2006-11-22 10:54:37.089984780 +1100 >> +++ 2.6.x-xfs-new/fs/xfs/support/debug.h 2006-11-22 11:30:20.433326839 +1100 >> @@ -38,13 +38,37 @@ extern void assfail(char *expr, char *f, >> >> #ifndef DEBUG >> # define ASSERT(expr) ((void)0) >> -#else >> + >> +#ifndef STATIC >> +# define STATIC static noinline >> +#endif >> + >> +#ifndef STATIC_INLINE >> +# define STATIC_INLINE static inline >> +#endif >> + >> +#else /* DEBUG */ >> + >> # define ASSERT(expr) ASSERT_ALWAYS(expr) >> extern unsigned long random(void); >> -#endif >> >> #ifndef STATIC >> -# define STATIC static >> +# define STATIC noinline >> +#endif >> + >> +/* >> + * We stop inlining of inline functions in debug mode. >> + * Unfortunately, this means static inline in header files >> + * get multiple definitions, so they need to remain static. >> + * This then gives tonnes of warnings about unused but defined >> + * functions, so we need to add the unused attribute to prevent >> + * these spurious warnings. >> + */ >> +#ifndef STATIC_INLINE >> +# define STATIC_INLINE static __attribute__ ((unused)) noinline >> #endif >> >> +#endif /* DEBUG */ >> + >> + >> #endif /* __XFS_SUPPORT_DEBUG_H__ */ >> >> ------ >> >> Is this acceptible to everyone? > Yup. > >> FWIW, there is one other thing that this conversion causes >> problems with, and that's variable definitions. i.e. we can't >> use STATIC on them any more because of the "noinline" attribute >> it has. Do we care about this and if so, any suggestions on >> how to keep this functionality (a different STATIC_xxx define >> for structures)? > So I know things like systemtap kgdb oprofile all work better when > functions are not static, but what about variables/structures? > do things really get that confused? > Maybe we shouldn't worry about conditioning them and just make them > static > I agree with Russell, is there a case for not defining a structure static? I can't think of one, unless it kdb/lcrash is going to work better if they are not static in a debug build. Otherwise, we should just use "static" and not "STATIC". Some for static file variables. David -- David Chatterton XFS Engineering Manager SGI Australia