parisc: fix module loading failure of large kernel modules (take 2)

[Adding rusty to CC]

include/linux/moduleloader.h int module_frob_arch_sections(Elf_Ehdr *hdr, · line 18

Quoted 3 lines · Helge Deller

include/linux/moduleloader.h +3 -0

+unsigned long module_additional_section_size(struct module *mod,
+					     unsigned int section);
+

I think this would be more palatable as

#ifdef HAVE_MODULE_SECTION_STUBS
unsigned long module_additional_section_size(struct module *mod,

	unsigned int section);
#else
static inline unsigned long module_additional_section_size(struct module *mod,
	unsigned int section)
{
	return 0;
}

#endif

and removing the conditional in kernel/module.c, possibly the symbol should be "arch_module_a..." just to make it clear to anyone reading.

Anyway, it's up to rusty.

Rusty, we'd like to get this patch in, so I can merge the dependent parisc-specific patch.

regards, Kyle

Quoted 57 lines

/* Allocator used for allocating struct module, core sections and init
sections. Returns NULL on failure. */
void *module_alloc(unsigned long size);

init/Kconfig +3 -0

index f763762..5383815 100644
@@ -908,6 +908,9 @@ config MODULE_SRCVERSION_ALL
 	  the version).  With this option, such a "srcversion" field
 	  will be created for all modules.  If unsure, say N.
 
+config HAVE_MODULE_SECTION_STUBS
+	bool
+
 config KMOD
 	def_bool y
 	help

kernel/module.c +8 -3

index 1f4cc00..4b86308 100644
@@ -1579,10 +1579,15 @@ static int simplify_symbols(Elf_Shdr *sechdrs,
 }
 
 /* Update size with this section: return offset. */
-static long get_offset(unsigned int *size, Elf_Shdr *sechdr)
+static long get_offset(struct module *mod, unsigned int *size,
+		Elf_Shdr *sechdr, unsigned int section)
 {
 	long ret;
 
+#ifdef CONFIG_HAVE_MODULE_SECTION_STUBS
+	/* allocate some memory for stubs in front of each section */
+	*size += module_additional_section_size(mod, section);
+#endif
 	ret = ALIGN(*size, sechdr->sh_addralign ?: 1);
 	*size = ret + sechdr->sh_size;
 	return ret;

@@ -1622,7 +1627,7 @@ static void layout_sections(struct module *mod,
 			    || strncmp(secstrings + s->sh_name,
 				       ".init", 5) == 0)
 				continue;
-			s->sh_entsize = get_offset(&mod->core_size, s);
+			s->sh_entsize = get_offset(mod, &mod->core_size, s, i);
 			DEBUGP("\t%s\n", secstrings + s->sh_name);
 		}
 		if (m == 0)

@@ -1640,7 +1645,7 @@ static void layout_sections(struct module *mod,
 			    || strncmp(secstrings + s->sh_name,
 				       ".init", 5) != 0)
 				continue;
-			s->sh_entsize = (get_offset(&mod->init_size, s)
+			s->sh_entsize = (get_offset(mod, &mod->init_size, s, i)
 					 | INIT_OFFSET_MASK);
 			DEBUGP("\t%s\n", secstrings + s->sh_name);
 		}

Ugh, I fail at typing, resending for rusty's benefit.

Quoted 26 lines · Kyle McMartin

[Adding rusty to CC]

include/linux/moduleloader.h int module_frob_arch_sections(Elf_Ehdr *hdr, · line 18

Quoted 3 lines · Helge Deller

include/linux/moduleloader.h +3 -0

+unsigned long module_additional_section_size(struct module *mod,
+					     unsigned int section);
+

I think this would be more palatable as

#ifdef HAVE_MODULE_SECTION_STUBS
unsigned long module_additional_section_size(struct module *mod,

	unsigned int section);
#else
static inline unsigned long module_additional_section_size(struct module *mod,
	unsigned int section)
{
	return 0;
}

#endif

and removing the conditional in kernel/module.c, possibly the symbol should be "arch_module_a..." just to make it clear to anyone reading.

Anyway, it's up to rusty.

Rusty, we'd like to get this patch in, so I can merge the dependent parisc-specific patch.

Quoted 4 lines · Kyle McMartin

Quoted 4 lines

Anyway, it's up to rusty.

Rusty, we'd like to get this patch in, so I can merge the dependent parisc-specific patch.

This is the second take of the patch series. Changes to previous version:
- new CONFIG_HAVE_MODULE_SECTION_STUBS config option
- put stub entries of a code section in front of the section

____________
The parisc port (esp. the 32bit kernel) currently lacks the ability to load large kernel modules like xfs or ipv6. This is a long outstanding bug and has already been reported a few times, e.g.:
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=350482,
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=401439,
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=508489

The symptom is like this:

# modprobe xfs
FATAL: Error inserting xfs
(/lib/modules/2.6.26-1-parisc/kernel/fs/xfs/xfs.ko): Invalid module
format

In dmesg:
module xfs relocation of symbol xfs_btree_read_bufs is out of range (0x3ffefffe in 17 bits)

The reason for the failure is, that the architecture only provides the R_PARISC_PCREL17F (for 32bit kernels) and R_PARISC_PCREL22F (for PA2.0 and 64bit kernels) relocations, which sometimes can't reach the target address of the stub entry if the kernel module is too large. Currently parisc (like other architectures) creates one big PLT section for all stubs at the beginning of the init and core sections.

The following two patches changes the parisc module loader to put stubs for the code sections in front of each section, so that the distance to the stubs more easily fits into the available 17/22 bits.

Quoted 30 lines · On Tuesday 30 December 2008 07:04:54 Helge Deller wrote:

This is the second take of the patch series. Changes to previous version:
- new CONFIG_HAVE_MODULE_SECTION_STUBS config option
- put stub entries of a code section in front of the section

____________
The parisc port (esp. the 32bit kernel) currently lacks the ability to load large kernel modules like xfs or ipv6. This is a long outstanding bug and has already been reported a few times, e.g.:
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=350482,
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=401439,
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=508489

The symptom is like this:

# modprobe xfs
FATAL: Error inserting xfs
(/lib/modules/2.6.26-1-parisc/kernel/fs/xfs/xfs.ko): Invalid module
format

In dmesg:
module xfs relocation of symbol xfs_btree_read_bufs is out of range (0x3ffefffe in 17 bits)

The reason for the failure is, that the architecture only provides the R_PARISC_PCREL17F (for 32bit kernels) and R_PARISC_PCREL22F (for PA2.0 and 64bit kernels) relocations, which sometimes can't reach the target address of the stub entry if the kernel module is too large. Currently parisc (like other architectures) creates one big PLT section for all stubs at the beginning of the init and core sections.

Quoted 3 lines

The following two patches changes the parisc module loader to put stubs for the code sections in front of each section, so that the distance to the stubs more easily fits into the available 17/22 bits.

So now any one section has to pass 17 bits to break? How close are you with the xfs module?

But it's kind of nasty, overloading sh_entsize further. Could we instead do something like add a arch_module_section_size() weak fn which you can overload?

We'd use that in get_offset() so our layout and size calculations were correct,

and use sh_size everywhere else.

BTW, although the comment states that arches can change section sizes in the module_frob_arch_sections() function:
/* Allow arches to frob section contents and sizes. */
it will break horrible if you do so.
What I found was, that if you change sh_size, at least the module references / dependency chain will break when running lsmod.

include/linux/moduleloader.h +4 -0

+/* Additional bytes needed by arch in front of individual sections */
+unsigned int arch_module_section_size(struct module *mod,
+				      unsigned int section);
+

Not quite what I had in mind... let me show you:
...
Otherwise I'd have called it "arch_module_extra_size()".

Quoted 3 lines · Rusty Russell

Not quite what I had in mind... let me show you:
...
Otherwise I'd have called it "arch_module_extra_size()".

This would mean that I can increase the section size in the arch-specific function by returning a bigger value than sh_size.
This would give me space at the end of the section, but not at the beginning (which is what I need), as sh_entsize (the offset into memory) will stay the same as before.

Quoted 9 lines · On Thursday 01 January 2009 00:43:56 Helge Deller wrote:

Quoted 3 lines · Rusty Russell

Not quite what I had in mind... let me show you:
...
Otherwise I'd have called it "arch_module_extra_size()".

This would mean that I can increase the section size in the arch-specific function by returning a bigger value than sh_size.
This would give me space at the end of the section, but not at the beginning (which is what I need), as sh_entsize (the offset into memory) will stay the same as before.

Ah, OK. I assumed that extra room at the end was sufficient.

So I've taken your previous patch and renamed the function to arch_mod_section_prepend().

If we decide not to kill those bad gcc versions,
I'll stick it in some other random file (lib/weak.c? Blech...)

BTW, this is very late for this merge window. I would have liked to see this a month ago :(

Subject: module: fix module loading failure of large kernel modules for parisc
Date: Wed, 31 Dec 2008 12:31:18 +0100
From: Helge Deller <[email protected]>

When creating the final layout of a kernel module in memory, allow the module loader to reserve some additional memory in front of a given section. This is currently only needed for the parisc port which needs to put the stub entries there to fulfill the 17/22bit PCREL relocations with large kernel modules like xfs.

Signed-off-by Helge Deller <[email protected]>

Signed-off-by Rusty Russell <[email protected]> (renamed fn)

include/linux/moduleloader.h +3 -0

@@ -12,6 +12,9 @@ int module_frob_arch_sections(Elf_Ehdr *
 			      Elf_Shdr *sechdrs,
 			      char *secstrings,
 			      struct module *mod);
+
+/* Additional bytes needed by arch in front of individual sections */
+unsigned int arch_mod_section_prepend(struct module *mod, unsigned int section);
 
 /* Allocator used for allocating struct module, core sections and init
    sections.  Returns NULL on failure. */

kernel/module.c +13 -3

@@ -1588,11 +1588,21 @@ static int simplify_symbols(Elf_Shdr *se
 	return ret;
 }
 
+/* Additional bytes needed by arch in front of individual sections */
+unsigned int __weak arch_mod_section_prepend(struct module *mod,
+					     unsigned int section)
+{
+	/* default implementation just returns zero */
+	return 0;
+}
+
 /* Update size with this section: return offset. */
-static long get_offset(unsigned int *size, Elf_Shdr *sechdr)
+static long get_offset(struct module *mod, unsigned int *size,
+		       Elf_Shdr *sechdr, unsigned int section)
 {
 	long ret;
 
+	*size += arch_mod_section_prepend(mod, section);
 	ret = ALIGN(*size, sechdr->sh_addralign ?: 1);
 	*size = ret + sechdr->sh_size;
 	return ret;

@@ -1632,7 +1642,7 @@ static void layout_sections(struct modul
 			    || strncmp(secstrings + s->sh_name,
 				       ".init", 5) == 0)
 				continue;
-			s->sh_entsize = get_offset(&mod->core_size, s);
+			s->sh_entsize = get_offset(mod, &mod->core_size, s, i);
 			DEBUGP("\t%s\n", secstrings + s->sh_name);
 		}
 		if (m == 0)

@@ -1650,7 +1660,7 @@ static void layout_sections(struct modul
 			    || strncmp(secstrings + s->sh_name,
 				       ".init", 5) != 0)
 				continue;
-			s->sh_entsize = (get_offset(&mod->init_size, s)
+			s->sh_entsize = (get_offset(mod, &mod->init_size, s, i)
 					 | INIT_OFFSET_MASK);
 			DEBUGP("\t%s\n", secstrings + s->sh_name);
 		}

[PATCH 1/2] module.c: fix module loading failure of large kernel modules

When creating the final layout of a kernel module in memory, allow the module loader to reserve some additional memory in front of a given section. This is currently only needed for the parisc port which needs to put the stub entries there to fulfill the 17/22bit PCREL relocations with large kernel modules like xfs.

Differences of this patch to previous versions:
- added weak funtion arch_module_section_size()

Some gcc versions will inline weak functions if they are in scope - even if there is a non-weak function somewhere else. So you MUST NOT have the weak definition in the same file (or indirectly called through some inline functions in a header file) as the call. Because if you do, then any user with the wrong version of gcc will get the weak function semantics, even if it was meant to be overridden by something else.

Does this mean lib/swiotlb.c is broken now?

It has eg:

void * __weak swiotlb_alloc_boot(size_t size, unsigned long nslabs)

and then

	void __init
	swiotlb_init_with_default_size(size_t default_size)
	{
...
		io_tlb_start = swiotlb_alloc_boot(bytes, io_tlb_nslabs);

later on in the same file.

(I just notice this because I saw the warning about swiotlb_alloc_boot() not being __init but calling __alloc_bootmem_low and so I looked at the code yesterday)

I guess I could make a sparse rule for this, but nobody seems to run or care about sparse anyway. Sad.

I guess I could make a sparse rule for this, but nobody seems to run or care about sparse anyway. Sad.

Quoted 2 lines

I guess I could make a sparse rule for this, but nobody seems to run or care about sparse anyway. Sad.

There is some increased janitorial effort recently. Try to grep for [Ss]+parse in the shortlog for the last couple of months.

Quoted 4 lines

(I just notice this because I saw the warning about swiotlb_alloc_boot() not being __init but calling __alloc_bootmem_low and so I looked at the code yesterday)

Lucky us. What's nasty about this is that most developers probably have updated versions of gcc, and are then surprised when some odd user has insane behavior that doesn't match the source code - because the compiler did something unexpected.

I guess I could make a sparse rule for this, but nobody seems to run or care about sparse anyway. Sad.

Adrian claimed that it was gcc-4.1.0 and 4.1.1 only. He proposed banning them: http://lkml.org/lkml/2008/8/5/444

Quoted 3 lines

Adrian claimed that it was gcc-4.1.0 and 4.1.1 only. He proposed banning them: http://lkml.org/lkml/2008/8/5/444

If it really is just those releases, then yes, considering the number of cases we apparently have, and considering how ugly it is in some cases to move the weak function anywhere else, maybe banning those versions is the proper thing to do.

It probably won't hurt very many people - yeah, some people will be forced to upgrade, but I have this memory of early 4.1 having had other bugs anyway, so it's probably a good idea.

From: Linus Torvalds <[email protected]>
Date: Wed, 31 Dec 2008 13:22:53 -0800 (PST)

Quoted 10 lines · Andrew Morton

Quoted 3 lines

Adrian claimed that it was gcc-4.1.0 and 4.1.1 only. He proposed banning them: http://lkml.org/lkml/2008/8/5/444

If it really is just those releases, then yes, considering the number of cases we apparently have, and considering how ugly it is in some cases to move the weak function anywhere else, maybe banning those versions is the proper thing to do.

Quoted 3 lines

It probably won't hurt very many people - yeah, some people will be forced to upgrade, but I have this memory of early 4.1 having had other bugs anyway, so it's probably a good idea.

I think this is probably the best way to handle this.

From e6346e5ab54dcf12888a79dfd5402f5de09b3fad Mon Sep 17 00:00:00 2001

Quoted 17 lines · David Miller

From: Linus Torvalds <[email protected]>
Date: Wed, 31 Dec 2008 13:22:53 -0800 (PST)

Quoted 13 lines · Andrew Morton

Quoted 3 lines

Adrian claimed that it was gcc-4.1.0 and 4.1.1 only. He proposed banning them: http://lkml.org/lkml/2008/8/5/444

If it really is just those releases, then yes, considering the number of cases we apparently have, and considering how ugly it is in some cases to move the weak function anywhere else, maybe banning those versions is the proper thing to do.

It probably won't hurt very many people - yeah, some people will be forced to upgrade, but I have this memory of early 4.1 having had other bugs anyway, so it's probably a good idea.

Quoted 1 line

I think this is probably the best way to handle this.

okay - to move this matter from the discussion-space to the solution-space, how about the patch below? (tested on x86 with a non-affected compiler version.)

Ingo

-------------->
From e6346e5ab54dcf12888a79dfd5402f5de09b3fad Mon Sep 17 00:00:00 2001
From: Ingo Molnar <[email protected]>
Date: Fri, 2 Jan 2009 12:46:22 +0100
Subject: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

Impact: fix crashes that can trigger if built with GCC 4.1.0 or 4.1.1

GCC 4.1.0 and 4.1.1 has a bug that can miscompile __weak symbols, by inlining __weak functions into same-file call sites - breaking the kernel if the __weak symbol is overriden later on.

In the past we tried to work around this problem by artificially isolating call site and definition site - but these bugs tend to pop up regularly:

43a2563: sparseirq: move __weak symbols into separate compilation unit 13a0c3c: sparseirq: work around compiler optimizing away __weak functions

And Linus has extended Sparse to report same-file callsites for __weak symbols - which gave two dozen hits.

We have not found a clean way to work around this bug on the source code level (noinline and explicit barrier()s are ignored by GCC), so we do not allow these compilers (which are quite rare these days, have other bugs and are superceded by the 4.1.2 bugfix release anyway).

Kernel builds under gcc 410 or 411 will now fail with this error message:

Sorry, your compiler is too old, too buggy or not recognized.

Signed-off-by Ingo Molnar <[email protected]>

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

include/linux/compiler.h +15 -2

index ea7c6be..dd558ce 100644
@@ -36,12 +36,25 @@ extern void __chk_io_ptr(const volatile void __iomem *);
 
 #ifdef __KERNEL__
 
-#if __GNUC__ >= 4
+/*
+ * GCC 4.1.0 and 4.1.1 has a bug that can miscompile __weak symbols,
+ * by inlining __weak functions into same-file call sites - breaking the
+ * kernel if the __weak symbol is overriden later on.
+ *
+ * We have not found a clean way to work around this bug on the source
+ * code level, so we do not allow these compilers (which are quite
+ * rare these days, have other bugs and are superceded by the 4.1.2
+ * bugfix release anyway):
+ */
+#define gcc41_inlining_bug \
+	(__GNUC__ == 4 && __GNUC_MINOR__ == 1 && __GNUC_PATCHLEVEL__ <= 1)
+
+#if __GNUC__ >= 4 && !gcc41_inlining_bug
 # include <linux/compiler-gcc4.h>
 #elif __GNUC__ == 3 && __GNUC_MINOR__ >= 2
 # include <linux/compiler-gcc3.h>
 #else
-# error Sorry, your compiler is too old/not recognized.
+# error Sorry, your compiler is too old, too buggy or not recognized.
 #endif
 
 #define notrace __attribute__((no_instrument_function))

Quoted 20 lines · Ingo Molnar

Quoted 17 lines · David Miller

From: Linus Torvalds <[email protected]>
Date: Wed, 31 Dec 2008 13:22:53 -0800 (PST)

Quoted 13 lines · Andrew Morton

Quoted 3 lines

Adrian claimed that it was gcc-4.1.0 and 4.1.1 only. He proposed banning them: http://lkml.org/lkml/2008/8/5/444

If it really is just those releases, then yes, considering the number of cases we apparently have, and considering how ugly it is in some cases to move the weak function anywhere else, maybe banning those versions is the proper thing to do.

It probably won't hurt very many people - yeah, some people will be forced to upgrade, but I have this memory of early 4.1 having had other bugs anyway, so it's probably a good idea.

Quoted 5 lines

Quoted 1 line

I think this is probably the best way to handle this.

okay - to move this matter from the discussion-space to the solution-space, how about the patch below? (tested on x86 with a non-affected compiler version.)

...or we can just merge Adrian's patch from June 2008 which also fixes the issue nicely.

OTOH your patch has an advantage of addressing the problem in the more appropriate place (include/linux/compiler.h) and from what I see allows us to remove previous gcc 4.1.0 check from init/main.c?

From d23cbaaa342e5555a919a543095d656415a55950 Mon Sep 17 00:00:00 2001

From d23cbaaa342e5555a919a543095d656415a55950 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <[email protected]>
Date: Fri, 2 Jan 2009 16:16:16 +0100
Subject: [PATCH] kbuild: Remove gcc 4.1.0 quirk from init/main.c

Impact: cleanup

We now have a cleaner check for gcc 4.1.0/4.1.1 trouble in include/linux/compiler.h, so remove the 4.1.0 quirk from init/main.c.

Reported-by Bartlomiej Zolnierkiewicz <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

---
 init/main.c |    9 ---------
 1 files changed, 0 insertions(+), 9 deletions(-)

init/main.c +0 -9

index 2a7ce0f..5ced153 100644
@@ -75,15 +75,6 @@
 #include <asm/smp.h>
 #endif
 
-/*
- * This is one of the first .c files built. Error out early if we have compiler
- * trouble.
- */
-
-#if __GNUC__ == 4 && __GNUC_MINOR__ == 1 && __GNUC_PATCHLEVEL__ == 0
-#warning gcc-4.1.0 is known to miscompile the kernel.  A different compiler version is recommended.
-#endif
-
 static int kernel_init(void *);
 
 extern void init_IRQ(void);

include/linux/compiler.h +5 -1

@@ -36,12 +36,25 @@ extern void __chk_io_ptr(const volatile void __iomem *);
 
 #ifdef __KERNEL__
 
-#if __GNUC__ >= 4
+/*
+ * GCC 4.1.0 and 4.1.1 has a bug that can miscompile __weak symbols,
+ * by inlining __weak functions into same-file call sites - breaking the
+ * kernel if the __weak symbol is overriden later on.
+ *

I think this is wrong.

Just move the check into <linux/compiler-gcc4.h>

Quoted 4 lines · Linus Torvalds

I think this is wrong.

Just move the check into <linux/compiler-gcc4.h>

In fact, looking at that whole mess, I redid it all. It was disgusting how conditionals in gcc4.h needed to double-check that __GNUC__ really was 4 (rather than something bigger), which largely negated the whole nice clean compiler version separation.

I pushed out my preferred version, which fixes up the whole thing. The gcc4 header file now only gets included for __GNUC__ == 4, and when we ever see a __GNUC__ of 5, it will automatically DTRT and try to include <linux/compiler-gcc5.h> instead of #4.

And then the check in gcc4.h for 4.1.0 and 4.1.1 is much simplified. The two patches I pushed out add functionality, but don't actually add any new lines (the first simplification patch removes more lines than it adds, and the second one that adds the __weak bug test adds as mahy lines as the cleanup removed).

And it all looks more logical too, imho.

Quoted 1 line

And it all looks more logical too, imho.

yeah, agreed, much cleaner.

The gcc3 check for gcc 3.0 and 3.1 can be pushed into gcc3 as well - not sure whether you fixed that. (i fixed it in the patch i just sent - but i didnt notice the gcc5 mess in gcc4.h that you fixed)

From 1458f25412a838968e845ec0bc1b18db70cba7cd Mon Sep 17 00:00:00 2001

The gcc3 check for gcc 3.0 and 3.1 can be pushed into gcc3 as well - not sure whether you fixed that. (i fixed it in the patch i just sent - but i didnt notice the gcc5 mess in gcc4.h that you fixed)

Quoted 4 lines · Ingo Molnar

The gcc3 check for gcc 3.0 and 3.1 can be pushed into gcc3 as well - not sure whether you fixed that. (i fixed it in the patch i just sent - but i didnt notice the gcc5 mess in gcc4.h that you fixed)

I just dropped it. Gcc 3.0 and 3.1 are so old that we simply don't care.

So what i tried to point out that your patch causes a regression here - we dont filter out gcc 3.0/3.1 out anymore.

What I tried to tell you was that we don't care - nobody can have it anyway. If somebody tried to compile the kernel with it, we've already refused for the last 2+ years. So there are no old users out there. And there certainly aren't any new ones.

Quoted 3 lines · Ingo Molnar

So what i tried to point out that your patch causes a regression here - we dont filter out gcc 3.0/3.1 out anymore.

What I tried to tell you was that we don't care - nobody can have it anyway. If somebody tried to compile the kernel with it, we've already refused for the last 2+ years. So there are no old users out there. And there certainly aren't any new ones.

Yes some people (including me) still have them and are happy that there is a check to prevent accidental builds with an inappropriate compiler.

Quoted 4 lines · Linus Torvalds

I think this is wrong.

Just move the check into <linux/compiler-gcc4.h>

In fact, looking at that whole mess, I redid it all. It was disgusting how conditionals in gcc4.h needed to double-check that __GNUC__ really was 4 (rather than something bigger), which largely negated the whole nice clean compiler version separation.

I pushed out my preferred version, which fixes up the whole thing. The gcc4 header file now only gets included for __GNUC__ == 4, and when we ever see a __GNUC__ of 5, it will automatically DTRT and try to include <linux/compiler-gcc5.h> instead of #4.

And then the check in gcc4.h for 4.1.0 and 4.1.1 is much simplified. The two patches I pushed out add functionality, but don't actually add any new lines (the first simplification patch removes more lines than it adds, and the second one that adds the __weak bug test adds as mahy lines as the cleanup removed).

And it all looks more logical too, imho.

Bugger....
Now I cannot do cross compile for: alpha, arm, m68k and sparc.

Not that I actually try to run these beasts but just being able to do cross compile has served me well.

Quoted 6 lines · Sam Ravnborg

Bugger....
Now I cannot do cross compile for: alpha, arm, m68k and sparc.

Not that I actually try to run these beasts but just being able to do cross compile has served me well.

We _could_ make a "CONFIG_COMPILE_ONLY" check, but wouldn't it be even nicer to make sure the cross-compiles are something that might actually be expected to work?

I realize that cross-tools tend to lag a bit - the pressure to maintain them tends to be much lower - but I was sure we had somebody who did a reasonable cross-compiler toolchain. Is gcc-4.1 really the most modern thing that is easily available?

Quoted 7 lines · Linus Torvalds

Quoted 5 lines · Sam Ravnborg

Bugger....
Now I cannot do cross compile for: alpha, arm, m68k and sparc.

Not that I actually try to run these beasts but just being able to do cross compile has served me well.

Quoted 8 lines

We _could_ make a "CONFIG_COMPILE_ONLY" check, but wouldn't it be even nicer to make sure the cross-compiles are something that might actually be expected to work?

I realize that cross-tools tend to lag a bit - the pressure to maintain them tends to be much lower - but I was sure we had somebody who did a reasonable cross-compiler toolchain. Is gcc-4.1 really the most modern thing that is easily available?

Asked google and it found following page:

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

Architecuter OK? gcc version
---------------------------------

alpha          No     gcc 4.0.2
arm            Yes    gcc 3.4.5
ia64           Yes    gcc 3.4.5
m68k           Yes    gcc 3.4.5
mips           Yes    gcc 3.4.5
sh4            Yes    gcc 3.4.5
sparc          Yes    gcc 3.4.5
sparc64        Yes    gcc 3.4.5
x86_64         Yes    gcc 3.4.5

So from this list of tool chains we can continue to do cross builds of all except alpha.
But the gcc version is getting ancient..

Why it shall be so hard to do cross build toolchains is above my imagination but then I also never looked at what it involes.

Added Vegard that maintain these pages.

Quoted 6 lines · Sam Ravnborg

Bugger....
Now I cannot do cross compile for: alpha, arm, m68k and sparc.

Not that I actually try to run these beasts but just being able to do cross compile has served me well.

We _could_ make a "CONFIG_COMPILE_ONLY" check, but wouldn't it be even nicer to make sure the cross-compiles are something that might actually be expected to work?

I realize that cross-tools tend to lag a bit - the pressure to maintain them tends to be much lower - but I was sure we had somebody who did a reasonable cross-compiler toolchain. Is gcc-4.1 really the most modern thing that is easily available?

FWIW, I'm using 4.3 on all targets at the moment. See
git://git.kernel.org/pub/scm/linux/kernel/git/viro/toolchain.git/
for fedora-based variant of that sucker. And yes, it does include cross-to-sparc; all but sh/sh64, in fact (sh had serious compiler breakage around 4.3.0 and backporting from -HEAD was far beyond what I considered reasonable at that point).

Quoted 7 lines · Al Viro

FWIW, I'm using 4.3 on all targets at the moment. See
git://git.kernel.org/pub/scm/linux/kernel/git/viro/toolchain.git/
for fedora-based variant of that sucker. And yes, it does include cross-to-sparc; all but sh/sh64, in fact (sh had serious compiler breakage around 4.3.0 and backporting from -HEAD was far beyond what I considered reasonable at that point).

The full opensuse distribution also has a couple of spec files for generating cross compilers from recent versions. These are for icecream, but can be relatively easily adapted (or the binaries reused)

In general I don't think building cross compilers is as hard as it used to be, so it can be reasonably done without any support scripts too.

I realize that cross-tools tend to lag a bit - the pressure to maintain them tends to be much lower - but I was sure we had somebody who did a reasonable cross-compiler toolchain. Is gcc-4.1 really the most modern thing that is easily available?

Quoted 1 line

And it all looks more logical too, imho.

Bugger....
Now I cannot do cross compile for: alpha, arm, m68k and sparc.

Quoted 4 lines · Sam Ravnborg

Quoted 1 line

And it all looks more logical too, imho.

Bugger....
Now I cannot do cross compile for: alpha, arm, m68k and sparc.

hm, i just did a successful cross-build from x86 to alpha:

  KSYM    .tmp_kallsyms2.S
  AS      .tmp_kallsyms2.o
  LD      vmlinux
  SYSMAP  System.map
  SYSMAP  .tmp_System.map

phoenix:~/linux/linux> head -6 /dev/shm/tip/build/.config #
# Automatically generated make config: don't edit # Linux kernel version: 2.6.28
# Fri Jan 2 19:28:14 2009
#
CONFIG_ALPHA=y

with these commits present:

f9d1425: Disallow gcc versions 4.1.{0,1}
f153b82: Sanitize gcc version header includes

what type of cross-build breakage do they cause?

Quoted 24 lines · Ingo Molnar

Quoted 4 lines · Sam Ravnborg

Quoted 1 line

And it all looks more logical too, imho.

Bugger....
Now I cannot do cross compile for: alpha, arm, m68k and sparc.

hm, i just did a successful cross-build from x86 to alpha:

  KSYM    .tmp_kallsyms2.S
  AS      .tmp_kallsyms2.o
  LD      vmlinux
  SYSMAP  System.map
  SYSMAP  .tmp_System.map

phoenix:~/linux/linux> head -6 /dev/shm/tip/build/.config #
# Automatically generated make config: don't edit # Linux kernel version: 2.6.28
# Fri Jan 2 19:28:14 2009
#
CONFIG_ALPHA=y

Quoted 6 lines

with these commits present:

f9d1425: Disallow gcc versions 4.1.{0,1}
f153b82: Sanitize gcc version header includes

what type of cross-build breakage do they cause?

The default gcc made by demo-alpha with crosstool is version 4.1.0:

/opt/crosstool/gcc-4.1.0-glibc-2.3.6/alpha-unknown-linux-gnu/bin/alpha-unknown-linux-gnu-gcc --version
alpha-unknown-linux-gnu-gcc (GCC) 4.1.0
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.

Quoted 9 lines · Sam Ravnborg

Quoted 1 line

what type of cross-build breakage do they cause?

The default gcc made by demo-alpha with crosstool is version 4.1.0:

/opt/crosstool/gcc-4.1.0-glibc-2.3.6/alpha-unknown-linux-gnu/bin/alpha-unknown-linux-gnu-gcc --version
alpha-unknown-linux-gnu-gcc (GCC) 4.1.0
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.

ah, ok. I build cross-compilers from scratch, so i have 4.3.3-pre:

phoenix:~/linux/linux> /opt/crossgcc/cross/bin/alpha-linux-gcc -v [...]
gcc version 4.3.3 20081123 (prerelease) (GCC)

Ingo

Bugger....
Now I cannot do cross compile for: alpha, arm, m68k and sparc.

Not that I actually try to run these beasts but just being able to do cross compile has served me well.

Last I looked (only few days ago) crosstool-ng did not support sparc :-(

Quoted 5 lines

Bugger....
Now I cannot do cross compile for: alpha, arm, m68k and sparc.

Quoted 4 lines

Not that I actually try to run these beasts but just being able to do cross compile has served me well.

Last I looked (only few days ago) crosstool-ng did not support sparc :-(

There are also various variants of 4.1.1 around with the fix for that inline bug backported (I think that's the case of the compiler that ships with the Cell SDK for example).

Quoted 12 lines · Ingo Molnar

include/linux/compiler.h +5 -1

@@ -36,12 +36,25 @@ extern void __chk_io_ptr(const volatile void __iomem *);
 
 #ifdef __KERNEL__
 
-#if __GNUC__ >= 4
+/*
+ * GCC 4.1.0 and 4.1.1 has a bug that can miscompile __weak symbols,
+ * by inlining __weak functions into same-file call sites - breaking the
+ * kernel if the __weak symbol is overriden later on.
+ *

include/linux/compiler.h extern void __chk_io_ptr(const volatile void __iomem *); · line 53

Quoted 10 lines

include/linux/compiler.h +9 -0

+ * We have not found a clean way to work around this bug on the source
+ * code level, so we do not allow these compilers (which are quite
+ * rare these days, have other bugs and are superceded by the 4.1.2
+ * bugfix release anyway):
+ */
+#define gcc41_inlining_bug \
+	(__GNUC__ == 4 && __GNUC_MINOR__ == 1 && __GNUC_PATCHLEVEL__ <= 1)
+
+#if __GNUC__ >= 4 && !gcc41_inlining_bug
 # include <linux/compiler-gcc4.h>

I think this is wrong.

Just move the check into <linux/compiler-gcc4.h>

It makes no sense to do stuff that is specific to gcc4 in the general gcc header file. It seems you did this just in order to re-use a (bad) generic error case.

From bb951ce0794f0e5974b834eb14e974a0a2c119db Mon Sep 17 00:00:00 2001

Quoted 3 lines · Andrew Morton

Adrian claimed that it was gcc-4.1.0 and 4.1.1 only. He proposed banning them: http://lkml.org/lkml/2008/8/5/444

If it really is just those releases, then yes, considering the number of cases we apparently have, and considering how ugly it is in some cases to move the weak function anywhere else, maybe banning those versions is the proper thing to do.

It probably won't hurt very many people - yeah, some people will be forced to upgrade, but I have this memory of early 4.1 having had other bugs anyway, so it's probably a good idea.

Quoted 12 lines · Linus Torvalds

Quoted 3 lines · Andrew Morton

Adrian claimed that it was gcc-4.1.0 and 4.1.1 only. He proposed banning them: http://lkml.org/lkml/2008/8/5/444

If it really is just those releases, then yes, considering the number of cases we apparently have, and considering how ugly it is in some cases to move the weak function anywhere else, maybe banning those versions is the proper thing to do.

Quoted 3 lines

It probably won't hurt very many people - yeah, some people will be forced to upgrade, but I have this memory of early 4.1 having had other bugs anyway, so it's probably a good idea.

That would be _really_ nice to do IMHO

Quoted 10 lines · Helge Deller

[PATCH 1/2] module.c: fix module loading failure of large kernel modules

When creating the final layout of a kernel module in memory, allow the module loader to reserve some additional memory in front of a given section. This is currently only needed for the parisc port which needs to put the stub entries there to fulfill the 17/22bit PCREL relocations with large kernel modules like xfs.

Differences of this patch to previous versions:
- added weak funtion arch_module_section_size()

This doesn't work.

We've had this bug several times now, and one of them just very recently.

Some gcc versions will inline weak functions if they are in scope - even if there is a non-weak function somewhere else. So you MUST NOT have the weak definition in the same file (or indirectly called through some inline functions in a header file) as the call. Because if you do, then any user with the wrong version of gcc will get the weak function semantics, even if it was meant to be overridden by something else.

kernel/module.c static int simplify_symbols(Elf_Shdr *sechdrs, · line 1583

Quoted 3 lines

kernel/module.c +3 -0

+/* Additional bytes needed by arch in front of individual sections */
+unsigned int __attribute__ ((weak)) arch_module_section_size(
+		struct module *mod, unsigned int section)

We don't write out that whole "__attribute__" crud. We use

unsigned in __weak arch_module_section_size(struct module *mod, unsigned int section)

instead.

But as mentioned, it needs to be in another compilation unit.

Quoted 11 lines · Helge Deller

[PATCH 1/2] module.c: fix module loading failure of large kernel modules

When creating the final layout of a kernel module in memory, allow the module loader to reserve some additional memory in front of a given section. This is currently only needed for the parisc port which needs to put the stub entries there to fulfill the 17/22bit PCREL relocations with large kernel modules like xfs.

Differences of this patch to previous versions:
- added weak funtion arch_module_section_size()

This doesn't work.

We've had this bug several times now, and one of them just very recently.

Some gcc versions will inline weak functions if they are in scope - even if there is a non-weak function somewhere else. So you MUST NOT have the weak definition in the same file (or indirectly called through some inline functions in a header file) as the call. Because if you do, then any user with the wrong version of gcc will get the weak function semantics, even if it was meant to be overridden by something else.

Quoted 9 lines · Helge Deller

[PATCH 1/2] module.c: fix module loading failure of large kernel modules

When creating the final layout of a kernel module in memory, allow the module loader to reserve some additional memory in front of a given section. This is currently only needed for the parisc port which needs to put the stub entries there to fulfill the 17/22bit PCREL relocations with large kernel modules like xfs.

Quoted 2 lines

Differences of this patch to previous versions:
- added weak funtion arch_module_section_size()

This doesn't work.

We've had this bug several times now, and one of them just very recently.

Some gcc versions will inline weak functions if they are in scope

We don't write out that whole "__attribute__" crud.

Some gcc versions will inline weak functions if they are in scope - even if there is a non-weak function somewhere else. So you MUST NOT have the weak definition in the same file (or indirectly called through some inline functions in a header file) as the call. Because if you do, then any user with the wrong version of gcc will get the weak function semantics, even if it was meant to be overridden by something else.