From 9a47ceec543bfb703fbe2f8d584850b582caf1a6 Mon Sep 17 00:00:00 2001 From: Masahiro Yamada Date: Tue, 20 Feb 2018 17:18:47 +0900 Subject: kconfig: clean-up reverse dependency help implementation This commit splits out the special E_OR handling ('-' instead of '||') into a dedicated helper expr_print_revdev(). Restore the original expr_print() prior to commit 1ccb27143360 ("kconfig: make "Selected by:" and "Implied by:" readable"). This makes sense because: - We need to chop those expressions only when printing the reverse dependency, and only when E_OR is encountered - Otherwise, it should be printed as before, so fall back to expr_print() This also improves the behavior; for a single line, it was previously displayed in the same line as "Selected by", like this: Selected by: A [=n] && B [=n] This will be displayed in a new line, consistently: Selected by: - A [=n] && B [=n] Signed-off-by: Masahiro Yamada Reviewed-by: Petr Vorel --- scripts/kconfig/expr.c | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) (limited to 'scripts/kconfig/expr.c') diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c index d45381986ac7..cd3a8f501f38 100644 --- a/scripts/kconfig/expr.c +++ b/scripts/kconfig/expr.c @@ -1179,7 +1179,9 @@ struct expr *expr_simplify_unmet_dep(struct expr *e1, struct expr *e2) return expr_get_leftmost_symbol(ret); } -static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, const char *), void *data, int prevtoken, bool revdep) +void expr_print(struct expr *e, + void (*fn)(void *, struct symbol *, const char *), + void *data, int prevtoken) { if (!e) { fn(data, NULL, "y"); @@ -1234,14 +1236,9 @@ 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 - fn(data, NULL, " || "); - __expr_print(e->right.expr, fn, data, E_OR, revdep); + expr_print(e->left.expr, fn, data, E_OR); + fn(data, NULL, " || "); + expr_print(e->right.expr, fn, data, E_OR); break; case E_AND: expr_print(e->left.expr, fn, data, E_AND); @@ -1274,11 +1271,6 @@ static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, con fn(data, NULL, ")"); } -void expr_print(struct expr *e, void (*fn)(void *, struct symbol *, const char *), void *data, int prevtoken) -{ - __expr_print(e, fn, data, prevtoken, false); -} - static void expr_print_file_helper(void *data, struct symbol *sym, const char *str) { xfwrite(str, strlen(str), 1, data); @@ -1329,7 +1321,21 @@ 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. */ +static void expr_print_revdep(struct expr *e, + void (*fn)(void *, struct symbol *, const char *), + void *data) +{ + if (e->type == E_OR) { + expr_print_revdep(e->left.expr, fn, data); + expr_print_revdep(e->right.expr, fn, data); + } else { + fn(data, NULL, " - "); + expr_print(e, fn, data, E_NONE); + fn(data, NULL, "\n"); + } +} + void expr_gstr_print_revdep(struct expr *e, struct gstr *gs) { - __expr_print(e, expr_print_gstr_helper, gs, E_NONE, true); + expr_print_revdep(e, expr_print_gstr_helper, gs); } -- cgit v1.2.3 From d9119b5925a03b9a3191fa3e93b4091651d8ad25 Mon Sep 17 00:00:00 2001 From: Eugeniu Rosca Date: Sat, 24 Feb 2018 16:24:18 +0100 Subject: kconfig: Print reverse dependencies in groups 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") 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 tries to make one step further by putting 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. To give some numbers and quantify the complexity of certain reverse dependencies, 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 by" reverse dependency expression. | Config | All revdep | Active revdep | |-------------------|------------|---------------| | 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 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 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] Suggested-by: Masahiro Yamada Signed-off-by: Eugeniu Rosca Reviewed-by: Petr Vorel Reviewed-by: Ulf Magnusson Signed-off-by: Masahiro Yamada --- scripts/kconfig/expr.c | 18 ++++++++++++------ scripts/kconfig/expr.h | 3 ++- scripts/kconfig/menu.c | 12 ++++++------ 3 files changed, 20 insertions(+), 13 deletions(-) (limited to 'scripts/kconfig/expr.c') diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c index cd3a8f501f38..49376e12fa30 100644 --- a/scripts/kconfig/expr.c +++ b/scripts/kconfig/expr.c @@ -1323,19 +1323,25 @@ void expr_gstr_print(struct expr *e, struct gstr *gs) */ static void expr_print_revdep(struct expr *e, void (*fn)(void *, struct symbol *, const char *), - void *data) + void *data, tristate pr_type, const char **title) { if (e->type == E_OR) { - expr_print_revdep(e->left.expr, fn, data); - expr_print_revdep(e->right.expr, fn, data); - } else { + expr_print_revdep(e->left.expr, fn, data, pr_type, title); + expr_print_revdep(e->right.expr, fn, data, pr_type, title); + } else if (expr_calc_value(e) == pr_type) { + if (*title) { + fn(data, NULL, *title); + *title = NULL; + } + fn(data, NULL, " - "); expr_print(e, fn, data, E_NONE); fn(data, NULL, "\n"); } } -void expr_gstr_print_revdep(struct expr *e, struct gstr *gs) +void expr_gstr_print_revdep(struct expr *e, struct gstr *gs, + tristate pr_type, const char *title) { - expr_print_revdep(e, expr_print_gstr_helper, gs); + expr_print_revdep(e, expr_print_gstr_helper, gs, pr_type, &title); } diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h index c16e82e302a2..8dbf2a4cdae1 100644 --- a/scripts/kconfig/expr.h +++ b/scripts/kconfig/expr.h @@ -310,7 +310,8 @@ 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, + tristate pr_type, const char *title); static inline int expr_is_yes(struct expr *e) { diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c index 02f46813e732..5c5c1374b151 100644 --- a/scripts/kconfig/menu.c +++ b/scripts/kconfig/menu.c @@ -828,16 +828,16 @@ 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: ")); - str_append(r, "\n"); - expr_gstr_print_revdep(sym->rev_dep.expr, r); + expr_gstr_print_revdep(sym->rev_dep.expr, r, yes, " Selected by [y]:\n"); + expr_gstr_print_revdep(sym->rev_dep.expr, r, mod, " Selected by [m]:\n"); + expr_gstr_print_revdep(sym->rev_dep.expr, r, no, " Selected by [n]:\n"); } get_symbol_props_str(r, sym, P_IMPLY, _(" Implies: ")); if (sym->implied.expr) { - str_append(r, _(" Implied by: ")); - str_append(r, "\n"); - expr_gstr_print_revdep(sym->implied.expr, r); + expr_gstr_print_revdep(sym->implied.expr, r, yes, " Implied by [y]:\n"); + expr_gstr_print_revdep(sym->implied.expr, r, mod, " Implied by [m]:\n"); + expr_gstr_print_revdep(sym->implied.expr, r, no, " Implied by [n]:\n"); } str_append(r, "\n\n"); -- cgit v1.2.3 From f8f69dc0b4e070d4a4d39889a85424913cc922d5 Mon Sep 17 00:00:00 2001 From: Masahiro Yamada Date: Tue, 13 Mar 2018 18:56:08 +0900 Subject: kconfig: make unmet dependency warnings readable Currently, the unmet dependency warnings end up with endlessly long expressions, most of which are false positives. Here is test code to demonstrate how it currently works. [Test Case] config DEP1 def_bool y config DEP2 bool "DEP2" config A bool "A" select E config B bool "B" depends on DEP2 select E config C bool "C" depends on DEP1 && DEP2 select E config D def_bool n select E config E bool depends on DEP1 && DEP2 [Result] $ make config scripts/kconfig/conf --oldaskconfig Kconfig * * Linux Kernel Configuration * DEP2 (DEP2) [N/y/?] (NEW) n A (A) [N/y/?] (NEW) y warning: (A && B && D) selects E which has unmet direct dependencies (DEP1 && DEP2) Here, I see some points to be improved. First, '(A || B || D)' would make more sense than '(A && B && D)'. I am not sure if this is intentional, but expr_simplify_unmet_dep() turns OR expressions into AND, like follows: case E_OR: return expr_alloc_and( Second, we see false positives. 'A' is a real unmet dependency. 'B' is false positive because 'DEP1' is fixed to 'y', and 'B' depends on 'DEP2'. 'C' was correctly dropped by expr_simplify_unmet_dep(). 'D' is also false positive because it has no chance to be enabled. Current expr_simplify_unmet_dep() cannot avoid those false positives. After all, I decided to use the same helpers as used for printing reverse dependencies in the help. With this commit, unreadable warnings (most of the reported symbols are false positives) in the real world: $ make ARCH=score allyesconfig scripts/kconfig/conf --allyesconfig Kconfig warning: (HWSPINLOCK_QCOM && AHCI_MTK && STMMAC_PLATFORM && DWMAC_IPQ806X && DWMAC_LPC18XX && DWMAC_OXNAS && DWMAC_ROCKCHIP && DWMAC_SOCFPGA && DWMAC_STI && TI_CPSW && PINCTRL_GEMINI && PINCTRL_OXNAS && PINCTRL_ROCKCHIP && PINCTRL_DOVE && PINCTRL_ARMADA_37XX && PINCTRL_STM32 && S3C2410_WATCHDOG && VIDEO_OMAP3 && VIDEO_S5P_FIMC && USB_XHCI_MTK && RTC_DRV_AT91SAM9 && LPC18XX_DMAMUX && VIDEO_OMAP4 && COMMON_CLK_GEMINI && COMMON_CLK_ASPEED && COMMON_CLK_NXP && COMMON_CLK_OXNAS && COMMON_CLK_BOSTON && QCOM_ADSP_PIL && QCOM_Q6V5_PIL && QCOM_GSBI && ATMEL_EBI && ST_IRQCHIP && RESET_IMX7 && PHY_HI6220_USB && PHY_RALINK_USB && PHY_ROCKCHIP_PCIE && PHY_DA8XX_USB) selects MFD_SYSCON which has unmet direct dependencies (HAS_IOMEM) warning: (PINCTRL_AT91 && PINCTRL_AT91PIO4 && PINCTRL_OXNAS && PINCTRL_PISTACHIO && PINCTRL_PIC32 && PINCTRL_MESON && PINCTRL_NOMADIK && PINCTRL_MTK && PINCTRL_MT7622 && GPIO_TB10X) selects OF_GPIO which has unmet direct dependencies (GPIOLIB && OF && HAS_IOMEM) warning: (FAULT_INJECTION_STACKTRACE_FILTER && LATENCYTOP && LOCKDEP) selects FRAME_POINTER which has unmet direct dependencies (DEBUG_KERNEL && (CRIS || M68K || FRV || UML || SUPERH || BLACKFIN || MN10300 || METAG) || ARCH_WANT_FRAME_POINTERS) will be turned into: $ make ARCH=score allyesconfig scripts/kconfig/conf --allyesconfig Kconfig WARNING: unmet direct dependencies detected for MFD_SYSCON Depends on [n]: HAS_IOMEM [=n] Selected by [y]: - PINCTRL_STM32 [=y] && PINCTRL [=y] && (ARCH_STM32 || COMPILE_TEST [=y]) && OF [=y] - RTC_DRV_AT91SAM9 [=y] && RTC_CLASS [=y] && (ARCH_AT91 || COMPILE_TEST [=y]) - RESET_IMX7 [=y] && RESET_CONTROLLER [=y] - PHY_HI6220_USB [=y] && (ARCH_HISI && ARM64 || COMPILE_TEST [=y]) - PHY_RALINK_USB [=y] && (RALINK || COMPILE_TEST [=y]) - PHY_ROCKCHIP_PCIE [=y] && (ARCH_ROCKCHIP && OF [=y] || COMPILE_TEST [=y]) WARNING: unmet direct dependencies detected for OF_GPIO Depends on [n]: GPIOLIB [=y] && OF [=y] && HAS_IOMEM [=n] Selected by [y]: - PINCTRL_MTK [=y] && PINCTRL [=y] && (ARCH_MEDIATEK || COMPILE_TEST [=y]) && OF [=y] - PINCTRL_MT7622 [=y] && PINCTRL [=y] && (ARCH_MEDIATEK || COMPILE_TEST [=y]) && OF [=y] && (ARM64 || COMPILE_TEST [=y]) WARNING: unmet direct dependencies detected for FRAME_POINTER Depends on [n]: DEBUG_KERNEL [=y] && (CRIS || M68K || FRV || UML || SUPERH || BLACKFIN || MN10300 || METAG) || ARCH_WANT_FRAME_POINTERS [=n] Selected by [y]: - LATENCYTOP [=y] && DEBUG_KERNEL [=y] && STACKTRACE_SUPPORT [=y] && PROC_FS [=y] && !MIPS && !PPC && !S390 && !MICROBLAZE && !ARM_UNWIND && !ARC && !X86 Signed-off-by: Masahiro Yamada Reviewed-by: Petr Vorel --- scripts/kconfig/expr.c | 42 ------------------------------------------ scripts/kconfig/expr.h | 1 - scripts/kconfig/symbol.c | 35 +++++++++++++++++++++++------------ 3 files changed, 23 insertions(+), 55 deletions(-) (limited to 'scripts/kconfig/expr.c') diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c index 49376e12fa30..e1a39e90841d 100644 --- a/scripts/kconfig/expr.c +++ b/scripts/kconfig/expr.c @@ -1137,48 +1137,6 @@ static int expr_compare_type(enum expr_type t1, enum expr_type t2) return 0; } -static inline struct expr * -expr_get_leftmost_symbol(const struct expr *e) -{ - - if (e == NULL) - return NULL; - - while (e->type != E_SYMBOL) - e = e->left.expr; - - return expr_copy(e); -} - -/* - * Given expression `e1' and `e2', returns the leaf of the longest - * sub-expression of `e1' not containing 'e2. - */ -struct expr *expr_simplify_unmet_dep(struct expr *e1, struct expr *e2) -{ - struct expr *ret; - - switch (e1->type) { - case E_OR: - return expr_alloc_and( - expr_simplify_unmet_dep(e1->left.expr, e2), - expr_simplify_unmet_dep(e1->right.expr, e2)); - case E_AND: { - struct expr *e; - e = expr_alloc_and(expr_copy(e1), expr_copy(e2)); - e = expr_eliminate_dups(e); - ret = (!expr_eq(e, e1)) ? e1 : NULL; - expr_free(e); - break; - } - default: - ret = e1; - break; - } - - return expr_get_leftmost_symbol(ret); -} - void expr_print(struct expr *e, void (*fn)(void *, struct symbol *, const char *), void *data, int prevtoken) diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h index 8dbf2a4cdae1..94a383b21df6 100644 --- a/scripts/kconfig/expr.h +++ b/scripts/kconfig/expr.h @@ -305,7 +305,6 @@ struct expr *expr_transform(struct expr *e); int expr_contains_symbol(struct expr *dep, struct symbol *sym); bool expr_depends_symbol(struct expr *dep, struct symbol *sym); struct expr *expr_trans_compare(struct expr *e, enum expr_type type, struct symbol *sym); -struct expr *expr_simplify_unmet_dep(struct expr *e1, struct expr *e2); void expr_fprint(struct expr *e, FILE *out); struct gstr; /* forward */ diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c index 6acf536c9726..f0b2e3b3102d 100644 --- a/scripts/kconfig/symbol.c +++ b/scripts/kconfig/symbol.c @@ -333,6 +333,27 @@ static struct symbol *sym_calc_choice(struct symbol *sym) return def_sym; } +static void sym_warn_unmet_dep(struct symbol *sym) +{ + struct gstr gs = str_new(); + + str_printf(&gs, + "\nWARNING: unmet direct dependencies detected for %s\n", + sym->name); + str_printf(&gs, + " Depends on [%c]: ", + sym->dir_dep.tri == mod ? 'm' : 'n'); + expr_gstr_print(sym->dir_dep.expr, &gs); + str_printf(&gs, "\n"); + + expr_gstr_print_revdep(sym->rev_dep.expr, &gs, yes, + " Selected by [y]:\n"); + expr_gstr_print_revdep(sym->rev_dep.expr, &gs, mod, + " Selected by [m]:\n"); + + fputs(str_get(&gs), stderr); +} + void sym_calc_value(struct symbol *sym) { struct symbol_value newval, oldval; @@ -414,18 +435,8 @@ void sym_calc_value(struct symbol *sym) } } calc_newval: - if (sym->dir_dep.tri < sym->rev_dep.tri) { - struct expr *e; - e = expr_simplify_unmet_dep(sym->rev_dep.expr, - sym->dir_dep.expr); - fprintf(stderr, "warning: ("); - expr_fprint(e, stderr); - fprintf(stderr, ") selects %s which has unmet direct dependencies (", - sym->name); - expr_fprint(sym->dir_dep.expr, stderr); - fprintf(stderr, ")\n"); - expr_free(e); - } + if (sym->dir_dep.tri < sym->rev_dep.tri) + sym_warn_unmet_dep(sym); newval.tri = EXPR_OR(newval.tri, sym->rev_dep.tri); } if (newval.tri == mod && -- cgit v1.2.3