Re: [PATCH 2/2] kconfig: Print the value of each reverse dependency

[Eugeniu Rosca <roscaeugeniu@gmail.com>]

Hi Ulf,

On Tue, Feb 13, 2018 at 07:18:27AM +0100, Ulf Magnusson wrote:
> On Tue, Feb 13, 2018 at 1:56 AM, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> > From: Eugeniu Rosca <erosca@de.adit-jv.com>
> >
> > Assuming commit 617aebe6a97e ("Merge tag 'usercopy-v4.16-rc1' of
> > git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux"), ARCH=arm64
> > and vanilla arm64 defconfig, here is the top 10 CONFIG options with
> > the highest amount of OR sub-expressions that make up the final
> > "{Selected,Implied} by" reverse dependency expression.
> >
> > | Config                        | Revdep all | Revdep ![=n] |
> > |-------------------------------|------------|--------------|
> > | REGMAP_I2C                    | 212        | 9            |
> > | CRC32                         | 167        | 25           |
> > | FW_LOADER                     | 128        | 5            |
> > | MFD_CORE                      | 124        | 9            |
> > | FB_CFB_IMAGEBLIT              | 114        | 2            |
> > | FB_CFB_COPYAREA               | 111        | 2            |
> > | FB_CFB_FILLRECT               | 110        | 2            |
> > | SND_PCM                       | 103        | 2            |
> > | CRYPTO_HASH                   | 87         | 19           |
> > | WATCHDOG_CORE                 | 86         | 6            |
> >
> > The story behind the above table is that the user needs to visually
> > review/evaluate 212 expressions which *potentially* select REGMAP_I2C
> > in order to identify the expressions which *actually* select REGMAP_I2C,
> > for a particular ARCH and for a particular defconfig used.
> >
> > To make this experience smoother, transform the way reverse dependencies
> > are displayed to the user from [1] to [2].
> >
> > [1] Before this commit
> > Symbol: MTD_BLKDEVS [=y]
> >   ...
> >   Selected by:
> >   - MTD_BLOCK [=y] && MTD [=y] && BLOCK [=y]
> >   - MTD_BLOCK_RO [=n] && MTD [=y] && MTD_BLOCK [=y]!=y && BLOCK [=y]
> >   - FTL [=n] && MTD [=y] && BLOCK [=y]
> >   - NFTL [=n] && MTD [=y] && BLOCK [=y]
> >   - INFTL [=n] && MTD [=y] && BLOCK [=y]
> >   - RFD_FTL [=n] && MTD [=y] && BLOCK [=y]
> >   - SSFDC [=n] && MTD [=y] && BLOCK [=y]
> >   - SM_FTL [=n] && MTD [=y] && BLOCK [=y]
> >   - MTD_SWAP [=n] && MTD [=y] && SWAP [=y]
> >
> > [2] After this commit
> > Symbol: MTD_BLKDEVS [=y]
> >   ...
> >   Selected by:
> >   - [y] MTD_BLOCK [=y] && MTD [=y] && BLOCK [=y]
> >   - [ ] MTD_BLOCK_RO [=n] && MTD [=y] && MTD_BLOCK [=y]!=y && BLOCK [=y]
> >   - [ ] FTL [=n] && MTD [=y] && BLOCK [=y]
> >   - [ ] NFTL [=n] && MTD [=y] && BLOCK [=y]
> >   - [ ] INFTL [=n] && MTD [=y] && BLOCK [=y]
> >   - [ ] RFD_FTL [=n] && MTD [=y] && BLOCK [=y]
> >   - [ ] SSFDC [=n] && MTD [=y] && BLOCK [=y]
> >   - [ ] SM_FTL [=n] && MTD [=y] && BLOCK [=y]
> >   - [ ] MTD_SWAP [=n] && MTD [=y] && SWAP [=y]
> >
> > This patch has been tested using a custom variant of zconfdump which
> > prints the reverse dependencies for each config symbol.
> >
> > Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> > Suggested-by: Ulf Magnusson <ulfalizer@gmail.com>
> > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> > ---
> >  scripts/kconfig/expr.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c
> > index 0704bcdf4c78..173fc5b252d5 100644
> > --- a/scripts/kconfig/expr.c
> > +++ b/scripts/kconfig/expr.c
> > @@ -1185,7 +1185,19 @@ expr_print_newline(struct expr *e,
> >                    void *data,
> >                    int prevtoken)
> >  {
> > -       fn(data, NULL, "\n  - ");
> > +       switch (expr_calc_value(e)) {
> > +       case yes:
> > +               fn(data, NULL, "\n  - [y] ");
> > +               break;
> > +       case mod:
> > +               fn(data, NULL, "\n  - [m] ");
> > +               break;
> > +       case no:
> > +               fn(data, NULL, "\n  - [ ] ");
> > +               break;
> > +       default:
> > +               break;
> 
> The default case is redundant. expr_calc_value() returns a
> 
>         typedef enum tristate {
>                 no, mod, yes
>         } tristate;
> 
> (There's a separate sym_get_string_value() that deals with "string
> values" of symbols.)

Is this variant better?

diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c
index 0704bcdf4c78..4bf01269ce72 100644
--- a/scripts/kconfig/expr.c
+++ b/scripts/kconfig/expr.c
@@ -1185,7 +1185,24 @@ expr_print_newline(struct expr *e,
 		   void *data,
 		   int prevtoken)
 {
-	fn(data, NULL, "\n  - ");
+	char buf[32];
+	const char *str;
+
+	switch (expr_calc_value(e)) {
+	case yes:
+		str = sym_get_string_value(&symbol_yes);
+		break;
+	case mod:
+		str = sym_get_string_value(&symbol_mod);
+		break;
+	case no:
+		/* Prefer '[ ]' to '[n]' for readability */
+		str = " ";
+		break;
+	}
+
+	sprintf(buf, "\n  - [%s] ", str);
+	fn(data, NULL, buf);
 	expr_print(e, fn, data, prevtoken);
 }
 
> > +       }
> >         expr_print(e, fn, data, prevtoken);
> >  }
> >
> > --
> > 2.16.1
> >
> 
> Looks good to me otherwise. I still like that earlier sorting idea,
> but this is a big improvement already.

To be honest, purely as a user, I would probably prefer something like
below:

Selected by [y/m]:
- EXPR_01
- EXPR_02
Selected by [n]:
- EXPR_03
- EXPR_04

But (as a developer) I feel it can only come at the price of increased
complexity of __expr_print() (at least, if not bigger than, [1]). What I
like about the current approach suggested by Masahiro is that it keeps
the impact low with still making a great improvement in readability.

If you think we should explore the second possibility of grouping the
active/inactive dependencies, I'll try to come up with some solution
in the next days.

[1] https://marc.info/?l=linux-kbuild&m=151777006005199&w=4

> Cheers,
> Ulf

Thanks for your review and comments!

Best regards,
Eugeniu.