RFR: 8210459: Add support for generating compile_commands.json

Thu Oct 4 11:38:59 UTC 2018

> 4 okt. 2018 kl. 10:33 skrev Robin Westberg <robin.westberg at oracle.com>:
> 
> Hi Erik,
> 
>> On 3 Oct 2018, at 20:51, Erik Joelsson <erik.joelsson at oracle.com> wrote:
>> 
>> Hello Robin,
>> 
>> make/CompileCommands.gmk: typo in comment: “prepened"
> 
> Fixed. (Couldn’t resist a slight rewording so the text has reflowed a bit, sorry!)

Much better written, thank you!

> 
>>> On 2018-10-03 11:09, Robin Westberg wrote:
>>> Hi Magnus,
>>> 
>>> Thanks for reviewing, sorry for taking a while to respond!
>>> 
>>>> On 19 Sep 2018, at 13:02, Magnus Ihse Bursie <magnus.ihse.bursie at oracle.com> wrote:
>>>> 
>>>> 19 sep. 2018 kl. 09:51 skrev Magnus Ihse Bursie <magnus.ihse.bursie at oracle.com>:
>>>> 
>>>>>>>>> In Awt2dLibraries.gmk, I think lines 733 and 735 needs to be fixed as well. Using FindLib is generally a good idea for this. I suspect there may be more such instances sprinkled around the makefiles.
>>>>>>>> Only fixed the last one, I think the first one is ok as is?
>>>>>>> The first one is sort of OK, but it's a questionable construct as the $(BUILD_LIBAWT_XAWT) variable may contain additional targets. We used to do it that way but these days I prefer the more explicit and precise FindLib. In this particular case you get a non needed dependency added when running compile-commands with ENABLE_HEADLESS_ONLY=true, which will not really affect anything so it doesn't really matter.
>>>>>> Yeah I certainly agree, but a quick grep shows that there’s about 50 such constructs present right now. I wouldn’t mind cleaning those up, but perhaps that should be in a separate change?
>>>>> If that does not affect your patch, you do not need to clean up those constructs.
>>>>> 
>>>>> I've now looked through your patch. Overall, it looks good. Some minor comments:
>>>>> 
>>>>> * In  make/CompileCommands.gmk, are you sure the find -exec + construct does not exceed command line limits on problematic platforms such as Solaris and Windows?
>>> It runs successfully on Windows and Solaris right now at least, and the filenames only include a relative path, so should not be sensitive to working directory location. But I have to admit I’m not sure how close to the limit it is right now.. Perhaps I can build something around “xargs -s” instead if you think this is risky?
>> I think it would be good to make sure the file list is split using xargs to avoid weird failures in the future. I also just realized it would be good to sort the file list to guarantee stable output.
> 
> Makes sense, changed it to use sort and xargs.

Much better, thanks!

> 
> Webrev (full): http://cr.openjdk.java.net/~rwestberg/8210459/webrev.04/

I was about to say that this looks good, but then I discovered a weird thing. I'm the SED line, line 50, there is a weird C-like Unicode character instead of a quote, after -e. Looks really odd. Is it a "smart quote" gone wrong? Can you look into it and fix it? You don't need to respin the webrev for that. Otherwise, you're all good to go. 

/Magnus

> Webrev (incremental): http://cr.openjdk.java.net/~rwestberg/8210459/webrev.03-04/
> 
> Best regards,
> Robin
> 
>> 
>> Otherwise I think this is good now.
>> 
>> /Erik
>>>>> The AWT constructs also confuses me. Maybe you can expand a bit on the comment, because it really is non-trivial. You are executing a cp for each tmpfile you find? But what if you find multiple tmpfiles? There could certainly be multiple commands using @ constructs?
>>> What it does it actually to invoke fixpath on everything inside the final compile_commands.json file, but in order to make fixpath do that, it presents the compile_commands.json file as an @-argument. Fixpath will then translate that argument to a temporary filename containing corrected paths, and that’s the file we save (since it’s deleted when the invoked command terminates). I’ve updated the comment a bit, hopefully it’s more clear now.
>>> 
>>> Another option would be to extend the fixpath utility itself to support some additional argument to just do inline path correction on a given file, but I felt that it would be a more risky change.
>>> 
>>>>> * In make/Main.gmk, you can just do "($(CD) $(OUTPUTDIR) && $(RM) -r build*.log* compile_commands.json)" on a single line.
>>> Fixed.
>>> 
>>>>> * In make/common/MakeBase.gmk: I'd prefer if these functions were move to make/common/JdkNativeCompilation.gmk, close to the FindSrcDirsForLib etc definitions.
>>> Fixed.
>>> 
>>>>> * In make/common/NativeCompilation.gmk, I'm not entirely happy with the $1_OBJ_COMMON_OPTIONS_PRE/POST construct. I understand what you are trying to achieve, but I think this gets very hard to read. I don't have a perfect solution in mind. But perhaps something like this:
>>>>> $1_DEPS_OPTIONS := $$($1_DEP_FLAG) $$($1_DEP).tmp
>>>>> $1_COMPILE_COMMAND := $$($1_COMPILER) $$($1_FLAGS) $$($1_DEPS_OPTIONS) $(CC_OUT_OPTION)$$($1_OBJ) $$($1_SRC_FILE))
>>>>> 
>>>>> and for the compile commands, use $$(filter-out $$($1_DEPS_OPTIONS), $$($1_COMPILE_COMMAND)).
>>>>> 
>>>>> Perhaps some unification with the Microsoft toolchain is possible by setting $1_DEPS_OPTIONS to -showIncludes.
>>>>> 
>>>> An alternative, perhaps better, is to move the deps flag to the start. Then you could do something like the above, but set
>>>> 
>>>> $1_COMPILE_OPTIONS := $$($1_FLAGS) $(CC_OUT_OPTION)$$($1_OBJ) $$($1_SRC_FILE))
>>>> and when compiling do this:
>>>> $$($1_COMPILER)  $$($1_DEPS_OPTIONS)  $$($1_COMPILE_OPTIONS)
>>>> 
>>>> That would get rid of the filter-out, keep duplication low but still be readable.
>>>> 
>>>> This assumes that ordering for the deps option is irrelevant. I think it is but it would have to be tested.
>>> Sounds good, fixed.
>>> 
>>>> /Magnus
>>>> 
>>>>> Erik, what do you think?
>>>>> 
>>>>> * In make/lib/Lib-jdk.accessibility.gmk, this seems double incorrect. You are missing a libjawt path segment. And you wanted to use FindStaticLib.
>>> Ah, good catch, I suspect it still works as the jawt.lib file in the modules_lib folder is dependent on jawt.lib in the native folder. Fixed.
>>> 
>>>>> Overall, I believe we're misusing the "static lib" concept on Windows. The .lib files are not static libs, the way we mean it on unixes. Instead, the lib files is some kind of metadata on dll that are used when linking with those dlls. So we should introduce a better concept for these, and maybe something like FindLibDescriptor (or whatever). That should not really be part of this fix, though, so for the moment I'm going to accept that we call these "static libs" on Windows.
>>>>> 
>>>>> This also makes me wonder how much testing this patch has recieved? I know a broken dependency in the worst case only shows up as a race, but have you verified it thoroughly even on Windows? And even without compile_commands?
>>> What I’ve been doing, apart from making sure tier1 tests and tier5-builds for all platforms still work, is to “diff” the .cmdline files from a build of “make jdk” with and without the patch applied (and with --with-version-opt= set during configure to avoid the timestamp). The only difference so far is that the EXTRA_OBJECT_FILES change for the make/launcher/Launcher-jdk.pack.gmk moves the files from the command line to an @file, but the contents are the same. (Had to apply a bit of sed to perform this verification after reordering the dependency flags, but still looks correct).
>>> 
>>> This obviously won’t catch any subtle dependency mistakes though, not sure if there’s much to be done about that though unfortunately.
>>> 
>>> Webrev (full): http://cr.openjdk.java.net/~rwestberg/8210459/webrev.03/
>>> Webrev (incremental): http://cr.openjdk.java.net/~rwestberg/8210459/webrev.02-03/
>>> 
>>> Best regards,
>>> Robin
>>> 
>>>>> /Magnus
>>>>> 
>>>>>>> Otherwise this looks good now.
>>>>>> Thanks, I’ll include the latest webrevs with a comment added:
>>>>>> 
>>>>>> Incremental: http://cr.openjdk.java.net/~rwestberg/8210459/webrev.01-02/
>>>>>> Full: http://cr.openjdk.java.net/~rwestberg/8210459/webrev.02/
>>>>>> 
>>>>>> Best regards,
>>>>>> Robin
>>>>>> 
>>>>>>> /Erik
>>>>>>>> Webrev (incremental): http://cr.openjdk.java.net/~rwestberg/8210459/webrev.00-01/
>>>>>>>> Webrev (full): http://cr.openjdk.java.net/~rwestberg/8210459/webrev.01/
>>>>>>>> Testing: tier1, builds-tier5
>>>>>>>> 
>>>>>>>> Best regards,
>>>>>>>> Robin
>>>>>>>> 
>>>>>>>>> /Erik
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>>> On 2018-09-10 06:36, Robin Westberg wrote:
>>>>>>>>>> Hi all,
>>>>>>>>>> 
>>>>>>>>>> Please review the following change that adds support for generating compile_commands.json as a top-level make target. This is a popular format for describing how to compile object files for a project, and is defined in [1]. This file can be used directly by IDEs such as Visual Studio Code and CLion as well as "language servers" such as cquery [2] and rtags [3].
>>>>>>>>>> 
>>>>>>>>>> While it’s currently possible to generate this file as part of a full build (using tools such as Bear [4], or simply parsing .cmdline files), this change aims to make it possible to generate the compile commands data without actually compiling the object files. This means that it’s for example possible to start using an IDE fairly quickly on freshly cloned source, instead of having to wait for a full build to complete.
>>>>>>>>>> 
>>>>>>>>>> This was originally inspired by the work done in [5] by Mikael Gerdin and Erik Joelsson, but has been through a few revisions since then. Erik has also provided a lot of assistance with the current version, thanks Erik!
>>>>>>>>>> 
>>>>>>>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8210459
>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~rwestberg/8210459/webrev.00/
>>>>>>>>>> Testing: tier1, builds-tier5
>>>>>>>>>> 
>>>>>>>>>> Best regards,
>>>>>>>>>> Robin
>>>>>>>>>> 
>>>>>>>>>> [1] https://clang.llvm.org/docs/JSONCompilationDatabase.html
>>>>>>>>>> [2] https://github.com/cquery-project/cquery
>>>>>>>>>> [3] https://github.com/Andersbakken/rtags
>>>>>>>>>> [4] https://github.com/rizsotto/Bear
>>>>>>>>>> [5] http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-March/026354.html
> 