[PATCH v2 1/3] kconfig: error out when seeing recursive dependency

[PATCH v2 1/3] kconfig: error out when seeing recursive dependency

[Masahiro Yamada]

Originally, recursive dependency was a fatal error for Kconfig
because Kconfig cannot compute symbol values in such a situation.

Commit d595cea62403 ("kconfig: print more info when we see a recursive
dependency") changed it to a warning, which I guess was not intentional.

Get it back to an error again.

Also, rename the unit test directory "warn_recursive_dep" to
"err_recursive_dep" so that it matches to the behavior.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2:
 - Newly added

 scripts/kconfig/symbol.c                                       |  3 ---
 .../tests/{warn_recursive_dep => err_recursive_dep}/Kconfig    |  0
 scripts/kconfig/tests/err_recursive_dep/__init__.py            | 10 ++++++++++
 .../{warn_recursive_dep => err_recursive_dep}/expected_stderr  |  0
 scripts/kconfig/tests/warn_recursive_dep/__init__.py           |  9 ---------
 5 files changed, 10 insertions(+), 12 deletions(-)
 rename scripts/kconfig/tests/{warn_recursive_dep => err_recursive_dep}/Kconfig (100%)
 create mode 100644 scripts/kconfig/tests/err_recursive_dep/__init__.py
 rename scripts/kconfig/tests/{warn_recursive_dep => err_recursive_dep}/expected_stderr (100%)
 delete mode 100644 scripts/kconfig/tests/warn_recursive_dep/__init__.py

diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index 4ec8b1f..688f487 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -1248,9 +1248,6 @@ struct symbol *sym_check_deps(struct symbol *sym)
 		sym->flags &= ~SYMBOL_CHECK;
 	}
 
-	if (sym2 && sym2 == sym)
-		sym2 = NULL;
-
 	return sym2;
 }
 
diff --git a/scripts/kconfig/tests/warn_recursive_dep/Kconfig b/scripts/kconfig/tests/err_recursive_dep/Kconfig
similarity index 100%
rename from scripts/kconfig/tests/warn_recursive_dep/Kconfig
rename to scripts/kconfig/tests/err_recursive_dep/Kconfig
diff --git a/scripts/kconfig/tests/err_recursive_dep/__init__.py b/scripts/kconfig/tests/err_recursive_dep/__init__.py
new file mode 100644
index 0000000..5f3821b
--- /dev/null
+++ b/scripts/kconfig/tests/err_recursive_dep/__init__.py
@@ -0,0 +1,10 @@
+# SPDX-License-Identifier: GPL-2.0
+"""
+Detect recursive dependency error.
+
+Recursive dependency should be treated as an error.
+"""
+
+def test(conf):
+    assert conf.oldaskconfig() == 1
+    assert conf.stderr_contains('expected_stderr')
diff --git a/scripts/kconfig/tests/warn_recursive_dep/expected_stderr b/scripts/kconfig/tests/err_recursive_dep/expected_stderr
similarity index 100%
rename from scripts/kconfig/tests/warn_recursive_dep/expected_stderr
rename to scripts/kconfig/tests/err_recursive_dep/expected_stderr
diff --git a/scripts/kconfig/tests/warn_recursive_dep/__init__.py b/scripts/kconfig/tests/warn_recursive_dep/__init__.py
deleted file mode 100644
index adb2195..0000000
--- a/scripts/kconfig/tests/warn_recursive_dep/__init__.py
+++ /dev/null
@@ -1,9 +0,0 @@
-"""
-Warn recursive inclusion.
-
-Recursive dependency should be warned.
-"""
-
-def test(conf):
-    assert conf.oldaskconfig() == 0
-    assert conf.stderr_contains('expected_stderr')
-- 
2.7.4

[PATCH v2 2/3] kconfig: report recursive dependency involving 'imply'

[Masahiro Yamada]

Currently, Kconfig does not complain about the recursive dependency
where 'imply' keywords are involved.

[Test Code]

  config A
          bool "a"

  config B
          bool "b"
          imply A
          depends on A

In the code above, Kconfig cannot calculate the symbol values correctly
due to the circular dependency.  For example, allyesconfig followed by
syncconfig results in an odd behavior because CONFIG_B becomes visible
in syncconfig.

  $ make allyesconfig
  scripts/kconfig/conf  --allyesconfig Kconfig
  #
  # configuration written to .config
  #
  $ cat .config
  #
  # Automatically generated file; DO NOT EDIT.
  # Main menu
  #
  CONFIG_A=y
  $ make syncconfig
  scripts/kconfig/conf  --syncconfig Kconfig
  *
  * Restart config...
  *
  *
  * Main menu
  *
  a (A) [Y/n/?] y
    b (B) [N/y/?] (NEW)

To detect this correctly, sym_check_expr_deps() should recurse to
not only sym->rev_dep.expr but also sym->implied.expr .

At this moment, sym_check_print_recursive() cannot distinguish
'select' and 'imply' since it does not know the precise context
where the recursive dependency has been hit.  This will be solved
by the next commit.

In fact, even the document and the unit-test are confused.  Using
'imply' does not solve recursive dependency since 'imply' addresses
the unmet direct dependency, which 'select' could cause.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2:
 - Fix documentation and unit-test

 Documentation/kbuild/kconfig-language.txt          |  3 +-
 scripts/kconfig/symbol.c                           |  9 +++--
 scripts/kconfig/tests/err_recursive_dep/Kconfig    |  3 +-
 .../tests/err_recursive_dep/expected_stderr        | 38 +++++++++++++---------
 4 files changed, 33 insertions(+), 20 deletions(-)

diff --git a/Documentation/kbuild/kconfig-language.txt b/Documentation/kbuild/kconfig-language.txt
index c54cb7c..864e740 100644
--- a/Documentation/kbuild/kconfig-language.txt
+++ b/Documentation/kbuild/kconfig-language.txt
@@ -545,7 +545,7 @@ make KBUILD_KCONFIG=Documentation/kbuild/Kconfig.recursion-issue-02 allnoconfig
 Practical solutions to kconfig recursive issue
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-Developers who run into the recursive Kconfig issue have three options
+Developers who run into the recursive Kconfig issue have two options
 at their disposal. We document them below and also provide a list of
 historical issues resolved through these different solutions.
 
@@ -553,7 +553,6 @@ historical issues resolved through these different solutions.
   b) Match dependency semantics:
 	b1) Swap all "select FOO" to "depends on FOO" or,
 	b2) Swap all "depends on FOO" to "select FOO"
-  c) Consider the use of "imply" instead of "select"
 
 The resolution to a) can be tested with the sample Kconfig file
 Documentation/kbuild/Kconfig.recursion-issue-01 through the removal
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index 688f487..90e70609 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -1098,7 +1098,7 @@ static void sym_check_print_recursive(struct symbol *last_sym)
 				sym->name ? sym->name : "<choice>",
 				next_sym->name ? next_sym->name : "<choice>");
 		} else {
-			fprintf(stderr, "%s:%d:\tsymbol %s is selected by %s\n",
+			fprintf(stderr, "%s:%d:\tsymbol %s is selected or implied by %s\n",
 				prop->file->name, prop->lineno,
 				sym->name ? sym->name : "<choice>",
 				next_sym->name ? next_sym->name : "<choice>");
@@ -1161,8 +1161,13 @@ static struct symbol *sym_check_sym_deps(struct symbol *sym)
 	if (sym2)
 		goto out;
 
+	sym2 = sym_check_expr_deps(sym->implied.expr);
+	if (sym2)
+		goto out;
+
 	for (prop = sym->prop; prop; prop = prop->next) {
-		if (prop->type == P_CHOICE || prop->type == P_SELECT)
+		if (prop->type == P_CHOICE || prop->type == P_SELECT ||
+		    prop->type == P_IMPLY)
 			continue;
 		stack.prop = prop;
 		sym2 = sym_check_expr_deps(prop->visible.expr);
diff --git a/scripts/kconfig/tests/err_recursive_dep/Kconfig b/scripts/kconfig/tests/err_recursive_dep/Kconfig
index a65bfcb..ebdb3ff 100644
--- a/scripts/kconfig/tests/err_recursive_dep/Kconfig
+++ b/scripts/kconfig/tests/err_recursive_dep/Kconfig
@@ -1,3 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+
 # depends on itself
 
 config A
@@ -31,7 +33,6 @@ config D2
 	bool
 
 # depends on and imply
-# This is not recursive dependency
 
 config E1
 	bool "E1"
diff --git a/scripts/kconfig/tests/err_recursive_dep/expected_stderr b/scripts/kconfig/tests/err_recursive_dep/expected_stderr
index 3de807d..2eb2528 100644
--- a/scripts/kconfig/tests/err_recursive_dep/expected_stderr
+++ b/scripts/kconfig/tests/err_recursive_dep/expected_stderr
@@ -1,30 +1,38 @@
-Kconfig:9:error: recursive dependency detected!
-Kconfig:9:	symbol B is selected by B
+Kconfig:11:error: recursive dependency detected!
+Kconfig:11:	symbol B is selected or implied by B
 For a resolution refer to Documentation/kbuild/kconfig-language.txt
 subsection "Kconfig recursive dependency limitations"
 
-Kconfig:3:error: recursive dependency detected!
-Kconfig:3:	symbol A depends on A
+Kconfig:5:error: recursive dependency detected!
+Kconfig:5:	symbol A depends on A
 For a resolution refer to Documentation/kbuild/kconfig-language.txt
 subsection "Kconfig recursive dependency limitations"
 
-Kconfig:15:error: recursive dependency detected!
-Kconfig:15:	symbol C1 depends on C2
-Kconfig:19:	symbol C2 depends on C1
+Kconfig:17:error: recursive dependency detected!
+Kconfig:17:	symbol C1 depends on C2
+Kconfig:21:	symbol C2 depends on C1
 For a resolution refer to Documentation/kbuild/kconfig-language.txt
 subsection "Kconfig recursive dependency limitations"
 
-Kconfig:30:error: recursive dependency detected!
-Kconfig:30:	symbol D2 is selected by D1
-Kconfig:25:	symbol D1 depends on D2
+Kconfig:32:error: recursive dependency detected!
+Kconfig:32:	symbol D2 is selected or implied by D1
+Kconfig:27:	symbol D1 depends on D2
 For a resolution refer to Documentation/kbuild/kconfig-language.txt
 subsection "Kconfig recursive dependency limitations"
 
-Kconfig:59:error: recursive dependency detected!
-Kconfig:59:	symbol G depends on G
+Kconfig:37:error: recursive dependency detected!
+Kconfig:37:	symbol E1 depends on E2
+Kconfig:42:	symbol E2 is selected or implied by E1
 For a resolution refer to Documentation/kbuild/kconfig-language.txt
 subsection "Kconfig recursive dependency limitations"
 
-Kconfig:50:error: recursive dependency detected!
-Kconfig:50:	symbol F2 depends on F1
-Kconfig:48:	symbol F1 default value contains F2
+Kconfig:60:error: recursive dependency detected!
+Kconfig:60:	symbol G depends on G
+For a resolution refer to Documentation/kbuild/kconfig-language.txt
+subsection "Kconfig recursive dependency limitations"
+
+Kconfig:51:error: recursive dependency detected!
+Kconfig:51:	symbol F2 depends on F1
+Kconfig:49:	symbol F1 default value contains F2
+For a resolution refer to Documentation/kbuild/kconfig-language.txt
+subsection "Kconfig recursive dependency limitations"
-- 
2.7.4

[PATCH v2 3/3] kconfig: improve the recursive dependency report

[Masahiro Yamada]

This commit improves the messages of the recursive dependency.
Currently, sym->dir_dep.expr is not checked.  Hence, any dependency
in property visibility is regarded as the dependency of the symbol.

[Test Code 1]

  config A
          bool "a"
          depends on B

  config B
          bool "b"
          depends on A

[Test Code 2]

  config A
          bool "a" if B

  config B
          bool "b"
          depends on A

For both cases above, the same message is displayed:

        symbol B depends on A
        symbol A depends on B

This commit changes the message for the latter case like this:

        symbol B depends on A
        symbol A prompt is visible depending on B

Also, 'select' and 'imply' are distinguished.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2:
 - Update unit test

 scripts/kconfig/symbol.c                           | 48 +++++++++++++++-------
 .../tests/err_recursive_dep/expected_stderr        |  6 +--
 2 files changed, 37 insertions(+), 17 deletions(-)

diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index 90e70609..703b9b8 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -1011,7 +1011,7 @@ static struct dep_stack {
 	struct dep_stack *prev, *next;
 	struct symbol *sym;
 	struct property *prop;
-	struct expr *expr;
+	struct expr **expr;
 } *check_top;
 
 static void dep_stack_insert(struct dep_stack *stack, struct symbol *sym)
@@ -1076,31 +1076,42 @@ static void sym_check_print_recursive(struct symbol *last_sym)
 			fprintf(stderr, "%s:%d:error: recursive dependency detected!\n",
 				prop->file->name, prop->lineno);
 
-		if (stack->expr) {
-			fprintf(stderr, "%s:%d:\tsymbol %s %s value contains %s\n",
-				prop->file->name, prop->lineno,
+		if (sym_is_choice(sym)) {
+			fprintf(stderr, "%s:%d:\tchoice %s contains symbol %s\n",
+				menu->file->name, menu->lineno,
+				sym->name ? sym->name : "<choice>",
+				next_sym->name ? next_sym->name : "<choice>");
+		} else if (sym_is_choice_value(sym)) {
+			fprintf(stderr, "%s:%d:\tsymbol %s is part of choice %s\n",
+				menu->file->name, menu->lineno,
 				sym->name ? sym->name : "<choice>",
-				prop_get_type_name(prop->type),
 				next_sym->name ? next_sym->name : "<choice>");
-		} else if (stack->prop) {
+		} else if (stack->expr == &sym->dir_dep.expr) {
 			fprintf(stderr, "%s:%d:\tsymbol %s depends on %s\n",
 				prop->file->name, prop->lineno,
 				sym->name ? sym->name : "<choice>",
 				next_sym->name ? next_sym->name : "<choice>");
-		} else if (sym_is_choice(sym)) {
-			fprintf(stderr, "%s:%d:\tchoice %s contains symbol %s\n",
-				menu->file->name, menu->lineno,
+		} else if (stack->expr == &sym->rev_dep.expr) {
+			fprintf(stderr, "%s:%d:\tsymbol %s is selected by %s\n",
+				prop->file->name, prop->lineno,
 				sym->name ? sym->name : "<choice>",
 				next_sym->name ? next_sym->name : "<choice>");
-		} else if (sym_is_choice_value(sym)) {
-			fprintf(stderr, "%s:%d:\tsymbol %s is part of choice %s\n",
-				menu->file->name, menu->lineno,
+		} else if (stack->expr == &sym->implied.expr) {
+			fprintf(stderr, "%s:%d:\tsymbol %s is implied by %s\n",
+				prop->file->name, prop->lineno,
 				sym->name ? sym->name : "<choice>",
 				next_sym->name ? next_sym->name : "<choice>");
+		} else if (stack->expr) {
+			fprintf(stderr, "%s:%d:\tsymbol %s %s value contains %s\n",
+				prop->file->name, prop->lineno,
+				sym->name ? sym->name : "<choice>",
+				prop_get_type_name(prop->type),
+				next_sym->name ? next_sym->name : "<choice>");
 		} else {
-			fprintf(stderr, "%s:%d:\tsymbol %s is selected or implied by %s\n",
+			fprintf(stderr, "%s:%d:\tsymbol %s %s is visible depending on %s\n",
 				prop->file->name, prop->lineno,
 				sym->name ? sym->name : "<choice>",
+				prop_get_type_name(prop->type),
 				next_sym->name ? next_sym->name : "<choice>");
 		}
 	}
@@ -1157,14 +1168,23 @@ static struct symbol *sym_check_sym_deps(struct symbol *sym)
 
 	dep_stack_insert(&stack, sym);
 
+	stack.expr = &sym->dir_dep.expr;
+	sym2 = sym_check_expr_deps(sym->dir_dep.expr);
+	if (sym2)
+		goto out;
+
+	stack.expr = &sym->rev_dep.expr;
 	sym2 = sym_check_expr_deps(sym->rev_dep.expr);
 	if (sym2)
 		goto out;
 
+	stack.expr = &sym->implied.expr;
 	sym2 = sym_check_expr_deps(sym->implied.expr);
 	if (sym2)
 		goto out;
 
+	stack.expr = NULL;
+
 	for (prop = sym->prop; prop; prop = prop->next) {
 		if (prop->type == P_CHOICE || prop->type == P_SELECT ||
 		    prop->type == P_IMPLY)
@@ -1175,7 +1195,7 @@ static struct symbol *sym_check_sym_deps(struct symbol *sym)
 			break;
 		if (prop->type != P_DEFAULT || sym_is_choice(sym))
 			continue;
-		stack.expr = prop->expr;
+		stack.expr = &prop->expr;
 		sym2 = sym_check_expr_deps(prop->expr);
 		if (sym2)
 			break;
diff --git a/scripts/kconfig/tests/err_recursive_dep/expected_stderr b/scripts/kconfig/tests/err_recursive_dep/expected_stderr
index 2eb2528..84679b1 100644
--- a/scripts/kconfig/tests/err_recursive_dep/expected_stderr
+++ b/scripts/kconfig/tests/err_recursive_dep/expected_stderr
@@ -1,5 +1,5 @@
 Kconfig:11:error: recursive dependency detected!
-Kconfig:11:	symbol B is selected or implied by B
+Kconfig:11:	symbol B is selected by B
 For a resolution refer to Documentation/kbuild/kconfig-language.txt
 subsection "Kconfig recursive dependency limitations"
 
@@ -15,14 +15,14 @@ For a resolution refer to Documentation/kbuild/kconfig-language.txt
 subsection "Kconfig recursive dependency limitations"
 
 Kconfig:32:error: recursive dependency detected!
-Kconfig:32:	symbol D2 is selected or implied by D1
+Kconfig:32:	symbol D2 is selected by D1
 Kconfig:27:	symbol D1 depends on D2
 For a resolution refer to Documentation/kbuild/kconfig-language.txt
 subsection "Kconfig recursive dependency limitations"
 
 Kconfig:37:error: recursive dependency detected!
 Kconfig:37:	symbol E1 depends on E2
-Kconfig:42:	symbol E2 is selected or implied by E1
+Kconfig:42:	symbol E2 is implied by E1
 For a resolution refer to Documentation/kbuild/kconfig-language.txt
 subsection "Kconfig recursive dependency limitations"
 
-- 
2.7.4

Re: [PATCH v2 1/3] kconfig: error out when seeing recursive dependency

[Dirk Gouders]

Masahiro Yamada <yamada.masahiro@socionext.com> writes:

> Originally, recursive dependency was a fatal error for Kconfig
> because Kconfig cannot compute symbol values in such a situation.
>
> Commit d595cea62403 ("kconfig: print more info when we see a recursive
> dependency") changed it to a warning, which I guess was not intentional.
>
> Get it back to an error again.
>
> Also, rename the unit test directory "warn_recursive_dep" to
> "err_recursive_dep" so that it matches to the behavior.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

I tested v2 of this series and to me it looks all good, now:

Tested-by: Dirk Gouders <dirk@gouders.net>

> ---
>
> Changes in v2:
>  - Newly added
>
>  scripts/kconfig/symbol.c                                       |  3 ---
>  .../tests/{warn_recursive_dep => err_recursive_dep}/Kconfig    |  0
>  scripts/kconfig/tests/err_recursive_dep/__init__.py            | 10 ++++++++++
>  .../{warn_recursive_dep => err_recursive_dep}/expected_stderr  |  0
>  scripts/kconfig/tests/warn_recursive_dep/__init__.py           |  9 ---------
>  5 files changed, 10 insertions(+), 12 deletions(-)
>  rename scripts/kconfig/tests/{warn_recursive_dep => err_recursive_dep}/Kconfig (100%)
>  create mode 100644 scripts/kconfig/tests/err_recursive_dep/__init__.py
>  rename scripts/kconfig/tests/{warn_recursive_dep => err_recursive_dep}/expected_stderr (100%)
>  delete mode 100644 scripts/kconfig/tests/warn_recursive_dep/__init__.py
>
> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
> index 4ec8b1f..688f487 100644
> --- a/scripts/kconfig/symbol.c
> +++ b/scripts/kconfig/symbol.c
> @@ -1248,9 +1248,6 @@ struct symbol *sym_check_deps(struct symbol *sym)
>  		sym->flags &= ~SYMBOL_CHECK;
>  	}
>  
> -	if (sym2 && sym2 == sym)
> -		sym2 = NULL;
> -
>  	return sym2;
>  }
>  
> diff --git a/scripts/kconfig/tests/warn_recursive_dep/Kconfig b/scripts/kconfig/tests/err_recursive_dep/Kconfig
> similarity index 100%
> rename from scripts/kconfig/tests/warn_recursive_dep/Kconfig
> rename to scripts/kconfig/tests/err_recursive_dep/Kconfig
> diff --git a/scripts/kconfig/tests/err_recursive_dep/__init__.py b/scripts/kconfig/tests/err_recursive_dep/__init__.py
> new file mode 100644
> index 0000000..5f3821b
> --- /dev/null
> +++ b/scripts/kconfig/tests/err_recursive_dep/__init__.py
> @@ -0,0 +1,10 @@
> +# SPDX-License-Identifier: GPL-2.0
> +"""
> +Detect recursive dependency error.
> +
> +Recursive dependency should be treated as an error.
> +"""
> +
> +def test(conf):
> +    assert conf.oldaskconfig() == 1
> +    assert conf.stderr_contains('expected_stderr')
> diff --git a/scripts/kconfig/tests/warn_recursive_dep/expected_stderr b/scripts/kconfig/tests/err_recursive_dep/expected_stderr
> similarity index 100%
> rename from scripts/kconfig/tests/warn_recursive_dep/expected_stderr
> rename to scripts/kconfig/tests/err_recursive_dep/expected_stderr
> diff --git a/scripts/kconfig/tests/warn_recursive_dep/__init__.py b/scripts/kconfig/tests/warn_recursive_dep/__init__.py
> deleted file mode 100644
> index adb2195..0000000
> --- a/scripts/kconfig/tests/warn_recursive_dep/__init__.py
> +++ /dev/null
> @@ -1,9 +0,0 @@
> -"""
> -Warn recursive inclusion.
> -
> -Recursive dependency should be warned.
> -"""
> -
> -def test(conf):
> -    assert conf.oldaskconfig() == 0
> -    assert conf.stderr_contains('expected_stderr')