Message-ID: <CAK7LNAT=aMDThLLV5O2pD-PfuQs6_aGr5hdLB+JAn8rzM0+Yaw@mail.gmail.com>
Date:   Tue, 6 Mar 2018 18:21:05 +0900
From:   Masahiro Yamada <yamada.masahiro@...ionext.com>
To:     Ulf Magnusson <ulfalizer@...il.com>
Cc:     Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        Sam Ravnborg <sam@...nborg.org>,
        Michal Marek <michal.lkml@...kovi.net>,
        Randy Dunlap <rdunlap@...radead.org>,
        "Luis R . Rodriguez" <mcgrof@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 08/11] kconfig: unittest: test defconfig when two
 choices interact

2018-03-02 19:41 GMT+09:00 Ulf Magnusson <ulfalizer@...il.com>:
> On Fri, Mar 2, 2018 at 5:31 AM, Masahiro Yamada
> <yamada.masahiro@...ionext.com> 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 && <visibibility>' 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 <yamada.masahiro@...ionext.com>
>> ---
>>
>> 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 0000000..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 0000000..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', etc.
>> +
>> +Related Linux commit: fbe98bb9ed3dae23e320c6b113e35f129538d14a
>> +"""
>> +
>> +
>> +def test(conf):
>> +    assert conf.defconfig('defconfig') == 0
>> +    assert conf.config_contains('expected_config')
>> diff --git a/scripts/kconfig/tests/inter_choice/defconfig b/scripts/kconfig/tests/inter_choice/defconfig
>> new file mode 100644
>> index 0000000..162c414
>> --- /dev/null
>> +++ b/scripts/kconfig/tests/inter_choice/defconfig
>> @@ -0,0 +1 @@
>> +CONFIG_CHOICE_VAL0=y
>> diff --git a/scripts/kconfig/tests/inter_choice/expected_config b/scripts/kconfig/tests/inter_choice/expected_config
>> new file mode 100644
>> index 0000000..5dceefb
>> --- /dev/null
>> +++ b/scripts/kconfig/tests/inter_choice/expected_config
>> @@ -0,0 +1,4 @@
>> +CONFIG_MODULES=y
>> +CONFIG_CHOICE_VAL0=y
>> +# CONFIG_CHOIVE_VAL1 is not set
>> +CONFIG_DUMMY=y
>> --
>> 2.7.4
>>
>
> Reviewed-by: Ulf Magnusson <ulfalizer@...il.com>
>
> This reminded me of a bug I reported ages ago, which afaict hasn't
> been fixed: https://lkml.org/lkml/2012/12/5/458 (in retrospect,
> sym_clear_all_valid() is cheap).

Fixed by fbe98bb9ed3dae23e320c6b113e35f129538d14a
a.k.a v3.10-rc1-1-gfbe98bb

The root cause is the same.


> When manually patching all defconfig files in the kernel to disable
> modules and running the Kconfiglib test suite, that bug triggers for a
> few defconfigs. It has previously triggered for a few unpatched
> defconfig files too -- see
> https://github.com/ulfalizer/Kconfiglib#notes.
>
> I just add an extra sym_clear_all_valid() at the end of
> conf_set_all_new_symbols() to fix it. It'd be worth checking if that
> fixes this problem too.
>
> Cheers,
> Ulf
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best Regards
Masahiro Yamada

Powered by blists - more mailing lists