section breakage on ppc64 (aka __devinitconst is broken by design)

section breakage on ppc64 (aka __devinitconst is broken by design)

[Al Viro]

; cat >a.c <<'EOF'
const char foo[] __attribute__ ((__section__(".blah"))) = "";
const char * const bar __attribute__((__section__(".blah"))) = "";
EOF
; gcc -m32 -S a.c
; gcc -m64 -S a.c
a.c:2: error: bar causes a section type conflict
;

That's 4.1.2 on ppc.  What happens is that the second declaration
wants to make .blah writable.  We actually trigger that in ppc64
builds on drivers/net/natsemi.c.

Note that on ppc64 without explicit sections you have the second one land in
.data.rel.ro.local, which is "aw",progbits.

The reason why it didn't visibly bite us before is that usually __devinit...
just expanded to nothing (unless you disable HOTPLUG, which requires
EMBEDDED, which wasn't apparently common enough for ppc64 builds).

Suggestions?

Re: section breakage on ppc64 (aka __devinitconst is broken by design)

[Sam Ravnborg]

On Sun, Feb 03, 2008 at 01:08:44PM +0000, Al Viro wrote:
> ; cat >a.c <<'EOF'
> const char foo[] __attribute__ ((__section__(".blah"))) = "";
> const char * const bar __attribute__((__section__(".blah"))) = "";
> EOF
> ; gcc -m32 -S a.c
> ; gcc -m64 -S a.c
> a.c:2: error: bar causes a section type conflict
> ;
> 
> That's 4.1.2 on ppc.  What happens is that the second declaration
> wants to make .blah writable.  We actually trigger that in ppc64
> builds on drivers/net/natsemi.c.
> 
> Note that on ppc64 without explicit sections you have the second one land in
> .data.rel.ro.local, which is "aw",progbits.
> 
> The reason why it didn't visibly bite us before is that usually __devinit...
> just expanded to nothing (unless you disable HOTPLUG, which requires
> EMBEDDED, which wasn't apparently common enough for ppc64 builds).
> 
> Suggestions?

Hi Al.

__devinitconst were invented to cover this issue.
So use __devinitconst for const data and 
__devinitdata for non-const data.

We recently had breakage in mainline with x86 64 bit
(sis190) for the exact same case.

Does this work in your ppc example or do we need
to find another solution?

	Sam

Re: section breakage on ppc64 (aka __devinitconst is broken by design)

[Al Viro]

On Sun, Feb 03, 2008 at 06:26:35PM +0100, Sam Ravnborg wrote:
> On Sun, Feb 03, 2008 at 01:08:44PM +0000, Al Viro wrote:
> > ; cat >a.c <<'EOF'
> > const char foo[] __attribute__ ((__section__(".blah"))) = "";
> > const char * const bar __attribute__((__section__(".blah"))) = "";
> > EOF
> > ; gcc -m32 -S a.c
> > ; gcc -m64 -S a.c
> > a.c:2: error: bar causes a section type conflict
> > ;
> > 
> > That's 4.1.2 on ppc.  What happens is that the second declaration
> > wants to make .blah writable.  We actually trigger that in ppc64
> > builds on drivers/net/natsemi.c.
> > 
> > Note that on ppc64 without explicit sections you have the second one land in
> > .data.rel.ro.local, which is "aw",progbits.
> > 
> > The reason why it didn't visibly bite us before is that usually __devinit...
> > just expanded to nothing (unless you disable HOTPLUG, which requires
> > EMBEDDED, which wasn't apparently common enough for ppc64 builds).
> > 
> > Suggestions?
> 
> Hi Al.
> 
> __devinitconst were invented to cover this issue.
> So use __devinitconst for const data and 
> __devinitdata for non-const data.

As the example above shows, what is and what is not const data is
irrelevant.  The data _is_ const.  On ppc32 gcc is happy to put
it into read-only section.  On ppc64 the same version of gcc insists
on making the section this data object is going to *writable*.

> We recently had breakage in mainline with x86 64 bit
> (sis190) for the exact same case.

No, this is not exact same case.  Unfortunately.

> Does this work in your ppc example or do we need
> to find another solution?

Please, read the posted example.  s/.blah/.devinit.rodata/ if you wish - it's
not magical.  What happens is that
	* gcc choice of r/o vs. r/w section is not determined by object
being const
	* that choice is actually platform-dependent, even between related
platforms (see ppc32 and ppc64 in the example above).

Re: section breakage on ppc64 (aka __devinitconst is broken by design)

[Sam Ravnborg]

On Sun, Feb 03, 2008 at 06:02:34PM +0000, Al Viro wrote:
> On Sun, Feb 03, 2008 at 06:26:35PM +0100, Sam Ravnborg wrote:
> > On Sun, Feb 03, 2008 at 01:08:44PM +0000, Al Viro wrote:
> > > ; cat >a.c <<'EOF'
> > > const char foo[] __attribute__ ((__section__(".blah"))) = "";
> > > const char * const bar __attribute__((__section__(".blah"))) = "";
> > > EOF
> > > ; gcc -m32 -S a.c
> > > ; gcc -m64 -S a.c
> > > a.c:2: error: bar causes a section type conflict
> > > ;
> > > 
> > > That's 4.1.2 on ppc.  What happens is that the second declaration
> > > wants to make .blah writable.  We actually trigger that in ppc64
> > > builds on drivers/net/natsemi.c.
> > > 
> > > Note that on ppc64 without explicit sections you have the second one land in
> > > .data.rel.ro.local, which is "aw",progbits.
> > > 
> > > The reason why it didn't visibly bite us before is that usually __devinit...
> > > just expanded to nothing (unless you disable HOTPLUG, which requires
> > > EMBEDDED, which wasn't apparently common enough for ppc64 builds).
> > > 
> > > Suggestions?
> > 
> > Hi Al.
> > 
> > __devinitconst were invented to cover this issue.
> > So use __devinitconst for const data and 
> > __devinitdata for non-const data.
> 
> As the example above shows, what is and what is not const data is
> irrelevant.  The data _is_ const.  On ppc32 gcc is happy to put
> it into read-only section.  On ppc64 the same version of gcc insists
> on making the section this data object is going to *writable*.
> 
> > We recently had breakage in mainline with x86 64 bit
> > (sis190) for the exact same case.
> 
> No, this is not exact same case.  Unfortunately.
> 
> > Does this work in your ppc example or do we need
> > to find another solution?
> 
> Please, read the posted example.  s/.blah/.devinit.rodata/ if you wish - it's
> not magical.  What happens is that
> 	* gcc choice of r/o vs. r/w section is not determined by object
> being const
> 	* that choice is actually platform-dependent, even between related
> platforms (see ppc32 and ppc64 in the example above).

Got it now - sorry.

And I'm suprised to see that gcc thinks bar is writeable.
If I try to assign it gcc error out as expected.

To get your example building I had to kill the const in front of foo:
char foo[] __attribute__ ((__section__(".blah"))) = "";
const char * const bar __attribute__((__section__(".blah"))) = "";

This is not an acceptable situation but for now I do not see a solution.

It is as such not the __devinitconst thing that causes us problems here.
It is the total concept of controlling the section of variables from
our C-code base.

We could invent a __initstr annotation but I dunno if that would suffice.
Do you see any pattern when gcc do the r/w choice compared to the
r/o choise. Maybe it is only const char[] that happens to be considered
r/o and the rest is r/w?

Should a gcc-bug be filed for this btw?

	Sam

Re: section breakage on ppc64 (aka __devinitconst is broken by design)

[Al Viro]

On Sun, Feb 03, 2008 at 09:30:02PM +0100, Sam Ravnborg wrote:
> And I'm suprised to see that gcc thinks bar is writeable.
> If I try to assign it gcc error out as expected.

That's because "not modifiable" and "goes into r/o section" are not the
same thing.  The former belongs to C and is target-independent, of course.
The latter is up to implementation, except that modifiable objects
obviously *can not* go into r/o.  You are not guaranteed the reverse.

> We could invent a __initstr annotation but I dunno if that would suffice.
> Do you see any pattern when gcc do the r/w choice compared to the
> r/o choise. Maybe it is only const char[] that happens to be considered
> r/o and the rest is r/w?

On ppc64 relocs => r/w, AFAICS.  On other targets we might have any number
of other rules.
 
> Should a gcc-bug be filed for this btw?

Why?  gcc is entirely within its rights - it's not even a matter of ABI,
it's way below that.  Note that you are meddling with section assignment
rules and those are target-dependent, so target-independent overrides
are nowhere near being promised to work.

Frankly, "do not ever make __initdata et.al. const" is probably the best
we can do - adding __initconst, __initconst_but_has_relocs,
__initconst_but_has_something_that_puts_it_into_writable_on_this_weird_target,
__initconst_but_has_something_that_puts_it_into_writable_on_that_weird_target,
etc. is not feasible - we'll keep getting portability bugs all the time since
nobody will remember all rules (or care about ones that do not apply on
amd64).

Suppose we have an array of ad-hoc structs with a bunch of ints
in those.  Used only in ->probe(), so __devinitdata.  Constant, so
__devinitconst (and no special per-target rules trigger on the current
struct contents).  Fast forward half a year; somebody adds a string field
to those.  Oops - suddenly it's __devinitconst_but_has_relocs, but author
of that patch has never heard of ppc64 oddities; on all targets he knows
__devinitconst still works fine.

Re: section breakage on ppc64 (aka __devinitconst is broken by design)

[Olivier Galibert]

On Sun, Feb 03, 2008 at 09:02:08PM +0000, Al Viro wrote:
> On ppc64 relocs => r/w, AFAICS.  On other targets we might have any number
> of other rules.

And -fpic/PIC => (relocs => r/w) because of the DT_TEXTREL crap.  Not
of immediate interest to the kernel though.

  OG.

Re: section breakage on ppc64 (aka __devinitconst is broken by design)

[Chuck Ebbert]

On 02/03/2008 08:08 AM, Al Viro wrote:
>
> The reason why it didn't visibly bite us before is that usually __devinit...
> just expanded to nothing (unless you disable HOTPLUG, which requires
> EMBEDDED, which wasn't apparently common enough for ppc64 builds).
>
> Suggestions?

This ugly hackset was needed to get 2.6.24 to build with GCC 4.3. It would
be nice to get a real fix...


--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -62,6 +62,15 @@ struct kparam_array
 	void *elem;
 };

+/* On some platforms relocations to global data cannot go into read-only
+   sections, so 'const' makes no sense and even causes compile failures
+   with some compilers. */
+#if defined(CONFIG_ALPHA) || defined(CONFIG_IA64) || defined(CONFIG_PPC64)
+#define __moduleparam_const
+#else
+#define __moduleparam_const const
+#endif
+
 /* This is the fundamental function for registering boot/module
    parameters.  perm sets the visibility in sysfs: 000 means it's
    not there, read bits mean it's readable, write bits mean it's
@@ -71,7 +80,7 @@ struct kparam_array
 	static int __param_perm_check_##name __attribute__((unused)) =	\
 	BUILD_BUG_ON_ZERO((perm) < 0 || (perm) > 0777 || ((perm) & 2));	\
 	static const char __param_str_##name[] = prefix #name;		\
-	static struct kernel_param const __param_##name			\
+	static struct kernel_param __moduleparam_const __param_##name	\
 	__attribute_used__						\
     __attribute__ ((unused,__section__ ("__param"),aligned(sizeof(void *)))) \
 	= { __param_str_##name, perm, set, get, { arg } }
--- linux-2.6.24.noarch.orig/include/linux/module.h
+++ linux-2.6.24.noarch/include/linux/module.h
@@ -30,6 +30,15 @@

 #define MODULE_NAME_LEN (64 - sizeof(unsigned long))

+/* On some platforms relocations to global data cannot go into read-only
+   sections, so 'const' makes no sense and even causes compile failures
+   with some compilers. */
+#if defined(CONFIG_ALPHA) || defined(CONFIG_IA64) || defined(CONFIG_PPC64)
+#define __ksym_const
+#else
+#define __ksym_const const
+#endif
+
 struct kernel_symbol
 {
 	unsigned long value;
@@ -192,7 +201,7 @@ void *__symbol_get_gpl(const char *symbo
 	static const char __kstrtab_##sym[]			\
 	__attribute__((section("__ksymtab_strings")))		\
 	= MODULE_SYMBOL_PREFIX #sym;                    	\
-	static const struct kernel_symbol __ksymtab_##sym	\
+	static __ksym_const struct kernel_symbol __ksymtab_##sym	\
 	__attribute_used__					\
 	__attribute__((section("__ksymtab" sec), unused))	\
 	= { (unsigned long)&sym, __kstrtab_##sym }

Re: section breakage on ppc64 (aka __devinitconst is broken by design)

[Sam Ravnborg]

On Thu, Feb 07, 2008 at 01:33:50PM -0500, Chuck Ebbert wrote:
> On 02/03/2008 08:08 AM, Al Viro wrote:
> >
> > The reason why it didn't visibly bite us before is that usually __devinit...
> > just expanded to nothing (unless you disable HOTPLUG, which requires
> > EMBEDDED, which wasn't apparently common enough for ppc64 builds).
> >
> > Suggestions?
> 
> This ugly hackset was needed to get 2.6.24 to build with GCC 4.3. It would
> be nice to get a real fix...

I cannot see any other way out of this than to loose all the newly added
consts. We have to different behavior across platforms to find a suitable
solution that is reliable.

[Kept rest of mail as I added Jan - hope he have some ideas to throw in].

	Sam

> 
> 
> --- a/include/linux/moduleparam.h
> +++ b/include/linux/moduleparam.h
> @@ -62,6 +62,15 @@ struct kparam_array
>  	void *elem;
>  };
> 
> +/* On some platforms relocations to global data cannot go into read-only
> +   sections, so 'const' makes no sense and even causes compile failures
> +   with some compilers. */
> +#if defined(CONFIG_ALPHA) || defined(CONFIG_IA64) || defined(CONFIG_PPC64)
> +#define __moduleparam_const
> +#else
> +#define __moduleparam_const const
> +#endif
> +
>  /* This is the fundamental function for registering boot/module
>     parameters.  perm sets the visibility in sysfs: 000 means it's
>     not there, read bits mean it's readable, write bits mean it's
> @@ -71,7 +80,7 @@ struct kparam_array
>  	static int __param_perm_check_##name __attribute__((unused)) =	\
>  	BUILD_BUG_ON_ZERO((perm) < 0 || (perm) > 0777 || ((perm) & 2));	\
>  	static const char __param_str_##name[] = prefix #name;		\
> -	static struct kernel_param const __param_##name			\
> +	static struct kernel_param __moduleparam_const __param_##name	\
>  	__attribute_used__						\
>      __attribute__ ((unused,__section__ ("__param"),aligned(sizeof(void *)))) \
>  	= { __param_str_##name, perm, set, get, { arg } }
> --- linux-2.6.24.noarch.orig/include/linux/module.h
> +++ linux-2.6.24.noarch/include/linux/module.h
> @@ -30,6 +30,15 @@
> 
>  #define MODULE_NAME_LEN (64 - sizeof(unsigned long))
> 
> +/* On some platforms relocations to global data cannot go into read-only
> +   sections, so 'const' makes no sense and even causes compile failures
> +   with some compilers. */
> +#if defined(CONFIG_ALPHA) || defined(CONFIG_IA64) || defined(CONFIG_PPC64)
> +#define __ksym_const
> +#else
> +#define __ksym_const const
> +#endif
> +
>  struct kernel_symbol
>  {
>  	unsigned long value;
> @@ -192,7 +201,7 @@ void *__symbol_get_gpl(const char *symbo
>  	static const char __kstrtab_##sym[]			\
>  	__attribute__((section("__ksymtab_strings")))		\
>  	= MODULE_SYMBOL_PREFIX #sym;                    	\
> -	static const struct kernel_symbol __ksymtab_##sym	\
> +	static __ksym_const struct kernel_symbol __ksymtab_##sym	\
>  	__attribute_used__					\
>  	__attribute__((section("__ksymtab" sec), unused))	\
>  	= { (unsigned long)&sym, __kstrtab_##sym }

Re: section breakage on ppc64 (aka __devinitconst is broken by design)

[Jan Beulich]

>I cannot see any other way out of this than to loose all the newly added
>consts. We have to different behavior across platforms to find a suitable
>solution that is reliable.
>
>[Kept rest of mail as I added Jan - hope he have some ideas to throw in].

I'd first of all need a better understanding of what these comments are
really based upon:

/* On some platforms relocations to global data cannot go into read-only
   sections, so 'const' makes no sense and even causes compile failures
   with some compilers. */

While I can see such behavior as reasonable for, say, shared objects,
I severely doubt that this is generally appropriate for executables, not
to say for the kernel. This is particularly in the light of this comment in
gcc/output.h:

  /* To optimize loading of shared programs, define following subsections
     of data section:

which clearly says that the resulting (default) object placement (of read-
only data in writeable sections) is an optimization, not a requirement,
and even then only for shared programs (which the kernel clearly isn't).
Has there been any communication with the gcc folks on this subject?

Jan

Re: section breakage on ppc64 (aka __devinitconst is broken by design)

[Sam Ravnborg]

On Fri, Feb 08, 2008 at 08:14:11AM +0000, Jan Beulich wrote:
> >I cannot see any other way out of this than to loose all the newly added
> >consts. We have to different behavior across platforms to find a suitable
> >solution that is reliable.
> >
> >[Kept rest of mail as I added Jan - hope he have some ideas to throw in].
> 
> I'd first of all need a better understanding of what these comments are
> really based upon:
> 
> /* On some platforms relocations to global data cannot go into read-only
>    sections, so 'const' makes no sense and even causes compile failures
>    with some compilers. */

Al posted the following:
====================================================================
; cat >a.c <<'EOF'
const char foo[] __attribute__ ((__section__(".blah"))) = "";
const char * const bar __attribute__((__section__(".blah"))) = "";
EOF
; gcc -m32 -S a.c
; gcc -m64 -S a.c
a.c:2: error: bar causes a section type conflict
;

That's 4.1.2 on ppc.  What happens is that the second declaration
wants to make .blah writable.  We actually trigger that in ppc64
builds on drivers/net/natsemi.c.

Note that on ppc64 without explicit sections you have the second one land in
.data.rel.ro.local, which is "aw",progbits.
====================================================================

Se we see that despite being marked as const the the section
is in one case marked as read-only by gcc and in another case the section
is marked as rw.

And this is with the gcc version that is in use today and which
we must support.
Thats why I think we have to loose the constification that is going on
or we should loose the section attribute on the data.


> 
> While I can see such behavior as reasonable for, say, shared objects,
> I severely doubt that this is generally appropriate for executables, not
> to say for the kernel. This is particularly in the light of this comment in
> gcc/output.h:
> 
>   /* To optimize loading of shared programs, define following subsections
>      of data section:
> 
> which clearly says that the resulting (default) object placement (of read-
> only data in writeable sections) is an optimization, not a requirement,
> and even then only for shared programs (which the kernel clearly isn't).
> Has there been any communication with the gcc folks on this subject?

No.

	Sam

Re: section breakage on ppc64 (aka __devinitconst is broken by design)

[Jan Beulich]

>Al posted the following:
>====================================================================
>; cat >a.c <<'EOF'
>const char foo[] __attribute__ ((__section__(".blah"))) = "";
>const char * const bar __attribute__((__section__(".blah"))) = "";
>EOF
>; gcc -m32 -S a.c
>; gcc -m64 -S a.c
>a.c:2: error: bar causes a section type conflict
>;
>
>That's 4.1.2 on ppc.  What happens is that the second declaration
>wants to make .blah writable.  We actually trigger that in ppc64
>builds on drivers/net/natsemi.c.
>
>Note that on ppc64 without explicit sections you have the second one land in
>.data.rel.ro.local, which is "aw",progbits.
>====================================================================
>
>Se we see that despite being marked as const the the section
>is in one case marked as read-only by gcc and in another case the section
>is marked as rw.

Oh, indeed, this kind of a construct also fails for ia64 on existing gcc-s.

>And this is with the gcc version that is in use today and which
>we must support.
>Thats why I think we have to loose the constification that is going on
>or we should loose the section attribute on the data.

That is very unfortunate, but is then a good reason to fold the
'const' into the __devinitconst definition as I originally suggested (and
perhaps that's the reason why I didn't have problems with my version
of the patch on ia64) - that way, those architectures that can't tolerate
it could have __devinitconst continue to resolve to __devinitdata,
resulting in traditional section allocation, while targets like x86 can still
benefit from the constification.

Apart from that we may want to approach the gcc folks to allow a way
to override this 'optimization for the kernel (or more generally for
statically linked executables).

Jan