[clang-scan-deps] Fix for input file given as relative path in compilation database "command" entry

[saudi]

Summary

The bug appeared when clang-scan-deps was run from a different directory than the one provided in the "directory" entry.

Bug: https://bugs.llvm.org/show_bug.cgi?id=47252

[dexonsmith]

Can we use getPhysicalFileSystem() instead of getRealFileSystem() here?

[saudi]

Updated the patch.

Followed suggestion from @dexonsmith. Indeed it simplifies the code.
Also, improved the test, to also test with -j 2

[dexonsmith]

In D91204#2387050, @saudi wrote:

Updated the patch.

Followed suggestion from @dexonsmith. Indeed it simplifies the code.
Also, improved the test, to also test with -j 2

A couple of more comments. @arphaman , can you confirm this is a good direction?

clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

216–218

I think this can just be:

RealFS = llvm::vfs::createPhysicalFileSystem();

The ProxyFileSystem wrapper is a convenience for building other file systems; it doesn't do anything on its own IIRC.

clang/test/ClangScanDeps/relative_directory.cpp

13–14

Can you use CHECK-DAG for this?
https://www.llvm.org/docs/CommandGuide/FileCheck.html#the-check-dag-directive

20–26

I.e., maybe this can be:

// CHECK-DAG: relative_directory_input1.o:
// CHECK-DAG:   relative_directory_input1.cpp
// CHECK-DAG:   header.h

// CHECK-DAG: relative_directory_input2.o:
// CHECK-DAG:   relative_directory_input2.cpp
// CHECK-DAG:   header.h

[saudi]

Removed use of ProxyFileSystem

[saudi]

clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

216–218

Note that createPhysicalFileSystem() returns a std::unique_ptr whereas getRealFileSystem() return type is llvm::IntrusiveRfCntPtr.

Trying to change the type of RealFS requires more changes, as DependencyScanningWorkerFilesystem is itself a ProxyFileSystem and thus requires a llvm::IntrusiveRfCntPtr object.

Should DependencyScanningWorkerFilesystem not be a ProxyFileSystem? @arphaman WDYT?
However, in this case it might fall out of the scope of this patch, I'd suggest we keep it as a separate patch.

I updated my patch for an intermediate fix, which also passes the tests:

RealFS = llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>(llvm::vfs::createPhysicalFileSystem().release());

clang/test/ClangScanDeps/relative_directory.cpp

13–14

If I understand correctly from the doc, it would lose the order (CHECK1, CHECK1-NEXT).
We need 2 blocks of 3 lines to be present, but only the blocks can be in any order.

I did this mimicking what is done in other tests like regular_cdb.cpp.

[dexonsmith]

clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

216–218

Ah, didn't realize it returned unique_ptr. RealFS should be IntrusiveRefCntPtr, but you can be less verbose like this:

RealFS = llvm::vfs::createPhysicalFileSystem().release();

[dexonsmith]

clang/test/ClangScanDeps/relative_directory.cpp

13–14

If I understand correctly from the doc, it would lose the order (CHECK1, CHECK1-NEXT).

We need 2 blocks of 3 lines to be present, but only the blocks can be in any order.

That's correct, but I think it's safe to assume in this test that the general format of the output is correct. Best would be to add a CHECK-DAG-NEXT: feature to FileCheck but that seems above and beyond for fixing this bug.

I did this mimicking what is done in other tests like regular_cdb.cpp.

Sure, it's probably fine either way, and it might be better to match the other tests.

[saudi]

Simplified the RealFS initialization, as suggested

[dexonsmith]

This LGTM, but I'd like @arphaman to look as well in case he originally tried getPhysicalFileSystem and it caused a problem. I've switched him to a blocking reviewer.

[arphaman]

This make sense to me, thank you

clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

ProxyFileSystemWithoutChdir(
       : ProxyFileSystem(std::move(FS)) {}
 
   llvm::ErrorOr<std::string> getCurrentWorkingDirectory() const override {
     assert(!CWD.empty() && "empty CWD");
     return CWD;
   }
 
   std::error_code setCurrentWorkingDirectory(const Twine &Path) override {
+    assert(llvm::sys::path::is_absolute(Path) && "relative CWD");
     CWD = Path.str();
     return {};
   }
 
+  // Provide absolute paths to the underlying FS, to prevent it from using
+  // the system's CWD.
+  llvm::ErrorOr<llvm::vfs::Status> status(const Twine &Path) override {
+    SmallString<256> Storage;
+    return ProxyFileSystem::status(adjustPath(Path, Storage));
+  }
+
+  llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
+  openFileForRead(const Twine &Path) override {
+    SmallString<256> Storage;
+    return ProxyFileSystem::openFileForRead(adjustPath(Path, Storage));
+  }
+  llvm::vfs::directory_iterator dir_begin(const Twine &Dir,
+                                          std::error_code &EC) override {
+    SmallString<256> Storage;
+    return ProxyFileSystem::dir_begin(adjustPath(Dir, Storage), EC);
+  }
+  std::error_code getRealPath(const Twine &Path,
+                              SmallVectorImpl<char> &Output) const override {
+    SmallString<256> Storage;
+    return ProxyFileSystem::getRealPath(adjustPath(Path, Storage), Output);
+  }
+  std::error_code isLocal(const Twine &Path, bool &Result) override {
+    SmallString<256> Storage;
+    return ProxyFileSystem::isLocal(adjustPath(Path, Storage), Result);
+  }
+
 private:
+  Twine adjustPath(const Twine &Path, SmallVectorImpl<char> &Storage) const {
+    assert(!CWD.empty() && "empty CWD");
+
+    Path.toVector(Storage);
+    llvm::sys::fs::make_absolute(CWD, Storage);
+    return Storage;
+  }
+
   std::string CWD;
 };
 
 /// A clang tool that runs the preprocessor in a mode that's optimized for
 /// dependency scanning for the given compiler invocation.
 class DependencyScanningAction : public tooling::ToolAction {
 public:
   DependencyScanningAction(

 } // end anonymous namespace
 
 DependencyScanningWorker::DependencyScanningWorker(
     DependencyScanningService &Service)
     : Format(Service.getFormat()) {
   DiagOpts = new DiagnosticOptions();
   PCHContainerOps = std::make_shared<PCHContainerOperations>();
   RealFS = new ProxyFileSystemWithoutChdir(llvm::vfs::getRealFileSystem());
   if (Service.canSkipExcludedPPRanges())
     PPSkipMappings =
         std::make_unique<ExcludedPreprocessorDirectiveSkipMapping>();
   if (Service.getMode() == ScanningMode::MinimizedSourcePreprocessing)
     DepFS = new DependencyScanningWorkerFilesystem(
         Service.getSharedCache(), RealFS, PPSkipMappings.get());
   if (Service.canReuseFileManager())
     Files = new FileManager(FileSystemOptions(), RealFS);
 }
 
 static llvm::Error runWithDiags(

clang/test/ClangScanDeps/Inputs/relative_directory.json

+[
+{
+  "directory": "DIR",
+  "command": "clang -E Inputs/relative_directory_input1.cpp -IInputs",
+  "file": "DIR/Inputs/relative_directory_input1.cpp"
+},
+{
+  "directory": "DIR/Inputs",
+  "command": "clang -E relative_directory_input2.cpp -I.",
+  "file": "DIR/Inputs/relative_directory_input2.cpp"
+}
+]

clang/test/ClangScanDeps/relative_directory.cpp

+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.cdb
+// RUN: mkdir -p %t.dir
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %s %t.dir/Inputs/relative_directory_input1.cpp
+// RUN: cp %s %t.dir/Inputs/relative_directory_input2.cpp
+// RUN: touch %t.dir/Inputs/header.h
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/relative_directory.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#include <header.h>
+
+// CHECK: relative_directory_input1.o:
+// CHECK-NEXT: relative_directory_input1.cpp
+// CHECK-NEXT: header.h
+
+// CHECK: relative_directory_input2.o:
+// CHECK-NEXT: relative_directory_input2.cpp
+// CHECK-NEXT: header.h