Re: [2.6 patch] kill __always_inline

[Tim Bird <tim.bird@am.sony.com>]

Adrian Bunk wrote:
>>>>>>The patch below removes __always_inline again:
>>>>>

>>If the compiler supports attribute((always_inline)) then the kernel build
>>system will use that.  If the compiler doesn't support
>>attribute((always_inline)) then we just emit `inline' from cpp and hope
>>that it works out.
> 
> 
> That's exactly how `inline' is already #define'd in the Linux kernel.
> 
> And __always_inline is currently simply #define'd to `inline' ...

Sorry, but the way this is currently done in the Linux kernel is, IMHO,
braindead.

I ran into a lot of problems with inline handling while I was working
on a piece of instrumentation code that used gcc's -finstrument-functions.
I found that the kernel redefinitions for inline were not being universally
applied (see the patch below for how I fixed it for my situation). I found
that the compiler version affected whether I really got an
attribute(always_inline) or not.  Also, for some uses, the include order
(based on configuration options) affected whether I got the redefinition.

I didn't do a full survey of all inline uses.  But the errors I found
with this made me nervous.

IMHO, a better approach, if it really is desired to force always_inline,
would be to globally replace 'inline' (and friends) with kern_inline
or something similar, so you at least get a compiler error if you're
picking up the traditional gcc semantics instead of the kernel-mandated
semantics.

But my preference would be to do as Andi Kleen has suggested, which
is to revert to the use of (un-redefined) 'inline' for optional inlines,
and use '__always_inline' for those special cases that REALLY need to be
inlines.  Most (but probably not all) of these are marked with 'extern
inline' in the code today. (Unfortunately, there are a few optional
ones marked with 'extern inline' as well - which complicates things).
I realize this is riskier, but it seems to make more sense in the
long run.

Finally, I think it's bad form to change the meaning of a compiler
keyword.  It misleading for 'inline' to mean something different
in the kernel than it does everywhere else.  It means a developer
can't use standard gcc documentation to understand kernel code, without
inside knowledge.  This can be painful for casual or new kernel
developers.

Regards,
   -- Tim

diff -ruN -X ../../dontdiff linux-2.6.7/include/asm-ppc/delay.h branch_KFI/include/asm-ppc/delay.h
--- linux-2.6.7/include/asm-ppc/delay.h	2004-06-15 22:19:42.000000000 -0700
+++ branch_KFI/include/asm-ppc/delay.h	2004-08-11 15:30:22.000000000 -0700
@@ -2,6 +2,7 @@
  #ifndef _PPC_DELAY_H
  #define _PPC_DELAY_H

+#include <linux/compiler.h>	/* for inline weirdness */
  #include <asm/param.h>

  /*
diff -ruN -X ../../dontdiff linux-2.6.7/include/asm-ppc/processor.h branch_KFI/include/asm-ppc/processor.h
--- linux-2.6.7/include/asm-ppc/processor.h	2004-06-15 22:18:38.000000000 -0700
+++ branch_KFI/include/asm-ppc/processor.h	2004-08-11 15:35:25.000000000 -0700
@@ -10,6 +10,7 @@

  #include <linux/config.h>
  #include <linux/stringify.h>
+#include <linux/compiler.h>	/* for inline weirdness */

  #include <asm/ptrace.h>
  #include <asm/types.h>
diff -ruN -X ../../dontdiff linux-2.6.7/include/linux/compiler-gcc3.h branch_KFI/include/linux/compiler-gcc3.h
--- linux-2.6.7/include/linux/compiler-gcc3.h	2004-06-15 22:18:45.000000000 -0700
+++ branch_KFI/include/linux/compiler-gcc3.h	2004-08-11 14:16:34.000000000 -0700
@@ -3,7 +3,7 @@
  /* These definitions are for GCC v3.x.  */
  #include <linux/compiler-gcc.h>

-#if __GNUC_MINOR__ >= 1  && __GNUC_MINOR__ < 4
+#if __GNUC_MINOR__ >= 1
  # define inline		__inline__ __attribute__((always_inline))
  # define __inline__	__inline__ __attribute__((always_inline))
  # define __inline	__inline__ __attribute__((always_inline))


=============================
Tim Bird
Architecture Group Co-Chair, CE Linux Forum
Senior Staff Engineer, Sony Electronics
E-mail: tim.bird@am.sony.com
=============================