| Message ID | 559655E3.6010400@fau.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 3, 2015 at 11:29 AM, Andreas Ruprecht <andreas.ruprecht@fau.de> wrote: > On 07/03/2015 10:59, Paul Bolle wrote: >> On vr, 2015-07-03 at 09:33 +0200, Andreas Ruprecht wrote: >>> I tested the behaviour on yesterday's linux-next, but the commit >>> mentioned above will only complain for invalid characters inside the >>> PARAM case and not for COMMANDs. So, as an example, if you write >>> something like >>> >>> config ACPI_REV_OVERRIDE_POSSIBLE >>> depends on X86 + >>> [...] >>> >>> Kconfig will complain about the '+'. This, however, does not apply for >>> top-level statements like 'config', 'menuconfig', and so on. >> >> Which might explain why this silly mistake went unnoticed. (And, as I >> think you implied, it doesn't help that the empty rule we're hitting >> here is not commented.) >> >> So the naive solution seems to be to also add the warning to COMMAND's >> rule for '.'. A quick test suggest that would work. Am I missing some >> obvious downside with that solution? > > Well, as I mentioned earlier, with a patch similar to the one below this > warning is also generated three times for every '---' before 'help'. > This results in a giant pile of warnings: > > ruprecht@box:linux-next$ rm -f scripts/kconfig/*_shipped && > REGENERATE_PARSERS=1 make allyesconfig 2>&1 | wc -l > 7419 > > The output looks like this: > scripts/kconfig/conf --allyesconfig Kconfig > arch/x86/Kconfig:4:warning: ignoring unsupported character '-' > arch/x86/Kconfig:4:warning: ignoring unsupported character '-' > arch/x86/Kconfig:4:warning: ignoring unsupported character '-' > init/Kconfig:222:warning: ignoring unsupported character '-' > init/Kconfig:222:warning: ignoring unsupported character '-' > init/Kconfig:222:warning: ignoring unsupported character '-' > init/Kconfig:244:warning: ignoring unsupported character '-' > init/Kconfig:244:warning: ignoring unsupported character '-' > init/Kconfig:244:warning: ignoring unsupported character '-' > [...] > > So we would need to add special treatment for '-' also in the command > case, right? But that doesn't look appealing to me, more like a dirty, > dirty hack around the actual problem... > > Regards, > > Andreas > Except for scattered accidents like in the original message, which are hopefully pretty rare and easy to fix, the only documented thing that depends on that lexer sloppiness is the ---help--- "token". I'd just add "---help---" as another T_HELP alias (or get rid of it altogether, but that's probably more work than it's worth). Tightening things up should be safe after that. /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, Jul 3, 2015 at 12:46 PM, Ulf Magnusson <ulfalizer.lkml@gmail.com> wrote: > On Fri, Jul 3, 2015 at 11:29 AM, Andreas Ruprecht > <andreas.ruprecht@fau.de> wrote: >> On 07/03/2015 10:59, Paul Bolle wrote: >>> On vr, 2015-07-03 at 09:33 +0200, Andreas Ruprecht wrote: >>>> I tested the behaviour on yesterday's linux-next, but the commit >>>> mentioned above will only complain for invalid characters inside the >>>> PARAM case and not for COMMANDs. So, as an example, if you write >>>> something like >>>> >>>> config ACPI_REV_OVERRIDE_POSSIBLE >>>> depends on X86 + >>>> [...] >>>> >>>> Kconfig will complain about the '+'. This, however, does not apply for >>>> top-level statements like 'config', 'menuconfig', and so on. >>> >>> Which might explain why this silly mistake went unnoticed. (And, as I >>> think you implied, it doesn't help that the empty rule we're hitting >>> here is not commented.) >>> >>> So the naive solution seems to be to also add the warning to COMMAND's >>> rule for '.'. A quick test suggest that would work. Am I missing some >>> obvious downside with that solution? >> >> Well, as I mentioned earlier, with a patch similar to the one below this >> warning is also generated three times for every '---' before 'help'. >> This results in a giant pile of warnings: >> >> ruprecht@box:linux-next$ rm -f scripts/kconfig/*_shipped && >> REGENERATE_PARSERS=1 make allyesconfig 2>&1 | wc -l >> 7419 >> >> The output looks like this: >> scripts/kconfig/conf --allyesconfig Kconfig >> arch/x86/Kconfig:4:warning: ignoring unsupported character '-' >> arch/x86/Kconfig:4:warning: ignoring unsupported character '-' >> arch/x86/Kconfig:4:warning: ignoring unsupported character '-' >> init/Kconfig:222:warning: ignoring unsupported character '-' >> init/Kconfig:222:warning: ignoring unsupported character '-' >> init/Kconfig:222:warning: ignoring unsupported character '-' >> init/Kconfig:244:warning: ignoring unsupported character '-' >> init/Kconfig:244:warning: ignoring unsupported character '-' >> init/Kconfig:244:warning: ignoring unsupported character '-' >> [...] >> >> So we would need to add special treatment for '-' also in the command >> case, right? But that doesn't look appealing to me, more like a dirty, >> dirty hack around the actual problem... >> >> Regards, >> >> Andreas >> > > Except for scattered accidents like in the original message, which are > hopefully pretty rare and easy to fix, the only documented thing that depends > on that lexer sloppiness is the ---help--- "token". > > I'd just add "---help---" as another T_HELP alias (or get rid of it altogether, > but that's probably more work than it's worth). Tightening things up should be > safe after that. This idea has a big ACK from me. It seems to me the cleanest way to solve the issue. Kind regards, Valentin > /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 vr, 2015-07-03 at 11:29 +0200, Andreas Ruprecht wrote: > Well, as I mentioned earlier, with a patch similar to the one below > this > warning is also generated three times for every '---' before 'help'. You're right, that was in your first message. It already slipped my mind. > So we would need to add special treatment for '-' also in the command > case, right? But that doesn't look appealing to me, more like a dirty, > dirty hack around the actual problem... It seems so. Someone ambitious might want to jump in the lex rules and see what can be done in a clean way. (Perhaps that will start with renaming COMMAND and PARAM and/or documenting these states.) I think I already demonstrated that I'm too unfamiliar with lex for it to make sense to volunteer. Thanks, Paul Bolle -- 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
2015-07-03 12:51 GMT+02:00 Valentin Rothberg <valentinrothberg@gmail.com>: >>> >>> The output looks like this: >>> scripts/kconfig/conf --allyesconfig Kconfig >>> arch/x86/Kconfig:4:warning: ignoring unsupported character '-' >>> arch/x86/Kconfig:4:warning: ignoring unsupported character '-' >>> arch/x86/Kconfig:4:warning: ignoring unsupported character '-' >>> init/Kconfig:222:warning: ignoring unsupported character '-' >>> init/Kconfig:222:warning: ignoring unsupported character '-' >>> init/Kconfig:222:warning: ignoring unsupported character '-' >>> init/Kconfig:244:warning: ignoring unsupported character '-' >>> init/Kconfig:244:warning: ignoring unsupported character '-' >>> init/Kconfig:244:warning: ignoring unsupported character '-' >>> [...] >>> >>> So we would need to add special treatment for '-' also in the command >>> case, right? But that doesn't look appealing to me, more like a dirty, >>> dirty hack around the actual problem... >>> >>> Regards, >>> >>> Andreas >>> >> >> Except for scattered accidents like in the original message, which are >> hopefully pretty rare and easy to fix, the only documented thing that depends >> on that lexer sloppiness is the ---help--- "token". >> >> I'd just add "---help---" as another T_HELP alias (or get rid of it altogether, >> but that's probably more work than it's worth). Tightening things up should be >> safe after that. > > This idea has a big ACK from me. It seems to me the cleanest way to > solve the issue. > Agreed! I also wanted to suggest this solution, but Ulf was faster :) Kind Regards, Stefan -- 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 vr, 2015-07-03 at 12:46 +0200, Ulf Magnusson wrote: > Except for scattered accidents like in the original message, which are > hopefully pretty rare and easy to fix, Correct. > the only documented thing that depends > on that lexer sloppiness is the ---help--- "token". > > I'd just add "---help---" as another T_HELP alias Which implies dropping the empty rule for "---", right? > (or get rid of it altogether, > but that's probably more work than it's worth). $git grep -e "---help---" -- "*Kconfig*" | wc -l 2590 Doable. Might not make you friends. > Tightening things up should be safe after that. Did you already try adding ---help--- (and something similar to Andreas' check for the mistake we're discussing here)? Paul Bolle -- 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
2015-07-03 12:56 GMT+02:00 Stefan Hengelein <stefan.hengelein@fau.de>: > 2015-07-03 12:51 GMT+02:00 Valentin Rothberg <valentinrothberg@gmail.com>: >>>> >>>> The output looks like this: >>>> scripts/kconfig/conf --allyesconfig Kconfig >>>> arch/x86/Kconfig:4:warning: ignoring unsupported character '-' >>>> arch/x86/Kconfig:4:warning: ignoring unsupported character '-' >>>> arch/x86/Kconfig:4:warning: ignoring unsupported character '-' >>>> init/Kconfig:222:warning: ignoring unsupported character '-' >>>> init/Kconfig:222:warning: ignoring unsupported character '-' >>>> init/Kconfig:222:warning: ignoring unsupported character '-' >>>> init/Kconfig:244:warning: ignoring unsupported character '-' >>>> init/Kconfig:244:warning: ignoring unsupported character '-' >>>> init/Kconfig:244:warning: ignoring unsupported character '-' >>>> [...] >>>> >>>> So we would need to add special treatment for '-' also in the command >>>> case, right? But that doesn't look appealing to me, more like a dirty, >>>> dirty hack around the actual problem... >>>> >>>> Regards, >>>> >>>> Andreas >>>> >>> >>> Except for scattered accidents like in the original message, which are >>> hopefully pretty rare and easy to fix, the only documented thing that depends >>> on that lexer sloppiness is the ---help--- "token". >>> >>> I'd just add "---help---" as another T_HELP alias (or get rid of it altogether, >>> but that's probably more work than it's worth). Tightening things up should be >>> safe after that. >> >> This idea has a big ACK from me. It seems to me the cleanest way to >> solve the issue. >> > > Agreed! I also wanted to suggest this solution, but Ulf was faster :) > > Kind Regards, > Stefan However, thinking about this solution a little more.... This change might lead to parser conflicts....shift-reduce conflicts maybe. Is the '-' used somewhere else and has a distinct token or is it just ignored and thrown away? Kind Regards, Stefan -- 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, Jul 3, 2015 at 1:11 PM, Stefan Hengelein <stefan.hengelein@fau.de> wrote: > 2015-07-03 12:56 GMT+02:00 Stefan Hengelein <stefan.hengelein@fau.de>: >> 2015-07-03 12:51 GMT+02:00 Valentin Rothberg <valentinrothberg@gmail.com>: >>>>> >>>>> The output looks like this: >>>>> scripts/kconfig/conf --allyesconfig Kconfig >>>>> arch/x86/Kconfig:4:warning: ignoring unsupported character '-' >>>>> arch/x86/Kconfig:4:warning: ignoring unsupported character '-' >>>>> arch/x86/Kconfig:4:warning: ignoring unsupported character '-' >>>>> init/Kconfig:222:warning: ignoring unsupported character '-' >>>>> init/Kconfig:222:warning: ignoring unsupported character '-' >>>>> init/Kconfig:222:warning: ignoring unsupported character '-' >>>>> init/Kconfig:244:warning: ignoring unsupported character '-' >>>>> init/Kconfig:244:warning: ignoring unsupported character '-' >>>>> init/Kconfig:244:warning: ignoring unsupported character '-' >>>>> [...] >>>>> >>>>> So we would need to add special treatment for '-' also in the command >>>>> case, right? But that doesn't look appealing to me, more like a dirty, >>>>> dirty hack around the actual problem... >>>>> >>>>> Regards, >>>>> >>>>> Andreas >>>>> >>>> >>>> Except for scattered accidents like in the original message, which are >>>> hopefully pretty rare and easy to fix, the only documented thing that depends >>>> on that lexer sloppiness is the ---help--- "token". >>>> >>>> I'd just add "---help---" as another T_HELP alias (or get rid of it altogether, >>>> but that's probably more work than it's worth). Tightening things up should be >>>> safe after that. >>> >>> This idea has a big ACK from me. It seems to me the cleanest way to >>> solve the issue. >>> >> >> Agreed! I also wanted to suggest this solution, but Ulf was faster :) >> >> Kind Regards, >> Stefan > > However, thinking about this solution a little more.... > This change might lead to parser conflicts....shift-reduce conflicts maybe. > > Is the '-' used somewhere else and has a distinct token or is it just > ignored and thrown away? > Yeah, I considered that too. Been a few years since I really dug into zconf.{l,y,gperf} and zconf.y, but from a quick look I think it might work out. With things tightened up, '-' should only ever appear as part of identifiers and inside strings and comments. There's no T_MINUS or similar. /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
diff --git a/scripts/kconfig/zconf.l b/scripts/kconfig/zconf.l index 200a3fe..642f5b2 100644 --- a/scripts/kconfig/zconf.l +++ b/scripts/kconfig/zconf.l @@ -106,7 +106,11 @@ n [A-Za-z0-9_] zconflval.string = text; return T_WORD; } - . + . { + fprintf(stderr, + "%s:%d:warning: ignoring unsupported character '%c'\n", + zconf_curname(), zconf_lineno(), *yytext); + } \n { BEGIN(INITIAL); current_file->lineno++;