Received-SPF: pass (google.com: domain of linux-kernel+bounces-160583-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240418101903.11314-1-sunying@isrc.iscas.ac.cn>
In-Reply-To: <20240418101903.11314-1-sunying@isrc.iscas.ac.cn>
From: Masahiro Yamada <masahiroy@kernel.org>
Date: Sat, 27 Apr 2024 03:49:28 +0900
Message-ID: <CAK7LNATvhUOV-kzVLkT=S=CLBJT8wbirkD1v9C7J=Gg4EBecUQ@mail.gmail.com>
Subject: Re: [PATCHv4 next] kconfig: add dependency warning print about
 invalid values while KBUILD_EXTRA_WARN=c
To: sunying@isrc.iscas.ac.cn
Cc: linux-kbuild@vger.kernel.org, linux-kernel@vger.kernel.org, 
	pengpeng@iscas.ac.cn, zy21df106@buaa.edu.cn
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Apr 18, 2024 at 7:19=E2=80=AFPM <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 an=
d
>  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=3D1 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 +=3D arch/$(SRCARCH)/configs/$(K=
BUILD_DEFCONFIG)
>
>  ifneq ($(findstring c, $(KBUILD_EXTRA_WARN)),)
>  export KCONFIG_WARN_UNKNOWN_SYMBOLS=3D1
> +export KCONFIG_WARN_CHANGED_INPUT=3D1
>  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 =3D 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 =3D 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 =3D=3D 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 !=3D=
 sym_get_tristate_value(sym)) {
> +                                               if (sym->rev_dep.tri < sy=
m->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 > sy=
m->def[S_DEF_USER].tri &&
> +                                                       sym->dir_dep.tri =
>=3D 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 !=3D 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 =3D=3D no &&=
 sym_has_value(sym))
> +                                               conf_warn_dep_error(sym);
> +                                       break;
> +                               default:
> +                                       break;
> +                               }
> +                       }
> +               }
>         }
>
>         for_all_symbols(sym) {
> --
> 2.43.0
>
>


--=20
Best Regards
Masahiro Yamada