Re: [PATCH v2] gen_compile_commands: fix overlooked module files

[John Hubbard <jhubbard@nvidia.com>]

On 6/19/22 17:15, Masahiro Yamada wrote:
> On Thu, Jun 16, 2022 at 11:12 AM John Hubbard <jhubbard@nvidia.com> wrote:
>>
>> scripts/clang-tools/gen_compile_commands.py incorrectly assumes that
>> each .mod file only contains one line. In fact, such files contain one
>> entry per line, and for some subsystems, there can be many, many lines.
>> For example, Nouveau has 762 entries, but only the first entry was being
>> processed. This problem causes clangd to fail to provide references and
>> definitions for valid files that are part of the current kernel
>> configuration.
> 
> 
> My previous comment "Can you update the commit log?"
> meant, "Can you rewrite the commit log (in a more concise way)?".
> 
> 
> 
> I am not sure the Nouveau example is needed here, but
> more important information is that
> ecca4fea1ede4 changed the .mod file
> from "all entries at the first line" to "one entry per line".
> 
> 
> 
> 
>> This problem only occurs when using Kbuild to generate, like this:
>>
>>     make CC=clang compile_commands.json
>>
>> It does not occur if you just run gen_compile_commands.py "bare", like
>> this (below):
>>
>>     scripts/clang-tools/gen_compile_commands.py/gen_compile_commands.py .
> 
> I am not sure if this is needed, but at least, the command line is wrong.
> 
> scripts/clang-tools/gen_compile_commands.py/gen_compile_commands.py .
>      -->
> scripts/clang-tools/gen_compile_commands.py .

Good catch, not sure how that typo leaked in!

> 
> 
>> Fix this by fully processing each .mod file. This fix causes the number
>> of build commands that clangd finds in my kernel build (these numbers
>> are heavily dependent upon .config), from 2848 to 5292, which is an 85%
>> increase.
> 
> I am not sure if the second sentence is needed
> because this patch is just an obvious fix.
> 
> 
> 
> Sorry, I missed to adjust this file in ecca4fea1ede4,
> and ended up bothering you too much.
> 

I'll have a go at rewriting the commit description, sure.

> 
> 
>> Fixes: 9413e7640564 ("kbuild: split the second line of *.mod into *.usyms")
>> Cc: Masahiro Yamada <masahiroy@kernel.org>
>> Cc: Nick Desaulniers <ndesaulniers@google.com>
>> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
>> ---
>>
>> Changes since v1:
>>
>> Applied changes recommended by Masahiro Yamada (thanks!): corrected the
>> "Fixes" tag, and improved the python code for parsing .mod files.
>>
>>
>>   scripts/clang-tools/gen_compile_commands.py | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/scripts/clang-tools/gen_compile_commands.py b/scripts/clang-tools/gen_compile_commands.py
>> index 1d1bde1fd45e..acf4ec28aaf1 100755
>> --- a/scripts/clang-tools/gen_compile_commands.py
>> +++ b/scripts/clang-tools/gen_compile_commands.py
>> @@ -157,11 +157,10 @@ def cmdfiles_for_modorder(modorder):
>>               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.
>> +            # Read from *.mod, to get a list of objects that compose the module.
>>               with open(mod) as m:
>> -                for obj in m.readline().split():
>> -                    yield to_cmdfile(obj)
>> -
>> +                for mod_line in m:
>> +                    yield to_cmdfile(mod_line.rstrip())
>>
> 
> 
> Please do not remove the blank line.

Sure, I didn't really intend to change the vertical whitespace. I'll restore that.

I'll send out a v3 shortly.

thanks,
-- 
John Hubbard
NVIDIA