[clang][Tooling] Add support for .rsp files in compile_commands.json

[lh123]

Summary

Add support for .rsp files.

Fixes https://github.com/clangd/clangd/issues/81

[lh123]

This comment was removed by lh123.

[lh123]

update diff

[lh123]

format patch

[kadircet]

Thanks for taking a look into this, the rsp files issue has came up before in the past but there wasn't enough investment to implement it.

Haven't checked the implementation in detail yet, I believe the layering should be different;

This is a common problem for all of the clang-related tools, as they all share the same "compilation database" abstraction layer, therefore it would be better to implement this at that layer so that other tools (e.g, clang-tidy) can also benefit from this.
You can find the related code in clang/include/clang/Tooling/CompilationDatabase.h and clang/lib/Tooling/CompilationDatabase.cpp.

Also compilation databases has been historically neglecting Virtual File System abstractions, it is hard to change it now. But would be great if you could try to keep that in mind while performing reads.

So would you mind making such changes ?

[lh123]

In D70222#1749533, @kadircet wrote:

Thanks for taking a look into this, the rsp files issue has came up before in the past but there wasn't enough investment to implement it.

Haven't checked the implementation in detail yet, I believe the layering should be different;

This is a common problem for all of the clang-related tools, as they all share the same "compilation database" abstraction layer, therefore it would be better to implement this at that layer so that other tools (e.g, clang-tidy) can also benefit from this.
You can find the related code in clang/include/clang/Tooling/CompilationDatabase.h and clang/lib/Tooling/CompilationDatabase.cpp.

Also compilation databases has been historically neglecting Virtual File System abstractions, it is hard to change it now. But would be great if you could try to keep that in mind while performing reads.

So would you mind making such changes ?

Ok, I will look into this.

[lh123]

Move the implementation to JSONCompilationDatabase.

[lh123]

Respect JSONCommandLineSyntax

[kadircet]

Thanks, layering seems better now. But as Sam mentioned in the issue it would be better to have something like expandRSPFiles similar to inferMissingCompileCommands.
As the problem is not in the JSONCompilationDatabase itself, it can also occur in FixedCompilationDatabase or any other models clang might've in the future.

The CompilationDatabase returned by expandRSPFiles could wrap any other CDB and upon querying of commands, it can expand any RSP files returned by the underlying CDB.

This will also ensure minimum changes to the existing code, reducing any chances of breakage. As for unittests, please see clang/unittests/Tooling/CompilationDatabaseTest.cpp

and a small nit, when modifying existing files, please only run formatting on the lines you touched to preserve history.

[lh123]

Address comment

[lh123]

fix typo in function document.

[lh123]

fixes some bug and add more test.

[kadircet]

Sorry wrote comments but forgot to hit submit :(

clang/include/clang/Tooling/CompilationDatabase.h

229

instead of exposing this functionality and handling them in Fixed and JSON compilation databases separately, I was rather talking about creating a new type of compilation database.

as inferTargetAndDriverMode or inferMissingCompileCommands functions does. which will take an Inner/Base compilation database, and in its getCompileCommands method will try to expand any rsp files.

does that make sense?

Also please implement it in a new file. (but still put the declaration into this header)

[lh123]

Address comment

[lh123]

Expand response files before inferMissingCompileCommands and inferTargetAndDriverMode

[kadircet]

clang/include/clang/Tooling/CompilationDatabase.h

223

nit: s/response files/rsp(response) files/

230

it looks like none of the callsites are setting this parameter can we get rid of this one?

clang/lib/Tooling/CompilationDatabase.cpp

402 ↗

(On Diff #230756)

is it really common for rsp files to show up in fixed compilation databases? since compile_flags.txt itself is also a file doesn't make much sense to refer to another one. as it can also contain text of arbitrary length.

clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp

168

so it looks like we already have ExpandResponseFiles exposed in llvm/include/llvm/Support/CommandLine.h could you make use of it in here instead of re-implementing it again?

I see that it has a different signature, but we can do the conversion back and forth in here, going from std::string to char* is cheap anyways, coming back is expensive though, and we can limit that to iff we have seen an argument starting with an @. So this would be something like:

llvm::SmallVector<const char*, 20> Argvs;
Argvs.reserve(Cmd.CommandLine.size());
bool SeenRSPFile = false;
for(auto &Argv : Cmd.CommandLine) {
  Argvs.push_back(Argv.c_str());
  SeenRSPFile |= Argv.front() == '@';
}
if(!SeenRSPFile)
  continue;

// call ExpandResponseFiles with Argvs, convert back to vector<std::string> and assign to Cmd.CommandLine

[lh123]

clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp

168

I think it's not easy to reuse ExpandResponseFiles without changing code because it uses llvm::sys::fs::current_path to resolve relative paths.

In fact, most of the code here is copied from CommandLine

[lh123]

clang/lib/Tooling/CompilationDatabase.cpp

402 ↗

(On Diff #230756)

Yes, rsp files usually only appear on the command line.

[lh123]

address comments

[lh123]

clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp

168

But we can reuse static bool ExpandResponseFile(StringRef FName, StringSaver &Saver, TokenizerCallback Tokenizer, SmallVectorImpl<const char *> &NewArgv, bool MarkEOLs, bool RelativeNames) if we expose it in CommandLine.h

[lh123]

clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp

168

static bool ExpandResponseFile(StringRef FName, StringSaver &Saver, TokenizerCallback Tokenizer, SmallVectorImpl<const char *> &NewArgv, bool MarkEOLs, bool RelativeNames) also seems to use llvm::sys::fs::current_path to resolve relative paths.

[kadircet]

clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp

168

I think it's not easy to reuse ExpandResponseFiles without changing code because it uses llvm::sys::fs::current_path to resolve relative paths.

Ah, I missed this, thanks for pointing it out.

In fact, most of the code here is copied from CommandLine

I believe in such a scenario, the correct course of action would be to update ExpandResponseFiles in CommandLine.h to accept a FileSystem &FS, instead of duplicating the logic.
Namely changing the signature to:

bool ExpandResponseFiles(StringSaver &Saver, TokenizerCallback Tokenizer,
                         llvm::vfs::FileSystem &FS, SmallVectorImpl<const char *> &Argv,
                         bool MarkEOLs = false, bool RelativeNames = false);

Then you can pass llvm::vfs::getRealFileSystem() to these function in existing call sites.
And create a FS with working directory set to Cmd.Directory via FileSystem::setCurrentWorkingDirectory in this specific call site you want to introduce.

Also to update the implementation of ExpandResponseFiles you would need to make use of FileSystem::getCurrentWorkingDirectory() instead of llvm::sys::fs::current_path and FileSystem::getBufferForFile() instead of llvm::MemoryBuffer::getFile the rest shouldn't need any changes hopefully, except plumbing VFS to some helpers.

Please send the signature change and call site updates in a separate patch though.

After that patch you can change signature for std::unique_ptr<CompilationDatabase> expandResponseFiles(std::unique_ptr<CompilationDatabase> Base) to accept a IntrusiveRefCntPtr<FileSystem> VFS, which will be getRealFileSystem in case of JSONCompilationDatabase. But this will enable you to pass a InMemoryFileSystem in the unittest below.

[lh123]

rebase to D70769

[merge_guards_bot]

Build result: fail - 60344 tests passed, 2 failed and 732 were skipped.

failed: Clang.CodeGen/catch-implicit-integer-sign-changes-incdec.c
failed: Clang.CodeGen/catch-implicit-signed-integer-truncations-incdec.c

Log files: console-log.txt, CMakeCache.txt

[lh123]

rebase

[merge_guards_bot]

Build result: FAILURE -
Log files: console-log.txt, CMakeCache.txt

[kadircet]

clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp

29

nit: FS(std::move(FS))

50

no need to restore/save, ExpandResponseFilesDatabase should have a FS that's not shared with others.

it is iffy anyway, because if initial working directory is faulty, we'll mutate it to the working directory of the last compile command.
or even if it wasn't faulty, someone could delete the directory before we can restore, or someone can change it while we are in the middle of ExpandResponseFile calls.

62

maybe make this check at the beginning of the for loop?

could you also leave a fixme saying that we should rather propagate the current directory into ExpandResponseFiles as well in addition to FS and someone can take a look at it later on.
That would be more principled than relying on ExpandResponseFilesDatabase having its own non-shared copy of FS.

clang/lib/Tooling/JSONCompilationDatabase.cpp

172

let's change this one to llvm::vfs::createPhysicalFileSystem to make sure we are passing a non-shared FS. Unfortunately, getRealFileSystem is shared with the whole process.

clang/unittests/Tooling/CompilationDatabaseTest.cpp

910

nit: instead of clang-format off maybe provide command as a raw string literal?

912

this test is checking the functionality of ExpandFileCommands helper, which is already tested in llvm/unittests/Support/CommandLineTest.cpp

it should be enough to have two simple tests:

• with a compile command that refers to a simple rsp file, and making sure it was expanded.
• with a regular compile command, making sure it stays the same.

[lh123]

address comments.

[merge_guards_bot]

Build result: FAILURE -
Log files: console-log.txt, CMakeCache.txt

[lh123]

fix bug.

[kadircet]

mostly LG, a few last nits.

clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp

52

nit: drop and someone can take a look at it later on.

clang/unittests/Tooling/CompilationDatabaseTest.cpp

865

make this part of the constructor instead of having a SetUp

866

move this to the test

870

nit: I would rather keep ASSERT_TRUE in here

877

nit: no need for braces

[lh123]

address comments.

[kadircet]

Thanks a lot for working on this patch, LGTM!

again please make sure the changes are clang-formatted.

Do you have commit access?

[lh123]

In D70222#1763553, @kadircet wrote:

Thanks a lot for working on this patch, LGTM!

again please make sure the changes are clang-formatted.

I'm sure the code has been formatted.

Do you have commit access?

I don't have commit access, please commit it for me.

clang/include/clang/Tooling/CompilationDatabase.h

 
 #ifndef LLVM_CLANG_TOOLING_COMPILATIONDATABASE_H
 #define LLVM_CLANG_TOOLING_COMPILATIONDATABASE_H
 
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include <memory>
 #include <string>
 #include <utility>
 #include <vector>
 
 namespace clang {
 namespace tooling {
 
std::unique_ptr<CompilationDatabase>
     inferMissingCompileCommands(std::unique_ptr<CompilationDatabase>);
 
 /// Returns a wrapped CompilationDatabase that will add -target and -mode flags
 /// to commandline when they can be deduced from argv[0] of commandline returned
 /// by underlying database.
 std::unique_ptr<CompilationDatabase>
 inferTargetAndDriverMode(std::unique_ptr<CompilationDatabase> Base);
 
+/// Returns a wrapped CompilationDatabase that will expand all rsp(response)
+/// files on commandline returned by underlying database.
+/// Note: This may change the working directory of FS.
+std::unique_ptr<CompilationDatabase>
+expandResponseFiles(std::unique_ptr<CompilationDatabase> Base,
+                    llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS);
+
 } // namespace tooling
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLING_COMPILATIONDATABASE_H

clang/lib/Tooling/CMakeLists.txt

 add_subdirectory(Transformer)
 
 add_clang_library(clangTooling
   AllTUsExecution.cpp
   ArgumentsAdjusters.cpp
   CommonOptionsParser.cpp
   CompilationDatabase.cpp
   Execution.cpp
+  ExpandResponseFilesCompilationDatabase.cpp
   FileMatchTrie.cpp
   FixIt.cpp
   GuessTargetAndModeCompilationDatabase.cpp
   InterpolatingCompilationDatabase.cpp
   JSONCompilationDatabase.cpp
   Refactoring.cpp
   RefactoringCallbacks.cpp
   StandaloneExecution.cpp

clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp

+//===- ExpandResponseFileCompilationDataBase.cpp --------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/Tooling/CompilationDatabase.h"
+#include "llvm/ADT/Triple.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/ConvertUTF.h"
+#include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/StringSaver.h"
+
+namespace clang {
+namespace tooling {
+namespace {
+
+class ExpandResponseFilesDatabase : public CompilationDatabase {
+public:
+  ExpandResponseFilesDatabase(
+      std::unique_ptr<CompilationDatabase> Base,
+      llvm::cl::TokenizerCallback Tokenizer,
+      llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS)
+      : Base(std::move(Base)), Tokenizer(Tokenizer), FS(std::move(FS)) {
+    assert(this->Base != nullptr);
+    assert(this->Tokenizer != nullptr);
+    assert(this->FS != nullptr);
+  }
+
+  std::vector<std::string> getAllFiles() const override {
+    return Base->getAllFiles();
+  }
+
+  std::vector<CompileCommand>
+  getCompileCommands(StringRef FilePath) const override {
+    return expand(Base->getCompileCommands(FilePath));
+  }
+
+  std::vector<CompileCommand> getAllCompileCommands() const override {
+    return expand(Base->getAllCompileCommands());
+  }
+
+private:
+  std::vector<CompileCommand> expand(std::vector<CompileCommand> Cmds) const {
+    for (auto &Cmd : Cmds) {
+      // FIXME: we should rather propagate the current directory into
+      // ExpandResponseFiles as well in addition to FS.
+      if (std::error_code EC = FS->setCurrentWorkingDirectory(Cmd.Directory)) {
+        llvm::consumeError(llvm::errorCodeToError(EC));
+        continue;
+      }
+      bool SeenRSPFile = false;
+      llvm::SmallVector<const char *, 20> Argv;
+      Argv.reserve(Cmd.CommandLine.size());
+      for (auto &Arg : Cmd.CommandLine) {
+        Argv.push_back(Arg.c_str());
+        SeenRSPFile |= Arg.front() == '@';
+      }
+      if (!SeenRSPFile)
+        continue;
+      llvm::BumpPtrAllocator Alloc;
+      llvm::StringSaver Saver(Alloc);
+      llvm::cl::ExpandResponseFiles(Saver, Tokenizer, Argv, false, false, *FS);
+      Cmd.CommandLine.assign(Argv.begin(), Argv.end());
+    }
+    return Cmds;
+  }
+
+private:
+  std::unique_ptr<CompilationDatabase> Base;
+  llvm::cl::TokenizerCallback Tokenizer;
+  llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS;
+};
+
+} // namespace
+
+std::unique_ptr<CompilationDatabase>
+expandResponseFiles(std::unique_ptr<CompilationDatabase> Base,
+                    llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS) {
+  auto Tokenizer = llvm::Triple(llvm::sys::getProcessTriple()).isOSWindows()
+                       ? llvm::cl::TokenizeWindowsCommandLine
+                       : llvm::cl::TokenizeGNUCommandLine;
+  return std::make_unique<ExpandResponseFilesDatabase>(
+      std::move(Base), Tokenizer, std::move(FS));
+}
+
+} // namespace tooling
+} // namespace clang

clang/lib/Tooling/JSONCompilationDatabase.cpp

 class JSONCompilationDatabasePlugin : public CompilationDatabasePlugin {
   std::unique_ptr<CompilationDatabase>
   loadFromDirectory(StringRef Directory, std::string &ErrorMessage) override {
     SmallString<1024> JSONDatabasePath(Directory);
     llvm::sys::path::append(JSONDatabasePath, "compile_commands.json");
     auto Base = JSONCompilationDatabase::loadFromFile(
         JSONDatabasePath, ErrorMessage, JSONCommandLineSyntax::AutoDetect);
     return Base ? inferTargetAndDriverMode(
-                      inferMissingCompileCommands(std::move(Base)))
+                      inferMissingCompileCommands(expandResponseFiles(
+                          std::move(Base),
+                          llvm::vfs::createPhysicalFileSystem().release())))
                 : nullptr;
   }
 };
 
 } // namespace
 
 // Register the JSONCompilationDatabasePlugin with the
 // CompilationDatabasePluginRegistry using this statically initialized variable.

clang/unittests/Tooling/CompilationDatabaseTest.cpp

TEST_F(TargetAndModeTest, TargetAndMode) {
   add("bar.cpp", "clang++", "");
 
   EXPECT_EQ(getCommand("foo.cpp"),
             "clang-cl --driver-mode=cl foo.cpp -D foo.cpp");
   EXPECT_EQ(getCommand("bar.cpp"),
             "clang++ --driver-mode=g++ bar.cpp -D bar.cpp");
 }
 
+class ExpandResponseFilesTest : public MemDBTest {
+public:
+  ExpandResponseFilesTest() : FS(new llvm::vfs::InMemoryFileSystem) {}
+
+protected:
+  void addFile(StringRef File, StringRef Content) {
+    ASSERT_TRUE(
+        FS->addFile(File, 0, llvm::MemoryBuffer::getMemBufferCopy(Content)));
+  }
+
+  std::string getCommand(llvm::StringRef F) {
+    auto Results = expandResponseFiles(std::make_unique<MemCDB>(Entries), FS)
+                       ->getCompileCommands(path(F));
+    if (Results.empty())
+      return "none";
+    return llvm::join(Results[0].CommandLine, " ");
+  }
+
+  llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> FS;
+};
+
+TEST_F(ExpandResponseFilesTest, ExpandResponseFiles) {
+  addFile(path(StringRef("rsp1.rsp")), "-Dflag");
+
+  add("foo.cpp", "clang", "@rsp1.rsp");
+  add("bar.cpp", "clang", "-Dflag");
+  EXPECT_EQ(getCommand("foo.cpp"), "clang foo.cpp -D foo.cpp -Dflag");
+  EXPECT_EQ(getCommand("bar.cpp"), "clang bar.cpp -D bar.cpp -Dflag");
+}
+
 } // end namespace tooling
 } // end namespace clang