Re: huge gcc 4.1.{0,1} __weak problem

Re: huge gcc 4.1.{0,1} __weak problem

[Chris Knadle]

On Thu, 1 May 2008, Linus Torvalds wrote:
> On Thu, 1 May 2008, Andrew Morton wrote:
> > > 
> > > I see only the following choices:
> > > - remove __weak and replace all current usages
> > > - move all __weak functions into own files, and ensure that also happens
> > >   for future usages
> > > - #error for gcc 4.1.{0,1}
> > 
> > Can we detect the {0,1}?  __GNUC_EVEN_MORE_MINOR__?
>
> It's __GNUC_PATCHLEVEL__, I believe.
>
> So yes, we can distinguish 4.1.2 (good, and very common) from 4.1.{0,1} 
> (bad, and rather uncommon).
> And yes, considering that 4.1.1 (and even more so 4.1.0) should be rare to 
> begin with, I think it's better to just not support it.
>
>			Linus

   Unfortunately Debian Stable (i.e. Etch), which is relatively popular for server 
use, is still using 4.1.1  :-(  (The current gcc package is gcc-4.1.1-21)

   I have not looked to see if Debian Stable's gcc-4.1.1-21 has been patched for 
the currently discussed __weak bug.

   -- Chris

Chris Knadle
Chris.Knadle@coredump.us

Re: huge gcc 4.1.{0,1} __weak problem

[Miquel van Smoorenburg]

On Thu, 2008-05-01 at 19:55 -0400, Chris Knadle wrote:
> On Thu, 1 May 2008, Linus Torvalds wrote:
> > On Thu, 1 May 2008, Andrew Morton wrote:
> > > > 
> > > > I see only the following choices:
> > > > - remove __weak and replace all current usages
> > > > - move all __weak functions into own files, and ensure that also happens
> > > >   for future usages
> > > > - #error for gcc 4.1.{0,1}
> > > 
> > > Can we detect the {0,1}?  __GNUC_EVEN_MORE_MINOR__?
> >
> > It's __GNUC_PATCHLEVEL__, I believe.
> >
> > So yes, we can distinguish 4.1.2 (good, and very common) from 4.1.{0,1} 
> > (bad, and rather uncommon).
> > And yes, considering that 4.1.1 (and even more so 4.1.0) should be rare to 
> > begin with, I think it's better to just not support it.

>    Unfortunately Debian Stable (i.e. Etch), which is relatively popular for server 
> use, is still using 4.1.1  :-(  (The current gcc package is gcc-4.1.1-21)

Well the package version string is 4.1.1, but the compiler thinks it's
4.1.2, at least on i386, amd64 and ia64:

$ cc -v
gcc version 4.1.2 20061115 (prerelease) (Debian 4.1.1-21)

A small test program agrees:

        printf("%d\n", __GNUC_PATCHLEVEL__);
$ ./a.out 
2

Mike.

Re: huge gcc 4.1.{0,1} __weak problem

[Alistair John Strachan]

Hi Chris,

(I fixed the corrupted CC and Reply-to: address from your email.)

On Friday 02 May 2008 00:55:58 Chris Knadle wrote:
> On Thu, 1 May 2008, Linus Torvalds wrote:
> > On Thu, 1 May 2008, Andrew Morton wrote:
> > > > I see only the following choices:
> > > > - remove __weak and replace all current usages
> > > > - move all __weak functions into own files, and ensure that also
> > > > happens for future usages
> > > > - #error for gcc 4.1.{0,1}
> > >
> > > Can we detect the {0,1}?  __GNUC_EVEN_MORE_MINOR__?
> >
> > It's __GNUC_PATCHLEVEL__, I believe.
> >
> > So yes, we can distinguish 4.1.2 (good, and very common) from 4.1.{0,1}
> > (bad, and rather uncommon).
> > And yes, considering that 4.1.1 (and even more so 4.1.0) should be rare
> > to begin with, I think it's better to just not support it.
> >
> >			Linus
>
>    Unfortunately Debian Stable (i.e. Etch), which is relatively popular for
> server use, is still using 4.1.1  :-(  (The current gcc package is
> gcc-4.1.1-21)
>
>    I have not looked to see if Debian Stable's gcc-4.1.1-21 has been
> patched for the currently discussed __weak bug.

I checked and it has been patched in 4.1.1-21. This would make checking for 
4.1.1 via __GNUC_PATCHLEVEL__ potentially invalid, as patched distro 
compilers may (and in this case do) have this fixed.

-- 
Cheers,
Alistair.

137/1 Warrender Park Road, Edinburgh, UK.

Re: huge gcc 4.1.{0,1} __weak problem

[Sam Ravnborg]

On Fri, May 02, 2008 at 10:55:09AM +0100, Alistair John Strachan wrote:
> Hi Chris,
> 
> (I fixed the corrupted CC and Reply-to: address from your email.)
> 
> On Friday 02 May 2008 00:55:58 Chris Knadle wrote:
> > On Thu, 1 May 2008, Linus Torvalds wrote:
> > > On Thu, 1 May 2008, Andrew Morton wrote:
> > > > > I see only the following choices:
> > > > > - remove __weak and replace all current usages
> > > > > - move all __weak functions into own files, and ensure that also
> > > > > happens for future usages
> > > > > - #error for gcc 4.1.{0,1}
> > > >
> > > > Can we detect the {0,1}?  __GNUC_EVEN_MORE_MINOR__?
> > >
> > > It's __GNUC_PATCHLEVEL__, I believe.
> > >
> > > So yes, we can distinguish 4.1.2 (good, and very common) from 4.1.{0,1}
> > > (bad, and rather uncommon).
> > > And yes, considering that 4.1.1 (and even more so 4.1.0) should be rare
> > > to begin with, I think it's better to just not support it.
> > >
> > >			Linus
> >
> >    Unfortunately Debian Stable (i.e. Etch), which is relatively popular for
> > server use, is still using 4.1.1  :-(  (The current gcc package is
> > gcc-4.1.1-21)
> >
> >    I have not looked to see if Debian Stable's gcc-4.1.1-21 has been
> > patched for the currently discussed __weak bug.
> 
> I checked and it has been patched in 4.1.1-21. This would make checking for 
> 4.1.1 via __GNUC_PATCHLEVEL__ potentially invalid, as patched distro 
> compilers may (and in this case do) have this fixed.

Is it possible to cook up a small sample file we could build as part
of the kernel build. If it fails => error out.
If someone comes up with the code I shall try to integrate it
in the build system.

	Sam

Re: huge gcc 4.1.{0,1} __weak problem

[Alistair John Strachan]

On Friday 02 May 2008 11:43:48 Sam Ravnborg wrote:
[snip]
> > > > It's __GNUC_PATCHLEVEL__, I believe.
> > > >
> > > > So yes, we can distinguish 4.1.2 (good, and very common) from
> > > > 4.1.{0,1} (bad, and rather uncommon).
> > > > And yes, considering that 4.1.1 (and even more so 4.1.0) should be
> > > > rare to begin with, I think it's better to just not support it.
> > > >
> > > >			Linus
> > >
> > >    Unfortunately Debian Stable (i.e. Etch), which is relatively popular
> > > for server use, is still using 4.1.1  :-(  (The current gcc package is
> > > gcc-4.1.1-21)
> > >
> > >    I have not looked to see if Debian Stable's gcc-4.1.1-21 has been
> > > patched for the currently discussed __weak bug.
> >
> > I checked and it has been patched in 4.1.1-21. This would make checking
> > for 4.1.1 via __GNUC_PATCHLEVEL__ potentially invalid, as patched distro
> > compilers may (and in this case do) have this fixed.
>
> Is it possible to cook up a small sample file we could build as part
> of the kernel build. If it fails => error out.
> If someone comes up with the code I shall try to integrate it
> in the build system.

The GCC PR has a test case for this regression which might be usable.

http://gcc.gnu.org/viewcvs/branches/gcc-4_1-branch/gcc/testsuite/gcc.dg/tree-ssa/pr27781.c?view=markup&pathrev=114852

-- 
Cheers,
Alistair.

137/1 Warrender Park Road, Edinburgh, UK.

Re: huge gcc 4.1.{0,1} __weak problem

[Sam Ravnborg]

On Fri, May 02, 2008 at 12:48:55PM +0100, Alistair John Strachan wrote:
> On Friday 02 May 2008 11:43:48 Sam Ravnborg wrote:
> [snip]
> > > > > It's __GNUC_PATCHLEVEL__, I believe.
> > > > >
> > > > > So yes, we can distinguish 4.1.2 (good, and very common) from
> > > > > 4.1.{0,1} (bad, and rather uncommon).
> > > > > And yes, considering that 4.1.1 (and even more so 4.1.0) should be
> > > > > rare to begin with, I think it's better to just not support it.
> > > > >
> > > > >			Linus
> > > >
> > > >    Unfortunately Debian Stable (i.e. Etch), which is relatively popular
> > > > for server use, is still using 4.1.1  :-(  (The current gcc package is
> > > > gcc-4.1.1-21)
> > > >
> > > >    I have not looked to see if Debian Stable's gcc-4.1.1-21 has been
> > > > patched for the currently discussed __weak bug.
> > >
> > > I checked and it has been patched in 4.1.1-21. This would make checking
> > > for 4.1.1 via __GNUC_PATCHLEVEL__ potentially invalid, as patched distro
> > > compilers may (and in this case do) have this fixed.
> >
> > Is it possible to cook up a small sample file we could build as part
> > of the kernel build. If it fails => error out.
> > If someone comes up with the code I shall try to integrate it
> > in the build system.
> 
> The GCC PR has a test case for this regression which might be usable.
> 
> http://gcc.gnu.org/viewcvs/branches/gcc-4_1-branch/gcc/testsuite/gcc.dg/tree-ssa/pr27781.c?view=markup&pathrev=114852

OK, can anyone confirm that this fails to build which a
buggy gcc:


void __attribute__((weak)) func(void)
{
    /* no code */
}

int main()
{
    func();
    return 0;
}



	Sam

Re: huge gcc 4.1.{0,1} __weak problem

[Jakub Jelinek]

On Fri, May 02, 2008 at 03:57:08PM +0200, Sam Ravnborg wrote:
> OK, can anyone confirm that this fails to build which a
> buggy gcc:
> 
> 
> void __attribute__((weak)) func(void)
> {
>     /* no code */
> }
> 
> int main()
> {
>     func();
>     return 0;
> }

Of course it doesn't fail to build.  With buggy gcc it will be optimized
to
void __attribute__((weak)) func (void)
{
}

int main ()
{
  return 0;
}

(similarly how all recent gccs optimize this without the weak attribute)
while non-buggy gcc keeps the func call.
So, you either need to grep the assembly (that's what e.g. the GCC testcase
does), or you can e.g. use a runtime testcase:
extern void abort (void);
void __attribute__((weak)) func (void) { }
int main () { func (); abort (); }
in one compilation unit and
extern void exit (int);
void func (void) { exit (0); }
in another one.  I doubt a runtime testcase is acceptable though for the
kernel, as the cross compiler used to build the kernel might not be able to
create userland executables (missing C library, etc.).

	Jakub

Re: huge gcc 4.1.{0,1} __weak problem

[Alistair John Strachan]

On Friday 02 May 2008 15:11:32 Jakub Jelinek wrote:
> On Fri, May 02, 2008 at 03:57:08PM +0200, Sam Ravnborg wrote:
> > OK, can anyone confirm that this fails to build which a
> > buggy gcc:
> >
> >
> > void __attribute__((weak)) func(void)
> > {
> >     /* no code */
> > }
> >
> > int main()
> > {
> >     func();
> >     return 0;
> > }
>
> Of course it doesn't fail to build.
[snip]
>  I doubt a runtime testcase is acceptable though for the
> kernel, as the cross compiler used to build the kernel might not be able to
> create userland executables (missing C library, etc.).

I assume the GCC testsuite has the same generic problem, which is probably why 
it uses the -fdump-tree-optimized parameter to gcc. For example:

alistair@just:~$ cat test.c
void __attribute__((weak)) func(void)
{
    /* no code */
}

int main()
{
    func();
    return 0;
}

alistair@just:~$ gcc -O2 -fdump-tree-optimized test.c
alistair@just:~$ rm -f a.out
alistair@just:~$ cat test.c*.optimized | egrep "func \\(\\);"
  func ();

Ergo, my compiler isn't buggy. As this doesn't require a runtime test I think 
it would be OK for the kernel.

Of course, whether or not it's worth it is now debatable, given the 
information Sven-Haegar Koch provided (Debian's GCC version number is 
actually 4.1.2).

-- 
Cheers,
Alistair.

137/1 Warrender Park Road, Edinburgh, UK.

Re: huge gcc 4.1.{0,1} __weak problem

[Jeremy Fitzhardinge]

Sam Ravnborg wrote:
> OK, can anyone confirm that this fails to build which a
> buggy gcc:
>
>
> void __attribute__((weak)) func(void)
> {
>     /* no code */
> }
>
> int main()
> {
>     func();
>     return 0;
> }
>   

I think the problem is that main() would have no call to func(), not 
that it wouldn't build.

Does making func noinline fix it?  I wonder if we should make __weak 
implicitly make the function noinline too.

    J

Re: huge gcc 4.1.{0,1} __weak problem

[Sven-Haegar Koch]

On Fri, 2 May 2008, Alistair John Strachan wrote:
> On Friday 02 May 2008 00:55:58 Chris Knadle wrote:
> > On Thu, 1 May 2008, Linus Torvalds wrote:
> > > On Thu, 1 May 2008, Andrew Morton wrote:
> > > > > I see only the following choices:
> > > > > - remove __weak and replace all current usages
> > > > > - move all __weak functions into own files, and ensure that also
> > > > > happens for future usages
> > > > > - #error for gcc 4.1.{0,1}
> > > >
> > > > Can we detect the {0,1}?  __GNUC_EVEN_MORE_MINOR__?
> > >
> > > It's __GNUC_PATCHLEVEL__, I believe.
> > >
> > > So yes, we can distinguish 4.1.2 (good, and very common) from 4.1.{0,1}
> > > (bad, and rather uncommon).
> > > And yes, considering that 4.1.1 (and even more so 4.1.0) should be rare
> > > to begin with, I think it's better to just not support it.
> > >
> > >			Linus
> >
> >    Unfortunately Debian Stable (i.e. Etch), which is relatively popular for
> > server use, is still using 4.1.1  :-(  (The current gcc package is
> > gcc-4.1.1-21)
> >
> >    I have not looked to see if Debian Stable's gcc-4.1.1-21 has been
> > patched for the currently discussed __weak bug.
> 
> I checked and it has been patched in 4.1.1-21. This would make checking for 
> 4.1.1 via __GNUC_PATCHLEVEL__ potentially invalid, as patched distro 
> compilers may (and in this case do) have this fixed.

Not a problem for the Debian Stable Version, as someone already wrote, it 
calls itself 4.1.2, even if the package version number is 4.1.1-xx

> gcc --version
gcc (GCC) 4.1.2 20061115 (prerelease) (Debian 4.1.1-21)
Copyright (C) 2006 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.


c'ya
sven

-- 

The Internet treats censorship as a routing problem, and routes around
it. (John Gilmore on http://www.cygnus.com/~gnu/)

[PATCH] /dev/mem gcc weak function workaround

[Venki Pallipadi]

Some flavors of gcc 4.1.0 and 4.1.1 seems to have trouble understanding
weak function definitions. Calls to function from the same file where it is
defined as weak _may_ get inlined, even when there is a non-weak definition of
the function elsewhere. I tried using attribute 'noinline' which does not
seem to help either.

One workaround for this is to have weak functions defined in different
file as below. Other possible way is to not use weak functions and go back
to ifdef logic.

There are few other usages in kernel that seem to depend on weak (and noinline)
working correctly, which can also potentially break and needs such workarounds.
Example -
mach_reboot_fixups() in arch/x86/kernel/reboot.c is one such call which
is getting inlined with a flavor of gcc 4.1.1.

Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>

---
 drivers/char/Makefile     |    2 +-
 drivers/char/mem.c        |   23 +----------------------
 drivers/char/mem_weak.c   |   25 +++++++++++++++++++++++++
 include/asm-x86/io.h      |    1 -
 include/asm-x86/pgtable.h |    2 --
 include/linux/mmap.h      |   15 +++++++++++++++
 6 files changed, 42 insertions(+), 26 deletions(-)

Index: linux-2.6/drivers/char/mem_weak.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6/drivers/char/mem_weak.c	2008-04-29 18:14:10.000000000 -0700
@@ -0,0 +1,25 @@
+
+#include <linux/mmap.h>
+
+void __attribute__((weak)) unxlate_dev_mem_ptr(unsigned long phys, void *addr)
+{
+}
+
+int __attribute__((weak)) phys_mem_access_prot_allowed(struct file *file,
+	unsigned long pfn, unsigned long size, pgprot_t *vma_prot)
+{
+	return 1;
+}
+
+void __attribute__((weak))
+map_devmem(unsigned long pfn, unsigned long len, pgprot_t prot)
+{
+	/* nothing. architectures can override. */
+}
+
+void __attribute__((weak))
+unmap_devmem(unsigned long pfn, unsigned long len, pgprot_t prot)
+{
+	/* nothing. architectures can override. */
+}
+
Index: linux-2.6/drivers/char/mem.c
===================================================================
--- linux-2.6.orig/drivers/char/mem.c	2008-04-29 18:12:14.000000000 -0700
+++ linux-2.6/drivers/char/mem.c	2008-04-29 18:14:43.000000000 -0700
@@ -26,6 +26,7 @@
 #include <linux/bootmem.h>
 #include <linux/splice.h>
 #include <linux/pfn.h>
+#include <linux/mmap.h>
 
 #include <asm/uaccess.h>
 #include <asm/io.h>
@@ -105,10 +106,6 @@ static inline int range_is_allowed(unsig
 }
 #endif
 
-void __attribute__((weak)) unxlate_dev_mem_ptr(unsigned long phys, void *addr)
-{
-}
-
 /*
  * This funcion reads the *physical* memory. The f_pos points directly to the 
  * memory location. 
@@ -254,12 +251,6 @@ static ssize_t write_mem(struct file * f
 	return written;
 }
 
-int __attribute__((weak)) phys_mem_access_prot_allowed(struct file *file,
-	unsigned long pfn, unsigned long size, pgprot_t *vma_prot)
-{
-	return 1;
-}
-
 #ifndef __HAVE_PHYS_MEM_ACCESS_PROT
 static pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
 				     unsigned long size, pgprot_t vma_prot)
@@ -300,18 +291,6 @@ static inline int private_mapping_ok(str
 }
 #endif
 
-void __attribute__((weak))
-map_devmem(unsigned long pfn, unsigned long len, pgprot_t prot)
-{
-	/* nothing. architectures can override. */
-}
-
-void __attribute__((weak))
-unmap_devmem(unsigned long pfn, unsigned long len, pgprot_t prot)
-{
-	/* nothing. architectures can override. */
-}
-
 static void mmap_mem_open(struct vm_area_struct *vma)
 {
 	map_devmem(vma->vm_pgoff,  vma->vm_end - vma->vm_start,
Index: linux-2.6/include/linux/mmap.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6/include/linux/mmap.h	2008-04-29 18:14:10.000000000 -0700
@@ -0,0 +1,15 @@
+#ifndef _LINUX_MMAP_H
+#define _LINUX_MMAP_H
+
+#include <linux/fs.h>
+#include <asm/page.h>
+
+extern void unxlate_dev_mem_ptr(unsigned long phys, void *addr);
+
+extern int phys_mem_access_prot_allowed(struct file *file,
+	unsigned long pfn, unsigned long size, pgprot_t *vma_prot);
+
+extern void map_devmem(unsigned long pfn, unsigned long len, pgprot_t prot);
+extern void unmap_devmem(unsigned long pfn, unsigned long len, pgprot_t prot);
+
+#endif /* _LINUX_MMAP_H */
Index: linux-2.6/include/asm-x86/io.h
===================================================================
--- linux-2.6.orig/include/asm-x86/io.h	2008-04-29 17:27:43.000000000 -0700
+++ linux-2.6/include/asm-x86/io.h	2008-04-29 18:14:10.000000000 -0700
@@ -10,7 +10,6 @@
 #endif
 
 extern void *xlate_dev_mem_ptr(unsigned long phys);
-extern void unxlate_dev_mem_ptr(unsigned long phys, void *addr);
 
 extern int ioremap_change_attr(unsigned long vaddr, unsigned long size,
 				unsigned long prot_val);
Index: linux-2.6/include/asm-x86/pgtable.h
===================================================================
--- linux-2.6.orig/include/asm-x86/pgtable.h	2008-04-29 17:27:43.000000000 -0700
+++ linux-2.6/include/asm-x86/pgtable.h	2008-04-29 18:14:10.000000000 -0700
@@ -303,8 +303,6 @@ static inline pte_t pte_modify(pte_t pte
 struct file;
 pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
                               unsigned long size, pgprot_t vma_prot);
-int phys_mem_access_prot_allowed(struct file *file, unsigned long pfn,
-                              unsigned long size, pgprot_t *vma_prot);
 #endif
 
 #ifdef CONFIG_PARAVIRT
Index: linux-2.6/drivers/char/Makefile
===================================================================
--- linux-2.6.orig/drivers/char/Makefile	2008-04-29 18:12:14.000000000 -0700
+++ linux-2.6/drivers/char/Makefile	2008-04-29 18:14:43.000000000 -0700
@@ -7,7 +7,7 @@
 #
 FONTMAPFILE = cp437.uni
 
-obj-y	 += mem.o random.o tty_io.o n_tty.o tty_ioctl.o
+obj-y	 += mem.o mem_weak.o random.o tty_io.o n_tty.o tty_ioctl.o
 
 obj-$(CONFIG_LEGACY_PTYS)	+= pty.o
 obj-$(CONFIG_UNIX98_PTYS)	+= pty.o

Re: [PATCH] /dev/mem gcc weak function workaround

[David Miller]

From: Venki Pallipadi <venkatesh.pallipadi@intel.com>
Date: Tue, 29 Apr 2008 18:31:09 -0700

> Some flavors of gcc 4.1.0 and 4.1.1 seems to have trouble understanding
> weak function definitions. Calls to function from the same file where it is
> defined as weak _may_ get inlined, even when there is a non-weak definition of
> the function elsewhere. I tried using attribute 'noinline' which does not
> seem to help either.
> 
> One workaround for this is to have weak functions defined in different
> file as below. Other possible way is to not use weak functions and go back
> to ifdef logic.
> 
> There are few other usages in kernel that seem to depend on weak (and noinline)
> working correctly, which can also potentially break and needs such workarounds.
> Example -
> mach_reboot_fixups() in arch/x86/kernel/reboot.c is one such call which
> is getting inlined with a flavor of gcc 4.1.1.
> 
> Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
> Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>

This sounds like a bug.  And if gcc does multi-file compilation it
can in theory do the same mistake even if you move it to another
file.

We need something more bulletproof here.

Also, we have a macro for marking things weak "__weak" which should
be used here.

RE: [PATCH] /dev/mem gcc weak function workaround

[Pallipadi, Venkatesh]

 

>-----Original Message-----
>From: David Miller [mailto:davem@davemloft.net] 
>Sent: Tuesday, April 29, 2008 9:29 PM
>To: Pallipadi, Venkatesh
>Cc: mingo@elte.hu; tglx@linutronix.de; hpa@zytor.com; 
>akpm@linux-foundation.org; linux-kernel@vger.kernel.org
>Subject: Re: [PATCH] /dev/mem gcc weak function workaround
>
>From: Venki Pallipadi <venkatesh.pallipadi@intel.com>
>Date: Tue, 29 Apr 2008 18:31:09 -0700
>
>> Some flavors of gcc 4.1.0 and 4.1.1 seems to have trouble 
>understanding
>> weak function definitions. Calls to function from the same 
>file where it is
>> defined as weak _may_ get inlined, even when there is a 
>non-weak definition of
>> the function elsewhere. I tried using attribute 'noinline' 
>which does not
>> seem to help either.
>> 
>> One workaround for this is to have weak functions defined in 
>different
>> file as below. Other possible way is to not use weak 
>functions and go back
>> to ifdef logic.
>> 
>> There are few other usages in kernel that seem to depend on 
>weak (and noinline)
>> working correctly, which can also potentially break and 
>needs such workarounds.
>> Example -
>> mach_reboot_fixups() in arch/x86/kernel/reboot.c is one such 
>call which
>> is getting inlined with a flavor of gcc 4.1.1.
>> 
>> Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
>> Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
>
>This sounds like a bug.  And if gcc does multi-file compilation it
>can in theory do the same mistake even if you move it to another
>file.
>
>We need something more bulletproof here.
>

The references here
http://gcc.gnu.org/ml/gcc-bugs/2006-05/msg02801.html
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27781

seem to suggest that the bug is only with weak definition in the same
file.
So, having them in a different file should be good enough workaround
here.

Tom: Comments?

>Also, we have a macro for marking things weak "__weak" which should
>be used here.

Yes. Will change that.

Thanks,
Venki

huge gcc 4.1.{0,1} __weak problem

[Adrian Bunk]

On Wed, Apr 30, 2008 at 05:49:46AM -0700, Pallipadi, Venkatesh wrote:
>  
> >-----Original Message----- From: David Miller 
> >From: Venki Pallipadi <venkatesh.pallipadi@intel.com> Date: Tue, 29 
> >Apr 2008 18:31:09 -0700
> >
> >> Some flavors of gcc 4.1.0 and 4.1.1 seems to have trouble 
> >understanding
> >> weak function definitions. Calls to function from the same 
> >file where it is
> >> defined as weak _may_ get inlined, even when there is a 
> >non-weak definition of
> >> the function elsewhere. I tried using attribute 'noinline' 
> >which does not
> >> seem to help either.
> >> 
> >> One workaround for this is to have weak functions defined in 
> >different
> >> file as below. Other possible way is to not use weak 
> >functions and go back
> >> to ifdef logic.
> >> 
> >> There are few other usages in kernel that seem to depend on 
> >weak (and noinline)
> >> working correctly, which can also potentially break and 
> >needs such workarounds.
> >> Example -
> >> mach_reboot_fixups() in arch/x86/kernel/reboot.c is one such 
> >call which
> >> is getting inlined with a flavor of gcc 4.1.1.
> >> 
> >> Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
> >> Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
> >
> >This sounds like a bug.  And if gcc does multi-file compilation it
> >can in theory do the same mistake even if you move it to another
> >file.
> >
> >We need something more bulletproof here.
> >
> 
> The references here
> http://gcc.gnu.org/ml/gcc-bugs/2006-05/msg02801.html
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27781
> 
> seem to suggest that the bug is only with weak definition in the same
> file.
> So, having them in a different file should be good enough workaround
> here.
>...

A workaround here is the wrong solution since this isn't the only place 
that suffers from this issue.

We currently give a #warning for 4.1.0.
But not for 4.1.1.
(Accordingto the bug >= 4.1.2 is fixed.)

And a #warning is not enough.

The huge problem is that "empty __weak function in the same file and 
non-weak arch function" has recently become a common pattern with 
several new usages added during this merge window alone.

And the breakages can be very subtle runtime breakages.

I see only the following choices:
- remove __weak and replace all current usages
- move all __weak functions into own files, and ensure that also happens
  for future usages
- #error for gcc 4.1.{0,1}

> Thanks,
> Venki

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

Re: huge gcc 4.1.{0,1} __weak problem

[Andrew Morton]

On Fri, 2 May 2008 00:56:33 +0300
Adrian Bunk <bunk@kernel.org> wrote:

> On Wed, Apr 30, 2008 at 05:49:46AM -0700, Pallipadi, Venkatesh wrote:
> >  
> > >-----Original Message----- From: David Miller 
> > >From: Venki Pallipadi <venkatesh.pallipadi@intel.com> Date: Tue, 29 
> > >Apr 2008 18:31:09 -0700
> > >
> > >> Some flavors of gcc 4.1.0 and 4.1.1 seems to have trouble 
> > >understanding
> > >> weak function definitions. Calls to function from the same 
> > >file where it is
> > >> defined as weak _may_ get inlined, even when there is a 
> > >non-weak definition of
> > >> the function elsewhere. I tried using attribute 'noinline' 
> > >which does not
> > >> seem to help either.
> > >> 
> > >> One workaround for this is to have weak functions defined in 
> > >different
> > >> file as below. Other possible way is to not use weak 
> > >functions and go back
> > >> to ifdef logic.
> > >> 
> > >> There are few other usages in kernel that seem to depend on 
> > >weak (and noinline)
> > >> working correctly, which can also potentially break and 
> > >needs such workarounds.
> > >> Example -
> > >> mach_reboot_fixups() in arch/x86/kernel/reboot.c is one such 
> > >call which
> > >> is getting inlined with a flavor of gcc 4.1.1.
> > >> 
> > >> Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
> > >> Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
> > >
> > >This sounds like a bug.  And if gcc does multi-file compilation it
> > >can in theory do the same mistake even if you move it to another
> > >file.
> > >
> > >We need something more bulletproof here.
> > >
> > 
> > The references here
> > http://gcc.gnu.org/ml/gcc-bugs/2006-05/msg02801.html
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27781
> > 
> > seem to suggest that the bug is only with weak definition in the same
> > file.
> > So, having them in a different file should be good enough workaround
> > here.
> >...
> 
> A workaround here is the wrong solution since this isn't the only place 
> that suffers from this issue.
> 
> We currently give a #warning for 4.1.0.
> But not for 4.1.1.
> (Accordingto the bug >= 4.1.2 is fixed.)
> 
> And a #warning is not enough.
> 
> The huge problem is that "empty __weak function in the same file and 
> non-weak arch function" has recently become a common pattern

Perhaps the commonest.  Certainly there will be more.

> with 
> several new usages added during this merge window alone.
> 
> And the breakages can be very subtle runtime breakages.
> 
> I see only the following choices:
> - remove __weak and replace all current usages
> - move all __weak functions into own files, and ensure that also happens
>   for future usages
> - #error for gcc 4.1.{0,1}

Can we detect the {0,1}?  __GNUC_EVEN_MORE_MINOR__?

Yes, I guess we should ban 4.1.x.  Ouch.

Re: huge gcc 4.1.{0,1} __weak problem

[Linus Torvalds]

On Thu, 1 May 2008, Andrew Morton wrote:
> > 
> > I see only the following choices:
> > - remove __weak and replace all current usages
> > - move all __weak functions into own files, and ensure that also happens
> >   for future usages
> > - #error for gcc 4.1.{0,1}
> 
> Can we detect the {0,1}?  __GNUC_EVEN_MORE_MINOR__?

It's __GNUC_PATCHLEVEL__, I believe.

So yes, we can distinguish 4.1.2 (good, and very common) from 4.1.{0,1} 
(bad, and rather uncommon).

And yes, considering that 4.1.1 (and even more so 4.1.0) should be rare to 
begin with, I think it's better to just not support it.

			Linus

Re: huge gcc 4.1.{0,1} __weak problem

[Andrew Morton]

On Thu, 1 May 2008 15:27:26 -0700 (PDT)
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 
> 
> On Thu, 1 May 2008, Andrew Morton wrote:
> > > 
> > > I see only the following choices:
> > > - remove __weak and replace all current usages
> > > - move all __weak functions into own files, and ensure that also happens
> > >   for future usages
> > > - #error for gcc 4.1.{0,1}
> > 
> > Can we detect the {0,1}?  __GNUC_EVEN_MORE_MINOR__?
> 
> It's __GNUC_PATCHLEVEL__, I believe.
> 
> So yes, we can distinguish 4.1.2 (good, and very common) from 4.1.{0,1} 
> (bad, and rather uncommon).
> 
> And yes, considering that 4.1.1 (and even more so 4.1.0) should be rare to 
> begin with, I think it's better to just not support it.
> 

Drat.  There go my alpha, i386, m68k, s390, sparc and powerpc
cross-compilers.  Vagard, save me!

Meanwhile I guess I can locally unpatch that patch.

Re: huge gcc 4.1.{0,1} __weak problem

[Tom Rini]

On Thu, May 01, 2008 at 03:33:49PM -0700, Andrew Morton wrote:
> On Thu, 1 May 2008 15:27:26 -0700 (PDT)
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > 
> > 
> > On Thu, 1 May 2008, Andrew Morton wrote:
> > > > 
> > > > I see only the following choices:
> > > > - remove __weak and replace all current usages
> > > > - move all __weak functions into own files, and ensure that also happens
> > > >   for future usages
> > > > - #error for gcc 4.1.{0,1}
> > > 
> > > Can we detect the {0,1}?  __GNUC_EVEN_MORE_MINOR__?
> > 
> > It's __GNUC_PATCHLEVEL__, I believe.
> > 
> > So yes, we can distinguish 4.1.2 (good, and very common) from 4.1.{0,1} 
> > (bad, and rather uncommon).
> > 
> > And yes, considering that 4.1.1 (and even more so 4.1.0) should be rare to 
> > begin with, I think it's better to just not support it.
> > 
> 
> Drat.  There go my alpha, i386, m68k, s390, sparc and powerpc
> cross-compilers.  Vagard, save me!
> 
> Meanwhile I guess I can locally unpatch that patch.

I know I'll come off as an ass, but you can't make new ones with 4.1.2?
It's not like we're talking about gcc 2.95/96 fun here :)

-- 
Tom Rini

Re: huge gcc 4.1.{0,1} __weak problem

[Andrew Morton]

On Thu, 1 May 2008 16:24:47 -0700
Tom Rini <trini@kernel.crashing.org> wrote:

> On Thu, May 01, 2008 at 03:33:49PM -0700, Andrew Morton wrote:
> > On Thu, 1 May 2008 15:27:26 -0700 (PDT)
> > Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > 
> > > 
> > > 
> > > On Thu, 1 May 2008, Andrew Morton wrote:
> > > > > 
> > > > > I see only the following choices:
> > > > > - remove __weak and replace all current usages
> > > > > - move all __weak functions into own files, and ensure that also happens
> > > > >   for future usages
> > > > > - #error for gcc 4.1.{0,1}
> > > > 
> > > > Can we detect the {0,1}?  __GNUC_EVEN_MORE_MINOR__?
> > > 
> > > It's __GNUC_PATCHLEVEL__, I believe.
> > > 
> > > So yes, we can distinguish 4.1.2 (good, and very common) from 4.1.{0,1} 
> > > (bad, and rather uncommon).
> > > 
> > > And yes, considering that 4.1.1 (and even more so 4.1.0) should be rare to 
> > > begin with, I think it's better to just not support it.
> > > 
> > 
> > Drat.  There go my alpha, i386, m68k, s390, sparc and powerpc
> > cross-compilers.  Vagard, save me!
> > 
> > Meanwhile I guess I can locally unpatch that patch.
> 
> I know I'll come off as an ass, but you can't make new ones with 4.1.2?
> It's not like we're talking about gcc 2.95/96 fun here :)

Honestly, I nearly died when I built all those cross-compilers.  Sooooooo
many combinations of gcc/binutils/glibc refused to work for obscure
reasons.  Compilation on x86_64 just didn't work at all and I ended up
having to build everything on a slow i386 box, etc, etc.  The stream of
email to Dan got increasingly strident ;)

I think crosstool has become a lot better since then, judging from the ease
with which Jens was able to spin up the powerpc compiler, but the trauma
was a life-long thing.

Vegard has been making noises about (finally!) preparing and maintaining a
decent set of cross-compilers for us.  It would be a great service (begs).

Re: huge gcc 4.1.{0,1} __weak problem

[Justin Mattock]

On Thu, May 1, 2008 at 11:59 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Thu, 1 May 2008 16:24:47 -0700
>  Tom Rini <trini@kernel.crashing.org> wrote:
>
>  > On Thu, May 01, 2008 at 03:33:49PM -0700, Andrew Morton wrote:
>  > > On Thu, 1 May 2008 15:27:26 -0700 (PDT)
>  > > Linus Torvalds <torvalds@linux-foundation.org> wrote:
>  > >
>  > > >
>  > > >
>  > > > On Thu, 1 May 2008, Andrew Morton wrote:
>  > > > > >
>  > > > > > I see only the following choices:
>  > > > > > - remove __weak and replace all current usages
>  > > > > > - move all __weak functions into own files, and ensure that also happens
>  > > > > >   for future usages
>  > > > > > - #error for gcc 4.1.{0,1}
>  > > > >
>  > > > > Can we detect the {0,1}?  __GNUC_EVEN_MORE_MINOR__?
>  > > >
>  > > > It's __GNUC_PATCHLEVEL__, I believe.
>  > > >
>  > > > So yes, we can distinguish 4.1.2 (good, and very common) from 4.1.{0,1}
>  > > > (bad, and rather uncommon).
>  > > >
>  > > > And yes, considering that 4.1.1 (and even more so 4.1.0) should be rare to
>  > > > begin with, I think it's better to just not support it.
>  > > >
>  > >
>  > > Drat.  There go my alpha, i386, m68k, s390, sparc and powerpc
>  > > cross-compilers.  Vagard, save me!
>  > >
>  > > Meanwhile I guess I can locally unpatch that patch.
>  >
>  > I know I'll come off as an ass, but you can't make new ones with 4.1.2?
>  > It's not like we're talking about gcc 2.95/96 fun here :)
>
>  Honestly, I nearly died when I built all those cross-compilers.  Sooooooo
>  many combinations of gcc/binutils/glibc refused to work for obscure
>  reasons.  Compilation on x86_64 just didn't work at all and I ended up
>  having to build everything on a slow i386 box, etc, etc.  The stream of
>  email to Dan got increasingly strident ;)
>
>  I think crosstool has become a lot better since then, judging from the ease
>  with which Jens was able to spin up the powerpc compiler, but the trauma
>  was a life-long thing.
>
>  Vegard has been making noises about (finally!) preparing and maintaining a
>  decent set of cross-compilers for us.  It would be a great service (begs).
>
>  --
>  To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>  the body of a message to majordomo@vger.kernel.org
>  More majordomo info at  http://vger.kernel.org/majordomo-info.html
>  Please read the FAQ at  http://www.tux.org/lkml/
>

I hate to jump in like this, but I noticed something while compiling
madwifi as well with
gcc, I'm wondering if this is related to the above:
/home/name/Desktop/madwifi-trunk-r3574-20080426/Makefile:50
Makefile.inc: no such file or directory
even though the files exists,


Makefile.inc98: scripts/get_arch.mk: no such file or directory
Makefile.inc:102 ath_hal/ah_target.inc: no such file or directory
Makefile.inc:163 *** TARGET i386-elf is invalid, invalid target are: . Stop.
make[1] *** [modules] Error 2

After modifying the  Makefiles you're left with ***TARGET i386-elf problem

regards;

-- 
Justin P. Mattock

Re: huge gcc 4.1.{0,1} __weak problem

[Vegard Nossum]

Hey :)

On Fri, May 2, 2008 at 1:59 AM, Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Thu, 1 May 2008 16:24:47 -0700
>  Tom Rini <trini@kernel.crashing.org> wrote:
>
>  > On Thu, May 01, 2008 at 03:33:49PM -0700, Andrew Morton wrote:
>  > > On Thu, 1 May 2008 15:27:26 -0700 (PDT)
>  > > Linus Torvalds <torvalds@linux-foundation.org> wrote:
>  > >
>  > > > On Thu, 1 May 2008, Andrew Morton wrote:
>  > > > > >
>  > > > > > I see only the following choices:
>  > > > > > - remove __weak and replace all current usages
>  > > > > > - move all __weak functions into own files, and ensure that also happens
>  > > > > >   for future usages
>  > > > > > - #error for gcc 4.1.{0,1}
>  > > > >
>  > > > > Can we detect the {0,1}?  __GNUC_EVEN_MORE_MINOR__?
>  > > >
>  > > > It's __GNUC_PATCHLEVEL__, I believe.
>  > > >
>  > > > So yes, we can distinguish 4.1.2 (good, and very common) from 4.1.{0,1}
>  > > > (bad, and rather uncommon).
>  > > >
>  > > > And yes, considering that 4.1.1 (and even more so 4.1.0) should be rare to
>  > > > begin with, I think it's better to just not support it.
>  > > >
>  > >
>  > > Drat.  There go my alpha, i386, m68k, s390, sparc and powerpc
>  > > cross-compilers.  Vagard, save me!
>  > >
>  > > Meanwhile I guess I can locally unpatch that patch.
>  >
>  > I know I'll come off as an ass, but you can't make new ones with 4.1.2?
>  > It's not like we're talking about gcc 2.95/96 fun here :)

It's true; crosstool hasn't been much of a trouble (for me anyway). A
couple of combinations that I tried failed.

>  Honestly, I nearly died when I built all those cross-compilers.  Sooooooo
>  many combinations of gcc/binutils/glibc refused to work for obscure
>  reasons.  Compilation on x86_64 just didn't work at all and I ended up
>  having to build everything on a slow i386 box, etc, etc.  The stream of
>  email to Dan got increasingly strident ;)
>
>  I think crosstool has become a lot better since then, judging from the ease
>  with which Jens was able to spin up the powerpc compiler, but the trauma
>  was a life-long thing.
>
>  Vegard has been making noises about (finally!) preparing and maintaining a
>  decent set of cross-compilers for us.  It would be a great service (begs).

Noises? Heh...

So I've only just begun that work. For i386 host we have the following targets:
alpha, arm, ia64, sparc64, sparc, x86_64

though these are largely untested with Linux Kernel. Most of them are
gcc 3.4.5 (this should in theory compile the kernel correctly), though
I found an e-mail from Ingo saying that you need the -tls versions for
stackprotector to work correctly. This might be a good time to ask if
I should be making gcc 4.1.2s instead. I need to recompile in either
case.

In other words, we are NOT fully operational yet :-)

Though H. Peter gave me a directory to stuff it in, you could try it
out if you're impatient:

http://www.kernel.org/pub/tools/crosstool/

(Hm, it seems that all my .asc files disappeared in the transition.. Oh well.)


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: huge gcc 4.1.{0,1} __weak problem

[Theodore Tso]

On Fri, May 02, 2008 at 09:18:10AM +0200, Vegard Nossum wrote:
> though these are largely untested with Linux Kernel. Most of them are
> gcc 3.4.5 (this should in theory compile the kernel correctly), though
> I found an e-mail from Ingo saying that you need the -tls versions for
> stackprotector to work correctly. This might be a good time to ask if
> I should be making gcc 4.1.2s instead. I need to recompile in either
> case.

Out of curiosity, are you using the stock gcc/binutils from the FSF,
or one of the distro toolchains?  When we were investigating which
compiler/toolchain to use for the LSB Sample Implementation, one of
the comments that I got from folks like Eric Troan from rPath and
others was that out-of-the-box compiler/toolchain had so many bugs,
particularly on non-x86 architectures, that they found they were much
better off using a distro-patched/bug-fixed compiler/toolchain and
treating that as the upstream.

There has been some requests to include cross-compilation
functionality into the LSB Build Environment, so I'd be interested in
seeing how your work has been going, and maybe there's some
opportunity to work together.  Is there a mailing list you have for
this project?

						- Ted

Re: huge gcc 4.1.{0,1} __weak problem

[Adrian Bunk]

On Thu, May 01, 2008 at 04:59:45PM -0700, Andrew Morton wrote:
> On Thu, 1 May 2008 16:24:47 -0700
> Tom Rini <trini@kernel.crashing.org> wrote:
> > On Thu, May 01, 2008 at 03:33:49PM -0700, Andrew Morton wrote:
> > > 
> > > Drat.  There go my alpha, i386, m68k, s390, sparc and powerpc
> > > cross-compilers.  Vagard, save me!
> > > 
> > > Meanwhile I guess I can locally unpatch that patch.
> > 
> > I know I'll come off as an ass, but you can't make new ones with 4.1.2?
> > It's not like we're talking about gcc 2.95/96 fun here :)
> 
> Honestly, I nearly died when I built all those cross-compilers.  Sooooooo
> many combinations of gcc/binutils/glibc refused to work for obscure
> reasons.  Compilation on x86_64 just didn't work at all and I ended up
> having to build everything on a slow i386 box, etc, etc.  The stream of
> email to Dan got increasingly strident ;)
>...

I have my binutils 2.18.50.0.6 / gcc 4.3 based cross toolchains for an 
i386 host here (no glibc included since I'm using them only for kernel 
testcompiles).

If you are using an x86 machine I can rebuild them without -march=k8 
and give you a copy.

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

Re: huge gcc 4.1.{0,1} __weak problem

[Andi Kleen]

Andrew Morton <akpm@linux-foundation.org> writes:
>
> Honestly, I nearly died when I built all those cross-compilers.  Sooooooo
> many combinations of gcc/binutils/glibc refused to work for obscure
> reasons.  Compilation on x86_64 just didn't work at all and I ended up
> having to build everything on a slow i386 box, etc, etc.  The stream of
> email to Dan got increasingly strident ;)

This might be pointing out the obvious, but to just update the compiler
from 4.1.0 to 4.1.2 (or other) you don't need to recompile binutils and glibc.

-Andi

Re: huge gcc 4.1.{0,1} __weak problem

[Venki Pallipadi]

On Thu, May 01, 2008 at 03:27:26PM -0700, Linus Torvalds wrote:
> 
> 
> On Thu, 1 May 2008, Andrew Morton wrote:
> > > 
> > > I see only the following choices:
> > > - remove __weak and replace all current usages
> > > - move all __weak functions into own files, and ensure that also happens
> > >   for future usages
> > > - #error for gcc 4.1.{0,1}
> > 
> > Can we detect the {0,1}?  __GNUC_EVEN_MORE_MINOR__?
> 
> It's __GNUC_PATCHLEVEL__, I believe.
> 
> So yes, we can distinguish 4.1.2 (good, and very common) from 4.1.{0,1} 
> (bad, and rather uncommon).
> 
> And yes, considering that 4.1.1 (and even more so 4.1.0) should be rare to 
> begin with, I think it's better to just not support it.
> 

Not sure whether #error on gcc 4.1.{0.1} is the right thing as I found atleast
one distro gcc which says itself as 4.1.1, do not exhibit the problem as it
most likely has fix backported.

Putting all weak functions in one file is something Suresh and I considered
before sending this patch. But, looking at various users of __weak, that
single file did not look very appropriate.

Thanks,
Venki

Re: huge gcc 4.1.{0,1} __weak problem

[Andrew Morton]

On Thu, 1 May 2008 15:35:15 -0700
Venki Pallipadi <venkatesh.pallipadi@intel.com> wrote:

> On Thu, May 01, 2008 at 03:27:26PM -0700, Linus Torvalds wrote:
> > 
> > 
> > On Thu, 1 May 2008, Andrew Morton wrote:
> > > > 
> > > > I see only the following choices:
> > > > - remove __weak and replace all current usages
> > > > - move all __weak functions into own files, and ensure that also happens
> > > >   for future usages
> > > > - #error for gcc 4.1.{0,1}
> > > 
> > > Can we detect the {0,1}?  __GNUC_EVEN_MORE_MINOR__?
> > 
> > It's __GNUC_PATCHLEVEL__, I believe.
> > 
> > So yes, we can distinguish 4.1.2 (good, and very common) from 4.1.{0,1} 
> > (bad, and rather uncommon).
> > 
> > And yes, considering that 4.1.1 (and even more so 4.1.0) should be rare to 
> > begin with, I think it's better to just not support it.
> > 
> 
> Not sure whether #error on gcc 4.1.{0.1} is the right thing as I found atleast
> one distro gcc which says itself as 4.1.1, do not exhibit the problem as it
> most likely has fix backported.
> 
> Putting all weak functions in one file is something Suresh and I considered
> before sending this patch. But, looking at various users of __weak, that
> single file did not look very appropriate.
> 

Is there some vaguely maintainable workaround we can do?  If the problem
only affects completely-empty weak functions then we could put something in
them to make them non-empty?

#if __GNUC__ == ...
#define weak_function_filler for ( ; ; );
#else
#define weak_function_filler()
#endif

because __weak and attribute((weak)) are pretty easy to grep for. Dunno.

Re: huge gcc 4.1.{0,1} __weak problem

[Jakub Jelinek]

On Thu, May 01, 2008 at 03:42:38PM -0700, Andrew Morton wrote:
> Is there some vaguely maintainable workaround we can do?  If the problem
> only affects completely-empty weak functions then we could put something in
> them to make them non-empty?

for (;;); isn't enough, the function would be still considered const and by
4.1.0 and some 4.1.1 incorrectly optimized out, without regard to weak
attribute.
But e.g.
asm ("");
should be enough.

	Jakub

Re: huge gcc 4.1.{0,1} __weak problem

[Tom Rini]

On Thu, May 01, 2008 at 03:42:38PM -0700, Andrew Morton wrote:

[snip]
> Is there some vaguely maintainable workaround we can do?  If the problem
> only affects completely-empty weak functions then we could put something in
> them to make them non-empty?

My memory is a tiny bit hazy (it was a while ago), but no, it's not just
empty functions (again, I _think_ I hit it with a generic vs arch weak
function).

-- 
Tom Rini

Re: huge gcc 4.1.{0,1} __weak problem

[Venki Pallipadi]

On Thu, May 01, 2008 at 04:21:16PM -0700, Tom Rini wrote:
> On Thu, May 01, 2008 at 03:42:38PM -0700, Andrew Morton wrote:
> 
> [snip]
> > Is there some vaguely maintainable workaround we can do?  If the problem
> > only affects completely-empty weak functions then we could put something in
> > them to make them non-empty?
> 
> My memory is a tiny bit hazy (it was a while ago), but no, it's not just
> empty functions (again, I _think_ I hit it with a generic vs arch weak
> function).
> 

Other thing we observed was: this does not just depend on the __weak
function definition. It also depends on where the function is called from.

__weak function with single return statement, did not get inlined when called
from say

caller()
{
	function();
}

but got inlined when called as in

caller()
{
	for (;;) {
		function();
	}
}

Thanks,
Venki

Re: huge gcc 4.1.{0,1} __weak problem

[Linus Torvalds]

On Thu, 1 May 2008, Venki Pallipadi wrote:
> 
> __weak function with single return statement, did not get inlined when called
> from say

Is it always about inlining? If so, can't we add a __noinline__ to the 
declaration of __weak?

Something like this oneliner?

		Linus

---
 include/linux/compiler-gcc.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index 5c8351b..4061fc7 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -41,7 +41,7 @@
 
 #define __deprecated			__attribute__((deprecated))
 #define __packed			__attribute__((packed))
-#define __weak				__attribute__((weak))
+#define __weak				__attribute__((weak)) noinline
 #define __naked				__attribute__((naked))
 #define __noreturn			__attribute__((noreturn))
 

Re: huge gcc 4.1.{0,1} __weak problem

[Suresh Siddha]

On Thu, May 01, 2008 at 05:34:34PM -0700, Linus Torvalds wrote:
> Is it always about inlining? If so, can't we add a __noinline__ to the 
> declaration of __weak?

We tried that and it was still getting inlined.

thanks,
suresh

Re: huge gcc 4.1.{0,1} __weak problem

[Jeremy Fitzhardinge]

Suresh Siddha wrote:
> On Thu, May 01, 2008 at 05:34:34PM -0700, Linus Torvalds wrote:
>   
>> Is it always about inlining? If so, can't we add a __noinline__ to the 
>> declaration of __weak?
>>     
>
> We tried that and it was still getting inlined.

That's a pity.  I've worked around this bug with noinline before.

    J

Re: huge gcc 4.1.{0,1} __weak problem

[David Miller]

From: Jeremy Fitzhardinge <jeremy@goop.org>
Date: Fri, 02 May 2008 14:11:18 -0700

> Suresh Siddha wrote:
> > On Thu, May 01, 2008 at 05:34:34PM -0700, Linus Torvalds wrote:
> >   
> >> Is it always about inlining? If so, can't we add a __noinline__ to the 
> >> declaration of __weak?
> >>     
> >
> > We tried that and it was still getting inlined.
> 
> That's a pity.  I've worked around this bug with noinline before.

It's "constness", as Jakub mentioned, not inlineability, that triggers
this bug.

That's why his workaround of using an empty asm("") to the function is
an effective workaround, because the compiler can no longer internally
decide that the function is "const".

Re: huge gcc 4.1.{0,1} __weak problem

[Tom Rini]

On Thu, May 01, 2008 at 03:35:15PM -0700, Venki Pallipadi wrote:
> On Thu, May 01, 2008 at 03:27:26PM -0700, Linus Torvalds wrote:
[snip]
> > It's __GNUC_PATCHLEVEL__, I believe.
> > 
> > So yes, we can distinguish 4.1.2 (good, and very common) from 4.1.{0,1} 
> > (bad, and rather uncommon).
> > 
> > And yes, considering that 4.1.1 (and even more so 4.1.0) should be rare to 
> > begin with, I think it's better to just not support it.
> > 
> 
> Not sure whether #error on gcc 4.1.{0.1} is the right thing as I found atleast
> one distro gcc which says itself as 4.1.1, do not exhibit the problem as it
> most likely has fix backported.

Really?  At the time this was a very uncommon thing (hence the initial
it's not a bug, you just didn't use the right flags) comments.  I
suppose it's possible of course that some distro took a 4.1 snapshot and
called it 4.1.1.

> Putting all weak functions in one file is something Suresh and I considered
> before sending this patch. But, looking at various users of __weak, that
> single file did not look very appropriate.

Indeed.  I suspect that even if you go so far as to do a single patch
per "feature", it's gonna be a lotta stuff.

-- 
Tom Rini

Re: huge gcc 4.1.{0,1} __weak problem

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, 1 May 2008 15:27:26 -0700 (PDT)

> So yes, we can distinguish 4.1.2 (good, and very common) from 4.1.{0,1} 
> (bad, and rather uncommon).
> 
> And yes, considering that 4.1.1 (and even more so 4.1.0) should be rare to 
> begin with, I think it's better to just not support it.

This is my impression as well.

Re: huge gcc 4.1.{0,1} __weak problem

[Jeremy Fitzhardinge]

Adrian Bunk wrote:
> On Wed, Apr 30, 2008 at 05:49:46AM -0700, Pallipadi, Venkatesh wrote:
>   
>>  
>>     
>>> -----Original Message----- From: David Miller 
>>> From: Venki Pallipadi <venkatesh.pallipadi@intel.com> Date: Tue, 29 
>>> Apr 2008 18:31:09 -0700
>>>
>>>       
>>>> Some flavors of gcc 4.1.0 and 4.1.1 seems to have trouble 
>>>>         
>>> understanding
>>>       
>>>> weak function definitions. Calls to function from the same 
>>>>         
>>> file where it is
>>>       
>>>> defined as weak _may_ get inlined, even when there is a 
>>>>         
>>> non-weak definition of
>>>       
>>>> the function elsewhere. I tried using attribute 'noinline' 
>>>>         
>>> which does not
>>>       
>>>> seem to help either.
>>>>
>>>> One workaround for this is to have weak functions defined in 
>>>>         
>>> different
>>>       
>>>> file as below. Other possible way is to not use weak 
>>>>         
>>> functions and go back
>>>       
>>>> to ifdef logic.
>>>>
>>>> There are few other usages in kernel that seem to depend on 
>>>>         
>>> weak (and noinline)
>>>       
>>>> working correctly, which can also potentially break and 
>>>>         
>>> needs such workarounds.
>>>       
>>>> Example -
>>>> mach_reboot_fixups() in arch/x86/kernel/reboot.c is one such 
>>>>         
>>> call which
>>>       
>>>> is getting inlined with a flavor of gcc 4.1.1.
>>>>
>>>> Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
>>>> Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
>>>>         
>>> This sounds like a bug.  And if gcc does multi-file compilation it
>>> can in theory do the same mistake even if you move it to another
>>> file.
>>>
>>> We need something more bulletproof here.
>>>
>>>       
>> The references here
>> http://gcc.gnu.org/ml/gcc-bugs/2006-05/msg02801.html
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27781
>>
>> seem to suggest that the bug is only with weak definition in the same
>> file.
>> So, having them in a different file should be good enough workaround
>> here.
>> ...
>>     
>
> A workaround here is the wrong solution since this isn't the only place 
> that suffers from this issue.
>
> We currently give a #warning for 4.1.0.
> But not for 4.1.1.
> (Accordingto the bug >= 4.1.2 is fixed.)
>
> And a #warning is not enough.
>
> The huge problem is that "empty __weak function in the same file and 
> non-weak arch function" has recently become a common pattern with 
> several new usages added during this merge window alone.
>
> And the breakages can be very subtle runtime breakages.
>
> I see only the following choices:
> - remove __weak and replace all current usages
> - move all __weak functions into own files, and ensure that also happens
>   for future usages
> - #error for gcc 4.1.{0,1}
>   

- make __weak also include noinline.  I think that's sufficient (at 
least it was when I encountered a gcc bug with these symptoms last year 
or so).

    J

Re: huge gcc 4.1.{0,1} __weak problem

[Adrian Bunk]

On Fri, May 02, 2008 at 02:09:53PM -0700, Jeremy Fitzhardinge wrote:
> Adrian Bunk wrote:
>> A workaround here is the wrong solution since this isn't the only place 
>> that suffers from this issue.
>>
>> We currently give a #warning for 4.1.0.
>> But not for 4.1.1.
>> (Accordingto the bug >= 4.1.2 is fixed.)
>>
>> And a #warning is not enough.
>>
>> The huge problem is that "empty __weak function in the same file and  
>> non-weak arch function" has recently become a common pattern with  
>> several new usages added during this merge window alone.
>>
>> And the breakages can be very subtle runtime breakages.
>>
>> I see only the following choices:
>> - remove __weak and replace all current usages
>> - move all __weak functions into own files, and ensure that also happens
>>   for future usages
>> - #error for gcc 4.1.{0,1}
>>   
>
> - make __weak also include noinline.  I think that's sufficient (at  
> least it was when I encountered a gcc bug with these symptoms last year  
> or so).

I've tried it and it doesn't work.

>    J

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