| Message ID | 20180223060901.4052-1-ulfalizer@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 23, 2018 at 7:09 AM, Ulf Magnusson <ulfalizer@gmail.com> wrote: > === Background === > > A "# CONFIG_FOO is not set" line is written to .config for visible > bool/tristate symbols with value n. The idea is to remember the user > selection without having to set a Makefile variable (having undefined > Make variables correspond to n is handy when testing them in the > Makefiles). > > Currently, a "# CONFIG_FOO is not set" line is also written to .config > for all bool/tristate symbols that get the value n through a 'default'. > This is inconsistent with how 'select' and 'imply' work, which only > write non-n symbols. It also seems redundant: > > - If the symbol is visible and has value n, then > "# CONFIG_FOO is not set" will always be written anyway. > > - If the symbol is not visible, then "# CONFIG_FOO is not set" has no > effect on it. > > - If the symbol becomes visible later, there shouldn't be any harm in > recalculating the default value at that point. > > === Changes === > > Change sym_calc_value() to only set SYMBOL_WRITE (write to .config) for > non-n defaults. This reduces the size of the x86 .config on my system by > about 1% (due to removed "# CONFIG_FOO is not set" entries). > > One side effect of this change is making 'default n' equivalent to > having no explicit default. That might make it clearer to people that > 'default n' is redundant. > > This change only affects generated .config files and not autoconf.h: > autoconf.h only includes #defines for non-n bool/tristate symbols. > > === Testing === > > The following testing was done with the x86 Kconfigs: > > - .config files generated before and after the change were compared to > verify that the only difference is some '# CONFIG_FOO is not set' > entries disappearing. A couple of these were inspected manually, and > most turned out to be from redundant 'default n/def_bool n' > properties. > > - The generated include/generated/autoconf.h was compared before and > after the change and verified to be identical. > > - As a sanity check, the same modification was done to Kconfiglib. > The Kconfiglib test suite was then run to check for any mismatches > against the output of the C implementation. > > Signed-off-by: Ulf Magnusson <ulfalizer@gmail.com> > --- > scripts/kconfig/symbol.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c > index cca9663be5dd..02eb8b10a83c 100644 > --- a/scripts/kconfig/symbol.c > +++ b/scripts/kconfig/symbol.c > @@ -403,9 +403,10 @@ void sym_calc_value(struct symbol *sym) > if (!sym_is_choice(sym)) { > prop = sym_get_default_prop(sym); > if (prop) { > - sym->flags |= SYMBOL_WRITE; > newval.tri = EXPR_AND(expr_calc_value(prop->expr), > prop->visible.tri); > + if (newval.tri != no) > + sym->flags |= SYMBOL_WRITE; > } > if (sym->implied.tri != no) { > sym->flags |= SYMBOL_WRITE; > -- > 2.14.1 > This stuff gets pretty obscure, so please tell me if you can think of any practical benefits to remembering an n default as a user selection for non-visible symbols (which is all '# CONFIG_FOO is not set' does in practice). I couldn't think of anything. Cheers, Ulf -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 23, 2018 at 7:14 AM, Ulf Magnusson <ulfalizer@gmail.com> wrote: > On Fri, Feb 23, 2018 at 7:09 AM, Ulf Magnusson <ulfalizer@gmail.com> wrote: >> scripts/kconfig/symbol.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c >> index cca9663be5dd..02eb8b10a83c 100644 >> --- a/scripts/kconfig/symbol.c >> +++ b/scripts/kconfig/symbol.c >> @@ -403,9 +403,10 @@ void sym_calc_value(struct symbol *sym) >> if (!sym_is_choice(sym)) { >> prop = sym_get_default_prop(sym); >> if (prop) { >> - sym->flags |= SYMBOL_WRITE; >> newval.tri = EXPR_AND(expr_calc_value(prop->expr), >> prop->visible.tri); >> + if (newval.tri != no) >> + sym->flags |= SYMBOL_WRITE; >> } >> if (sym->implied.tri != no) { >> sym->flags |= SYMBOL_WRITE; >> -- >> 2.14.1 >> > > This stuff gets pretty obscure, so please tell me if you can think of > any practical benefits to remembering an n default as a user selection > for non-visible symbols (which is all '# CONFIG_FOO is not set' does > in practice). I couldn't think of anything. I had to read the patch description twice to see that you are only changing the invisible symbols. This seems fine for any automated interaction with the .config file. The only possible downside I can think of is that it's sometimes easier to see the '# CONFIG_FOO is unset' when using 'grep CONFIG_FOO .config' to see if something was set. On the other hand, I might edit .config and remove that line today, and then be surprised that 'make oldconfig' doesn't ask me about it again (since it's invisible). So no, I don't see a strong reason against it either. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 23, 2018 at 10:59 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Fri, Feb 23, 2018 at 7:14 AM, Ulf Magnusson <ulfalizer@gmail.com> wrote: >> On Fri, Feb 23, 2018 at 7:09 AM, Ulf Magnusson <ulfalizer@gmail.com> wrote: > >>> scripts/kconfig/symbol.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c >>> index cca9663be5dd..02eb8b10a83c 100644 >>> --- a/scripts/kconfig/symbol.c >>> +++ b/scripts/kconfig/symbol.c >>> @@ -403,9 +403,10 @@ void sym_calc_value(struct symbol *sym) >>> if (!sym_is_choice(sym)) { >>> prop = sym_get_default_prop(sym); >>> if (prop) { >>> - sym->flags |= SYMBOL_WRITE; >>> newval.tri = EXPR_AND(expr_calc_value(prop->expr), >>> prop->visible.tri); >>> + if (newval.tri != no) >>> + sym->flags |= SYMBOL_WRITE; >>> } >>> if (sym->implied.tri != no) { >>> sym->flags |= SYMBOL_WRITE; >>> -- >>> 2.14.1 >>> >> >> This stuff gets pretty obscure, so please tell me if you can think of >> any practical benefits to remembering an n default as a user selection >> for non-visible symbols (which is all '# CONFIG_FOO is not set' does >> in practice). I couldn't think of anything. > > I had to read the patch description twice to see that you are only > changing the invisible symbols. This seems fine for any automated > interaction with the .config file. Yeah... maybe a patch title like "kconfig: only write '# CONFIG_FOO is not set' for visible symbols" would make it clearer (or something to that effect early on in the commit message). > > The only possible downside I can think of is that it's sometimes easier > to see the '# CONFIG_FOO is unset' when using 'grep CONFIG_FOO > .config' to see if something was set. On the other hand, I might > edit .config and remove that line today, and then be surprised that > 'make oldconfig' doesn't ask me about it again (since it's invisible). Note that it only affects symbols that get the value 'n' through an explicit 'default'. You won't get a '# CONFIG_FOO is not set' for n-valued invisible symbols without a 'default' property neither before nor after this patch. > > So no, I don't see a strong reason against it either. > > Arnd Cheers, Ulf -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-02-23 15:14 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: > On Fri, Feb 23, 2018 at 7:09 AM, Ulf Magnusson <ulfalizer@gmail.com> wrote: >> === Background === >> >> A "# CONFIG_FOO is not set" line is written to .config for visible >> bool/tristate symbols with value n. The idea is to remember the user >> selection without having to set a Makefile variable (having undefined >> Make variables correspond to n is handy when testing them in the >> Makefiles). >> >> Currently, a "# CONFIG_FOO is not set" line is also written to .config >> for all bool/tristate symbols that get the value n through a 'default'. >> This is inconsistent with how 'select' and 'imply' work, which only >> write non-n symbols. It also seems redundant: >> >> - If the symbol is visible and has value n, then >> "# CONFIG_FOO is not set" will always be written anyway. >> >> - If the symbol is not visible, then "# CONFIG_FOO is not set" has no >> effect on it. >> >> - If the symbol becomes visible later, there shouldn't be any harm in >> recalculating the default value at that point. >> >> === Changes === >> >> Change sym_calc_value() to only set SYMBOL_WRITE (write to .config) for >> non-n defaults. This reduces the size of the x86 .config on my system by >> about 1% (due to removed "# CONFIG_FOO is not set" entries). >> >> One side effect of this change is making 'default n' equivalent to >> having no explicit default. That might make it clearer to people that >> 'default n' is redundant. >> >> This change only affects generated .config files and not autoconf.h: >> autoconf.h only includes #defines for non-n bool/tristate symbols. >> >> === Testing === >> >> The following testing was done with the x86 Kconfigs: >> >> - .config files generated before and after the change were compared to >> verify that the only difference is some '# CONFIG_FOO is not set' >> entries disappearing. A couple of these were inspected manually, and >> most turned out to be from redundant 'default n/def_bool n' >> properties. >> >> - The generated include/generated/autoconf.h was compared before and >> after the change and verified to be identical. >> >> - As a sanity check, the same modification was done to Kconfiglib. >> The Kconfiglib test suite was then run to check for any mismatches >> against the output of the C implementation. >> >> Signed-off-by: Ulf Magnusson <ulfalizer@gmail.com> >> --- >> scripts/kconfig/symbol.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c >> index cca9663be5dd..02eb8b10a83c 100644 >> --- a/scripts/kconfig/symbol.c >> +++ b/scripts/kconfig/symbol.c >> @@ -403,9 +403,10 @@ void sym_calc_value(struct symbol *sym) >> if (!sym_is_choice(sym)) { >> prop = sym_get_default_prop(sym); >> if (prop) { >> - sym->flags |= SYMBOL_WRITE; >> newval.tri = EXPR_AND(expr_calc_value(prop->expr), >> prop->visible.tri); >> + if (newval.tri != no) >> + sym->flags |= SYMBOL_WRITE; >> } >> if (sym->implied.tri != no) { >> sym->flags |= SYMBOL_WRITE; >> -- >> 2.14.1 >> > > This stuff gets pretty obscure, so please tell me if you can think of > any practical benefits to remembering an n default as a user selection > for non-visible symbols (which is all '# CONFIG_FOO is not set' does > in practice). I couldn't think of anything. > In the context of config CC_HAS_STACKPROTECTOR def_bool $(cc-option -fstack-protector) Currently, we have 3 cases: [1] CONFIG_CC_HAS_STACKPROTECTOR=y -> compiler flag is supported [2] # CONFIG_CC_HAS_STACKPROTECTOR is not set -> compiler flag is unsupported [3] Missing -> The symbol was hidden probably due to unmet "if ... endif" With this change, [2] will be turned into [3]. That is the only drawback I came up with. I am not sure how many people want to check .config to know the compiler capability...
2018-02-23 22:25 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>: > 2018-02-23 15:14 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: >> On Fri, Feb 23, 2018 at 7:09 AM, Ulf Magnusson <ulfalizer@gmail.com> wrote: >>> === Background === >>> >>> A "# CONFIG_FOO is not set" line is written to .config for visible >>> bool/tristate symbols with value n. The idea is to remember the user >>> selection without having to set a Makefile variable (having undefined >>> Make variables correspond to n is handy when testing them in the >>> Makefiles). >>> >>> Currently, a "# CONFIG_FOO is not set" line is also written to .config >>> for all bool/tristate symbols that get the value n through a 'default'. >>> This is inconsistent with how 'select' and 'imply' work, which only >>> write non-n symbols. It also seems redundant: >>> >>> - If the symbol is visible and has value n, then >>> "# CONFIG_FOO is not set" will always be written anyway. >>> >>> - If the symbol is not visible, then "# CONFIG_FOO is not set" has no >>> effect on it. >>> >>> - If the symbol becomes visible later, there shouldn't be any harm in >>> recalculating the default value at that point. >>> >>> === Changes === >>> >>> Change sym_calc_value() to only set SYMBOL_WRITE (write to .config) for >>> non-n defaults. This reduces the size of the x86 .config on my system by >>> about 1% (due to removed "# CONFIG_FOO is not set" entries). >>> >>> One side effect of this change is making 'default n' equivalent to >>> having no explicit default. That might make it clearer to people that >>> 'default n' is redundant. >>> >>> This change only affects generated .config files and not autoconf.h: >>> autoconf.h only includes #defines for non-n bool/tristate symbols. >>> >>> === Testing === >>> >>> The following testing was done with the x86 Kconfigs: >>> >>> - .config files generated before and after the change were compared to >>> verify that the only difference is some '# CONFIG_FOO is not set' >>> entries disappearing. A couple of these were inspected manually, and >>> most turned out to be from redundant 'default n/def_bool n' >>> properties. >>> >>> - The generated include/generated/autoconf.h was compared before and >>> after the change and verified to be identical. >>> >>> - As a sanity check, the same modification was done to Kconfiglib. >>> The Kconfiglib test suite was then run to check for any mismatches >>> against the output of the C implementation. >>> >>> Signed-off-by: Ulf Magnusson <ulfalizer@gmail.com> >>> --- >>> scripts/kconfig/symbol.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c >>> index cca9663be5dd..02eb8b10a83c 100644 >>> --- a/scripts/kconfig/symbol.c >>> +++ b/scripts/kconfig/symbol.c >>> @@ -403,9 +403,10 @@ void sym_calc_value(struct symbol *sym) >>> if (!sym_is_choice(sym)) { >>> prop = sym_get_default_prop(sym); >>> if (prop) { >>> - sym->flags |= SYMBOL_WRITE; >>> newval.tri = EXPR_AND(expr_calc_value(prop->expr), >>> prop->visible.tri); >>> + if (newval.tri != no) >>> + sym->flags |= SYMBOL_WRITE; >>> } >>> if (sym->implied.tri != no) { >>> sym->flags |= SYMBOL_WRITE; >>> -- >>> 2.14.1 >>> >> >> This stuff gets pretty obscure, so please tell me if you can think of >> any practical benefits to remembering an n default as a user selection >> for non-visible symbols (which is all '# CONFIG_FOO is not set' does >> in practice). I couldn't think of anything. >> > > In the context of > > config CC_HAS_STACKPROTECTOR > def_bool $(cc-option -fstack-protector) > > > Currently, we have 3 cases: > > [1] CONFIG_CC_HAS_STACKPROTECTOR=y > -> compiler flag is supported > [2] # CONFIG_CC_HAS_STACKPROTECTOR is not set > -> compiler flag is unsupported > [3] Missing > -> The symbol was hidden probably due to unmet "if ... endif" > > > With this change, [2] will be turned into [3]. > > That is the only drawback I came up with. > > I am not sure how many people want to check .config > to know the compiler capability... > I thought a bit more, then probably the grammatical consistency would win. (default n is always redundant) I want to apply this, but take a bit time in case somebody may have comments. BTW, do you want to check redundant 'default n' by checkpatch.pl ?
On Sat, Feb 24, 2018 at 12:59:51AM +0900, Masahiro Yamada wrote: > 2018-02-23 22:25 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>: > > 2018-02-23 15:14 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: > >> On Fri, Feb 23, 2018 at 7:09 AM, Ulf Magnusson <ulfalizer@gmail.com> wrote: > >>> === Background === > >>> > >>> A "# CONFIG_FOO is not set" line is written to .config for visible > >>> bool/tristate symbols with value n. The idea is to remember the user > >>> selection without having to set a Makefile variable (having undefined > >>> Make variables correspond to n is handy when testing them in the > >>> Makefiles). > >>> > >>> Currently, a "# CONFIG_FOO is not set" line is also written to .config > >>> for all bool/tristate symbols that get the value n through a 'default'. > >>> This is inconsistent with how 'select' and 'imply' work, which only > >>> write non-n symbols. It also seems redundant: > >>> > >>> - If the symbol is visible and has value n, then > >>> "# CONFIG_FOO is not set" will always be written anyway. > >>> > >>> - If the symbol is not visible, then "# CONFIG_FOO is not set" has no > >>> effect on it. > >>> > >>> - If the symbol becomes visible later, there shouldn't be any harm in > >>> recalculating the default value at that point. > >>> > >>> === Changes === > >>> > >>> Change sym_calc_value() to only set SYMBOL_WRITE (write to .config) for > >>> non-n defaults. This reduces the size of the x86 .config on my system by > >>> about 1% (due to removed "# CONFIG_FOO is not set" entries). > >>> > >>> One side effect of this change is making 'default n' equivalent to > >>> having no explicit default. That might make it clearer to people that > >>> 'default n' is redundant. > >>> > >>> This change only affects generated .config files and not autoconf.h: > >>> autoconf.h only includes #defines for non-n bool/tristate symbols. > >>> > >>> === Testing === > >>> > >>> The following testing was done with the x86 Kconfigs: > >>> > >>> - .config files generated before and after the change were compared to > >>> verify that the only difference is some '# CONFIG_FOO is not set' > >>> entries disappearing. A couple of these were inspected manually, and > >>> most turned out to be from redundant 'default n/def_bool n' > >>> properties. > >>> > >>> - The generated include/generated/autoconf.h was compared before and > >>> after the change and verified to be identical. > >>> > >>> - As a sanity check, the same modification was done to Kconfiglib. > >>> The Kconfiglib test suite was then run to check for any mismatches > >>> against the output of the C implementation. > >>> > >>> Signed-off-by: Ulf Magnusson <ulfalizer@gmail.com> > >>> --- > >>> scripts/kconfig/symbol.c | 3 ++- > >>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c > >>> index cca9663be5dd..02eb8b10a83c 100644 > >>> --- a/scripts/kconfig/symbol.c > >>> +++ b/scripts/kconfig/symbol.c > >>> @@ -403,9 +403,10 @@ void sym_calc_value(struct symbol *sym) > >>> if (!sym_is_choice(sym)) { > >>> prop = sym_get_default_prop(sym); > >>> if (prop) { > >>> - sym->flags |= SYMBOL_WRITE; > >>> newval.tri = EXPR_AND(expr_calc_value(prop->expr), > >>> prop->visible.tri); > >>> + if (newval.tri != no) > >>> + sym->flags |= SYMBOL_WRITE; > >>> } > >>> if (sym->implied.tri != no) { > >>> sym->flags |= SYMBOL_WRITE; > >>> -- > >>> 2.14.1 > >>> > >> > >> This stuff gets pretty obscure, so please tell me if you can think of > >> any practical benefits to remembering an n default as a user selection > >> for non-visible symbols (which is all '# CONFIG_FOO is not set' does > >> in practice). I couldn't think of anything. > >> > > > > In the context of > > > > config CC_HAS_STACKPROTECTOR > > def_bool $(cc-option -fstack-protector) > > > > > > Currently, we have 3 cases: > > > > [1] CONFIG_CC_HAS_STACKPROTECTOR=y > > -> compiler flag is supported > > [2] # CONFIG_CC_HAS_STACKPROTECTOR is not set > > -> compiler flag is unsupported > > [3] Missing > > -> The symbol was hidden probably due to unmet "if ... endif" > > > > > > With this change, [2] will be turned into [3]. > > > > That is the only drawback I came up with. > > > > I am not sure how many people want to check .config > > to know the compiler capability... > > > > > I thought a bit more, then probably the grammatical > consistency would win. (default n is always redundant) The behavior should be easy to explain in kconfig-language.txt too: A missing entry means n, except visible n-valued symbols generate a '# CONFIG_FOO is not set' comment just to keep track of the user's choice. No weird exception for 'default'. That would demystify those '...is not set' lines too. > > I want to apply this, but take a bit time > in case somebody may have comments. > > > BTW, do you want to check redundant 'default n' > by checkpatch.pl ? Was thinking of that. Guess you could generate a warning for any of the following: default n default "n" default 'n' Could skip the warning for defaults with conditions maybe, as people sometimes do stuff like default n if <cond> default FOO (Though some of those look like they could refactored as well.) Or you could just say something like this for all of them: warning: check whether 'default n' is redundant -- n is the implicit default value for bool/tristate symbols Cheers, Ulf -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 23, 2018 at 10:33 PM, Ulf Magnusson <ulfalizer@gmail.com> wrote: > On Sat, Feb 24, 2018 at 12:59:51AM +0900, Masahiro Yamada wrote: >> 2018-02-23 22:25 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>: >> > 2018-02-23 15:14 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: >> >> On Fri, Feb 23, 2018 at 7:09 AM, Ulf Magnusson <ulfalizer@gmail.com> wrote: >> >>> === Background === >> >>> >> >>> A "# CONFIG_FOO is not set" line is written to .config for visible >> >>> bool/tristate symbols with value n. The idea is to remember the user >> >>> selection without having to set a Makefile variable (having undefined >> >>> Make variables correspond to n is handy when testing them in the >> >>> Makefiles). >> >>> >> >>> Currently, a "# CONFIG_FOO is not set" line is also written to .config >> >>> for all bool/tristate symbols that get the value n through a 'default'. >> >>> This is inconsistent with how 'select' and 'imply' work, which only >> >>> write non-n symbols. It also seems redundant: >> >>> >> >>> - If the symbol is visible and has value n, then >> >>> "# CONFIG_FOO is not set" will always be written anyway. >> >>> >> >>> - If the symbol is not visible, then "# CONFIG_FOO is not set" has no >> >>> effect on it. >> >>> >> >>> - If the symbol becomes visible later, there shouldn't be any harm in >> >>> recalculating the default value at that point. >> >>> >> >>> === Changes === >> >>> >> >>> Change sym_calc_value() to only set SYMBOL_WRITE (write to .config) for >> >>> non-n defaults. This reduces the size of the x86 .config on my system by >> >>> about 1% (due to removed "# CONFIG_FOO is not set" entries). >> >>> >> >>> One side effect of this change is making 'default n' equivalent to >> >>> having no explicit default. That might make it clearer to people that >> >>> 'default n' is redundant. >> >>> >> >>> This change only affects generated .config files and not autoconf.h: >> >>> autoconf.h only includes #defines for non-n bool/tristate symbols. >> >>> >> >>> === Testing === >> >>> >> >>> The following testing was done with the x86 Kconfigs: >> >>> >> >>> - .config files generated before and after the change were compared to >> >>> verify that the only difference is some '# CONFIG_FOO is not set' >> >>> entries disappearing. A couple of these were inspected manually, and >> >>> most turned out to be from redundant 'default n/def_bool n' >> >>> properties. >> >>> >> >>> - The generated include/generated/autoconf.h was compared before and >> >>> after the change and verified to be identical. >> >>> >> >>> - As a sanity check, the same modification was done to Kconfiglib. >> >>> The Kconfiglib test suite was then run to check for any mismatches >> >>> against the output of the C implementation. >> >>> >> >>> Signed-off-by: Ulf Magnusson <ulfalizer@gmail.com> >> >>> --- >> >>> scripts/kconfig/symbol.c | 3 ++- >> >>> 1 file changed, 2 insertions(+), 1 deletion(-) >> >>> >> >>> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c >> >>> index cca9663be5dd..02eb8b10a83c 100644 >> >>> --- a/scripts/kconfig/symbol.c >> >>> +++ b/scripts/kconfig/symbol.c >> >>> @@ -403,9 +403,10 @@ void sym_calc_value(struct symbol *sym) >> >>> if (!sym_is_choice(sym)) { >> >>> prop = sym_get_default_prop(sym); >> >>> if (prop) { >> >>> - sym->flags |= SYMBOL_WRITE; >> >>> newval.tri = EXPR_AND(expr_calc_value(prop->expr), >> >>> prop->visible.tri); >> >>> + if (newval.tri != no) >> >>> + sym->flags |= SYMBOL_WRITE; >> >>> } >> >>> if (sym->implied.tri != no) { >> >>> sym->flags |= SYMBOL_WRITE; >> >>> -- >> >>> 2.14.1 >> >>> >> >> >> >> This stuff gets pretty obscure, so please tell me if you can think of >> >> any practical benefits to remembering an n default as a user selection >> >> for non-visible symbols (which is all '# CONFIG_FOO is not set' does >> >> in practice). I couldn't think of anything. >> >> >> > >> > In the context of >> > >> > config CC_HAS_STACKPROTECTOR >> > def_bool $(cc-option -fstack-protector) >> > >> > >> > Currently, we have 3 cases: >> > >> > [1] CONFIG_CC_HAS_STACKPROTECTOR=y >> > -> compiler flag is supported >> > [2] # CONFIG_CC_HAS_STACKPROTECTOR is not set >> > -> compiler flag is unsupported >> > [3] Missing >> > -> The symbol was hidden probably due to unmet "if ... endif" >> > >> > >> > With this change, [2] will be turned into [3]. >> > >> > That is the only drawback I came up with. >> > >> > I am not sure how many people want to check .config >> > to know the compiler capability... >> > >> >> >> I thought a bit more, then probably the grammatical >> consistency would win. (default n is always redundant) > > The behavior should be easy to explain in kconfig-language.txt too: A > missing entry means n, except visible n-valued symbols generate a > '# CONFIG_FOO is not set' comment just to keep track of the user's choice. > No weird exception for 'default'. > > That would demystify those '...is not set' lines too. > Looks like someone already did it, see b7d4ec395673 ("Documentation/kbuild: Add guidance for the use of default"). I could add a small note to kconfig-language.txt to explain '...is not set' separately later as well. Cheers, Ulf -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/23/2018 01:33 PM, Ulf Magnusson wrote: > On Sat, Feb 24, 2018 at 12:59:51AM +0900, Masahiro Yamada wrote: >> 2018-02-23 22:25 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>: >>> 2018-02-23 15:14 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: >>>> On Fri, Feb 23, 2018 at 7:09 AM, Ulf Magnusson <ulfalizer@gmail.com> wrote: >>>>> === Background === >>>>> >>>>> A "# CONFIG_FOO is not set" line is written to .config for visible >>>>> bool/tristate symbols with value n. The idea is to remember the user >>>>> selection without having to set a Makefile variable (having undefined >>>>> Make variables correspond to n is handy when testing them in the >>>>> Makefiles). >>>>> >>>>> Currently, a "# CONFIG_FOO is not set" line is also written to .config >>>>> for all bool/tristate symbols that get the value n through a 'default'. >>>>> This is inconsistent with how 'select' and 'imply' work, which only >>>>> write non-n symbols. It also seems redundant: >>>>> >>>>> - If the symbol is visible and has value n, then >>>>> "# CONFIG_FOO is not set" will always be written anyway. >>>>> >>>>> - If the symbol is not visible, then "# CONFIG_FOO is not set" has no >>>>> effect on it. >>>>> >>>>> - If the symbol becomes visible later, there shouldn't be any harm in >>>>> recalculating the default value at that point. >>>>> >>>>> === Changes === >>>>> >>>>> Change sym_calc_value() to only set SYMBOL_WRITE (write to .config) for >>>>> non-n defaults. This reduces the size of the x86 .config on my system by >>>>> about 1% (due to removed "# CONFIG_FOO is not set" entries). >>>>> >>>>> One side effect of this change is making 'default n' equivalent to >>>>> having no explicit default. That might make it clearer to people that >>>>> 'default n' is redundant. >>>>> >>>>> This change only affects generated .config files and not autoconf.h: >>>>> autoconf.h only includes #defines for non-n bool/tristate symbols. >>>>> >>>>> === Testing === >>>>> >>>>> The following testing was done with the x86 Kconfigs: >>>>> >>>>> - .config files generated before and after the change were compared to >>>>> verify that the only difference is some '# CONFIG_FOO is not set' >>>>> entries disappearing. A couple of these were inspected manually, and >>>>> most turned out to be from redundant 'default n/def_bool n' >>>>> properties. >>>>> >>>>> - The generated include/generated/autoconf.h was compared before and >>>>> after the change and verified to be identical. >>>>> >>>>> - As a sanity check, the same modification was done to Kconfiglib. >>>>> The Kconfiglib test suite was then run to check for any mismatches >>>>> against the output of the C implementation. >>>>> >>>>> Signed-off-by: Ulf Magnusson <ulfalizer@gmail.com> >>>>> --- >>>>> scripts/kconfig/symbol.c | 3 ++- >>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c >>>>> index cca9663be5dd..02eb8b10a83c 100644 >>>>> --- a/scripts/kconfig/symbol.c >>>>> +++ b/scripts/kconfig/symbol.c >>>>> @@ -403,9 +403,10 @@ void sym_calc_value(struct symbol *sym) >>>>> if (!sym_is_choice(sym)) { >>>>> prop = sym_get_default_prop(sym); >>>>> if (prop) { >>>>> - sym->flags |= SYMBOL_WRITE; >>>>> newval.tri = EXPR_AND(expr_calc_value(prop->expr), >>>>> prop->visible.tri); >>>>> + if (newval.tri != no) >>>>> + sym->flags |= SYMBOL_WRITE; >>>>> } >>>>> if (sym->implied.tri != no) { >>>>> sym->flags |= SYMBOL_WRITE; >>>>> -- >>>>> 2.14.1 >>>>> >>>> >>>> This stuff gets pretty obscure, so please tell me if you can think of >>>> any practical benefits to remembering an n default as a user selection >>>> for non-visible symbols (which is all '# CONFIG_FOO is not set' does >>>> in practice). I couldn't think of anything. >>>> >>> >>> In the context of >>> >>> config CC_HAS_STACKPROTECTOR >>> def_bool $(cc-option -fstack-protector) >>> >>> >>> Currently, we have 3 cases: >>> >>> [1] CONFIG_CC_HAS_STACKPROTECTOR=y >>> -> compiler flag is supported >>> [2] # CONFIG_CC_HAS_STACKPROTECTOR is not set >>> -> compiler flag is unsupported >>> [3] Missing >>> -> The symbol was hidden probably due to unmet "if ... endif" >>> >>> >>> With this change, [2] will be turned into [3]. >>> >>> That is the only drawback I came up with. >>> >>> I am not sure how many people want to check .config >>> to know the compiler capability... >>> >> >> >> I thought a bit more, then probably the grammatical >> consistency would win. (default n is always redundant) > > The behavior should be easy to explain in kconfig-language.txt too: A > missing entry means n, except visible n-valued symbols generate a > '# CONFIG_FOO is not set' comment just to keep track of the user's choice. > No weird exception for 'default'. > > That would demystify those '...is not set' lines too. > >> >> I want to apply this, but take a bit time >> in case somebody may have comments. >> >> >> BTW, do you want to check redundant 'default n' >> by checkpatch.pl ? > > Was thinking of that. Guess you could generate a warning for any of the > following: > > default n > default "n" > default 'n' We also sometimes see: default N :) > Could skip the warning for defaults with conditions maybe, as people > sometimes do stuff like > > default n if <cond> > default FOO > > (Though some of those look like they could refactored as well.) > > Or you could just say something like this for all of them: > > warning: check whether 'default n' is redundant -- n is the implicit default value for bool/tristate symbols
On Fri, Feb 23, 2018 at 11:19 PM, Randy Dunlap <rdunlap@infradead.org> wrote: > On 02/23/2018 01:33 PM, Ulf Magnusson wrote: >> On Sat, Feb 24, 2018 at 12:59:51AM +0900, Masahiro Yamada wrote: >>> 2018-02-23 22:25 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>: >>>> 2018-02-23 15:14 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: >>>>> On Fri, Feb 23, 2018 at 7:09 AM, Ulf Magnusson <ulfalizer@gmail.com> wrote: >>>>>> === Background === >>>>>> >>>>>> A "# CONFIG_FOO is not set" line is written to .config for visible >>>>>> bool/tristate symbols with value n. The idea is to remember the user >>>>>> selection without having to set a Makefile variable (having undefined >>>>>> Make variables correspond to n is handy when testing them in the >>>>>> Makefiles). >>>>>> >>>>>> Currently, a "# CONFIG_FOO is not set" line is also written to .config >>>>>> for all bool/tristate symbols that get the value n through a 'default'. >>>>>> This is inconsistent with how 'select' and 'imply' work, which only >>>>>> write non-n symbols. It also seems redundant: >>>>>> >>>>>> - If the symbol is visible and has value n, then >>>>>> "# CONFIG_FOO is not set" will always be written anyway. >>>>>> >>>>>> - If the symbol is not visible, then "# CONFIG_FOO is not set" has no >>>>>> effect on it. >>>>>> >>>>>> - If the symbol becomes visible later, there shouldn't be any harm in >>>>>> recalculating the default value at that point. >>>>>> >>>>>> === Changes === >>>>>> >>>>>> Change sym_calc_value() to only set SYMBOL_WRITE (write to .config) for >>>>>> non-n defaults. This reduces the size of the x86 .config on my system by >>>>>> about 1% (due to removed "# CONFIG_FOO is not set" entries). >>>>>> >>>>>> One side effect of this change is making 'default n' equivalent to >>>>>> having no explicit default. That might make it clearer to people that >>>>>> 'default n' is redundant. >>>>>> >>>>>> This change only affects generated .config files and not autoconf.h: >>>>>> autoconf.h only includes #defines for non-n bool/tristate symbols. >>>>>> >>>>>> === Testing === >>>>>> >>>>>> The following testing was done with the x86 Kconfigs: >>>>>> >>>>>> - .config files generated before and after the change were compared to >>>>>> verify that the only difference is some '# CONFIG_FOO is not set' >>>>>> entries disappearing. A couple of these were inspected manually, and >>>>>> most turned out to be from redundant 'default n/def_bool n' >>>>>> properties. >>>>>> >>>>>> - The generated include/generated/autoconf.h was compared before and >>>>>> after the change and verified to be identical. >>>>>> >>>>>> - As a sanity check, the same modification was done to Kconfiglib. >>>>>> The Kconfiglib test suite was then run to check for any mismatches >>>>>> against the output of the C implementation. >>>>>> >>>>>> Signed-off-by: Ulf Magnusson <ulfalizer@gmail.com> >>>>>> --- >>>>>> scripts/kconfig/symbol.c | 3 ++- >>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c >>>>>> index cca9663be5dd..02eb8b10a83c 100644 >>>>>> --- a/scripts/kconfig/symbol.c >>>>>> +++ b/scripts/kconfig/symbol.c >>>>>> @@ -403,9 +403,10 @@ void sym_calc_value(struct symbol *sym) >>>>>> if (!sym_is_choice(sym)) { >>>>>> prop = sym_get_default_prop(sym); >>>>>> if (prop) { >>>>>> - sym->flags |= SYMBOL_WRITE; >>>>>> newval.tri = EXPR_AND(expr_calc_value(prop->expr), >>>>>> prop->visible.tri); >>>>>> + if (newval.tri != no) >>>>>> + sym->flags |= SYMBOL_WRITE; >>>>>> } >>>>>> if (sym->implied.tri != no) { >>>>>> sym->flags |= SYMBOL_WRITE; >>>>>> -- >>>>>> 2.14.1 >>>>>> >>>>> >>>>> This stuff gets pretty obscure, so please tell me if you can think of >>>>> any practical benefits to remembering an n default as a user selection >>>>> for non-visible symbols (which is all '# CONFIG_FOO is not set' does >>>>> in practice). I couldn't think of anything. >>>>> >>>> >>>> In the context of >>>> >>>> config CC_HAS_STACKPROTECTOR >>>> def_bool $(cc-option -fstack-protector) >>>> >>>> >>>> Currently, we have 3 cases: >>>> >>>> [1] CONFIG_CC_HAS_STACKPROTECTOR=y >>>> -> compiler flag is supported >>>> [2] # CONFIG_CC_HAS_STACKPROTECTOR is not set >>>> -> compiler flag is unsupported >>>> [3] Missing >>>> -> The symbol was hidden probably due to unmet "if ... endif" >>>> >>>> >>>> With this change, [2] will be turned into [3]. >>>> >>>> That is the only drawback I came up with. >>>> >>>> I am not sure how many people want to check .config >>>> to know the compiler capability... >>>> >>> >>> >>> I thought a bit more, then probably the grammatical >>> consistency would win. (default n is always redundant) >> >> The behavior should be easy to explain in kconfig-language.txt too: A >> missing entry means n, except visible n-valued symbols generate a >> '# CONFIG_FOO is not set' comment just to keep track of the user's choice. >> No weird exception for 'default'. >> >> That would demystify those '...is not set' lines too. >> >>> >>> I want to apply this, but take a bit time >>> in case somebody may have comments. >>> >>> >>> BTW, do you want to check redundant 'default n' >>> by checkpatch.pl ? >> >> Was thinking of that. Guess you could generate a warning for any of the >> following: >> >> default n >> default "n" >> default 'n' > > We also sometimes see: > default N > :) > Could add a warning that looks for 'default N/M/Y' and says to use 'default n/m/y' as well, maybe (after which you'd get a new warning telling you to get rid of the "default n"... never happy :P). Masahiro: Could you bump https://lkml.org/lkml/2018/2/22/935 if you're happy with it? Maybe Joe is waiting to see if you have objections. After that I could add some new checks. Cheers, Ulf -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
=== Background ===
A "# CONFIG_FOO is not set" line is written to .config for visible
bool/tristate symbols with value n. The idea is to remember the user
selection without having to set a Makefile variable (having undefined
Make variables correspond to n is handy when testing them in the
Makefiles).
Currently, a "# CONFIG_FOO is not set" line is also written to .config
for all bool/tristate symbols that get the value n through a 'default'.
This is inconsistent with how 'select' and 'imply' work, which only
write non-n symbols. It also seems redundant:
- If the symbol is visible and has value n, then
"# CONFIG_FOO is not set" will always be written anyway.
- If the symbol is not visible, then "# CONFIG_FOO is not set" has no
effect on it.
- If the symbol becomes visible later, there shouldn't be any harm in
recalculating the default value at that point.
=== Changes ===
Change sym_calc_value() to only set SYMBOL_WRITE (write to .config) for
non-n defaults. This reduces the size of the x86 .config on my system by
about 1% (due to removed "# CONFIG_FOO is not set" entries).
One side effect of this change is making 'default n' equivalent to
having no explicit default. That might make it clearer to people that
'default n' is redundant.
This change only affects generated .config files and not autoconf.h:
autoconf.h only includes #defines for non-n bool/tristate symbols.
=== Testing ===
The following testing was done with the x86 Kconfigs:
- .config files generated before and after the change were compared to
verify that the only difference is some '# CONFIG_FOO is not set'
entries disappearing. A couple of these were inspected manually, and
most turned out to be from redundant 'default n/def_bool n'
properties.
- The generated include/generated/autoconf.h was compared before and
after the change and verified to be identical.
- As a sanity check, the same modification was done to Kconfiglib.
The Kconfiglib test suite was then run to check for any mismatches
against the output of the C implementation.
Signed-off-by: Ulf Magnusson <ulfalizer@gmail.com>
---
scripts/kconfig/symbol.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index cca9663be5dd..02eb8b10a83c 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -403,9 +403,10 @@ void sym_calc_value(struct symbol *sym)
if (!sym_is_choice(sym)) {
prop = sym_get_default_prop(sym);
if (prop) {
- sym->flags |= SYMBOL_WRITE;
newval.tri = EXPR_AND(expr_calc_value(prop->expr),
prop->visible.tri);
+ if (newval.tri != no)
+ sym->flags |= SYMBOL_WRITE;
}
if (sym->implied.tri != no) {
sym->flags |= SYMBOL_WRITE;