RFR: 8210459: Add support for generating compile_commands.json

Wed Sep 19 14:47:51 UTC 2018

On 2018-09-19 04:02, Magnus Ihse Bursie wrote:
>
>
> 19 sep. 2018 kl. 09:51 skrev Magnus Ihse Bursie 
> <magnus.ihse.bursie at oracle.com <mailto:magnus.ihse.bursie at oracle.com>>:
>
>>
>>
>> On 2018-09-19 09:25, Robin Westberg wrote:
>>> Hi Erik,
>>>
>>>> On 14 Sep 2018, at 19:21, Erik Joelsson <erik.joelsson at oracle.com 
>>>> <mailto:erik.joelsson at oracle.com>> wrote:
>>>>
>>>>>> Main.gmk:888: I think you meant to add test-compile-commands 
>>>>>> here, which also requires you to define that target.
>>>>> Fixed, the tests are now invoked from a top-level target. I also 
>>>>> added a second test to ensure that the no-build-libraries property 
>>>>> of this actually stays intact.
>>>> This is ok, but the test will fail unless run from a clean output 
>>>> dir. This should probably be noted in a comment at least.
>>> Yes, good point, I tried briefly to include a clean as a 
>>> prerequisite target to the test, but it refused to execute 
>>> sequentially. But perhaps it’s good enough with a comment, added one.
>>
>> Yes, the clean target is treated specially before entering Main.gmk, 
>> to ensure commands like "make clean all" gets executed correctly. A 
>> comment is enough, I think.
>>
>>>
>>>>>> 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?
>>
>> 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?
>>
>> * In make/Main.gmk, you can just do "($(CD) $(OUTPUTDIR) && $(RM) -r 
>> build*.log* compile_commands.json)" on a single line.
>>
>> * In make/common/MakeBase.gmk: I'd prefer if these functions were 
>> move to make/common/JdkNativeCompilation.gmk, close to the 
>> FindSrcDirsForLib etc definitions.
>>
>> * 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.
I think that sounds good. The ordering of deps options should certainly 
be irrelevant.

/Erik
> /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.
>>
>> 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?
>>
>> /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/ 
>>> <http://cr.openjdk.java.net/%7Erwestberg/8210459/webrev.01-02/>
>>> Full: http://cr.openjdk.java.net/~rwestberg/8210459/webrev.02/ 
>>> <http://cr.openjdk.java.net/%7Erwestberg/8210459/webrev.02/>
>>>
>>> Best regards,
>>> Robin
>>>
>>>> /Erik
>>>>> Webrev (incremental): 
>>>>> http://cr.openjdk.java.net/~rwestberg/8210459/webrev.00-01/ 
>>>>> <http://cr.openjdk.java.net/%7Erwestberg/8210459/webrev.00-01/>
>>>>> Webrev (full): 
>>>>> http://cr.openjdk.java.net/~rwestberg/8210459/webrev.01/ 
>>>>> <http://cr.openjdk.java.net/%7Erwestberg/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/ 
>>>>>>> <http://cr.openjdk.java.net/%7Erwestberg/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
>>