On Tue, Aug 31, 2004 at 04:39:14PM -0700, Andrew Morton wrote:
> Nigel Cunningham <[email protected]> wrote:
> >
> > Hi.
> >
> > On Wed, 2004-09-01 at 08:52, Adrian Bunk wrote:
> > > On Tue, Aug 31, 2004 at 03:36:49PM -0700, Andrew Morton wrote:
> > > > Adrian Bunk <[email protected]> wrote:
> > > > >
> > > > > An issue that we already discussed at 2.6.8-rc2-mm2 times:
> > > > >
> > > > > 2.6.9-rc1 includes __always_inline which was formerly in -mm.
> > > > > __always_inline doesn't make any sense:
> > > > >
> > > > > __always_inline is _exactly_ the same as __inline__, __inline and inline .
> > > > >
> > > > >
> > > > > The patch below removes __always_inline again:
> > > >
> > > > But what happens if we later change `inline' so that it doesn't do
> > > > the `always inline' thing?
> > > >
> > > > An explicit usage of __always_inline is semantically different than
> > > > boring old `inline'.
> >
> > Excuse me if I'm being ignorant, but I thought always_inline was
> > introduced because with some recent versions of gcc, inline wasn't doing
> > the job (suspend2, which requires a working inline, was broken by it for
> > example).
>
> IIRC, the compiler was generating out-of-line versions of functions in
> every compilation unit whcih included the header file. When we the
> developers just wanted `inline' to mean `inline, dammit'.
`inline, dammit' is
__attribute__((always_inline))
The original `inline' never guarantees that the function will be
inlined.
Therefore, `inline' is already #define'd in the Linux kernel to always
be __attribute__((always_inline)).
> If that broke swsusp in some manner then the relevant swsusp functions
> should be marked always_inline, because they have some special needs.
>
> > That is to say, doesn't the definition of always_inline vary
> > with the compiler version?
>
> 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' ...
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
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: [email protected]
=============================
Tim Bird <[email protected]> wrote:
>
> 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.
yes, it's horrid. But until and unless gcc gets fixed, what choice do we
have? Without this hack, kernel text size increases significantly.
We don't want to have to patch the kernel source in a zillion places for
what is hopefully a temporary problem. I'd much rather add `--dwim-dammit'
to the gcc command line.
> +#include <linux/compiler.h> /* for inline weirdness */
> #include <asm/param.h>
Mozilla white-space-stuffed your patch. You'll have to use attachments.