[RFC PATCH] kbuild: Do not select symbols with unmet dependencies

[RFC PATCH] kbuild: Do not select symbols with unmet dependencies

[Catalin Marinas]

The "select" statement in Kconfig files allows the enabling of options
even if they have unmet direct dependencies (i.e. "depends on" expands
to "no"). Currently, the "depends on" clauses are used in calculating
the visibility but they do not affect the reverse dependencies in any
way.

The patch introduces additional tracking of the "depends on" statements
and does not allow selecting an option if its direct dependencies are
not met, also printing a warning.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---

This patch is meant to make "select" safer in Kconfig files. The
"depends on", if not true, takes priority over "select".

For example, in -next, the ARM FRAME_POINTER (arch/arm/Kconfig.debug)
depends on !THUMB2_KERNEL because compiling the kernel to the Thumb-2
instruction set and FRAME_POINTER enabled fails. However, there are
several places where this symbol is forced by "select" statements (where
stacktrace support would be enough). Another example is DEBUG_PAGEALLOC
which depends on ARCH_SUPPORTS_DEBUG_PAGEALLOC (=n on ARM) but still
selected.

A safer approach currently may be to only warn but still select the
corresponding symbol.

The patch could probably be improved to also point to the symbols
containing the "select" statements, though I was getting some segfaults
with expr_list_for_each_sym(sym->rev_dep.expr).

Please note that the patch doesn't entirely follow the maximum line
length recommendation as none of the modified files comply with it.

Thank you for you comments.

 scripts/kconfig/expr.h   |    1 +
 scripts/kconfig/menu.c   |    5 +++++
 scripts/kconfig/symbol.c |   17 ++++++++++++++++-
 3 files changed, 22 insertions(+), 1 deletions(-)

diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
index 6408fef..2381562 100644
--- a/scripts/kconfig/expr.h
+++ b/scripts/kconfig/expr.h
@@ -83,6 +83,7 @@ struct symbol {
 	tristate visible;
 	int flags;
 	struct property *prop;
+	struct expr_value dep;
 	struct expr_value rev_dep;
 };
 
diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index 07ff8d1..7d32d74 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -385,6 +385,11 @@ void menu_finalize(struct menu *parent)
 				expr_alloc_and(parent->prompt->visible.expr,
 					expr_alloc_symbol(&symbol_mod)));
 	}
+
+	if (sym) {
+		sym->dep.expr = expr_transform(expr_copy(parent->dep));
+		sym->dep.expr = expr_eliminate_dups(sym->dep.expr);
+	}
 }
 
 bool menu_is_visible(struct menu *menu)
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index 18f3e5c..af362d0 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -205,6 +205,16 @@ static void sym_calc_visibility(struct symbol *sym)
 	}
 	if (sym_is_choice_value(sym))
 		return;
+	/* defaulting to "yes" if no explicit "depends on" are given */
+	tri = yes;
+	if (sym->dep.expr)
+		tri = expr_calc_value(sym->dep.expr);
+	if (tri == mod)
+		tri = yes;
+	if (sym->dep.tri != tri) {
+		sym->dep.tri = tri;
+		sym_set_changed(sym);
+	}
 	tri = no;
 	if (sym->rev_dep.expr)
 		tri = expr_calc_value(sym->rev_dep.expr);
@@ -321,7 +331,12 @@ void sym_calc_value(struct symbol *sym)
 				}
 			}
 		calc_newval:
-			newval.tri = EXPR_OR(newval.tri, sym->rev_dep.tri);
+			if (sym->dep.tri == no && sym->rev_dep.tri != no)
+				fprintf(stderr, "%s:%d:warning: not selecting symbol with unmet dependencies: %s\n",
+					sym->prop->file->name, sym->prop->lineno,
+					sym->name ? sym->name : "<choice>");
+			else
+				newval.tri = EXPR_OR(newval.tri, sym->rev_dep.tri);
 		}
 		if (newval.tri == mod && sym_get_type(sym) == S_BOOLEAN)
 			newval.tri = yes;

Re: [RFC PATCH] kbuild: Do not select symbols with unmet dependencies

[Arnd Bergmann]

On Wednesday 12 August 2009, Catalin Marinas wrote:
> The "select" statement in Kconfig files allows the enabling of options
> even if they have unmet direct dependencies (i.e. "depends on" expands
> to "no"). Currently, the "depends on" clauses are used in calculating
> the visibility but they do not affect the reverse dependencies in any
> way.
> 
> The patch introduces additional tracking of the "depends on" statements
> and does not allow selecting an option if its direct dependencies are
> not met, also printing a warning.
> 
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>

I guess this will change the behaviour of a number of subsystems,
likely causing unexpected regressions. I think your change
makes sense, but we need to be much more careful.

Can you extract a list of configuration symbols that are
impacted by your patch? A possibly way out could be to annotate
all of them first by changing

---
config FOO
	bool

config BAR
	depends on FOO

config BAZ
	select BAR
---

so that we either get

config BAZ
	select FOO
	select BAR

or alternatively, on a case-by-case basis

config BAZ
	depends on FOO
	select BAR

Once that is in place, all the symbols have a well-defined behaviour
and we can safely apply your patch.

	Arnd <><

Re: [RFC PATCH] kbuild: Do not select symbols with unmet dependencies

[Catalin Marinas]

On Wed, 2009-08-12 at 14:36 +0200, Arnd Bergmann wrote:
> On Wednesday 12 August 2009, Catalin Marinas wrote:
> > The "select" statement in Kconfig files allows the enabling of options
> > even if they have unmet direct dependencies (i.e. "depends on" expands
> > to "no"). Currently, the "depends on" clauses are used in calculating
> > the visibility but they do not affect the reverse dependencies in any
> > way.
> > 
> > The patch introduces additional tracking of the "depends on" statements
> > and does not allow selecting an option if its direct dependencies are
> > not met, also printing a warning.
> > 
> > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Sam Ravnborg <sam@ravnborg.org>
> 
> I guess this will change the behaviour of a number of subsystems,
> likely causing unexpected regressions. I think your change
> makes sense, but we need to be much more careful.

It would indeed cause regressions, that's why I'm only asking for
comments currently.

> Can you extract a list of configuration symbols that are
> impacted by your patch?

I can generate a list with allyesconfig but it looks like it breaks some
common usage in Linux. For example, the VIDEO_* entries in
drivers/media/video/Kconfig under the "Encoders/decoders and other
helper chips" menu automatically inherit a dependency on
!VIDEO_HELPER_CHIPS_AUTO. This option is enabled to allow other config
options to select whatever they need. But it would fail with my patch
because of the direct dependency of the VIDEO_* options on
!VIDEO_HELPER_CHIPS_AUTO.

It needs a bit more thinking here and maybe writing something like:

menu "..." if !VIDEO_HELPER_CHIPS_AUTO

rather than

menu "..."
	depends on !VIDEO_HELPER_CHIPS_AUTO

though the parser (after modifying the zconf.y to handle this) seems to
consider both dependencies at the same level

-- 
Catalin

Re: [RFC PATCH] kbuild: Do not select symbols with unmet dependencies

[Catalin Marinas]

On Wed, 2009-08-12 at 15:00 +0100, Catalin Marinas wrote:
> I can generate a list with allyesconfig but it looks like it breaks some
> common usage in Linux. For example, the VIDEO_* entries in
> drivers/media/video/Kconfig under the "Encoders/decoders and other
> helper chips" menu automatically inherit a dependency on
> !VIDEO_HELPER_CHIPS_AUTO. This option is enabled to allow other config
> options to select whatever they need. But it would fail with my patch
> because of the direct dependency of the VIDEO_* options on
> !VIDEO_HELPER_CHIPS_AUTO.

It seems that allyesconfig is not the best test since the direct
dependencies tend to be true most of the time. It only showed a problem
with IP_VS_PROTO_AH_ESP which depends on UNDEFINED but is still
selected.

I modified the patch slightly to only consider the "depends on"
statements specific to a config option (without inherited ones) when
validating the "select" statements. This would allow the above menu
usecase without problems.

As for avoiding run-time regressions, the patch should either only warn
but continue as before or fail the kernel building in the config stage
so that the Kconfig files are fixed.

Here's the updated patch:


kbuild: Do not select symbols with unmet dependencies

From: Catalin Marinas <catalin.marinas@arm.com>

The "select" statement in Kconfig files allows the enabling of options
even if they have unmet direct dependencies (i.e. "depends on" expands
to "no"). Currently, the "depends on" clauses are used in calculating
the visibility but they do not affect the reverse dependencies in any
way.

The patch introduces additional tracking of the "depends on" statements
and does not allow selecting an option if its direct dependencies are
not met, also printing a warning.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
 scripts/kconfig/expr.h   |    2 ++
 scripts/kconfig/menu.c   |    5 +++++
 scripts/kconfig/symbol.c |   17 ++++++++++++++++-
 3 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
index 6408fef..c8be420 100644
--- a/scripts/kconfig/expr.h
+++ b/scripts/kconfig/expr.h
@@ -83,6 +83,7 @@ struct symbol {
 	tristate visible;
 	int flags;
 	struct property *prop;
+	struct expr_value dir_dep;
 	struct expr_value rev_dep;
 };
 
@@ -164,6 +165,7 @@ struct menu {
 	struct symbol *sym;
 	struct property *prompt;
 	struct expr *dep;
+	struct expr *dir_dep;
 	unsigned int flags;
 	char *help;
 	struct file *file;
diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index 07ff8d1..16e713f 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -102,6 +102,7 @@ struct expr *menu_check_dep(struct expr *e)
 void menu_add_dep(struct expr *dep)
 {
 	current_entry->dep = expr_alloc_and(current_entry->dep, menu_check_dep(dep));
+	current_entry->dir_dep = current_entry->dep;
 }
 
 void menu_set_type(int type)
@@ -285,6 +286,10 @@ void menu_finalize(struct menu *parent)
 		for (menu = parent->list; menu; menu = menu->next)
 			menu_finalize(menu);
 	} else if (sym) {
+		/* ignore inherited dependencies for dir_dep */
+		sym->dir_dep.expr = expr_transform(expr_copy(parent->dir_dep));
+		sym->dir_dep.expr = expr_eliminate_dups(sym->dir_dep.expr);
+
 		basedep = parent->prompt ? parent->prompt->visible.expr : NULL;
 		basedep = expr_trans_compare(basedep, E_UNEQUAL, &symbol_no);
 		basedep = expr_eliminate_dups(expr_transform(basedep));
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index 18f3e5c..3147d0a 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -205,6 +205,16 @@ static void sym_calc_visibility(struct symbol *sym)
 	}
 	if (sym_is_choice_value(sym))
 		return;
+	/* defaulting to "yes" if no explicit "depends on" are given */
+	tri = yes;
+	if (sym->dir_dep.expr)
+		tri = expr_calc_value(sym->dir_dep.expr);
+	if (tri == mod)
+		tri = yes;
+	if (sym->dir_dep.tri != tri) {
+		sym->dir_dep.tri = tri;
+		sym_set_changed(sym);
+	}
 	tri = no;
 	if (sym->rev_dep.expr)
 		tri = expr_calc_value(sym->rev_dep.expr);
@@ -321,7 +331,12 @@ void sym_calc_value(struct symbol *sym)
 				}
 			}
 		calc_newval:
-			newval.tri = EXPR_OR(newval.tri, sym->rev_dep.tri);
+			if (sym->dir_dep.tri == no && sym->rev_dep.tri != no)
+				fprintf(stderr, "warning: not selecting symbol with unmet dependencies: %s\n",
+					sym->name);
+			else
+				newval.tri = EXPR_OR(newval.tri,
+						     sym->rev_dep.tri);
 		}
 		if (newval.tri == mod && sym_get_type(sym) == S_BOOLEAN)
 			newval.tri = yes;


-- 
Catalin