Re: [PATCHv4 next] kconfig: add dependency warning print about invalid values while KBUILD_EXTRA_WARN=c

[Masahiro Yamada <masahiroy@kernel.org>]

On Thu, Apr 18, 2024 at 7:19 PM <sunying@isrc.iscas.ac.cn> wrote:
>
> From: Ying Sun <sunying@isrc.iscas.ac.cn>
>
> 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@buaa.edu.cn>
> Signed-off-by: Ying Sun <sunying@isrc.iscas.ac.cn>
> ---
>  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