| Message ID | CAD3Xx4Kh9ASyVYo7ZyRyr2maJbXwbqauQ7tUsLBUHof_hsaP2w@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
[Dropped Yann. You already know Yann disappeared.] On Thu, 2015-07-02 at 10:08 +0200, Valentin Rothberg wrote: > commit ed013214afa7 ("ACPI / init: Make it possible to override _REV") > is in today's linux-next tree (i.e., next-20150702) adding the > following hunk to drivers/acpi/Kconfig: > > --- a/drivers/acpi/Kconfig > +++ b/drivers/acpi/Kconfig > @@ -428,6 +428,26 @@ config XPOWER_PMIC_OPREGION > help > This config adds ACPI operation region support for XPower > AXP288 PMIC. > > ++config ACPI_REV_OVERRIDE_POSSIBLE (Odd. Botched conflict resolution?) > + bool "Allow supported ACPI revision to be overriden" > + depends on X86 > + default y > [...] > > By having a close look at the first added line, we can see that > '+config ACPI_...' is added. To my great surprise, it's valid Kconfig > syntax. I played a bit with this. It seems you can basically add a '+' anywhere you like and kconfig will just ignore it. > How is that possible? IMHO it's an invalid token, such that > Kconfig should complain about it. Or do I miss something? Welcome to the wonders of lex and yacc! I try to spend as little time as possible looking at the lex rules, so I'm just guessing here. Anyhow, you might start by looking at this snippet in zconf.l: . { unput(yytext[0]); BEGIN(COMMAND); } <COMMAND>{ {n}+ { [...] } . \n { BEGIN(INITIAL); current_file->lineno++; return T_EOL; } } Which perhaps translates to: - ignore unknown stuff for now and go in COMMAND state; - do something if we encounter some text ({n} = [A-Za-z0-9_]); - go in INITIAL state if we encounter newlines or unknown stuff. At the end of which we're back where we started before encountering the'+'. But there are more references to '.' in the lex rules so it's probably more complicated. Hope this helps, 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
On do, 2015-07-02 at 11:01 +0200, Paul Bolle wrote: > I'm just guessing here. Anyhow, you might start by looking at this > snippet in zconf.l: > . { > unput(yytext[0]); > BEGIN(COMMAND); > } > > > <COMMAND>{ > {n}+ { > [...] > } > . > \n { > BEGIN(INITIAL); > current_file->lineno++; > return T_EOL; > } > } > > Which perhaps translates to: > - ignore unknown stuff for now and go in COMMAND state; > - do something if we encounter some text ({n} = [A-Za-z0-9_]); > - go in INITIAL state if we encounter newlines or unknown stuff. > > At the end of which we're back where we started before encountering > the'+'. But there are more references to '.' in the lex rules so it's > probably more complicated. All of which is moot after commit 2e0d737fc76f ("kconfig: don't silently ignore unhandled characters"). That's in linux-next but not (yet) in v4.1+. It even has my Ack! My memory really must be degrading now... 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
Hi, On 07/02/2015 11:01, Paul Bolle wrote: > [Dropped Yann. You already know Yann disappeared.] > > On Thu, 2015-07-02 at 10:08 +0200, Valentin Rothberg wrote: >> commit ed013214afa7 ("ACPI / init: Make it possible to override _REV") >> is in today's linux-next tree (i.e., next-20150702) adding the >> following hunk to drivers/acpi/Kconfig: >> >> --- a/drivers/acpi/Kconfig >> +++ b/drivers/acpi/Kconfig >> @@ -428,6 +428,26 @@ config XPOWER_PMIC_OPREGION >> help >> This config adds ACPI operation region support for XPower >> AXP288 PMIC. >> >> ++config ACPI_REV_OVERRIDE_POSSIBLE > > (Odd. Botched conflict resolution?) > >> + bool "Allow supported ACPI revision to be overriden" >> + depends on X86 >> + default y >> [...] >> >> By having a close look at the first added line, we can see that >> '+config ACPI_...' is added. To my great surprise, it's valid Kconfig >> syntax. > > I played a bit with this. It seems you can basically add a '+' anywhere > you like and kconfig will just ignore it. > >> How is that possible? IMHO it's an invalid token, such that >> Kconfig should complain about it. Or do I miss something? > > Welcome to the wonders of lex and yacc! > > I try to spend as little time as possible looking at the lex rules, so > I'm just guessing here. Anyhow, you might start by looking at this > snippet in zconf.l: > . { > unput(yytext[0]); > BEGIN(COMMAND); > } > > > <COMMAND>{ > {n}+ { > [...] > } > . > \n { > BEGIN(INITIAL); > current_file->lineno++; > return T_EOL; > } > } > > Which perhaps translates to: > - ignore unknown stuff for now and go in COMMAND state; > - do something if we encounter some text ({n} = [A-Za-z0-9_]); > - go in INITIAL state if we encounter newlines or unknown stuff. This is _almost_ true (which I think is the problem). The rule for "." is empty, and not the same rule as for \n. So what happens here, is that any unknown characters are simply ignored until something in {n}+ shows up. If I add something like the following instead: + . { + fprintf(stderr, "something else: %s\n", yytext); + BEGIN(INITIAL); + } then Kconfig prints the message for the "+", but unfortunately also lots of "-" (which come from the occasional "---help---" instead of "help". As it looks to me, they are only ignored one step later inside the <PARAM> case. So changing it like the above is not the solution, but at least we know where the silent ignore is coming from... Any idea how to properly fix this? Regards, Andreas -- 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
[Spoiler: please start at the end of my reply.] On do, 2015-07-02 at 13:57 +0200, Andreas Ruprecht wrote: > On 07/02/2015 11:01, Paul Bolle wrote: > > On Thu, 2015-07-02 at 10:08 +0200, Valentin Rothberg wrote: > > Welcome to the wonders of lex and yacc! > > > > I try to spend as little time as possible looking at the lex rules, > > so > > I'm just guessing here. Anyhow, you might start by looking at this > > snippet in zconf.l: > > . { > > unput(yytext[0]); > > BEGIN(COMMAND); > > } > > > > > > <COMMAND>{ > > {n}+ { > > [...] > > } > > . > > \n { > > BEGIN(INITIAL); > > current_file->lineno++; > > return T_EOL; > > } > > } > > > > Which perhaps translates to: > > - ignore unknown stuff for now and go in COMMAND state; > > - do something if we encounter some text ({n} = [A-Za-z0-9_]); > > - go in INITIAL state if we encounter newlines or unknown stuff. > > This is _almost_ true (which I think is the problem). The rule for "." > is empty, and not the same rule as for \n. I see. That's nice to know. > So what happens here, is that > any unknown characters are simply ignored until something in {n}+ > shows up. How can unknown characters be part of {n}+? > If I add something like the following instead: > + . { > + fprintf(stderr, "something else: %s\n", yytext); > + BEGIN(INITIAL); > + } > > then Kconfig prints the message for the "+", but unfortunately also > lots > of "-" (which come from the occasional "---help---" instead of "help". > As it looks to me, they are only ignored one step later inside the > <PARAM> case. (Years ago I submitted a few trivial cleanups for typos regarding "-- -help---". I should have followed up on those cleanups with a patch to remove the silly lex rule that just ignores "---". Perhaps we should add an actual definition for "---help---". On the other hand: last time I checked nothing actually cares about the "---" markers so adding them achieves nothing. Cleaning all Kconfig files to get rid of these markers is probably not worth it. Add a checkpatch rule to warn about their uselessness?) > So changing it like the above is not the solution, but at least we > know > where the silent ignore is coming from... > > Any idea how to properly fix this? As I said in my follow up: see commit 2e0d737fc76f ("kconfig: don't silently ignore unhandled characters"). 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
On Thursday, July 02, 2015 11:01:02 AM Paul Bolle wrote: > [Dropped Yann. You already know Yann disappeared.] > > On Thu, 2015-07-02 at 10:08 +0200, Valentin Rothberg wrote: > > commit ed013214afa7 ("ACPI / init: Make it possible to override _REV") > > is in today's linux-next tree (i.e., next-20150702) adding the > > following hunk to drivers/acpi/Kconfig: > > > > --- a/drivers/acpi/Kconfig > > +++ b/drivers/acpi/Kconfig > > @@ -428,6 +428,26 @@ config XPOWER_PMIC_OPREGION > > help > > This config adds ACPI operation region support for XPower > > AXP288 PMIC. > > > > ++config ACPI_REV_OVERRIDE_POSSIBLE > > (Odd. Botched conflict resolution?) A mistake. Thanks, Rafael -- 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 07/02/2015 14:10, Paul Bolle wrote: > [Spoiler: please start at the end of my reply.] > > On do, 2015-07-02 at 13:57 +0200, Andreas Ruprecht wrote: >> On 07/02/2015 11:01, Paul Bolle wrote: >>> On Thu, 2015-07-02 at 10:08 +0200, Valentin Rothberg wrote: >>> Welcome to the wonders of lex and yacc! >>> >>> I try to spend as little time as possible looking at the lex rules, >>> so >>> I'm just guessing here. Anyhow, you might start by looking at this >>> snippet in zconf.l: >>> . { >>> unput(yytext[0]); >>> BEGIN(COMMAND); >>> } >>> >>> >>> <COMMAND>{ >>> {n}+ { >>> [...] >>> } >>> . >>> \n { >>> BEGIN(INITIAL); >>> current_file->lineno++; >>> return T_EOL; >>> } >>> } >>> >>> Which perhaps translates to: >>> - ignore unknown stuff for now and go in COMMAND state; >>> - do something if we encounter some text ({n} = [A-Za-z0-9_]); >>> - go in INITIAL state if we encounter newlines or unknown stuff. >> >> This is _almost_ true (which I think is the problem). The rule for "." >> is empty, and not the same rule as for \n. > > I see. That's nice to know. > >> So what happens here, is that >> any unknown characters are simply ignored until something in {n}+ >> shows up. > > How can unknown characters be part of {n}+? > They are not considered part of {n}+, but through ignoring the '+' character with the empty '.' rule, the parser will go back into the top-level rule - the very first rule in your snippet above - see the 'c' character (from 'config'), go into COMMAND again and parse the 'config' item properly. > > As I said in my follow up: see commit 2e0d737fc76f ("kconfig: don't > silently ignore unhandled characters"). 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. Regards, Andreas -- 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 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? 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
On Thu, Jul 2, 2015 at 10:08 AM, Valentin Rothberg <valentinrothberg@gmail.com> wrote: > Hi, > > commit ed013214afa7 ("ACPI / init: Make it possible to override _REV") > is in today's linux-next tree (i.e., next-20150702) adding the > following hunk to drivers/acpi/Kconfig: > > --- a/drivers/acpi/Kconfig > +++ b/drivers/acpi/Kconfig > @@ -428,6 +428,26 @@ config XPOWER_PMIC_OPREGION > help > This config adds ACPI operation region support for XPower AXP288 PMIC. > > ++config ACPI_REV_OVERRIDE_POSSIBLE > + bool "Allow supported ACPI revision to be overriden" > + depends on X86 > + default y > [...] > > By having a close look at the first added line, we can see that > '+config ACPI_...' is added. To my great surprise, it's valid Kconfig > syntax. How is that possible? IMHO it's an invalid token, such that > Kconfig should complain about it. Or do I miss something? > > Kind regards, > Valentin For another take on this, you could look at the Config._tokenize() function in Kconfiglib (https://github.com/ulfalizer/Kconfiglib/blob/master/kconfiglib.py). Here's a relevant comment from that function: # The initial word on a line is parsed specially. Let # command_chars = [A-Za-z0-9_]. Then # - leading non-command_chars characters are ignored, and # - the first token consists the following one or more # command_chars characters. # This is why things like "----help--" are accepted. The root cause is sloppy lexing in zconf.l, IIRC. I didn't check whether it's been improved, but Kconfiglib needs to be backwards compatible either way. /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
--- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -428,6 +428,26 @@ config XPOWER_PMIC_OPREGION help This config adds ACPI operation region support for XPower AXP288 PMIC. ++config ACPI_REV_OVERRIDE_POSSIBLE + bool "Allow supported ACPI revision to be overriden" + depends on X86 + default y [...] By having a close look at the first added line, we can see that '+config ACPI_...' is added. To my great surprise, it's valid Kconfig syntax. How is that possible? IMHO it's an invalid token, such that Kconfig should complain about it. Or do I miss something? Kind regards, Valentin -- 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
Hi, commit ed013214afa7 ("ACPI / init: Make it possible to override _REV") is in today's linux-next tree (i.e., next-20150702) adding the following hunk to drivers/acpi/Kconfig: