Re: Re: [PATCH] kconfig: add dependency warning print about invalid values in verbose mode

[郭思远]

I apologize for this, but I cannot reproduce this issue in my environment. 

I have tried removing CONFIG_SMP or changing it to n in the 
arch/x86/configs/x86_64_defconfig file,and none of them resulted in segfault. 
I am using the latest Linux kernel version and compilation toolchain.

In order to better locate and solve this issue, could you provide more information？


> On Sun, Aug 20, 2023 at 10:40AM Jesse T <mr.bossman075@xxxxxxxxx>wrote:
> 
> On Sat, Aug 19, 2023 at 8:59 PM Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote:
> >
> > On Wed, Aug 9, 2023 at 9:32 AM <sunying@xxxxxxxxxxxxxx> wrote:
> > >
> > > From: Ying Sun <sunying@xxxxxxxxxxxxxx>
> > >
> > > Add warning about the configuration option's invalid value in verbose mode,
> > >  including error causes, mismatch dependency, old and new values,
> > >  to help users correct them.
> 
> It should also warn about invalid options which were being talked
> about in other places.
> https://lore.kernel.org/all/20230817012007.131868-1-senozhatsky@xxxxxxxxxxxx/T/#u
> https://lore.kernel.org/lkml/CAMuHMdWF0LseSGK6=aodXaoK9D16mxok4DDRs=gKoGox8k6zjg@xxxxxxxxxxxxxx/T/#mb5b73532166014ce0a66b0e7e11dbe06528d863c
> 
> I can also cause a segfault by removing options such as removing
> `CONFIG_SMP=y`  from x86_64_defconfig
> 
> > >
> > > Detailed error messages are printed only when the environment variable
> > >  is set like "KCONFIG_VERBOSE=1".
> > > By default, the current behavior is not changed.
> > >
> > > Signed-off-by: Siyuan Guo <zy21df106@xxxxxxxxxxx>
> > > Signed-off-by: Ying Sun <sunying@xxxxxxxxxxxxxx>
> >
> >
> > This patch is much bigger than I had expected.
> >
> > I did not go through all the lines, but
> > I think the behavior should be improved.
> >
> >
> >
> > [1] A downgrade from 'y' to 'm' should be warned.
> >
> >
> > You only check (sym->dir_dep.tri == no && sym->def[S_DEF_USER].tri != no)
> >
> > but (sym->dir_dep.tri == mod && sym->def[S_DEF_USER].tri == yes)
> > will cause =y to be downgraded to =m.
> > This must be warned.
> >
> >
> >
> >
> > [2] A new CONFIG option should not be warned.
> >
> > $ make defconfig
> >
> > Add the following two lines to a Kconfig file.
> >
> > config THIS_IS_A_NEW_OPTION
> >        def_bool y
> >
> >
> > $ make KCONFIG_VERBOSE=1 oldconfig
> >
> > ERROR : THIS_IS_A_NEW_OPTION[n => y] value is invalid
> >  due to it has default value
> > #
> > # configuration written to .config
> > #
> >
> >
> >
> >
> > This is totally harmless.
> > I do not understand why it is warned.
> >
> >
> >
> >
> >
> > BTW, your patch subject says
> > "add dependency warning", but your actually code prints "ERROR".
> >
> > Also, check the coding style.
> >
> >
> >
> >
> >
> >
> >
> > > ---
> > >  scripts/kconfig/confdata.c | 121 +++++++++++++++++++++++++++++++++++--
> > >  scripts/kconfig/lkc.h      |   3 +
> > >  scripts/kconfig/symbol.c   |  82 +++++++++++++++++++++++--
> > >  3 files changed, 195 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
> > > index 992575f1e976..fa2ae6f63352 100644
> > > --- a/scripts/kconfig/confdata.c
> > > +++ b/scripts/kconfig/confdata.c
> > > @@ -154,6 +154,7 @@ static void conf_message(const char *fmt, ...)
> > >
> > >  static const char *conf_filename;
> > >  static int conf_lineno, conf_warnings;
> > > +const char *verbose;
> > >
> > >  static void conf_warning(const char *fmt, ...)
> > >  {
> > > @@ -226,7 +227,7 @@ static const char *conf_get_rustccfg_name(void)
> > >  static int conf_set_sym_val(struct symbol *sym, int def, int def_flags, char *p)
> > >  {
> > >         char *p2;
> > > -
> > > +       static const char * const type[] = {"unknown", "bool", "tristate", "int", "hex", "string"};
> 
> Add a new line after the declaration.
> Not sure why checkpatch didn't get this.
> 
> > >         switch (sym->type) {
> > >         case S_TRISTATE:
> > >                 if (p[0] == 'm') {
> > > @@ -246,9 +247,14 @@ static int conf_set_sym_val(struct symbol *sym, int def, int def_flags, char *p)
> > >                         sym->flags |= def_flags;
> > >                         break;
> > >                 }
> > > -               if (def != S_DEF_AUTO)
> > > -                       conf_warning("symbol value '%s' invalid for %s",
> > > +               if (def != S_DEF_AUTO) {
> > > +                       if (verbose)
> > > +                               conf_warning("symbol value '%s' invalid for %s\n due to its type is %s",
> > > +                                    p, sym->name, type[sym->type]);
> 
> normally indented to the opening parenthesis, same with the rest.
> 
> > > +                       else
> > > +                               conf_warning("symbol value '%s' invalid for %s",
> > >                                      p, sym->name);
> > > +               }
> > >                 return 1;
> > >         case S_STRING:
> > >                 /* No escaping for S_DEF_AUTO (include/config/auto.conf) */
> > > @@ -274,9 +280,14 @@ static int conf_set_sym_val(struct symbol *sym, int def, int def_flags, char *p)
> > >                         sym->def[def].val = xstrdup(p);
> > >                         sym->flags |= def_flags;
> > >                 } else {
> > > -                       if (def != S_DEF_AUTO)
> > > -                               conf_warning("symbol value '%s' invalid for %s",
> > > -                                            p, sym->name);
> > > +                       if (def != S_DEF_AUTO) {
> > > +                               if (verbose)
> > > +                                       conf_warning("symbol value '%s' invalid for %s\n due to its type is %s",
> > > +                                               p, sym->name, type[sym->type]);
> > > +                               else
> > > +                                       conf_warning("symbol value '%s' invalid for %s",
> > > +                                               p, sym->name);
> > > +                       }
> > >                         return 1;
> > >                 }
> > >                 break;
> > > @@ -528,6 +539,7 @@ int conf_read(const char *name)
> > >         int conf_unsaved = 0;
> > >         int i;
> > >
> > > +       verbose = getenv("KCONFIG_VERBOSE");
> > >         conf_set_changed(false);
> > >
> > >         if (conf_read_simple(name, S_DEF_USER)) {
> > > @@ -559,6 +571,103 @@ int conf_read(const char *name)
> > >                         continue;
> > >                 conf_unsaved++;
> > >                 /* maybe print value in verbose mode... */
> > > +               if (verbose) {
> > > +                       if (sym_is_choice_value(sym)) {
> > > +                               struct property *prop = sym_get_choice_prop(sym);
> > > +                               struct symbol *defsym = prop_get_symbol(prop)->curr.val;
> > > +
> > > +                               if (defsym && defsym != sym) {
> > > +                                       struct gstr gs = str_new();
> > > +
> > > +                                       str_printf(&gs,
> > > +                                               "\nERROR : %s[%c => %c] value is invalid\n",
> > > +                                               sym->name,
> > > +                                               tristate2char[sym->def[S_DEF_USER].tri],
> > > +                                               tristate2char[sym->curr.tri]);
> > > +                                       str_printf(&gs,
> > > +                                               " due to its not the choice default symbol\n");
> > > +                                       str_printf(&gs,
> > > +                                               " the default symbol is %s\n",
> > > +                                               defsym->name);
> > > +                                       fputs(str_get(&gs), stderr);
> > > +                               }
> > > +                       } else {
> > > +                               switch (sym->type) {
> > > +                               case S_BOOLEAN:
> > > +                               case S_TRISTATE:
> > > +                                       if (sym->dir_dep.tri == no &&
> > > +                                               sym->def[S_DEF_USER].tri != no) {
> > > +                                               struct gstr gs = str_new();
> > > +
> > > +                                               str_printf(&gs,
> > > +                                                       "\nERROR: unmet direct dependencies detected for %s[%c => %c]\n",
> > > +                                                       sym->name,
> > > +                                                       tristate2char[sym->def[S_DEF_USER].tri],
> > > +                                                       tristate2char[sym->curr.tri]);
> > > +                                               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);
> > > +                                       } else if (sym->rev_dep.tri != no) {
> > > +                                               struct gstr gs = str_new();
> > > +
> > > +                                               str_printf(&gs,
> > > +                                                       "\nERROR : %s[%c => %c] value is invalid\n",
> > > +                                                       sym->name,
> > > +                                                       tristate2char[sym->def[S_DEF_USER].tri],
> > > +                                                       tristate2char[sym->curr.tri]);
> > > +                                               str_printf(&gs,
> > > +                                                       " due to its invisible and it is selected\n");
> 
> As Masahiro said this is unnecessary.
> Maybe add a verbosity level for this.
> 
> > > +                                               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);
> > > +                                       } else {
> > > +                                               sym_validate_default(sym);
> > > +                                       }
> > > +                                       break;
> > > +                               case S_INT:
> > > +                               case S_HEX:
> > > +                                       if (sym->dir_dep.tri == no &&
> > > +                                       strcmp((char *)(sym->def[S_DEF_USER].val), "") != 0) {
> > > +                                               struct gstr gs = str_new();
> > > +
> > > +                                               str_printf(&gs,
> > > +                                                       "\nERROR: unmet direct dependencies detected for %s\n",
> > > +                                                       sym->name);
> > > +                                               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);
> > > +                                       } else {
> > > +                                               sym_validate_default(sym);
> > > +                                       }
> > > +                                       break;
> > > +                               case S_STRING:
> > > +                                       if (sym->dir_dep.tri == no &&
> > > +                                       strcmp((char *)(sym->def[S_DEF_USER].val), "") != 0) {
> > > +                                               struct gstr gs = str_new();
> > > +
> > > +                                               str_printf(&gs,
> > > +                                                       "\nERROR: unmet direct dependencies detected for %s\n",
> > > +                                                       sym->name);
> > > +                                               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);
> > > +                                       }
> > > +                               default:
> > > +                                       break;
> > > +                               }
> > > +                       }
> > > +               }
> > >         }
> > >
> > >         for_all_symbols(i, sym) {
> > > diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
> > > index 471a59acecec..820a47fb4968 100644
> > > --- a/scripts/kconfig/lkc.h
> > > +++ b/scripts/kconfig/lkc.h
> > > @@ -38,6 +38,8 @@ void zconf_initscan(const char *name);
> > >  void zconf_nextfile(const char *name);
> > >  int zconf_lineno(void);
> > >  const char *zconf_curname(void);
> > > +extern const char *verbose;
> > > +static const char tristate2char[3] = {'n', 'm', 'y'};
> 
> This seems non-ideal
> 
> > >
> > >  /* confdata.c */
> > >  const char *conf_get_configname(void);
> > > @@ -112,6 +114,7 @@ struct property *sym_get_range_prop(struct symbol *sym);
> > >  const char *sym_get_string_default(struct symbol *sym);
> > >  struct symbol *sym_check_deps(struct symbol *sym);
> > >  struct symbol *prop_get_symbol(struct property *prop);
> > > +void sym_validate_default(struct symbol *sym);
> > >
> > >  static inline tristate sym_get_tristate_value(struct symbol *sym)
> > >  {
> > > diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
> > > index 0572330bf8a7..8b11d6ea1d30 100644
> > > --- a/scripts/kconfig/symbol.c
> > > +++ b/scripts/kconfig/symbol.c
> > > @@ -91,6 +91,53 @@ static struct property *sym_get_default_prop(struct symbol *sym)
> > >         return NULL;
> > >  }
> > >
> > > +void sym_validate_default(struct symbol *sym)
> > > +{
> > > +       if (sym->visible == no) {
> > > +               struct gstr gs = str_new();
> > > +               const char *value = sym_get_string_default(sym);
> > > +
> > > +               switch (sym->type) {
> > > +               case S_BOOLEAN:
> > > +               case S_TRISTATE:
> > > +                       if (strcmp(value, "n") != 0) {
> > > +                               str_printf(&gs,
> > > +                                       "\nERROR : %s[%c => %c] value is invalid\n due to it has default value\n",
> > > +                                       sym->name,
> Same here
> > > +                                       tristate2char[sym->def[S_DEF_USER].tri],
> > > +                                       tristate2char[sym->curr.tri]);
> > > +                       } else if (sym->implied.tri != no) {
> > > +                               str_printf(&gs,
> > > +                                       "\nERROR : %s[%c => %c] value is invalid\n due to its invisible and has imply value\n",
> > > +                                       sym->name,
> And here
> > > +                                       tristate2char[sym->def[S_DEF_USER].tri],
> > > +                                       tristate2char[sym->curr.tri]);
> > > +                               str_printf(&gs,
> > > +                                       " Imply : ");
> > > +                               expr_gstr_print(sym->implied.expr, &gs);
> > > +                               str_printf(&gs, "\n");
> > > +                       }
> > > +                       break;
> > > +               case S_STRING:
> > > +               case S_INT:
> > > +               case S_HEX:
> > > +                       if (strcmp(value, "") != 0) {
> > > +                               str_printf(&gs,
> > > +                                       "\nERROR : %s[%s => %s] value is invalid\n",
> > > +                                       sym->name,
> > > +                                       (char *)(sym->def[S_DEF_USER].val),
> > > +                                       (char *)(sym->curr.val));
> > > +                               str_printf(&gs,
> > > +                                       " due to it has default value\n");
> > > +                       }
> > > +                       break;
> > > +               default:
> > > +                       break;
> > > +               }
> > > +               fputs(str_get(&gs), stderr);
> > > +       }
> > > +}
> > > +
> > >  struct property *sym_get_range_prop(struct symbol *sym)
> > >  {
> > >         struct property *prop;
> > > @@ -600,7 +647,8 @@ bool sym_string_valid(struct symbol *sym, const char *str)
> > >  bool sym_string_within_range(struct symbol *sym, const char *str)
> > >  {
> > >         struct property *prop;
> > > -       long long val;
> > > +       long long val, left, right;
> > > +       struct gstr gs = str_new();
> > >
> > >         switch (sym->type) {
> > >         case S_STRING:
> > > @@ -612,8 +660,20 @@ bool sym_string_within_range(struct symbol *sym, const char *str)
> > >                 if (!prop)
> > >                         return true;
> > >                 val = strtoll(str, NULL, 10);
> > > -               return val >= sym_get_range_val(prop->expr->left.sym, 10) &&
> > > -                      val <= sym_get_range_val(prop->expr->right.sym, 10);
> > > +               left = sym_get_range_val(prop->expr->left.sym, 10);
> > > +               right = sym_get_range_val(prop->expr->right.sym, 10);
> > > +               if (val >= left && val <= right)
> > > +                       return true;
> > > +               if (verbose) {
> > > +                       str_printf(&gs,
> > > +                               "\nERROR: unmet range detected for %s\n",
> > > +                               sym->name);
> > > +                       str_printf(&gs,
> > > +                               " symbol value is %lld, the range is (%lld %lld)\n",
> > > +                               val, left, right);
> > > +                       fputs(str_get(&gs), stderr);
> > > +               }
> > > +               return false;
> > >         case S_HEX:
> > >                 if (!sym_string_valid(sym, str))
> > >                         return false;
> > > @@ -621,8 +681,20 @@ bool sym_string_within_range(struct symbol *sym, const char *str)
> > >                 if (!prop)
> > >                         return true;
> > >                 val = strtoll(str, NULL, 16);
> > > -               return val >= sym_get_range_val(prop->expr->left.sym, 16) &&
> > > -                      val <= sym_get_range_val(prop->expr->right.sym, 16);
> > > +               left = sym_get_range_val(prop->expr->left.sym, 16);
> > > +               right = sym_get_range_val(prop->expr->right.sym, 16);
> > > +               if (val >= left && val <= right)
> > > +                       return true;
> > > +               if (verbose) {
> > > +                       str_printf(&gs,
> > > +                               "\nERROR: unmet range detected for %s\n",
> > > +                               sym->name);
> > > +                       str_printf(&gs,
> > > +                               " symbol value is 0x%llx, the range is (0x%llx 0x%llx)\n",
> > > +                               val, left, right);
> > > +                       fputs(str_get(&gs), stderr);
> > > +               }
> > > +               return false;
> > >         case S_BOOLEAN:
> > >         case S_TRISTATE:
> > >                 switch (str[0]) {
> > > --
> > > 2.17.1
> > >
> >
> >
> > --
> > Best Regards
> > Masahiro Yamada