Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
This revision was automatically updated to reflect the committed changes.
Closed by commit rL278964: [Tooling] Parse compilation database command lines
on Windows. (authored by zturner).
Changed prior to commit:
https://reviews.llvm.org/D23455?vs=67947&id=68407#toc
Repository:
rL LLVM
https://reviews.llvm.org/D23455
Files:
cfe/trunk/lib/Tooling/JSONCompilationDatabase.cpp
Index: cfe/trunk/lib/Tooling/JSONCompilationDatabase.cpp
===
--- cfe/trunk/lib/Tooling/JSONCompilationDatabase.cpp
+++ cfe/trunk/lib/Tooling/JSONCompilationDatabase.cpp
@@ -16,7 +16,10 @@
#include "clang/Tooling/CompilationDatabasePluginRegistry.h"
#include "clang/Tooling/Tooling.h"
#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/Allocator.h"
+#include "llvm/Support/CommandLine.h"
#include "llvm/Support/Path.h"
+#include "llvm/Support/StringSaver.h"
#include
namespace clang {
@@ -113,6 +116,21 @@
std::vector unescapeCommandLine(
StringRef EscapedCommandLine) {
+ llvm::Triple Triple(llvm::sys::getProcessTriple());
+ if (Triple.getOS() == llvm::Triple::OSType::Win32) {
+// Assume Windows command line parsing on Win32 unless the triple
explicitly
+// tells us otherwise.
+if (!Triple.hasEnvironment() ||
+Triple.getEnvironment() == llvm::Triple::EnvironmentType::MSVC) {
+ llvm::BumpPtrAllocator Alloc;
+ llvm::StringSaver Saver(Alloc);
+ llvm::SmallVector T;
+ llvm::cl::TokenizeWindowsCommandLine(EscapedCommandLine, Saver, T);
+ std::vector Result(T.begin(), T.end());
+ return Result;
+}
+ }
+
CommandLineArgumentParser parser(EscapedCommandLine);
return parser.parse();
}
Index: cfe/trunk/lib/Tooling/JSONCompilationDatabase.cpp
===
--- cfe/trunk/lib/Tooling/JSONCompilationDatabase.cpp
+++ cfe/trunk/lib/Tooling/JSONCompilationDatabase.cpp
@@ -16,7 +16,10 @@
#include "clang/Tooling/CompilationDatabasePluginRegistry.h"
#include "clang/Tooling/Tooling.h"
#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/Allocator.h"
+#include "llvm/Support/CommandLine.h"
#include "llvm/Support/Path.h"
+#include "llvm/Support/StringSaver.h"
#include
namespace clang {
@@ -113,6 +116,21 @@
std::vector unescapeCommandLine(
StringRef EscapedCommandLine) {
+ llvm::Triple Triple(llvm::sys::getProcessTriple());
+ if (Triple.getOS() == llvm::Triple::OSType::Win32) {
+// Assume Windows command line parsing on Win32 unless the triple explicitly
+// tells us otherwise.
+if (!Triple.hasEnvironment() ||
+Triple.getEnvironment() == llvm::Triple::EnvironmentType::MSVC) {
+ llvm::BumpPtrAllocator Alloc;
+ llvm::StringSaver Saver(Alloc);
+ llvm::SmallVector T;
+ llvm::cl::TokenizeWindowsCommandLine(EscapedCommandLine, Saver, T);
+ std::vector Result(T.begin(), T.end());
+ return Result;
+}
+ }
+
CommandLineArgumentParser parser(EscapedCommandLine);
return parser.parse();
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
alexfh added a comment. In https://reviews.llvm.org/D23455#518317, @zturner wrote: > We could also provide a command line option to clang-tidy that lets the user > specify the command line syntax of the compilation database. It could choose > a smart default (i.e. what we do in this patch after using > `llvm::sys::getProcessTriple()`) but if the command line option is specified > it could just force that parsing mode. This way we don't have to get in the > business of guessing, which could itself end up having unforeseen edge cases. If someone is actually interested in this, the option can be added to `CommonOptionsParser`. https://reviews.llvm.org/D23455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.
In https://reviews.llvm.org/D23455#518312, @rnk wrote:
> So, I actually went ahead and generated some MSYS makefiles and made a
> compile_commands.json, and it doesn't work with clang-tidy. You get this kind
> of output:
>
> [
> {
> "directory": "C:/src/test_proj",
> "command": "\"/C/Program
> Files/mingw-w64/x86_64-6.1.0-win32-seh-rt_v5-rev0/mingw64/bin/g++.exe\"
> -Dsomething\\evil -o CMakeFiles/foo.dir/foo.obj -c /C/src/test_proj/foo.cpp",
> "file": "C:/src/test_proj/foo.cpp"
> }
> ]
> ...
> $ clang-tidy foo.cpp
> 1 error generated.
> Error while processing C:\src\test_proj\foo.cpp.
> error: error reading '/C/src/test_proj/foo.cpp' [clang-diagnostic-error]
>
>
> Hypothetically we could make this work, but there are bigger problems here.
> In that light, I think we should go with this patch. Long term, we should
> solve this problem by emitting "arguments" or "command_shell" in cmake.
Yep, if we're not breaking anything with this change, let's proceed. LG
https://reviews.llvm.org/D23455
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
zturner added a comment. We could also provide a command line option to clang-tidy that lets the user specify the command line syntax of the compilation database. It could choose a smart default (i.e. what we do in this patch after using `llvm::sys::getProcessTriple()`) but if the command line option is specified it could just force that parsing mode. This way we don't have to get in the business of guessing, which could itself end up having unforeseen edge cases. https://reviews.llvm.org/D23455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
rnk added a comment.
So, I actually went ahead and generated some MSYS makefiles and made a
compile_commands.json, and it doesn't work with clang-tidy. You get this kind
of output:
[
{
"directory": "C:/src/test_proj",
"command": "\"/C/Program
Files/mingw-w64/x86_64-6.1.0-win32-seh-rt_v5-rev0/mingw64/bin/g++.exe\"
-Dsomething\\evil -o CMakeFiles/foo.dir/foo.obj -c /C/src/test_proj/foo.cpp",
"file": "C:/src/test_proj/foo.cpp"
}
]
...
$ clang-tidy foo.cpp
1 error generated.
Error while processing C:\src\test_proj\foo.cpp.
error: error reading '/C/src/test_proj/foo.cpp' [clang-diagnostic-error]
Hypothetically we could make this work, but there are bigger problems here. In
that light, I think we should go with this patch. Long term, we should solve
this problem by emitting "arguments" or "command_shell" in cmake.
https://reviews.llvm.org/D23455
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
aaron.ballman added a comment. In https://reviews.llvm.org/D23455#517774, @alexfh wrote: > In https://reviews.llvm.org/D23455#516760, @zturner wrote: > > > In https://reviews.llvm.org/D23455#516753, @alexfh wrote: > > > > > In https://reviews.llvm.org/D23455#515527, @rnk wrote: > > > > > > > In https://reviews.llvm.org/D23455#515486, @brad.king wrote: > > > > > > > > > > the feasibility of emitting 'arguments' instead of 'command' into > > > > > > the JSON compilation database. > > > > > > > > > > > > > > > CMake constructs the command lines internally using string > > > > > replacement on templates. We never actually know the exact > > > > > arguments. Therefore providing arguments instead of the whole > > > > > command would require parsing to be done on the CMake side instead. > > > > > This is theoretically possible because we do know the shell for which > > > > > we are generating (Windows `cmd` versus MSYS `sh`). However, it may > > > > > also require a bunch of logic we don't have yet but that LLVM does. > > > > > > > > > > Alternatively, the JSON could have an additional > > > > > `command_shell="..."` field that indicates the shell for which the > > > > > command line is encoded. > > > > > > > > > > > > Bummer. Given that this is hard to do in CMake, then I think we should > > > > just tokenize in Clang. Let's use llvm::sys::getProcessTriple() instead > > > > of LLVM_ON_WIN32 and check if that is an MSVC environment as a proxy > > > > for the shell type. > > > > > > > > > Do I understand correctly that `llvm::sys::getProcessTriple()` returns > > > basically a compile-time constant? If yes, it won't allow the same clang > > > tool binary to be used with both command line formats. Should we instead > > > allow runtime selection of the command line format? For example, by > > > extending JSON compilation database with the aforementioned > > > `command_shell="..."` option. > > > > > > It does return a compile time constant, but it is a compile time constant > > representing the platform that clang-tidy is running on. I don't see a > > need to have a single clang-tidy binary be able to parse both command line > > formats. In fact, it seems actively harmful. > > > > If you are running on a Windows system with windows command line parsing > > rules, and someone has generated a command line on linux, then this command > > line was never intended to be run on Windows in the first place. The JSON > > compilation database spec already says that the command line represents > > "the exact command to be run **in the environment of the build system**". > > By adding the flexibility to change the environment, this is no longer > > true. If I try to run a command generated for one build system in the > > environment of another build system, even if I tokenize it correctly whose > > to say it will work? > > > > I understand the desire to make sure we get the right change so we don't > > have to revisit this in the future, but to me this sounds like YAGNI and > > actually increasing the risk of someone using the software and getting > > unexpected results, not less risk. > > > Now I'm less sure. And I might be misunderstanding something here. Folks > actually using clang tools on Windows (Aaron?) can tell with more confidence > whether Linux-style command line tokenization is of any use on Windows. I had > an impression that it allowed clang tools to be used with mingw (and msys > shell), but I'm not sure whether it's an important use case for anyone. I agree with @zturner's perspective -- if the command was generated on Linux, I would not expect it to work on Windows (and vice versa). Not only are path separators an issue, you run into other things like reserved file names, different -D flags, different triples, etc. I can't think of a situation where I would expect that to work. https://reviews.llvm.org/D23455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
alexfh added a comment. In https://reviews.llvm.org/D23455#516760, @zturner wrote: > In https://reviews.llvm.org/D23455#516753, @alexfh wrote: > > > In https://reviews.llvm.org/D23455#515527, @rnk wrote: > > > > > In https://reviews.llvm.org/D23455#515486, @brad.king wrote: > > > > > > > > the feasibility of emitting 'arguments' instead of 'command' into the > > > > > JSON compilation database. > > > > > > > > > > > > CMake constructs the command lines internally using string replacement > > > > on templates. We never actually know the exact arguments. Therefore > > > > providing arguments instead of the whole command would require parsing > > > > to be done on the CMake side instead. This is theoretically possible > > > > because we do know the shell for which we are generating (Windows `cmd` > > > > versus MSYS `sh`). However, it may also require a bunch of logic we > > > > don't have yet but that LLVM does. > > > > > > > > Alternatively, the JSON could have an additional `command_shell="..."` > > > > field that indicates the shell for which the command line is encoded. > > > > > > > > > Bummer. Given that this is hard to do in CMake, then I think we should > > > just tokenize in Clang. Let's use llvm::sys::getProcessTriple() instead > > > of LLVM_ON_WIN32 and check if that is an MSVC environment as a proxy for > > > the shell type. > > > > > > Do I understand correctly that `llvm::sys::getProcessTriple()` returns > > basically a compile-time constant? If yes, it won't allow the same clang > > tool binary to be used with both command line formats. Should we instead > > allow runtime selection of the command line format? For example, by > > extending JSON compilation database with the aforementioned > > `command_shell="..."` option. > > > It does return a compile time constant, but it is a compile time constant > representing the platform that clang-tidy is running on. I don't see a need > to have a single clang-tidy binary be able to parse both command line > formats. In fact, it seems actively harmful. > > If you are running on a Windows system with windows command line parsing > rules, and someone has generated a command line on linux, then this command > line was never intended to be run on Windows in the first place. The JSON > compilation database spec already says that the command line represents "the > exact command to be run **in the environment of the build system**". By > adding the flexibility to change the environment, this is no longer true. If > I try to run a command generated for one build system in the environment of > another build system, even if I tokenize it correctly whose to say it will > work? > > I understand the desire to make sure we get the right change so we don't have > to revisit this in the future, but to me this sounds like YAGNI and actually > increasing the risk of someone using the software and getting unexpected > results, not less risk. Now I'm less sure. And I might be misunderstanding something here. Folks actually using clang tools on Windows (Aaron?) can tell with more confidence whether Linux-style command line tokenization is of any use on Windows. I had an impression that it allowed clang tools to be used with mingw (and msys shell), but I'm not sure whether it's an important use case for anyone. https://reviews.llvm.org/D23455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
klimek added a reviewer: diltsman. klimek added a comment. +Daniel dilts who wrote the arguments support One problem is that getting into the game of parsing arbitrary shell modes is rather ugly. https://reviews.llvm.org/D23455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
zturner added a comment. In https://reviews.llvm.org/D23455#516753, @alexfh wrote: > In https://reviews.llvm.org/D23455#515527, @rnk wrote: > > > In https://reviews.llvm.org/D23455#515486, @brad.king wrote: > > > > > > the feasibility of emitting 'arguments' instead of 'command' into the > > > > JSON compilation database. > > > > > > > > > CMake constructs the command lines internally using string replacement on > > > templates. We never actually know the exact arguments. Therefore > > > providing arguments instead of the whole command would require parsing to > > > be done on the CMake side instead. This is theoretically possible > > > because we do know the shell for which we are generating (Windows `cmd` > > > versus MSYS `sh`). However, it may also require a bunch of logic we > > > don't have yet but that LLVM does. > > > > > > Alternatively, the JSON could have an additional `command_shell="..."` > > > field that indicates the shell for which the command line is encoded. > > > > > > Bummer. Given that this is hard to do in CMake, then I think we should just > > tokenize in Clang. Let's use llvm::sys::getProcessTriple() instead of > > LLVM_ON_WIN32 and check if that is an MSVC environment as a proxy for the > > shell type. > > > Do I understand correctly that `llvm::sys::getProcessTriple()` returns > basically a compile-time constant? If yes, it won't allow the same clang tool > binary to be used with both command line formats. Should we instead allow > runtime selection of the command line format? For example, by extending JSON > compilation database with the aforementioned `command_shell="..."` option. It does return a compile time constant, but it is a compile time constant representing the platform that clang-tidy is running on. I don't see a need to have a single clang-tidy binary be able to parse both command line formats. In fact, it seems actively harmful. If you are running on a Windows system with windows command line parsing rules, and someone has generated a command line on linux, then this command line was never intended to be run on Windows in the first place. The JSON compilation database spec already says that the command line represents "the exact command to be run **in the environment of the build system**". By adding the flexibility to change the environment, this is no longer true. If I try to run a command generated for one build system in the environment of another build system, even if I tokenize it correctly whose to say it will work? I understand the desire to make sure we get the right change so we don't have to revisit this in the future, but to me this sounds like YAGNI and actually increasing the risk of someone using the software and getting unexpected results, not less risk. https://reviews.llvm.org/D23455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
alexfh added a comment. Adding Aaron, since he seems to have interest in Windows support for clang-tidy. https://reviews.llvm.org/D23455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
alexfh added a comment. In https://reviews.llvm.org/D23455#515527, @rnk wrote: > In https://reviews.llvm.org/D23455#515486, @brad.king wrote: > > > > the feasibility of emitting 'arguments' instead of 'command' into the > > > JSON compilation database. > > > > > > CMake constructs the command lines internally using string replacement on > > templates. We never actually know the exact arguments. Therefore > > providing arguments instead of the whole command would require parsing to > > be done on the CMake side instead. This is theoretically possible because > > we do know the shell for which we are generating (Windows `cmd` versus MSYS > > `sh`). However, it may also require a bunch of logic we don't have yet but > > that LLVM does. > > > > Alternatively, the JSON could have an additional `command_shell="..."` > > field that indicates the shell for which the command line is encoded. > > > Bummer. Given that this is hard to do in CMake, then I think we should just > tokenize in Clang. Let's use llvm::sys::getProcessTriple() instead of > LLVM_ON_WIN32 and check if that is an MSVC environment as a proxy for the > shell type. Do I understand correctly that `llvm::sys::getProcessTriple()` returns basically a compile-time constant? If yes, it won't allow the same clang tool binary to be used with both command line formats. Should we instead allow runtime selection of the command line format? For example, by extending JSON compilation database with the aforementioned `command_shell="..."` option. https://reviews.llvm.org/D23455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
rnk added a comment. In https://reviews.llvm.org/D23455#515486, @brad.king wrote: > > the feasibility of emitting 'arguments' instead of 'command' into the JSON > > compilation database. > > > CMake constructs the command lines internally using string replacement on > templates. We never actually know the exact arguments. Therefore providing > arguments instead of the whole command would require parsing to be done on > the CMake side instead. This is theoretically possible because we do know > the shell for which we are generating (Windows `cmd` versus MSYS `sh`). > However, it may also require a bunch of logic we don't have yet but that LLVM > does. > > Alternatively, the JSON could have an additional `command_shell="..."` field > that indicates the shell for which the command line is encoded. Bummer. Given that this is hard to do in CMake, then I think we should just tokenize in Clang. Let's use llvm::sys::getProcessTriple() instead of LLVM_ON_WIN32 and check if that is an MSVC environment as a proxy for the shell type. https://reviews.llvm.org/D23455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
brad.king added a comment. > the feasibility of emitting 'arguments' instead of 'command' into the JSON > compilation database. CMake constructs the command lines internally using string replacement on templates. We never actually know the exact arguments. Therefore providing arguments instead of the whole command would require parsing to be done on the CMake side instead. This is theoretically possible because we do know the shell for which we are generating (Windows `cmd` versus MSYS `sh`). However, it may also require a bunch of logic we don't have yet but that LLVM does. Alternatively, the JSON could have an additional `command_shell="..."` field that indicates the shell for which the command line is encoded. https://reviews.llvm.org/D23455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
rnk added a comment. +cmake people for the feasibility of emitting 'arguments' instead of 'command' into the JSON compilation database. https://reviews.llvm.org/D23455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
It occurred to me that eol detection won't work reliably. Especially for tests, someone will write a compilation database by hand and check it in. Maybe they have git set to convert newlines for them, and we don't want a test to behave differently based on your source control settings. The target triple seems correct to me, but I'll see what others have to say On Sun, Aug 14, 2016 at 1:52 AM Manuel Klimek wrote: > > > On Sun, Aug 14, 2016, 9:52 AM Zachary Turner wrote: > >> I'll try and figure out who that was on Monday and loop them in. I'm not >> sure what problems the previous person ran into, but the test suite passes, >> i can run it on a large codebase, and all the results look fine and as if >> the tool is working. Hopefully the previous person has more insight into >> what I should be looking for. >> >> It's not that it's time critical, I'm just a little surprised that it >> seems so important to support a use case that I'm not sure anyone has ever >> tried to do or would ever want to do. >> > > my concern is not copying files around. > My main concern is making the code more complex with a solution that only > half works in the end and confusing users even more in the end vs having a > solution that is well thought out and works reliably. > If we're sure enough eol detection will work reliably, I'm fine. So far > all attempts I've seen had some problems. > > >> The idea of copying a compilation database around across machines, is >> this really something we need to go out of our way to support? >> On Sat, Aug 13, 2016 at 11:34 PM Manuel Klimek wrote: >> >>> 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 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 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 > 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 >> wrote: >> >>> >>> >>> On Sat, Aug 13, 2016, 10:17 PM Zachary Turner >>> 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
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
On Sun, Aug 14, 2016, 9:52 AM Zachary Turner wrote: > I'll try and figure out who that was on Monday and loop them in. I'm not > sure what problems the previous person ran into, but the test suite passes, > i can run it on a large codebase, and all the results look fine and as if > the tool is working. Hopefully the previous person has more insight into > what I should be looking for. > > It's not that it's time critical, I'm just a little surprised that it > seems so important to support a use case that I'm not sure anyone has ever > tried to do or would ever want to do. > my concern is not copying files around. My main concern is making the code more complex with a solution that only half works in the end and confusing users even more in the end vs having a solution that is well thought out and works reliably. If we're sure enough eol detection will work reliably, I'm fine. So far all attempts I've seen had some problems. > The idea of copying a compilation database around across machines, is this > really something we need to go out of our way to support? > On Sat, Aug 13, 2016 at 11:34 PM Manuel Klimek wrote: > >> 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 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 >>> 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 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 > wrote: > >> >> >> On Sat, Aug 13, 2016, 10:17 PM Zachary Turner >> 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
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
I'll try and figure out who that was on Monday and loop them in. I'm not
sure what problems the previous person ran into, but the test suite passes,
i can run it on a large codebase, and all the results look fine and as if
the tool is working. Hopefully the previous person has more insight into
what I should be looking for.
It's not that it's time critical, I'm just a little surprised that it seems
so important to support a use case that I'm not sure anyone has ever tried
to do or would ever want to do.
The idea of copying a compilation database around across machines, is this
really something we need to go out of our way to support?
On Sat, Aug 13, 2016 at 11:34 PM Manuel Klimek wrote:
> 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 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
>> 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
>>> 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
wrote:
>
>
> On Sat, Aug 13, 2016, 10:17 PM Zachary Turner
> 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 <
>> ale...@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
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
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 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
> 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
>> 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 wrote:
>>>
On Sat, Aug 13, 2016, 10:17 PM Zachary Turner
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 <
> ale...@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
>>
>>
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
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 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
> 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 wrote:
>>
>>>
>>>
>>> On Sat, Aug 13, 2016, 10:17 PM Zachary Turner
>>> 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
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
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
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 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 wrote:
>
>>
>>
>> On Sat, Aug 13, 2016, 10:17 PM Zachary Turner 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
>>> 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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
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 wrote:
>
>
> On Sat, Aug 13, 2016, 10:17 PM Zachary Turner 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
>> 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
>>>
>>>
>>>
>>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
On Sat, Aug 13, 2016, 10:17 PM Zachary Turner 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
> 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
>>
>>
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
The only thing that won't work is generating a compilation database on one
machine and physically copying it to another machine, but I don't think we
should worry about that. We can try to submit a request to CMake to get
more info in the compilation database, but something like that is months
out, if they do it all
On Sat, Aug 13, 2016 at 12:17 PM Zachary Turner wrote:
> The json is generated by CMake, so I don't thinks we can do this without
> patching CMake, which is a non-starter.
>
> 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?
>
> On Sat, Aug 13, 2016 at 10:35 AM Alexander Kornienko
> 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
>>
>>
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
The json is generated by CMake, so I don't thinks we can do this without
patching CMake, which is a non-starter.
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?
On Sat, Aug 13, 2016 at 10:35 AM Alexander Kornienko
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
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
zturner updated this revision to Diff 67947.
zturner added a comment.
After much head banging I think I finally figured this out. I'm putting this
back to my original revision, unchanged. The problem is not the original
patch. The original patch is perfectly fine. The problem is the way i was
writing my test. This is just updating the revision back to its original
state. I can't make a cross-repo diff, so I will make a new diff with the test.
https://reviews.llvm.org/D23455
Files:
lib/Tooling/JSONCompilationDatabase.cpp
Index: lib/Tooling/JSONCompilationDatabase.cpp
===
--- lib/Tooling/JSONCompilationDatabase.cpp
+++ lib/Tooling/JSONCompilationDatabase.cpp
@@ -16,7 +16,10 @@
#include "clang/Tooling/CompilationDatabasePluginRegistry.h"
#include "clang/Tooling/Tooling.h"
#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/Allocator.h"
+#include "llvm/Support/CommandLine.h"
#include "llvm/Support/Path.h"
+#include "llvm/Support/StringSaver.h"
#include
namespace clang {
@@ -113,8 +116,17 @@
std::vector unescapeCommandLine(
StringRef EscapedCommandLine) {
+#if defined(LLVM_ON_WIN32)
+ llvm::BumpPtrAllocator Alloc;
+ llvm::StringSaver Saver(Alloc);
+ llvm::SmallVector T;
+ llvm::cl::TokenizeWindowsCommandLine(EscapedCommandLine, Saver, T);
+ std::vector Result(T.begin(), T.end());
+ return Result;
+#else
CommandLineArgumentParser parser(EscapedCommandLine);
return parser.parse();
+#endif
}
class JSONCompilationDatabasePlugin : public CompilationDatabasePlugin {
Index: lib/Tooling/JSONCompilationDatabase.cpp
===
--- lib/Tooling/JSONCompilationDatabase.cpp
+++ lib/Tooling/JSONCompilationDatabase.cpp
@@ -16,7 +16,10 @@
#include "clang/Tooling/CompilationDatabasePluginRegistry.h"
#include "clang/Tooling/Tooling.h"
#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/Allocator.h"
+#include "llvm/Support/CommandLine.h"
#include "llvm/Support/Path.h"
+#include "llvm/Support/StringSaver.h"
#include
namespace clang {
@@ -113,8 +116,17 @@
std::vector unescapeCommandLine(
StringRef EscapedCommandLine) {
+#if defined(LLVM_ON_WIN32)
+ llvm::BumpPtrAllocator Alloc;
+ llvm::StringSaver Saver(Alloc);
+ llvm::SmallVector T;
+ llvm::cl::TokenizeWindowsCommandLine(EscapedCommandLine, Saver, T);
+ std::vector Result(T.begin(), T.end());
+ return Result;
+#else
CommandLineArgumentParser parser(EscapedCommandLine);
return parser.parse();
+#endif
}
class JSONCompilationDatabasePlugin : public CompilationDatabasePlugin {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
zturner updated this revision to Diff 67915.
zturner added a comment.
Update with changes needed to make fixed compilation database work (which also
breaks many other tests)
https://reviews.llvm.org/D23455
Files:
include/clang/Tooling/CompilationDatabase.h
lib/Tooling/CommonOptionsParser.cpp
lib/Tooling/CompilationDatabase.cpp
lib/Tooling/JSONCompilationDatabase.cpp
Index: lib/Tooling/JSONCompilationDatabase.cpp
===
--- lib/Tooling/JSONCompilationDatabase.cpp
+++ lib/Tooling/JSONCompilationDatabase.cpp
@@ -16,7 +16,10 @@
#include "clang/Tooling/CompilationDatabasePluginRegistry.h"
#include "clang/Tooling/Tooling.h"
#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/Allocator.h"
+#include "llvm/Support/CommandLine.h"
#include "llvm/Support/Path.h"
+#include "llvm/Support/StringSaver.h"
#include
namespace clang {
@@ -113,8 +116,17 @@
std::vector unescapeCommandLine(
StringRef EscapedCommandLine) {
+#if defined(LLVM_ON_WIN32)
+ llvm::BumpPtrAllocator Alloc;
+ llvm::StringSaver Saver(Alloc);
+ llvm::SmallVector T;
+ llvm::cl::TokenizeWindowsCommandLine(EscapedCommandLine, Saver, T);
+ std::vector Result(T.begin(), T.end());
+ return Result;
+#else
CommandLineArgumentParser parser(EscapedCommandLine);
return parser.parse();
+#endif
}
class JSONCompilationDatabasePlugin : public CompilationDatabasePlugin {
Index: lib/Tooling/CompilationDatabase.cpp
===
--- lib/Tooling/CompilationDatabase.cpp
+++ lib/Tooling/CompilationDatabase.cpp
@@ -205,25 +205,27 @@
/// \li true if successful.
/// \li false if \c Args cannot be used for compilation jobs (e.g.
/// contains an option like -E or -version).
-static bool stripPositionalArgs(std::vector Args,
+static bool stripPositionalArgs(std::vector Args, std::string &ProgName,
std::vector &Result) {
+ ProgName = "clang-tool";
+ Result.clear();
+ if (Args.empty())
+return true;
+
IntrusiveRefCntPtr DiagOpts = new DiagnosticOptions();
UnusedInputDiagConsumer DiagClient;
DiagnosticsEngine Diagnostics(
IntrusiveRefCntPtr(new DiagnosticIDs()),
&*DiagOpts, &DiagClient, false);
- // The clang executable path isn't required since the jobs the driver builds
- // will not be executed.
+ // Although the jobs the driver builds will not be executed, the clang
+ // executable path is still required in order to determine the correct
+ // driver mode (CL, GCC, G++, etc).
+ ProgName = Args[0];
std::unique_ptr NewDriver(new driver::Driver(
- /* ClangExecutable= */ "", llvm::sys::getDefaultTargetTriple(),
- Diagnostics));
+ ProgName, llvm::sys::getDefaultTargetTriple(), Diagnostics));
NewDriver->setCheckInputsExist(false);
- // This becomes the new argv[0]. The value is actually not important as it
- // isn't used for invoking Tools.
- Args.insert(Args.begin(), "clang-tool");
-
// By adding -c, we force the driver to treat compilation as the last phase.
// It will then issue warnings via Diagnostics about un-used options that
// would have been used for linking. If the user provided a compiler name as
@@ -290,14 +292,15 @@
Argc = DoubleDash - Argv;
std::vector StrippedArgs;
- if (!stripPositionalArgs(CommandLine, StrippedArgs))
+ std::string ToolName;
+ if (!stripPositionalArgs(CommandLine, ToolName, StrippedArgs))
return nullptr;
- return new FixedCompilationDatabase(Directory, StrippedArgs);
+ return new FixedCompilationDatabase(Directory, ToolName, StrippedArgs);
}
-FixedCompilationDatabase::
-FixedCompilationDatabase(Twine Directory, ArrayRef CommandLine) {
- std::vector ToolCommandLine(1, "clang-tool");
+FixedCompilationDatabase::FixedCompilationDatabase(
+Twine Directory, StringRef ProgName, ArrayRef CommandLine) {
+ std::vector ToolCommandLine(1, ProgName);
ToolCommandLine.insert(ToolCommandLine.end(),
CommandLine.begin(), CommandLine.end());
CompileCommands.emplace_back(Directory, StringRef(),
Index: lib/Tooling/CommonOptionsParser.cpp
===
--- lib/Tooling/CommonOptionsParser.cpp
+++ lib/Tooling/CommonOptionsParser.cpp
@@ -136,8 +136,8 @@
if (!Compilations) {
llvm::errs() << "Error while trying to load a compilation database:\n"
<< ErrorMessage << "Running without flags.\n";
- Compilations.reset(
- new FixedCompilationDatabase(".", std::vector()));
+ Compilations.reset(new FixedCompilationDatabase(
+ ".", "clang-tool", std::vector()));
}
}
auto AdjustingCompilations =
Index: include/clang/Tooling/CompilationDatabase.h
===
--- include/clang/Tooling/CompilationDatabase.h
+++ include/clang/Tooling/CompilationDatabase.h
@@ -189
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
zturner added a comment.
This is starting to get fairly difficult, and I'm afraid it may require someone
more knowledgable about clang-format's internals than me. I wrote a test to
have it use a fixed compilation database, and I was able to make that test
pass, but it breaks many other tests. I will go ahead and upload that patch
anyway just so you can see what I did.
What's weird is that it doesn't seem to break clang-tidy itself, just the
tests. For example, here's what I get when I run the following command:
D:\src\llvmbuild\ninja>"bin\clang-tidy"
"--checks=-*,google-readability-casting" "-header-filter=.*"
"D:\src\llvm\tools\clang\tools\extra\test\clang-tidy\google-readability-casting.c"
"--" "-ID:\src\llvm\tools\clang\tools\extra\test\clang-tidy" "-DTEST_INCLUDE"
"-x" "c++"
1 warning generated.
D:\src\llvm\tools\clang\tools\extra\test\clang-tidy/google-readability-casting.c:16:22:
warning: redundant cast to the same type [google-readability-casting]
const char *cpc2 = (const char*)cpc;
^
It appears to work fine.
But when I run the test, it complains about trying to copy some file which
seems unrelated to my change and more about the test infrastructure.
D:\src\llvmbuild\ninja>python bin\llvm-lit.py -sv
d:\src\llvm\tools\clang\tools\extra\test\clang-tidy\google-readability-casting.c
-- Testing: 1 tests, 1 threads --
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
FAIL: Clang Tools :: clang-tidy/google-readability-casting.c (1 of 1)
TEST 'Clang Tools ::
clang-tidy/google-readability-casting.c' FAILED
Script:
--
C:/Python27/python.exe
D:/src/llvm/tools/clang/tools/extra/test/../test\clang-tidy\check_clang_tidy.py
D:\src\llvm\tools\clang\tools\extra\test\clang-tidy\google-readability-casting.c
google-readability-casting
D:\src\llvmbuild\ninja\tools\clang\tools\extra\test\clang-tidy\Output\google-readability-casting.c.tmp
-- -- -x c
clang-tidy --checks=-*,google-readability-casting
D:\src\llvm\tools\clang\tools\extra\test\clang-tidy\google-readability-casting.c
-- -x c++ | FileCheck
D:\src\llvm\tools\clang\tools\extra\test\clang-tidy\google-readability-casting.c
-check-prefix=CHECK-MESSAGES -implicit-check-not='{{warning|error}}:'
cp
D:\src\llvm\tools\clang\tools\extra\test\clang-tidy\google-readability-casting.c
D:\src\llvmbuild\ninja\tools\clang\tools\extra\test\clang-tidy\Output\google-readability-casting.c.tmp.main_file.cpp
clang-tidy --checks=-*,google-readability-casting -header-filter='.*'
D:\src\llvmbuild\ninja\tools\clang\tools\extra\test\clang-tidy\Output\google-readability-casting.c.tmp.main_file.cpp
-- -ID:\src\llvm\tools\clang\tools\extra\test\clang-tidy -DTEST_INCLUDE -x c++
| FileCheck
D:\src\llvm\tools\clang\tools\extra\test\clang-tidy\google-readability-casting.c
-check-prefix=CHECK-MESSAGES -implicit-check-not='{{warning|error}}:'
--
Exit Code: 1
Command Output (stdout):
--
$ "C:/Python27/python.exe"
"D:/src/llvm/tools/clang/tools/extra/test/../test\clang-tidy\check_clang_tidy.py"
"D:\src\llvm\tools\clang\tools\extra\test\clang-tidy\google-readability-casting.c"
"google-readability-casting"
"D:\src\llvmbuild\ninja\tools\clang\tools\extra\test\clang-tidy\Output\google-readability-casting.c.tmp"
"--" "--" "-x" "c"
# command output:
Running ['clang-tidy',
'D:\\src\\llvmbuild\\ninja\\tools\\clang\\tools\\extra\\test\\clang-tidy\\Output\\google-readability-casting.c.tmp.c',
'-fix', '--checks=-*,google-readability-casting', '--', '-x', 'c']...
clang-tidy output ---
1 warning generated.
D:\src\llvmbuild\ninja\tools\clang\tools\extra\test\clang-tidy\Output\google-readability-casting.c.tmp.c:16:22:
warning: redundant cast to the same type [google-readability-casting]
const char *cpc2 = (const char*)cpc;
^
D:\src\llvmbuild\ninja\tools\clang\tools\extra\test\clang-tidy\Output\google-readability-casting.c.tmp.c:16:22:
note: FIX-IT applied suggested code changes
const char *cpc2 = (const char*)cpc;
^
clang-tidy applied 1 of 1 suggested fixes.
--
-- Fixes -
---
D:\src\llvmbuild\ninja\tools\clang\tools\extra\test\clang-tidy\Output\google-readability-casting.c.tmp.c.orig
2016-08-12 14:29:14.035738300 -0700
+++
D:\src\llvmbuild\ninja\tools\clang\tools\extra\test\clang-tidy\Output\google-readability-casting.c.tmp.c
2016-08-12 14:29:14.133810600 -0700
@@ -13,7 +13,7 @@
#else
void f(const char *cpc) {
- const char *cpc2 = (const char*)cpc;
+ const char *cpc2 = cpc;
//
//
char *pc = (char*)cpc;
--
$ "clang-tidy" "--checks=-*,google-readability-casting"
"D:\src\llvm\tools\clang\tools
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
zturner added a comment. I mentioned offline that we could detect CRLF or LF and heuristically decide whether it's a windows compilation database. That seems like a horrible hack, so failing that, yes having CMake do it for us would be better. https://reviews.llvm.org/D23455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
rnk added a subscriber: rnk. rnk added a comment. We should convince cmake to emit "arguments" instead of "command" so that we don't have this ambiguity. https://reviews.llvm.org/D23455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
