[02/14] kconfig: do not write choice values when their dependency becomes n
diff mbox

Message ID 20180207225551.imdp6gqbdwzyvhwm@huvuddator
State New
Headers show

Commit Message

Ulf Magnusson Feb. 7, 2018, 10:55 p.m. UTC
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.)

Comments

Masahiro Yamada Feb. 8, 2018, 2:42 a.m. UTC | #1
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.
Ulf Magnusson Feb. 8, 2018, 2:46 a.m. UTC | #2
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
Ulf Magnusson Feb. 8, 2018, 9:21 p.m. UTC | #3
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

Patch
diff mbox

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) {