Improve dependency checks in kconfig
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
Added a few comments - no functional change.
--- scripts/kconfig/expr.h | 8 ++++++-- 1 files changed, 6 insertions(+), 2 deletions(-)
scripts/kconfig/expr.h +6 -2
@@ -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;
Signature · 1 line
1.6.0.2.GIT
PATCH 2/4 kconfig: add comments to symbol flags
In reply to Sam RavnborgNo functional changes - only comments.
--- scripts/kconfig/expr.h | 34 ++++++++++++++++++---------------- 1 files changed, 18 insertions(+), 16 deletions(-)
scripts/kconfig/expr.h +18 -16
@@ -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
Signature · 1 line
1.6.0.2.GIT
PATCH 3/4 kconfig: struct property commented
In reply to Sam RavnborgNo functional changes
--- scripts/kconfig/expr.h | 40 ++++++++++++++++++++++++++++++---------- 1 files changed, 30 insertions(+), 10 deletions(-)
scripts/kconfig/expr.h +30 -10
@@ -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) \
Signature · 1 line
1.6.0.2.GIT
PATCH 4/4 kconfig: improve readout when we hit recursive dependencies
In reply to Sam RavnborgFollowing sample:
config TEST1
bool
depends on TEST2 && PCI
select TEST2
config TEST2
boolWill 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.
--- scripts/kconfig/expr.h | 1 + scripts/kconfig/menu.c | 2 ++ scripts/kconfig/symbol.c | 11 ++++++++--- 3 files changed, 11 insertions(+), 3 deletions(-)
scripts/kconfig/expr.h +1 -0
@@ -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 {
scripts/kconfig/menu.c +2 -0
@@ -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 )
scripts/kconfig/symbol.c +8 -3
@@ -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 ; }
Signature · 1 line
1.6.0.2.GIT
Quoted 4 lines · Sam Ravnborg
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
In reply to Sam RavnborgFrom 7e9617872fe81a9515e8ce22c9ece8bd1e905b4c Mon Sep 17 00:00:00 2001
From: Sam Ravnborg <[email protected]>
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.
--- scripts/kconfig/symbol.c | 24 ++++++++++++++++++------ 1 files changed, 18 insertions(+), 6 deletions(-)
scripts/kconfig/symbol.c +18 -6
@@ -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 ; }
Signature · 1 line
1.6.0.2.GIT
PATCH 2/2 kconfig: improve error messages for bad source statements
In reply to Sam RavnborgFrom 31606ff6946f5fc841b9d990c6c5479f54f16835 Mon Sep 17 00:00:00 2001
From: Sam Ravnborg <[email protected]>
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.
--- scripts/kconfig/lex.zconf.c_shipped | 7 +++++-- scripts/kconfig/zconf.l | 7 +++++-- 2 files changed, 10 insertions(+), 4 deletions(-)
scripts/kconfig/lex.zconf.c_shipped +5 -2
@@ -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;
scripts/kconfig/zconf.l +5 -2
@@ -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;
Signature · 1 line
1.6.0.2.GIT