Received-SPF: pass (google.com: domain of linux-kernel+bounces-60576-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240102020405.32701-1-sunying@isrc.iscas.ac.cn>
In-Reply-To: <20240102020405.32701-1-sunying@isrc.iscas.ac.cn>
From: Masahiro Yamada <masahiroy@kernel.org>
Date: Sun, 11 Feb 2024 08:24:50 +0900
Message-ID: <CAK7LNATCtoCEs4-S6kgGCH8XRkX+sKT3zuP-awdFJr+E1bkZww@mail.gmail.com>
Subject: Re: [PATCHv3 next] kconfig: add dependency warning print about
 invalid values in verbose mode
To: sunying@isrc.iscas.ac.cn
Cc: linux-kbuild@vger.kernel.org, linux-kernel@vger.kernel.org, 
	pengpeng@iscas.ac.cn, Siyuan Guo <zy21df106@buaa.edu.cn>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

"in verbose mode" is stale.

Please rephrase the subject.


On Tue, Jan 2, 2024 at 11:11=E2=80=AFAM <sunying@isrc.iscas.ac.cn> wrote:
>
> From: Ying Sun <sunying@isrc.iscas.ac.cn>
>
> Warning in verbose mode about incorrect causes and
>  mismatch dependency of invalid values to help users correct them.

Same here, "verbose mode" does not exist in this patch.


>
> Detailed warning messages are printed only when the environment variable
>  is set like "KCONFIG_WARN_CHANGED_INPUT=3D1".
> By default, the current behavior is not changed.
>
> Signed-off-by: Siyuan Guo <zy21df106@buaa.edu.cn>
> Signed-off-by: Ying Sun <sunying@isrc.iscas.ac.cn>
> ---
> v2 -> v3:
> * Fixed warning message that mess up the ncurses display.
> * Fixed memory leak where str_new() was called but str_free() was not use=
d.
> * Integrated the code to avoid excessive dispersion.
> * Use the environment variable KCONFIG_WARN_CHANGED_INPUT as the switch


This checker prints wrong reports.


I just attached one test case.


[test Kconfig]

config MODULES
       def_bool y
       modules

config FOO
       tristate "foo"
       depends on BAR

config BAR
       tristate "bar"

config BAZ
       tristate "baz"
       select FOO


[test .config]

CONFIG_FOO=3Dm
# CONFIG_BAR is not set
CONFIG_BAZ=3Dy


[test command]


$ KCONFIG_WARN_CHANGED_INPUT=3D1 make  olddefconfig


[result]


Kconfig:8:warning: 'MODULES' set to y due to option constraints


WARNING: unmet direct dependencies detected for FOO
  Depends on [n]: BAR [=3Dn]
  Selected by [y]:
  - BAZ [=3Dy]
Kconfig:12:warning: 'FOO' set to n for it unmet direct dependencies
 Depends on [n]: BAR [=3Dn]


$ cat .config
#
# Automatically generated file; DO NOT EDIT.
# Linux/x86 6.8.0-rc3 Kernel Configuration
#
CONFIG_MODULES=3Dy
CONFIG_FOO=3Dy
# CONFIG_BAR is not set
CONFIG_BAZ=3Dy


The first report

  Kconfig:8:warning: 'MODULES' set to y due to option constraints

should not be printed.

CONFIG options without prompt can be omitted.


The second report

  Kconfig:12:warning: 'FOO' set to n for it unmet direct dependencies

is completely wrong.


CONFIG_FOO was changed to 'y' due to the select.


One more thing,

Add
export KCONFIG_WARN_CHANGED_INPUT=3D1

to scripts/kconfig/Makefile


> v1 -> v2:
> * Reduced the number of code lines by refactoring and simplifying the log=
ic.
> * Changed the print "ERROR" to "WARNING".
> * Focused on handling dependency errors: dir_dep and rev_dep, and range e=
rror.
>   - A downgrade from 'y' to 'm' has be warned.
>   - A new CONFIG option should not be warned.
>   - Overwriting caused by default value is not an error and is no longer =
printed.
> * Fixed style issues.
> ---
> ---
>  scripts/kconfig/confdata.c | 76 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
>
> diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
> index f1197e672431..352774928558 100644
> --- a/scripts/kconfig/confdata.c
> +++ b/scripts/kconfig/confdata.c
> @@ -195,6 +195,52 @@ static void conf_message(const char *fmt, ...)
>         va_end(ap);
>  }
>
> +static void conf_warn_unmet_rev_dep(struct symbol *sym)
> +{
> +       struct gstr gs =3D str_new();
> +       char value =3D 'n';
> +
> +       switch (sym->curr.tri) {
> +       case no:
> +               value =3D 'n';
> +               break;
> +       case mod:
> +               sym_calc_value(modules_sym);
> +               value =3D (modules_sym->curr.tri =3D=3D no) ? 'n' : 'm';
> +               break;
> +       case yes:
> +               value =3D 'y';
> +       }
> +
> +       str_printf(&gs,
> +               "'%s' set to %c for it is selected\n",
> +               sym->name, value);
> +       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");
> +
> +       conf_warning("%s", str_get(&gs));
> +       str_free(&gs);
> +}
> +
> +static void conf_warn_dep_error(struct symbol *sym)
> +{
> +       struct gstr gs =3D str_new();
> +
> +       str_printf(&gs,
> +               "'%s' set to n for it unmet direct dependencies\n",
> +               sym->name);
> +
> +       str_printf(&gs,
> +               " Depends on [%c]: ",
> +               sym->dir_dep.tri =3D=3D mod ? 'm' : 'n');
> +       expr_gstr_print(sym->dir_dep.expr, &gs);
> +
> +       conf_warning("%s\n", str_get(&gs));
> +       str_free(&gs);
> +}
> +
>  const char *conf_get_configname(void)
>  {
>         char *name =3D getenv("KCONFIG_CONFIG");
> @@ -568,6 +614,36 @@ int conf_read(const char *name)
>                         continue;
>                 conf_unsaved++;
>                 /* maybe print value in verbose mode... */
> +               if (getenv("KCONFIG_WARN_CHANGED_INPUT") && (sym->prop)) =
{
> +                       conf_filename =3D sym->prop->file->name;
> +                       conf_lineno =3D sym->prop->menu->lineno;
> +
> +                       switch (sym->type) {
> +                       case S_BOOLEAN:
> +                       case S_TRISTATE:
> +                               if (sym->def[S_DEF_USER].tri !=3D sym_get=
_tristate_value(sym)) {
> +                                       if (sym->dir_dep.tri < sym->def[S=
_DEF_USER].tri)
> +                                               conf_warn_dep_error(sym);
> +                                       else if (sym->rev_dep.tri > sym->=
def[S_DEF_USER].tri)
> +                                               conf_warn_unmet_rev_dep(s=
ym);
> +                                       else
> +                                               conf_warning("'%s' set to=
 %s due to option constraints\n",
> +                                                       sym->name, sym_ge=
t_string_value(sym));
> +                               }
> +                               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);
> +                               else
> +                                       conf_warning("'%s' set to %s due =
to option constraints\n",
> +                                                       sym->name, sym_ge=
t_string_value(sym));
> +                               break;
> +                       default:
> +                               break;
> +                       }
> +               }
>         }
>
>         for_all_symbols(i, sym) {
> --
> 2.43.0
>
>


--
Best Regards


Masahiro Yamada