Kconfig: '+config' valid syntax?

Kconfig: '+config' valid syntax?

[Valentin Rothberg]

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

Re: Kconfig: '+config' valid syntax?

[Paul Bolle]

[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

Re: Kconfig: '+config' valid syntax?

[Paul Bolle]

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

Re: Kconfig: '+config' valid syntax?

[Andreas Ruprecht]

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

Re: Kconfig: '+config' valid syntax?

[Paul Bolle]

[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

Re: Kconfig: '+config' valid syntax?

[Rafael J. Wysocki]

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

Re: Kconfig: '+config' valid syntax?

[Andreas Ruprecht]

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

Re: Kconfig: '+config' valid syntax?

[Paul Bolle]

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

Re: Kconfig: '+config' valid syntax?

[Andreas Ruprecht]

[-- Attachment #1: Type: text/plain, Size: 2035 bytes --]

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: zconf.l.patch --]
[-- Type: text/x-patch; name="zconf.l.patch", Size: 463 bytes --]

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++;

Re: Kconfig: '+config' valid syntax?

[Ulf Magnusson]

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

Re: Kconfig: '+config' valid syntax?

[Ulf Magnusson]

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

Re: Kconfig: '+config' valid syntax?

[Valentin Rothberg]

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

Re: Kconfig: '+config' valid syntax?

[Paul Bolle]

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

Re: Kconfig: '+config' valid syntax?

[Stefan Hengelein]

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

Re: Kconfig: '+config' valid syntax?

[Paul Bolle]

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

Re: Kconfig: '+config' valid syntax?

[Stefan Hengelein]

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

Re: Kconfig: '+config' valid syntax?

[Andreas Ruprecht]

[-- Attachment #1: Type: text/plain, Size: 942 bytes --]

On 07/03/2015 12:46, Ulf Magnusson wrote:
> 
> 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
> 

So we might want to do something like the attached patch, right?

Unfortunately, when I generate the zconf.{hash,lex,tab}.c files on my
machine, they have some notable differences to the _shipped versions:

ruprecht@box:linux-next$ diff -u zconf.tab.c zconf.tab.c_shipped
-  return yyresult;
+  /* Make sure YYID is used.  */
+  return YYID (yyresult);

and I can't find any Documentation on how to properly rebuild the
_shipped files... Does anybody have a hint on that?

Regards,

Andreas

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: zconf.l.patch --]
[-- Type: text/x-patch; name="zconf.l.patch", Size: 814 bytes --]

diff --git a/scripts/kconfig/zconf.l b/scripts/kconfig/zconf.l
index 200a3fe..84a5d05 100644
--- a/scripts/kconfig/zconf.l
+++ b/scripts/kconfig/zconf.l
@@ -106,7 +106,15 @@ n	[A-Za-z0-9_]
 		zconflval.string = text;
 		return T_WORD;
 	}
-	.
+	"---help---"	{
+		/* Support old syntax for help statement */
+		return T_HELP;
+	}
+	.	{
+		fprintf(stderr,
+		        "%s:%d:warning: ignoring unsupported character '%c'\n",
+		        zconf_curname(), zconf_lineno(), *yytext);
+	}
 	\n	{
 		BEGIN(INITIAL);
 		current_file->lineno++;
@@ -132,7 +140,6 @@ n	[A-Za-z0-9_]
 		BEGIN(STRING);
 	}
 	\n	BEGIN(INITIAL); current_file->lineno++; return T_EOL;
-	---	/* ignore */
 	({n}|[-/.])+	{
 		const struct kconf_id *id = kconf_id_lookup(yytext, yyleng);
 		if (id && id->flags & TF_PARAM) {

Re: Kconfig: '+config' valid syntax?

[Ulf Magnusson]

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

Re: Kconfig: '+config' valid syntax?

[Ulf Magnusson]

On Fri, Jul 3, 2015 at 1:33 PM, Andreas Ruprecht
<andreas.ruprecht@fau.de> wrote:
> On 07/03/2015 12:46, Ulf Magnusson wrote:
>>
>> 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
>>
>
> So we might want to do something like the attached patch, right?
>
> Unfortunately, when I generate the zconf.{hash,lex,tab}.c files on my
> machine, they have some notable differences to the _shipped versions:
>
> ruprecht@box:linux-next$ diff -u zconf.tab.c zconf.tab.c_shipped
> -  return yyresult;
> +  /* Make sure YYID is used.  */
> +  return YYID (yyresult);
>
> and I can't find any Documentation on how to properly rebuild the
> _shipped files... Does anybody have a hint on that?
>
> Regards,
>
> Andreas

Yup, something like that. Been too long, so I don't remember either,
though I don't remember having to do anything special.

I'm heading off to bed soon, so probably shouldn't be thinking. Could
take a look tomorrow if there's still a problem. :)

/Ulf

Re: Kconfig: '+config' valid syntax?

[Paul Bolle]

On vr, 2015-07-03 at 13:33 +0200, Andreas Ruprecht wrote:
> Unfortunately, when I generate the zconf.{hash,lex,tab}.c files on my
> machine, they have some notable differences to the _shipped versions:
> 
> ruprecht@box:linux-next$ diff -u zconf.tab.c zconf.tab.c_shipped
> -  return yyresult;
> +  /* Make sure YYID is used.  */
> +  return YYID (yyresult);

Different version of bison?

> and I can't find any Documentation on how to properly rebuild the
> _shipped files... Does anybody have a hint on that?

One of my many, many pet peeves: patches touching _shipped files like
these should mention the command line that was used. Because now
everyone is expected to figure that out themselves. Bonus points for
mentioning the version of the tool that was used.
 

Paul Bolle

Re: Kconfig: '+config' valid syntax?

[Ulf Magnusson]

On Fri, Jul 3, 2015 at 1:40 PM, Ulf Magnusson <ulfalizer.lkml@gmail.com> wrote:
> On Fri, Jul 3, 2015 at 1:33 PM, Andreas Ruprecht
> <andreas.ruprecht@fau.de> wrote:
>> On 07/03/2015 12:46, Ulf Magnusson wrote:
>>>
>>> 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
>>>
>>
>> So we might want to do something like the attached patch, right?
>>
>> Unfortunately, when I generate the zconf.{hash,lex,tab}.c files on my
>> machine, they have some notable differences to the _shipped versions:
>>
>> ruprecht@box:linux-next$ diff -u zconf.tab.c zconf.tab.c_shipped
>> -  return yyresult;
>> +  /* Make sure YYID is used.  */
>> +  return YYID (yyresult);
>>
>> and I can't find any Documentation on how to properly rebuild the
>> _shipped files... Does anybody have a hint on that?
>>
>> Regards,
>>
>> Andreas
>
> Yup, something like that. Been too long, so I don't remember either,
> though I don't remember having to do anything special.
>
> I'm heading off to bed soon, so probably shouldn't be thinking. Could
> take a look tomorrow if there's still a problem. :)
>
> /Ulf

Or add '---help---' to zconf.gperf I guess, to avoid special-casing it. Would
need to add '-' to the

n    [A-Za-z0-9_]

definition in zconf.l in that case too.

That should be safe provided the gperf part works out, since zconf.y should
verify that the T_WORD has the expected form. (As a side note, the first token
on a line is always a keyword, except for within help texts, which are handled
specially.)

I guess that might change the meaning of lines like "foo-bar" if '-' is
introduced as an operator, but that seems kinda academic as 'foo' would
probably still be a keyword. It's already broken due to the <PARAM> regex
({n}|[-/.])+ having '-' in it too.

/Ulf

Re: Kconfig: '+config' valid syntax?

[Ulf Magnusson]

On Fri, Jul 3, 2015 at 2:39 PM, Ulf Magnusson <ulfalizer.lkml@gmail.com> wrote:
> On Fri, Jul 3, 2015 at 1:40 PM, Ulf Magnusson <ulfalizer.lkml@gmail.com> wrote:
>> On Fri, Jul 3, 2015 at 1:33 PM, Andreas Ruprecht
>> <andreas.ruprecht@fau.de> wrote:
>>> On 07/03/2015 12:46, Ulf Magnusson wrote:
>>>>
>>>> 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
>>>>
>>>
>>> So we might want to do something like the attached patch, right?
>>>
>>> Unfortunately, when I generate the zconf.{hash,lex,tab}.c files on my
>>> machine, they have some notable differences to the _shipped versions:
>>>
>>> ruprecht@box:linux-next$ diff -u zconf.tab.c zconf.tab.c_shipped
>>> -  return yyresult;
>>> +  /* Make sure YYID is used.  */
>>> +  return YYID (yyresult);
>>>
>>> and I can't find any Documentation on how to properly rebuild the
>>> _shipped files... Does anybody have a hint on that?
>>>
>>> Regards,
>>>
>>> Andreas
>>
>> Yup, something like that. Been too long, so I don't remember either,
>> though I don't remember having to do anything special.
>>
>> I'm heading off to bed soon, so probably shouldn't be thinking. Could
>> take a look tomorrow if there's still a problem. :)
>>
>> /Ulf
>
> Or add '---help---' to zconf.gperf I guess, to avoid special-casing it. Would
> need to add '-' to the
>
> n    [A-Za-z0-9_]
>
> definition in zconf.l in that case too.
>
> That should be safe provided the gperf part works out, since zconf.y should
> verify that the T_WORD has the expected form. (As a side note, the first token
> on a line is always a keyword, except for within help texts, which are handled
> specially.)

...or that it's not a T_WORD rather, but a command. :P

>
> I guess that might change the meaning of lines like "foo-bar" if '-' is
> introduced as an operator, but that seems kinda academic as 'foo' would
> probably still be a keyword. It's already broken due to the <PARAM> regex
> ({n}|[-/.])+ having '-' in it too.
>
> /Ulf

/Ulf