| Message ID | 20180207225551.imdp6gqbdwzyvhwm@huvuddator (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2018-02-08 7:55 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: > On Tue, Feb 06, 2018 at 09:34:42AM +0900, Masahiro Yamada wrote: >> "# CONFIG_... is not set" for choice values are wrongly written into >> the .config file if they are once visible, then become invisible later. >> >> Test case >> --------- >> >> ---------------------------(Kconfig)---------------------------- >> config A >> bool "A" >> >> choice >> prompt "Choice ?" >> depends on A >> >> config CHOICE_B >> bool "Choice B" >> >> config CHOICE_C >> bool "Choice C" >> >> endchoice >> ---------------------------------------------------------------- >> >> ---------------------------(.config)---------------------------- >> CONFIG_A=y >> ---------------------------------------------------------------- >> >> With the Kconfig and .config above, >> >> $ make config >> scripts/kconfig/conf --oldaskconfig Kconfig >> * >> * Linux Kernel Configuration >> * >> A (A) [Y/n] n >> # >> # configuration written to .config >> # >> $ cat .config >> # >> # Automatically generated file; DO NOT EDIT. >> # Linux Kernel Configuration >> # >> # CONFIG_A is not set >> # CONFIG_CHOICE_B is not set >> # CONFIG_CHOICE_C is not set >> >> Here, >> >> # CONFIG_CHOICE_B is not set >> # CONFIG_CHOICE_C is not set >> >> should not be written into the .config file because their dependency >> "depends on A" is unmet. >> >> Currently, there is no code that clears SYMBOL_WRITE of choice values. >> >> Clear SYMBOL_WRITE for all symbols in sym_calc_value(), then set it >> again after calculating visibility. >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> >> --- > >> scripts/kconfig/symbol.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c >> index c9123ed..5d6f6b1 100644 >> --- a/scripts/kconfig/symbol.c >> +++ b/scripts/kconfig/symbol.c >> @@ -371,8 +371,7 @@ void sym_calc_value(struct symbol *sym) >> sym->curr.tri = no; >> return; >> } >> - if (!sym_is_choice_value(sym)) >> - sym->flags &= ~SYMBOL_WRITE; >> + sym->flags &= ~SYMBOL_WRITE; >> >> sym_calc_visibility(sym); >> >> @@ -385,6 +384,7 @@ void sym_calc_value(struct symbol *sym) >> 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; >> + sym->flags |= SYMBOL_WRITE; >> } else { >> if (sym->visible != no) { >> /* if the symbol is visible use the user value >> -- >> 2.7.4 >> > > Reviewed-by: Ulf Magnusson <ulfalizer@gmail.com> > > There's a possible simplification here: > > All defined symbols, regardless of type, and regardless of whether > they're choice value symbols or not, always get written out if they have > non-n visibility. Therefore, the sym->visible != no check for > SYMBOL_WRITE can be moved to before the symbol type check, which gets > rid of two SYMBOL_WRITE assignments and makes it clear that the logic is > the same for all paths. > > This is safe for symbols defined without a type (S_UNKNOWN) too, because > conf_write() skips those (plus they already generate a warning). > > This matches how I do it in the tri_value() function in Kconfiglib: > https://github.com/ulfalizer/Kconfiglib/blob/master/kconfiglib.py#L2574. > SYMBOL_WRITE corresponds to _write_to_conf. > > I've included a patch below. I tested it with the Kconfiglib test suite, > which verifies that the C implementation still generates the same > .config file for all defconfig files as well as for > all{no,yes,def}config, for all ARCHes. > > (The Kconfiglib test suite runs scripts/kconfig/conf and compares its > output against it, which means it doubles as a regression test for the C > tools.) Thank you for this. This is simpler, and please let me take it. I confirmed the same results were produced.
On Thu, Feb 8, 2018 at 3:42 AM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > 2018-02-08 7:55 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: >> On Tue, Feb 06, 2018 at 09:34:42AM +0900, Masahiro Yamada wrote: >>> "# CONFIG_... is not set" for choice values are wrongly written into >>> the .config file if they are once visible, then become invisible later. >>> >>> Test case >>> --------- >>> >>> ---------------------------(Kconfig)---------------------------- >>> config A >>> bool "A" >>> >>> choice >>> prompt "Choice ?" >>> depends on A >>> >>> config CHOICE_B >>> bool "Choice B" >>> >>> config CHOICE_C >>> bool "Choice C" >>> >>> endchoice >>> ---------------------------------------------------------------- >>> >>> ---------------------------(.config)---------------------------- >>> CONFIG_A=y >>> ---------------------------------------------------------------- >>> >>> With the Kconfig and .config above, >>> >>> $ make config >>> scripts/kconfig/conf --oldaskconfig Kconfig >>> * >>> * Linux Kernel Configuration >>> * >>> A (A) [Y/n] n >>> # >>> # configuration written to .config >>> # >>> $ cat .config >>> # >>> # Automatically generated file; DO NOT EDIT. >>> # Linux Kernel Configuration >>> # >>> # CONFIG_A is not set >>> # CONFIG_CHOICE_B is not set >>> # CONFIG_CHOICE_C is not set >>> >>> Here, >>> >>> # CONFIG_CHOICE_B is not set >>> # CONFIG_CHOICE_C is not set >>> >>> should not be written into the .config file because their dependency >>> "depends on A" is unmet. >>> >>> Currently, there is no code that clears SYMBOL_WRITE of choice values. >>> >>> Clear SYMBOL_WRITE for all symbols in sym_calc_value(), then set it >>> again after calculating visibility. >>> >>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> >>> --- >> >>> scripts/kconfig/symbol.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c >>> index c9123ed..5d6f6b1 100644 >>> --- a/scripts/kconfig/symbol.c >>> +++ b/scripts/kconfig/symbol.c >>> @@ -371,8 +371,7 @@ void sym_calc_value(struct symbol *sym) >>> sym->curr.tri = no; >>> return; >>> } >>> - if (!sym_is_choice_value(sym)) >>> - sym->flags &= ~SYMBOL_WRITE; >>> + sym->flags &= ~SYMBOL_WRITE; >>> >>> sym_calc_visibility(sym); >>> >>> @@ -385,6 +384,7 @@ void sym_calc_value(struct symbol *sym) >>> 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; >>> + sym->flags |= SYMBOL_WRITE; >>> } else { >>> if (sym->visible != no) { >>> /* if the symbol is visible use the user value >>> -- >>> 2.7.4 >>> >> >> Reviewed-by: Ulf Magnusson <ulfalizer@gmail.com> >> >> There's a possible simplification here: >> >> All defined symbols, regardless of type, and regardless of whether >> they're choice value symbols or not, always get written out if they have >> non-n visibility. Therefore, the sym->visible != no check for >> SYMBOL_WRITE can be moved to before the symbol type check, which gets >> rid of two SYMBOL_WRITE assignments and makes it clear that the logic is >> the same for all paths. >> >> This is safe for symbols defined without a type (S_UNKNOWN) too, because >> conf_write() skips those (plus they already generate a warning). >> >> This matches how I do it in the tri_value() function in Kconfiglib: >> https://github.com/ulfalizer/Kconfiglib/blob/master/kconfiglib.py#L2574. >> SYMBOL_WRITE corresponds to _write_to_conf. >> >> I've included a patch below. I tested it with the Kconfiglib test suite, >> which verifies that the C implementation still generates the same >> .config file for all defconfig files as well as for >> all{no,yes,def}config, for all ARCHes. >> >> (The Kconfiglib test suite runs scripts/kconfig/conf and compares its >> output against it, which means it doubles as a regression test for the C >> tools.) > > Thank you for this. This is simpler, and please let me take it. > Could just mod the existing patch. Saves the hassle of creating a new one. :) > I confirmed the same results were produced. > > -- > Best Regards > Masahiro Yamada Cheers, Ulf -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 8, 2018 at 3:46 AM, Ulf Magnusson <ulfalizer@gmail.com> wrote: > On Thu, Feb 8, 2018 at 3:42 AM, Masahiro Yamada > <yamada.masahiro@socionext.com> wrote: >> 2018-02-08 7:55 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: >>> On Tue, Feb 06, 2018 at 09:34:42AM +0900, Masahiro Yamada wrote: >>>> "# CONFIG_... is not set" for choice values are wrongly written into >>>> the .config file if they are once visible, then become invisible later. >>>> >>>> Test case >>>> --------- >>>> >>>> ---------------------------(Kconfig)---------------------------- >>>> config A >>>> bool "A" >>>> >>>> choice >>>> prompt "Choice ?" >>>> depends on A >>>> >>>> config CHOICE_B >>>> bool "Choice B" >>>> >>>> config CHOICE_C >>>> bool "Choice C" >>>> >>>> endchoice >>>> ---------------------------------------------------------------- >>>> >>>> ---------------------------(.config)---------------------------- >>>> CONFIG_A=y >>>> ---------------------------------------------------------------- >>>> >>>> With the Kconfig and .config above, >>>> >>>> $ make config >>>> scripts/kconfig/conf --oldaskconfig Kconfig >>>> * >>>> * Linux Kernel Configuration >>>> * >>>> A (A) [Y/n] n >>>> # >>>> # configuration written to .config >>>> # >>>> $ cat .config >>>> # >>>> # Automatically generated file; DO NOT EDIT. >>>> # Linux Kernel Configuration >>>> # >>>> # CONFIG_A is not set >>>> # CONFIG_CHOICE_B is not set >>>> # CONFIG_CHOICE_C is not set >>>> >>>> Here, >>>> >>>> # CONFIG_CHOICE_B is not set >>>> # CONFIG_CHOICE_C is not set >>>> >>>> should not be written into the .config file because their dependency >>>> "depends on A" is unmet. >>>> >>>> Currently, there is no code that clears SYMBOL_WRITE of choice values. >>>> >>>> Clear SYMBOL_WRITE for all symbols in sym_calc_value(), then set it >>>> again after calculating visibility. >>>> >>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> >>>> --- >>> >>>> scripts/kconfig/symbol.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c >>>> index c9123ed..5d6f6b1 100644 >>>> --- a/scripts/kconfig/symbol.c >>>> +++ b/scripts/kconfig/symbol.c >>>> @@ -371,8 +371,7 @@ void sym_calc_value(struct symbol *sym) >>>> sym->curr.tri = no; >>>> return; >>>> } >>>> - if (!sym_is_choice_value(sym)) >>>> - sym->flags &= ~SYMBOL_WRITE; >>>> + sym->flags &= ~SYMBOL_WRITE; >>>> >>>> sym_calc_visibility(sym); >>>> >>>> @@ -385,6 +384,7 @@ void sym_calc_value(struct symbol *sym) >>>> 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; >>>> + sym->flags |= SYMBOL_WRITE; >>>> } else { >>>> if (sym->visible != no) { >>>> /* if the symbol is visible use the user value >>>> -- >>>> 2.7.4 >>>> >>> >>> Reviewed-by: Ulf Magnusson <ulfalizer@gmail.com> >>> >>> There's a possible simplification here: >>> >>> All defined symbols, regardless of type, and regardless of whether >>> they're choice value symbols or not, always get written out if they have >>> non-n visibility. Therefore, the sym->visible != no check for >>> SYMBOL_WRITE can be moved to before the symbol type check, which gets >>> rid of two SYMBOL_WRITE assignments and makes it clear that the logic is >>> the same for all paths. >>> >>> This is safe for symbols defined without a type (S_UNKNOWN) too, because >>> conf_write() skips those (plus they already generate a warning). >>> >>> This matches how I do it in the tri_value() function in Kconfiglib: >>> https://github.com/ulfalizer/Kconfiglib/blob/master/kconfiglib.py#L2574. >>> SYMBOL_WRITE corresponds to _write_to_conf. >>> >>> I've included a patch below. I tested it with the Kconfiglib test suite, >>> which verifies that the C implementation still generates the same >>> .config file for all defconfig files as well as for >>> all{no,yes,def}config, for all ARCHes. >>> >>> (The Kconfiglib test suite runs scripts/kconfig/conf and compares its >>> output against it, which means it doubles as a regression test for the C >>> tools.) >> >> Thank you for this. This is simpler, and please let me take it. >> > > Could just mod the existing patch. Saves the hassle of creating a new one. :) > >> I confirmed the same results were produced. >> >> -- >> Best Regards >> Masahiro Yamada > > Cheers, > Ulf I might've misunderstood. Should I prepare a separate patch that adds the simplification, assuming your patch? I'm fine with whatever approach. Could always say "includes a simplification suggested by..." if you go with a single patch and want to give credit (which isn't required). Cheers, Ulf -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c index c9123ed2b791..13f7fdfe328d 100644 --- a/scripts/kconfig/symbol.c +++ b/scripts/kconfig/symbol.c @@ -371,11 +371,13 @@ void sym_calc_value(struct symbol *sym) sym->curr.tri = no; return; } - if (!sym_is_choice_value(sym)) - sym->flags &= ~SYMBOL_WRITE; + sym->flags &= ~SYMBOL_WRITE; sym_calc_visibility(sym); + if (sym->visible != no) + sym->flags |= SYMBOL_WRITE; + /* set default if recursively called */ sym->curr = newval; @@ -390,7 +392,6 @@ void sym_calc_value(struct symbol *sym) /* if the symbol is visible use the user value * if available, otherwise try the default value */ - sym->flags |= SYMBOL_WRITE; if (sym_has_value(sym)) { newval.tri = EXPR_AND(sym->def[S_DEF_USER].tri, sym->visible); @@ -433,12 +434,9 @@ void sym_calc_value(struct symbol *sym) case S_STRING: case S_HEX: case S_INT: - if (sym->visible != no) { - sym->flags |= SYMBOL_WRITE; - if (sym_has_value(sym)) { - newval.val = sym->def[S_DEF_USER].val; - break; - } + if (sym->visible != no && sym_has_value(sym)) { + newval.val = sym->def[S_DEF_USER].val; + break; } prop = sym_get_default_prop(sym); if (prop) {