[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

158–160

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

158–160

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

158–160

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

void finishedMainFile(DiagnosticsEngine &Diags) override {
     }
   }
 
 private:
   std::unique_ptr<DependencyOutputOptions> Opts;
   DependencyConsumer &C;
 };
 
-/// A proxy file system that doesn't call `chdir` when changing the working
-/// directory of a clang tool.
-class ProxyFileSystemWithoutChdir : public llvm::vfs::ProxyFileSystem {
-public:
-  ProxyFileSystemWithoutChdir(
-      llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS)
-      : 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 {
-    CWD = Path.str();
-    return {};
-  }
-
-private:
-  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(
       StringRef WorkingDirectory, DependencyConsumer &Consumer,
       llvm::IntrusiveRefCntPtr<DependencyScanningWorkerFilesystem> DepFS,
       ExcludedPreprocessorDirectiveSkipMapping *PPSkipMappings,

 
 } // end anonymous namespace
 
 DependencyScanningWorker::DependencyScanningWorker(
     DependencyScanningService &Service)
     : Format(Service.getFormat()) {
   DiagOpts = new DiagnosticOptions();
   PCHContainerOps = std::make_shared<PCHContainerOperations>();
-  RealFS = new ProxyFileSystemWithoutChdir(llvm::vfs::getRealFileSystem());
+  RealFS = llvm::vfs::createPhysicalFileSystem().release();
   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 --check-prefixes=CHECK1,CHECK2 %s
+
+// The output order is non-deterministic when using more than one thread,
+// so check the output using two runs.
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 2 | FileCheck --check-prefix=CHECK1 %s
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 2 | FileCheck --check-prefix=CHECK2 %s
+
+#include <header.h>
+
+// CHECK1: relative_directory_input1.o:
+// CHECK1-NEXT: relative_directory_input1.cpp
+// CHECK1-NEXT: header.h
+
+// CHECK2: relative_directory_input2.o:
+// CHECK2-NEXT: relative_directory_input2.cpp
+// CHECK2-NEXT: header.h