Re: [PATCH v2 08/11] kconfig: unittest: test defconfig when two choices interact
2018-03-02 19:41 GMT+09:00 Ulf Magnusson :
> On Fri, Mar 2, 2018 at 5:31 AM, Masahiro Yamada
> wrote:
>> Commit fbe98bb9ed3d ("kconfig: Fix defconfig when one choice menu
>> selects options that another choice menu depends on") fixed defconfig
>> when two choices interact (i.e. calculating the visibility of a choice
>> requires to calculate another choice).
>>
>> The test code in that commit log was based on the real world example,
>> and complicated. So, I shrunk it down to the following:
>>
>> defconfig.choice:
>> ---8<---
>> CONFIG_CHOICE_VAL0=y
>> ---8<---
>>
>> ---8<---
>> config MODULES
>> bool "Enable loadable module support"
>> option modules
>> default y
>>
>> choice
>> prompt "Choice"
>>
>> config CHOICE_VAL0
>> tristate "Choice 0"
>>
>> config CHOICE_VAL1
>> tristate "Choice 1"
>>
>> endchoice
>>
>> choice
>> prompt "Another choice"
>> depends on CHOICE_VAL0
>>
>> config DUMMY
>> bool "dummy"
>>
>> endchoice
>> ---8<---
>>
>> Prior to commit fbe98bb9ed3d,
>>
>> $ scripts/kconfig/conf --defconfig=defconfig.choice Kconfig.choice
>>
>> resulted in:
>>
>> CONFIG_MODULES=y
>> CONFIG_CHOICE_VAL0=m
>> # CONFIG_CHOICE_VAL1 is not set
>> CONFIG_DUMMY=y
>>
>> where the expected result would be:
>>
>> CONFIG_MODULES=y
>> CONFIG_CHOICE_VAL0=y
>> # CONFIG_CHOICE_VAL1 is not set
>> CONFIG_DUMMY=y
>>
>> Roughly, this weird behavior happened like this:
>>
>> Symbols are calculated a couple of times. First, all symbols are
>> calculated in conf_read(). The first 'choice' is evaluated to 'y'
>> due to the SYMBOL_DEF_USER flag, but sym_calc_choice() clears it
>> unless all of its choice values are explicitly set by the user.
>>
>> conf_set_all_new_symbols() clears all SYMBOL_VALID flags. Then, only
>> choices are calculated. At this point, the SYMBOL_DEF_USER for the
>> first choice is unset, so, it is evaluated to 'm'. (this is weird)
>
> This is because tristate choices start out in m mode btw (they have an
> implicit select of 'm && ' on them, added add the end of
> menu_finalize()).
Ah, right. But indeed weird to forget SYMBOL_DEF_USER.
>> set_all_choice_values() sets SYMBOL_DEF_USER again to choice symbols.
>>
>> When calculating the second choice, due to 'depends on CHOICE_VAL0',
>> it triggers the calculation of CHOICE_VAL0. As a result, SYMBOL_VALID
>> is set for CHOICE_VAL0.
>>
>> Symbols except choices get the final chance of re-calculation in
>> conf_write(). In a normal case, CHOICE_VAL0 would be re-caluculated,
>> then the first choice would be indirectly re-calculated with the
>> SYMBOL_DEF_USER which has been set by set_all_choice_values(), which
>> would be evaluated to 'y'. But, in this case, CHOICE_VAL0 has been
>> marked SYMBOL_VALID, so it is simply skipped. Then, =m is written out
>> to the .config file.
>>
>> Add a unit test for this naive case.
>
> At a high level, I think the problem is that the choice mode is
> forgotten. It should be y because of the CONFIG_CHOICE_VAL0=y
> assignment, but reverts back to m temporarily, and during that window
> a choice symbol is evaluated and gets the wrong value.
>
> I wonder if all this twisty code and the weird flags
> (SYMBOL_NEED_SET_CHOICE_VALUES... hmm) are required. Perhaps an extra
> invalidation or the like would be enough.
Agree.
Probably, 5d09598d488f and fbe98bb9ed3d fixed issues in a bad way.
I believe SYMBOL_DEF_USER should be set only in
conf_read(_simple) and conf_set_all_new_symbols().
It is strange to set and clear SYMBOL_DEF_USER
while calculating symbols.
>>
>> Signed-off-by: Masahiro Yamada
>> ---
>>
>> Changes in v2:
>> - Newly added
>>
>> scripts/kconfig/tests/inter_choice/Kconfig | 24
>> ++
>> scripts/kconfig/tests/inter_choice/__init__.py | 14 +
>> scripts/kconfig/tests/inter_choice/defconfig | 1 +
>> scripts/kconfig/tests/inter_choice/expected_config | 4
>> 4 files changed, 43 insertions(+)
>> create mode 100644 scripts/kconfig/tests/inter_choice/Kconfig
>> create mode 100644 scripts/kconfig/tests/inter_choice/__init__.py
>> create mode 100644 scripts/kconfig/tests/inter_choice/defconfig
>> create mode 100644 scripts/kconfig/tests/inter_choice/expected_config
>>
>> diff --git a/scripts/kconfig/tests/inter_choice/Kconfig
>> b/scripts/kconfig/tests/inter_choice/Kconfig
>> new file mode 100644
>> index 000..57d55c4
>> --- /dev/null
>> +++ b/scripts/kconfig/tests/inter_choice/Kconfig
>> @@ -0,0 +1,24 @@
>> +config MODULES
>> + bool "Enable loadable module support"
>> + option modules
>> + default y
>> +
>> +choice
>> + prompt "Choice"
>> +
>> +config CHOICE_VAL0
>> + tristate "Choice 0"
>> +
>> +config CHOIVE_VAL1
>> + tristate "Choice 1"
>> +
>> +endchoice
>> +
>> +choice
>> + prompt "Another choice"
>> + depends on CHOICE_VAL0
>> +
>> +config DUMMY
>> + bool "dummy"
>> +
>> +endchoice
>> diff
Re: [PATCH v2 08/11] kconfig: unittest: test defconfig when two choices interact
On Fri, Mar 2, 2018 at 5:31 AM, Masahiro Yamada
wrote:
> Commit fbe98bb9ed3d ("kconfig: Fix defconfig when one choice menu
> selects options that another choice menu depends on") fixed defconfig
> when two choices interact (i.e. calculating the visibility of a choice
> requires to calculate another choice).
>
> The test code in that commit log was based on the real world example,
> and complicated. So, I shrunk it down to the following:
>
> defconfig.choice:
> ---8<---
> CONFIG_CHOICE_VAL0=y
> ---8<---
>
> ---8<---
> config MODULES
> bool "Enable loadable module support"
> option modules
> default y
>
> choice
> prompt "Choice"
>
> config CHOICE_VAL0
> tristate "Choice 0"
>
> config CHOICE_VAL1
> tristate "Choice 1"
>
> endchoice
>
> choice
> prompt "Another choice"
> depends on CHOICE_VAL0
>
> config DUMMY
> bool "dummy"
>
> endchoice
> ---8<---
>
> Prior to commit fbe98bb9ed3d,
>
> $ scripts/kconfig/conf --defconfig=defconfig.choice Kconfig.choice
>
> resulted in:
>
> CONFIG_MODULES=y
> CONFIG_CHOICE_VAL0=m
> # CONFIG_CHOICE_VAL1 is not set
> CONFIG_DUMMY=y
>
> where the expected result would be:
>
> CONFIG_MODULES=y
> CONFIG_CHOICE_VAL0=y
> # CONFIG_CHOICE_VAL1 is not set
> CONFIG_DUMMY=y
>
> Roughly, this weird behavior happened like this:
>
> Symbols are calculated a couple of times. First, all symbols are
> calculated in conf_read(). The first 'choice' is evaluated to 'y'
> due to the SYMBOL_DEF_USER flag, but sym_calc_choice() clears it
> unless all of its choice values are explicitly set by the user.
>
> conf_set_all_new_symbols() clears all SYMBOL_VALID flags. Then, only
> choices are calculated. At this point, the SYMBOL_DEF_USER for the
> first choice is unset, so, it is evaluated to 'm'. (this is weird)
This is because tristate choices start out in m mode btw (they have an
implicit select of 'm && ' on them, added add the end of
menu_finalize()).
> set_all_choice_values() sets SYMBOL_DEF_USER again to choice symbols.
>
> When calculating the second choice, due to 'depends on CHOICE_VAL0',
> it triggers the calculation of CHOICE_VAL0. As a result, SYMBOL_VALID
> is set for CHOICE_VAL0.
>
> Symbols except choices get the final chance of re-calculation in
> conf_write(). In a normal case, CHOICE_VAL0 would be re-caluculated,
> then the first choice would be indirectly re-calculated with the
> SYMBOL_DEF_USER which has been set by set_all_choice_values(), which
> would be evaluated to 'y'. But, in this case, CHOICE_VAL0 has been
> marked SYMBOL_VALID, so it is simply skipped. Then, =m is written out
> to the .config file.
>
> Add a unit test for this naive case.
At a high level, I think the problem is that the choice mode is
forgotten. It should be y because of the CONFIG_CHOICE_VAL0=y
assignment, but reverts back to m temporarily, and during that window
a choice symbol is evaluated and gets the wrong value.
I wonder if all this twisty code and the weird flags
(SYMBOL_NEED_SET_CHOICE_VALUES... hmm) are required. Perhaps an extra
invalidation or the like would be enough.
>
> Signed-off-by: Masahiro Yamada
> ---
>
> Changes in v2:
> - Newly added
>
> scripts/kconfig/tests/inter_choice/Kconfig | 24
> ++
> scripts/kconfig/tests/inter_choice/__init__.py | 14 +
> scripts/kconfig/tests/inter_choice/defconfig | 1 +
> scripts/kconfig/tests/inter_choice/expected_config | 4
> 4 files changed, 43 insertions(+)
> create mode 100644 scripts/kconfig/tests/inter_choice/Kconfig
> create mode 100644 scripts/kconfig/tests/inter_choice/__init__.py
> create mode 100644 scripts/kconfig/tests/inter_choice/defconfig
> create mode 100644 scripts/kconfig/tests/inter_choice/expected_config
>
> diff --git a/scripts/kconfig/tests/inter_choice/Kconfig
> b/scripts/kconfig/tests/inter_choice/Kconfig
> new file mode 100644
> index 000..57d55c4
> --- /dev/null
> +++ b/scripts/kconfig/tests/inter_choice/Kconfig
> @@ -0,0 +1,24 @@
> +config MODULES
> + bool "Enable loadable module support"
> + option modules
> + default y
> +
> +choice
> + prompt "Choice"
> +
> +config CHOICE_VAL0
> + tristate "Choice 0"
> +
> +config CHOIVE_VAL1
> + tristate "Choice 1"
> +
> +endchoice
> +
> +choice
> + prompt "Another choice"
> + depends on CHOICE_VAL0
> +
> +config DUMMY
> + bool "dummy"
> +
> +endchoice
> diff --git a/scripts/kconfig/tests/inter_choice/__init__.py
> b/scripts/kconfig/tests/inter_choice/__init__.py
> new file mode 100644
> index 000..5c7fc36
> --- /dev/null
> +++ b/scripts/kconfig/tests/inter_choice/__init__.py
> @@ -0,0 +1,14 @@
> +"""
> +Do not affect user-assigned choice value by another choice.
> +
> +Handling of state flags for choices is complecated. In old days,
> +the defconfig result of a choice could be affected by another choice
> +if those choices interact by 'depends on', 'select',
