lkml.org 
[lkml]   [2018]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v2 08/11] kconfig: unittest: test defconfig when two choices interact
    On Fri, Mar 2, 2018 at 5:31 AM, Masahiro Yamada
    <yamada.masahiro@socionext.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()).

    > 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 <yamada.masahiro@socionext.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@gmail.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).

    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

    \
     
     \ /
      Last update: 2018-03-02 11:42    [W:4.380 / U:0.236 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site