* [PATCH v4 2/2] kconfig: make unmet dependency warnings readable
2018-03-13 9:56 [PATCH v4 1/2] kconfig: warn unmet direct dependency of tristate symbols selected by y Masahiro Yamada
@ 2018-03-13 9:56 ` Masahiro Yamada
2018-03-18 22:17 ` [PATCH v4 1/2] kconfig: warn unmet direct dependency of tristate symbols selected by y Eugeniu Rosca
1 sibling, 0 replies; 4+ messages in thread
From: Masahiro Yamada @ 2018-03-13 9:56 UTC (permalink / raw)
To: linux-kbuild
Cc: Sam Ravnborg, Ulf Magnusson, Eugeniu Rosca, Randy Dunlap,
Luis R . Rodriguez, Petr Vorel, Masahiro Yamada, linux-kernel
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 <yamada.masahiro@socionext.com>
Reviewed-by: Petr Vorel <petr.vorel@gmail.com>
---
Changes in v4:
- Rename sym_warn_unmet_dependency() to sym_warn_unmet_dep()
- Shift "Depends on:" and "Selected by" to the right by one space
- Display value of "Depends on". This can be '[m]' or '[n]'.
Changes in v3:
- Remove unused expr_get_leftmost_symbol()
- Move error message to the same line as 'WARNING'
- update test case
Changes in v2:
- update test case
scripts/kconfig/expr.c | 42 ------------------------------------------
scripts/kconfig/expr.h | 1 -
scripts/kconfig/symbol.c | 35 +++++++++++++++++++++++------------
3 files changed, 23 insertions(+), 55 deletions(-)
diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c
index 49376e1..e1a39e9 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 8dbf2a4..94a383b 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 def860b..9e7d742 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 &&
--
2.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v4 1/2] kconfig: warn unmet direct dependency of tristate symbols selected by y
2018-03-13 9:56 [PATCH v4 1/2] kconfig: warn unmet direct dependency of tristate symbols selected by y Masahiro Yamada
2018-03-13 9:56 ` [PATCH v4 2/2] kconfig: make unmet dependency warnings readable Masahiro Yamada
@ 2018-03-18 22:17 ` Eugeniu Rosca
2018-03-20 3:57 ` Masahiro Yamada
1 sibling, 1 reply; 4+ messages in thread
From: Eugeniu Rosca @ 2018-03-18 22:17 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-kbuild, Sam Ravnborg, Ulf Magnusson, Eugeniu Rosca,
Randy Dunlap, Luis R . Rodriguez, Petr Vorel, Eugeniu Rosca,
linux-kernel
Hi Masahiro,
On Tue, Mar 13, 2018 at 06:56:07PM +0900, Masahiro Yamada wrote:
> Commit 246cf9c26bf1 ("kbuild: Warn on selecting symbols with unmet
> direct dependencies") forcibly promoted ->dir_dep.tri to yes from mod.
> So, the unmet direct dependencies of tristate symbols are not reported.
>
> [Test Case]
>
> config MODULES
> def_bool y
> option modules
>
> config A
> def_bool y
> select B
>
> config B
> tristate "B"
> depends on m
>
> This causes unmet dependency because 'B' is forced 'y' ignoring
> 'depends on m'. This should be warned.
>
> On the other hand, the following case ('B' is bool) should not be
> warned, so 'depends on m' for bool symbols should be naturally treated
> as 'depends on y'.
>
> [Test Case2 (not unmet dependency)]
>
> config MODULES
> def_bool y
> option modules
>
> config A
> def_bool y
> select B
>
> config B
> bool "B"
> depends on m
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> Changes in v4:
> - newly added
>
>
> scripts/kconfig/symbol.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
> index 0f7eba7..def860b 100644
> --- a/scripts/kconfig/symbol.c
> +++ b/scripts/kconfig/symbol.c
> @@ -243,7 +243,7 @@ static void sym_calc_visibility(struct symbol *sym)
> tri = yes;
> if (sym->dir_dep.expr)
> tri = expr_calc_value(sym->dir_dep.expr);
> - if (tri == mod)
> + if (sym->type == S_BOOLEAN && tri == mod)
This seems to be very close to one of the issues fixed in the old
v2.6.5 commit 5a87d187fce1 ("[PATCH] config: choice fix") accessible at
https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/commit/?h=5a87d187fce1
IMHO you could probably use (not tested!):
+ if (tri == mod && sym_get_type(sym) == S_BOOLEAN)
instead of
+ if (sym->type == S_BOOLEAN && tri == mod)
to be consistent with the rest of sym_calc_visibility().
Also, since after this commit there will be a great amount of symmetry
between how dirdep/revdep/weak-revdep are calculated inside
sym_calc_visibility(), somebody could probably unify the implementations
in future (not subject of this patch).
> tri = yes;
> if (sym->dir_dep.tri != tri) {
> sym->dir_dep.tri = tri;
> @@ -414,7 +414,7 @@ void sym_calc_value(struct symbol *sym)
> }
> }
> calc_newval:
> - if (sym->dir_dep.tri == no && sym->rev_dep.tri != no) {
> + if (sym->dir_dep.tri < sym->rev_dep.tri) {
Looks good.
> struct expr *e;
> e = expr_simplify_unmet_dep(sym->rev_dep.expr,
> sym->dir_dep.expr);
> --
> 2.7.4
>
Best regards,
Eugeniu.
^ permalink raw reply [flat|nested] 4+ messages in thread