Improve dependency checks in kconfig

Improve dependency checks in kconfig

[Sam Ravnborg]

While working on some unification of Kconfig stuff across
all architectures I hit a Kconfig bug where it segfaulted
due to a recursive dependency.
It's a bug I have introduced myself long time ago :-(

I decided to do the reporting more verbose while fixing
this bug.

When looking into this I added a few comments to expr.h
so I do not have to fnd out how it works next time.

The problem was that kconfig do not save where it finds
a config symbol but only the related properties.

But a symbol defined like this:

    config FOO
            bool

does not have any properties so we do not save any
filename/line numbers.

The solution I came up with was to add a new property:
P_SYMBOL used solely to record filename/line number for
defined symbols.
I could not add it to struct symbol because a symbol can
be defined at several places.

With this change kconfig at least report one of the
places where said symbol is defined. It is not perfect as
we should report all places where a symbol is reported but
for the cases I hit what we have here was good enough.

With this I hit a recursive dependency issue in m68k - a patch
is already sent to the m68k guys.

Patches follows.

Sam Ravnborg (4):
      kconfig: explain symbol value defaults
      kconfig: add comments to symbol flags
      kconfig: struct property commented
      kconfig: improve readout when we hit recursive dependencies

 scripts/kconfig/expr.h   |   83 ++++++++++++++++++++++++++++++---------------
 scripts/kconfig/menu.c   |    2 +
 scripts/kconfig/symbol.c |   11 ++++--
 3 files changed, 65 insertions(+), 31 deletions(-)

	Sam

[PATCH 1/4] kconfig: explain symbol value defaults

[Sam Ravnborg]

Added a few comments - no functional change.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
---
 scripts/kconfig/expr.h |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
index 9d4cba1..455f2c8 100644
--- a/scripts/kconfig/expr.h
+++ b/scripts/kconfig/expr.h
@@ -65,9 +65,13 @@ enum symbol_type {
 	S_UNKNOWN, S_BOOLEAN, S_TRISTATE, S_INT, S_HEX, S_STRING, S_OTHER
 };
 
+/* enum values are used as index to symbol.def[] */
 enum {
 	S_DEF_USER,		/* main user value */
-	S_DEF_AUTO,
+	S_DEF_AUTO,		/* values read from auto.conf */
+	S_DEF_DEF3,		/* Reserved for UI usage */
+	S_DEF_DEF4,		/* Reserved for UI usage */
+	S_DEF_COUNT
 };
 
 struct symbol {
@@ -75,7 +79,7 @@ struct symbol {
 	char *name;
 	enum symbol_type type;
 	struct symbol_value curr;
-	struct symbol_value def[4];
+	struct symbol_value def[S_DEF_COUNT];
 	tristate visible;
 	int flags;
 	struct property *prop;
-- 
1.6.0.2.GIT

[PATCH 2/4] kconfig: add comments to symbol flags

[Sam Ravnborg]

No functional changes - only comments.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
---
 scripts/kconfig/expr.h |   34 ++++++++++++++++++----------------
 1 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
index 455f2c8..0bdb58e 100644
--- a/scripts/kconfig/expr.h
+++ b/scripts/kconfig/expr.h
@@ -88,22 +88,24 @@ struct symbol {
 
 #define for_all_symbols(i, sym) for (i = 0; i < 257; i++) for (sym = symbol_hash[i]; sym; sym = sym->next) if (sym->type != S_OTHER)
 
-#define SYMBOL_CONST		0x0001
-#define SYMBOL_CHECK		0x0008
-#define SYMBOL_CHOICE		0x0010
-#define SYMBOL_CHOICEVAL	0x0020
-#define SYMBOL_VALID		0x0080
-#define SYMBOL_OPTIONAL		0x0100
-#define SYMBOL_WRITE		0x0200
-#define SYMBOL_CHANGED		0x0400
-#define SYMBOL_AUTO		0x1000
-#define SYMBOL_CHECKED		0x2000
-#define SYMBOL_WARNED		0x8000
-#define SYMBOL_DEF		0x10000
-#define SYMBOL_DEF_USER		0x10000
-#define SYMBOL_DEF_AUTO		0x20000
-#define SYMBOL_DEF3		0x40000
-#define SYMBOL_DEF4		0x80000
+#define SYMBOL_CONST      0x0001  /* symbol is const */
+#define SYMBOL_CHECK      0x0008  /* used during dependency checking */
+#define SYMBOL_CHOICE     0x0010  /* start of a choice block (null name) */
+#define SYMBOL_CHOICEVAL  0x0020  /* used as a value in a choice block */
+#define SYMBOL_VALID      0x0080  /* set when symbol.curr is calculated */
+#define SYMBOL_OPTIONAL   0x0100  /* choice is optional - values can be 'n' */
+#define SYMBOL_WRITE      0x0200  /* ? */
+#define SYMBOL_CHANGED    0x0400  /* ? */
+#define SYMBOL_AUTO       0x1000  /* value from environment variable */
+#define SYMBOL_CHECKED    0x2000  /* used during dependency checking */
+#define SYMBOL_WARNED     0x8000  /* warning has been issued */
+
+/* Set when symbol.def[] is used */
+#define SYMBOL_DEF        0x10000  /* First bit of SYMBOL_DEF */
+#define SYMBOL_DEF_USER   0x10000  /* symbol.def[S_DEF_USER] is valid */
+#define SYMBOL_DEF_AUTO   0x20000  /* symbol.def[S_DEF_AUTO] is valid */
+#define SYMBOL_DEF3       0x40000  /* symbol.def[S_DEF_3] is valid */
+#define SYMBOL_DEF4       0x80000  /* symbol.def[S_DEF_4] is valid */
 
 #define SYMBOL_MAXLENGTH	256
 #define SYMBOL_HASHSIZE		257
-- 
1.6.0.2.GIT

[PATCH 3/4] kconfig: struct property commented

[Sam Ravnborg]

No functional changes

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
---
 scripts/kconfig/expr.h |   40 ++++++++++++++++++++++++++++++----------
 1 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
index 0bdb58e..6408fef 100644
--- a/scripts/kconfig/expr.h
+++ b/scripts/kconfig/expr.h
@@ -111,21 +111,41 @@ struct symbol {
 #define SYMBOL_HASHSIZE		257
 #define SYMBOL_HASHMASK		0xff
 
+/* A property represent the config options that can be associated
+ * with a config "symbol".
+ * Sample:
+ * config FOO
+ *         default y
+ *         prompt "foo prompt"
+ *         select BAR
+ * config BAZ
+ *         int "BAZ Value"
+ *         range 1..255
+ */
 enum prop_type {
-	P_UNKNOWN, P_PROMPT, P_COMMENT, P_MENU, P_DEFAULT, P_CHOICE,
-	P_SELECT, P_RANGE, P_ENV
+	P_UNKNOWN,
+	P_PROMPT,   /* prompt "foo prompt" or "BAZ Value" */
+	P_COMMENT,  /* text associated with a comment */
+	P_MENU,     /* prompt associated with a menuconfig option */
+	P_DEFAULT,  /* default y */
+	P_CHOICE,   /* choice value */
+	P_SELECT,   /* select BAR */
+	P_RANGE,    /* range 7..100 (for a symbol) */
+	P_ENV,      /* value from environment variable */
 };
 
 struct property {
-	struct property *next;
-	struct symbol *sym;
-	enum prop_type type;
-	const char *text;
+	struct property *next;     /* next property - null if last */
+	struct symbol *sym;        /* the symbol for which the property is associated */
+	enum prop_type type;       /* type of property */
+	const char *text;          /* the prompt value - P_PROMPT, P_MENU, P_COMMENT */
 	struct expr_value visible;
-	struct expr *expr;
-	struct menu *menu;
-	struct file *file;
-	int lineno;
+	struct expr *expr;         /* the optional conditional part of the property */
+	struct menu *menu;         /* the menu the property are associated with
+	                            * valid for: P_SELECT, P_RANGE, P_CHOICE,
+	                            * P_PROMPT, P_DEFAULT, P_MENU, P_COMMENT */
+	struct file *file;         /* what file was this property defined */
+	int lineno;                /* what lineno was this property defined */
 };
 
 #define for_all_properties(sym, st, tok) \
-- 
1.6.0.2.GIT

[PATCH 4/4] kconfig: improve readout when we hit recursive dependencies

[Sam Ravnborg]

Following sample:
    config TEST1
            bool
            depends on TEST2 && PCI
            select TEST2

    config TEST2
            bool

Will result in the following output:

    Kconfig:1:error: found recursive dependency:
                TEST1
             -> TEST2 (arch/x86/Kconfig:6)
             -> TEST1 (arch/x86/Kconfig:1)

The sample above gave a segmentation fault
which is also fixed by this patch.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
---
 scripts/kconfig/expr.h   |    1 +
 scripts/kconfig/menu.c   |    2 ++
 scripts/kconfig/symbol.c |   11 ++++++++---
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
index 6408fef..d587895 100644
--- a/scripts/kconfig/expr.h
+++ b/scripts/kconfig/expr.h
@@ -132,6 +132,7 @@ enum prop_type {
 	P_SELECT,   /* select BAR */
 	P_RANGE,    /* range 7..100 (for a symbol) */
 	P_ENV,      /* value from environment variable */
+	P_SYMBOL,   /* where a symbol is defined */
 };
 
 struct property {
diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index 07ff8d1..aa0e2f2 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -55,6 +55,8 @@ void menu_add_entry(struct symbol *sym)
 	*last_entry_ptr = menu;
 	last_entry_ptr = &menu->next;
 	current_entry = menu;
+	if (sym)
+		 menu_add_prop(P_SYMBOL, NULL, NULL, NULL);
 }
 
 void menu_end_entry(void)
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index 18f3e5c..474dae2 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -856,8 +856,9 @@ struct symbol *sym_check_deps(struct symbol *sym)
 	struct property *prop;
 
 	if (sym->flags & SYMBOL_CHECK) {
-		fprintf(stderr, "%s:%d:error: found recursive dependency: %s",
-		        sym->prop->file->name, sym->prop->lineno,
+		fprintf(stderr, "%s:%d:error: found recursive dependency:\n",
+		        sym->prop->file->name, sym->prop->lineno);
+		fprintf(stderr, "\t    %s\n",
 			sym->name ? sym->name : "<choice>");
 		return sym;
 	}
@@ -877,7 +878,9 @@ struct symbol *sym_check_deps(struct symbol *sym)
 	}
 
 	if (sym2) {
-		fprintf(stderr, " -> %s", sym->name ? sym->name : "<choice>");
+		fprintf(stderr, "\t -> %s (%s:%d)\n",
+		        sym->name ? sym->name : "<choice>",
+			sym->prop->file->name, sym->prop->lineno);
 		if (sym2 == sym) {
 			fprintf(stderr, "\n");
 			zconfnerrs++;
@@ -937,6 +940,8 @@ const char *prop_get_type_name(enum prop_type type)
 		return "select";
 	case P_RANGE:
 		return "range";
+	case P_SYMBOL:
+		return "symbol";
 	case P_UNKNOWN:
 		break;
 	}
-- 
1.6.0.2.GIT

Re: Improve dependency checks in kconfig

[Sam Ravnborg]

On Sat, Dec 27, 2008 at 10:03:39AM +0100, Sam Ravnborg wrote:

> With this change kconfig at least report one of the
> places where said symbol is defined. It is not perfect as
> we should report all places where a symbol is reported but
> for the cases I hit what we have here was good enough.

I gave it a spin and following two patches does:
- print out all places where a symbol is defined
- print out where we hit source "" issues

	Sam

[PATCH 1/2] kconfig: print all locations when we see a recursive dependency

[Sam Ravnborg]

From 7e9617872fe81a9515e8ce22c9ece8bd1e905b4c Mon Sep 17 00:00:00 2001
From: Sam Ravnborg <sam@ravnborg.org>
Date: Sat, 27 Dec 2008 21:25:29 +0100
Subject: [PATCH 1/2] kconfig: print all locations when we see a recursive dependency

It is nessesary to know all locations when trying to track
a recursive dependency. So print out all locations for each symbol.

Sample output:
Kconfig:1: found recursive dependency:
            TEST1
         -> TEST2 (Kconfig:4),(Kconfig:12)
         -> TEST1 (Kconfig:1),(Kconfig:7)

In a more complex example the symbols can be defined in different
files thus making it harder to track.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Roman Zippel <zippel@linux-m68k.org>
---
 scripts/kconfig/symbol.c |   24 ++++++++++++++++++------
 1 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index 474dae2..fc62f34 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -790,6 +790,21 @@ static struct symbol *sym_check_expr_deps(struct expr *e)
 	return NULL;
 }
 
+static void print_symbol_location(struct symbol *sym)
+{
+	struct property *prop;
+	int comma = 0;
+
+	fprintf(stderr, "\t -> %s ", sym->name ? sym->name : "<choice>");
+	for (prop = sym->prop; prop; prop = prop->next) {
+		if (prop->type == P_SYMBOL) {
+			fprintf(stderr, "%s(%s:%d)",
+			comma ? "," : "", prop->file->name, prop->lineno);
+			comma = 1;
+		}
+	}
+	fprintf(stderr, "\n");
+}
 /* return NULL when dependencies are OK */
 static struct symbol *sym_check_sym_deps(struct symbol *sym)
 {
@@ -835,7 +850,7 @@ static struct symbol *sym_check_choice_deps(struct symbol *choice)
 	expr_list_for_each_sym(prop->expr, e, sym) {
 		sym2 = sym_check_sym_deps(sym);
 		if (sym2) {
-			fprintf(stderr, " -> %s", sym->name);
+			print_symbol_location(sym);
 			break;
 		}
 	}
@@ -856,7 +871,7 @@ struct symbol *sym_check_deps(struct symbol *sym)
 	struct property *prop;
 
 	if (sym->flags & SYMBOL_CHECK) {
-		fprintf(stderr, "%s:%d:error: found recursive dependency:\n",
+		fprintf(stderr, "%s:%d: found recursive dependency:\n",
 		        sym->prop->file->name, sym->prop->lineno);
 		fprintf(stderr, "\t    %s\n",
 			sym->name ? sym->name : "<choice>");
@@ -878,11 +893,8 @@ struct symbol *sym_check_deps(struct symbol *sym)
 	}
 
 	if (sym2) {
-		fprintf(stderr, "\t -> %s (%s:%d)\n",
-		        sym->name ? sym->name : "<choice>",
-			sym->prop->file->name, sym->prop->lineno);
+		print_symbol_location(sym);
 		if (sym2 == sym) {
-			fprintf(stderr, "\n");
 			zconfnerrs++;
 			sym2 = NULL;
 		}
-- 
1.6.0.2.GIT

[PATCH 2/2] kconfig: improve error messages for bad source statements

[Sam Ravnborg]

From 31606ff6946f5fc841b9d990c6c5479f54f16835 Mon Sep 17 00:00:00 2001
From: Sam Ravnborg <sam@ravnborg.org>
Date: Sat, 27 Dec 2008 21:51:59 +0100
Subject: [PATCH 2/2] kconfig: improve error messages for bad source statements

We now say where we detect the second source of a file,
and where we detect a recursively source of the same file.
This makes it easier to fix such errors.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Roman Zippel <zippel@linux-m68k.org>
---
 scripts/kconfig/lex.zconf.c_shipped |    7 +++++--
 scripts/kconfig/zconf.l             |    7 +++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/scripts/kconfig/lex.zconf.c_shipped b/scripts/kconfig/lex.zconf.c_shipped
index 7342ce0..dc3e818 100644
--- a/scripts/kconfig/lex.zconf.c_shipped
+++ b/scripts/kconfig/lex.zconf.c_shipped
@@ -2370,11 +2370,14 @@ void zconf_nextfile(const char *name)
 	current_buf = buf;
 
 	if (file->flags & FILE_BUSY) {
-		printf("recursive scan (%s)?\n", name);
+		printf("%s:%d: do not source '%s' from itself\n",
+		       zconf_curname(), zconf_lineno(), name);
 		exit(1);
 	}
 	if (file->flags & FILE_SCANNED) {
-		printf("file %s already scanned?\n", name);
+		printf("%s:%d: file '%s' is already sourced from '%s'\n",
+		       zconf_curname(), zconf_lineno(), name,
+		       file->parent->name);
 		exit(1);
 	}
 	file->flags |= FILE_BUSY;
diff --git a/scripts/kconfig/zconf.l b/scripts/kconfig/zconf.l
index 5164ef7..21ff69c 100644
--- a/scripts/kconfig/zconf.l
+++ b/scripts/kconfig/zconf.l
@@ -314,11 +314,14 @@ void zconf_nextfile(const char *name)
 	current_buf = buf;
 
 	if (file->flags & FILE_BUSY) {
-		printf("recursive scan (%s)?\n", name);
+		printf("%s:%d: do not source '%s' from itself\n",
+		       zconf_curname(), zconf_lineno(), name);
 		exit(1);
 	}
 	if (file->flags & FILE_SCANNED) {
-		printf("file %s already scanned?\n", name);
+		printf("%s:%d: file '%s' is already sourced from '%s'\n",
+		       zconf_curname(), zconf_lineno(), name,
+		       file->parent->name);
 		exit(1);
 	}
 	file->flags |= FILE_BUSY;
-- 
1.6.0.2.GIT