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

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

[Eugeniu Rosca]

From: Eugeniu Rosca <erosca@de.adit-jv.com>

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(-)

-- 
2.16.1

[PATCH v4 1/3] kconfig: Print reverse dependencies on new line consistently

[Eugeniu Rosca]

From: Eugeniu Rosca <erosca@de.adit-jv.com>

Commit 1ccb27143360 ("kconfig: make "Selected by:" and "Implied by:"
readable") made an incredible improvement in how reverse dependencies
are perceived by the user, by breaking down the single (often
interminable) expression string into small readable chunks, each of
them displayed on a separate line:

Selected by:
- A && B
- C && (D || E)

Unfortunately, what happens with the non-OR (either E_SYMBOL or E_AND)
expressions is that they don't get a dedicated line:

Selected by: F && G

That's arguably a bug/misbehavior, but it puts some amount of burden in
implementing new ways of printing reverse dependencies to the user. As
example, if we prefix every reverse dependency top level "||" token by
its tristate value, then subjectively [2] looks more readable than [1].

[1] Selected by: [m] F && G
[2] Selected by:
    - [m] F && G

Also, if we print the reverse dependency sub-expressions in groups
(clustered by the tristate value they evaluate to), then
subjectively [4] looks more readable than [3].

[3] Selected by [m]: F && G
[4] Selected by [m]:
    - F && G

Based on the above, print all the reverse dependency sub-expressions on
a separate line _consistently_. An example of change contributed by this
commit is seen below.

| Symbol: NEED_SG_DMA_LENGTH [=y]
| ...
|   Selected by: IOMMU_DMA [=y] && IOMMU_SUPPORT [=y]

becomes:

| Symbol: NEED_SG_DMA_LENGTH [=y]
| ...
|   Selected by:
|   - IOMMU_DMA [=y] && IOMMU_SUPPORT [=y]

This patch has been tested using a tuned 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>
Reviewed-by: Ulf Magnusson <ulfalizer@gmail.com>
---
 scripts/kconfig/expr.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c
index d45381986ac7..b89baed7f15c 100644
--- a/scripts/kconfig/expr.c
+++ b/scripts/kconfig/expr.c
@@ -1179,6 +1179,16 @@ struct expr *expr_simplify_unmet_dep(struct expr *e1, struct expr *e2)
 	return expr_get_leftmost_symbol(ret);
 }
 
+static void
+expr_print_newline(struct expr *e,
+		   void (*fn)(void *, struct symbol *, const char *),
+		   void *data,
+		   int prevtoken)
+{
+	fn(data, NULL, "\n  - ");
+	expr_print(e, fn, data, prevtoken);
+}
+
 static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, const char *), void *data, int prevtoken, bool revdep)
 {
 	if (!e) {
@@ -1191,7 +1201,10 @@ static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, con
 	switch (e->type) {
 	case E_SYMBOL:
 		if (e->left.sym->name)
-			fn(data, e->left.sym, e->left.sym->name);
+			if (!revdep)
+				fn(data, e->left.sym, e->left.sym->name);
+			else
+				expr_print_newline(e, fn, data, E_OR);
 		else
 			fn(data, NULL, "<choice>");
 		break;
@@ -1234,19 +1247,19 @@ static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, con
 		fn(data, e->right.sym, e->right.sym->name);
 		break;
 	case E_OR:
-		if (revdep && e->left.expr->type != E_OR)
-			fn(data, NULL, "\n  - ");
 		__expr_print(e->left.expr, fn, data, E_OR, revdep);
-		if (revdep)
-			fn(data, NULL, "\n  - ");
-		else
+		if (!revdep)
 			fn(data, NULL, " || ");
 		__expr_print(e->right.expr, fn, data, E_OR, revdep);
 		break;
 	case E_AND:
-		expr_print(e->left.expr, fn, data, E_AND);
-		fn(data, NULL, " && ");
-		expr_print(e->right.expr, fn, data, E_AND);
+		if (!revdep) {
+			expr_print(e->left.expr, fn, data, E_AND);
+			fn(data, NULL, " && ");
+			expr_print(e->right.expr, fn, data, E_AND);
+		} else {
+			expr_print_newline(e, fn, data, E_OR);
+		}
 		break;
 	case E_LIST:
 		fn(data, e->right.sym, e->right.sym->name);
-- 
2.16.1

[PATCH v4 2/3] kconfig: Prepare for printing reverse dependencies in groups

[Eugeniu Rosca]

From: Eugeniu Rosca <erosca@de.adit-jv.com>

Currently, reverse dependencies are printed like [1]. Prepare the ground
for printing them like [2]. No functional change is intended in this
patch.

[1] Current pattern for printing reverse dependencies:
  Selected by:
  - EXPR_A_Y /* evaluates to =y */
  - EXPR_B_N /* evaluates to =n */
  - EXPR_C_Y /* evaluates to =y */
  - EXPR_D_M /* evaluates to =m */
  - EXPR_E_N /* evaluates to =n */

[2] Upcoming pattern for printing reverse dependencies:
  Selected by [y]:
  - EXPR_A_Y
  - EXPR_C_Y
  Selected by [m]:
  - EXPR_D_M
  Selected by [n]:
  - EXPR_B_N
  - EXPR_E_N

Suggested-by: Ulf Magnusson <ulfalizer@gmail.com>
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Reviewed-by: Ulf Magnusson <ulfalizer@gmail.com>
---
 scripts/kconfig/expr.c | 34 +++++++++++++++++++++++-----------
 scripts/kconfig/expr.h |  7 ++++++-
 scripts/kconfig/menu.c |  4 ++--
 3 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c
index b89baed7f15c..66b435bca80c 100644
--- a/scripts/kconfig/expr.c
+++ b/scripts/kconfig/expr.c
@@ -1189,7 +1189,12 @@ expr_print_newline(struct expr *e,
 	expr_print(e, fn, data, prevtoken);
 }
 
-static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, const char *), void *data, int prevtoken, bool revdep)
+static void
+__expr_print(struct expr *e,
+	     void (*fn)(void *, struct symbol *, const char *),
+	     void *data,
+	     int prevtoken,
+	     enum print_type type)
 {
 	if (!e) {
 		fn(data, NULL, "y");
@@ -1201,10 +1206,14 @@ static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, con
 	switch (e->type) {
 	case E_SYMBOL:
 		if (e->left.sym->name)
-			if (!revdep)
+			switch (type) {
+			case PRINT_NORMAL:
 				fn(data, e->left.sym, e->left.sym->name);
-			else
+				break;
+			case PRINT_REVDEP_ALL:
 				expr_print_newline(e, fn, data, E_OR);
+				break;
+			}
 		else
 			fn(data, NULL, "<choice>");
 		break;
@@ -1247,18 +1256,21 @@ static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, con
 		fn(data, e->right.sym, e->right.sym->name);
 		break;
 	case E_OR:
-		__expr_print(e->left.expr, fn, data, E_OR, revdep);
-		if (!revdep)
+		__expr_print(e->left.expr, fn, data, E_OR, type);
+		if (type == PRINT_NORMAL)
 			fn(data, NULL, " || ");
-		__expr_print(e->right.expr, fn, data, E_OR, revdep);
+		__expr_print(e->right.expr, fn, data, E_OR, type);
 		break;
 	case E_AND:
-		if (!revdep) {
+		switch (type) {
+		case PRINT_NORMAL:
 			expr_print(e->left.expr, fn, data, E_AND);
 			fn(data, NULL, " && ");
 			expr_print(e->right.expr, fn, data, E_AND);
-		} else {
+			break;
+		case PRINT_REVDEP_ALL:
 			expr_print_newline(e, fn, data, E_OR);
+			break;
 		}
 		break;
 	case E_LIST:
@@ -1289,7 +1301,7 @@ static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, con
 
 void expr_print(struct expr *e, void (*fn)(void *, struct symbol *, const char *), void *data, int prevtoken)
 {
-	__expr_print(e, fn, data, prevtoken, false);
+	__expr_print(e, fn, data, prevtoken, PRINT_NORMAL);
 }
 
 static void expr_print_file_helper(void *data, struct symbol *sym, const char *str)
@@ -1342,7 +1354,7 @@ void expr_gstr_print(struct expr *e, struct gstr *gs)
  * line with a minus. This makes expressions much easier to read.
  * Suitable for reverse dependency expressions.
  */
-void expr_gstr_print_revdep(struct expr *e, struct gstr *gs)
+void expr_gstr_print_revdep(struct expr *e, struct gstr *gs, enum print_type t)
 {
-	__expr_print(e, expr_print_gstr_helper, gs, E_NONE, true);
+	__expr_print(e, expr_print_gstr_helper, gs, E_NONE, t);
 }
diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
index c16e82e302a2..21cb67c15091 100644
--- a/scripts/kconfig/expr.h
+++ b/scripts/kconfig/expr.h
@@ -34,6 +34,11 @@ enum expr_type {
 	E_LIST, E_SYMBOL, E_RANGE
 };
 
+enum print_type {
+	PRINT_NORMAL,
+	PRINT_REVDEP_ALL,
+};
+
 union expr_data {
 	struct expr *expr;
 	struct symbol *sym;
@@ -310,7 +315,7 @@ struct expr *expr_simplify_unmet_dep(struct expr *e1, struct expr *e2);
 void expr_fprint(struct expr *e, FILE *out);
 struct gstr; /* forward */
 void expr_gstr_print(struct expr *e, struct gstr *gs);
-void expr_gstr_print_revdep(struct expr *e, struct gstr *gs);
+void expr_gstr_print_revdep(struct expr *e, struct gstr *gs, enum print_type t);
 
 static inline int expr_is_yes(struct expr *e)
 {
diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index 99222855544c..5b8edba105f2 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -828,14 +828,14 @@ static void get_symbol_str(struct gstr *r, struct symbol *sym,
 	get_symbol_props_str(r, sym, P_SELECT, _("  Selects: "));
 	if (sym->rev_dep.expr) {
 		str_append(r, _("  Selected by: "));
-		expr_gstr_print_revdep(sym->rev_dep.expr, r);
+		expr_gstr_print_revdep(sym->rev_dep.expr, r, PRINT_REVDEP_ALL);
 		str_append(r, "\n");
 	}
 
 	get_symbol_props_str(r, sym, P_IMPLY, _("  Implies: "));
 	if (sym->implied.expr) {
 		str_append(r, _("  Implied by: "));
-		expr_gstr_print_revdep(sym->implied.expr, r);
+		expr_gstr_print_revdep(sym->implied.expr, r, PRINT_REVDEP_ALL);
 		str_append(r, "\n");
 	}
 
-- 
2.16.1

[PATCH v4 3/3] kconfig: Print reverse dependencies in groups

[Eugeniu Rosca]

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 top level "||" sub-expressions/tokens 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 is that users need 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, change the way reverse dependencies
are displayed to the user from [1] to [2].

[1] Old representation of reverse dependencies for DMA_ENGINE_RAID:
  Selected by:
  - AMCC_PPC440SPE_ADMA [=n] && DMADEVICES [=y] && (440SPe || 440SP)
  - BCM_SBA_RAID [=m] && DMADEVICES [=y] && (ARM64 [=y] || ...
  - FSL_RAID [=n] && DMADEVICES [=y] && FSL_SOC && ...
  - INTEL_IOATDMA [=n] && DMADEVICES [=y] && PCI [=y] && X86_64
  - MV_XOR [=n] && DMADEVICES [=y] && (PLAT_ORION || ARCH_MVEBU [=y] ...
  - MV_XOR_V2 [=y] && DMADEVICES [=y] && ARM64 [=y]
  - XGENE_DMA [=n] && DMADEVICES [=y] && (ARCH_XGENE [=y] || ...
  - DMATEST [=n] && DMADEVICES [=y] && DMA_ENGINE [=y]

[2] New representation of reverse dependencies for DMA_ENGINE_RAID:
  Selected by [y]:
  - MV_XOR_V2 [=y] && DMADEVICES [=y] && ARM64 [=y]
  Selected by [m]:
  - BCM_SBA_RAID [=m] && 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
  - MV_XOR [=n] && DMADEVICES [=y] && (PLAT_ORION || ARCH_MVEBU [=y] ...
  - XGENE_DMA [=n] && DMADEVICES [=y] && (ARCH_XGENE [=y] || ...
  - DMATEST [=n] && DMADEVICES [=y] && DMA_ENGINE [=y]

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 scripts/kconfig/expr.c      | 63 ++++++++++++++++++++++++++++++++++++++-------
 scripts/kconfig/expr.h      |  4 +++
 scripts/kconfig/lkc_proto.h |  1 +
 scripts/kconfig/menu.c      | 37 +++++++++++++++++++-------
 4 files changed, 86 insertions(+), 19 deletions(-)

diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c
index 66b435bca80c..95dc058a236f 100644
--- a/scripts/kconfig/expr.c
+++ b/scripts/kconfig/expr.c
@@ -1180,13 +1180,19 @@ struct expr *expr_simplify_unmet_dep(struct expr *e1, struct expr *e2)
 }
 
 static void
-expr_print_newline(struct expr *e,
-		   void (*fn)(void *, struct symbol *, const char *),
-		   void *data,
-		   int prevtoken)
+expr_print_revdep(struct expr *e,
+		  void (*fn)(void *, struct symbol *, const char *),
+		  void *data,
+		  int prevtoken,
+		  enum print_type type)
 {
-	fn(data, NULL, "\n  - ");
-	expr_print(e, fn, data, prevtoken);
+	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) {
+		fn(data, NULL, "\n  - ");
+		expr_print(e, fn, data, prevtoken);
+	}
 }
 
 static void
@@ -1211,7 +1217,10 @@ __expr_print(struct expr *e,
 				fn(data, e->left.sym, e->left.sym->name);
 				break;
 			case PRINT_REVDEP_ALL:
-				expr_print_newline(e, fn, data, E_OR);
+			case PRINT_REVDEP_YES:
+			case PRINT_REVDEP_MOD:
+			case PRINT_REVDEP_NO:
+				expr_print_revdep(e, fn, data, E_OR, type);
 				break;
 			}
 		else
@@ -1269,7 +1278,10 @@ __expr_print(struct expr *e,
 			expr_print(e->right.expr, fn, data, E_AND);
 			break;
 		case PRINT_REVDEP_ALL:
-			expr_print_newline(e, fn, data, E_OR);
+		case PRINT_REVDEP_YES:
+		case PRINT_REVDEP_MOD:
+		case PRINT_REVDEP_NO:
+			expr_print_revdep(e, fn, data, E_OR, type);
 			break;
 		}
 		break;
@@ -1349,10 +1361,43 @@ void expr_gstr_print(struct expr *e, struct gstr *gs)
 	expr_print(e, expr_print_gstr_helper, gs, E_NONE);
 }
 
+/*
+ * Allow front ends to check if a specific reverse dependency expression
+ * has at least one top level "||" member which evaluates to "val". This,
+ * will allow front ends to, as example, avoid printing "Selected by [y]:"
+ * line when there are actually no top level "||" sub-expressions which
+ * evaluate to =y.
+ */
+bool expr_revdep_contains(struct expr *e, tristate val)
+{
+	bool ret = false;
+
+	if (!e)
+		return ret;
+
+	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;
+}
+
 /*
  * Transform the top level "||" tokens into newlines and prepend each
  * line with a minus. This makes expressions much easier to read.
- * Suitable for reverse dependency expressions.
+ * Suitable for reverse dependency expressions. In addition, allow
+ * selective printing of tokens/sub-expressions by their tristate value.
  */
 void expr_gstr_print_revdep(struct expr *e, struct gstr *gs, enum print_type t)
 {
diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
index 21cb67c15091..d5b096725ca8 100644
--- a/scripts/kconfig/expr.h
+++ b/scripts/kconfig/expr.h
@@ -37,6 +37,9 @@ enum expr_type {
 enum print_type {
 	PRINT_NORMAL,
 	PRINT_REVDEP_ALL,
+	PRINT_REVDEP_YES,
+	PRINT_REVDEP_MOD,
+	PRINT_REVDEP_NO,
 };
 
 union expr_data {
@@ -316,6 +319,7 @@ void expr_fprint(struct expr *e, FILE *out);
 struct gstr; /* forward */
 void expr_gstr_print(struct expr *e, struct gstr *gs);
 void expr_gstr_print_revdep(struct expr *e, struct gstr *gs, enum print_type t);
+bool expr_revdep_contains(struct expr *e, tristate val);
 
 static inline int expr_is_yes(struct expr *e)
 {
diff --git a/scripts/kconfig/lkc_proto.h b/scripts/kconfig/lkc_proto.h
index 9dc8abfb1dc3..69ed1477e4ef 100644
--- a/scripts/kconfig/lkc_proto.h
+++ b/scripts/kconfig/lkc_proto.h
@@ -25,6 +25,7 @@ bool menu_has_help(struct menu *menu);
 const char * menu_get_help(struct menu *menu);
 struct gstr get_relations_str(struct symbol **sym_arr, struct list_head *head);
 void menu_get_ext_help(struct menu *menu, struct gstr *help);
+void get_revdep_by_type(struct expr *e, char *s, struct gstr *r);
 
 /* symbol.c */
 extern struct symbol * symbol_hash[SYMBOL_HASHSIZE];
diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index 5b8edba105f2..029da77fe1b0 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -790,6 +790,31 @@ static void get_symbol_props_str(struct gstr *r, struct symbol *sym,
 		str_append(r, "\n");
 }
 
+void get_revdep_by_type(struct expr *e, char *s, struct gstr *r)
+{
+	if (!e)
+		return;
+
+	if (expr_revdep_contains(e, yes)) {
+		str_append(r, s);
+		str_append(r, " [y]:");
+		expr_gstr_print_revdep(e, r, PRINT_REVDEP_YES);
+		str_append(r, "\n");
+	}
+	if (expr_revdep_contains(e, mod)) {
+		str_append(r, s);
+		str_append(r, " [m]:");
+		expr_gstr_print_revdep(e, r, PRINT_REVDEP_MOD);
+		str_append(r, "\n");
+	}
+	if (expr_revdep_contains(e, no)) {
+		str_append(r, s);
+		str_append(r, " [n]:");
+		expr_gstr_print_revdep(e, r, PRINT_REVDEP_NO);
+		str_append(r, "\n");
+	}
+}
+
 /*
  * head is optional and may be NULL
  */
@@ -826,18 +851,10 @@ static void get_symbol_str(struct gstr *r, struct symbol *sym,
 	}
 
 	get_symbol_props_str(r, sym, P_SELECT, _("  Selects: "));
-	if (sym->rev_dep.expr) {
-		str_append(r, _("  Selected by: "));
-		expr_gstr_print_revdep(sym->rev_dep.expr, r, PRINT_REVDEP_ALL);
-		str_append(r, "\n");
-	}
+	get_revdep_by_type(sym->rev_dep.expr, "  Selected by", r);
 
 	get_symbol_props_str(r, sym, P_IMPLY, _("  Implies: "));
-	if (sym->implied.expr) {
-		str_append(r, _("  Implied by: "));
-		expr_gstr_print_revdep(sym->implied.expr, r, PRINT_REVDEP_ALL);
-		str_append(r, "\n");
-	}
+	get_revdep_by_type(sym->implied.expr, "  Implied by", r);
 
 	str_append(r, "\n\n");
 }
-- 
2.16.1

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

[Ulf Magnusson]

On Sun, Feb 18, 2018 at 9:47 PM, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> From: Eugeniu Rosca <erosca@de.adit-jv.com>
>
> 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(-)
>
> --
> 2.16.1
>

Whole patchset looks fine to me now:

Reviewed-by: Ulf Magnusson <ulfalizer@gmail.com>

Cheers,
Ulf

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

[Masahiro Yamada]

2018-02-19 5:47 GMT+09:00 Eugeniu Rosca <roscaeugeniu@gmail.com>:
> From: Eugeniu Rosca <erosca@de.adit-jv.com>
>
> 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.




-- 
Best Regards
Masahiro Yamada

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

[Masahiro Yamada]

2018-02-20 17:13 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> 2018-02-19 5:47 GMT+09:00 Eugeniu Rosca <roscaeugeniu@gmail.com>:
>> From: Eugeniu Rosca <erosca@de.adit-jv.com>
>>
>> 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.



-- 
Best Regards
Masahiro Yamada

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

[Ulf Magnusson]

On Tue, Feb 20, 2018 at 05:24:01PM +0900, Masahiro Yamada wrote:
> 2018-02-20 17:13 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> > 2018-02-19 5:47 GMT+09:00 Eugeniu Rosca <roscaeugeniu@gmail.com>:
> >> From: Eugeniu Rosca <erosca@de.adit-jv.com>
> >>
> >> 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. :)


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,

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

[Eugeniu Rosca]

On Tue, Feb 20, 2018 at 05:24:01PM +0900, Masahiro Yamada wrote:
> 2018-02-20 17:13 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> > 2018-02-19 5:47 GMT+09:00 Eugeniu Rosca <roscaeugeniu@gmail.com>:
> >> From: Eugeniu Rosca <erosca@de.adit-jv.com>
> >>
> >> 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/

Fwiw CP reports for https://patchwork.kernel.org/patch/10229545/

WARNING: unnecessary whitespace before a quoted newline
#113: FILE: scripts/kconfig/menu.c:830:
+		str_append(r, _("  Selected by: \n"));

WARNING: unnecessary whitespace before a quoted newline
#121: FILE: scripts/kconfig/menu.c:836:
+		str_append(r, _("  Implied by: \n"));

Both solutions behave the same for me.
I agree Masahiro's patch is much nicer.

Fwiw, both solutions display reverse dependencies of certain choices
like below:

Prompt: Compiler optimization level
   Location:
     -> General setup
  Defined at init/Kconfig:1021
  Selected by:
   - m

I can notice this output at least for:
  prompt "Cputime accounting"
  prompt "Compiler optimization level"
  prompt "Choose SLAB allocator"
  prompt "Processor family"
  prompt "Preemption Model"
  prompt "Timer frequency"
  prompt "IO delay type"
  prompt "Choose kernel unwinder"
  prompt "Default security module"

I'm not sure if `Selected by:\n  - m` is of any use/help. Omitting it,
if desired, is probably subject of a different patch?

> No 'enum print_type', please.

Ok. Feedback is appreciated.

> 
> 
> -- 
> Best Regards
> Masahiro Yamada

Best regards,
Eugeniu.

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

[Ulf Magnusson]

On Tue, Feb 20, 2018 at 08:25:08PM +0100, Eugeniu Rosca wrote:
> Fwiw, both solutions display reverse dependencies of certain choices
> like below:
> 
> Prompt: Compiler optimization level
>    Location:
>      -> General setup
>   Defined at init/Kconfig:1021
>   Selected by:
>    - m
> 
> I can notice this output at least for:
>   prompt "Cputime accounting"
>   prompt "Compiler optimization level"
>   prompt "Choose SLAB allocator"
>   prompt "Processor family"
>   prompt "Preemption Model"
>   prompt "Timer frequency"
>   prompt "IO delay type"
>   prompt "Choose kernel unwinder"
>   prompt "Default security module"
> 
> I'm not sure if `Selected by:\n  - m` is of any use/help. Omitting it,
> if desired, is probably subject of a different patch?

This is due to some obscure Kconfig internals. I added a comment in
scripts/kconfig/menu.c to explain it recently:

	/*
	 * For non-optional choices, add a reverse dependency (corresponding to
	 * a select) of '<visibility> && m'. This prevents the user from
	 * setting the choice mode to 'n' when the choice is visible.
	 *
	 * This would also work for non-choice symbols, but only non-optional
	 * choices clear SYMBOL_OPTIONAL as of writing. Choices are implemented
	 * as a type of symbol.
	 */
	if (sym && !sym_is_optional(sym) && parent->prompt) {
		sym->rev_dep.expr = expr_alloc_or(sym->rev_dep.expr,
				expr_alloc_and(parent->prompt->visible.expr,
					expr_alloc_symbol(&symbol_mod)));
	}

It doesn't hurt to show it, IMO. Might cause some confusion at worst,
but it's probably pretty minimal.

Cheers,
Ulf

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

[Eugeniu Rosca]

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 <yamada.masahiro@socionext.com>:
> > >
> > > 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.

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

[Ulf Magnusson]

On Tue, Feb 20, 2018 at 8:52 PM, Eugeniu Rosca <erosca@de.adit-jv.com> 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 <yamada.masahiro@socionext.com>:
>> > >
>> > > 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

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

[Masahiro Yamada]

2018-02-21 5:09 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>:
> On Tue, Feb 20, 2018 at 8:52 PM, Eugeniu Rosca <erosca@de.adit-jv.com> 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 <yamada.masahiro@socionext.com>:
>>> > >
>>> > > 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best Regards
Masahiro Yamada

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

[Eugeniu Rosca]

On Wed, Feb 21, 2018 at 02:48:45PM +0900, Masahiro Yamada wrote:
> 2018-02-21 5:09 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>:
> > On Tue, Feb 20, 2018 at 8:52 PM, Eugeniu Rosca <erosca@de.adit-jv.com> 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 <yamada.masahiro@socionext.com>:
> >>> > >
> >>> > > 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 majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 
> -- 
> Best Regards
> Masahiro Yamada

Best regards,
Eugeniu.

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

[Masahiro Yamada]

Eugeniu,


2018-02-21 15:10 GMT+09:00 Eugeniu Rosca <roscaeugeniu@gmail.com>:

>
> 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.  :)


-- 
Best Regards
Masahiro Yamada

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

[Eugeniu Rosca]

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 <roscaeugeniu@gmail.com>:
> 
> >
> > 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.

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

[Ulf Magnusson]

On Fri, Feb 23, 2018 at 3:04 PM, Eugeniu Rosca <erosca@de.adit-jv.com> 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 <roscaeugeniu@gmail.com>:
>>
>> >
>> > 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

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

[Eugeniu Rosca]

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 <erosca@de.adit-jv.com> 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 <roscaeugeniu@gmail.com>:
> >>
> >> >
> >> > 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.

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

[Ulf Magnusson]

On Sat, Feb 24, 2018 at 1:13 AM, Eugeniu Rosca <roscaeugeniu@gmail.com> 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 <erosca@de.adit-jv.com> 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 <roscaeugeniu@gmail.com>:
>> >>
>> >> >
>> >> > 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