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

299 ↗

(On Diff #31824)

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

302–304 ↗

(On Diff #31824)

Can we use for-range loops with auto?

[diltsman]

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

299 ↗

(On Diff #31824)

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?

302–304 ↗

(On Diff #31824)

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

[klimek]

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

295 ↗

(On Diff #31952)

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?

298–300 ↗

(On Diff #31952)

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 ↗

(On Diff #32082)

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!

include/clang/Tooling/JSONCompilationDatabase.h

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;
 

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;
for (llvm::yaml::SequenceNode::iterator AI = Array->begin(),
                                           AE = Array->end();
        AI != AE; ++AI) {
     llvm::yaml::MappingNode *Object = dyn_cast<llvm::yaml::MappingNode>(&*AI);
     if (!Object) {
       ErrorMessage = "Expected object.";
       return false;
     }
     llvm::yaml::ScalarNode *Directory = nullptr;
-    llvm::yaml::ScalarNode *Command = nullptr;
+    std::vector<std::string> Args;
+    bool CommandFound = false;
     llvm::yaml::ScalarNode *File = nullptr;
     for (llvm::yaml::MappingNode::iterator KVI = Object->begin(),
                                            KVE = Object->end();
          KVI != KVE; ++KVI) {
+      llvm::yaml::ScalarNode *KeyString =
+          dyn_cast<llvm::yaml::ScalarNode>((*KVI).getKey());
+      if (!KeyString) {
+        ErrorMessage = "Expected strings as key.";
+        return false;
+      }
+      SmallString<8> KeyStorage;
+      StringRef KeyValue = KeyString->getValue(KeyStorage);
       llvm::yaml::Node *Value = (*KVI).getValue();
       if (!Value) {
         ErrorMessage = "Expected value.";
         return false;
       }
       llvm::yaml::ScalarNode *ValueString =
           dyn_cast<llvm::yaml::ScalarNode>(Value);
-      if (!ValueString) {
+      llvm::yaml::SequenceNode *SequenceString =
+          dyn_cast<llvm::yaml::SequenceNode>(Value);
+      if (KeyValue == "cmd" && !SequenceString) {
+        ErrorMessage = "Expected sequence as value.";
+        return false;
+      } else if (KeyValue != "cmd" && !ValueString) {
         ErrorMessage = "Expected string as value.";
         return false;
       }
-      llvm::yaml::ScalarNode *KeyString =
-          dyn_cast<llvm::yaml::ScalarNode>((*KVI).getKey());
-      if (!KeyString) {
-        ErrorMessage = "Expected strings as key.";
+      if (KeyValue == "directory") {
+        Directory = ValueString;
+      } else if (KeyValue == "command") {
+        if (CommandFound) {
+          ErrorMessage = "Multiple command and cmd found";
           return false;
         }
-      SmallString<8> KeyStorage;
-      if (KeyString->getValue(KeyStorage) == "directory") {
-        Directory = ValueString;
-      } else if (KeyString->getValue(KeyStorage) == "command") {
-        Command = ValueString;
-      } else if (KeyString->getValue(KeyStorage) == "file") {
+        SmallString<1024> CommandStorage;
+        // FIXME: Escape correctly:
+        Args = unescapeCommandLine(ValueString->getValue(CommandStorage));
+        CommandFound = true;
+      } else if (KeyValue == "file") {
         File = ValueString;
+      } else if (KeyValue == "cmd") {
+        if (CommandFound) {
+          ErrorMessage = "Multiple command and cmd found";
+          return false;
+        }
+        for (llvm::yaml::SequenceNode::iterator CI = SequenceString->begin(),
+          CE = SequenceString->end();
+          CI != CE; ++CI) {
+          SmallString<128> CommandStorage;
+          auto ValueString = dyn_cast<llvm::yaml::ScalarNode>(&*CI);
+
+          Args.push_back(ValueString->getValue(CommandStorage));
+        }
+        CommandFound = true;
       } else {
         ErrorMessage = ("Unknown key: \"" +
                         KeyString->getRawValue() + "\"").str();
         return false;
       }
     }
     if (!File) {
       ErrorMessage = "Missing key: \"file\".";
       return false;
     }
-    if (!Command) {
-      ErrorMessage = "Missing key: \"command\".";
+    if (!CommandFound) {
+      ErrorMessage = "Missing key: \"command\" or \"cmd\".";
       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, Args));
     MatchTrie.insert(NativeFilePath);
   }
   return true;
 }
 
 } // end namespace tooling
 } // end namespace 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 cmd");
   expectFailure("[{\"command\":\"\",\"file\":\"\"}]", "Missing directory");
+  expectFailure("[{\"directory\":\"\",\"cmd\":[]}]", "Missing file");
+  expectFailure("[{\"cmd\":\"\",\"file\":\"\"}]", "Missing directory");
+  expectFailure("[{\"command\":\"\",\"cmd\":[],\"file\":\"\"}]", "Command and cmd");
+  expectFailure("[{\"directory\":\"\",\"cmd\":\"\",\"file\":\"\"}]", "Cmd 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;