randconfig bug when used with KCONFIG_ALLCONFIG

randconfig bug when used with KCONFIG_ALLCONFIG

[Thomas Petazzoni]

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

Re: randconfig bug when used with KCONFIG_ALLCONFIG

[Thomas Petazzoni]

Hello,

On Wed, 27 Feb 2013 21:41:02 +0100, Thomas Petazzoni wrote:

> 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.

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

[PATCH] kconfig: do randomise choice entries in presence of KCONFIG_ALLCONFIG

[Yann E. MORIN]

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.

Reported-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Michal Marek <mmarek@suse.cz>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Arnaud Lacombe <lacombar@gmail.com>

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

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index 13ddf11..17c0816 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -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

Re: [PATCH] kconfig: do randomise choice entries in presence of KCONFIG_ALLCONFIG

[Thomas Petazzoni]

Dear Yann E. MORIN,

On Sun, 10 Mar 2013 17:01:53 +0100, Yann E. MORIN wrote:
> 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.
> 
> Reported-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Michal Marek <mmarek@suse.cz>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Arnaud Lacombe <lacombar@gmail.com>

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!

Tested-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH] kconfig: do randomise choice entries in presence of KCONFIG_ALLCONFIG

[Yann E. MORIN]

Hello All!

On Sunday 10 March 2013 Yann E. MORIN wrote:
> Currently, randconfig does randomise choice entries, unless KCONFIG_ALLCONFIG
> is specified.
[--SNIP--]
> 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.

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.  |
'------------------------------^-------^------------------^--------------------'

Re: [PATCH] kconfig: do randomise choice entries in presence of KCONFIG_ALLCONFIG

[Yann E. MORIN]

On Sun, Mar 10, 2013 at 05:01:53PM +0100, Yann E. MORIN wrote:
> Currently, randconfig does randomise choice entries, unless KCONFIG_ALLCONFIG
> is specified.
[--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.  |
'------------------------------^-------^------------------^--------------------'

Re: [PATCH] kconfig: do randomise choice entries in presence of KCONFIG_ALLCONFIG

[Yann E. MORIN]

On Mon, Apr 08, 2013 at 01:52:35PM +0200, Yann E. MORIN wrote:
> On Sun, Mar 10, 2013 at 05:01:53PM +0100, Yann E. MORIN wrote:
> > Currently, randconfig does randomise choice entries, unless KCONFIG_ALLCONFIG
> > is specified.
[--SNIP--]
> 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).

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.  |
'------------------------------^-------^------------------^--------------------'

[PATCH] kconfig: do randomise choice entries in presence of KCONFIG_ALLCONFIG

[Yann E. MORIN]

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.

Reported-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Michal Marek <mmarek@suse.cz>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Arnaud Lacombe <lacombar@gmail.com>

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

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index 13ddf11..55697b2 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -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

Re: [PATCH] kconfig: do randomise choice entries in presence of KCONFIG_ALLCONFIG

[Michal Marek]

On 10.4.2013 23:20, Yann E. MORIN wrote:
> 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.
> 
> Reported-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Michal Marek <mmarek@suse.cz>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Arnaud Lacombe <lacombar@gmail.com>

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

Re: [PATCH] kconfig: do randomise choice entries in presence of KCONFIG_ALLCONFIG

[Yann E. MORIN]

Hello Michal!

On Thursday 11 April 2013 11:44:00 Michal Marek wrote:
> On 10.4.2013 23:20, Yann E. MORIN wrote:
> > Currently, randconfig does randomise choice entries, unless KCONFIG_ALLCONFIG
> > is specified.
[--SNIP--]
> will you add this to your yem-kconfig-for-next branch, or should I apply
> it to kbuild.git#kconfig directly?

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.  |
'------------------------------'-------'------------------'--------------------'