On Thu, Feb 8, 2018 at 3:46 AM, Ulf Magnusson <ulfalizer@xxxxxxxxx> wrote:
> On Thu, Feb 8, 2018 at 3:42 AM, Masahiro Yamada
> <yamada.masahiro@xxxxxxxxxxxxx> wrote:
>> 2018-02-08 7:55 GMT+09:00 Ulf Magnusson <ulfalizer@xxxxxxxxx>:
>>> 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@xxxxxxxxxxxxx>
>>>> ---
>>>
>>>> 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@xxxxxxxxx>
>>>
>>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
