[v4,0/3] Kconfig: Print reverse dependencies in groups

Commit Message

Ulf Magnusson Feb. 20, 2018, 3 p.m. UTC

On Tue, Feb 20, 2018 at 05:24:01PM +0900, Masahiro Yamada wrote:
> 2018-02-20 17:13 GMT+09:00 Masahiro Yamada <[e-mail]>:
> > 2018-02-19 5:47 GMT+09:00 Eugeniu Rosca <[e-mail]>:
> >> From: Eugeniu Rosca <[e-mail]>
> >>
> >> Hello Masahiro, Ulf, Petr and all,
> >>
> >> Here are a few words about the motivation behind this patch series.
> >>
> >> First, the reason I got in touch with Kconfig is to optimize the
> >> configuration of automotive kernels, as well as to align the kernel
> >> configuration across a number of platforms. In the context of kernel
> >> optimization, one of the primary goals is to filter out any features
> >> that are not mentioned in the platform requirements.
> >>
> >> Surprisingly or not, disabling a CONFIG option (which is assumed to
> >> be unneeded) may be not so trivial. Especially it is not trivial, when
> >> this CONFIG option is selected by a dozen of other configs. Before the
> >> moment commit 1ccb27143360 ("kconfig: make "Selected by:" and
> >> "Implied by:" readable") was submitted by Petr and eventually popped
> >> up in v4.16-rc1, it was an absolute pain to break down the "Selected by"
> >> reverse dependency expression in order to identify all those configs
> >> which select (IOW *do not allow disabling*) a certain feature (assumed
> >> to be not needed).
> >>
> >> This patch series tries to make one step further and puts at users'
> >> fingertips the revdep top level OR sub-expressions grouped/clustered by
> >> the tristate value they evaluate to. This should allow the users to
> >> directly concentrate on and tackle the active reverse dependencies,
> >> which imho are the only ones that matter for a given ARCH and for a
> >> given defconfig (nevertheless we still print all of them).
> >>
> >> Changes v3->v4 (fixed review findings from Ulf):
> >> - Remove redundant default cases in switch constructs.
> >> - Remove gettext _() tokens in str_append() calls.
> >> - Aggregate code repetitions in expr_print_revdep().
> >>
> >> Changes v2->v3:
> >> - Switch from reverse dependencies prefixed by their tristate value to
> >>   reverse dependencies grouped by the tristate value they evaluate to.
> >> - Skip printing "{Selected,Implied} by [y|m|n]:" if there are no top
> >>   level OR tokens/sub-expressions that evaluate to y|m|n (suggested
> >>   by Petr).
> >> - Use [1] as template for updating the interface/prototype of
> >>   __expr_print() (suggested by Ulf).
> >>
> >> Changes v1->v2:
> >> - Don't skip the =n reverse dependency OR tokens, since some users might
> >>   still need this information (suggested by Ulf).
> >> - Instead of using "Selected by" for active tokens only, use it for all
> >>   OR tokens, but specify the tristate value of each token as prefix
> >>   (suggested by Masahiro).
> >>
> >> [1] https://marc.info/?l=linux-kbuild&m=151777006005199&w=4
> >>
> >> Eugeniu Rosca (3):
> >>   kconfig: Print reverse dependencies on new line consistently
> >>   kconfig: Prepare for printing reverse dependencies in groups
> >>   kconfig: Print reverse dependencies in groups
> >>
> >>  scripts/kconfig/expr.c      | 102 +++++++++++++++++++++++++++++++++++++-------
> >>  scripts/kconfig/expr.h      |  11 ++++-
> >>  scripts/kconfig/lkc_proto.h |   1 +
> >>  scripts/kconfig/menu.c      |  37 +++++++++++-----
> >>  4 files changed, 124 insertions(+), 27 deletions(-)
> >>
> >
> >
> > I do not like this implementation.
> > The code is super ugly, the diff-stat is too much than needed.
> >
> > Please rewrite the code within 20 lines.
> >
> 
> For a hint, I cleaned up the code base.
> https://patchwork.kernel.org/patch/10229545/
> 
> which should be equivalent to yours:
> https://patchwork.kernel.org/patch/10226951/
> 
> 
> No 'enum print_type', please.

The reason I prefer them on separate lines consistently is to avoid stuff like the following:

  Selected by [y]: MV_XOR_V2 [=y] && DMADEVICES [=y] && ARM64 [=y]
  Selected by [n]:
  - AMCC_PPC440SPE_ADMA [=n] && DMADEVICES [=y] && (440SPe || ...
  - FSL_RAID [=n] && DMADEVICES [=y] && FSL_SOC && ...
  - INTEL_IOATDMA [=n] && DMADEVICES [=y] && PCI [=y] && X86_64

That looks confusing and unbalanced to me.

There are some simple ways to trim down the size of this patchset
though.

Eugeniu:
What do you think about the following refactoring of your 3/3 patch?
Maybe there's a way to have expr_print_revdep() take just a tristate
too, though it's not worth it if it just grows the code elsewhere.

(By the way, I noticed that expr_print_revdep() previously generated a
warning suggesting parentheses, which was my fault. If you saw that,
don't copy my mistakes. The build should be warning-free. :)


--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Eugeniu Rosca Feb. 20, 2018, 7:52 p.m. UTC | #1

On Tue, Feb 20, 2018 at 04:00:18PM +0100, Ulf Magnusson wrote:
> On Tue, Feb 20, 2018 at 05:24:01PM +0900, Masahiro Yamada wrote:
> > 2018-02-20 17:13 GMT+09:00 Masahiro Yamada <[e-mail]>:
> > >
> > > I do not like this implementation.
> > > The code is super ugly, the diff-stat is too much than needed.
> > >
> > > Please rewrite the code within 20 lines.
> > >
> > 
> > For a hint, I cleaned up the code base.
> > https://patchwork.kernel.org/patch/10229545/
> > 
> > which should be equivalent to yours:
> > https://patchwork.kernel.org/patch/10226951/
> > 
> > 
> > No 'enum print_type', please.
> 
> The reason I prefer them on separate lines consistently is to avoid stuff like the following:
> 
>   Selected by [y]: MV_XOR_V2 [=y] && DMADEVICES [=y] && ARM64 [=y]
>   Selected by [n]:
>   - AMCC_PPC440SPE_ADMA [=n] && DMADEVICES [=y] && (440SPe || ...
>   - FSL_RAID [=n] && DMADEVICES [=y] && FSL_SOC && ...
>   - INTEL_IOATDMA [=n] && DMADEVICES [=y] && PCI [=y] && X86_64
> 
> That looks confusing and unbalanced to me.

I think nobody is disputing this. Masahiro's
https://patchwork.kernel.org/patch/10229545/ is also showing every
reverse dependency top level OR expression on a new row.

> 
> There are some simple ways to trim down the size of this patchset
> though.
> 
> Eugeniu:
> What do you think about the following refactoring of your 3/3 patch?
> Maybe there's a way to have expr_print_revdep() take just a tristate
> too, though it's not worth it if it just grows the code elsewhere.
> 

Well, Masahiro requests not using 'enum print_type' and I still see it
in your example.

> (By the way, I noticed that expr_print_revdep() previously generated a
> warning suggesting parentheses, which was my fault. If you saw that,
> don't copy my mistakes. The build should be warning-free. :)

I didn't notice any warnings using gcc 5.4.0.

> 
> diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c
> index 95dc058a236f..db9a89b9bede 100644
> --- a/scripts/kconfig/expr.c
> +++ b/scripts/kconfig/expr.c
> @@ -1186,10 +1186,9 @@ expr_print_revdep(struct expr *e,
>  		  int prevtoken,
>  		  enum print_type type)
>  {
> -	if (type == PRINT_REVDEP_ALL				  ||
> -	    type == PRINT_REVDEP_YES && expr_calc_value(e) == yes ||
> -	    type == PRINT_REVDEP_MOD && expr_calc_value(e) == mod ||
> -	    type == PRINT_REVDEP_NO  && expr_calc_value(e) == no) {
> +	if ((type == PRINT_REVDEP_YES && expr_calc_value(e) == yes) ||
> +	    (type == PRINT_REVDEP_MOD && expr_calc_value(e) == mod) ||
> +	    (type == PRINT_REVDEP_NO  && expr_calc_value(e) == no)) {
>  		fn(data, NULL, "\n  - ");
>  		expr_print(e, fn, data, prevtoken);
>  	}
> @@ -1212,17 +1211,10 @@ __expr_print(struct expr *e,
>  	switch (e->type) {
>  	case E_SYMBOL:
>  		if (e->left.sym->name)
> -			switch (type) {
> -			case PRINT_NORMAL:
> +			if (type == PRINT_NORMAL)
>  				fn(data, e->left.sym, e->left.sym->name);
> -				break;
> -			case PRINT_REVDEP_ALL:
> -			case PRINT_REVDEP_YES:
> -			case PRINT_REVDEP_MOD:
> -			case PRINT_REVDEP_NO:
> +			else
>  				expr_print_revdep(e, fn, data, E_OR, type);
> -				break;
> -			}
>  		else
>  			fn(data, NULL, "<choice>");
>  		break;
> @@ -1271,18 +1263,12 @@ __expr_print(struct expr *e,
>  		__expr_print(e->right.expr, fn, data, E_OR, type);
>  		break;
>  	case E_AND:
> -		switch (type) {
> -		case PRINT_NORMAL:
> +		if (type == PRINT_NORMAL) {
>  			expr_print(e->left.expr, fn, data, E_AND);
>  			fn(data, NULL, " && ");
>  			expr_print(e->right.expr, fn, data, E_AND);
> -			break;
> -		case PRINT_REVDEP_ALL:
> -		case PRINT_REVDEP_YES:
> -		case PRINT_REVDEP_MOD:
> -		case PRINT_REVDEP_NO:
> +		} else {
>  			expr_print_revdep(e, fn, data, E_OR, type);
> -			break;
>  		}
>  		break;
>  	case E_LIST:
> @@ -1370,27 +1356,14 @@ void expr_gstr_print(struct expr *e, struct gstr *gs)
>   */
>  bool expr_revdep_contains(struct expr *e, tristate val)
>  {
> -	bool ret = false;
> -
>  	if (!e)
> -		return ret;
> +		return false;
>  
> -	switch (e->type) {
> -	case E_SYMBOL:
> -	case E_AND:
> -		if (expr_calc_value(e) == val)
> -			ret = true;
> -		break;
> -	case E_OR:
> -		if (expr_revdep_contains(e->left.expr, val))
> -			ret = true;
> -		else if (expr_revdep_contains(e->right.expr, val))
> -			ret = true;
> -		break;
> -	default:
> -		break;
> -	}
> -	return ret;
> +	if (e->type == E_OR)
> +		return expr_revdep_contains(e->left.expr, val) ||
> +		       expr_revdep_contains(e->right.expr, val);
> +
> +	return expr_calc_value(e) == val;
>  }

I really like how you've minimized the size of expr_revdep_contains().
Thanks for that. I will try to come up with something in the next days.

>  
>  /*
> diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
> index d5b096725ca8..e5687b430c17 100644
> --- a/scripts/kconfig/expr.h
> +++ b/scripts/kconfig/expr.h
> @@ -36,7 +36,6 @@ enum expr_type {
>  
>  enum print_type {
>  	PRINT_NORMAL,
> -	PRINT_REVDEP_ALL,
>  	PRINT_REVDEP_YES,
>  	PRINT_REVDEP_MOD,
>  	PRINT_REVDEP_NO,

Best regards,
Eugeniu.
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ulf Magnusson Feb. 20, 2018, 8:09 p.m. UTC | #2

On Tue, Feb 20, 2018 at 8:52 PM, Eugeniu Rosca <[e-mail]> wrote:
> On Tue, Feb 20, 2018 at 04:00:18PM +0100, Ulf Magnusson wrote:
>> On Tue, Feb 20, 2018 at 05:24:01PM +0900, Masahiro Yamada wrote:
>> > 2018-02-20 17:13 GMT+09:00 Masahiro Yamada <[e-mail]>:
>> > >
>> > > I do not like this implementation.
>> > > The code is super ugly, the diff-stat is too much than needed.
>> > >
>> > > Please rewrite the code within 20 lines.
>> > >
>> >
>> > For a hint, I cleaned up the code base.
>> > https://patchwork.kernel.org/patch/10229545/
>> >
>> > which should be equivalent to yours:
>> > https://patchwork.kernel.org/patch/10226951/
>> >
>> >
>> > No 'enum print_type', please.
>>
>> The reason I prefer them on separate lines consistently is to avoid stuff like the following:
>>
>>   Selected by [y]: MV_XOR_V2 [=y] && DMADEVICES [=y] && ARM64 [=y]
>>   Selected by [n]:
>>   - AMCC_PPC440SPE_ADMA [=n] && DMADEVICES [=y] && (440SPe || ...
>>   - FSL_RAID [=n] && DMADEVICES [=y] && FSL_SOC && ...
>>   - INTEL_IOATDMA [=n] && DMADEVICES [=y] && PCI [=y] && X86_64
>>
>> That looks confusing and unbalanced to me.
>
> I think nobody is disputing this. Masahiro's
> https://patchwork.kernel.org/patch/10229545/ is also showing every
> reverse dependency top level OR expression on a new row.

Ohh... sorry, didn't read closely enough.

Yeah, I agree that that looks like a nice and neat solution.

>
>>
>> There are some simple ways to trim down the size of this patchset
>> though.
>>
>> Eugeniu:
>> What do you think about the following refactoring of your 3/3 patch?
>> Maybe there's a way to have expr_print_revdep() take just a tristate
>> too, though it's not worth it if it just grows the code elsewhere.
>>
>
> Well, Masahiro requests not using 'enum print_type' and I still see it
> in your example.

One alternative would be to keep the 'revdep' flag and add an extra
tristate parameter for the kinds of selects to print.

I'm not a huge fan of having parameters that turn meaningless
depending on the values of other parameters, but it might not be
terrible there, if we must get rid of 'enum print_type'. I'm always
open to other suggestions too.

The whole expression printing code feels a bit twisty to me with all
the mutual recursion and stuff going on, but that's outside the scope
of this patchset. :)

>
>> (By the way, I noticed that expr_print_revdep() previously generated a
>> warning suggesting parentheses, which was my fault. If you saw that,
>> don't copy my mistakes. The build should be warning-free. :)
>
> I didn't notice any warnings using gcc 5.4.0.

I'm on 7.2.0, so that's probably why. It was "just" a style warning anyway. :)

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Masahiro Yamada Feb. 21, 2018, 5:48 a.m. UTC | #3

2018-02-21 5:09 GMT+09:00 Ulf Magnusson <[e-mail]>:
> On Tue, Feb 20, 2018 at 8:52 PM, Eugeniu Rosca <[e-mail]> wrote:
>> On Tue, Feb 20, 2018 at 04:00:18PM +0100, Ulf Magnusson wrote:
>>> On Tue, Feb 20, 2018 at 05:24:01PM +0900, Masahiro Yamada wrote:
>>> > 2018-02-20 17:13 GMT+09:00 Masahiro Yamada <[e-mail]>:
>>> > >
>>> > > I do not like this implementation.
>>> > > The code is super ugly, the diff-stat is too much than needed.
>>> > >
>>> > > Please rewrite the code within 20 lines.
>>> > >
>>> >
>>> > For a hint, I cleaned up the code base.
>>> > https://patchwork.kernel.org/patch/10229545/
>>> >
>>> > which should be equivalent to yours:
>>> > https://patchwork.kernel.org/patch/10226951/
>>> >
>>> >
>>> > No 'enum print_type', please.
>>>
>>> The reason I prefer them on separate lines consistently is to avoid stuff like the following:
>>>
>>>   Selected by [y]: MV_XOR_V2 [=y] && DMADEVICES [=y] && ARM64 [=y]
>>>   Selected by [n]:
>>>   - AMCC_PPC440SPE_ADMA [=n] && DMADEVICES [=y] && (440SPe || ...
>>>   - FSL_RAID [=n] && DMADEVICES [=y] && FSL_SOC && ...
>>>   - INTEL_IOATDMA [=n] && DMADEVICES [=y] && PCI [=y] && X86_64
>>>
>>> That looks confusing and unbalanced to me.
>>
>> I think nobody is disputing this. Masahiro's
>> https://patchwork.kernel.org/patch/10229545/ is also showing every
>> reverse dependency top level OR expression on a new row.
>
> Ohh... sorry, didn't read closely enough.
>
> Yeah, I agree that that looks like a nice and neat solution.
>
>>
>>>
>>> There are some simple ways to trim down the size of this patchset
>>> though.
>>>
>>> Eugeniu:
>>> What do you think about the following refactoring of your 3/3 patch?
>>> Maybe there's a way to have expr_print_revdep() take just a tristate
>>> too, though it's not worth it if it just grows the code elsewhere.
>>>
>>
>> Well, Masahiro requests not using 'enum print_type' and I still see it
>> in your example.
>
> One alternative would be to keep the 'revdep' flag and add an extra
> tristate parameter for the kinds of selects to print.

Using 'enum tristate' is what was in mind,
but I do not like to mess up __expr_print() any more.

I re-implemented this.
https://patchwork.kernel.org/patch/10231295/





> I'm not a huge fan of having parameters that turn meaningless
> depending on the values of other parameters, but it might not be
> terrible there, if we must get rid of 'enum print_type'. I'm always
> open to other suggestions too.
>
> The whole expression printing code feels a bit twisty to me with all
> the mutual recursion and stuff going on, but that's outside the scope
> of this patchset. :)
>
>>
>>> (By the way, I noticed that expr_print_revdep() previously generated a
>>> warning suggesting parentheses, which was my fault. If you saw that,
>>> don't copy my mistakes. The build should be warning-free. :)
>>
>> I didn't notice any warnings using gcc 5.4.0.
>
> I'm on 7.2.0, so that's probably why. It was "just" a style warning anyway. :)
>
> Cheers,
> Ulf
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Eugeniu Rosca Feb. 21, 2018, 6:10 a.m. UTC | #4

On Wed, Feb 21, 2018 at 02:48:45PM +0900, Masahiro Yamada wrote:
> 2018-02-21 5:09 GMT+09:00 Ulf Magnusson <[e-mail]>:
> > On Tue, Feb 20, 2018 at 8:52 PM, Eugeniu Rosca <[e-mail]> wrote:
> >> On Tue, Feb 20, 2018 at 04:00:18PM +0100, Ulf Magnusson wrote:
> >>> On Tue, Feb 20, 2018 at 05:24:01PM +0900, Masahiro Yamada wrote:
> >>> > 2018-02-20 17:13 GMT+09:00 Masahiro Yamada <[e-mail]>:
> >>> > >
> >>> > > I do not like this implementation.
> >>> > > The code is super ugly, the diff-stat is too much than needed.
> >>> > >
> >>> > > Please rewrite the code within 20 lines.
> >>> > >
> >>> >
> >>> > For a hint, I cleaned up the code base.
> >>> > https://patchwork.kernel.org/patch/10229545/
> >>> >
> >>> > which should be equivalent to yours:
> >>> > https://patchwork.kernel.org/patch/10226951/
> >>> >
> >>> >
> >>> > No 'enum print_type', please.
> >>>
> >>> The reason I prefer them on separate lines consistently is to avoid stuff like the following:
> >>>
> >>>   Selected by [y]: MV_XOR_V2 [=y] && DMADEVICES [=y] && ARM64 [=y]
> >>>   Selected by [n]:
> >>>   - AMCC_PPC440SPE_ADMA [=n] && DMADEVICES [=y] && (440SPe || ...
> >>>   - FSL_RAID [=n] && DMADEVICES [=y] && FSL_SOC && ...
> >>>   - INTEL_IOATDMA [=n] && DMADEVICES [=y] && PCI [=y] && X86_64
> >>>
> >>> That looks confusing and unbalanced to me.
> >>
> >> I think nobody is disputing this. Masahiro's
> >> https://patchwork.kernel.org/patch/10229545/ is also showing every
> >> reverse dependency top level OR expression on a new row.
> >
> > Ohh... sorry, didn't read closely enough.
> >
> > Yeah, I agree that that looks like a nice and neat solution.
> >
> >>
> >>>
> >>> There are some simple ways to trim down the size of this patchset
> >>> though.
> >>>
> >>> Eugeniu:
> >>> What do you think about the following refactoring of your 3/3 patch?
> >>> Maybe there's a way to have expr_print_revdep() take just a tristate
> >>> too, though it's not worth it if it just grows the code elsewhere.
> >>>
> >>
> >> Well, Masahiro requests not using 'enum print_type' and I still see it
> >> in your example.
> >
> > One alternative would be to keep the 'revdep' flag and add an extra
> > tristate parameter for the kinds of selects to print.
> 
> Using 'enum tristate' is what was in mind,
> but I do not like to mess up __expr_print() any more.
> 
> I re-implemented this.
> https://patchwork.kernel.org/patch/10231295/
> 

I've tested https://patchwork.kernel.org/patch/10229545/ and
https://patchwork.kernel.org/patch/10231295/ and they work great for me.
Thank you for this feature.

> 
> 
> 
> > I'm not a huge fan of having parameters that turn meaningless
> > depending on the values of other parameters, but it might not be
> > terrible there, if we must get rid of 'enum print_type'. I'm always
> > open to other suggestions too.
> >
> > The whole expression printing code feels a bit twisty to me with all
> > the mutual recursion and stuff going on, but that's outside the scope
> > of this patchset. :)
> >
> >>
> >>> (By the way, I noticed that expr_print_revdep() previously generated a
> >>> warning suggesting parentheses, which was my fault. If you saw that,
> >>> don't copy my mistakes. The build should be warning-free. :)
> >>
> >> I didn't notice any warnings using gcc 5.4.0.
> >
> > I'm on 7.2.0, so that's probably why. It was "just" a style warning anyway. :)
> >
> > Cheers,
> > Ulf
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> > the body of a message to [e-mail]
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 
> -- 
> Best Regards
> Masahiro Yamada

Best regards,
Eugeniu.
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Masahiro Yamada Feb. 23, 2018, 12:10 p.m. UTC | #5

Eugeniu,


2018-02-21 15:10 GMT+09:00 Eugeniu Rosca <[e-mail]>:

>
> I've tested https://patchwork.kernel.org/patch/10229545/ and
> https://patchwork.kernel.org/patch/10231295/ and they work great for me.
> Thank you for this feature.
>

I will give you that code in the second one.

Please fill the commit log with yours,
and also fix the bugs pointed by Petr.

Anyway, I will claim my contribution
in the form of Signed-off-by when I pick it up.  :)

Eugeniu Rosca Feb. 23, 2018, 2:04 p.m. UTC | #6

Hi Masahiro,

On Fri, Feb 23, 2018 at 09:10:37PM +0900, Masahiro Yamada wrote:
> Eugeniu,
> 
> 
> 2018-02-21 15:10 GMT+09:00 Eugeniu Rosca <[e-mail]>:
> 
> >
> > I've tested https://patchwork.kernel.org/patch/10229545/ and
> > https://patchwork.kernel.org/patch/10231295/ and they work great for me.
> > Thank you for this feature.
> >
> 
> I will give you that code in the second one.

I appreciate that, although I am open minded who takes the authorship.
This feature makes my life easier and that's the most important part.

> 
> Please fill the commit log with yours,
> and also fix the bugs pointed by Petr.

Sure. Will do that.

> Anyway, I will claim my contribution
> in the form of Signed-off-by when I pick it up.  :)

I have one side question loosely related to this patch.

Reverse dependencies are still not printed in zconfdump() (used in our kernel
team for e.g. cross platform .config alignment/comparison). My gut feeling
tells me this is mainly caused by the extremely unfriendly unreadable
expressions which is how reverse dependencies used to be represented before
commit 1ccb27143360 ("kconfig: make "Selected by:" and "Implied by:"
readable"). Since this is now greatly improved, would you mind printing
Selected/Implied-by in zconfdump? There are also other zconfdump fixes
sitting in my local branch, like https://patchwork.kernel.org/patch/9253535/ .
I would like to push them to you (they probably deserve a separate
thread), if you think this is appropriate.

Best regards,
Eugeniu.
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ulf Magnusson Feb. 23, 2018, 10:18 p.m. UTC | #7

On Fri, Feb 23, 2018 at 3:04 PM, Eugeniu Rosca <[e-mail]> wrote:
>
> Hi Masahiro,
>
> On Fri, Feb 23, 2018 at 09:10:37PM +0900, Masahiro Yamada wrote:
>> Eugeniu,
>>
>>
>> 2018-02-21 15:10 GMT+09:00 Eugeniu Rosca <[e-mail]>:
>>
>> >
>> > I've tested https://patchwork.kernel.org/patch/10229545/ and
>> > https://patchwork.kernel.org/patch/10231295/ and they work great for me.
>> > Thank you for this feature.
>> >
>>
>> I will give you that code in the second one.
>
> I appreciate that, although I am open minded who takes the authorship.
> This feature makes my life easier and that's the most important part.
>
>>
>> Please fill the commit log with yours,
>> and also fix the bugs pointed by Petr.
>
> Sure. Will do that.
>
>> Anyway, I will claim my contribution
>> in the form of Signed-off-by when I pick it up.  :)
>
> I have one side question loosely related to this patch.
>
> Reverse dependencies are still not printed in zconfdump() (used in our kernel
> team for e.g. cross platform .config alignment/comparison). My gut feeling
> tells me this is mainly caused by the extremely unfriendly unreadable
> expressions which is how reverse dependencies used to be represented before
> commit 1ccb27143360 ("kconfig: make "Selected by:" and "Implied by:"
> readable"). Since this is now greatly improved, would you mind printing
> Selected/Implied-by in zconfdump? There are also other zconfdump fixes
> sitting in my local branch, like https://patchwork.kernel.org/patch/9253535/ .
> I would like to push them to you (they probably deserve a separate
> thread), if you think this is appropriate.
>
> Best regards,
> Eugeniu.

Shameless plug:

If you need to do any fancy Kconfig parsing and comparison, then
Kconfiglib (https://github.com/ulfalizer/Kconfiglib) might be handy.
It makes it easy to walk the menu tree, print and inspect symbols and
expressions, etc.

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Eugeniu Rosca Feb. 24, 2018, 12:13 a.m. UTC | #8

Hi Ulf,

On Fri, Feb 23, 2018 at 11:18:17PM +0100, Ulf Magnusson wrote:
> On Fri, Feb 23, 2018 at 3:04 PM, Eugeniu Rosca <[e-mail]> wrote:
> >
> > Hi Masahiro,
> >
> > On Fri, Feb 23, 2018 at 09:10:37PM +0900, Masahiro Yamada wrote:
> >> Eugeniu,
> >>
> >>
> >> 2018-02-21 15:10 GMT+09:00 Eugeniu Rosca <[e-mail]>:
> >>
> >> >
> >> > I've tested https://patchwork.kernel.org/patch/10229545/ and
> >> > https://patchwork.kernel.org/patch/10231295/ and they work great for me.
> >> > Thank you for this feature.
> >> >
> >>
> >> I will give you that code in the second one.
> >
> > I appreciate that, although I am open minded who takes the authorship.
> > This feature makes my life easier and that's the most important part.
> >
> >>
> >> Please fill the commit log with yours,
> >> and also fix the bugs pointed by Petr.
> >
> > Sure. Will do that.
> >
> >> Anyway, I will claim my contribution
> >> in the form of Signed-off-by when I pick it up.  :)
> >
> > I have one side question loosely related to this patch.
> >
> > Reverse dependencies are still not printed in zconfdump() (used in our kernel
> > team for e.g. cross platform .config alignment/comparison). My gut feeling
> > tells me this is mainly caused by the extremely unfriendly unreadable
> > expressions which is how reverse dependencies used to be represented before
> > commit 1ccb27143360 ("kconfig: make "Selected by:" and "Implied by:"
> > readable"). Since this is now greatly improved, would you mind printing
> > Selected/Implied-by in zconfdump? There are also other zconfdump fixes
> > sitting in my local branch, like https://patchwork.kernel.org/patch/9253535/ .
> > I would like to push them to you (they probably deserve a separate
> > thread), if you think this is appropriate.
> >
> > Best regards,
> > Eugeniu.
> 
> Shameless plug:
> 
> If you need to do any fancy Kconfig parsing and comparison, then
> Kconfiglib (https://github.com/ulfalizer/Kconfiglib) might be handy.
> It makes it easy to walk the menu tree, print and inspect symbols and
> expressions, etc.

Currently, when comparing kernel configurations of two platforms P1 and
P2, our zconfdump parser generates reports like below. It helps us quite
much during .config alignment. The report can be isolated to a specific menu
entry (in this case, it is "General setup"). In this particular case, P1 is
imx6 using v3.14 kernel and P2 is rcar3 using a v4.x kernel. If Kconfiglib
is able to generate a similar diff report, I will happily embrace it.
If not, we might have no choice but keep using zconfdump for a while.

Unfortunately, I haven't found some time to put my hands on Kconfiglib
yet and I don't know what it is (or would be) capable of. Bottomline,
the motivation to fix and enrich vanilla zconfdump functionality
originates from making such reports possible on our side.

                                  Boolean -    - String   
                                Tristate - \  / - Hex
                              config -    \ || / - Integer
                         menuconfig - \    |||| /
                                     \|    |||||
  Config Name                 Val    MC    TBSHI    Cfg.Origin         Comments
                             P1 P2  P1 P2  P1 P2  P1         P2         P1 P2
HAVE_KERNEL_GZIP             y  -   C  C   B  B   sel-by     -          01 -
HAVE_KERNEL_LZMA             y  -   C  C   B  B   sel-by     -          02 -
HAVE_KERNEL_XZ               y  -   C  C   B  B   sel-by     -          03 -
HAVE_KERNEL_LZO              y  -   C  C   B  B   sel-by     -          04 -
HAVE_KERNEL_LZ4              y  -   C  C   B  B   sel-by     -          05 -
HAVE_KERNEL_LZ77             y  -   C  -   B  -   sel-by     -          06 -
KERNEL_LZO                   y  -   C  C   B  B   defconfig  -          -  -
KTIME_SCALAR                 y  -   C  -   B  -   sel-by     -          07 -
GENERIC_CLOCKEVENTS_BUILD    y  -   C  -   B  -   default    -          08 -
IKCONFIG                     m  y   C  C   T  T   defconfig  defconfig  -  -
MM_OWNER                     y  -   C  -   B  -   sel-by     -          09 -
PERF_USE_VMALLOC             y  -   C  C   B  B   sel-by     -          10 -
HAVE_OPROFILE                y  -   C  C   B  B   sel-by     -          11 -
UPROBES                      y  n   C  C   B  B   sel-by     default    12 13
ARCH_USE_BUILTIN_BSWAP       y  -   C  C   B  B   sel-by     -          14 -
HAVE_DMA_ATTRS               y  -   C  -   B  -   sel-by     -          15 -
ARCH_WANT_IPC_PARSE_VERSION  y  -   C  C   B  B   sel-by     -          16 -
MODULES_USE_ELF_REL          y  -   C  C   B  B   sel-by     -          17 -
OLD_SIGACTION                y  -   C  C   B  B   sel-by     -          18 -
TREE_PREEMPT_RCU             y  -   C  -   B  -   sel-by-ch  -          19 -
RCU_FANOUT                   32 64  C  C   I  I   default    default    20 21
RESOURCE_COUNTERS            y  -   C  -   B  -   defconfig  -          -  -

Comments:
01) selected by (ARM [=y])
02) selected by (ARM [=y])
03) selected by (ARM [=y])
04) selected by (ARM [=y])
05) selected by (ARM [=y])
06) selected by (ARM [=y])
07) selected by (ARM [=y])
08) default y if GENERIC_CLOCKEVENTS [=y]
09) selected by (MEMCG [=y] && CGROUPS [=y] && RESOURCE_COUNTERS [=y])
10) selected by (ARM [=y])
11) selected by (ARM [=y] && HAVE_PERF_EVENTS [=y])
12) selected by (UPROBE_EVENT [=y] && TRACING_SUPPORT [=y] && FTRACE [=y] &&
                 ARCH_SUPPORTS_UPROBES [=y] && MMU [=y] && PERF_EVENTS [=y])
13) default n
14) selected by (ARM [=y])
15) selected by (ARM [=y])
16) selected by (ARM [=y])
17) selected by (ARM [=y])
18) selected by (ARM [=y])
19) selected by choice "RCU Implementation"
20) default 32 if ((TREE_RCU || TREE_PREEMPT_RCU) && !64BIT) [=y]
21) default 64 if ((TREE_RCU || PREEMPT_RCU) && RCU_EXPERT && 64BIT) [=y]
 
> Cheers,
> Ulf

Best regards,
Eugeniu.
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ulf Magnusson Feb. 24, 2018, 12:31 a.m. UTC | #9

On Sat, Feb 24, 2018 at 1:13 AM, Eugeniu Rosca <[e-mail]> wrote:
> Hi Ulf,
>
> On Fri, Feb 23, 2018 at 11:18:17PM +0100, Ulf Magnusson wrote:
>> On Fri, Feb 23, 2018 at 3:04 PM, Eugeniu Rosca <[e-mail]> wrote:
>> >
>> > Hi Masahiro,
>> >
>> > On Fri, Feb 23, 2018 at 09:10:37PM +0900, Masahiro Yamada wrote:
>> >> Eugeniu,
>> >>
>> >>
>> >> 2018-02-21 15:10 GMT+09:00 Eugeniu Rosca <[e-mail]>:
>> >>
>> >> >
>> >> > I've tested https://patchwork.kernel.org/patch/10229545/ and
>> >> > https://patchwork.kernel.org/patch/10231295/ and they work great for me.
>> >> > Thank you for this feature.
>> >> >
>> >>
>> >> I will give you that code in the second one.
>> >
>> > I appreciate that, although I am open minded who takes the authorship.
>> > This feature makes my life easier and that's the most important part.
>> >
>> >>
>> >> Please fill the commit log with yours,
>> >> and also fix the bugs pointed by Petr.
>> >
>> > Sure. Will do that.
>> >
>> >> Anyway, I will claim my contribution
>> >> in the form of Signed-off-by when I pick it up.  :)
>> >
>> > I have one side question loosely related to this patch.
>> >
>> > Reverse dependencies are still not printed in zconfdump() (used in our kernel
>> > team for e.g. cross platform .config alignment/comparison). My gut feeling
>> > tells me this is mainly caused by the extremely unfriendly unreadable
>> > expressions which is how reverse dependencies used to be represented before
>> > commit 1ccb27143360 ("kconfig: make "Selected by:" and "Implied by:"
>> > readable"). Since this is now greatly improved, would you mind printing
>> > Selected/Implied-by in zconfdump? There are also other zconfdump fixes
>> > sitting in my local branch, like https://patchwork.kernel.org/patch/9253535/ .
>> > I would like to push them to you (they probably deserve a separate
>> > thread), if you think this is appropriate.
>> >
>> > Best regards,
>> > Eugeniu.
>>
>> Shameless plug:
>>
>> If you need to do any fancy Kconfig parsing and comparison, then
>> Kconfiglib (https://github.com/ulfalizer/Kconfiglib) might be handy.
>> It makes it easy to walk the menu tree, print and inspect symbols and
>> expressions, etc.
>
> Currently, when comparing kernel configurations of two platforms P1 and
> P2, our zconfdump parser generates reports like below. It helps us quite
> much during .config alignment. The report can be isolated to a specific menu
> entry (in this case, it is "General setup"). In this particular case, P1 is
> imx6 using v3.14 kernel and P2 is rcar3 using a v4.x kernel. If Kconfiglib
> is able to generate a similar diff report, I will happily embrace it.
> If not, we might have no choice but keep using zconfdump for a while.
>
> Unfortunately, I haven't found some time to put my hands on Kconfiglib
> yet and I don't know what it is (or would be) capable of. Bottomline,
> the motivation to fix and enrich vanilla zconfdump functionality
> originates from making such reports possible on our side.
>
>                                   Boolean -    - String
>                                 Tristate - \  / - Hex
>                               config -    \ || / - Integer
>                          menuconfig - \    |||| /
>                                      \|    |||||
>   Config Name                 Val    MC    TBSHI    Cfg.Origin         Comments
>                              P1 P2  P1 P2  P1 P2  P1         P2         P1 P2
> HAVE_KERNEL_GZIP             y  -   C  C   B  B   sel-by     -          01 -
> HAVE_KERNEL_LZMA             y  -   C  C   B  B   sel-by     -          02 -
> HAVE_KERNEL_XZ               y  -   C  C   B  B   sel-by     -          03 -
> HAVE_KERNEL_LZO              y  -   C  C   B  B   sel-by     -          04 -
> HAVE_KERNEL_LZ4              y  -   C  C   B  B   sel-by     -          05 -
> HAVE_KERNEL_LZ77             y  -   C  -   B  -   sel-by     -          06 -
> KERNEL_LZO                   y  -   C  C   B  B   defconfig  -          -  -
> KTIME_SCALAR                 y  -   C  -   B  -   sel-by     -          07 -
> GENERIC_CLOCKEVENTS_BUILD    y  -   C  -   B  -   default    -          08 -
> IKCONFIG                     m  y   C  C   T  T   defconfig  defconfig  -  -
> MM_OWNER                     y  -   C  -   B  -   sel-by     -          09 -
> PERF_USE_VMALLOC             y  -   C  C   B  B   sel-by     -          10 -
> HAVE_OPROFILE                y  -   C  C   B  B   sel-by     -          11 -
> UPROBES                      y  n   C  C   B  B   sel-by     default    12 13
> ARCH_USE_BUILTIN_BSWAP       y  -   C  C   B  B   sel-by     -          14 -
> HAVE_DMA_ATTRS               y  -   C  -   B  -   sel-by     -          15 -
> ARCH_WANT_IPC_PARSE_VERSION  y  -   C  C   B  B   sel-by     -          16 -
> MODULES_USE_ELF_REL          y  -   C  C   B  B   sel-by     -          17 -
> OLD_SIGACTION                y  -   C  C   B  B   sel-by     -          18 -
> TREE_PREEMPT_RCU             y  -   C  -   B  -   sel-by-ch  -          19 -
> RCU_FANOUT                   32 64  C  C   I  I   default    default    20 21
> RESOURCE_COUNTERS            y  -   C  -   B  -   defconfig  -          -  -
>
> Comments:
> 01) selected by (ARM [=y])
> 02) selected by (ARM [=y])
> 03) selected by (ARM [=y])
> 04) selected by (ARM [=y])
> 05) selected by (ARM [=y])
> 06) selected by (ARM [=y])
> 07) selected by (ARM [=y])
> 08) default y if GENERIC_CLOCKEVENTS [=y]
> 09) selected by (MEMCG [=y] && CGROUPS [=y] && RESOURCE_COUNTERS [=y])
> 10) selected by (ARM [=y])
> 11) selected by (ARM [=y] && HAVE_PERF_EVENTS [=y])
> 12) selected by (UPROBE_EVENT [=y] && TRACING_SUPPORT [=y] && FTRACE [=y] &&
>                  ARCH_SUPPORTS_UPROBES [=y] && MMU [=y] && PERF_EVENTS [=y])
> 13) default n
> 14) selected by (ARM [=y])
> 15) selected by (ARM [=y])
> 16) selected by (ARM [=y])
> 17) selected by (ARM [=y])
> 18) selected by (ARM [=y])
> 19) selected by choice "RCU Implementation"
> 20) default 32 if ((TREE_RCU || TREE_PREEMPT_RCU) && !64BIT) [=y]
> 21) default 64 if ((TREE_RCU || PREEMPT_RCU) && RCU_EXPERT && 64BIT) [=y]

Think it would be a good fit for that. There's a load_config()
function for loading a .config, and after that you can inspect symbol
values, properties, expressions, etc.

The menu tree is exposed directly too, so you could look for a menu
with a particular name and limit yourself to that. See e.g.
https://github.com/ulfalizer/Kconfiglib/blob/master/examples/print_tree.py
for how to walk the menu tree.

Kconfiglib is kinda like an "open-internals" version of the C tools if
you will, and pretty flexible.

I'm starting to feel a bit dirty, so I guess we could move it to
private mail or an issue on the GitHub page or something if you give
it a try and have any questions. Think it would work well at least. :)

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c
index 95dc058a236f..db9a89b9bede 100644
--- a/scripts/kconfig/expr.c
+++ b/scripts/kconfig/expr.c
@@ -1186,10 +1186,9 @@  expr_print_revdep(struct expr *e,
 		  int prevtoken,
 		  enum print_type type)
 {
-	if (type == PRINT_REVDEP_ALL				  ||
-	    type == PRINT_REVDEP_YES && expr_calc_value(e) == yes ||
-	    type == PRINT_REVDEP_MOD && expr_calc_value(e) == mod ||
-	    type == PRINT_REVDEP_NO  && expr_calc_value(e) == no) {
+	if ((type == PRINT_REVDEP_YES && expr_calc_value(e) == yes) ||
+	    (type == PRINT_REVDEP_MOD && expr_calc_value(e) == mod) ||
+	    (type == PRINT_REVDEP_NO  && expr_calc_value(e) == no)) {
 		fn(data, NULL, "\n  - ");
 		expr_print(e, fn, data, prevtoken);
 	}
@@ -1212,17 +1211,10 @@  __expr_print(struct expr *e,
 	switch (e->type) {
 	case E_SYMBOL:
 		if (e->left.sym->name)
-			switch (type) {
-			case PRINT_NORMAL:
+			if (type == PRINT_NORMAL)
 				fn(data, e->left.sym, e->left.sym->name);
-				break;
-			case PRINT_REVDEP_ALL:
-			case PRINT_REVDEP_YES:
-			case PRINT_REVDEP_MOD:
-			case PRINT_REVDEP_NO:
+			else
 				expr_print_revdep(e, fn, data, E_OR, type);
-				break;
-			}
 		else
 			fn(data, NULL, "<choice>");
 		break;
@@ -1271,18 +1263,12 @@  __expr_print(struct expr *e,
 		__expr_print(e->right.expr, fn, data, E_OR, type);
 		break;
 	case E_AND:
-		switch (type) {
-		case PRINT_NORMAL:
+		if (type == PRINT_NORMAL) {
 			expr_print(e->left.expr, fn, data, E_AND);
 			fn(data, NULL, " && ");
 			expr_print(e->right.expr, fn, data, E_AND);
-			break;
-		case PRINT_REVDEP_ALL:
-		case PRINT_REVDEP_YES:
-		case PRINT_REVDEP_MOD:
-		case PRINT_REVDEP_NO:
+		} else {
 			expr_print_revdep(e, fn, data, E_OR, type);
-			break;
 		}
 		break;
 	case E_LIST:
@@ -1370,27 +1356,14 @@  void expr_gstr_print(struct expr *e, struct gstr *gs)
  */
 bool expr_revdep_contains(struct expr *e, tristate val)
 {
-	bool ret = false;
-
 	if (!e)
-		return ret;
+		return false;
 
-	switch (e->type) {
-	case E_SYMBOL:
-	case E_AND:
-		if (expr_calc_value(e) == val)
-			ret = true;
-		break;
-	case E_OR:
-		if (expr_revdep_contains(e->left.expr, val))
-			ret = true;
-		else if (expr_revdep_contains(e->right.expr, val))
-			ret = true;
-		break;
-	default:
-		break;
-	}
-	return ret;
+	if (e->type == E_OR)
+		return expr_revdep_contains(e->left.expr, val) ||
+		       expr_revdep_contains(e->right.expr, val);
+
+	return expr_calc_value(e) == val;
 }
 
 /*
diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
index d5b096725ca8..e5687b430c17 100644
--- a/scripts/kconfig/expr.h
+++ b/scripts/kconfig/expr.h
@@ -36,7 +36,6 @@  enum expr_type {
 
 enum print_type {
 	PRINT_NORMAL,
-	PRINT_REVDEP_ALL,
 	PRINT_REVDEP_YES,
 	PRINT_REVDEP_MOD,
 	PRINT_REVDEP_NO,