Re: [PATCH] gen_compile_commands: Add support for separate kbuild output directory

[Masahiro Yamada]

(+cc clang-built-linux)


On Sat, Aug 1, 2020 at 6:21 AM Peter Kalauskas <peskal@xxxxxxxxxx> wrote:
>
> Add support for builds that use an output directory different than the
> kernel source tree (e.g. make O=/tmp/kernel-obj). This also introduces
> support for .cmd files that use absolute paths.
>
> Previously, gen_compile_commands.py only supported builds where the
> kernel source tree and the output directory were the same:
>  $ make defconfig
>  $ make -j32
>  $ ./scripts/gen_compile_commands.py
>
> gen_compile_commands.py had flags to support out of tree use, but the
> generated compile_commands.json file still assumed that the source tree
> and kbuild output directory were the same.

Thanks Tom for CCing Kbuild ML.
Otherwise, I would not have noticed this patch.



I think the current code _mostly_ supports the out of tree use.
The key is to specify '-p <build-dir>'
when you use compile_commands.json from
clang-check, clang-tidy, etc.





> Now, the following cases are supported as well:
>
>  - Absolute output path:
>    $ mkdir /tmp/kernel-obj
>    $ make O=/tmp/kernel-obj defconfig
>    $ make O=/tmp/kernel-obj -j32
>    $ ./scripts/gen_compile_commands.py -k /tmp/kernel-obj
>
>  - Relative output path:
>    $ mkdir kernel-obj
>    $ make O=kernel-obj/ defconfig
>    $ make O=kernel-obj/ -j32
>    $ ./scripts/gen_compile_commands.py -k kernel-obj


In the current script, I would do like follows:

- Absolute output path:
$ export CC=clang
$ make O=/tmp/kernel-obj defconfig
$ make O=/tmp/kernel-obj -j32
$ ./scripts/gen_compile_commands.py -d /tmp/kernel-obj
$ clang-tidy  '--checks=linuxkernel*'  -p /tmp/kernel-obj kernel/fork.c


- Relative output path:
$ export CC=clang
$ make O=kernel-obj defconfig
$ make O=kernel-obj -j32
$ ./scripts/gen_compile_commands.py -d kernel-obj
$ clang-tidy  '--checks=linuxkernel*'  -p kernel-obj   kernel/fork.c



With your patch and the -k option use,
compile_commands.json is always created in the source tree
whether O= is given or not.
Then, you no longer need to pass '-p <build-path>'
from clang tools.

However, I like the workflow to create any build artifact
in the output tree for O= use-case, and keep the source tree
completely clean.
This is because the source tree might be read-only.
Perhaps, it is located under /usr/src/,
or the source code might be provided by DVD-ROM, etc.





In my understanding, the flaw of the -d option is,
it cannot handle objtool.

'--log_level DEBUG' emits error log for objtool.

masahiro@oscar:~/ref/linux$ ./scripts/gen_compile_commands.py  -d
/tmp/kernel-obj  --log_level  DEBUG
INFO: Could not add line from
/tmp/kernel-obj/tools/objtool/.elf.o.cmd: File elf.c not in
/tmp/kernel-obj or /tmp/kernel-obj/tools/objtool
INFO: Could not add line from
/tmp/kernel-obj/tools/objtool/.builtin-orc.o.cmd: File builtin-orc.c
not in /tmp/kernel-obj or /tmp/kernel-obj/tools/objtool
INFO: Could not add line from
/tmp/kernel-obj/tools/objtool/.builtin-check.o.cmd: File
builtin-check.c not in /tmp/kernel-obj or
/tmp/kernel-obj/tools/objtool
INFO: Could not add line from
/tmp/kernel-obj/tools/objtool/.orc_dump.o.cmd: File orc_dump.c not in
/tmp/kernel-obj or /tmp/kernel-obj/tools/objtool
INFO: Could not add line from
/tmp/kernel-obj/tools/objtool/.libstring.o.cmd: File ../lib/string.c
not in /tmp/kernel-obj or /tmp/kernel-obj/tools/objtool
INFO: Could not add line from
/tmp/kernel-obj/tools/objtool/.libctype.o.cmd: File ../lib/ctype.c not
in /tmp/kernel-obj or /tmp/kernel-obj/tools/objtool
INFO: Could not add line from
/tmp/kernel-obj/tools/objtool/.parse-options.o.cmd: File
parse-options.c not in /tmp/kernel-obj or
/tmp/kernel-obj/tools/objtool
INFO: Could not add line from
/tmp/kernel-obj/tools/objtool/.objtool.o.cmd: File objtool.c not in
/tmp/kernel-obj or /tmp/kernel-obj/tools/objtool
INFO: Could not add line from
/tmp/kernel-obj/tools/objtool/.fixdep.o.cmd: File fixdep.c not in
/tmp/kernel-obj or /tmp/kernel-obj/tools/objtool
INFO: Could not add line from
/tmp/kernel-obj/tools/objtool/.pager.o.cmd: File pager.c not in
/tmp/kernel-obj or /tmp/kernel-obj/tools/objtool
INFO: Could not add line from
/tmp/kernel-obj/tools/objtool/.str_error_r.o.cmd: File
../lib/str_error_r.c not in /tmp/kernel-obj or
/tmp/kernel-obj/tools/objtool
INFO: Could not add line from
/tmp/kernel-obj/tools/objtool/.special.o.cmd: File special.c not in
/tmp/kernel-obj or /tmp/kernel-obj/tools/objtool
INFO: Could not add line from
/tmp/kernel-obj/tools/objtool/.orc_gen.o.cmd: File orc_gen.c not in
/tmp/kernel-obj or /tmp/kernel-obj/tools/objtool
INFO: Could not add line from
/tmp/kernel-obj/tools/objtool/.help.o.cmd: File help.c not in
/tmp/kernel-obj or /tmp/kernel-obj/tools/objtool
INFO: Could not add line from
/tmp/kernel-obj/tools/objtool/.librbtree.o.cmd: File ../lib/rbtree.c
not in /tmp/kernel-obj or /tmp/kernel-obj/tools/objtool
INFO: Could not add line from
/tmp/kernel-obj/tools/objtool/.run-command.o.cmd: File run-command.c
not in /tmp/kernel-obj or /tmp/kernel-obj/tools/objtool
INFO: Could not add line from
/tmp/kernel-obj/tools/objtool/.exec-cmd.o.cmd: File exec-cmd.c not in
/tmp/kernel-obj or /tmp/kernel-obj/tools/objtool
INFO: Could not add line from
/tmp/kernel-obj/tools/objtool/.weak.o.cmd: File weak.c not in
/tmp/kernel-obj or /tmp/kernel-obj/tools/objtool
INFO: Could not add line from
/tmp/kernel-obj/tools/objtool/.subcmd-config.o.cmd: File
subcmd-config.c not in /tmp/kernel-obj or
/tmp/kernel-obj/tools/objtool
INFO: Could not add line from
/tmp/kernel-obj/tools/objtool/.check.o.cmd: File check.c not in
/tmp/kernel-obj or /tmp/kernel-obj/tools/objtool
INFO: Could not add line from
/tmp/kernel-obj/tools/objtool/.sigchain.o.cmd: File sigchain.c not in
/tmp/kernel-obj or /tmp/kernel-obj/tools/objtool
INFO: Could not add line from
/tmp/kernel-obj/tools/objtool/arch/x86/.decode.o.cmd: File
arch/x86/decode.c not in /tmp/kernel-obj or
/tmp/kernel-obj/tools/objtool/arch/x86



Your patch solves it, but I wonder if it is worth lots of code addition.
objtool is the only unfortunate case because it did not join Kbuild.




> The new argument, -k, is introduced in a way that makes the script
> backward compatible with how -d was previously used.
>
> Signed-off-by: Peter Kalauskas <peskal@xxxxxxxxxx>
> ---



> -def process_line(root_directory, file_directory, command_prefix, relative_path):
> +def process_line(src_dir, kbuild_out_dir, file_dir, cmd_prefix, file_path):
>      """Extracts information from a .cmd line and creates an entry from it.
>
>      Args:
> -        root_directory: The directory that was searched for .cmd files. Usually
> +        src_dir: The directory of the kernel source tree.
> +        kbuild_out_dir: The directory that was searched for .cmd files. Usually
>              used directly in the "directory" entry in compile_commands.json.
> -        file_directory: The path to the directory the .cmd file was found in.
> -        command_prefix: The extracted command line, up to the last element.
> -        relative_path: The .c file from the end of the extracted command.
> -            Usually relative to root_directory, but sometimes relative to
> -            file_directory and sometimes neither.
> +        file_dir: The path to the directory the .cmd file was found in.
> +        cmd_prefix: The extracted command line, up to the last element.
> +        file_path: The .c file from the end of the extracted command.
> +            Usually relative to kbuild_out_dir, but sometimes relative to
> +            src_dir and sometimes neither.
>
>      Returns:
>          An entry to append to compile_commands.
>
>      Raises:
> -        ValueError: Could not find the extracted file based on relative_path and
> -            root_directory or file_directory.
> +        ValueError: Could not find the extracted file.
>      """
>      # The .cmd files are intended to be included directly by Make, so they
>      # escape the pound sign '#', either as '\#' or '$(pound)' (depending on the
> -    # kernel version). The compile_commands.json file is not interepreted
> +    # kernel version). The compile_commands.json file is not interpreted
>      # by Make, so this code replaces the escaped version with '#'.
> -    prefix = command_prefix.replace('\#', '#').replace('$(pound)', '#')
> -
> -    cur_dir = root_directory
> -    expected_path = os.path.join(cur_dir, relative_path)
> -    if not os.path.exists(expected_path):
> -        # Try using file_directory instead. Some of the tools have a different
> -        # style of .cmd file than the kernel.
> -        cur_dir = file_directory
> -        expected_path = os.path.join(cur_dir, relative_path)
> +    prefix = cmd_prefix.replace('\#', '#').replace('$(pound)', '#')
> +
> +    # Compile commands are usually run in the top level of the kbuild output
> +    # directory
> +    working_dir = kbuild_out_dir
> +
> +    if os.path.isabs(file_path):

I might be misreading the code, but
is this if-else switch needed?


os.path.join() returns the last parameter as-is
if it is already absolute, right?


This is my quick experiment:

masahiro@oscar:~$ python3
Python 3.8.2 (default, Jul 16 2020, 14:00:26)
[GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.path.join("/foo/bar/", "a/b/c")
'/foo/bar/a/b/c'
>>> os.path.join("/foo/bar/", "/a/b/c")
'/a/b/c'




So, the current code:

   expected_path = os.path.join(cur_dir, relative_path)

works whether 'relative_path' is relative or not.

I was also thinking this variable name was misleading
since 'relative_path' may be actually absolute.





--
Best Regards

Masahiro Yamada