| Message ID | 20240611175536.3518179-7-masahiroy@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kconfig: fix choice value calculation with misc cleanups. | expand |
Hi Masahiro, kernel test robot noticed the following build warnings: [auto build test WARNING on masahiroy-kbuild/kbuild] [also build test WARNING on masahiroy-kbuild/for-next next-20240611] [cannot apply to masahiroy-kbuild/fixes linus/master v6.10-rc3] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Masahiro-Yamada/kconfig-remove-unneeded-code-in-expr_compare_type/20240612-020202 base: https://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git kbuild patch link: https://lore.kernel.org/r/20240611175536.3518179-7-masahiroy%40kernel.org patch subject: [PATCH 06/16] kconfig: refactor choice value calculation reproduce: (https://download.01.org/0day-ci/archive/20240612/202406120445.P5QmPYgD-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202406120445.P5QmPYgD-lkp@intel.com/ All warnings (new ones prefixed by >>): >> scripts/kconfig/symbol.c:448:3: warning: label followed by a declaration is a C23 extension [-Wc23-extensions] 448 | struct menu *choice_menu = sym_get_choice_menu(sym); | ^ 1 warning generated. vim +448 scripts/kconfig/symbol.c 398 399 void sym_calc_value(struct symbol *sym) 400 { 401 struct symbol_value newval, oldval; 402 struct property *prop; 403 404 if (!sym) 405 return; 406 407 if (sym->flags & SYMBOL_VALID) 408 return; 409 410 sym->flags |= SYMBOL_VALID; 411 412 oldval = sym->curr; 413 414 newval.tri = no; 415 416 switch (sym->type) { 417 case S_INT: 418 newval.val = "0"; 419 break; 420 case S_HEX: 421 newval.val = "0x0"; 422 break; 423 case S_STRING: 424 newval.val = ""; 425 break; 426 case S_BOOLEAN: 427 case S_TRISTATE: 428 newval.val = "n"; 429 break; 430 default: 431 sym->curr.val = sym->name; 432 sym->curr.tri = no; 433 return; 434 } 435 sym->flags &= ~SYMBOL_WRITE; 436 437 sym_calc_visibility(sym); 438 439 if (sym->visible != no) 440 sym->flags |= SYMBOL_WRITE; 441 442 /* set default if recursively called */ 443 sym->curr = newval; 444 445 switch (sym_get_type(sym)) { 446 case S_BOOLEAN: 447 case S_TRISTATE: > 448 struct menu *choice_menu = sym_get_choice_menu(sym); 449 450 if (choice_menu) { 451 sym_calc_choice(choice_menu); 452 newval.tri = sym->curr.tri; 453 } else { 454 if (sym->visible != no) { 455 /* if the symbol is visible use the user value 456 * if available, otherwise try the default value 457 */ 458 if (sym_has_value(sym)) { 459 newval.tri = EXPR_AND(sym->def[S_DEF_USER].tri, 460 sym->visible); 461 goto calc_newval; 462 } 463 } 464 if (sym->rev_dep.tri != no) 465 sym->flags |= SYMBOL_WRITE; 466 if (!sym_is_choice(sym)) { 467 prop = sym_get_default_prop(sym); 468 if (prop) { 469 newval.tri = EXPR_AND(expr_calc_value(prop->expr), 470 prop->visible.tri); 471 if (newval.tri != no) 472 sym->flags |= SYMBOL_WRITE; 473 } 474 if (sym->implied.tri != no) { 475 sym->flags |= SYMBOL_WRITE; 476 newval.tri = EXPR_OR(newval.tri, sym->implied.tri); 477 newval.tri = EXPR_AND(newval.tri, 478 sym->dir_dep.tri); 479 } 480 } 481 calc_newval: 482 if (sym->dir_dep.tri < sym->rev_dep.tri) 483 sym_warn_unmet_dep(sym); 484 newval.tri = EXPR_OR(newval.tri, sym->rev_dep.tri); 485 } 486 if (newval.tri == mod && sym_get_type(sym) == S_BOOLEAN) 487 newval.tri = yes; 488 break; 489 case S_STRING: 490 case S_HEX: 491 case S_INT: 492 if (sym->visible != no && sym_has_value(sym)) { 493 newval.val = sym->def[S_DEF_USER].val; 494 break; 495 } 496 prop = sym_get_default_prop(sym); 497 if (prop) { 498 struct symbol *ds = prop_get_symbol(prop); 499 if (ds) { 500 sym->flags |= SYMBOL_WRITE; 501 sym_calc_value(ds); 502 newval.val = ds->curr.val; 503 } 504 } 505 break; 506 default: 507 ; 508 } 509 510 sym->curr = newval; 511 sym_validate_range(sym); 512 513 if (memcmp(&oldval, &sym->curr, sizeof(oldval))) { 514 sym_set_changed(sym); 515 if (modules_sym == sym) { 516 sym_set_all_changed(); 517 modules_val = modules_sym->curr.tri; 518 } 519 } 520 521 if (sym_is_choice(sym)) 522 sym->flags &= ~SYMBOL_WRITE; 523 } 524
Hi Masahiro, kernel test robot noticed the following build errors: [auto build test ERROR on masahiroy-kbuild/kbuild] [also build test ERROR on masahiroy-kbuild/for-next next-20240611] [cannot apply to masahiroy-kbuild/fixes linus/master v6.10-rc3] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Masahiro-Yamada/kconfig-remove-unneeded-code-in-expr_compare_type/20240612-020202 base: https://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git kbuild patch link: https://lore.kernel.org/r/20240611175536.3518179-7-masahiroy%40kernel.org patch subject: [PATCH 06/16] kconfig: refactor choice value calculation config: i386-buildonly-randconfig-002-20240612 (attached as .config) compiler: gcc-8 (Ubuntu 8.4.0-3ubuntu2) 8.4.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240612/202406121008.8zFuX4VH-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202406121008.8zFuX4VH-lkp@intel.com/ All errors (new ones prefixed by >>): scripts/kconfig/symbol.c: In function 'sym_calc_value': >> scripts/kconfig/symbol.c:448:3: error: a label can only be part of a statement and a declaration is not a statement struct menu *choice_menu = sym_get_choice_menu(sym); ^~~~~~ make[3]: *** [scripts/Makefile.host:133: scripts/kconfig/symbol.o] Error 1 shuffle=1413228972 make[3]: Target 'oldconfig' not remade because of errors. make[2]: *** [Makefile:695: oldconfig] Error 2 shuffle=1413228972 make[1]: *** [Makefile:240: __sub-make] Error 2 shuffle=1413228972 make[1]: Target 'oldconfig' not remade because of errors. make: *** [Makefile:240: __sub-make] Error 2 shuffle=1413228972 make: Target 'oldconfig' not remade because of errors. -- scripts/kconfig/symbol.c: In function 'sym_calc_value': >> scripts/kconfig/symbol.c:448:3: error: a label can only be part of a statement and a declaration is not a statement struct menu *choice_menu = sym_get_choice_menu(sym); ^~~~~~ make[3]: *** [scripts/Makefile.host:133: scripts/kconfig/symbol.o] Error 1 shuffle=1413228972 make[3]: Target 'olddefconfig' not remade because of errors. make[2]: *** [Makefile:695: olddefconfig] Error 2 shuffle=1413228972 make[1]: *** [Makefile:240: __sub-make] Error 2 shuffle=1413228972 make[1]: Target 'olddefconfig' not remade because of errors. make: *** [Makefile:240: __sub-make] Error 2 shuffle=1413228972 make: Target 'olddefconfig' not remade because of errors. vim +448 scripts/kconfig/symbol.c 398 399 void sym_calc_value(struct symbol *sym) 400 { 401 struct symbol_value newval, oldval; 402 struct property *prop; 403 404 if (!sym) 405 return; 406 407 if (sym->flags & SYMBOL_VALID) 408 return; 409 410 sym->flags |= SYMBOL_VALID; 411 412 oldval = sym->curr; 413 414 newval.tri = no; 415 416 switch (sym->type) { 417 case S_INT: 418 newval.val = "0"; 419 break; 420 case S_HEX: 421 newval.val = "0x0"; 422 break; 423 case S_STRING: 424 newval.val = ""; 425 break; 426 case S_BOOLEAN: 427 case S_TRISTATE: 428 newval.val = "n"; 429 break; 430 default: 431 sym->curr.val = sym->name; 432 sym->curr.tri = no; 433 return; 434 } 435 sym->flags &= ~SYMBOL_WRITE; 436 437 sym_calc_visibility(sym); 438 439 if (sym->visible != no) 440 sym->flags |= SYMBOL_WRITE; 441 442 /* set default if recursively called */ 443 sym->curr = newval; 444 445 switch (sym_get_type(sym)) { 446 case S_BOOLEAN: 447 case S_TRISTATE: > 448 struct menu *choice_menu = sym_get_choice_menu(sym); 449 450 if (choice_menu) { 451 sym_calc_choice(choice_menu); 452 newval.tri = sym->curr.tri; 453 } else { 454 if (sym->visible != no) { 455 /* if the symbol is visible use the user value 456 * if available, otherwise try the default value 457 */ 458 if (sym_has_value(sym)) { 459 newval.tri = EXPR_AND(sym->def[S_DEF_USER].tri, 460 sym->visible); 461 goto calc_newval; 462 } 463 } 464 if (sym->rev_dep.tri != no) 465 sym->flags |= SYMBOL_WRITE; 466 if (!sym_is_choice(sym)) { 467 prop = sym_get_default_prop(sym); 468 if (prop) { 469 newval.tri = EXPR_AND(expr_calc_value(prop->expr), 470 prop->visible.tri); 471 if (newval.tri != no) 472 sym->flags |= SYMBOL_WRITE; 473 } 474 if (sym->implied.tri != no) { 475 sym->flags |= SYMBOL_WRITE; 476 newval.tri = EXPR_OR(newval.tri, sym->implied.tri); 477 newval.tri = EXPR_AND(newval.tri, 478 sym->dir_dep.tri); 479 } 480 } 481 calc_newval: 482 if (sym->dir_dep.tri < sym->rev_dep.tri) 483 sym_warn_unmet_dep(sym); 484 newval.tri = EXPR_OR(newval.tri, sym->rev_dep.tri); 485 } 486 if (newval.tri == mod && sym_get_type(sym) == S_BOOLEAN) 487 newval.tri = yes; 488 break; 489 case S_STRING: 490 case S_HEX: 491 case S_INT: 492 if (sym->visible != no && sym_has_value(sym)) { 493 newval.val = sym->def[S_DEF_USER].val; 494 break; 495 } 496 prop = sym_get_default_prop(sym); 497 if (prop) { 498 struct symbol *ds = prop_get_symbol(prop); 499 if (ds) { 500 sym->flags |= SYMBOL_WRITE; 501 sym_calc_value(ds); 502 newval.val = ds->curr.val; 503 } 504 } 505 break; 506 default: 507 ; 508 } 509 510 sym->curr = newval; 511 sym_validate_range(sym); 512 513 if (memcmp(&oldval, &sym->curr, sizeof(oldval))) { 514 sym_set_changed(sym); 515 if (modules_sym == sym) { 516 sym_set_all_changed(); 517 modules_val = modules_sym->curr.tri; 518 } 519 } 520 521 if (sym_is_choice(sym)) 522 sym->flags &= ~SYMBOL_WRITE; 523 } 524
On Wed, Jun 12, 2024 at 6:19 AM kernel test robot <lkp@intel.com> wrote: > > Hi Masahiro, > > kernel test robot noticed the following build warnings: > > [auto build test WARNING on masahiroy-kbuild/kbuild] > [also build test WARNING on masahiroy-kbuild/for-next next-20240611] > [cannot apply to masahiroy-kbuild/fixes linus/master v6.10-rc3] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/Masahiro-Yamada/kconfig-remove-unneeded-code-in-expr_compare_type/20240612-020202 > base: https://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git kbuild > patch link: https://lore.kernel.org/r/20240611175536.3518179-7-masahiroy%40kernel.org > patch subject: [PATCH 06/16] kconfig: refactor choice value calculation > reproduce: (https://download.01.org/0day-ci/archive/20240612/202406120445.P5QmPYgD-lkp@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202406120445.P5QmPYgD-lkp@intel.com/ > > All warnings (new ones prefixed by >>): > > >> scripts/kconfig/symbol.c:448:3: warning: label followed by a declaration is a C23 extension [-Wc23-extensions] > 448 | struct menu *choice_menu = sym_get_choice_menu(sym); > | ^ > 1 warning generated. > OK. I will move it to the top of the function. iff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c index b4d085342b94..063a478197e0 100644 --- a/scripts/kconfig/symbol.c +++ b/scripts/kconfig/symbol.c @@ -383,6 +383,7 @@ void sym_calc_value(struct symbol *sym) { struct symbol_value newval, oldval; struct property *prop; + struct menu *choice_menu; if (!sym) return; @@ -428,7 +429,7 @@ void sym_calc_value(struct symbol *sym) switch (sym_get_type(sym)) { case S_BOOLEAN: case S_TRISTATE: - struct menu *choice_menu = sym_get_choice_menu(sym); + choice_menu = sym_get_choice_menu(sym); if (choice_menu) { sym_calc_choice(choice_menu);
On Wed, Jun 12, 2024 at 2:56 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > Handling choices has always been in a PITA in Kconfig. > > For example, fixes and reverts were repeated for randconfig with > KCONFIG_ALLCONFIG: > > - 422c809f03f0 ("kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG") > - 23a5dfdad22a ("Revert "kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG"") > - 8357b48549e1 ("kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG") > - 490f16171119 ("Revert "kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG"") > > As these commits pointed out, randconfig does not randomize choices when > KCONFIG_ALLCONFIG is used. This issue still remains. > > [Test Case] > > choice > prompt "choose" > > config A > bool "A" > > config B > bool "B" > > endchoice > > $ echo > all.config > $ make KCONFIG_ALLCONFIG=1 randconfig > > The output is always as follows: > > CONFIG_A=y > # CONFIG_B is not set > > Not only randconfig, but other all*config variants are broken with > KCONFIG_ALLCONFIG. > > With the same Kconfig, > > $ echo '# CONFIG_A is not set' > all.config > $ make KCONFIG_ALLCONFIG=1 allyesconfig > > You will get this: > > CONFIG_A=y > # CONFIG_B is not set > > This is incorrect because it does not respect all.config. > > The correct output should be: > > # CONFIG_A is not set > CONFIG_B=y > > To handle user inputs more accurately, this commit refactors the code > based on the following principles: > > - When a user value is given, Kconfig must set it immediately. > Do not defer it by setting SYMBOL_NEED_SET_CHOICE_VALUES. > > - The SYMBOL_DEF_USER flag must not be cleared, unless a new config > file is loaded. Kconfig must not forget user inputs. > > In addition, user values for choices must be managed with priority. > If user inputs conflict within a choice block, the newest value wins. > The values given by randconfig have lower priority than explicit user > inputs. > > This commit implements it by using a linked list. Every time a choice > block gets a new input, it is moved to the top of the list. > > Let me explain how it works. > > Let's say, we have a choice block that consists of three symbols: > A, B, and C. > > Initially, the linked list looks like this: > > A(=?) --> B(=?) --> C(=?) > > Say, '# CONFIG_B is not set' is specified by KCONFIG_ALLCONFIG. > > B is set to 'n', and moved to the top of the linked list: > > B(=n) --> A(=?) --> C(=?) > > The randconfig shuffles the symbols without a user value. > > So, you will get: > > B(=n) --> A(=y) --> C(=y) > or > B(=n) --> C(=y) --> A(=y) > > When calculating the output, the linked list is traversed. The first > visible symbol with =y is taken. You will get either CONFIG_A=y or > CONFIG_C=y with equal probability. > > As another example, let's say the .config with the following content > is loaded: > > CONFIG_B=y > CONFIG_C=y > > The linked list will become: > > C(=y) --> B(=y) --> A(=?) > > Please note the last one is prioritized when a decision conflicts in > the same file. This is reasonable behavior because merge_config.sh > appends config fragments to the existing .config file. > > So, the output will be CONFIG_C=y if C is visible, but otherwise > CONFIG_B=y. > > This is different from the former implementation; previously, Kconfig > forgot CONFIG_B=y when CONFIG_C=y appeared later in the same file. > > In the new implementation, Kconfig remembers both CONFIG_B=y and > CONFIG_C=y, prioritizing the former. prioritizing the latter. > If C is hidden due to unmet > dependency, CONFIG_B=y arises as the second best. > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> > ---
diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c index 5dbdd9459f21..1c59998a62f7 100644 --- a/scripts/kconfig/conf.c +++ b/scripts/kconfig/conf.c @@ -114,41 +114,54 @@ static void set_randconfig_seed(void) srand(seed); } -static void randomize_choice_values(struct symbol *csym) +/** + * randomize_choice_values - randomize choice block + * + * @choice: menu entry for the choice + */ +static void randomize_choice_values(struct menu *choice) { - struct property *prop; - struct symbol *sym; - struct expr *e; - int cnt, def; - - prop = sym_get_choice_prop(csym); - - /* count entries in choice block */ - cnt = 0; - expr_list_for_each_sym(prop->expr, e, sym) - cnt++; + struct menu *menu; + int x; + int cnt = 0; /* - * find a random value and set it to yes, - * set the rest to no so we have only one set + * First, count the number of symbols to randomize. If sym_has_value() + * is true, it was specified by KCONFIG_ALLCONFIG. It needs to be + * respected. */ - def = rand() % cnt; + menu_for_each_sub_entry(menu, choice) { + struct symbol *sym = menu->sym; - cnt = 0; - expr_list_for_each_sym(prop->expr, e, sym) { - if (def == cnt++) { - sym->def[S_DEF_USER].tri = yes; - csym->def[S_DEF_USER].val = sym; - } else { - sym->def[S_DEF_USER].tri = no; - } - sym->flags |= SYMBOL_DEF_USER; - /* clear VALID to get value calculated */ - sym->flags &= ~SYMBOL_VALID; + if (sym && !sym_has_value(sym)) + cnt++; + } + + while (cnt > 0) { + x = rand() % cnt; + + menu_for_each_sub_entry(menu, choice) { + struct symbol *sym = menu->sym; + + if (sym && !sym_has_value(sym)) + x--; + + if (x < 0) { + sym->def[S_DEF_USER].tri = yes; + sym->flags |= SYMBOL_DEF_USER; + /* + * Move the selected item to the _tail_ because + * this needs to have a lower priority than the + * user input from KCONFIG_ALLCONFIG. + */ + list_move_tail(&sym->choice_link, + &choice->choice_members); + + break; + } + } + cnt--; } - csym->flags |= SYMBOL_DEF_USER; - /* clear VALID to get value calculated */ - csym->flags &= ~SYMBOL_VALID; } enum conf_def_mode { @@ -159,9 +172,9 @@ enum conf_def_mode { def_random }; -static bool conf_set_all_new_symbols(enum conf_def_mode mode) +static void conf_set_all_new_symbols(enum conf_def_mode mode) { - struct symbol *sym, *csym; + struct menu *menu; int cnt; /* * can't go as the default in switch-case below, otherwise gcc whines @@ -170,7 +183,6 @@ static bool conf_set_all_new_symbols(enum conf_def_mode mode) int pby = 50; /* probability of bool = y */ int pty = 33; /* probability of tristate = y */ int ptm = 33; /* probability of tristate = m */ - bool has_changed = false; if (mode == def_random) { int n, p[3]; @@ -217,14 +229,21 @@ static bool conf_set_all_new_symbols(enum conf_def_mode mode) } } - for_all_symbols(sym) { + menu_for_each_entry(menu) { + struct symbol *sym = menu->sym; tristate val; - if (sym_has_value(sym) || sym->flags & SYMBOL_VALID || - (sym->type != S_BOOLEAN && sym->type != S_TRISTATE)) + if (!sym || !menu->prompt || sym_has_value(sym) || + (sym->type != S_BOOLEAN && sym->type != S_TRISTATE) || + sym_is_choice_value(sym)) continue; - has_changed = true; + if (sym_is_choice(sym)) { + if (mode == def_random) + randomize_choice_values(menu); + continue; + } + switch (mode) { case def_yes: val = yes; @@ -251,34 +270,10 @@ static bool conf_set_all_new_symbols(enum conf_def_mode mode) continue; } sym->def[S_DEF_USER].tri = val; - - if (!(sym_is_choice(sym) && mode == def_random)) - sym->flags |= SYMBOL_DEF_USER; + sym->flags |= SYMBOL_DEF_USER; } sym_clear_all_valid(); - - if (mode != def_random) { - for_all_symbols(csym) { - if ((sym_is_choice(csym) && !sym_has_value(csym)) || - sym_is_choice_value(csym)) - csym->flags |= SYMBOL_NEED_SET_CHOICE_VALUES; - } - } - - for_all_symbols(csym) { - if (sym_has_value(csym) || !sym_is_choice(csym)) - continue; - - sym_calc_value(csym); - if (mode == def_random) - randomize_choice_values(csym); - else - set_all_choice_values(csym); - has_changed = true; - } - - return has_changed; } static void conf_rewrite_tristates(tristate old_val, tristate new_val) @@ -429,10 +424,9 @@ static void conf_choice(struct menu *menu) { struct symbol *sym, *def_sym; struct menu *child; - bool is_new; + bool is_new = false; sym = menu->sym; - is_new = !sym_has_value(sym); while (1) { int cnt, def; @@ -456,8 +450,10 @@ static void conf_choice(struct menu *menu) printf("%*c", indent, ' '); printf(" %d. %s (%s)", cnt, menu_get_prompt(child), child->sym->name); - if (!sym_has_value(child->sym)) + if (!sym_has_value(child->sym)) { + is_new = true; printf(" (NEW)"); + } printf("\n"); } printf("%*schoice", indent - 1, ""); @@ -586,9 +582,7 @@ static void check_conf(struct menu *menu) return; sym = menu->sym; - if (sym && !sym_has_value(sym) && - (sym_is_changeable(sym) || sym_is_choice(sym))) { - + if (sym && !sym_has_value(sym) && sym_is_changeable(sym)) { switch (input_mode) { case listnewconfig: if (sym->name) @@ -804,8 +798,7 @@ int main(int ac, char **av) conf_set_all_new_symbols(def_default); break; case randconfig: - /* Really nothing to do in this loop */ - while (conf_set_all_new_symbols(def_random)) ; + conf_set_all_new_symbols(def_random); break; case defconfig: conf_set_all_new_symbols(def_default); diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c index 1ac7fc9ad756..05823f85402a 100644 --- a/scripts/kconfig/confdata.c +++ b/scripts/kconfig/confdata.c @@ -382,10 +382,7 @@ int conf_read_simple(const char *name, int def) def_flags = SYMBOL_DEF << def; for_all_symbols(sym) { - sym->flags |= SYMBOL_CHANGED; sym->flags &= ~(def_flags|SYMBOL_VALID); - if (sym_is_choice(sym)) - sym->flags |= def_flags; switch (sym->type) { case S_INT: case S_HEX: @@ -399,6 +396,8 @@ int conf_read_simple(const char *name, int def) } while (getline_stripped(&line, &line_asize, in) != -1) { + struct menu *choice; + conf_lineno++; if (!line[0]) /* blank line */ @@ -460,15 +459,14 @@ int conf_read_simple(const char *name, int def) if (conf_set_sym_val(sym, def, def_flags, val)) continue; - if (sym && sym_is_choice_value(sym)) { - struct symbol *cs = prop_get_symbol(sym_get_choice_prop(sym)); - if (sym->def[def].tri == yes) { - if (cs->def[def].tri != no) - conf_warning("override: %s changes choice state", sym->name); - cs->def[def].val = sym; - cs->def[def].tri = yes; - } - } + /* + * If this is a choice member, give it the highest priority. + * If conflicting CONFIG options are given from an input file, + * the last one wins. + */ + choice = sym_get_choice_menu(sym); + if (choice) + list_move(&sym->choice_link, &choice->choice_members); } free(line); fclose(in); @@ -514,18 +512,6 @@ int conf_read(const char *name) /* maybe print value in verbose mode... */ } - for_all_symbols(sym) { - if (sym_has_value(sym) && !sym_is_choice_value(sym)) { - /* Reset values of generates values, so they'll appear - * as new, if they should become visible, but that - * doesn't quite work if the Kconfig and the saved - * configuration disagree. - */ - if (sym->visible == no && !conf_unsaved) - sym->flags &= ~SYMBOL_DEF_USER; - } - } - if (conf_warnings || conf_unsaved) conf_set_changed(true); @@ -1146,23 +1132,3 @@ void conf_set_changed_callback(void (*fn)(bool)) { conf_changed_callback = fn; } - -void set_all_choice_values(struct symbol *csym) -{ - struct property *prop; - struct symbol *sym; - struct expr *e; - - prop = sym_get_choice_prop(csym); - - /* - * Set all non-assinged choice values to no - */ - expr_list_for_each_sym(prop->expr, e, sym) { - if (!sym_has_value(sym)) - sym->def[S_DEF_USER].tri = no; - } - csym->flags |= SYMBOL_DEF_USER; - /* clear VALID to get value calculated */ - csym->flags &= ~(SYMBOL_VALID | SYMBOL_NEED_SET_CHOICE_VALUES); -} diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h index 7c0c242318bc..7acf27a4f454 100644 --- a/scripts/kconfig/expr.h +++ b/scripts/kconfig/expr.h @@ -73,6 +73,8 @@ enum { * Represents a configuration symbol. * * Choices are represented as a special kind of symbol with null name. + * + * @choice_link: linked to menu::choice_members */ struct symbol { /* link node for the hash table */ @@ -110,6 +112,8 @@ struct symbol { /* config entries associated with this symbol */ struct list_head menus; + struct list_head choice_link; + /* SYMBOL_* flags */ int flags; @@ -133,7 +137,6 @@ struct symbol { #define SYMBOL_CHOICEVAL 0x0020 /* used as a value in a choice block */ #define SYMBOL_VALID 0x0080 /* set when symbol.curr is calculated */ #define SYMBOL_WRITE 0x0200 /* write symbol to file (KCONFIG_CONFIG) */ -#define SYMBOL_CHANGED 0x0400 /* ? */ #define SYMBOL_WRITTEN 0x0800 /* track info to avoid double-write to .config */ #define SYMBOL_CHECKED 0x2000 /* used during dependency checking */ #define SYMBOL_WARNED 0x8000 /* warning has been issued */ @@ -145,9 +148,6 @@ struct symbol { #define SYMBOL_DEF3 0x40000 /* symbol.def[S_DEF_3] is valid */ #define SYMBOL_DEF4 0x80000 /* symbol.def[S_DEF_4] is valid */ -/* choice values need to be set before calculating this symbol value */ -#define SYMBOL_NEED_SET_CHOICE_VALUES 0x100000 - #define SYMBOL_MAXLENGTH 256 /* A property represent the config options that can be associated @@ -204,6 +204,8 @@ struct property { * for all front ends). Each symbol, menu, etc. defined in the Kconfig files * gets a node. A symbol defined in multiple locations gets one node at each * location. + * + * @choice_members: list of choice members with priority. */ struct menu { /* The next menu node at the same level */ @@ -223,6 +225,8 @@ struct menu { struct list_head link; /* link to symbol::menus */ + struct list_head choice_members; + /* * The prompt associated with the node. This holds the prompt for a * symbol as well as the text for a menu or comment, along with the diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h index 64dfc354dd5c..bdd37a16b040 100644 --- a/scripts/kconfig/lkc.h +++ b/scripts/kconfig/lkc.h @@ -40,7 +40,6 @@ void zconf_nextfile(const char *name); /* confdata.c */ extern struct gstr autoconf_cmd; const char *conf_get_configname(void); -void set_all_choice_values(struct symbol *csym); /* confdata.c and expr.c */ static inline void xfwrite(const void *str, size_t len, size_t count, FILE *out) @@ -121,11 +120,7 @@ static inline tristate sym_get_tristate_value(struct symbol *sym) return sym->curr.tri; } - -static inline struct symbol *sym_get_choice_value(struct symbol *sym) -{ - return (struct symbol *)sym->curr.val; -} +struct symbol *sym_get_choice_value(struct symbol *sym); static inline bool sym_is_choice(struct symbol *sym) { diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c index bf5dcc05350b..170a269a8d7c 100644 --- a/scripts/kconfig/menu.c +++ b/scripts/kconfig/menu.c @@ -591,7 +591,6 @@ bool menu_is_empty(struct menu *menu) bool menu_is_visible(struct menu *menu) { - struct menu *child; struct symbol *sym; tristate visible; @@ -610,21 +609,7 @@ bool menu_is_visible(struct menu *menu) } else visible = menu->prompt->visible.tri = expr_calc_value(menu->prompt->visible.expr); - if (visible != no) - return true; - - if (!sym || sym_get_tristate_value(menu->sym) == no) - return false; - - for (child = menu->list; child; child = child->next) { - if (menu_is_visible(child)) { - if (sym) - sym->flags |= SYMBOL_DEF_USER; - return true; - } - } - - return false; + return visible != no; } const char *menu_get_prompt(struct menu *menu) diff --git a/scripts/kconfig/parser.y b/scripts/kconfig/parser.y index 20538e1d3788..9d58544b0255 100644 --- a/scripts/kconfig/parser.y +++ b/scripts/kconfig/parser.y @@ -157,6 +157,9 @@ config_stmt: config_entry_start config_option_list current_entry->filename, current_entry->lineno); yynerrs++; } + + list_add_tail(¤t_entry->sym->choice_link, + ¤t_choice->choice_members); } printd(DEBUG_PARSE, "%s:%d:endconfig\n", cur_filename, cur_lineno); @@ -240,6 +243,7 @@ choice: T_CHOICE T_EOL menu_add_entry(sym); menu_add_expr(P_CHOICE, NULL, NULL); menu_set_type(S_BOOLEAN); + INIT_LIST_HEAD(¤t_entry->choice_members); printd(DEBUG_PARSE, "%s:%d:choice\n", cur_filename, cur_lineno); }; diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c index 8df0a75f40b9..e59f2d2ce4e6 100644 --- a/scripts/kconfig/symbol.c +++ b/scripts/kconfig/symbol.c @@ -188,7 +188,6 @@ static void sym_set_changed(struct symbol *sym) { struct menu *menu; - sym->flags |= SYMBOL_CHANGED; list_for_each_entry(menu, &sym->menus, link) menu->flags |= MENU_CHANGED; } @@ -282,36 +281,90 @@ struct symbol *sym_choice_default(struct symbol *sym) return NULL; } -static struct symbol *sym_calc_choice(struct symbol *sym) +/* + * sym_calc_choice - calculate symbol values in a choice + * + * @choice: a menu of the choice + * + * Return: a chosen symbol + */ +static struct symbol *sym_calc_choice(struct menu *choice) { - struct symbol *def_sym; - struct property *prop; - struct expr *e; - int flags; + struct symbol *res = NULL; + struct symbol *sym; + struct menu *menu; - /* first calculate all choice values' visibilities */ - flags = sym->flags; - prop = sym_get_choice_prop(sym); - expr_list_for_each_sym(prop->expr, e, def_sym) { - sym_calc_visibility(def_sym); - if (def_sym->visible != no) - flags &= def_sym->flags; + /* Traverse the list of choice members in the priority order. */ + list_for_each_entry(sym, &choice->choice_members, choice_link) { + sym_calc_visibility(sym); + if (sym->visible == no) + continue; + + /* The first visible symble with the user value 'y'. */ + if (sym_has_value(sym) && sym->def[S_DEF_USER].tri == yes) { + res = sym; + break; + } } - sym->flags &= flags | ~SYMBOL_DEF_USER; + /* If 'y' is not found in the user input, try the default */ + if (!res) { + res = sym_choice_default(choice->sym); + if (res && sym_has_value(res) && res->def[S_DEF_USER].tri == no) + res = NULL; + } - /* is the user choice visible? */ - def_sym = sym->def[S_DEF_USER].val; - if (def_sym && def_sym->visible != no) - return def_sym; + /* Still not found. Pick up the first visible, user-unspecified symbol. */ + if (!res) { + menu_for_each_sub_entry(menu, choice) { + sym = menu->sym; - def_sym = sym_choice_default(sym); + if (!sym || sym->visible == no || sym_has_value(sym)) + continue; - if (def_sym == NULL) - /* no choice? reset tristate value */ - sym->curr.tri = no; + res = sym; + break; + } + } - return def_sym; + /* Still not found. Pick up the first visible symbol. */ + if (!res) { + menu_for_each_sub_entry(menu, choice) { + sym = menu->sym; + + if (!sym || sym->visible == no) + continue; + + res = sym; + break; + } + } + + menu_for_each_sub_entry(menu, choice) { + tristate val; + + sym = menu->sym; + + if (!sym || sym->visible == no) + continue; + + val = sym == res ? yes : no; + + if (sym->curr.tri != val) + sym_set_changed(sym); + + sym->curr.tri = val; + sym->flags |= SYMBOL_VALID | SYMBOL_WRITE; + } + + return res; +} + +struct symbol *sym_get_choice_value(struct symbol *sym) +{ + struct menu *menu = list_first_entry(&sym->menus, struct menu, link); + + return sym_calc_choice(menu); } static void sym_warn_unmet_dep(struct symbol *sym) @@ -347,7 +400,6 @@ void sym_calc_value(struct symbol *sym) { struct symbol_value newval, oldval; struct property *prop; - struct expr *e; if (!sym) return; @@ -355,13 +407,6 @@ void sym_calc_value(struct symbol *sym) if (sym->flags & SYMBOL_VALID) return; - if (sym_is_choice_value(sym) && - sym->flags & SYMBOL_NEED_SET_CHOICE_VALUES) { - sym->flags &= ~SYMBOL_NEED_SET_CHOICE_VALUES; - prop = sym_get_choice_prop(sym); - sym_calc_value(prop_get_symbol(prop)); - } - sym->flags |= SYMBOL_VALID; oldval = sym->curr; @@ -400,9 +445,11 @@ void sym_calc_value(struct symbol *sym) switch (sym_get_type(sym)) { case S_BOOLEAN: case S_TRISTATE: - if (sym_is_choice_value(sym) && sym->visible == yes) { - prop = sym_get_choice_prop(sym); - newval.tri = (prop_get_symbol(prop)->curr.val == sym) ? yes : no; + struct menu *choice_menu = sym_get_choice_menu(sym); + + if (choice_menu) { + sym_calc_choice(choice_menu); + newval.tri = sym->curr.tri; } else { if (sym->visible != no) { /* if the symbol is visible use the user value @@ -461,8 +508,6 @@ void sym_calc_value(struct symbol *sym) } sym->curr = newval; - if (sym_is_choice(sym) && newval.tri == yes) - sym->curr.val = sym_calc_choice(sym); sym_validate_range(sym); if (memcmp(&oldval, &sym->curr, sizeof(oldval))) { @@ -473,23 +518,8 @@ void sym_calc_value(struct symbol *sym) } } - if (sym_is_choice(sym)) { - struct symbol *choice_sym; - - prop = sym_get_choice_prop(sym); - expr_list_for_each_sym(prop->expr, e, choice_sym) { - if ((sym->flags & SYMBOL_WRITE) && - choice_sym->visible != no) - choice_sym->flags |= SYMBOL_WRITE; - if (sym->flags & SYMBOL_CHANGED) - sym_set_changed(choice_sym); - } - + if (sym_is_choice(sym)) sym->flags &= ~SYMBOL_WRITE; - } - - if (sym->flags & SYMBOL_NEED_SET_CHOICE_VALUES) - set_all_choice_values(sym); } void sym_clear_all_valid(void) @@ -523,15 +553,15 @@ bool sym_set_tristate_value(struct symbol *sym, tristate val) { tristate oldval = sym_get_tristate_value(sym); - if (oldval != val && !sym_tristate_within_range(sym, val)) + if (!sym_tristate_within_range(sym, val)) return false; - if (!(sym->flags & SYMBOL_DEF_USER)) { + if (!(sym->flags & SYMBOL_DEF_USER) || sym->def[S_DEF_USER].tri != val) { + sym->def[S_DEF_USER].tri = val; sym->flags |= SYMBOL_DEF_USER; sym_set_changed(sym); } - sym->def[S_DEF_USER].tri = val; if (oldval != val) sym_clear_all_valid(); @@ -565,10 +595,13 @@ void choice_set_value(struct menu *choice, struct symbol *sym) menu->sym->def[S_DEF_USER].tri = val; menu->sym->flags |= SYMBOL_DEF_USER; - } - choice->sym->def[S_DEF_USER].val = sym; - choice->sym->flags |= SYMBOL_DEF_USER; + /* + * Now, the user has explicitly enabled or disabled this symbol, + * it should be given the highest priority + */ + list_move(&menu->sym->choice_link, &choice->choice_members); + } if (changed) sym_clear_all_valid();
Handling choices has always been in a PITA in Kconfig. For example, fixes and reverts were repeated for randconfig with KCONFIG_ALLCONFIG: - 422c809f03f0 ("kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG") - 23a5dfdad22a ("Revert "kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG"") - 8357b48549e1 ("kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG") - 490f16171119 ("Revert "kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG"") As these commits pointed out, randconfig does not randomize choices when KCONFIG_ALLCONFIG is used. This issue still remains. [Test Case] choice prompt "choose" config A bool "A" config B bool "B" endchoice $ echo > all.config $ make KCONFIG_ALLCONFIG=1 randconfig The output is always as follows: CONFIG_A=y # CONFIG_B is not set Not only randconfig, but other all*config variants are broken with KCONFIG_ALLCONFIG. With the same Kconfig, $ echo '# CONFIG_A is not set' > all.config $ make KCONFIG_ALLCONFIG=1 allyesconfig You will get this: CONFIG_A=y # CONFIG_B is not set This is incorrect because it does not respect all.config. The correct output should be: # CONFIG_A is not set CONFIG_B=y To handle user inputs more accurately, this commit refactors the code based on the following principles: - When a user value is given, Kconfig must set it immediately. Do not defer it by setting SYMBOL_NEED_SET_CHOICE_VALUES. - The SYMBOL_DEF_USER flag must not be cleared, unless a new config file is loaded. Kconfig must not forget user inputs. In addition, user values for choices must be managed with priority. If user inputs conflict within a choice block, the newest value wins. The values given by randconfig have lower priority than explicit user inputs. This commit implements it by using a linked list. Every time a choice block gets a new input, it is moved to the top of the list. Let me explain how it works. Let's say, we have a choice block that consists of three symbols: A, B, and C. Initially, the linked list looks like this: A(=?) --> B(=?) --> C(=?) Say, '# CONFIG_B is not set' is specified by KCONFIG_ALLCONFIG. B is set to 'n', and moved to the top of the linked list: B(=n) --> A(=?) --> C(=?) The randconfig shuffles the symbols without a user value. So, you will get: B(=n) --> A(=y) --> C(=y) or B(=n) --> C(=y) --> A(=y) When calculating the output, the linked list is traversed. The first visible symbol with =y is taken. You will get either CONFIG_A=y or CONFIG_C=y with equal probability. As another example, let's say the .config with the following content is loaded: CONFIG_B=y CONFIG_C=y The linked list will become: C(=y) --> B(=y) --> A(=?) Please note the last one is prioritized when a decision conflicts in the same file. This is reasonable behavior because merge_config.sh appends config fragments to the existing .config file. So, the output will be CONFIG_C=y if C is visible, but otherwise CONFIG_B=y. This is different from the former implementation; previously, Kconfig forgot CONFIG_B=y when CONFIG_C=y appeared later in the same file. In the new implementation, Kconfig remembers both CONFIG_B=y and CONFIG_C=y, prioritizing the former. If C is hidden due to unmet dependency, CONFIG_B=y arises as the second best. Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> --- scripts/kconfig/conf.c | 131 +++++++++++++++----------------- scripts/kconfig/confdata.c | 54 +++----------- scripts/kconfig/expr.h | 12 ++- scripts/kconfig/lkc.h | 7 +- scripts/kconfig/menu.c | 17 +---- scripts/kconfig/parser.y | 4 + scripts/kconfig/symbol.c | 149 ++++++++++++++++++++++--------------- 7 files changed, 177 insertions(+), 197 deletions(-)