Add cmd to compilation database file format

[diltsman]

Summary

In the compilation database command must be escaped. This can cause problems on some systems (Windows). cmd is added as a list of command line arguments that will not be escaped.

[klimek]

Small aside: phab helps mainly when context is added to the diff (see http://llvm.org/docs/Phabricator.html)
git: git diff -U999999 other-branch
svn: svn diff --diff-cmd=diff -x -U999999
(or you can use the arc diff command line tool)

Apart from that, my comment on the initial thread still stands:
I think we should name the list one "arguments" or (if you prefer) "args"; I think that makes a lot more sense: command: the shell command that is run, arguments: the command line arguments.

[diltsman]

I did the rename to args, but apparently I uploaded the old patch file. New patch file coming shortly.

[klimek]

Sorry I missed the update - I still see "cmd" in the latest patch (note
that phab is currently down due to somebody trying to DDOS or hack it).

[diltsman]

I'm trying to upload a new patch and I get an error from Phabricator:
'Unhandled Exception ("Exception")'.

Any ideas how to get this to work? I have attached the diff file.

• Arguments.patch77 KBDownload

[klimek]

Ok, we're very close now :) Thanks for taking the time to work through this!

../llvm/tools/clang/lib/Tooling/JSONCompilationDatabase.cpp

301

Any reason we don't want to allow both, but prefer the arguments?

304–306

Can we use for-range loops with auto?

[diltsman]

../llvm/tools/clang/lib/Tooling/JSONCompilationDatabase.cpp

301

I don't see how the arguments are preferred.

I was thinking that the user would either want to use the command line or list arguments. Is there a case where someone would want to do both?

Also, if we permit both, do we need to do any kind of uniquing?

304–306

I changed two other locations to range-based for loops, too.

[klimek]

../llvm/tools/clang/lib/Tooling/JSONCompilationDatabase.cpp

301

You're probably right. I guess people are not distributing clang tools (yet!), so we probably don't need tools like cmake to output both at the same time.

What I meant with "preferred" is that we could allow specifying both, and then we would use 'arguments', if specified, and otherwise fall back to 'command'. Does that make sense?

304–306

Thanks!

[diltsman]

Arguments and Command can now be in the same compilation database for the same file. Arguments are preferred when both are present.

[klimek]

LG with a happy path test.

../llvm/tools/clang/unittests/Tooling/CompilationDatabaseTest.cpp

45

Please add a happy path test, too.

[diltsman]

Where/how does documentation (http://clang.llvm.org/docs/JSONCompilationDatabase.html) get updated?

[diltsman]

Added test for command and arguments in same object.
Fixed bug where parse failed if command and argument resolved to empty lists.

[klimek]

Cool, looks good! If you need me to land it let me know.

[diltsman]

Yes, please land it.

[klimek]

Unfortunately I can't get the patch to apply cleanly due to different line endings.

[klimek]

Solved the line ending problem.

[klimek]

Submitted as r245036. Thanks!

../llvm/tools/clang/include/clang/Tooling/JSONCompilationDatabase.h

 #include <vector>
 
 namespace clang {
 namespace tooling {
 
 /// \brief A JSON based compilation database.
 ///
 /// JSON compilation database files must contain a list of JSON objects which
-/// provide the command lines in the attributes 'directory', 'command' and
-/// 'file':
+/// provide the command lines in the attributes 'directory', 'command',
+/// 'arguments' and 'file':
 /// [
 ///   { "directory": "<working directory of the compile>",
 ///     "command": "<compile command line>",
 ///     "file": "<path to source file>"
 ///   },
+///   { "directory": "<working directory of the compile>",
+///     "arguments": ["<raw>", "<command>" "<line>" "<parameters>"],
+///     "file": "<path to source file>"
+///   },
 ///   ...
 /// ]
 /// Each object entry defines one compile action. The specified file is
 /// considered to be the main source file for the translation unit.
 ///
+/// 'command' is a full command line that will be unescaped.
+///
+/// 'arguments' is a list of command line arguments that will not be unescaped.
+///
 /// JSON compilation databases can for example be generated in CMake projects
 /// by setting the flag -DCMAKE_EXPORT_COMPILE_COMMANDS.
 class JSONCompilationDatabase : public CompilationDatabase {
 public:
   /// \brief Loads a JSON compilation database from the specified file.
   ///
   /// Returns NULL and sets ErrorMessage if the database could not be
   /// loaded from the given file.
private:
   ///
   /// Returns whether parsing succeeded. Sets ErrorMessage if parsing
   /// failed.
   bool parse(std::string &ErrorMessage);
 
   // Tuple (directory, commandline) where 'commandline' pointing to the
   // corresponding nodes in the YAML stream.
   typedef std::pair<llvm::yaml::ScalarNode*,
-                    llvm::yaml::ScalarNode*> CompileCommandRef;
+                    std::vector<std::string>> CompileCommandRef;
 
   /// \brief Converts the given array of CompileCommandRefs to CompileCommands.
   void getCommands(ArrayRef<CompileCommandRef> CommandsRef,
                    std::vector<CompileCommand> &Commands) const;
 
   // Maps file paths to the compile command lines for that file.
   llvm::StringMap< std::vector<CompileCommandRef> > IndexByFile;
 

../llvm/tools/clang/lib/Tooling/JSONCompilationDatabase.cpp

 
 void JSONCompilationDatabase::getCommands(
                                   ArrayRef<CompileCommandRef> CommandsRef,
                                   std::vector<CompileCommand> &Commands) const {
   for (int I = 0, E = CommandsRef.size(); I != E; ++I) {
     SmallString<8> DirectoryStorage;
     SmallString<1024> CommandStorage;
     Commands.emplace_back(
-        // FIXME: Escape correctly:
-        CommandsRef[I].first->getValue(DirectoryStorage),
-        unescapeCommandLine(CommandsRef[I].second->getValue(CommandStorage)));
+      CommandsRef[I].second);
   }
 }
 
 bool JSONCompilationDatabase::parse(std::string &ErrorMessage) {
   llvm::yaml::document_iterator I = YAMLStream.begin();
   if (I == YAMLStream.end()) {
     ErrorMessage = "Error while parsing YAML.";
     return false;
   }
   llvm::yaml::Node *Root = I->getRoot();
   if (!Root) {
     ErrorMessage = "Error while parsing YAML.";
     return false;
   }
   llvm::yaml::SequenceNode *Array = dyn_cast<llvm::yaml::SequenceNode>(Root);
   if (!Array) {
     ErrorMessage = "Expected array.";
     return false;
   }
-  for (llvm::yaml::SequenceNode::iterator AI = Array->begin(),
-                                          AE = Array->end();
-       AI != AE; ++AI) {
-    llvm::yaml::MappingNode *Object = dyn_cast<llvm::yaml::MappingNode>(&*AI);
+  for (auto& NextObject : *Array) {
+    llvm::yaml::MappingNode *Object = dyn_cast<llvm::yaml::MappingNode>(&NextObject);
     if (!Object) {
       ErrorMessage = "Expected object.";
       return false;
     }
     llvm::yaml::ScalarNode *Directory = nullptr;
-    llvm::yaml::ScalarNode *Command = nullptr;
+    std::vector<std::string> Arguments;
+    std::vector<std::string> Command;
     llvm::yaml::ScalarNode *File = nullptr;
-    for (llvm::yaml::MappingNode::iterator KVI = Object->begin(),
-                                           KVE = Object->end();
-         KVI != KVE; ++KVI) {
-      llvm::yaml::Node *Value = (*KVI).getValue();
+    bool ArgumentsFound = false;
+    bool CommandFound = false;
+    for (auto& NextKeyValue : *Object) {
+      llvm::yaml::ScalarNode *KeyString =
+          dyn_cast<llvm::yaml::ScalarNode>(NextKeyValue.getKey());
+      if (!KeyString) {
+        ErrorMessage = "Expected strings as key.";
+        return false;
+      }
+      SmallString<10> KeyStorage;
+      StringRef KeyValue = KeyString->getValue(KeyStorage);
+      llvm::yaml::Node *Value = NextKeyValue.getValue();
       if (!Value) {
         ErrorMessage = "Expected value.";
         return false;
       }
       llvm::yaml::ScalarNode *ValueString =
           dyn_cast<llvm::yaml::ScalarNode>(Value);
-      if (!ValueString) {
-        ErrorMessage = "Expected string as value.";
+      llvm::yaml::SequenceNode *SequenceString =
+          dyn_cast<llvm::yaml::SequenceNode>(Value);
+      if (KeyValue == "arguments" && !SequenceString) {
+        ErrorMessage = "Expected sequence as value.";
         return false;
-      }
-      llvm::yaml::ScalarNode *KeyString =
-          dyn_cast<llvm::yaml::ScalarNode>((*KVI).getKey());
-      if (!KeyString) {
-        ErrorMessage = "Expected strings as key.";
+      } else if (KeyValue != "arguments" && !ValueString) {
+        ErrorMessage = "Expected string as value.";
         return false;
       }
-      SmallString<8> KeyStorage;
-      if (KeyString->getValue(KeyStorage) == "directory") {
+      if (KeyValue == "directory") {
         Directory = ValueString;
-      } else if (KeyString->getValue(KeyStorage) == "command") {
-        Command = ValueString;
-      } else if (KeyString->getValue(KeyStorage) == "file") {
+      } else if (KeyValue == "arguments") {
+        for (auto& NextArgument : *SequenceString) {
+          SmallString<128> CommandStorage;
+          auto ValueString = dyn_cast<llvm::yaml::ScalarNode>(&NextArgument);
+
+          Arguments.push_back(ValueString->getValue(CommandStorage));
+        }
+        ArgumentsFound = true;
+      } else if (KeyValue == "command") {
+        SmallString<1024> CommandStorage;
+        // FIXME: Escape correctly:
+        Command = unescapeCommandLine(ValueString->getValue(CommandStorage));
+        CommandFound = true;
+      } else if (KeyValue == "file") {
         File = ValueString;
       } else {
         ErrorMessage = ("Unknown key: \"" +
                         KeyString->getRawValue() + "\"").str();
         return false;
       }
     }
     if (!File) {
       ErrorMessage = "Missing key: \"file\".";
       return false;
     }
-    if (!Command) {
-      ErrorMessage = "Missing key: \"command\".";
+    if (!ArgumentsFound && !CommandFound) {
+      ErrorMessage = "Missing key: \"command\" or \"arguments\".";
       return false;
     }
     if (!Directory) {
       ErrorMessage = "Missing key: \"directory\".";
       return false;
     }
     SmallString<8> FileStorage;
     StringRef FileName = File->getValue(FileStorage);
     SmallString<128> NativeFilePath;
     if (llvm::sys::path::is_relative(FileName)) {
       SmallString<8> DirectoryStorage;
       SmallString<128> AbsolutePath(
           Directory->getValue(DirectoryStorage));
       llvm::sys::path::append(AbsolutePath, FileName);
       llvm::sys::path::native(AbsolutePath, NativeFilePath);
     } else {
       llvm::sys::path::native(FileName, NativeFilePath);
     }
     IndexByFile[NativeFilePath].push_back(
-        CompileCommandRef(Directory, Command));
+        CompileCommandRef(Directory, ArgumentsFound ? Arguments : Command));
     MatchTrie.insert(NativeFilePath);
   }
   return true;
 }
 
 } // end namespace tooling
 } // end namespace clang

../llvm/tools/clang/unittests/Tooling/CompilationDatabaseTest.cpp

TEST(JSONCompilationDatabase, ErrsOnInvalidFormat) {
   expectFailure("", "Empty database");
   expectFailure("{", "Invalid JSON");
   expectFailure("[[]]", "Array instead of object");
   expectFailure("[{\"a\":[]}]", "Array instead of value");
   expectFailure("[{\"a\":\"b\"}]", "Unknown key");
   expectFailure("[{[]:\"\"}]", "Incorrectly typed entry");
   expectFailure("[{}]", "Empty entry");
   expectFailure("[{\"directory\":\"\",\"command\":\"\"}]", "Missing file");
-  expectFailure("[{\"directory\":\"\",\"file\":\"\"}]", "Missing command");
+  expectFailure("[{\"directory\":\"\",\"file\":\"\"}]", "Missing command or arguments");
   expectFailure("[{\"command\":\"\",\"file\":\"\"}]", "Missing directory");
+  expectFailure("[{\"directory\":\"\",\"arguments\":[]}]", "Missing file");
+  expectFailure("[{\"arguments\":\"\",\"file\":\"\"}]", "Missing directory");
+  expectFailure("[{\"directory\":\"\",\"arguments\":\"\",\"file\":\"\"}]", "Arguments not array");
+  expectFailure("[{\"directory\":\"\",\"command\":[],\"file\":\"\"}]", "Command not string");
 }
 
 static std::vector<std::string> getAllFiles(StringRef JSONDatabase,
                                             std::string &ErrorMessage) {
   std::unique_ptr<CompilationDatabase> Database(
       JSONCompilationDatabase::loadFromBuffer(JSONDatabase, ErrorMessage));
   if (!Database) {
     ADD_FAILURE() << ErrorMessage;
     return std::vector<std::string>();
if (!Database)
     return CompileCommand();
   std::vector<CompileCommand> Commands = Database->getCompileCommands(FileName);
   EXPECT_LE(Commands.size(), 1u);
   if (Commands.empty())
     return CompileCommand();
   return Commands[0];
 }
 
+TEST(JSONCompilationDatabase, ArgumentsPreferredOverCommand) {
+   StringRef Directory("//net/dir");
+   StringRef FileName("//net/dir/filename");
+   StringRef Command("command");
+   StringRef Arguments = "arguments";
+   Twine ArgumentsAccumulate;
+   std::string ErrorMessage;
+   CompileCommand FoundCommand = findCompileArgsInJsonDatabase(
+      FileName,
+      ("[{\"directory\":\"" + Directory + "\","
+         "\"command\":\"" + Command + "\","
+         "\"arguments\":[\"" + Arguments + "\"],"
+         "\"file\":\"" + FileName + "\"}]").str(),
+      ErrorMessage);
+   EXPECT_EQ(Directory, FoundCommand.Directory) << ErrorMessage;
+   EXPECT_EQ(1u, FoundCommand.CommandLine.size()) << ErrorMessage;
+   EXPECT_EQ(Arguments, FoundCommand.CommandLine[0]) << ErrorMessage;
+}
+
 struct FakeComparator : public PathComparator {
   ~FakeComparator() override {}
   bool equivalent(StringRef FileA, StringRef FileB) const override {
     return FileA.equals_lower(FileB);
   }
 };
 
 class FileMatchTrieTest : public ::testing::Test {