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

Jan Svoboda via cfe-commits cfe-commits at lists.llvm.org
Thu Sep 14 12:43:19 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 = "/";
----------------
jansvoboda11 wrote:

I would prefer if we avoided inventing a dummy value.

What do you think about enforcing that `DependencyScanningWorkerFilesystem` never gets constructed from a VFS that either does not have a working directory, or has one that's relative? We could do that by creating a factory function that returns `llvm::Expected<DependencyScanningWorkerFilesystem>` instead of using the constructor.

For the `setCurrentWorkingDirectory()` call, I think it'd be reasonable to clear our CWD if the new one is empty or relative, return an error code and assume the VFS doesn't get used in that state. If it does, hitting one of the "is file path absolute" assertions is fair game.

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


More information about the cfe-commits mailing list