Re: [PATCH v3] kconfig/symbol.c: handle choice_values that depend on 'm' symbols

[Dirk Gouders <dirk@gouders.net>]

"Yann E. MORIN" <yann.morin.1998@free.fr> writes:

> Dirk, All,
>
> On 2013-11-01 00:39 +0100, Dirk Gouders spake thusly:
>> If choices consist of choice_values that depend on symbols set to 'm',
>> those choice_values are not set to 'n' if the choice is changed from
>> 'm' to 'y' (in which case only one active choice_value is allowed).
>> Those values are also written to the config file causing modules to be
>> built when they should not.
>> 
>> The following config can be used to reproduce and examine the problem:
>> 
>> config modules
>> 	boolean modules
>> 	default y
>> 	option modules
>> 
>> config dependency
>> 	tristate "Dependency"
>> 	default m
>> 
>> choice
>> 	prompt "Tristate Choice"
>> 	default choice0
>> 
>> config choice0
>> 	tristate "Choice 0"
>> 
>> config choice1
>> 	tristate "Choice 1"
>> 	depends on dependency
>> 
>> endchoice
>> 
>> This patch sets choice_values' visibility that depend on symbols set
>> to 'm' to 'n' if the corresponding choice is set to 'y'.  This makes
>> them disappear from the choice list and will also cause the
>> choice_values' value set to 'n' in sym_calc_value() and as a result
>> they are written as "not set" to the resulting .config file.
>
> It seems I'm missing something here.
>
> I just copy-pasted your example (test.in. below) and used it with
> current master (cset #be408cd) without your patch, and then ran:
>     $ git clean -dX         # clean the tree
>     $ make menuconfig       # Generate the frontend
>       --> exit without saving
>     $ ./scripts/kconfig/mconf test.in
>       --> change the choice to 'y'
>       --> do not change anything else
>       --> exit and save
>
>     $ cat .config
>     CONFIG_modules=y
>     CONFIG_dependency=m
>     CONFIG_c0=y
>     # CONFIG_c1 is not set
>
> (.config header elided on purpose)
> This looks like the expected output to me.
>
> So I did further tests (still without your patch):
>     $ git clean -dX         # clean the tree
>     $ make menuconfig       # Generate the frontend
>       --> exit without saving
>     $ ./scripts/kconfig/mconf test.in
>       --> change nothing
>       --> exit and save
>
>     $ cat .config
>     CONFIG_modules=y
>     CONFIG_dependency=m
>     # CONFIG_c0 is not set
>     # CONFIG_c1 is not set

This, by the way, is the other problem I mentioned earlier.
There is a default value defined for "Tristate Choice" and the way I
understand the kconfig language, CONFIG_c0 should be 'm' here.

But that is another issue it is just a nice example for what I tried to
describe earlier.

>     $ ./scripts/kconfig/mconf test.in
>       --> change the choice to 'y'
>       --> do not change anything else
>       --> exit and save
>
>     $ cat .config
>     CONFIG_modules=y
>     CONFIG_dependency=m
>     CONFIG_c0=y
>     # CONFIG_c1 is not set
>
> Still the expected output, as far as I can see.
>
> I can observe the exact same result with your patch applied. Ditto with
> kbuild/for-next from Michal's tree, with or without your patch.
>
> So while I understand and can reproduce the original issue, and this
> patch indeed solves this original issue, the test-case above does not
> seem to completely illustrate the issue.
>
> Are you sure this test-case exhibits the problem for you?

Yes, but obviously, I did not describe it very clearly.  The steps to
reproduce the problem are:

     $ ./scripts/kconfig/mconf test.in
       --> change c0 and c1 to 'm'        # This is the missing part!
       --> change the choice to 'y'
       --> do not change anything else
       --> exit and save

I spontaneously planned to answer with a modified config file with
default values 'm' specified for 'c0' and 'c1' (complete file below) but
I noticed that my latest patch does not help in that case.  The first
patch that modifies sym_calc_value() would handle it nicely but the
latter one that modifies sym_calc_visibility() does not.  The
combination also does not work, because sym_calc_visibility() influences
sym_calc_value().

So, I have to say that I am no longer really satisfied with the patch.
It fixes the reported problem but I think it should fix related
obvious problems as well (see config below).  I'd prefer I take some
more time and try to find a more sensible fix.

Thanks for your review and testing, Yann.

Dirk

PS: Sebastian, I also want to say thank you to you for the testing so
    far!

- Sample Kconfig -------------------------------------------------------

config modules
       boolean modules
       default y
       option modules

config dependency
       tristate "Dependency"
       default m

choice
	tristate "Tristate Choice"
	default choice0

config choice0
       	tristate "Choice 0"
	default m

config choice1
	tristate "Choice 1"
	depends on dependency
	default m

endchoice

------------------------------------------------------------------------

> Anyway, I'm taking that in my tree locally, but that won't be part of
> for-next, since I'd like that we:
>   - either find a real reduced test-case,
>   - or just repeat the description from the original bug report
>
> Needless to say that I'd prefer the former over the latter. Then I'll
> queue it as a post-rc1 fix.
>
> Regards,
> Yann E. MORIN.
>
>> Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> Signed-off-by: Dirk Gouders <dirk@gouders.net>
>> ---
>>  scripts/kconfig/symbol.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>> 
>> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
>> index 22a3c40..32bbaa3 100644
>> --- a/scripts/kconfig/symbol.c
>> +++ b/scripts/kconfig/symbol.c
>> @@ -188,12 +188,23 @@ static void sym_validate_range(struct symbol *sym)
>>  static void sym_calc_visibility(struct symbol *sym)
>>  {
>>  	struct property *prop;
>> +	struct symbol *choice_sym = NULL;
>>  	tristate tri;
>>  
>>  	/* any prompt visible? */
>>  	tri = no;
>> +
>> +	if (sym_is_choice_value(sym))
>> +		choice_sym = prop_get_symbol(sym_get_choice_prop(sym));
>> +
>>  	for_all_prompts(sym, prop) {
>>  		prop->visible.tri = expr_calc_value(prop->visible.expr);
>> +		/*
>> +		 * choice_values with visibility 'mod' are not visible if the
>> +		 * corresponding choice's value is 'yes'.
>> +		 */
>> +		if (prop->visible.tri == mod && (choice_sym && choice_sym->curr.tri == yes))
>> +			prop->visible.tri = no;
>>  		tri = EXPR_OR(tri, prop->visible.tri);
>>  	}
>>  	if (tri == mod && (sym->type != S_TRISTATE || modules_val == no))
>> -- 
>> 1.8.4
>>