Skip to contentSkip to conversation

kconfig: warn about complex selects

PATCH In review Vegard Nossum 6 participants 10 messages May 4, 2008 – May 6, 2008 Kconfig

lore
Share & tools
lore

Vegard Nossum

author
#0 lore raw

PATCH 1/1 kconfig: warn about complex selects

Hi,

Given recent discussion about kconfig and the "select" feature, I have made the following quick & dirty patch to detect what I call "complex selects".

For v2.6.25, I get these warnings:
sound/pci/Kconfig:512:error: found complex select: SND_FM801_TEA575X -> VIDEO_V4L1 drivers/ide/Kconfig:890:error: found complex select: ETRAX_IDE -> BLK_DEV_IDEDMA drivers/acpi/Kconfig:185:error: found complex select: ACPI_HOTPLUG_CPU -> ACPI_CONTAINER

While for v2.6.26-rc1, I get these:
sound/pci/Kconfig:528:error: found complex select: SND_FM801_TEA575X -> VIDEO_V4L1 drivers/media/video/em28xx/Kconfig:2:error: found complex select: VIDEO_EM28XX -> MEDIA_TUNER drivers/media/video/bt8xx/Kconfig:2:error: found complex select: VIDEO_BT848 -> MEDIA_TUNER drivers/media/video/saa7134/Kconfig:2:error: found complex select: VIDEO_SAA7134 -> MEDIA_TUNER drivers/media/video/cx88/Kconfig:2:error: found complex select: VIDEO_CX88 -> MEDIA_TUNER drivers/media/video/cx23885/Kconfig:2:error: found complex select: VIDEO_CX23885 -> MEDIA_TUNER drivers/media/video/ivtv/Kconfig:2:error: found complex select: VIDEO_IVTV -> MEDIA_TUNER drivers/media/video/cx18/Kconfig:2:error: found complex select: VIDEO_CX18 -> MEDIA_TUNER drivers/media/video/pvrusb2/Kconfig:2:error: found complex select: VIDEO_PVRUSB2 -> MEDIA_TUNER drivers/media/video/Kconfig:690:error: found complex select: VIDEO_MXB -> MEDIA_TUNER drivers/media/video/usbvision/Kconfig:2:error: found complex select: VIDEO_USBVISION -> MEDIA_TUNER drivers/acpi/Kconfig:188:error: found complex select: ACPI_HOTPLUG_CPU -> ACPI_CONTAINER

(In other words, the number of these has increased significantly since the last release, and these will probably be hit as compile errors at one point or another.)

I am not a kconfig expert, so I might have missed some things. Roman, is this even remotely good? I think it's probably a good start anyway.

One other idea that might be feasible in terms of both correctness and
usability: If a symbol foo does "select bar", maybe foo should simply inherit bar's dependencies? This allows foo to be visible in most cases (when the dependencies of foo and bar are both satisified), but won't brute-force-enable any symbols.

Either way, here's my contribution to the discussion.

Vegard

Quoted 1 line

From 67bd6b970fcca8ba5078ba376131e3669dadfd7b Mon Sep 17 00:00:00 2001

From: Vegard Nossum <[email protected]>
Date: Sun, 4 May 2008 07:24:04 +0200
Subject: [PATCH] kconfig: warn about complex selects

"Complex selects" are selects that depend on symbols with dependencies. These are effectively circumventing the rest of the dependency chains of kconfig, often leading to build errors.

Signed-off-by Vegard Nossum <[email protected]>
---
 scripts/kconfig/symbol.c |   31 ++++++++++++++++++++++++++++++-
 1 files changed, 30 insertions(+), 1 deletions(-)
scripts/kconfig/symbol.c +30 -1
index 18f3e5c..e5c3c60 100644
@@ -790,6 +790,24 @@ static struct symbol *sym_check_expr_deps(struct expr *e)
 	return NULL;
 }
 
+bool sym_check_select(struct symbol *sym)
+{
+	struct property *prop;
+
+	for (prop = sym->prop; prop; prop = prop->next) {
+		struct expr *expr = prop->expr;
+
+		if (!expr)
+			continue;
+		if (expr->type == E_SYMBOL)
+			continue;
+
+		return true;
+	}
+
+	return false;
+}
+
 /* return NULL when dependencies are OK */
 static struct symbol *sym_check_sym_deps(struct symbol *sym)
 {

@@ -801,7 +819,18 @@ static struct symbol *sym_check_sym_deps(struct symbol *sym)
 		return sym2;
 
 	for (prop = sym->prop; prop; prop = prop->next) {
-		if (prop->type == P_CHOICE || prop->type == P_SELECT)
+		if (prop->type == P_SELECT) {
+			if (!sym_check_select(prop->expr->left.sym))
+				continue;
+
+			fprintf(stderr, "%s:%d:error: found complex select: "
+				"%s -> %s\n",
+				sym->prop->file->name, sym->prop->lineno,
+				sym->name, prop->expr->left.sym->name);
+			continue;
+		}
+
+		if (prop->type == P_CHOICE)
 			continue;
 		sym2 = sym_check_expr_deps(prop->visible.expr);
 		if (sym2)
Signature · 1 line
1.5.4.1
#1 lore raw
Quoted 3 lines · In article <20080504054044.GA32030@damson> you wrote:

(In other words, the number of these has increased significantly since the last release, and these will probably be hit as compile errors at one point or another.)

In other words this feature is very seldomly used and we could just disallow to use it. (However I guess it missed some - otherwise we wont have so much talk about it).

Gruss
Bernd

#2 lore raw
In reply to Vegard Nossum
Quoted 24 lines · Vegard Nossum

Hi,

Given recent discussion about kconfig and the "select" feature, I have made the following quick & dirty patch to detect what I call "complex selects".

For v2.6.25, I get these warnings:
sound/pci/Kconfig:512:error: found complex select: SND_FM801_TEA575X -> VIDEO_V4L1 drivers/ide/Kconfig:890:error: found complex select: ETRAX_IDE -> BLK_DEV_IDEDMA drivers/acpi/Kconfig:185:error: found complex select: ACPI_HOTPLUG_CPU -> ACPI_CONTAINER

While for v2.6.26-rc1, I get these:
sound/pci/Kconfig:528:error: found complex select: SND_FM801_TEA575X -> VIDEO_V4L1 drivers/media/video/em28xx/Kconfig:2:error: found complex select: VIDEO_EM28XX -> MEDIA_TUNER drivers/media/video/bt8xx/Kconfig:2:error: found complex select: VIDEO_BT848 -> MEDIA_TUNER drivers/media/video/saa7134/Kconfig:2:error: found complex select: VIDEO_SAA7134 -> MEDIA_TUNER drivers/media/video/cx88/Kconfig:2:error: found complex select: VIDEO_CX88 -> MEDIA_TUNER drivers/media/video/cx23885/Kconfig:2:error: found complex select: VIDEO_CX23885 -> MEDIA_TUNER drivers/media/video/ivtv/Kconfig:2:error: found complex select: VIDEO_IVTV -> MEDIA_TUNER drivers/media/video/cx18/Kconfig:2:error: found complex select: VIDEO_CX18 -> MEDIA_TUNER drivers/media/video/pvrusb2/Kconfig:2:error: found complex select: VIDEO_PVRUSB2 -> MEDIA_TUNER drivers/media/video/Kconfig:690:error: found complex select: VIDEO_MXB -> MEDIA_TUNER drivers/media/video/usbvision/Kconfig:2:error: found complex select: VIDEO_USBVISION -> MEDIA_TUNER drivers/acpi/Kconfig:188:error: found complex select: ACPI_HOTPLUG_CPU -> ACPI_CONTAINER

Quoted 6 lines

(In other words, the number of these has increased significantly since the last release, and these will probably be hit as compile errors at one point or another.)

I am not a kconfig expert, so I might have missed some things. Roman, is this even remotely good? I think it's probably a good start anyway.

I did a small test of this using following configuration:
config A
bool "a"

config B
bool "b"
depends on A
default AA || AAA

config BB
bool "bb"
depends on A

config C
bool "c"
select B

config CC
bool "cc"
select BB

I had expected a warning about complex select for both C and CC. But only C triggered the warning due to the complex default value of B.
So we need to work on this a bit more.

But great to see code instead of just talking!

Sam

#3 lore raw
Quoted 54 lines · Sam Ravnborg
Quoted 24 lines · Vegard Nossum

Hi,

Given recent discussion about kconfig and the "select" feature, I have made the following quick & dirty patch to detect what I call "complex selects".

For v2.6.25, I get these warnings:
sound/pci/Kconfig:512:error: found complex select: SND_FM801_TEA575X -> VIDEO_V4L1 drivers/ide/Kconfig:890:error: found complex select: ETRAX_IDE -> BLK_DEV_IDEDMA drivers/acpi/Kconfig:185:error: found complex select: ACPI_HOTPLUG_CPU -> ACPI_CONTAINER

While for v2.6.26-rc1, I get these:
sound/pci/Kconfig:528:error: found complex select: SND_FM801_TEA575X -> VIDEO_V4L1 drivers/media/video/em28xx/Kconfig:2:error: found complex select: VIDEO_EM28XX -> MEDIA_TUNER drivers/media/video/bt8xx/Kconfig:2:error: found complex select: VIDEO_BT848 -> MEDIA_TUNER drivers/media/video/saa7134/Kconfig:2:error: found complex select: VIDEO_SAA7134 -> MEDIA_TUNER drivers/media/video/cx88/Kconfig:2:error: found complex select: VIDEO_CX88 -> MEDIA_TUNER drivers/media/video/cx23885/Kconfig:2:error: found complex select: VIDEO_CX23885 -> MEDIA_TUNER drivers/media/video/ivtv/Kconfig:2:error: found complex select: VIDEO_IVTV -> MEDIA_TUNER drivers/media/video/cx18/Kconfig:2:error: found complex select: VIDEO_CX18 -> MEDIA_TUNER drivers/media/video/pvrusb2/Kconfig:2:error: found complex select: VIDEO_PVRUSB2 -> MEDIA_TUNER drivers/media/video/Kconfig:690:error: found complex select: VIDEO_MXB -> MEDIA_TUNER drivers/media/video/usbvision/Kconfig:2:error: found complex select: VIDEO_USBVISION -> MEDIA_TUNER drivers/acpi/Kconfig:188:error: found complex select: ACPI_HOTPLUG_CPU -> ACPI_CONTAINER

Quoted 6 lines

(In other words, the number of these has increased significantly since the last release, and these will probably be hit as compile errors at one point or another.)

I am not a kconfig expert, so I might have missed some things. Roman, is this even remotely good? I think it's probably a good start anyway.

I did a small test of this using following configuration:
config A
bool "a"

config B
bool "b"
depends on A
default AA || AAA

config BB
bool "bb"
depends on A

config C
bool "c"
select B

config CC
bool "cc"
select BB

Quoted 5 lines

I had expected a warning about complex select for both C and CC. But only C triggered the warning due to the complex default value of B. So we need to work on this a bit more.

But great to see code instead of just talking!

agreed, very cool stuff Vegard! :)

Ingo

author
#4 lore raw
In reply to Sam Ravnborg
Quoted 55 lines · Sam Ravnborg
Quoted 24 lines · Vegard Nossum

Hi,

Given recent discussion about kconfig and the "select" feature, I have made the following quick & dirty patch to detect what I call "complex selects".

For v2.6.25, I get these warnings:
sound/pci/Kconfig:512:error: found complex select: SND_FM801_TEA575X -> VIDEO_V4L1 drivers/ide/Kconfig:890:error: found complex select: ETRAX_IDE -> BLK_DEV_IDEDMA drivers/acpi/Kconfig:185:error: found complex select: ACPI_HOTPLUG_CPU -> ACPI_CONTAINER

While for v2.6.26-rc1, I get these:
sound/pci/Kconfig:528:error: found complex select: SND_FM801_TEA575X -> VIDEO_V4L1 drivers/media/video/em28xx/Kconfig:2:error: found complex select: VIDEO_EM28XX -> MEDIA_TUNER drivers/media/video/bt8xx/Kconfig:2:error: found complex select: VIDEO_BT848 -> MEDIA_TUNER drivers/media/video/saa7134/Kconfig:2:error: found complex select: VIDEO_SAA7134 -> MEDIA_TUNER drivers/media/video/cx88/Kconfig:2:error: found complex select: VIDEO_CX88 -> MEDIA_TUNER drivers/media/video/cx23885/Kconfig:2:error: found complex select: VIDEO_CX23885 -> MEDIA_TUNER drivers/media/video/ivtv/Kconfig:2:error: found complex select: VIDEO_IVTV -> MEDIA_TUNER drivers/media/video/cx18/Kconfig:2:error: found complex select: VIDEO_CX18 -> MEDIA_TUNER drivers/media/video/pvrusb2/Kconfig:2:error: found complex select: VIDEO_PVRUSB2 -> MEDIA_TUNER drivers/media/video/Kconfig:690:error: found complex select: VIDEO_MXB -> MEDIA_TUNER drivers/media/video/usbvision/Kconfig:2:error: found complex select: VIDEO_USBVISION -> MEDIA_TUNER drivers/acpi/Kconfig:188:error: found complex select: ACPI_HOTPLUG_CPU -> ACPI_CONTAINER

Quoted 6 lines

(In other words, the number of these has increased significantly since the last release, and these will probably be hit as compile errors at one point or another.)

I am not a kconfig expert, so I might have missed some things. Roman, is this even remotely good? I think it's probably a good start anyway.

I did a small test of this using following configuration:
config A
bool "a"

config B
bool "b"
depends on A
default AA || AAA

config BB
bool "bb"
depends on A

config C
bool "c"
select B

config CC
bool "cc"
select BB

Quoted 4 lines

I had expected a warning about complex select for both C and CC. But only C triggered the warning due to the complex default value of B.
So we need to work on this a bit more.

Oops, yes. What I am checking is in fact only the "default" property, not the "depends on". Hm. How to get the list of dependencies? :-)

Vegard

Signature · 4 lines
"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
author
#5 lore raw

Okay, here's try #2. (That's what I get for making patches too early in the morning.)

This checks dependencies instead of the "default" property. I don't know how useful this is, though, as it will generate some false positives. Actually, a lot of them.

Vegard

Quoted 1 line

From c5587b1c4a442ae8edde7e22f36ab9058d1e9121 Mon Sep 17 00:00:00 2001

From: Vegard Nossum <[email protected]>
Date: Sun, 4 May 2008 07:24:04 +0200
Subject: [PATCH] kconfig: warn about complex selects

"Complex selects" are selects that depend on symbols with dependencies. These are effectively circumventing the rest of the dependency chains of kconfig, often leading to build errors.

Signed-off-by Vegard Nossum <[email protected]>
---
 scripts/kconfig/symbol.c |   37 +++++++++++++++++++++++++++++++++++++
 1 files changed, 37 insertions(+), 0 deletions(-)
scripts/kconfig/symbol.c +37 -0
index 18f3e5c..4848588 100644
@@ -790,6 +790,41 @@ static struct symbol *sym_check_expr_deps(struct expr *e)
 	return NULL;
 }
 
+static bool sym_is_complex(struct symbol *sym)
+{
+	struct expr *expr;
+	struct property *prop;
+
+	for_all_prompts(sym, prop) {
+		expr = prop->visible.expr;
+
+		if (!expr)
+			continue;
+		if (expr->type != E_NONE)
+			return true;
+	}
+
+	return false;
+}
+
+static void sym_check_select(struct symbol *sym)
+{
+	struct property *prop;
+
+	for (prop = sym->prop; prop; prop = prop->next) {
+		if (prop->type != P_SELECT)
+			continue;
+
+		if (!sym_is_complex(prop->expr->left.sym))
+			continue;
+
+		fprintf(stderr, "%s:%d:error: found complex select: "
+			"%s -> %s\n",
+			sym->prop->file->name, sym->prop->lineno,
+			sym->name, prop->expr->left.sym->name);
+	}
+}
+
 /* return NULL when dependencies are OK */
 static struct symbol *sym_check_sym_deps(struct symbol *sym)
 {

@@ -864,6 +899,8 @@ struct symbol *sym_check_deps(struct symbol *sym)
 	if (sym->flags & SYMBOL_CHECKED)
 		return NULL;
 
+	sym_check_select(sym);
+
 	if (sym_is_choice_value(sym)) {
 		/* for choice groups start the check with main choice symbol */
 		prop = sym_get_choice_prop(sym);
Signature · 1 line
1.5.4.1
#6 lore raw
Quoted 6 lines · Vegard Nossum

Okay, here's try #2. (That's what I get for making patches too early in the morning.)

This checks dependencies instead of the "default" property. I don't know how useful this is, though, as it will generate some false positives. Actually, a lot of them.

Consider following simple Kconfig:
config A
bool "a"

config B
bool "b"
depends on A

config C
bool "c"
select B

config D
bool "d"
depends on A
select B

We should try to coe up with something that warns on on the select in "config C", because B has dependencies C does not have.
The select in D is OK since D has at least the same dependencies as B.

So if we had a expr_is_subset_of(expr *e1, expr *e2)....

Roman - what do you think?

Sam

#7 lore raw
In reply to Vegard Nossum
Quoted 24 lines · Vegard Nossum

Hi,

Given recent discussion about kconfig and the "select" feature, I have made the following quick & dirty patch to detect what I call "complex selects".

For v2.6.25, I get these warnings:
sound/pci/Kconfig:512:error: found complex select: SND_FM801_TEA575X -> VIDEO_V4L1 drivers/ide/Kconfig:890:error: found complex select: ETRAX_IDE -> BLK_DEV_IDEDMA drivers/acpi/Kconfig:185:error: found complex select: ACPI_HOTPLUG_CPU -> ACPI_CONTAINER

While for v2.6.26-rc1, I get these:
sound/pci/Kconfig:528:error: found complex select: SND_FM801_TEA575X -> VIDEO_V4L1 drivers/media/video/em28xx/Kconfig:2:error: found complex select: VIDEO_EM28XX -> MEDIA_TUNER drivers/media/video/bt8xx/Kconfig:2:error: found complex select: VIDEO_BT848 -> MEDIA_TUNER drivers/media/video/saa7134/Kconfig:2:error: found complex select: VIDEO_SAA7134 -> MEDIA_TUNER drivers/media/video/cx88/Kconfig:2:error: found complex select: VIDEO_CX88 -> MEDIA_TUNER drivers/media/video/cx23885/Kconfig:2:error: found complex select: VIDEO_CX23885 -> MEDIA_TUNER drivers/media/video/ivtv/Kconfig:2:error: found complex select: VIDEO_IVTV -> MEDIA_TUNER drivers/media/video/cx18/Kconfig:2:error: found complex select: VIDEO_CX18 -> MEDIA_TUNER drivers/media/video/pvrusb2/Kconfig:2:error: found complex select: VIDEO_PVRUSB2 -> MEDIA_TUNER drivers/media/video/Kconfig:690:error: found complex select: VIDEO_MXB -> MEDIA_TUNER drivers/media/video/usbvision/Kconfig:2:error: found complex select: VIDEO_USBVISION -> MEDIA_TUNER drivers/acpi/Kconfig:188:error: found complex select: ACPI_HOTPLUG_CPU -> ACPI_CONTAINER

Quoted 3 lines

(In other words, the number of these has increased significantly since the last release, and these will probably be hit as compile errors at one point or another.)

wow - these are exactly the problems that have been implicated statistically via all the build failures.

and this concurs with my observation about build failures in this cycle
- 2.6.26 showed a sharp rise in Kconfig space build problems. (and i've been doing random build tests for a long time)

so your patch is pure gold. A thirty-line quick hack like this if implemented years ago could have avoided dozens and dozens of build failures and anguish to users, and we'd have lots less trivial patches littering the commit logs (and wasting maintainer and review bandwidth) as well. (because maintainers could act on these messages right when they introduce the select complexity, avoiding such bugs right at their source.)

Ingo

#8 lore raw
In reply to Vegard Nossum

Hi,

Quoted 5 lines · Vegard Nossum

One other idea that might be feasible in terms of both correctness and
usability: If a symbol foo does "select bar", maybe foo should simply inherit bar's dependencies? This allows foo to be visible in most cases (when the dependencies of foo and bar are both satisified), but won't brute-force-enable any symbols.

Well, select was introduced for exactly this purpose.

In general I don't think it's possible to warn about this without producing a lot a false positives. If that would discourage people from using select everywhere, it might be an option but I'm afraid it will only cause them to add even more selects just to shut up the warnings.

Here is an example patch from me just a few days ago that might qualify as complex select, but is quite valid:

http://marc.info/?l=linux-kernel&m=120960318111346&w=2

The problem is usually that an option which might be visible, is selected without all the necessary build dependencies. The question is now how do you want to identify options which are necessary?
In this example a simple dependency check would flag that NEW_LEDS isn't selected, but NEW_LEDS is a pure config dependency, i.e. it has no effect on the build process.

It's not an impossible problem, but it's not trivial either, so that I haven't spend any time on it and rather would like to spend the energy on fixing the problem properly instead of working around it. And the problem BTW isn't select itself, the problem is that all the Kconfig dependencies have grown and become more complex, so it's becoming increasingly more difficult to maintain and to use them - select was never meant to solve this problem...

bye, Roman

#9 lore raw
Quoted 1 line · Roman Zippel

Hi,

Hi Roman,

Quoted 8 lines

...
It's not an impossible problem, but it's not trivial either, so that I haven't spend any time on it and rather would like to spend the energy on fixing the problem properly instead of working around it. And the problem BTW isn't select itself, the problem is that all the Kconfig dependencies have grown and become more complex, so it's becoming increasingly more difficult to maintain and to use them - select was never meant to solve this problem...

what do you consider the best solution?

Can we get it solved in kconfig?

Or should kconfig stay the same and use the pattern we already use with SSB_POSSIBLE?

Or is there some better solution?

Quoted 1 line

bye, Roman

cu
Adrian

Signature · 4 lines
       "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