[PATCH 1/2] kconfig: Print full defined and depends for multiply-defined symbols

[PATCH 1/2] kconfig: Print full defined and depends for multiply-defined symbols

[Gregory Fong]

get_symbol_str() was assuming that symbols would only have a single
property for the purpose of printing define and depends information.
This is not true, and one current example is FRAME_POINTER which is
both in lib/Kconfig.debug and arch/arm/Kconfig.debug.

In order to print out the correct Defined and Depends info, iterate
over all properties associated with the given symbol, similarly to was
done for selects.  And for depends, rather than iterating over the
property, just use the direct dependency expression.

CONFIG_FRAME_POINTER text, before:
  Defined at lib/Kconfig.debug:323
  Depends on: DEBUG_KERNEL [=y] && (ARM [=y] || CRIS || M68K || FRV || UML || AVR32 || SUPERH || BLACKFIN || MN10300 || METAG) || ARCH_WANT_FRAME_POINTERS [=n]

After:
  Defined at lib/Kconfig.debug:323, arch/arm/Kconfig.debug:35
  Depends on: DEBUG_KERNEL [=y] && (ARM [=y] || CRIS || M68K || FRV || UML || AVR32 || SUPERH || BLACKFIN || MN10300 || METAG) || ARCH_WANT_FRAME_POINTERS [=n] || !THUMB2_KERNEL [=n]

Removes now-unused function get_symbol_prop().

Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
---
 scripts/kconfig/menu.c | 35 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index 72c9dba..da482ff 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -601,18 +601,6 @@ static void get_prompt_str(struct gstr *r, struct property *prop,
 }
 
 /*
- * get property of type P_SYMBOL
- */
-static struct property *get_symbol_prop(struct symbol *sym)
-{
-	struct property *prop = NULL;
-
-	for_all_properties(sym, prop, P_SYMBOL)
-		break;
-	return prop;
-}
-
-/*
  * head is optional and may be NULL
  */
 void get_symbol_str(struct gstr *r, struct symbol *sym,
@@ -637,15 +625,22 @@ void get_symbol_str(struct gstr *r, struct symbol *sym,
 	for_all_prompts(sym, prop)
 		get_prompt_str(r, prop, head);
 
-	prop = get_symbol_prop(sym);
-	if (prop) {
-		str_printf(r, _("  Defined at %s:%d\n"), prop->menu->file->name,
+	hit = false;
+	for_all_properties(sym, prop, P_SYMBOL) {
+		if (!hit) {
+			str_append(r, "  Defined at ");
+			hit = true;
+		} else
+			str_append(r, ", ");
+		str_printf(r, _("%s:%d"), prop->menu->file->name,
 			prop->menu->lineno);
-		if (!expr_is_yes(prop->visible.expr)) {
-			str_append(r, _("  Depends on: "));
-			expr_gstr_print(prop->visible.expr, r);
-			str_append(r, "\n");
-		}
+	}
+	if (hit)
+		str_append(r, "\n");
+	if (!expr_is_yes(sym->dir_dep.expr)) {
+		str_append(r, _("  Depends on: "));
+		expr_gstr_print(sym->dir_dep.expr, r);
+		str_append(r, "\n");
 	}
 
 	hit = false;
-- 
1.9.1

[PATCH 1/2] kconfig: Print full defined and depends for multiply-defined symbols

[Gregory Fong]

get_symbol_str() was assuming that symbols would only have a single
property for the purpose of printing define and depends information.
This is not true, and one current example is FRAME_POINTER which is
both in lib/Kconfig.debug and arch/arm/Kconfig.debug.

In order to print out the correct Defined and Depends info, iterate
over all properties associated with the given symbol, similarly to was
done for selects.  And for depends, rather than iterating over the
property, just use the direct dependency expression.

CONFIG_FRAME_POINTER text, before:
  Defined at lib/Kconfig.debug:323
  Depends on: DEBUG_KERNEL [=y] && (ARM [=y] || CRIS || M68K || FRV || UML || AVR32 || SUPERH || BLACKFIN || MN10300 || METAG) || ARCH_WANT_FRAME_POINTERS [=n]

After:
  Defined at lib/Kconfig.debug:323, arch/arm/Kconfig.debug:35
  Depends on: DEBUG_KERNEL [=y] && (ARM [=y] || CRIS || M68K || FRV || UML || AVR32 || SUPERH || BLACKFIN || MN10300 || METAG) || ARCH_WANT_FRAME_POINTERS [=n] || !THUMB2_KERNEL [=n]

Removes now-unused function get_symbol_prop().

Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
---
 scripts/kconfig/menu.c | 35 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index 72c9dba..da482ff 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -601,18 +601,6 @@ static void get_prompt_str(struct gstr *r, struct property *prop,
 }
 
 /*
- * get property of type P_SYMBOL
- */
-static struct property *get_symbol_prop(struct symbol *sym)
-{
-	struct property *prop = NULL;
-
-	for_all_properties(sym, prop, P_SYMBOL)
-		break;
-	return prop;
-}
-
-/*
  * head is optional and may be NULL
  */
 void get_symbol_str(struct gstr *r, struct symbol *sym,
@@ -637,15 +625,22 @@ void get_symbol_str(struct gstr *r, struct symbol *sym,
 	for_all_prompts(sym, prop)
 		get_prompt_str(r, prop, head);
 
-	prop = get_symbol_prop(sym);
-	if (prop) {
-		str_printf(r, _("  Defined at %s:%d\n"), prop->menu->file->name,
+	hit = false;
+	for_all_properties(sym, prop, P_SYMBOL) {
+		if (!hit) {
+			str_append(r, "  Defined at ");
+			hit = true;
+		} else
+			str_append(r, ", ");
+		str_printf(r, _("%s:%d"), prop->menu->file->name,
 			prop->menu->lineno);
-		if (!expr_is_yes(prop->visible.expr)) {
-			str_append(r, _("  Depends on: "));
-			expr_gstr_print(prop->visible.expr, r);
-			str_append(r, "\n");
-		}
+	}
+	if (hit)
+		str_append(r, "\n");
+	if (!expr_is_yes(sym->dir_dep.expr)) {
+		str_append(r, _("  Depends on: "));
+		expr_gstr_print(sym->dir_dep.expr, r);
+		str_append(r, "\n");
 	}
 
 	hit = false;
-- 
1.9.1

Re: [PATCH 1/2] kconfig: Print full defined and depends for multiply-defined symbols

[Gregory Fong]

I accidentally marked this as patch 1/2, but this is the only patch.
Sorry for the confusion.

On Wed, Apr 8, 2015 at 4:56 PM, Gregory Fong <gregory.0xf0@gmail.com> wrote:
> get_symbol_str() was assuming that symbols would only have a single
> property for the purpose of printing define and depends information.
> This is not true, and one current example is FRAME_POINTER which is
> both in lib/Kconfig.debug and arch/arm/Kconfig.debug.
>
> In order to print out the correct Defined and Depends info, iterate
> over all properties associated with the given symbol, similarly to was
> done for selects.  And for depends, rather than iterating over the
> property, just use the direct dependency expression.
>
> CONFIG_FRAME_POINTER text, before:
>   Defined at lib/Kconfig.debug:323
>   Depends on: DEBUG_KERNEL [=y] && (ARM [=y] || CRIS || M68K || FRV || UML || AVR32 || SUPERH || BLACKFIN || MN10300 || METAG) || ARCH_WANT_FRAME_POINTERS [=n]
>
> After:
>   Defined at lib/Kconfig.debug:323, arch/arm/Kconfig.debug:35
>   Depends on: DEBUG_KERNEL [=y] && (ARM [=y] || CRIS || M68K || FRV || UML || AVR32 || SUPERH || BLACKFIN || MN10300 || METAG) || ARCH_WANT_FRAME_POINTERS [=n] || !THUMB2_KERNEL [=n]
>
> Removes now-unused function get_symbol_prop().
>
> Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
> [snip]

Re: [PATCH 1/2] kconfig: Print full defined and depends for multiply-defined symbols

[Paul Bolle]

[Removed Yann. Added the people that I hope might actually understand
what this is all about.] 

On Wed, 2015-04-08 at 16:56 -0700, Gregory Fong wrote:
> get_symbol_str() was assuming that symbols would only have a single
> property for the purpose of printing define and depends information.
> This is not true, and one current example is FRAME_POINTER which is
> both in lib/Kconfig.debug and arch/arm/Kconfig.debug.
> 
> In order to print out the correct Defined and Depends info, iterate
> over all properties associated with the given symbol, similarly to was
> done for selects.  And for depends, rather than iterating over the
> property, just use the direct dependency expression.
> 
> CONFIG_FRAME_POINTER text, before:
>   Defined at lib/Kconfig.debug:323
>   Depends on: DEBUG_KERNEL [=y] && (ARM [=y] || CRIS || M68K || FRV || UML || AVR32 || SUPERH || BLACKFIN || MN10300 || METAG) || ARCH_WANT_FRAME_POINTERS [=n]
> 
> After:
>   Defined at lib/Kconfig.debug:323, arch/arm/Kconfig.debug:35
>   Depends on: DEBUG_KERNEL [=y] && (ARM [=y] || CRIS || M68K || FRV || UML || AVR32 || SUPERH || BLACKFIN || MN10300 || METAG) || ARCH_WANT_FRAME_POINTERS [=n] || !THUMB2_KERNEL [=n]

Note that there are currently six places where FRAME_POINTER is defined:
    arch/arm/Kconfig.debug:35:config FRAME_POINTER
    arch/arm64/Kconfig.debug:5:config FRAME_POINTER
    arch/hexagon/Kconfig:40:config FRAME_POINTER
    arch/m32r/Kconfig.debug:13:config FRAME_POINTER
    arch/sparc/Kconfig.debug:19:config FRAME_POINTER
    lib/Kconfig.debug:323:config FRAME_POINTER

Anyhow, does anyone dare to state that the After: line above describes
FRAME_POINTER for arm correctly? (Of course, I could dive in the semi
documented kconfig code myself. But that might mean that Gregory will be
waiting for feedback for quite some time.)

> Removes now-unused function get_symbol_prop().

It might be better to do that in a separate patch. 

> Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
> ---
>  scripts/kconfig/menu.c | 35 +++++++++++++++--------------------
>  1 file changed, 15 insertions(+), 20 deletions(-)
> 
> diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
> index 72c9dba..da482ff 100644
> --- a/scripts/kconfig/menu.c
> +++ b/scripts/kconfig/menu.c
> @@ -601,18 +601,6 @@ static void get_prompt_str(struct gstr *r, struct property *prop,
>  }
>  
>  /*
> - * get property of type P_SYMBOL
> - */
> -static struct property *get_symbol_prop(struct symbol *sym)
> -{
> -	struct property *prop = NULL;
> -
> -	for_all_properties(sym, prop, P_SYMBOL)
> -		break;
> -	return prop;
> -}
> -
> -/*
>   * head is optional and may be NULL
>   */
>  void get_symbol_str(struct gstr *r, struct symbol *sym,
> @@ -637,15 +625,22 @@ void get_symbol_str(struct gstr *r, struct symbol *sym,
>  	for_all_prompts(sym, prop)
>  		get_prompt_str(r, prop, head);
>  
> -	prop = get_symbol_prop(sym);
> -	if (prop) {
> -		str_printf(r, _("  Defined at %s:%d\n"), prop->menu->file->name,
> +	hit = false;
> +	for_all_properties(sym, prop, P_SYMBOL) {
> +		if (!hit) {
> +			str_append(r, "  Defined at ");
> +			hit = true;
> +		} else
> +			str_append(r, ", ");
> +		str_printf(r, _("%s:%d"), prop->menu->file->name,
>  			prop->menu->lineno);
> -		if (!expr_is_yes(prop->visible.expr)) {
> -			str_append(r, _("  Depends on: "));
> -			expr_gstr_print(prop->visible.expr, r);
> -			str_append(r, "\n");
> -		}
> +	}
> +	if (hit)
> +		str_append(r, "\n");
> +	if (!expr_is_yes(sym->dir_dep.expr)) {
> +		str_append(r, _("  Depends on: "));
> +		expr_gstr_print(sym->dir_dep.expr, r);
> +		str_append(r, "\n");
>  	}
>  
>  	hit = false;

Thanks,


Paul Bolle

Re: [PATCH 1/2] kconfig: Print full defined and depends for multiply-defined symbols

[Stefan Hengelein]

2015-04-10 23:25 GMT+02:00 Paul Bolle <pebolle@tiscali.nl>:
> [Removed Yann. Added the people that I hope might actually understand
> what this is all about.]
>
> On Wed, 2015-04-08 at 16:56 -0700, Gregory Fong wrote:
>> get_symbol_str() was assuming that symbols would only have a single
>> property for the purpose of printing define and depends information.
>> This is not true, and one current example is FRAME_POINTER which is
>> both in lib/Kconfig.debug and arch/arm/Kconfig.debug.
>>
>> In order to print out the correct Defined and Depends info, iterate
>> over all properties associated with the given symbol, similarly to was
>> done for selects.  And for depends, rather than iterating over the
>> property, just use the direct dependency expression.
>>
>> CONFIG_FRAME_POINTER text, before:
>>   Defined at lib/Kconfig.debug:323
>>   Depends on: DEBUG_KERNEL [=y] && (ARM [=y] || CRIS || M68K || FRV || UML || AVR32 || SUPERH || BLACKFIN || MN10300 || METAG) || ARCH_WANT_FRAME_POINTERS [=n]
>>
>> After:
>>   Defined at lib/Kconfig.debug:323, arch/arm/Kconfig.debug:35
>>   Depends on: DEBUG_KERNEL [=y] && (ARM [=y] || CRIS || M68K || FRV || UML || AVR32 || SUPERH || BLACKFIN || MN10300 || METAG) || ARCH_WANT_FRAME_POINTERS [=n] || !THUMB2_KERNEL [=n]
>
> Note that there are currently six places where FRAME_POINTER is defined:
>     arch/arm/Kconfig.debug:35:config FRAME_POINTER
>     arch/arm64/Kconfig.debug:5:config FRAME_POINTER
>     arch/hexagon/Kconfig:40:config FRAME_POINTER
>     arch/m32r/Kconfig.debug:13:config FRAME_POINTER
>     arch/sparc/Kconfig.debug:19:config FRAME_POINTER
>     lib/Kconfig.debug:323:config FRAME_POINTER
>
> Anyhow, does anyone dare to state that the After: line above describes
> FRAME_POINTER for arm correctly? (Of course, I could dive in the semi
> documented kconfig code myself. But that might mean that Gregory will be
> waiting for feedback for quite some time.)
>

With my current knowledge of kconfig i would say: depends on what the
output of this is used for.
If you're reading the dependency list as "what do i have to enable to
be able to choose a value for FRAME_POINTER" and think, THUMB2_KERNEL
would be a good choice to leave disabled, you're going to have a bad
time.
(The second definition in arm/Kconfig.debug doesn't have a prompt and
the default has additional conditions)

If it should just point out, FRAME_POINTER could be enabled, the
"After Depends on:" is right. However, the conditions for the Default
in the second definition still would have to be satisfied.


If the whole list of kconfig menus is dumped, i'd expect there to be a
second print:
Defined at arch/arm/Kconfig.debug:35
Depends on: !THUMB2_KERNEL [=n]


After doing some reading:
the function is used in the search and help function for mconf to
print the dependencies and, at least in my output, did include a
Prompt statement. i'd say to add "!THUMB2_KERNEL" to the dependency
could be misleading! however the second definition of the symbol is
not hit by a search for FRAME_POINTER. I personally would prefer to
additionally find the second definition that doesn't a prompt and
other dependencies instead of adding them to the first entry, but
that's just my personal preference.


Stefan


>> Removes now-unused function get_symbol_prop().
>
> It might be better to do that in a separate patch.
>
>> Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
>> ---
>>  scripts/kconfig/menu.c | 35 +++++++++++++++--------------------
>>  1 file changed, 15 insertions(+), 20 deletions(-)
>>
>> diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
>> index 72c9dba..da482ff 100644
>> --- a/scripts/kconfig/menu.c
>> +++ b/scripts/kconfig/menu.c
>> @@ -601,18 +601,6 @@ static void get_prompt_str(struct gstr *r, struct property *prop,
>>  }
>>
>>  /*
>> - * get property of type P_SYMBOL
>> - */
>> -static struct property *get_symbol_prop(struct symbol *sym)
>> -{
>> -     struct property *prop = NULL;
>> -
>> -     for_all_properties(sym, prop, P_SYMBOL)
>> -             break;
>> -     return prop;
>> -}
>> -
>> -/*
>>   * head is optional and may be NULL
>>   */
>>  void get_symbol_str(struct gstr *r, struct symbol *sym,
>> @@ -637,15 +625,22 @@ void get_symbol_str(struct gstr *r, struct symbol *sym,
>>       for_all_prompts(sym, prop)
>>               get_prompt_str(r, prop, head);
>>
>> -     prop = get_symbol_prop(sym);
>> -     if (prop) {
>> -             str_printf(r, _("  Defined at %s:%d\n"), prop->menu->file->name,
>> +     hit = false;
>> +     for_all_properties(sym, prop, P_SYMBOL) {
>> +             if (!hit) {
>> +                     str_append(r, "  Defined at ");
>> +                     hit = true;
>> +             } else
>> +                     str_append(r, ", ");
>> +             str_printf(r, _("%s:%d"), prop->menu->file->name,
>>                       prop->menu->lineno);
>> -             if (!expr_is_yes(prop->visible.expr)) {
>> -                     str_append(r, _("  Depends on: "));
>> -                     expr_gstr_print(prop->visible.expr, r);
>> -                     str_append(r, "\n");
>> -             }
>> +     }
>> +     if (hit)
>> +             str_append(r, "\n");
>> +     if (!expr_is_yes(sym->dir_dep.expr)) {
>> +             str_append(r, _("  Depends on: "));
>> +             expr_gstr_print(sym->dir_dep.expr, r);
>> +             str_append(r, "\n");
>>       }
>>
>>       hit = false;
>
> Thanks,
>
>
> Paul Bolle
>

Re: [PATCH 1/2] kconfig: Print full defined and depends for multiply-defined symbols

[Paul Bolle]

On Sat, 2015-04-11 at 18:36 +0200, Stefan Hengelein wrote:
> If you're reading the dependency list as "what do i have to enable to
> be able to choose a value for FRAME_POINTER" and think, THUMB2_KERNEL
> would be a good choice to leave disabled, you're going to have a bad
> time.
> (The second definition in arm/Kconfig.debug doesn't have a prompt and
> the default has additional conditions)

Please elaborate on "bad time".

> I personally would prefer to
> additionally find the second definition that doesn't a prompt and
> other dependencies instead of adding them to the first entry, but
> that's just my personal preference.

I notice myself getting rather grumpy. (That usually translates to:
"Drop it, and revisit in a few days".) Let me explain.

This is the arm64 entry:
    config FRAME_POINTER
            bool
            default y

This is the hexagon entry
    config FRAME_POINTER
            def_bool y

This is the m32r entry:
    config FRAME_POINTER
            bool "Compile the kernel with frame pointers"
            help
              If you say Y here [...]

And this is the sparc entry:
    config FRAME_POINTER
            bool
            depends on MCOUNT
            default y

You'd expect these entries to yield really simple results when doing a
search in menuconfig. But the results show unparseable crap[1]. (And I'm
afraid Gregory's patch would make that even worse. Gregory: please prove
me wrong.)

So to the grumpy me it looks like either:
- menuconfig handles these redefinitions incorrectly in its UI;
- these redefinitions are actually complicated (as in: somehow they
concatenate the dependencies, etc.) and we should probably disallow
them. Because otherwise looking at a Kconfig entry tells you very little
about what is actually going on for the architecture you're interested
in.

What is the grumpy me missing here?


Paul Bolle

[1] The hexagon entry is interesting, probably because it sources
lib/Kconfig.debug _after_ it defined FRAME_POINTER for itself.

Re: [PATCH 1/2] kconfig: Print full defined and depends for multiply-defined symbols

[Stefan Hengelein]

2015-04-11 20:56 GMT+02:00 Paul Bolle <pebolle@tiscali.nl>:
> On Sat, 2015-04-11 at 18:36 +0200, Stefan Hengelein wrote:
>> If you're reading the dependency list as "what do i have to enable to
>> be able to choose a value for FRAME_POINTER" and think, THUMB2_KERNEL
>> would be a good choice to leave disabled, you're going to have a bad
>> time.
>> (The second definition in arm/Kconfig.debug doesn't have a prompt and
>> the default has additional conditions)
>
> Please elaborate on "bad time".

What i meant to say, you won't get a prompt (or for mconf, won't see
it in the menu) if THUMB2_KERNEL is disabled, FRAME_POINTER will
simply be enabled when the default condition in the definition without
the prompt is satisfied.

Therefore it might be misleading to add it to the conditions.

>
>> I personally would prefer to
>> additionally find the second definition that doesn't have a prompt and
>> other dependencies instead of adding them to the first entry, but
>> that's just my personal preference.
>
> I notice myself getting rather grumpy. (That usually translates to:
> "Drop it, and revisit in a few days".) Let me explain.
>
> This is the arm64 entry:
>     config FRAME_POINTER
>             bool
>             default y
>
> This is the hexagon entry
>     config FRAME_POINTER
>             def_bool y
>
> This is the m32r entry:
>     config FRAME_POINTER
>             bool "Compile the kernel with frame pointers"
>             help
>               If you say Y here [...]
>
> And this is the sparc entry:
>     config FRAME_POINTER
>             bool
>             depends on MCOUNT
>             default y
>
> You'd expect these entries to yield really simple results when doing a
> search in menuconfig. But the results show unparseable crap[1]. (And I'm
> afraid Gregory's patch would make that even worse. Gregory: please prove
> me wrong.)

would you please define unparseable crap? the only odd thing i notice
when i call menuconfig on hexagon is a really long "Selected by: "
list

>
> So to the grumpy me it looks like either:
> - menuconfig handles these redefinitions incorrectly in its UI;
> - these redefinitions are actually complicated (as in: somehow they
> concatenate the dependencies, etc.) and we should probably disallow
> them. Because otherwise looking at a Kconfig entry tells you very little
> about what is actually going on for the architecture you're interested
> in.
>
> What is the grumpy me missing here?

Redefinitions are more of an "overwrite" than a "add conditions to the entry".
It's perfectly reasonable for architecture A to say: if these
conditions hold, i want to enable option B, not matter what the
Kconfigfile in lib/ says (like arm64 does with FRAME_POINTER, it is
always on, (depending on if there are other dependencies around it)).

Redefinitions are a little more complicated...
If you have two options with the same symbol and both have a prompt,
you will see it two times in conf. Meaning, Kconfig doesn't merge both
declarations but they are separate two different instructions,
affecting the same symbol.
With menuconfig it's the same, it will show both definitions in the
menu, they might however be in another submenu, depending on the
dependencies both definitions have.

The kconfig rules state only one definition should have a prompt, but
as you can see, m32r does violate this "rule" and it doesn't break
kconfig ;)

That's why i said i'd prefer to have both declarations printed than
adding the conditions from the second definition to the printed entry
of the first....
If the order is different, you might only see the definition without
the prompt (what happens for hexagon) and miss the second possibility
to enable the feature.


>
> [1] The hexagon entry is interesting, probably because it sources
> lib/Kconfig.debug _after_ it defined FRAME_POINTER for itself.
>


Stefan

Re: [PATCH 1/2] kconfig: Print full defined and depends for multiply-defined symbols

[Paul Bolle]

On Sat, 2015-04-11 at 21:58 +0200, Stefan Hengelein wrote:
> 2015-04-11 20:56 GMT+02:00 Paul Bolle <pebolle@tiscali.nl>:
> > On Sat, 2015-04-11 at 18:36 +0200, Stefan Hengelein wrote:
> What i meant to say, you won't get a prompt (or for mconf, won't see
> it in the menu) if THUMB2_KERNEL is disabled, FRAME_POINTER will
> simply be enabled when the default condition in the definition without
> the prompt is satisfied.
> 
> Therefore it might be misleading to add it to the conditions.

That's a NAK to this patch, isn't it?

> >> I personally would prefer to
> >> additionally find the second definition that doesn't have a prompt and
> >> other dependencies instead of adding them to the first entry, but
> >> that's just my personal preference.
> >
> > I notice myself getting rather grumpy. (That usually translates to:
> > "Drop it, and revisit in a few days".) Let me explain.
> >
> > This is the arm64 entry:
> >     config FRAME_POINTER
> >             bool
> >             default y
> >
> > This is the hexagon entry
> >     config FRAME_POINTER
> >             def_bool y
> >
> > This is the m32r entry:
> >     config FRAME_POINTER
> >             bool "Compile the kernel with frame pointers"
> >             help
> >               If you say Y here [...]
> >
> > And this is the sparc entry:
> >     config FRAME_POINTER
> >             bool
> >             depends on MCOUNT
> >             default y
> >
> > You'd expect these entries to yield really simple results when doing a
> > search in menuconfig. But the results show unparseable crap[1]. (And I'm
> > afraid Gregory's patch would make that even worse. Gregory: please prove
> > me wrong.)
> 
> would you please define unparseable crap?

This is what I see for m32r:
    Depends on: DEBUG_KERNEL [=y] && (CRIS || M68K || FRV || UML || AVR32 || SUPERH || BLACKFIN || MN10300 || METAG) || [...] 
    Selected by: FAULT_INJECTION_STACKTRACE_FILTER [=n] && FAULT_INJECTION_DEBUG_FS [=n] && STACKTRACE_SUPPORT && !X86_6[...]

No one is going to understand what that means. (Did I say I was grumpy?)
Sure, it might be actually correct for most architectures. But it
resembles in no way what one expects to see after reading just the m32r
entry.

>  the only odd thing i notice
> when i call menuconfig on hexagon is a really long "Selected by: "
> list

Yes. That list makes no sense whatsoever. (Did I say I was grumpy?)

> > So to the grumpy me it looks like either:
> > - menuconfig handles these redefinitions incorrectly in its UI;
> > - these redefinitions are actually complicated (as in: somehow they
> > concatenate the dependencies, etc.) and we should probably disallow
> > them. Because otherwise looking at a Kconfig entry tells you very little
> > about what is actually going on for the architecture you're interested
> > in.
> >
> > What is the grumpy me missing here?
> 
> Redefinitions are more of an "overwrite" than a "add conditions to the entry".

That's again a NAK to this patch, isn't it?

> It's perfectly reasonable for architecture A to say: if these
> conditions hold, i want to enable option B, not matter what the
> Kconfigfile in lib/ says (like arm64 does with FRAME_POINTER, it is
> always on, (depending on if there are other dependencies around it)).
> 
> Redefinitions are a little more complicated...
> If you have two options with the same symbol and both have a prompt,
> you will see it two times in conf. Meaning, Kconfig doesn't merge both
> declarations but they are separate two different instructions,
> affecting the same symbol.

You lost me there.

> With menuconfig it's the same, it will show both definitions in the
> menu, they might however be in another submenu, depending on the
> dependencies both definitions have.
> 
> The kconfig rules state only one definition should have a prompt, but
> as you can see, m32r does violate this "rule" and it doesn't break
> kconfig ;)
> 
> That's why i said i'd prefer to have both declarations printed than
> adding the conditions from the second definition to the printed entry
> of the first....
> If the order is different, you might only see the definition without
> the prompt (what happens for hexagon) and miss the second possibility
> to enable the feature.

I'm beyond confused now. (But happy to have dragged you into this
discussion. I think we're making progress.)

I'd really prefer things to be simpler: how is anyone reading the
Kconfig entries I quoted going to realize all that?


Paul Bolle

Re: [PATCH 1/2] kconfig: Print full defined and depends for multiply-defined symbols

[Stefan Hengelein]

2015-04-11 22:23 GMT+02:00 Paul Bolle <pebolle@tiscali.nl>:
> On Sat, 2015-04-11 at 21:58 +0200, Stefan Hengelein wrote:
>> 2015-04-11 20:56 GMT+02:00 Paul Bolle <pebolle@tiscali.nl>:
>> > On Sat, 2015-04-11 at 18:36 +0200, Stefan Hengelein wrote:
>> What i meant to say, you won't get a prompt (or for mconf, won't see
>> it in the menu) if THUMB2_KERNEL is disabled, FRAME_POINTER will
>> simply be enabled when the default condition in the definition without
>> the prompt is satisfied.
>>
>> Therefore it might be misleading to add it to the conditions.
>
> That's a NAK to this patch, isn't it?

That's not for me to decide. Maybe I missed something!
But I wouldn't merge it in the current state.

>
>> >> I personally would prefer to
>> >> additionally find the second definition that doesn't have a prompt and
>> >> other dependencies instead of adding them to the first entry, but
>> >> that's just my personal preference.
>> >
>> > I notice myself getting rather grumpy. (That usually translates to:
>> > "Drop it, and revisit in a few days".) Let me explain.
>> >
>> > This is the arm64 entry:
>> >     config FRAME_POINTER
>> >             bool
>> >             default y
>> >
>> > This is the hexagon entry
>> >     config FRAME_POINTER
>> >             def_bool y
>> >
>> > This is the m32r entry:
>> >     config FRAME_POINTER
>> >             bool "Compile the kernel with frame pointers"
>> >             help
>> >               If you say Y here [...]
>> >
>> > And this is the sparc entry:
>> >     config FRAME_POINTER
>> >             bool
>> >             depends on MCOUNT
>> >             default y
>> >
>> > You'd expect these entries to yield really simple results when doing a
>> > search in menuconfig. But the results show unparseable crap[1]. (And I'm
>> > afraid Gregory's patch would make that even worse. Gregory: please prove
>> > me wrong.)
>>
>> would you please define unparseable crap?
>
> This is what I see for m32r:
>     Depends on: DEBUG_KERNEL [=y] && (CRIS || M68K || FRV || UML || AVR32 || SUPERH || BLACKFIN || MN10300 || METAG) || [...]
>     Selected by: FAULT_INJECTION_STACKTRACE_FILTER [=n] && FAULT_INJECTION_DEBUG_FS [=n] && STACKTRACE_SUPPORT && !X86_6[...]
>
> No one is going to understand what that means. (Did I say I was grumpy?)
> Sure, it might be actually correct for most architectures. But it
> resembles in no way what one expects to see after reading just the m32r
> entry.
>
>>  the only odd thing i notice
>> when i call menuconfig on hexagon is a really long "Selected by: "
>> list
>
> Yes. That list makes no sense whatsoever. (Did I say I was grumpy?)

Well, FAULT_INJECTION_STACKTRACE_FILTER selects FRAME_POINTER if
FAULT_INJECTION_DEBUG_FS && STACKTRACE_SUPPORT && !X86_64 && [...]

i'm assuming the [=X] statements say what the current value is.
To see the other architecture symbols is just a matter of how that
symbol was modelled, i guess it wouldn't be easy to filter that out in
a generic way.

>
>> > So to the grumpy me it looks like either:
>> > - menuconfig handles these redefinitions incorrectly in its UI;
>> > - these redefinitions are actually complicated (as in: somehow they
>> > concatenate the dependencies, etc.) and we should probably disallow
>> > them. Because otherwise looking at a Kconfig entry tells you very little
>> > about what is actually going on for the architecture you're interested
>> > in.
>> >
>> > What is the grumpy me missing here?
>>
>> Redefinitions are more of an "overwrite" than a "add conditions to the entry".
>
> That's again a NAK to this patch, isn't it?
>
>> It's perfectly reasonable for architecture A to say: if these
>> conditions hold, i want to enable option B, not matter what the
>> Kconfigfile in lib/ says (like arm64 does with FRAME_POINTER, it is
>> always on, (depending on if there are other dependencies around it)).
>>
>> Redefinitions are a little more complicated...
>> If you have two options with the same symbol and both have a prompt,
>> you will see it two times in conf. Meaning, Kconfig doesn't merge both
>> declarations but they are separate two different instructions,
>> affecting the same symbol.
>
> You lost me there.

Create a Kconfig file with
config A
    bool "A"

config A
    bool "B"

call scripts/kconfig/conf on it and you'll see what i meant.

>
>> With menuconfig it's the same, it will show both definitions in the
>> menu, they might however be in another submenu, depending on the
>> dependencies both definitions have.
>>
>> The kconfig rules state only one definition should have a prompt, but
>> as you can see, m32r does violate this "rule" and it doesn't break
>> kconfig ;)
>>
>> That's why i said i'd prefer to have both declarations printed than
>> adding the conditions from the second definition to the printed entry
>> of the first....
>> If the order is different, you might only see the definition without
>> the prompt (what happens for hexagon) and miss the second possibility
>> to enable the feature.
>
> I'm beyond confused now. (But happy to have dragged you into this
> discussion. I think we're making progress.)
>
> I'd really prefer things to be simpler: how is anyone reading the
> Kconfig entries I quoted going to realize all that?
>

No one has to, most of the things i explained to you come from a few
years of experience with Kconfig. FRAME_POINTER is a complicated
example, it is selected although it has dependencies or a prompt AND
it is redefined in many architectures.
AFAIUI, the "depends on" or "selected by" output should give hints
what you have to enable to get a prompt for that option or simply
enable it.

Wouldn't mentioning a symbol two times (because there are two
declarations) also confuse users if they search for FRAME_POINTER? But
at least it would give hints were both declarations are defined
without mixing them up.


Stefan

Re: [PATCH 1/2] kconfig: Print full defined and depends for multiply-defined symbols

[Paul Bolle]

On Sat, 2015-04-11 at 23:46 +0200, Stefan Hengelein wrote:
> 2015-04-11 22:23 GMT+02:00 Paul Bolle <pebolle@tiscali.nl>:
> > That's a NAK to this patch, isn't it?
> 
> That's not for me to decide. Maybe I missed something!
> But I wouldn't merge it in the current state.

Thanks. That's all I needed to hear.

> > I'd really prefer things to be simpler: how is anyone reading the
> > Kconfig entries I quoted going to realize all that?
> 
> No one has to, most of the things i explained to you come from a few
> years of experience with Kconfig. FRAME_POINTER is a complicated
> example, it is selected although it has dependencies or a prompt AND
> it is redefined in many architectures.
> AFAIUI, the "depends on" or "selected by" output should give hints
> what you have to enable to get a prompt for that option or simply
> enable it.
> 
> Wouldn't mentioning a symbol two times (because there are two
> declarations) also confuse users if they search for FRAME_POINTER? But
> at least it would give hints were both declarations are defined
> without mixing them up.

I think we can forget about this patch.

Let's focus, for example, on m32r and FRAME_POINTER. The m32r entry for
that symbol reads:
        config FRAME_POINTER
                bool "Compile the kernel with frame pointers"
                help
                  If you say Y here [...]

0) If one is building for m32r is that all there's to it? If so, "make
menuconfig"'s search facility is serving the people building for m32r a
load of crap.

1) If it's actually more complicated than that I think that anyone
reading arch/m32r/Kconfig.debug is being duped. Things look simple but
actually they are quite complicated. I think that's just wrong.

What am I missing here?


Paul Bolle

Re: [PATCH 1/2] kconfig: Print full defined and depends for multiply-defined symbols

[Stefan Hengelein]

> Let's focus, for example, on m32r and FRAME_POINTER. The m32r entry for
> that symbol reads:
>         config FRAME_POINTER
>                 bool "Compile the kernel with frame pointers"
>                 help
>                   If you say Y here [...]
>
> 0) If one is building for m32r is that all there's to it? If so, "make
> menuconfig"'s search facility is serving the people building for m32r a
> load of crap.
>
> 1) If it's actually more complicated than that I think that anyone
> reading arch/m32r/Kconfig.debug is being duped. Things look simple but
> actually they are quite complicated. I think that's just wrong.
>
> What am I missing here?

If you have a look at the definitions, lib/Kconfig.debug is included
before FRAME_POINTER is defined in m32r and the output in the search
facility looks indeed broken
as one "Defined at" is missing but there are somehow Location entries
(-> Kernel hacking    and  -> Kernel hacking -> compile time checks
and [...]) for both definitions in a weird order (i think (1) and (2)
might indicate both definitions)

both declarations are valid in kconfig, you have two ways of enabling
the same symbol, one easy without conditions and one with conditions
and both with the same prompt.

The search facility shows the first one that is found, you see the
complicated depends on but i think the text shown might not be
explicit enough to clarify you don't need to satisfy these complicated
conditions to actually choose a value.

Stefan

Re: [PATCH 1/2] kconfig: Print full defined and depends for multiply-defined symbols

[Gregory Fong]

Hi Paul and Stefan,

Thanks for taking a look at this.  I think Stefan has touched upon why
he thinks this change might (at least partially) make sense, but let
me now try to explain the rationale behind this patch better than I
did in the commit message.

On Sun, Apr 12, 2015 at 8:02 AM, Stefan Hengelein
<stefan.hengelein@fau.de> wrote:
>> Let's focus, for example, on m32r and FRAME_POINTER. The m32r entry for
>> that symbol reads:
>>         config FRAME_POINTER
>>                 bool "Compile the kernel with frame pointers"
>>                 help
>>                   If you say Y here [...]
>>
>> 0) If one is building for m32r is that all there's to it? If so, "make
>> menuconfig"'s search facility is serving the people building for m32r a
>> load of crap.
>>
>> 1) If it's actually more complicated than that I think that anyone
>> reading arch/m32r/Kconfig.debug is being duped. Things look simple but
>> actually they are quite complicated. I think that's just wrong.
>>
>> What am I missing here?
>
> If you have a look at the definitions, lib/Kconfig.debug is included
> before FRAME_POINTER is defined in m32r and the output in the search
> facility looks indeed broken
> as one "Defined at" is missing but there are somehow Location entries
> (-> Kernel hacking    and  -> Kernel hacking -> compile time checks
> and [...]) for both definitions in a weird order (i think (1) and (2)
> might indicate both definitions)
>
> both declarations are valid in kconfig, you have two ways of enabling
> the same symbol, one easy without conditions and one with conditions
> and both with the same prompt.
>
> The search facility shows the first one that is found, you see the
> complicated depends on but i think the text shown might not be
> explicit enough to clarify you don't need to satisfy these complicated
> conditions to actually choose a value.
>

Well, the thing is, you do need to satisfy _one_ of the set of
conditions.  But unfortunately it's not not possible to see that from
the search function because it prints out incomplete information.

This is exactly the motivation for this change.  When you search for
FRAME_POINTER, you only get information on one "Defined at", and the
only "Depends" information is from that definition.  This is clearly
incorrect, as FRAME_POINTER is defined in multiple places, so the
information printed does not cover its actual definitions or
dependencies.  The printed "Selected by" information is, however,
correct, because it actually uses the rev_dep expression.  This patch
changes the "Depends" information to be similar: it will also print
the actual expression used by kconfig used to determine whether its
direct dependencies are fulfilled.

Is this overly complicated or confusing?  Perhaps.  But it is better
than printing out an incomplete set of definitions and dependencies,
which is the current behavior.

As Stefan mentioned, "FRAME_POINTER is a complicated example, it is
selected although it has dependencies or a prompt AND it is redefined
in many architectures".  I'm pretty new to the Kconfig code, but to
me, the multiple symbol behavior is bizarre.  This is because:
- every definition is independent---dependencies are OR'd together
from each definition.
- each definition with a prompt creates a new location where that
symbol can be changed in menuconfig
- each definition _without_ a prompt that has its dependencies
fulfilled results in the default value set in that definition being
used unconditionally.  E.g. for FRAME_POINTER, this means that it is
impossible to disable the option on ARM.  I have submitted a separate
patch to try to fix that for ARM at least,
https://lkml.org/lkml/2015/4/8/765

I definitely think more thought needs to be put into cleaning up the
multiple definition behavior, as it is very confusing to follow.  But
the current behavior is flat-out wrong, as the search function does
not actually print out the actual set of definitions or dependencies
used by kconfig, and that is all that this patch is aiming to fix.

Aside: if the issue is in how this is displayed, maybe it would be
better to add in some parentheses and/or linebreaks to make it clearer
that these are distinct sets, so rather than printing

  Defined at lib/Kconfig.debug:323, arch/arm/Kconfig.debug:35
  Depends on: DEBUG_KERNEL [=y] && (CRIS || M68K || FRV || UML ||
AVR32 || SUPERH || BLACKFIN || MN10300 || METAG) ||
ARCH_WANT_FRAME_POINTERS [=n] || !THUMB2_KERNEL [=n]

you get something more like

  Defined at lib/Kconfig.debug:323, arch/arm/Kconfig.debug:35
  Depends on:
    (DEBUG_KERNEL [=y] && (CRIS || M68K || FRV || UML || AVR32 ||
SUPERH || BLACKFIN || MN10300 || METAG) || ARCH_WANT_FRAME_POINTERS
[=n]) ||
    (!THUMB2_KERNEL [=n])

which makes the logic a little bit more obvious.  (Also, sorry, the
ARM=y in the original before/after was from a separate change I was
testing.  But it doesn't affect what we get here because
THUMB_KERNEL=n anyway).  You could do this by iterating over the
associated properties rather than just printing out the dir_dep
expression of the symbol, but then you'd probably to change rev_dep to
do the same.  I don't really like this, because it doesn't match up
with what kconfig is _actually_ using (i.e. the rev_dep and dir_dep of
the associated symbol).

Best regards,
Gregory

Re: [PATCH 1/2] kconfig: Print full defined and depends for multiply-defined symbols

[Paul Bolle]

Hi Gregory,

On Sun, 2015-04-12 at 18:06 -0700, Gregory Fong wrote:
> On Sun, Apr 12, 2015 at 8:02 AM, Stefan Hengelein
> > If you have a look at the definitions, lib/Kconfig.debug is included
> > before FRAME_POINTER is defined in m32r and the output in the search
> > facility looks indeed broken
> > as one "Defined at" is missing but there are somehow Location entries
> > (-> Kernel hacking    and  -> Kernel hacking -> compile time checks
> > and [...]) for both definitions in a weird order (i think (1) and (2)
> > might indicate both definitions)
> >
> > both declarations are valid in kconfig, you have two ways of enabling
> > the same symbol, one easy without conditions and one with conditions
> > and both with the same prompt.

At least they're connected. Setting it at one location changes the other
location too.

> > The search facility shows the first one that is found, you see the
> > complicated depends on but i think the text shown might not be
> > explicit enough to clarify you don't need to satisfy these complicated
> > conditions to actually choose a value.
> >
> 
> Well, the thing is, you do need to satisfy _one_ of the set of
> conditions.  But unfortunately it's not not possible to see that from
> the search function because it prints out incomplete information.
> 
> This is exactly the motivation for this change.  When you search for
> FRAME_POINTER, you only get information on one "Defined at", and the
> only "Depends" information is from that definition.  This is clearly
> incorrect, as FRAME_POINTER is defined in multiple places, so the
> information printed does not cover its actual definitions or
> dependencies.  The printed "Selected by" information is, however,
> correct, because it actually uses the rev_dep expression.  This patch
> changes the "Depends" information to be similar: it will also print
> the actual expression used by kconfig used to determine whether its
> direct dependencies are fulfilled.
> 
> Is this overly complicated or confusing?  Perhaps.  But it is better
> than printing out an incomplete set of definitions and dependencies,
> which is the current behavior.
> 
> As Stefan mentioned, "FRAME_POINTER is a complicated example, it is
> selected although it has dependencies or a prompt AND it is redefined
> in many architectures".  I'm pretty new to the Kconfig code, but to
> me, the multiple symbol behavior is bizarre.

I didn't yet stumble on a rationale of the multiple definition behavior.
Without knowing that rationale things look rather bizarre.

>   This is because:
> - every definition is independent---dependencies are OR'd together
> from each definition.

(Embarrassing question: where in the code does that happen?)

> - each definition with a prompt creates a new location where that
> symbol can be changed in menuconfig

At least they are connected, see above.

> - each definition _without_ a prompt that has its dependencies
> fulfilled results in the default value set in that definition being
> used unconditionally.  E.g. for FRAME_POINTER, this means that it is
> impossible to disable the option on ARM.  I have submitted a separate
> patch to try to fix that for ARM at least,
> https://lkml.org/lkml/2015/4/8/765

I didn't see that patch. It's probably closely related to what we
discuss here. Please send closely related patches as series. (Was it
perhaps intended to be the 2/2 you never sent?)

I find the arm64 entry for this symbol interesting too. Since ARM64 will
always select ARCH_WANT_FRAME_POINTERS I think there's no reason for it
to have a separate FRAME_POINTER entry in the first place. (I didn't yet
bother to look at the git history of arm64.)

> I definitely think more thought needs to be put into cleaning up the
> multiple definition behavior, as it is very confusing to follow.

Yes. For starters I think each non-central definition (ie, the
definitions in our FRAME_POINTER example that we found in arch/) needs a
comment: why was it needed, why wasn't lib/Kconfig.debug altered? See my
arm64 example why I want to know that.

Stefan, Valentin, Andreas: can your bot spot newly added multiple
definitions? If so, would it make sense to have it emit warnings when
that happens, at least for the time being?

> But
> the current behavior is flat-out wrong, as the search function does
> not actually print out the actual set of definitions or dependencies
> used by kconfig, and that is all that this patch is aiming to fix.

Personally I'm not willing to think about changing the UI without
understanding the reasons for the current behavior:
- in what case does it make sense to define a symbol twice[0]; and
- does the, well, low-level kconfig code implement those cases in a
sensible way?

Only after questions like these are answered I'll be willing to think
about the UI. (Perhaps people like Michal or Martin know the answers to
those questions. I don't.) It's great that you raised this issue, but
neither Stefan, you, nor I appear to really grok these multiple
definitions, and that needs to change first.

Thanks,


Paul Bolle

[0] Obviously this is _not_ about symbols, like 64BIT, that are defined
multiple times in various arch/ directories. A valid parse of the
Kconfig files will only encounter such symbols exactly once.

Re: [PATCH 1/2] kconfig: Print full defined and depends for multiply-defined symbols

[Stefan Hengelein]

>> > The search facility shows the first one that is found, you see the
>> > complicated depends on but i think the text shown might not be
>> > explicit enough to clarify you don't need to satisfy these complicated
>> > conditions to actually choose a value.
>> >
>>
>> Well, the thing is, you do need to satisfy _one_ of the set of
>> conditions.  But unfortunately it's not not possible to see that from
>> the search function because it prints out incomplete information.
>>
>> This is exactly the motivation for this change.  When you search for
>> FRAME_POINTER, you only get information on one "Defined at", and the
>> only "Depends" information is from that definition.  This is clearly
>> incorrect, as FRAME_POINTER is defined in multiple places, so the
>> information printed does not cover its actual definitions or
>> dependencies.  The printed "Selected by" information is, however,
>> correct, because it actually uses the rev_dep expression.  This patch
>> changes the "Depends" information to be similar: it will also print
>> the actual expression used by kconfig used to determine whether its
>> direct dependencies are fulfilled.
>>
>> Is this overly complicated or confusing?  Perhaps.  But it is better
>> than printing out an incomplete set of definitions and dependencies,
>> which is the current behavior.
>>
>> As Stefan mentioned, "FRAME_POINTER is a complicated example, it is
>> selected although it has dependencies or a prompt AND it is redefined
>> in many architectures".  I'm pretty new to the Kconfig code, but to
>> me, the multiple symbol behavior is bizarre.
>
> I didn't yet stumble on a rationale of the multiple definition behavior.
> Without knowing that rationale things look rather bizarre.

Well, i think i tried to explain that rationale before. An
architecture wants to enable OPTION_X independently of the definition
in lib/whatever/Kconfig and therefore independently of the conditions
defined in lib/whatever/Kconfig like it's done in ARM64. You cannot
expect the non-architecture code in lib/ or other folders to consider
the opinions of each architecture, that would end in huge discussions
and everyone to be disappointed or mad at each other. Linux is huge,
you have different maintainers for almost everything. To have an
alternative definition is not inherently wrong or useless, you just
have to know about them without a grep over the the Kconfig-files.


>> - each definition _without_ a prompt that has its dependencies
>> fulfilled results in the default value set in that definition being
>> used unconditionally.  E.g. for FRAME_POINTER, this means that it is
>> impossible to disable the option on ARM.  I have submitted a separate
>> patch to try to fix that for ARM at least,
>> https://lkml.org/lkml/2015/4/8/765
>
> I didn't see that patch. It's probably closely related to what we
> discuss here. Please send closely related patches as series. (Was it
> perhaps intended to be the 2/2 you never sent?)
>
> I find the arm64 entry for this symbol interesting too. Since ARM64 will
> always select ARCH_WANT_FRAME_POINTERS I think there's no reason for it
> to have a separate FRAME_POINTER entry in the first place. (I didn't yet
> bother to look at the git history of arm64.)

Subsystems should have the right to use X without a need to alter
definitions in lib/ or other folders...if everyone would put their own
constraints into the definition of X in lib/Kconfig.debug, you would
probably complain because the conditions could get a lot longer than
they are now ;) What would mean the conditions would be harder to
maintain, harder to gasp for newbies and a certain wild growth would
be invited.

>
>> I definitely think more thought needs to be put into cleaning up the
>> multiple definition behavior, as it is very confusing to follow.
>
> Yes. For starters I think each non-central definition (ie, the
> definitions in our FRAME_POINTER example that we found in arch/) needs a
> comment: why was it needed, why wasn't lib/Kconfig.debug altered? See my
> arm64 example why I want to know that.
>
> Stefan, Valentin, Andreas: can your bot spot newly added multiple
> definitions? If so, would it make sense to have it emit warnings when
> that happens, at least for the time being?

No, we we're currently not searching for redefinitions, but i don't
see a need to add this.


> - does the, well, low-level kconfig code implement those cases in a
> sensible way?

define sensible. i explained how kconfig is dealing with these cases
:) what did you miss?

Stefan

Re: [PATCH 1/2] kconfig: Print full defined and depends for multiply-defined symbols

[Stefan Hengelein]

Hi Gregory,
2015-04-13 3:06 GMT+02:00 Gregory Fong <gregory.0xf0@gmail.com>:
> Hi Paul and Stefan,
>
> Thanks for taking a look at this.  I think Stefan has touched upon why
> he thinks this change might (at least partially) make sense, but let
> me now try to explain the rationale behind this patch better than I
> did in the commit message.

yes, i missed where and how it was used in your commit message.

>
> On Sun, Apr 12, 2015 at 8:02 AM, Stefan Hengelein
> <stefan.hengelein@fau.de> wrote:
>>> Let's focus, for example, on m32r and FRAME_POINTER. The m32r entry for
>>> that symbol reads:
>>>         config FRAME_POINTER
>>>                 bool "Compile the kernel with frame pointers"
>>>                 help
>>>                   If you say Y here [...]
>>>
>>> 0) If one is building for m32r is that all there's to it? If so, "make
>>> menuconfig"'s search facility is serving the people building for m32r a
>>> load of crap.
>>>
>>> 1) If it's actually more complicated than that I think that anyone
>>> reading arch/m32r/Kconfig.debug is being duped. Things look simple but
>>> actually they are quite complicated. I think that's just wrong.
>>>
>>> What am I missing here?
>>
>> If you have a look at the definitions, lib/Kconfig.debug is included
>> before FRAME_POINTER is defined in m32r and the output in the search
>> facility looks indeed broken
>> as one "Defined at" is missing but there are somehow Location entries
>> (-> Kernel hacking    and  -> Kernel hacking -> compile time checks
>> and [...]) for both definitions in a weird order (i think (1) and (2)
>> might indicate both definitions)
>>
>> both declarations are valid in kconfig, you have two ways of enabling
>> the same symbol, one easy without conditions and one with conditions
>> and both with the same prompt.
>>
>> The search facility shows the first one that is found, you see the
>> complicated depends on but i think the text shown might not be
>> explicit enough to clarify you don't need to satisfy these complicated
>> conditions to actually choose a value.
>>
>
> Well, the thing is, you do need to satisfy _one_ of the set of
> conditions.  But unfortunately it's not not possible to see that from
> the search function because it prints out incomplete information.

Yes and no. You would need to satisfy "!THUMB2_KERNEL" to be able to
enable FRAME_POINTER but for the second definition to enable
FRAME_POINTER you'd still have to satisfy the conditions of the
default.
However, what you're missing here is what is printed before "Depends
on" or "Defined at".
Namely: Prompt: Compile the kernel with frame pointers

that entry suggests you will be able to see a prompt (meaning an entry
in menuconfig or conf) when "!THUMB2_KERNEL" is satisfied and that's
just wrong.

I don't know if it is difficult to achieve but i would prefer to have
something like:

Symbol: FRAME_POINTER
Type: boolean
(1) Defined at lib/Kconfig.debug:323
    Prompt: Compile the kernel with frame pointers
       Location:
          -> Kernel hacking
            -> Compile-time checks and compiler options
    Depends on: DEBUG_KERNEL [=y] && (CRIS || M68K || FRV || UML || AVR32 ...
    Selected by: ....

(2) Defined at arch/arm/Kconfig.debug:35
    Depends on: !THUMB2_KERNEL


when you try menuconfig for m32r you'll see 2 prompt entries, to
locations, one depends on. That implies you already see multiple
definitions when these multiple definitions have prompts, but
definitions without a prompt are missed or intentionally left out
because you can't influence them from menuconfig either way. (so see
what the second definition does, you'd have a look at
arch/arm/Kconfig.debug:35)

>
> This is exactly the motivation for this change.  When you search for
> FRAME_POINTER, you only get information on one "Defined at", and the
> only "Depends" information is from that definition.  This is clearly
> incorrect, as FRAME_POINTER is defined in multiple places, so the
> information printed does not cover its actual definitions or
> dependencies.  The printed "Selected by" information is, however,
> correct, because it actually uses the rev_dep expression.  This patch
> changes the "Depends" information to be similar: it will also print
> the actual expression used by kconfig used to determine whether its
> direct dependencies are fulfilled.
>
> Is this overly complicated or confusing?  Perhaps.  But it is better
> than printing out an incomplete set of definitions and dependencies,
> which is the current behavior.

You're right, but "it's a little bit better than before and could be
misinterpreted in other ways" is not a good reason to merge a patch
imho.

>
> As Stefan mentioned, "FRAME_POINTER is a complicated example, it is
> selected although it has dependencies or a prompt AND it is redefined
> in many architectures".  I'm pretty new to the Kconfig code, but to
> me, the multiple symbol behavior is bizarre.  This is because:
> - every definition is independent---dependencies are OR'd together
> from each definition.
> - each definition with a prompt creates a new location where that
> symbol can be changed in menuconfig
> - each definition _without_ a prompt that has its dependencies
> fulfilled results in the default value set in that definition being
> used unconditionally.  E.g. for FRAME_POINTER, this means that it is
> impossible to disable the option on ARM.  I have submitted a separate
> patch to try to fix that for ARM at least,
> https://lkml.org/lkml/2015/4/8/765

that's actually not true, you still would have to satisfy the
conditions for the default, but i'm not sure how easy you could
disable or enable the features in the default conditions without
missing something you need.

>
> I definitely think more thought needs to be put into cleaning up the
> multiple definition behavior, as it is very confusing to follow.  But
> the current behavior is flat-out wrong, as the search function does
> not actually print out the actual set of definitions or dependencies
> used by kconfig, and that is all that this patch is aiming to fix.
>
> Aside: if the issue is in how this is displayed, maybe it would be
> better to add in some parentheses and/or linebreaks to make it clearer
> that these are distinct sets, so rather than printing
>
>   Defined at lib/Kconfig.debug:323, arch/arm/Kconfig.debug:35
>   Depends on: DEBUG_KERNEL [=y] && (CRIS || M68K || FRV || UML ||
> AVR32 || SUPERH || BLACKFIN || MN10300 || METAG) ||
> ARCH_WANT_FRAME_POINTERS [=n] || !THUMB2_KERNEL [=n]
>
> you get something more like
>
>   Defined at lib/Kconfig.debug:323, arch/arm/Kconfig.debug:35
>   Depends on:
>     (DEBUG_KERNEL [=y] && (CRIS || M68K || FRV || UML || AVR32 ||
> SUPERH || BLACKFIN || MN10300 || METAG) || ARCH_WANT_FRAME_POINTERS
> [=n]) ||
>     (!THUMB2_KERNEL [=n])
>
> which makes the logic a little bit more obvious.   You could do this by iterating over the
> associated properties rather than just printing out the dir_dep
> expression of the symbol, but then you'd probably to change rev_dep to
> do the same.  I don't really like this, because it doesn't match up
> with what kconfig is _actually_ using (i.e. the rev_dep and dir_dep of
> the associated symbol).

I guess that wouldn't help, especially...how would you determine to
add a newline before (!THUMB_KERNEL)  and not before
ARCH_WANT_FRAME_POINTERS? And it doesn't point out the condition is
originating in another definition.

Stefan