xfs
[Top] [All Lists]

Re: [PATCH 1/2] Make stuff static

To: Russell Cattelan <cattelan@xxxxxxxxxxx>
Subject: Re: [PATCH 1/2] Make stuff static
From: David Chatterton <chatz@xxxxxxxxxxxxxxxxx>
Date: Wed, 22 Nov 2006 13:16:17 +1100
Cc: David Chinner <dgc@xxxxxxx>, Tim Shimmin <tes@xxxxxxx>, Eric Sandeen <sandeen@xxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <1164157783.19915.46.camel@xxxxxxxxxxxxxxxxxxxx>
Organization: SGI
References: <20060929032856.8DA9C18001A5E@xxxxxxxxxxx> <23F15D6AE8566A54B81188AC@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <45338DDE.8020903@xxxxxxxxxxx> <4533FAEA.2080500@xxxxxxxxxxx> <20061016232250.GM11034@xxxxxxxxxxxxxxxxx> <1161042943.5723.117.camel@xxxxxxxxxxxxxxxxxxxx> <20061017005038.GN11034@xxxxxxxxxxxxxxxxx> <AED98B89E193744D39BAC541@xxxxxxxxxxxxxxxxxxxxxxxxxxx> <20061017215706.GI8394166@xxxxxxxxxxxxxxxxx> <1161125131.5723.158.camel@xxxxxxxxxxxxxxxxxxxx> <20061122004216.GT11034@xxxxxxxxxxxxxxxxx> <1164157783.19915.46.camel@xxxxxxxxxxxxxxxxxxxx>
Reply-to: chatz@xxxxxxxxxxxxxxxxx
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Thunderbird 1.5.0.8 (Windows/20061025)

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


<Prev in Thread] Current Thread [Next in Thread>