[PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows

[Manuel Klimek via cfe-commits]

For years nobody worked on Windows support, so I'm somewhat surprised this
is suddenly time critical.
Usually the way this works is that we add the feature to upstream cmake,
and then make a recent enough cmake a requirement for tooling. There's no
need to make it a requirement for anything else. That has worked well for
the initial version, too.

My main problem is that I'm surprised you say you got a working version at
all, given all the differences. I'm on vacation, but afterwards I'm happy
to look up who previously worked on this and loop them into this thread. Or
you can figure out who that was (they added the arguments support iirc) and
make sure they're signing of on this.


On Sun, Aug 14, 2016, 8:52 AM Zachary Turner <zturner at google.com> wrote:

> According to the existing spec [
> http://clang.llvm.org/docs/JSONCompilationDatabase.html], the command
> field "must be a valid command to rerun the exact compilation step for the
> translation unit in the environment the build system uses.".
>
> So copying compilation databases across environments with incompatible
> command line syntaxes is already against spec.
>
> That said, this does make me think that perhaps we should be checking the
> target triple instead of the host compilation environment, because if you
> were to cross compile clang-tidy for Windows from linux then try to use
> that clang-tidy on Windows, it would expect unix paths.
>
> How does that sound?
> On Sat, Aug 13, 2016 at 10:31 PM Zachary Turner <zturner at google.com>
> wrote:
>
>> Also, compilation database support with CMake works correctly on Windows.
>> It generates Windows command lines which need to be tokenized using Windows
>> command line rules (hence why this patch makes clang-tidy work on Windows)
>> On Sat, Aug 13, 2016 at 10:30 PM Zachary Turner <zturner at google.com>
>> wrote:
>>
>>> I'm not disagreeing that it would be nice if CMake supported this. It's
>>> probably worth trying to do even.
>>>
>>> But do we want to block having a working clang-tidy for clang-cl for
>>> months because of that? Even if we can upstream the patch, how long before
>>> we up the minimum required CMake version?
>>>
>>> The solution here does not regress behavior on any supported
>>> configuration, and adds support for a new platform. Copying a compilation
>>> database from one machine to another seems like a hypothetical edge case.
>>>
>>> To support testing perhaps we can make our compilation database parser
>>> support an optional field in the json that CMake doesn't know about like
>>> PathSyntax: 'windows'. Again, this seems like something we could do
>>> iteratively and with low priority because it's not needed in order to
>>> enable or fix any presently supported use cases.
>>> On Sat, Aug 13, 2016 at 9:58 PM Manuel Klimek <klimek at google.com> wrote:
>>>
>>>>
>>>>
>>>> On Sat, Aug 13, 2016, 10:17 PM Zachary Turner <zturner at google.com>
>>>> wrote:
>>>>
>>>>> The json is generated by CMake, so I don't thinks we can do this
>>>>> without patching CMake, which is a non-starter.
>>>>>
>>>>
>>>> Why? We did add compilation database support to cmake in the first
>>>> place. Back then I did not support windows, btw, so I'd be surprised if
>>>> that worked now without also using the arguments format that has already
>>>> been added to the spec for that reason.
>>>>
>>>> I don't think this will break mingw. Mingw is still Windows, and still
>>>>> uses Windows backslashes, quoting rules, and escaping rules.
>>>>>
>>>>> You might be thinking of cygwin, but in the case LLVM_ON_WIN32is not
>>>>> defined.
>>>>>
>>>>> Reid, do you agree with this?
>>>>>
>>>>
>>>> I'd like to see a stronger reason why adding arguments support to cmake
>>>> is not the right thing to do.
>>>>
>>>>
>>>>> On Sat, Aug 13, 2016 at 10:35 AM Alexander Kornienko <
>>>>> alexfh at google.com> wrote:
>>>>>
>>>>>> alexfh added inline comments.
>>>>>>
>>>>>> ================
>>>>>> Comment at: lib/Tooling/JSONCompilationDatabase.cpp:119
>>>>>> @@ -115,1 +118,3 @@
>>>>>>      StringRef EscapedCommandLine) {
>>>>>> +#if defined(LLVM_ON_WIN32)
>>>>>> +  llvm::BumpPtrAllocator Alloc;
>>>>>> ----------------
>>>>>> As noted on D23409, this will likely break mingw compatibility in
>>>>>> clang on windows. We should either add a sort of auto-detection of the
>>>>>> command line format, or extend the JSON compilation database with a way to
>>>>>> specify the command line format used (or as Reid suggests, specify a list
>>>>>> of arguments, but this should be done in a backward-compatible way).
>>>>>>
>>>>>>
>>>>>> https://reviews.llvm.org/D23455
>>>>>>
>>>>>>
>>>>>>
>>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160814/55060f62/attachment.html>