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

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

[Masahiro Yamada]

Currently, Kconfig does not report anything 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 report 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 is hit.  This will be solved by
the next commit.

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

 scripts/kconfig/symbol.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index 4ec8b1f..7de7463a 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);
-- 
2.7.4

[PATCH 2/2] kconfig: improve the recursive dependency report

[Masahiro Yamada]

This commit improves the warning 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>
---

 scripts/kconfig/symbol.c | 48 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 34 insertions(+), 14 deletions(-)

diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index 7de7463a..9cc443b 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;
-- 
2.7.4

Re: [PATCH 1/2] kconfig: report recursive dependency involving 'imply'

[Dirk Gouders]

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

> Currently, Kconfig does not report anything about the recursive
> dependency where 'imply' keywords are involved.
>
> [Test Code]
>
>   config A
>           bool "a"
>
>   config B
>           bool "b"
>           imply A
>           depends on A

Hello Masahiro,

obviously, it is hard to find a reason why one wants to use dependencies
like above but I also wonder how e.g. menuconfig handles this case:

First, only "a" is visible, if I then select "a", "b" does not become
visible but when I then reset "a" to "n", "b" becomes visible.  If I then
try to select "b", it becomes invisible...

Perhaps it would be better to just error out instead of giving users the
impression, Kconfig thinks such questionable behavior is OK.

Side note: perhaps, the documentation could be better when it comes to
           recursive dependencies.  The documentation says "select" and
           "imply" can be used to specify lower limits whereas direct
           dependencies specify upper limits for symbol values and with
           this in mind, one might wonder why it is a problem to work
           with both limits in a recursive way.

           Not very unlikely that it is just me who still has to
           understand recursive dependencies or problems with reading
           English text, though.

What definitely seems to get void with your patches is item c) in
"Practical solutions to kconfig recursive issue" in
Documentation/kbuild/kconfig-language:

        c) Consider the use of "imply" instead of "select"

Dirk

> 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 report 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 is hit.  This will be solved by
> the next commit.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
>  scripts/kconfig/symbol.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
> index 4ec8b1f..7de7463a 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);

Re: [PATCH 1/2] kconfig: report recursive dependency involving 'imply'

[Dirk Gouders]

Dirk Gouders <dirk@gouders.net> writes:

> Masahiro Yamada <yamada.masahiro@socionext.com> writes:
>
>> Currently, Kconfig does not report anything about the recursive
>> dependency where 'imply' keywords are involved.
>>
>> [Test Code]
>>
>>   config A
>>           bool "a"
>>
>>   config B
>>           bool "b"
>>           imply A
>>           depends on A
>
> Hello Masahiro,
>
> obviously, it is hard to find a reason why one wants to use dependencies
> like above but I also wonder how e.g. menuconfig handles this case:
>
> First, only "a" is visible, if I then select "a", "b" does not become
> visible but when I then reset "a" to "n", "b" becomes visible.  If I then
> try to select "b", it becomes invisible...
>
> Perhaps it would be better to just error out instead of giving users the
> impression, Kconfig thinks such questionable behavior is OK.
>
> Side note: perhaps, the documentation could be better when it comes to
>            recursive dependencies.  The documentation says "select" and
>            "imply" can be used to specify lower limits whereas direct
>            dependencies specify upper limits for symbol values and with
>            this in mind, one might wonder why it is a problem to work
>            with both limits in a recursive way.
>
>            Not very unlikely that it is just me who still has to
>            understand recursive dependencies or problems with reading
>            English text, though.
>
> What definitely seems to get void with your patches is item c) in
> "Practical solutions to kconfig recursive issue" in
> Documentation/kbuild/kconfig-language:
>
>         c) Consider the use of "imply" instead of "select"

Just some more information that adds to me feeling unsure about the
correct definition of recursive dependencies:

With commit 29c434f367ea (kconfig: tests: test if recursive dependencies
are detected) a test case similar to the example above was introduced,
explicitely stating it is _no_ recursive dependency:

+# depends on and imply
+# This is not recursive dependency
+
+config E1
+       bool "E1"
+       depends on E2
+       imply E2
+
+config E2
+       bool "E2"


Dirk

>
>> 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 report 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 is hit.  This will be solved by
>> the next commit.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>>  scripts/kconfig/symbol.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
>> index 4ec8b1f..7de7463a 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);

Re: [PATCH 1/2] kconfig: report recursive dependency involving 'imply'

[Masahiro Yamada]

2018-08-14 22:44 GMT+09:00 Dirk Gouders <dirk@gouders.net>:
> Dirk Gouders <dirk@gouders.net> writes:
>
>> Masahiro Yamada <yamada.masahiro@socionext.com> writes:
>>
>>> Currently, Kconfig does not report anything about the recursive
>>> dependency where 'imply' keywords are involved.
>>>
>>> [Test Code]
>>>
>>>   config A
>>>           bool "a"
>>>
>>>   config B
>>>           bool "b"
>>>           imply A
>>>           depends on A
>>
>> Hello Masahiro,
>>
>> obviously, it is hard to find a reason why one wants to use dependencies
>> like above but I also wonder how e.g. menuconfig handles this case:
>>
>> First, only "a" is visible, if I then select "a", "b" does not become
>> visible but when I then reset "a" to "n", "b" becomes visible.  If I then
>> try to select "b", it becomes invisible...
>>
>> Perhaps it would be better to just error out instead of giving users the
>> impression, Kconfig thinks such questionable behavior is OK.
>>
>> Side note: perhaps, the documentation could be better when it comes to
>>            recursive dependencies.  The documentation says "select" and
>>            "imply" can be used to specify lower limits whereas direct
>>            dependencies specify upper limits for symbol values and with
>>            this in mind, one might wonder why it is a problem to work
>>            with both limits in a recursive way.
>>
>>            Not very unlikely that it is just me who still has to
>>            understand recursive dependencies or problems with reading
>>            English text, though.
>>
>> What definitely seems to get void with your patches is item c) in
>> "Practical solutions to kconfig recursive issue" in
>> Documentation/kbuild/kconfig-language:
>>
>>         c) Consider the use of "imply" instead of "select"
>
> Just some more information that adds to me feeling unsure about the
> correct definition of recursive dependencies:
>
> With commit 29c434f367ea (kconfig: tests: test if recursive dependencies
> are detected) a test case similar to the example above was introduced,
> explicitely stating it is _no_ recursive dependency:
>
> +# depends on and imply
> +# This is not recursive dependency
> +
> +config E1
> +       bool "E1"
> +       depends on E2
> +       imply E2
> +
> +config E2
> +       bool "E2"
>
>
> Dirk


For some reason, I added this
without thinking why.


I believe this should be recursive dependency.


Thanks.






>>
>>> 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 report 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 is hit.  This will be solved by
>>> the next commit.
>>>
>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>>> ---
>>>
>>>  scripts/kconfig/symbol.c | 9 +++++++--
>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
>>> index 4ec8b1f..7de7463a 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);



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 1/2] kconfig: report recursive dependency involving 'imply'

[Masahiro Yamada]

2018-08-14 19:38 GMT+09:00 Dirk Gouders <dirk@gouders.net>:
> Masahiro Yamada <yamada.masahiro@socionext.com> writes:
>
>> Currently, Kconfig does not report anything about the recursive
>> dependency where 'imply' keywords are involved.
>>
>> [Test Code]
>>
>>   config A
>>           bool "a"
>>
>>   config B
>>           bool "b"
>>           imply A
>>           depends on A
>
> Hello Masahiro,
>
> obviously, it is hard to find a reason why one wants to use dependencies
> like above but I also wonder how e.g. menuconfig handles this case:
>
> First, only "a" is visible, if I then select "a", "b" does not become
> visible but when I then reset "a" to "n", "b" becomes visible.  If I then
> try to select "b", it becomes invisible...
>
> Perhaps it would be better to just error out instead of giving users the
> impression, Kconfig thinks such questionable behavior is OK.


Taking closer look at the code, the intention is 'recursive dependency
is error',
but the behavior changed probably by an accident.

I fixed this:
https://patchwork.kernel.org/patch/10566301/

> Side note: perhaps, the documentation could be better when it comes to
>            recursive dependencies.  The documentation says "select" and
>            "imply" can be used to specify lower limits whereas direct
>            dependencies specify upper limits for symbol values and with
>            this in mind, one might wonder why it is a problem to work
>            with both limits in a recursive way.
>
>            Not very unlikely that it is just me who still has to
>            understand recursive dependencies or problems with reading
>            English text, though.


To avoid confusion, two things should be discussed separately:

[1] Unmet dependency
   This is caused by a conflict between the upper limit from 'depends on'
   and the lower limit from 'select'.

    This issue does not happen for 'imply' because the upper limit
    specified by 'imply' is weaker.


[2] Recursive depenency

   This can happen for any combination of 'depends on',
   'select', 'imply', 'if', 'default', etc.


> What definitely seems to get void with your patches is item c) in
> "Practical solutions to kconfig recursive issue" in
> Documentation/kbuild/kconfig-language:
>
>         c) Consider the use of "imply" instead of "select"


I do not know why commit 237e3ad0f195d8 added this line.


Actually, I was also confused.

I sent v2 based on your feedback.

Thanks.



> Dirk
>
>> 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 report 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 is hit.  This will be solved by
>> the next commit.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>>  scripts/kconfig/symbol.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
>> index 4ec8b1f..7de7463a 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);



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 1/2] kconfig: report recursive dependency involving 'imply'

[Dirk Gouders]

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

> 2018-08-14 19:38 GMT+09:00 Dirk Gouders <dirk@gouders.net>:
>> Masahiro Yamada <yamada.masahiro@socionext.com> writes:
>>
>>> Currently, Kconfig does not report anything about the recursive
>>> dependency where 'imply' keywords are involved.
>>>
>>> [Test Code]
>>>
>>>   config A
>>>           bool "a"
>>>
>>>   config B
>>>           bool "b"
>>>           imply A
>>>           depends on A
>>
>> Hello Masahiro,
>>
>> obviously, it is hard to find a reason why one wants to use dependencies
>> like above but I also wonder how e.g. menuconfig handles this case:
>>
>> First, only "a" is visible, if I then select "a", "b" does not become
>> visible but when I then reset "a" to "n", "b" becomes visible.  If I then
>> try to select "b", it becomes invisible...
>>
>> Perhaps it would be better to just error out instead of giving users the
>> impression, Kconfig thinks such questionable behavior is OK.
>
>
> Taking closer look at the code, the intention is 'recursive dependency
> is error',
> but the behavior changed probably by an accident.
>
> I fixed this:
> https://patchwork.kernel.org/patch/10566301/
>
>> Side note: perhaps, the documentation could be better when it comes to
>>            recursive dependencies.  The documentation says "select" and
>>            "imply" can be used to specify lower limits whereas direct
>>            dependencies specify upper limits for symbol values and with
>>            this in mind, one might wonder why it is a problem to work
>>            with both limits in a recursive way.
>>
>>            Not very unlikely that it is just me who still has to
>>            understand recursive dependencies or problems with reading
>>            English text, though.
>
>
> To avoid confusion, two things should be discussed separately:
>
> [1] Unmet dependency
>    This is caused by a conflict between the upper limit from 'depends on'
>    and the lower limit from 'select'.
>
>     This issue does not happen for 'imply' because the upper limit
>     specified by 'imply' is weaker.
>
>
> [2] Recursive depenency
>
>    This can happen for any combination of 'depends on',
>    'select', 'imply', 'if', 'default', etc.

Yes, this is probably just a subject that I still have to get a deeper
understanding for, hence I am easyly confused when faced with
contradicting information.

>> What definitely seems to get void with your patches is item c) in
>> "Practical solutions to kconfig recursive issue" in
>> Documentation/kbuild/kconfig-language:
>>
>>         c) Consider the use of "imply" instead of "select"
>
>
> I do not know why commit 237e3ad0f195d8 added this line.
>
>
> Actually, I was also confused.
>
> I sent v2 based on your feedback.

Thanks for your responses, making that all more understandable.

Dirk

>
>
>> Dirk
>>
>>> 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 report 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 is hit.  This will be solved by
>>> the next commit.
>>>
>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>>> ---
>>>
>>>  scripts/kconfig/symbol.c | 9 +++++++--
>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
>>> index 4ec8b1f..7de7463a 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);