Commit f79dc03f authored by Masahiro Yamada's avatar Masahiro Yamada
Browse files

kconfig: refactor choice value calculation



Handling choices has always been in a PITA in Kconfig.

For example, fixes and reverts were repeated for randconfig with
KCONFIG_ALLCONFIG:

 - 422c809f ("kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG")
 - 23a5dfda ("Revert "kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG"")
 - 8357b485 ("kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG")
 - 490f1617 ("Revert "kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG"")

As these commits pointed out, randconfig does not randomize choices when
KCONFIG_ALLCONFIG is used. This issue still remains.

[Test Case]

    choice
            prompt "choose"

    config A
            bool "A"

    config B
            bool "B"

    endchoice

    $ echo > all.config
    $ make KCONFIG_ALLCONFIG=1 randconfig

The output is always as follows:

    CONFIG_A=y
    # CONFIG_B is not set

Not only randconfig, but other all*config variants are also broken with
KCONFIG_ALLCONFIG.

With the same Kconfig,

    $ echo '# CONFIG_A is not set' > all.config
    $ make KCONFIG_ALLCONFIG=1 allyesconfig

You will get this:

    CONFIG_A=y
    # CONFIG_B is not set

This is incorrect because it does not respect all.config.

The correct output should be:

    # CONFIG_A is not set
    CONFIG_B=y

To handle user inputs more accurately, this commit refactors the code
based on the following principles:

 - When a user value is given, Kconfig must set it immediately.
   Do not defer it by setting SYMBOL_NEED_SET_CHOICE_VALUES.

 - The SYMBOL_DEF_USER flag must not be cleared, unless a new config
   file is loaded. Kconfig must not forget user inputs.

In addition, user values for choices must be managed with priority.
If user inputs conflict within a choice block, the newest value wins.
The values given by randconfig have lower priority than explicit user
inputs.

This commit implements it by using a linked list. Every time a choice
block gets a new input, it is moved to the top of the list.

Let me explain how it works.

Let's say, we have a choice block that consists of five symbols:
A, B, C, D, and E.

Initially, the linked list looks like this:

    A(=?) --> B(=?) --> C(=?) --> D(=?) --> E(=?)

Suppose randconfig is executed with the following KCONFIG_ALLCONFIG:

    CONFIG_C=y
    # CONFIG_A is not set
    CONFIG_D=y

First, CONFIG_C=y is read. C is set to 'y' and moved to the top.

    C(=y) --> A(=?) --> B(=?) --> D(=?) --> E(=?)

Next, '# CONFIG_A is not set' is read. A is set to 'n' and moved to
the top.

    A(=n) --> C(=y) --> B(=?) --> D(=?) --> E(=?)

Then, 'CONFIG_D=y' is read. D is set to 'y' and moved to the top.

    D(=y) --> A(=n) --> C(=y) --> B(=?) --> E(=?)

Lastly, randconfig shuffles the order of the remaining symbols,
resulting in:

    D(=y) --> A(=n) --> C(=y) --> B(=y) --> E(=y)
or
    D(=y) --> A(=n) --> C(=y) --> E(=y) --> B(=y)

When calculating the output, the linked list is traversed and the first
visible symbol with 'y' is taken. In this case, it is D if visible.

If D is hidden by 'depends on', the next node, A, is examined. Since
it is already specified as 'n', it is skipped. Next, C is checked, and
selected if it is visible.

If C is also invisible, either B or E is chosen as a result of the
randomization.

If B and E are also invisible, the linked list is traversed in the
reverse order, and the least prioritized 'n' symbol is chosen. It is
A in this case.

Now, Kconfig remembers all user values. This is a big difference from
the previous implementation, where Kconfig would forget CONFIG_C=y when
CONFIG_D=y appeared in the same input file.

The new appaorch respects user-specified values as much as possible.

Signed-off-by: default avatarMasahiro Yamada <masahiroy@kernel.org>
parent ee29e620
Loading
Loading
Loading
Loading
+62 −69
Original line number Diff line number Diff line
@@ -114,41 +114,54 @@ static void set_randconfig_seed(void)
	srand(seed);
}

static void randomize_choice_values(struct symbol *csym)
/**
 * randomize_choice_values - randomize choice block
 *
 * @choice: menu entry for the choice
 */
static void randomize_choice_values(struct menu *choice)
{
	struct property *prop;
	struct symbol *sym;
	struct expr *e;
	int cnt, def;
	struct menu *menu;
	int x;
	int cnt = 0;

	prop = sym_get_choice_prop(csym);
	/*
	 * First, count the number of symbols to randomize. If sym_has_value()
	 * is true, it was specified by KCONFIG_ALLCONFIG. It needs to be
	 * respected.
	 */
	menu_for_each_sub_entry(menu, choice) {
		struct symbol *sym = menu->sym;

	/* count entries in choice block */
	cnt = 0;
	expr_list_for_each_sym(prop->expr, e, sym)
		if (sym && !sym_has_value(sym))
			cnt++;
	}

	while (cnt > 0) {
		x = rand() % cnt;

		menu_for_each_sub_entry(menu, choice) {
			struct symbol *sym = menu->sym;

			if (sym && !sym_has_value(sym))
				x--;

			if (x < 0) {
				sym->def[S_DEF_USER].tri = yes;
				sym->flags |= SYMBOL_DEF_USER;
				/*
	 * find a random value and set it to yes,
	 * set the rest to no so we have only one set
				 * Move the selected item to the _tail_ because
				 * this needs to have a lower priority than the
				 * user input from KCONFIG_ALLCONFIG.
				 */
	def = rand() % cnt;
				list_move_tail(&sym->choice_link,
					       &choice->choice_members);

	cnt = 0;
	expr_list_for_each_sym(prop->expr, e, sym) {
		if (def == cnt++) {
			sym->def[S_DEF_USER].tri = yes;
			csym->def[S_DEF_USER].val = sym;
		} else {
			sym->def[S_DEF_USER].tri = no;
				break;
			}
		sym->flags |= SYMBOL_DEF_USER;
		/* clear VALID to get value calculated */
		sym->flags &= ~SYMBOL_VALID;
		}
	csym->flags |= SYMBOL_DEF_USER;
	/* clear VALID to get value calculated */
	csym->flags &= ~SYMBOL_VALID;
		cnt--;
	}
}

enum conf_def_mode {
@@ -159,9 +172,9 @@ enum conf_def_mode {
	def_random
};

static bool conf_set_all_new_symbols(enum conf_def_mode mode)
static void conf_set_all_new_symbols(enum conf_def_mode mode)
{
	struct symbol *sym, *csym;
	struct menu *menu;
	int cnt;
	/*
	 * can't go as the default in switch-case below, otherwise gcc whines
@@ -170,7 +183,6 @@ static bool conf_set_all_new_symbols(enum conf_def_mode mode)
	int pby = 50; /* probability of bool     = y */
	int pty = 33; /* probability of tristate = y */
	int ptm = 33; /* probability of tristate = m */
	bool has_changed = false;

	if (mode == def_random) {
		int n, p[3];
@@ -217,14 +229,21 @@ static bool conf_set_all_new_symbols(enum conf_def_mode mode)
		}
	}

	for_all_symbols(sym) {
	menu_for_each_entry(menu) {
		struct symbol *sym = menu->sym;
		tristate val;

		if (sym_has_value(sym) || sym->flags & SYMBOL_VALID ||
		    (sym->type != S_BOOLEAN && sym->type != S_TRISTATE))
		if (!sym || !menu->prompt || sym_has_value(sym) ||
		    (sym->type != S_BOOLEAN && sym->type != S_TRISTATE) ||
		    sym_is_choice_value(sym))
			continue;

		if (sym_is_choice(sym)) {
			if (mode == def_random)
				randomize_choice_values(menu);
			continue;
		}

		has_changed = true;
		switch (mode) {
		case def_yes:
			val = yes;
@@ -251,34 +270,10 @@ static bool conf_set_all_new_symbols(enum conf_def_mode mode)
			continue;
		}
		sym->def[S_DEF_USER].tri = val;

		if (!(sym_is_choice(sym) && mode == def_random))
		sym->flags |= SYMBOL_DEF_USER;
	}

	sym_clear_all_valid();

	if (mode != def_random) {
		for_all_symbols(csym) {
			if ((sym_is_choice(csym) && !sym_has_value(csym)) ||
			    sym_is_choice_value(csym))
				csym->flags |= SYMBOL_NEED_SET_CHOICE_VALUES;
		}
	}

	for_all_symbols(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);
		has_changed = true;
	}

	return has_changed;
}

static void conf_rewrite_tristates(tristate old_val, tristate new_val)
@@ -429,10 +424,9 @@ static void conf_choice(struct menu *menu)
{
	struct symbol *sym, *def_sym;
	struct menu *child;
	bool is_new;
	bool is_new = false;

	sym = menu->sym;
	is_new = !sym_has_value(sym);

	while (1) {
		int cnt, def;
@@ -456,8 +450,10 @@ static void conf_choice(struct menu *menu)
				printf("%*c", indent, ' ');
			printf(" %d. %s (%s)", cnt, menu_get_prompt(child),
			       child->sym->name);
			if (!sym_has_value(child->sym))
			if (!sym_has_value(child->sym)) {
				is_new = true;
				printf(" (NEW)");
			}
			printf("\n");
		}
		printf("%*schoice", indent - 1, "");
@@ -586,9 +582,7 @@ static void check_conf(struct menu *menu)
		return;

	sym = menu->sym;
	if (sym && !sym_has_value(sym) &&
	    (sym_is_changeable(sym) || sym_is_choice(sym))) {

	if (sym && !sym_has_value(sym) && sym_is_changeable(sym)) {
		switch (input_mode) {
		case listnewconfig:
			if (sym->name)
@@ -804,8 +798,7 @@ int main(int ac, char **av)
		conf_set_all_new_symbols(def_default);
		break;
	case randconfig:
		/* Really nothing to do in this loop */
		while (conf_set_all_new_symbols(def_random)) ;
		conf_set_all_new_symbols(def_random);
		break;
	case defconfig:
		conf_set_all_new_symbols(def_default);
+10 −44
Original line number Diff line number Diff line
@@ -382,10 +382,7 @@ int conf_read_simple(const char *name, int def)

	def_flags = SYMBOL_DEF << def;
	for_all_symbols(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:
@@ -399,6 +396,8 @@ int conf_read_simple(const char *name, int def)
	}

	while (getline_stripped(&line, &line_asize, in) != -1) {
		struct menu *choice;

		conf_lineno++;

		if (!line[0]) /* blank line */
@@ -460,15 +459,14 @@ int conf_read_simple(const char *name, int def)
		if (conf_set_sym_val(sym, def, def_flags, val))
			continue;

		if (sym && sym_is_choice_value(sym)) {
			struct symbol *cs = prop_get_symbol(sym_get_choice_prop(sym));
			if (sym->def[def].tri == yes) {
				if (cs->def[def].tri != no)
					conf_warning("override: %s changes choice state", sym->name);
				cs->def[def].val = sym;
				cs->def[def].tri = yes;
			}
		}
		/*
		 * If this is a choice member, give it the highest priority.
		 * If conflicting CONFIG options are given from an input file,
		 * the last one wins.
		 */
		choice = sym_get_choice_menu(sym);
		if (choice)
			list_move(&sym->choice_link, &choice->choice_members);
	}
	free(line);
	fclose(in);
@@ -514,18 +512,6 @@ int conf_read(const char *name)
		/* maybe print value in verbose mode... */
	}

	for_all_symbols(sym) {
		if (sym_has_value(sym) && !sym_is_choice_value(sym)) {
			/* Reset values of generates values, so they'll appear
			 * as new, if they should become visible, but that
			 * doesn't quite work if the Kconfig and the saved
			 * configuration disagree.
			 */
			if (sym->visible == no && !conf_unsaved)
				sym->flags &= ~SYMBOL_DEF_USER;
		}
	}

	if (conf_warnings || conf_unsaved)
		conf_set_changed(true);

@@ -1146,23 +1132,3 @@ void conf_set_changed_callback(void (*fn)(bool))
{
	conf_changed_callback = fn;
}

void set_all_choice_values(struct symbol *csym)
{
	struct property *prop;
	struct symbol *sym;
	struct expr *e;

	prop = sym_get_choice_prop(csym);

	/*
	 * Set all non-assinged choice values to no
	 */
	expr_list_for_each_sym(prop->expr, e, sym) {
		if (!sym_has_value(sym))
			sym->def[S_DEF_USER].tri = no;
	}
	csym->flags |= SYMBOL_DEF_USER;
	/* clear VALID to get value calculated */
	csym->flags &= ~(SYMBOL_VALID | SYMBOL_NEED_SET_CHOICE_VALUES);
}
+8 −4
Original line number Diff line number Diff line
@@ -73,6 +73,8 @@ enum {
 * Represents a configuration symbol.
 *
 * Choices are represented as a special kind of symbol with null name.
 *
 * @choice_link: linked to menu::choice_members
 */
struct symbol {
	/* link node for the hash table */
@@ -110,6 +112,8 @@ struct symbol {
	/* config entries associated with this symbol */
	struct list_head menus;

	struct list_head choice_link;

	/* SYMBOL_* flags */
	int flags;

@@ -133,7 +137,6 @@ struct symbol {
#define SYMBOL_CHOICEVAL  0x0020  /* used as a value in a choice block */
#define SYMBOL_VALID      0x0080  /* set when symbol.curr is calculated */
#define SYMBOL_WRITE      0x0200  /* write symbol to file (KCONFIG_CONFIG) */
#define SYMBOL_CHANGED    0x0400  /* ? */
#define SYMBOL_WRITTEN    0x0800  /* track info to avoid double-write to .config */
#define SYMBOL_CHECKED    0x2000  /* used during dependency checking */
#define SYMBOL_WARNED     0x8000  /* warning has been issued */
@@ -145,9 +148,6 @@ struct symbol {
#define SYMBOL_DEF3       0x40000  /* symbol.def[S_DEF_3] is valid */
#define SYMBOL_DEF4       0x80000  /* symbol.def[S_DEF_4] is valid */

/* choice values need to be set before calculating this symbol value */
#define SYMBOL_NEED_SET_CHOICE_VALUES  0x100000

#define SYMBOL_MAXLENGTH	256

/* A property represent the config options that can be associated
@@ -204,6 +204,8 @@ struct property {
 * for all front ends). Each symbol, menu, etc. defined in the Kconfig files
 * gets a node. A symbol defined in multiple locations gets one node at each
 * location.
 *
 * @choice_members: list of choice members with priority.
 */
struct menu {
	/* The next menu node at the same level */
@@ -223,6 +225,8 @@ struct menu {

	struct list_head link;	/* link to symbol::menus */

	struct list_head choice_members;

	/*
	 * The prompt associated with the node. This holds the prompt for a
	 * symbol as well as the text for a menu or comment, along with the
+1 −6
Original line number Diff line number Diff line
@@ -40,7 +40,6 @@ void zconf_nextfile(const char *name);
/* confdata.c */
extern struct gstr autoconf_cmd;
const char *conf_get_configname(void);
void set_all_choice_values(struct symbol *csym);

/* confdata.c and expr.c */
static inline void xfwrite(const void *str, size_t len, size_t count, FILE *out)
@@ -121,11 +120,7 @@ static inline tristate sym_get_tristate_value(struct symbol *sym)
	return sym->curr.tri;
}


static inline struct symbol *sym_get_choice_value(struct symbol *sym)
{
	return (struct symbol *)sym->curr.val;
}
struct symbol *sym_get_choice_value(struct symbol *sym);

static inline bool sym_is_choice(struct symbol *sym)
{
+1 −16
Original line number Diff line number Diff line
@@ -591,7 +591,6 @@ bool menu_is_empty(struct menu *menu)

bool menu_is_visible(struct menu *menu)
{
	struct menu *child;
	struct symbol *sym;
	tristate visible;

@@ -610,21 +609,7 @@ bool menu_is_visible(struct menu *menu)
	} else
		visible = menu->prompt->visible.tri = expr_calc_value(menu->prompt->visible.expr);

	if (visible != no)
		return true;

	if (!sym || sym_get_tristate_value(menu->sym) == no)
		return false;

	for (child = menu->list; child; child = child->next) {
		if (menu_is_visible(child)) {
			if (sym)
				sym->flags |= SYMBOL_DEF_USER;
			return true;
		}
	}

	return false;
	return visible != no;
}

const char *menu_get_prompt(struct menu *menu)
Loading