Message-ID: <73d31fc9-c1d0-7643-c9aa-2fcebdb24b73@gmail.com>
Date:   Wed, 1 Sep 2021 18:17:55 +0300
From:   Ariel Marcovitch <arielmarcovitch@...il.com>
To:     Masahiro Yamada <masahiroy@...nel.org>
Cc:     Michal Marek <michal.lkml@...kovi.net>,
        Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Valentin Rothberg <valentinrothberg@...il.com>
Subject: Re: [PATCH 2/3] checkkconfigsymbols.py: Fix Kconfig parsing to find
 'if' lines

Hi again!


On 30/08/2021 2:41, Masahiro Yamada wrote:
 > On Sun, Aug 29, 2021 at 10:18 PM Ariel Marcovitch
 > <arielmarcovitch@...il.com> wrote:
 >>
 >> Hello again!
 >>
 >> On 24/08/2021 16:30, Masahiro Yamada wrote:
 >>  > On Mon, Aug 23, 2021 at 4:22 AM Ariel Marcovitch
 >>  > <arielmarcovitch@...il.com> wrote:
 >>  >>
 >>  >> When parsing Kconfig files to find symbol definitions and 
references,
 >>  >> lines after a 'help' line are skipped until a new config definition
 >>  >> starts.
 >>  >>
 >>  >> However, it is quite common to define a config and then make 
some other
 >>  >> configs depend on it by adding an 'if' line. This kind of kconfig
 >>  >> statement usually appears after a config definition which might 
contain
 >>  >> a 'help' section. The 'if' line is skipped in parse_kconfig_file()
 >>  >> because it is not a config definition.
 >>  >>
 >>  >> This means that symbols referenced in this kind of statements are
 >>  >> ignored by this function and thus are not considered undefined
 >>  >> references in case the symbol is not defined.
 >>  >>
 >>  >> The REGEX_KCONFIG_STMT regex can't be used because the other 
types of
 >>  >> statements can't break help lines.
 >>  >>
 >>  >> Define a new regex for matching 'if' statements and stop the 'help'
 >>  >> skipping in case it is encountered.
 >>  >>
 >>  >> Signed-off-by: Ariel Marcovitch <arielmarcovitch@...il.com>
 >>  >> ---
 >>  >>  scripts/checkkconfigsymbols.py | 8 +++++++-
 >>  >>  1 file changed, 7 insertions(+), 1 deletion(-)
 >>  >>
 >>  >> diff --git a/scripts/checkkconfigsymbols.py
 >> b/scripts/checkkconfigsymbols.py
 >>  >> index b9b0f15e5880..875e9a2c14b2 100755
 >>  >> --- a/scripts/checkkconfigsymbols.py
 >>  >> +++ b/scripts/checkkconfigsymbols.py
 >>  >> @@ -26,6 +26,7 @@ EXPR = r"(?:" + OPERATORS + r"|\s|" + SYMBOL + 
r")+"
 >>  >>  DEFAULT = r"default\s+.*?(?:if\s.+){,1}"
 >>  >>  STMT = r"^\s*(?:if|select|imply|depends\s+on|(?:" + DEFAULT +
 >> r"))\s+" + EXPR
 >>  >>  SOURCE_SYMBOL = r"(?:\W|\b)+[D]{,1}CONFIG_(" + SYMBOL + r")"
 >>  >> +IF_LINE = r"^\s*(?:if)\s+" + EXPR
 >>  >
 >>  >
 >>  > Why is it enclosed by "(?: )"   ?
 >>  >
 >>  > "(?:if)"  seems to the same as "if"
 >> Oh you are absolutely right.
 >> I just mindlessly copied the STMT regex and removed the other types :)
 >>  >
 >>  >
 >>  >
 >>  >
 >>  >
 >>  >
 >>  >>
 >>  >>  # regex objects
 >>  >>  REGEX_FILE_KCONFIG = re.compile(r".*Kconfig[\.\w+\-]*$")
 >>  >> @@ -35,11 +36,11 @@ REGEX_KCONFIG_DEF = re.compile(DEF)
 >>  >>  REGEX_KCONFIG_EXPR = re.compile(EXPR)
 >>  >>  REGEX_KCONFIG_STMT = re.compile(STMT)
 >>  >>  REGEX_KCONFIG_HELP = re.compile(r"^\s+help\s*$")
 >>  >> +REGEX_KCONFIG_IF_LINE = re.compile(IF_LINE)
 >>  >>  REGEX_FILTER_SYMBOLS = re.compile(r"[A-Za-z0-9]$")
 >>  >>  REGEX_NUMERIC = re.compile(r"0[xX][0-9a-fA-F]+|[0-9]+")
 >>  >>  REGEX_QUOTES = re.compile("(\"(.*?)\")")
 >>  >>
 >>  >> -
 >>  >>  def parse_options():
 >>  >>      """The user interface of this module."""
 >>  >>      usage = "Run this tool to detect Kconfig symbols that are
 >> referenced but " \
 >>  >> @@ -445,6 +446,11 @@ def parse_kconfig_file(kfile):
 >>  >>          line = line.strip('\n')
 >>  >>          line = line.split("#")[0]  # ignore comments
 >>  >>
 >>  >> +        # 'if EXPR' lines can be after help lines
 >>  >> +        # The if line itself is handled later
 >>  >> +        if REGEX_KCONFIG_IF_LINE.match(line):
 >>  >> +            skip = False
 >>  >> +
 >>  >
 >>  >
 >>  > I do not think this is the right fix.
 >>  > There are similar patterns where
 >>  > config references are ignored.
 >>  >
 >>  > For example, FOO and BAR are ignored
 >>  > in the following cases.
 >>  >
 >>  > ex1)
 >>  >
 >>  > choice
 >>  >           prompt "foo"
 >>  >           default FOO
 >>  >
 >>  >
 >>  >
 >>  > ex2)
 >>  >
 >>  > menu "bar"
 >>  >            depends on BAR
 >>  >
 >>  >
 >>  >
 >>  >
 >>  > The help block ends with shallower indentation.
 >> So IIUC we need to measure the indentation when we encounter a help
 >> statement and in the next lines look for a line with a different depth
 >> (which is not an empty line because these are allowed).
 >
 >
 >
 > If you want to implement it precisely, yes.
 >
 >
 > Or, if you want to adopt a simpler
 > solution, detect the following keywords.
 >
 > comment
 > if
 > menu
 > choice

Actually, it seems that all statements are legal in this context.

So we can just use the STMT regex!

It does require reshuffling the logic there a little but this should
do.

 >
 >
 >
 > This is not precise, but it will work
 > in most cases.
 >
 >
 >
 > In the following example, the first 'menu'
 > is just a comment.
 > The second 'menu' is a keyword since it has
 > a shallower indentation.
 >
 >
 >
 >     help
 >        blah blah
 >        menu blah blah
 >        blah blah
 >   menu "menu prompt"
 >      depends on FOO
 >
 >

Yeah this will probably drive the parser crazy (even without my
changes, although my changes might make it worse).

But the indentation solution is kinda nasty.

Do you think the STMT regex check is enough or should I handle the
cases you mentioned with the indentation check?


 >
 >
 >
 >
 >
 >
 >
 >
 >
 >
 > --
 > Best Regards
 > Masahiro Yamada

Thanks again for your time :)
Ariel Marcovitch

Powered by blists - more mailing lists