[clang] [DependencyScanningFilesystem] Make sure the local/shared cache filename lookups use only absolute paths (PR #66122)

Argyrios Kyrtzidis via cfe-commits cfe-commits at lists.llvm.org
Fri Sep 15 11:29:07 PDT 2023


================
@@ -330,3 +353,20 @@ DependencyScanningWorkerFilesystem::openFileForRead(const Twine &Path) {
     return Result.getError();
   return DepScanFile::create(Result.get());
 }
+
+std::error_code DependencyScanningWorkerFilesystem::setCurrentWorkingDirectory(
+    const Twine &Path) {
+  updateWorkingDirForCacheLookup(Path.str());
+  return ProxyFileSystem::setCurrentWorkingDirectory(Path);
+}
+
+void DependencyScanningWorkerFilesystem::updateWorkingDirForCacheLookup(
+    llvm::ErrorOr<std::string> CWD) {
+  if (CWD && !CWD->empty()) {
+    WorkingDirForCacheLookup = *CWD;
+  } else {
+    // The cache lookup functions will not accept relative paths for safety, so
+    // at least make it absolute from a "root".
+    WorkingDirForCacheLookup = "/";
----------------
akyrtzi wrote:

> In this case the path will be used when writing to the cache though, right? That affects scan results for other TUs just like you are fixing in this PR.

What is the specific concern? If multiple TUs got errors for working directory and subsequently used the same standard prefix then they may resolve to same path for a relative filename but these TUs are already in an erroneous state already, it's not invalidating their correctness.

> I'm not clear what recovery means here

My general point is that I don't think we should make the assumption that if a VFS implementation emits an error for `setCurrentWorkingDirectory()` then that VFS instance should not ever get used again subsequently.
We could say this is specific for the `DependencyScanningWorkerFilesystem` implementation but I don't see a motivating reason that it should not be able to continue operating instead of crashing if it gets used again.

https://github.com/llvm/llvm-project/pull/66122


More information about the cfe-commits mailing list