On Thu, Apr 18, 2024 at 7:19 PM <sunying@xxxxxxxxxxxxxxxx> wrote:
>
> From: Ying Sun <sunying@xxxxxxxxxxxxxxxx>
>
> The patch enhances kernel error messages, fixes problems with
> the previous version of v3,
Unneeded information.
You do not need to advertise your previous wrong patch.
The patch submission history should be
described below the '---' marker.
> and has been thoroughly tested.
Unneeded.
It is quite normal for a patch submitter
to test their patch before submission.
> We believe it will improve the clarity and usefulness
> of kconfig error messages, which will help developers better diagnose and
> resolve configuration issues.
"We believe" is unneeded.
>
> ----- v3 -> v4:
> 1. Fixed the dependency logic print error, distinguishing
> between unsatisfied dependencies and forced enable
> errors (related to the select keyword).
> 2. Add export KCONFIG_WARN_CHANGED_INPUT=1 to scripts/kconfig/Makefile,
> which can be enabled by setting KBUILD_EXTRA_WARN to -c.
>
> Signed-off-by: Siyuan Guo <zy21df106@xxxxxxxxxxx>
> Signed-off-by: Ying Sun <sunying@xxxxxxxxxxxxxxxx>
> ---
> scripts/kconfig/Makefile | 1 +
> scripts/kconfig/confdata.c | 60 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 61 insertions(+)
>
> diff --git a/scripts/kconfig/Makefile b/scripts/kconfig/Makefile
> index ea1bf3b3dbde..b755246fe057 100644
> --- a/scripts/kconfig/Makefile
> +++ b/scripts/kconfig/Makefile
> @@ -29,6 +29,7 @@ KCONFIG_DEFCONFIG_LIST += arch/$(SRCARCH)/configs/$(KBUILD_DEFCONFIG)
>
> ifneq ($(findstring c, $(KBUILD_EXTRA_WARN)),)
> export KCONFIG_WARN_UNKNOWN_SYMBOLS=1
> +export KCONFIG_WARN_CHANGED_INPUT=1
> endif
>
> ifneq ($(findstring e, $(KBUILD_EXTRA_WARN)),)
> diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
> index 0e35c4819cf1..91c63bd1cedd 100644
> --- a/scripts/kconfig/confdata.c
> +++ b/scripts/kconfig/confdata.c
> @@ -176,6 +176,41 @@ static void conf_warning(const char *fmt, ...)
> conf_warnings++;
> }
>
> +static void conf_warn_unmet_rev(struct symbol *sym)
> +{
> + struct gstr gs = str_new();
> +
> + str_printf(&gs,
> + "WARNING: unmet reverse dependencies detected for %s\n",
> + sym->name);
This message is wrong.
Kconfig changed the value so it meets the reverse dependency.
It should warn that the value has been changed.
> +
> + 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);
> + str_free(&gs);
> +}
> +
> +static void conf_warn_dep_error(struct symbol *sym)
> +{
> + struct gstr gs = str_new();
> +
> + str_printf(&gs,
> + "WARNING: unmet direct dependencies detected for %s\n",
> + sym->name);
Same here.
Kconfig changed the value so it meets the direct dependency.
> +
> + 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");
> + fputs(str_get(&gs), stderr);
> + str_free(&gs);
> +}
> +
> static void conf_default_message_callback(const char *s)
> {
> printf("#\n# ");
> @@ -522,6 +557,31 @@ int conf_read(const char *name)
> continue;
> conf_unsaved++;
> /* maybe print value in verbose mode... */
> + if (getenv("KCONFIG_WARN_CHANGED_INPUT")) {
> + if (sym->prop) {
Why did you check sym->prop here?
> + switch (sym->type) {
> + case S_BOOLEAN:
> + case S_TRISTATE:
> + if (sym->def[S_DEF_USER].tri != sym_get_tristate_value(sym)) {
> + if (sym->rev_dep.tri < sym->def[S_DEF_USER].tri &&
> + sym->dir_dep.tri < sym->def[S_DEF_USER].tri)
> + conf_warn_dep_error(sym);
> + if (sym->rev_dep.tri > sym->def[S_DEF_USER].tri &&
> + sym->dir_dep.tri >= sym->def[S_DEF_USER].tri)
> + conf_warn_unmet_rev(sym);
This is clumsy.
conf_warn_dep_error() and conf_warn_unmet_rev()
do not happen at the same time.
if (sym->def[S_DEF_USER].tri != sym_get_tristate_value(sym)) {
if (sym->rev_dep.tri > sym->def[S_DEF_USER].tri)
conf_warn_unmet_rev(sym);
else if (sym->dir_dep.tri < sym->def[S_DEF_USER].tri)
conf_warn_dep_error(sym);
}
is much simpler.
> + }
> + break;
> + case S_INT:
> + case S_HEX:
> + case S_STRING:
> + if (sym->dir_dep.tri == no && sym_has_value(sym))
> + conf_warn_dep_error(sym);
> + break;
> + default:
> + break;
> + }
> + }
> + }
> }
>
> for_all_symbols(sym) {
> --
> 2.43.0
>
>
--
Best Regards
Masahiro Yamada
