Skip to contentSkip to conversation

Fix build bug

IDE Ralf Baechle 5 participants 13 messages Oct 25, 2007 – Nov 2, 2007

Subsystems (2)
MIPS libata
lore
Share & tools
lore

Ralf Baechle

author
#0 lore raw

CC drivers/ide/pci/generic.o
drivers/ide/pci/generic.c:52: error: __setup_str_ide_generic_all_on causes a +section type conflict

This sort of build error is becoming a regular issue. Either all or non of the elements that go into a particular section of a compilation unit need to be const. Or an error may result such as in this case if CONFIG_HOTPLUG is unset.

Maybe worth a check in checkpatch.pl - but certainly gcc's interolerance is also being less than helpful here.

---
 drivers/ide/pci/generic.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/ide/pci/generic.c +1 -1
index f44d708..0047684 100644
@@ -67,7 +67,7 @@ MODULE_PARM_DESC(all_generic_ide, "IDE generic will claim all unknown PCI IDE st
 		.udma_mask	= ATA_UDMA6, \
 	}
 
-static const struct ide_port_info generic_chipsets[] __devinitdata = {
+static struct ide_port_info generic_chipsets[] __devinitdata = {
 	/*  0 */ DECLARE_GENERIC_PCI_DEV("Unknown",	0),
 
 	{	/* 1 */
#1 lore raw
Quoted 8 lines · Ralf Baechle

CC drivers/ide/pci/generic.o
drivers/ide/pci/generic.c:52: error: __setup_str_ide_generic_all_on causes a +section type conflict

This sort of build error is becoming a regular issue. Either all or non of the elements that go into a particular section of a compilation unit need to be const. Or an error may result such as in this case if CONFIG_HOTPLUG is unset.

So we can avoid this if we invent a __constinitdata tag that uses a new section?
I ask mainly to understand this error - not that I am that found of the idea.

Sam

author
#2 lore raw
Quoted 5 lines · Sam Ravnborg
Quoted 4 lines · Ralf Baechle

CC drivers/ide/pci/generic.o
drivers/ide/pci/generic.c:52: error: __setup_str_ide_generic_all_on causes a +section type conflict

Quoted 8 lines
Quoted 4 lines

This sort of build error is becoming a regular issue. Either all or non of the elements that go into a particular section of a compilation unit need to be const. Or an error may result such as in this case if CONFIG_HOTPLUG is unset.

So we can avoid this if we invent a __constinitdata tag that uses a new section?
I ask mainly to understand this error - not that I am that found of the idea.

Yes.

Ralf

#3 lore raw
In reply to Sam Ravnborg
Quoted 2 lines · Sam Ravnborg

So we can avoid this if we invent a __constinitdata tag that uses a new section?

That would do.

Quoted 2 lines

I ask mainly to understand this error - not that I am that found of the idea.

Somebody wants to mix up read-only and read/write data in the same
section and GCC quite legitimately complains about it. You cannot have both at a time.

Maciej

author
#4 lore raw
Quoted 11 lines · Maciej W. Rozycki
Quoted 2 lines

So we can avoid this if we invent a __constinitdata tag that uses a new section?

That would do.

Quoted 2 lines

I ask mainly to understand this error - not that I am that found of the idea.

Somebody wants to mix up read-only and read/write data in the same
section and GCC quite legitimately complains about it. You cannot have both at a time.

My interpretation is that it would be perfectly ok for a C compiler to do minimal handling of const by only throwing errors for attempted assignments to const objects but otherwise treating them as if they were non-const, that is for example putting them into an r/w section.

Ralf

#5 lore raw
Quoted 8 lines · Ralf Baechle
Quoted 3 lines

Somebody wants to mix up read-only and read/write data in the same
section and GCC quite legitimately complains about it. You cannot have both at a time.

My interpretation is that it would be perfectly ok for a C compiler to do minimal handling of const by only throwing errors for attempted assignments to const objects but otherwise treating them as if they were non-const, that is for example putting them into an r/w section.

That would probably be valid (any C standard expert please correct me if
I am wrong), but the approach looks like: since we have the capability in the hardware and the OS, then why not actually enforce the rule at the run time as well?

Maciej

#7 lore raw
In reply to Ralf Baechle

Hi,

Quoted 21 lines · Ralf Baechle

CC drivers/ide/pci/generic.o
drivers/ide/pci/generic.c:52: error: __setup_str_ide_generic_all_on causes a +section type conflict

This sort of build error is becoming a regular issue. Either all or non of the elements that go into a particular section of a compilation unit need to be const. Or an error may result such as in this case if CONFIG_HOTPLUG is unset.

Maybe worth a check in checkpatch.pl - but certainly gcc's interolerance is also being less than helpful here.

---
 drivers/ide/pci/generic.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/ide/pci/generic.c +0 -0
index f44d708..0047684 100644

drivers/ide/pci/generic.c MODULE_PARM_DESC(all_generic_ide, "IDE generic will claim all unknown PCI IDE st · line 73

Quoted 8 lines
drivers/ide/pci/generic.c +1 -1
@@ -67,7 +67,7 @@ MODULE_PARM_DESC(all_generic_ide, "IDE generic will claim all unknown PCI IDE st
 		.udma_mask	= ATA_UDMA6, \
 	}
 
-static const struct ide_port_info generic_chipsets[] __devinitdata = {
+static struct ide_port_info generic_chipsets[] __devinitdata = {
 	/*  0 */ DECLARE_GENERIC_PCI_DEV("Unknown",	0),
 
 	{	/* 1 */

I would prefer to not remove const from generic_chipsets[] so:

[PATCH] drivers/ide/pci/generic: fix build for CONFIG_HOTPLUG=n

It turns out that const and __{dev}initdata cannot be mixed currently and that generic IDE PCI host driver is also affected by the same issue:

Quoted 3 lines · Ralf Baechle

CC drivers/ide/pci/generic.o
drivers/ide/pci/generic.c:52: error: __setup_str_ide_generic_all_on causes a +section type conflict

[ Also reported by Martijn Uffing <[email protected]>. ]

This patch workarounds the problem in a bit hackish way but without removing const from generic_chipsets[] (it adds const to __setup() so __setup_str_ide_generic_all becomes const).

Now all __{dev}initdata data in generic IDE PCI host driver are read-only so it builds again (driver's .init.data section gets marked as READONLY).

Cc Martijn Uffing <[email protected]>
Cc Ralf Baechle <[email protected]>
Cc Andrew Morton <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/ide/pci/generic.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/ide/pci/generic.c +1 -1
Index: b/drivers/ide/pci/generic.c
===================================================================
@@ -49,7 +49,7 @@ static int __init ide_generic_all_on(cha
 	printk(KERN_INFO "IDE generic will claim all unknown PCI IDE storage controllers.\n");
 	return 1;
 }
-__setup("all-generic-ide", ide_generic_all_on);
+const __setup("all-generic-ide", ide_generic_all_on);
 #endif
 module_param_named(all_generic_ide, ide_generic_all, bool, 0444);
 MODULE_PARM_DESC(all_generic_ide, "IDE generic will claim all unknown PCI IDE storage controllers.");
#6 lore raw
In reply to Sam Ravnborg
Quoted 11 lines · Sam Ravnborg
Quoted 8 lines · Ralf Baechle

CC drivers/ide/pci/generic.o
drivers/ide/pci/generic.c:52: error: __setup_str_ide_generic_all_on causes a +section type conflict

This sort of build error is becoming a regular issue. Either all or non of the elements that go into a particular section of a compilation unit need to be const. Or an error may result such as in this case if CONFIG_HOTPLUG is unset.

So we can avoid this if we invent a __constinitdata tag that uses a new section?

I asked similar question on LKML few days ago together with the list of potentially problematic places:

	http://article.gmane.org/gmane.linux.kernel/594427

Now I see that the list is only partially complete since __initdata and co. may be also hidden in things like __setup() etc.

Thanks,
Bart

#8 lore raw
In reply to Bartlomiej Zolnierkiewicz
Quoted 18 lines · Bartlomiej Zolnierkiewicz

Review

Quoted 5 lines
(quoted diff) +1 -1
-static const struct ide_port_info generic_chipsets[] __devinitdata = {
+static struct ide_port_info generic_chipsets[] __devinitdata = {
 	/*  0 */ DECLARE_GENERIC_PCI_DEV("Unknown",	0),
 
 	{	/* 1 */

I would prefer to not remove const from generic_chipsets[] so:

[PATCH] drivers/ide/pci/generic: fix build for CONFIG_HOTPLUG=n

It turns out that const and __{dev}initdata cannot be mixed currently and that generic IDE PCI host driver is also affected by the same issue:

Quoted 3 lines · Ralf Baechle

CC drivers/ide/pci/generic.o
drivers/ide/pci/generic.c:52: error: __setup_str_ide_generic_all_on causes a +section type conflict

Quoted 5 lines

[ Also reported by Martijn Uffing <[email protected]>. ]

This patch workarounds the problem in a bit hackish way but without removing const from generic_chipsets[] (it adds const to __setup() so __setup_str_ide_generic_all becomes const).

You wouldn't believe how much const data is not marked as const because we don't have __constinitdata etc. Literally megabytes.
--
vda

author
#9 lore raw
Quoted 19 lines · Denys Vlasenko
Quoted 18 lines · Bartlomiej Zolnierkiewicz

Review

Quoted 5 lines
(quoted diff) +1 -1
-static const struct ide_port_info generic_chipsets[] __devinitdata = {
+static struct ide_port_info generic_chipsets[] __devinitdata = {
 	/*  0 */ DECLARE_GENERIC_PCI_DEV("Unknown",	0),
 
 	{	/* 1 */

I would prefer to not remove const from generic_chipsets[] so:

[PATCH] drivers/ide/pci/generic: fix build for CONFIG_HOTPLUG=n

It turns out that const and __{dev}initdata cannot be mixed currently and that generic IDE PCI host driver is also affected by the same issue:

Quoted 3 lines · Ralf Baechle

CC drivers/ide/pci/generic.o
drivers/ide/pci/generic.c:52: error: __setup_str_ide_generic_all_on causes a +section type conflict

Quoted 8 lines
Quoted 5 lines

[ Also reported by Martijn Uffing <[email protected]>. ]

This patch workarounds the problem in a bit hackish way but without removing const from generic_chipsets[] (it adds const to __setup() so __setup_str_ide_generic_all becomes const).

You wouldn't believe how much const data is not marked as const because we don't have __constinitdata etc. Literally megabytes.

The gain from marking it const is very little and once any non-const __initdata object is added to a compilation unit all other const declarations will have to be removed. Bad tradeoff.

Ralf

#10 lore raw
Quoted 27 lines · Ralf Baechle
Quoted 25 lines · Denys Vlasenko
Quoted 23 lines · Bartlomiej Zolnierkiewicz

Review

Quoted 5 lines
(quoted diff) +1 -1
-static const struct ide_port_info generic_chipsets[] __devinitdata = {
+static struct ide_port_info generic_chipsets[] __devinitdata = {
 	/*  0 */ DECLARE_GENERIC_PCI_DEV("Unknown",	0),
 
 	{	/* 1 */

I would prefer to not remove const from generic_chipsets[] so:

[PATCH] drivers/ide/pci/generic: fix build for CONFIG_HOTPLUG=n

It turns out that const and __{dev}initdata cannot be mixed currently and that generic IDE PCI host driver is also affected by the same issue:

Quoted 3 lines · Ralf Baechle

CC drivers/ide/pci/generic.o
drivers/ide/pci/generic.c:52: error: __setup_str_ide_generic_all_on causes a +section type conflict

[ Also reported by Martijn Uffing <[email protected]>. ]

This patch workarounds the problem in a bit hackish way but without removing const from generic_chipsets[] (it adds const to __setup() so __setup_str_ide_generic_all becomes const).

Quoted 6 lines
Quoted 2 lines

You wouldn't believe how much const data is not marked as const because we don't have __constinitdata etc. Literally megabytes.

The gain from marking it const is very little and once any non-const __initdata object is added to a compilation unit all other const declarations will have to be removed. Bad tradeoff.

In this case (struct ide_port_info) and probably few others having const is important (maybe even more important than having __{dev}initdata since majority of people use CONFIG_HOTPLUG=y) because it allows developers to catch subtle yet hard to find bugs very early in the development process.

We had a few such cases in IDE - struct ide_port_info _template_ was being modified because some quirk was needed for one version of the hardware which was of course incorrect if another version of the hardware was also present in the system.

Some other potential gains of using const like the better optimized code or the protection of read-only kernel data are only an extra bonuses. :)

I agree that we need __const{dev}initdata but until then the workaround that all __{dev}initdata must be const is an acceptable temporary solution for IDE host drivers.

Thanks,
Bart

#11 lore raw
In reply to Ralf Baechle
Quoted 27 lines · Ralf Baechle
Quoted 25 lines · Denys Vlasenko
Quoted 23 lines · Bartlomiej Zolnierkiewicz

Review

Quoted 5 lines
(quoted diff) +1 -1
-static const struct ide_port_info generic_chipsets[] __devinitdata = {
+static struct ide_port_info generic_chipsets[] __devinitdata = {
 	/*  0 */ DECLARE_GENERIC_PCI_DEV("Unknown",	0),
 
 	{	/* 1 */

I would prefer to not remove const from generic_chipsets[] so:

[PATCH] drivers/ide/pci/generic: fix build for CONFIG_HOTPLUG=n

It turns out that const and __{dev}initdata cannot be mixed currently and that generic IDE PCI host driver is also affected by the same issue:

Quoted 3 lines · Ralf Baechle

CC drivers/ide/pci/generic.o
drivers/ide/pci/generic.c:52: error: __setup_str_ide_generic_all_on causes a +section type conflict

[ Also reported by Martijn Uffing <[email protected]>. ]

This patch workarounds the problem in a bit hackish way but without removing const from generic_chipsets[] (it adds const to __setup() so __setup_str_ide_generic_all becomes const).

Quoted 6 lines
Quoted 2 lines

You wouldn't believe how much const data is not marked as const because we don't have __constinitdata etc. Literally megabytes.

The gain from marking it const is very little and once any non-const __initdata object is added to a compilation unit all other const declarations will have to be removed. Bad tradeoff.

We can intrduce new, ro sections or teach gcc that combining const objects into non-ro sections is not a crime. I wonder why it currently disallows that. (And it does it only _somethimes_, const pointers happily go into rw sections!) --
vda

author
#12 lore raw
Quoted 3 lines · Denys Vlasenko

We can intrduce new, ro sections or teach gcc that combining const objects into non-ro sections is not a crime. I wonder why it currently disallows that. (And it does it only _somethimes_, const pointers happily go into rw sections!)

The pattern seems to be that const-ness of the first object placed into a particular section determines the writability of that section. If that conflicts with the requirements for a later object such as a non-const object into a section r/o gcc doesn't consider making the section r/w but throws an error instead.

Ralf