[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 ↗

(On Diff #230060)

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 ↗

(On Diff #230756)

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

230 ↗

(On Diff #230756)

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

167 ↗

(On Diff #230756)

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

167 ↗

(On Diff #230756)

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

167 ↗

(On Diff #230756)

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

167 ↗

(On Diff #230756)

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

167 ↗

(On Diff #230756)

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

28 ↗

(On Diff #231235)

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

49 ↗

(On Diff #231235)

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.

61 ↗

(On Diff #231235)

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 ↗

(On Diff #231235)

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 ↗

(On Diff #231235)

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

912 ↗

(On Diff #231235)

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

51 ↗

(On Diff #231362)

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

clang/unittests/Tooling/CompilationDatabaseTest.cpp

865 ↗

(On Diff #231362)

make this part of the constructor instead of having a SetUp

866 ↗

(On Diff #231362)

move this to the test

870 ↗

(On Diff #231362)

nit: I would rather keep ASSERT_TRUE in here

877 ↗

(On Diff #231362)

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-tools-extra/clangd/GlobalCompilationDatabase.cpp

 #include "Path.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Tooling/ArgumentsAdjusters.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/StringSaver.h"
 #include <string>
 #include <tuple>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 namespace {
 
+bool expandResponseFile(llvm::StringRef FName, llvm::StringSaver &Saver,
+                        llvm::cl::TokenizerCallback Tokenizer,
+                        SmallVectorImpl<const char *> &NewArgv) {
+  llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> MemBufOrErr =
+      llvm::MemoryBuffer::getFile(FName);
+  if (!MemBufOrErr)
+    return false;
+  llvm::MemoryBuffer &MemBuf = *MemBufOrErr.get();
+  StringRef Str(MemBuf.getBufferStart(), MemBuf.getBufferSize());
+
+  // If we have a UTF-16 byte order mark, convert to UTF-8 for parsing.
+  ArrayRef<char> BufRef(MemBuf.getBufferStart(), MemBuf.getBufferEnd());
+  std::string UTF8Buf;
+  // It is called byte order marker but the UTF-8 BOM is actually not affected
+  // by the host system's endianness.
+  auto HasUtF8ByteOrderMark = [](ArrayRef<char> S) {
+    return (S.size() >= 3 && S[0] == '\xef' && S[1] == '\xbb' &&
+            S[2] == '\xbf');
+  };
+  if (llvm::hasUTF16ByteOrderMark(BufRef)) {
+    if (!convertUTF16ToUTF8String(BufRef, UTF8Buf))
+      return false;
+    Str = StringRef(UTF8Buf);
+  }
+  // If we see UTF-8 BOM sequence at the beginning of a file, we shall remove
+  // these bytes before parsing.
+  // Reference: http://en.wikipedia.org/wiki/UTF-8#Byte_order_mark
+  else if (HasUtF8ByteOrderMark(BufRef))
+    Str = StringRef(BufRef.data() + 3, BufRef.size() - 3);
+  // Tokenize the contents into NewArgv.
+  Tokenizer(Str, Saver, NewArgv, false);
+  return true;
+}
+
+bool expandResponseFiles(tooling::CompileCommand &Cmd,
+                         llvm::cl::TokenizerCallback Tokenizer) {
+  bool AllExpanded = true;
+  struct ResponseFileRecord {
+    llvm::StringRef File;
+    size_t End;
+  };
+  std::vector<std::string> &Argv = Cmd.CommandLine;
+  // To detect recursive response files, we maintain a stack of files and the
+  // position of the last argument in the file. This position is updated
+  // dynamically as we recursively expand files.
+  SmallVector<ResponseFileRecord, 3> FileStack;
+
+  // Push a dummy entry that represents the initial command line, removing
+  // the need to check for an empty list.
+  FileStack.push_back({"", Argv.size()});
+
+  // Don't cache Argv.size() because it can change.
+  for (unsigned I = 0; I != Argv.size();) {
+    while (I == FileStack.back().End) {
+      // Passing the end of a file's argument list, so we can remove it from the
+      // stack.
+      FileStack.pop_back();
+    }
+
+    std::string &Arg = Argv[I];
+
+    if (Arg[0] != '@') {
+      ++I;
+      continue;
+    }
+    SmallString<128> ResponseFile;
+    if (llvm::sys::path::is_relative(&Arg[1])) {
+      llvm::sys::path::append(ResponseFile, Cmd.Directory, &Arg[1]);
+    }
+    llvm::sys::path::remove_dots(ResponseFile);
+
+    auto IsEquivalent = [ResponseFile](const ResponseFileRecord &RFile) {
+      return llvm::sys::fs::equivalent(RFile.File, ResponseFile);
+    };
+
+    // Check for recursive response files.
+    if (std::any_of(FileStack.begin() + 1, FileStack.end(), IsEquivalent)) {
+      // This file is recursive, so we leave it in the argument stream and
+      // move on.
+      AllExpanded = false;
+      ++I;
+      continue;
+    }
+
+    // Replace this response file argument with the tokenization of its
+    // contents.  Nested response files are expanded in subsequent iterations.
+    SmallVector<const char *, 0> ExpandedArgv;
+    llvm::BumpPtrAllocator Alloc;
+    llvm::StringSaver Saver(Alloc);
+    llvm::SmallVector<const char *, 64> T;
+    if (!expandResponseFile(ResponseFile, Saver, Tokenizer, ExpandedArgv)) {
+      // We couldn't read this file, so we leave it in the argument stream and
+      // move on.
+      AllExpanded = false;
+      ++I;
+      continue;
+    }
+
+    for (ResponseFileRecord &Record : FileStack) {
+      // Increase the end of all active records by the number of newly expanded
+      // arguments, minus the response file itself.
+      Record.End += ExpandedArgv.size() - 1;
+    }
+
+    FileStack.push_back({ResponseFile, I + ExpandedArgv.size()});
+    Argv.erase(Argv.begin() + I);
+    Argv.insert(Argv.begin() + I, ExpandedArgv.begin(), ExpandedArgv.end());
+  }
+
+  // If successful, the top of the file stack will mark the end of the Argv
+  // stream. A failure here indicates a bug in the stack popping logic above.
+  // Note that FileStack may have more than one element at this point because we
+  // don't have a chance to pop the stack when encountering recursive files at
+  // the end of the stream, so seeing that doesn't indicate a bug.
+  assert(FileStack.size() > 0 && Argv.size() == FileStack.back().End);
+  return AllExpanded;
+}
+
 void adjustArguments(tooling::CompileCommand &Cmd,
                      llvm::StringRef ResourceDir) {
   tooling::ArgumentsAdjuster ArgsAdjuster = tooling::combineAdjusters(
       // clangd should not write files to disk, including dependency files
       // requested on the command line.
       tooling::getClangStripDependencyFileAdjuster(),
       // Strip plugin related command line arguments. Clangd does
       // not support plugins currently. Therefore it breaks if
       // compiler tries to load plugins.
       tooling::combineAdjusters(tooling::getStripPluginsAdjuster(),
                                 tooling::getClangSyntaxOnlyAdjuster()));
 
   Cmd.CommandLine = ArgsAdjuster(Cmd.CommandLine, Cmd.Filename);
+
+  // ExpandResponseFile
+  auto Tokenizer = llvm::Triple(llvm::sys::getProcessTriple()).isOSWindows()
+                       ? llvm::cl::TokenizeWindowsCommandLine
+                       : llvm::cl::TokenizeGNUCommandLine;
+  if (!expandResponseFiles(Cmd, Tokenizer))
+    log("Failed to expand response files for {0}", Cmd.Filename);
+
   // Inject the resource dir.
   // FIXME: Don't overwrite it if it's already there.
   if (!ResourceDir.empty())
     Cmd.CommandLine.push_back(("-resource-dir=" + ResourceDir).str());
 }
 
 std::string getStandardResourceDir() {
   static int Dummy; // Just an address in this process.