[PATCH] kconfig: warn about complex selects

[PATCH] kconfig: warn about complex selects

[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

(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


>From 67bd6b970fcca8ba5078ba376131e3669dadfd7b Mon Sep 17 00:00:00 2001
From: Vegard Nossum <vegard.nossum@gmail.com>
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 <vegard.nossum@gmail.com>
---
 scripts/kconfig/symbol.c |   31 ++++++++++++++++++++++++++++++-
 1 files changed, 30 insertions(+), 1 deletions(-)

diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index 18f3e5c..e5c3c60 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -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)
-- 
1.5.4.1

Re: [PATCH] kconfig: warn about complex selects

[Bernd Eckenfels]

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

Re: [PATCH] kconfig: warn about complex selects

[Sam Ravnborg]

On Sun, May 04, 2008 at 07:40:44AM +0200, Vegard Nossum wrote:
> 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.

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

Re: [PATCH] kconfig: warn about complex selects

[Ingo Molnar]

* Sam Ravnborg <sam@ravnborg.org> wrote:

> On Sun, May 04, 2008 at 07:40:44AM +0200, Vegard Nossum wrote:
> > 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.
> 
> 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!

agreed, very cool stuff Vegard! :)

	Ingo

Re: [PATCH] kconfig: warn about complex selects

[Ingo Molnar]

* Vegard Nossum <vegard.nossum@gmail.com> wrote:

> 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.)

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

Re: [PATCH] kconfig: warn about complex selects

[Vegard Nossum]

On Sun, May 4, 2008 at 10:21 AM, Sam Ravnborg <sam@ravnborg.org> wrote:
>
> On Sun, May 04, 2008 at 07:40:44AM +0200, Vegard Nossum wrote:
>  > 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.
>
>  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.

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

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

Re: [PATCH] kconfig: warn about complex selects

[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.


Vegard


>From c5587b1c4a442ae8edde7e22f36ab9058d1e9121 Mon Sep 17 00:00:00 2001
From: Vegard Nossum <vegard.nossum@gmail.com>
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 <vegard.nossum@gmail.com>
---
 scripts/kconfig/symbol.c |   37 +++++++++++++++++++++++++++++++++++++
 1 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index 18f3e5c..4848588 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -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);
-- 
1.5.4.1

Re: [PATCH] kconfig: warn about complex selects

[Sam Ravnborg]

On Sun, May 04, 2008 at 01:00:38PM +0200, Vegard Nossum wrote:
> 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

Re: [PATCH] kconfig: warn about complex selects

[Roman Zippel]

Hi,

On Sun, 4 May 2008, Vegard Nossum wrote:

> 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

Re: [PATCH] kconfig: warn about complex selects

[Adrian Bunk]

On Tue, May 06, 2008 at 05:43:32AM +0200, Roman Zippel wrote:
> Hi,

Hi Roman,

>...
> 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?

> bye, Roman

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed