linux-kernel - Re: [PATCH v2] kconfig: make unmet dependency warnings readable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20180306164910.tpdhl4e5xzvmwcua@dell5510>
Date:   Tue, 6 Mar 2018 17:49:10 +0100
From:   Petr Vorel <petr.vorel@...il.com>
To:     Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc:     linux-kbuild@...r.kernel.org, Sam Ravnborg <sam@...nborg.org>,
        Michal Marek <michal.lkml@...kovi.net>,
        Ulf Magnusson <ulfalizer@...il.com>,
        Eugeniu Rosca <roscaeugeniu@...il.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] kconfig: make unmet dependency warnings readable

Hi Masahiro,

...
>   $ make allyesconfig
>   scripts/kconfig/conf  --allyesconfig Kconfig
>   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)'.
Good point, make sense, current is really confusing.

> 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 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 && DWMA
> C_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 && LP
> C18XX_DMAMUX && VIDEO_OMAP4 && COMMON_CLK_GEMINI && COMMON_CLK_ASPEED && C
> OMMON_CLK_NXP && COMMON_CLK_OXNAS && COMMON_CLK_BOSTON && ATMEL_ST && QCOM
> _ADSP_PIL && QCOM_Q6V5_PIL && QCOM_GSBI && ATMEL_EBI && ST_IRQCHIP && RESE
> T_IMX7 && PHY_HI6220_USB && PHY_RALINK_USB && PHY_ROCKCHIP_PCIE && PHY_DA8
> XX_USB) selects MFD_SYSCON which has unmet direct dependencies (HAS_IOMEM)
> warning: (PINCTRL_AT91 && PINCTRL_AT91PIO4 && PINCTRL_OXNAS && PINCTRL_PIS
> TACHIO && PINCTRL_PIC32 && PINCTRL_MESON && PINCTRL_NOMADIK && PINCTRL_MTK
>  && PINCTRL_MT7622 && GPIO_TB10X) selects OF_GPIO which has unmet direct d
> ependencies (GPIOLIB && OF && HAS_IOMEM)
> warning: (FAULT_INJECTION_STACKTRACE_FILTER && LATENCYTOP && LOCKDEP) sele
> cts FRAME_POINTER which has unmet direct dependencies (DEBUG_KERNEL && (CR
> IS || M68K || FRV || UML || SUPERH || BLACKFIN || MN10300 || METAG) || ARC
> H_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:
>     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])
>   - ATMEL_ST [=y] && GENERIC_CLOCKEVENTS [=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:
>     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:
>     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

Although I liked the brevity of current form (one liner is nice), fixing false positives
justify it enough. The same look and feel with help and reusing code makes are also pros.

> Signed-off-by: Masahiro Yamada <yamada.masahiro@...ionext.com>
Reviewed-by: Petr Vorel <petr.vorel@...il.com>

...
> +static void sym_warn_unmet_dependency(struct symbol *sym)
> +{
> +	struct gstr gs = str_new();
> +
> +	str_printf(&gs, "\nWARNING:\n");
> +	str_printf(&gs, " Unmet direct dependencies detected for %s\n",

Just one change: keep error on the same line as 'WARNING:':
	str_printf(&gs, "\nWARNING: Unmet direct dependencies detected for %s\n",
It's make easier to grep error and save space vertically.


Kind regards,
Petr