[clang][deps] Fix dependency scanning with -working-directory

[benlangmuir]

Stop overriding -working-directory to CWD during argument parsing, which should no longer necessary after we set the VFS working directory, and set FSOpts correctly after parsing arguments so that working-directory behaves correctly.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Ben Langmuir (benlangmuir)

Changes

Stop overriding -working-directory to CWD during argument parsing, which should no longer necessary after we set the VFS working directory, and set FSOpts correctly after parsing arguments so that working-directory behaves correctly.

---

Full diff: https://github.com/llvm/llvm-project/pull/84525.diff

2 Files Affected:

• (modified) clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp (+5-3)
• (added) clang/test/ClangScanDeps/working-directory-option.c (+30)

diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
index 2b882f8a5e0793..f7c302854a2479 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -342,6 +342,9 @@ class DependencyScanningAction : public tooling::ToolAction {
     ScanInstance.getHeaderSearchOpts().ModulesIncludeVFSUsage =
         any(OptimizeArgs & ScanningOptimizations::VFS);
 
+    // FileMgr was created before option parsing; set its FileSystemOptions now.
+    FileMgr->getFileSystemOpts() = ScanInstance.getFileSystemOpts();
+
     ScanInstance.setFileManager(FileMgr);
     // Support for virtual file system overlays.
     FileMgr->setVirtualFileSystem(createVFSFromCompilerInvocation(
@@ -624,9 +627,8 @@ bool DependencyScanningWorker::computeDependencies(
       ModifiedCommandLine ? *ModifiedCommandLine : CommandLine;
   auto &FinalFS = ModifiedFS ? ModifiedFS : BaseFS;
 
-  FileSystemOptions FSOpts;
-  FSOpts.WorkingDir = WorkingDirectory.str();
-  auto FileMgr = llvm::makeIntrusiveRefCnt<FileManager>(FSOpts, FinalFS);
+  auto FileMgr =
+      llvm::makeIntrusiveRefCnt<FileManager>(FileSystemOptions{}, FinalFS);
 
   std::vector<const char *> FinalCCommandLine(FinalCommandLine.size(), nullptr);
   llvm::transform(FinalCommandLine, FinalCCommandLine.begin(),
diff --git a/clang/test/ClangScanDeps/working-directory-option.c b/clang/test/ClangScanDeps/working-directory-option.c
new file mode 100644
index 00000000000000..d57497d405d30f
--- /dev/null
+++ b/clang/test/ClangScanDeps/working-directory-option.c
@@ -0,0 +1,30 @@
+// Test that -working-directory works even when it differs from the working
+// directory of the filesystem.
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/other
+// RUN: split-file %s %t
+// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full \
+// RUN:   > %t/deps.json
+
+// RUN: cat %t/deps.json | sed 's:\\\\\?:/:g' | FileCheck %s -DPREFIX=%/t
+
+// CHECK:      "file-deps": [
+// CHECK-NEXT:   "[[PREFIX]]/cwd/t.c"
+// CHECK-NEXT:   "[[PREFIX]]/cwd/relative/h1.h"
+// CHECK-NEXT: ]
+// CHECK-NEXT: "input-file": "[[PREFIX]]/cwd/t.c"
+
+//--- cdb.json.template
+[{
+  "directory": "DIR/other",
+  "command": "clang -c t.c -I relative -working-directory DIR/cwd",
+  "file": "DIR/cwd/t.c"
+}]
+
+//--- cwd/relative/h1.h
+
+//--- cwd/t.c
+#include "h1.h"

[jansvoboda11]

Do you have any concerns about the consistency of FileManager caches?

I can see a situation where we ask FileManager about the same relative path before and after setting the parsed FileSystemOptions. The second call would blindly return the cached result, effectively ignoring -working-directory for that file.

[benlangmuir]

I can see a situation where we ask FileManager about the same relative path before and after setting the parsed FileSystemOptions. The second call would blindly return the cached result, effectively ignoring -working-directory for that file.

The driver calls VFS->setCurrentWorkingDirectory using the value of -working-directory. So in general relative paths will be resolved consistently with -working-directory if they're seen before configuring the FileManager. Now, a fair question is why are we changing both the VFS working directory and making paths absolute in the FileManager for -working-directory, which seems redundant, but I'm not looking to change that behaviour -- this PR should make scanning behave more consistently with compilation.

To be clear this situation with mismatched relative paths would currently be broken, because the way we were setting FSOpts.WorkingDir was not using the value of -working-directory before this change.

[jansvoboda11]

To clarify, I'm not concerned about the general issue of FileManager and VFS working directory mismatch. What I'm wary of is that this patch changes FileSystemOptions after creating and (potentially) using FileManager for a while. It uses relative paths as cache keys:

auto FileMgr = llvm::makeIntrusiveRefCnt<FileManager>(FileSystemOptions{}, FinalFS);
FileMgr->getOptionalFileRef("tu.c"); // caches "<VFS-CWD>/tu.c"
FileMgr->getFileSystemOpts() = ScanInstance.getFileSystemOpts(); // with WorkingDir = "<CLI-CWD>"
FileMgr->getOptionalFileRef("tu.c"); // returns cached "<VFS-CWD>/tu.c" instead of looking up "<CLI-CWD>/tu.c"

The driver calls VFS->setCurrentWorkingDirectory using the value of -working-directory. So in general relative paths will be resolved consistently with -working-directory if they're seen before configuring the FileManager.

Right, the driver uses VFS directly instead of going through FileManager, so caching isn't a concern. Driver's usage of DiagnosticsEngine (configured with FileManager) probably won't cause any actual file lookups. We're also passing FileManager to tooling::ToolInvocation, but that doesn't do much besides invoking the driver. Any of this might change, though.

We could prevent this by throwing away the provisional FileManager and its clients after dealing with the driver command line and create new ones before scanning.

[benlangmuir]

I don't think there's a difference we can test for here -- the VFS WD shouldn't be modified after the driver sets it and before the FM is used here, so it would be identical to -working-directory during the critical window. But I agree recreating the FileManager is probably a better solution since it defines away this problem. I'll look at doing it that way.

[jansvoboda11]

LGTM, thanks!