randconfig bug when used with KCONFIG_ALLCONFIG

Hello,

I'd like to report that the randconfig code seems to have a bug when it is given a set of fixed values using KCONFIG_ALLCONFIG: it forgets to randomize choice values.

Take the following Config.test.in:

-----8<----------8<------------8<----------
config OPTIONA
bool "Option A"

choice
prompt "This is a choice"

config CHOICE_OPTIONA
bool "Choice Option A"

config CHOICE_OPTIONB
bool "Choice Option B"

endchoice

config OPTIONB
bool "Option B"
-----8<----------8<------------8<----------

If you do (using the kernel's kconfig):

./scripts/kconfig/conf --randconfig Config.test.in

you will see that the generated .config file properly randomizes all options, including CHOICE_OPTIONA and CHOICE_OPTIONB.

Now, if we create a partial .config (named orig.config) file to set the value of some options:

-----8<----------8<------------8<----------
CONFIG_OPTIONA=y
-----8<----------8<------------8<----------

If you do:

KCONFIG_ALLCONFIG=orig.config ./scripts/kconfig/conf --randconfig Config.test.in

and look at the resulting .config, you will see that:

* CONFIG_OPTIONA is always set to y, which is OK.

* CONFIG_OPTIONB is properly randomized

* But in the choice, it's always CONFIG_CHOICE_OPTIONA that will be selected, and never CONFIG_CHOICE_OPTIONB. The randomization for choices doesn't work as soon as a file is passed as KCONFIG_ALLCONFIG.

As you can see, the randomization of choices does not work properly when KCONFIG_ALLCONFIG is used.

I've looked a little bit in the code, and in the following piece of code from confdata.c:

	for_all_symbols(i, csym) {
		if (sym_has_value(csym) || !sym_is_choice(csym))
			continue;

		sym_calc_value(csym);
		if (mode == def_random)
			randomize_choice_values(csym);
		else
			set_all_choice_values(csym);
	}

which is normally responsible for randomizing choice values, sym_has_value(csym) returns true for all choice options when KCONFIG_ALLCONFIG is used, which explains why the choices are not randomized. I haven't found out though why the kconfig code thinks that all choice values already have a value, even though no value has been passed through the KCONFIG_ALLCONFIG file.

Any hints? Or patch to test?

Thanks,

Thomas Petazzoni

Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Hello,

Any ideas about this bug?

Thanks a lot for your attention,

Thomas

Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Currently, randconfig does randomise choice entries, unless KCONFIG_ALLCONFIG is specified.

For example, given those two files (Thomas' test-case):

---8<--- Config.test.in
config OPTIONA
bool "Option A"

choice
prompt "This is a choice"

config CHOICE_OPTIONA
bool "Choice Option A"

config CHOICE_OPTIONB
bool "Choice Option B"

endchoice

config OPTIONB
bool "Option B"
---8<---

---8<--- config.defaults
CONFIG_OPTIONA=y
---8<---

And running:

    ./scripts/kconfig/conf --randconfig Config.test.in

does properly randomise the two choice symbols (and the two booleans).

However, running:

    KCONFIG_ALLCONFIG=config.defaults \
    ./scripts/kconfig/conf --randconfig Config.test.in

does *not* reandomise the two choice entries, and only CHOICE_OPTIONA will ever be selected. (OPTIONA will always be set (expected), and OPTIONB will be be properly randomised (expected).)

This patch defers setting that a choice has a value until a symbol for that choice is indeed set, so that choices are properly randomised when KCONFIG_ALLCONFIG is set, but not if a symbol for that choice is set.

---
I'm not requestiong this change to go in -rc-fixes for now. I would first like some review on this change.

---
 scripts/kconfig/confdata.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

index 13ddf11..17c0816 100644
@@ -288,8 +288,6 @@ load:
 	for_all_symbols(i, sym) {
 		sym->flags |= SYMBOL_CHANGED;
 		sym->flags &= ~(def_flags|SYMBOL_VALID);
-		if (sym_is_choice(sym))
-			sym->flags |= def_flags;
 		switch (sym->type) {
 		case S_INT:
 		case S_HEX:

@@ -389,6 +387,7 @@ setsym:
 				break;
 			}
 			cs->def[def].tri = EXPR_OR(cs->def[def].tri, sym->def[def].tri);
+			cs->flags |= def_flags;
 		}
 	}
 	free(line);

1.7.2.5

Dear Yann E. MORIN,

I confirm that the patch fixes the issue for me, but I am not able to judge the absence of side-effects. Thanks Yann for the investigation!

Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Hello All!

[--SNIP--]

Ping? Any more comments on that patch? :-)

Regards,
Yann E. MORIN.

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[--SNIP--]

Ping?

Although this patch does fix the suggested test-case, there are more complex situations where this patch is not enough. (I need to shrink down the currently failing Kconfig file to the smallest possible test-case).

We really need some people with more insight in the kconfig code-base to help us on this subject, at least by reviewing this patch and confirm we're heading in the right direction.

Regards,
Yann E. MORIN.

PS. Sorry if I missed any reply, I've had some mail churning the past
two weeks.
YEM.

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[--SNIP--]

Here is a newer, worse test-case (with my patch applied ontop Michal's kbuild/kconfig tree):

---8<--- config.in

    config A
        bool "A"
    
    if A
    choice
        bool "B/C"
    config B
        bool "B"
    config C
        bool "C"
    endchoice
    endif # A
    
    if B
    choice
        bool "D/E"
    config D
        bool "D"
    config E
        bool "E"
    endchoice
    endif # B

---8<---

With an empty './defconfig' file:

    KCONFIG_ALLCONFIG=defconfig conf --randconfig config.in

will sometime emit a .config with *both* B=y and C=y although they are mutually exclusive, being in a choice block.

However, if the two choices are inverted:

---8<--- config.in

    config A
        bool "A"
    
    if B
    choice
        bool "D/E"
    config D
        bool "D"
    config E
        bool "E"
    endchoice
    endif # B
    
    if A
    choice
        bool "B/C"
    config B
        bool "B"
    config C
        bool "C"
    endchoice
    endif # A

---8<---

Then --randconfig will properly randomise *both* choices!

Note: if my patch is not applied, then only B will ever be selected, and C will never be.

Regards,
Yann E. MORIN.

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Currently, randconfig does randomise choice entries, unless KCONFIG_ALLCONFIG is specified.

For example, given those two files (Thomas' test-case):

---8<--- Config.test.in
config OPTIONA
bool "Option A"

choice
prompt "This is a choice"

config CHOICE_OPTIONA
bool "Choice Option A"

config CHOICE_OPTIONB
bool "Choice Option B"

endchoice

config OPTIONB
bool "Option B"
---8<--- Config.test.in

---8<--- config.defaults
CONFIG_OPTIONA=y
---8<--- config.defaults

And running:

    ./scripts/kconfig/conf --randconfig Config.test.in

does properly randomise the two choice symbols (and the two booleans).

However, running:

    KCONFIG_ALLCONFIG=config.defaults \
    ./scripts/kconfig/conf --randconfig Config.test.in

does *not* reandomise the two choice entries, and only CHOICE_OPTIONA will ever be selected. (OPTIONA will always be set (expected), and OPTIONB will be be properly randomised (expected).)

This patch defers setting that a choice has a value until a symbol for that choice is indeed set, so that choices are properly randomised when KCONFIG_ALLCONFIG is set, but not if a symbol for that choice is set.

---
I'm not requesting this change to go in for now.
I would first like some review on this change, as I am not able to assert there wil be no nasty side effects (or even non-nasty ones).

Changes v1 -> v2:
- further postpone setting that a choice has a value until one is indeed set
- do not print symbols that are part of an invisible choice

---
 scripts/kconfig/confdata.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

index 13ddf11..55697b2 100644
@@ -288,8 +288,6 @@ load:
 	for_all_symbols(i, sym) {
 		sym->flags |= SYMBOL_CHANGED;
 		sym->flags &= ~(def_flags|SYMBOL_VALID);
-		if (sym_is_choice(sym))
-			sym->flags |= def_flags;
 		switch (sym->type) {
 		case S_INT:
 		case S_HEX:

@@ -379,13 +377,13 @@ setsym:
 			case mod:
 				if (cs->def[def].tri == yes) {
 					conf_warning("%s creates inconsistent choice state", sym->name);
-					cs->flags &= ~def_flags;
 				}
 				break;
 			case yes:
 				if (cs->def[def].tri != no)
 					conf_warning("override: %s changes choice state", sym->name);
 				cs->def[def].val = sym;
+				cs->flags |= def_flags;
 				break;
 			}
 			cs->def[def].tri = EXPR_OR(cs->def[def].tri, sym->def[def].tri);

@@ -791,6 +789,8 @@ int conf_write(const char *name)
 			sym_calc_value(sym);
 			if (!(sym->flags & SYMBOL_WRITE))
 				goto next;
+			if (sym_is_choice_value(sym) && !menu_is_visible(menu->parent))
+				goto next;
 			sym->flags &= ~SYMBOL_WRITE;
 
 			conf_write_symbol(out, sym, &kconfig_printer_cb, NULL);

@@ -1077,6 +1077,7 @@ static void randomize_choice_values(struct symbol *csym)
 		else {
 			sym->def[S_DEF_USER].tri = no;
 		}
+		sym->flags &= ~(SYMBOL_VALID);
 	}
 	csym->flags |= SYMBOL_DEF_USER;
 	/* clear VALID to get value calculated */

1.7.2.5

Hi Yann,

will you add this to your yem-kconfig-for-next branch, or should I apply it to kbuild.git#kconfig directly?

Thanks,
Michal

Hello Michal!

[--SNIP--]

I'll eventually add this to my branch, but I'm not confident with the change for now, and I'd like to wait for some more reviews by the people more knowlegeable in Kconfig than I currently am.

That patch is all I could come with after much head-scratching, and I would not be very surprised that someone comes with a better patch. Not surprised at all, in fact.

In all cases, let's just post-pone this for post-3.9, and I'll respin it for some time during the 3.10-rc phase. Well, unless powers-that-be deem that patch worth being applied as-is! ;-)

Also, I have a few other Kconfig changes pending here that will have to go in 3.10, so it's better to wait.

Regards,
Yann E. MORIN.

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +0/33 662376056 | Software  Designer | \ / CAMPAIGN     |   ^                |
| --==< O_o >==-- '------------.-------:  X  AGAINST      |  /e\  There is no  |
| http://ymorin.is-a-geek.org/ | (*_*) | / \ HTML MAIL    |  """  conspiracy.  |
'------------------------------'-------'------------------'--------------------'