kconfig: do randomise choice entries in presence of KCONFIG_ALLCONFIG
diff mbox

Message ID 1362931313-31829-1-git-send-email-yann.morin.1998@free.fr
State New, archived
Headers show

Commit Message

Yann E. MORIN March 10, 2013, 4:01 p.m. UTC
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(-)

Comments

Thomas Petazzoni March 11, 2013, 9:40 p.m. UTC | #1
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>
Yann E. MORIN March 13, 2013, 6:27 p.m. UTC | #2
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 April 8, 2013, 11:52 a.m. UTC | #3
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 April 9, 2013, 12:02 p.m. UTC | #4
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.

Patch
diff mbox

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