Date:   Sat, 22 Aug 2020 12:11:28 +0900
From:   Masahiro Yamada <masahiroy@...nel.org>
To:     Nick Desaulniers <ndesaulniers@...gle.com>
Cc:     Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        Nathan Huckleberry <nhuck@...gle.com>,
        Tom Roeder <tmroeder@...gle.com>,
        clang-built-linux <clang-built-linux@...glegroups.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 7/9] gen_compile_commands: support *.o, *.a,
 modules.order in positional argument

On Sat, Aug 22, 2020 at 9:59 AM Nick Desaulniers
<ndesaulniers@...gle.com> wrote:
>
> On Fri, Aug 21, 2020 at 12:02 PM Masahiro Yamada <masahiroy@...nel.org> wrote:
> >
> > This script currently searches the specified directory for .cmd files.
> > One drawback is it may contain stale .cmd files after you rebuild the
> > kernel several times without 'make clean'.
> >
> > This commit supports *.o, *.a, and modules.order as positional
> > parameters. If such files are given, they are parsed to collect
> > associated .cmd files. I added a generator helper for each of them.
> >
> > This feature is useful to get the list of active .cmd files from the
> > last build, and will be used by the next commit to wire up the
> > compile_commands.json rule to the Makefile.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@...nel.org>
> > ---
> >
> > Changes in v2:
> >   - Separate the file parser into generator functions
> >   - Use 'obj' instead of 'object' because 'object' is a built-in function
> >   - I think using 'file' is OK because it is not a built-in function in Python3
> >     (https://docs.python.org/3/library/functions.html)
> >     Anyway, the variable 'file' is no longer used in this version
> >   - Keep the previous work-flow to allow to search the given directory
> >
> >  scripts/gen_compile_commands.py | 100 ++++++++++++++++++++++++++++++--
> >  1 file changed, 96 insertions(+), 4 deletions(-)
> >
> > diff --git a/scripts/gen_compile_commands.py b/scripts/gen_compile_commands.py
> > index 6dec7e2c4098..65859e6044b5 100755
> > --- a/scripts/gen_compile_commands.py
> > +++ b/scripts/gen_compile_commands.py
> > @@ -12,6 +12,7 @@ import json
> >  import logging
> >  import os
> >  import re
> > +import subprocess
> >
> >  _DEFAULT_OUTPUT = 'compile_commands.json'
> >  _DEFAULT_LOG_LEVEL = 'WARNING'
> > @@ -32,8 +33,9 @@ def parse_arguments():
> >      Returns:
> >          log_level: A logging level to filter log output.
> >          directory: The work directory where the objects were built
> > +        ar: Command used for parsing .a archives
> >          output: Where to write the compile-commands JSON file.
> > -        paths: The list of directories to handle to find .cmd files
> > +        paths: The list of files/directories to handle to find .cmd files
> >      """
> >      usage = 'Creates a compile_commands.json database from kernel .cmd files'
> >      parser = argparse.ArgumentParser(description=usage)
> > @@ -53,12 +55,21 @@ def parse_arguments():
> >      parser.add_argument('--log_level', choices=_VALID_LOG_LEVELS,
> >                          default=_DEFAULT_LOG_LEVEL, help=log_level_help)
> >
> > +    ar_help = 'command used for parsing .a archives'
> > +    parser.add_argument('-a', '--ar', type=str, default='ar', help=ar_help)
>
> If there's a default, doesn't that mean it's no longer required? I
> think it should be required.  For a clang specific tool, we'd prefer
> the default to be llvm-ar anyways.

A good point.
I want to set reasonable values as default where possible.
'llvm-ar' is better.

I will change it.



> > +
> > +def cmdfiles_for_modorder(modorder):
> > +    """Generate the iterator of .cmd files associated with the modules.order.
> > +
> > +    Parse the given modules.order, and yield every .cmd file used to build the
> > +    contained modules.
> > +
> > +    Args:
> > +        modorder: The modules.order file to parse
> > +
> > +    Yields:
> > +        The path to every .cmd file found
> > +    """
> > +    with open(modorder) as f:
> > +        for line in f:
> > +            ko = line.rstrip()
> > +            base, ext = os.path.splitext(ko)
>
> below in main() you check the file extension with endswith().  Would
> it be good to be consistent between the two?

I want to re-use 'base' to convert
the *.ko into *.mod

path/to/my/driver.ko
-> path/to/my/driver.mod


I think using os.path.split()
is good for checking the valid suffix,
and replaceing it with '.mod'.






> > +            if ext != '.ko':
> > +                sys.exit('{}: module path must end with .ko'.format(ko))
> > +            mod = base + '.mod'
> > +           # The first line of *.mod lists the objects that compose the module.
> > +            with open(mod) as m:
> > +                for obj in m.readline().split():
> > +                    yield to_cmdfile(obj)
> > +
> > +
> >  def process_line(root_directory, command_prefix, file_path):
> >      """Extracts information from a .cmd line and creates an entry from it.
> >
> > @@ -116,7 +194,7 @@ def process_line(root_directory, command_prefix, file_path):
> >
> >  def main():
> >      """Walks through the directory and finds and parses .cmd files."""
> > -    log_level, directory, output, paths = parse_arguments()
> > +    log_level, directory, output, ar, paths = parse_arguments()
> >
> >      level = getattr(logging, log_level)
> >      logging.basicConfig(format='%(levelname)s: %(message)s', level=level)
> > @@ -126,7 +204,21 @@ def main():
> >      compile_commands = []
> >
> >      for path in paths:
> > -        cmdfiles = cmdfiles_in_dir(path)
> > +        # If 'path' is a directory, handle all .cmd files under it.
> > +        # Otherwise, handle .cmd files associated with the file.
> > +        # Most of built-in objects are linked via archives (built-in.a or lib.a)
> > +        # but some are linked to vmlinux directly.
> > +        # Modules are lis
>
> ^ was this comment cut off?

Oops, I will fix it.



-- 
Best Regards
Masahiro Yamada

Powered by blists - more mailing lists