[Tooling] Do not restore working dir in ClangTool

[ilya-biryukov]

Summary

Resolve all relative paths before running the tool instead.

This fixes the usage of ClangTool in AllTUsExecutor. The executor will
try running multiple ClangTool instances in parallel with compile
commands that usually have the same working directory.

Changing working directory is a global operation, so we end up
changing working directory in the middle of running other actions,
which leads to spurious compile errors.

[ioeric]

lib/Tooling/Tooling.cpp

419 ↗

(On Diff #163028)

Should we still check if CWD is correctly set?

[ilya-biryukov]

lib/Tooling/Tooling.cpp

419 ↗

(On Diff #163028)

Not sure we know which CWD is correct at this point. Do you mean to check that getCurrentWorkingDirectory does not return an error?

[ioeric]

lib/Tooling/Tooling.cpp

419 ↗

(On Diff #163028)

Yes.

Actually, after a closer look, I noticed getAbsolutePath below is just using the current working directory in real file system, which seems wrong. This is pre-existing but would be nice to fix since we are here :)

[ilya-biryukov]

• Use vfs for file accesses
• Report errors when getAbsolutePath fails, skip those files

lib/Tooling/Tooling.cpp

419 ↗

(On Diff #163028)

What do we do if CWD is wrong? Checking for errors in getAbsolutePath and setCurrentWorkingDirectory should be enough and that's what we already do.

Actually, after a closer look, I noticed getAbsolutePath below is just using the current working directory in real file system, which seems wrong. This is pre-existing but would be nice to fix since we are here :)

Unfortunately, getAbsolutePath is a public API (with not too many usages, but still). I've added a helper function that goes through VFS and forwarding to it in the getAbsoltuePath for now, but we might want to look at changing the API there.

[ioeric]

lg

lib/Tooling/Tooling.cpp

202 ↗

(On Diff #163071)

I think this is a better alternative to the original getAbsolutePath as a public interface. Maybe also expose it?

[ilya-biryukov]

• Expose getAbsolutePath in the public API

cfe/trunk/include/clang/Tooling/Tooling.h

 /// the current directory and \c File.
 ///
 /// The difference to llvm::sys::fs::make_absolute is the canonicalization this
 /// does by removing "./" and computing native paths.
 ///
 /// \param File Either an absolute or relative path.
 std::string getAbsolutePath(StringRef File);
 
+/// An overload of getAbsolutePath that works over the provided \p FS.
+llvm::Expected<std::string> getAbsolutePath(vfs::FileSystem &FS,
+                                            StringRef File);
+
 /// Changes CommandLine to contain implicit flags that would have been
 /// defined had the compiler driver been invoked through the path InvokedAs.
 ///
 /// For example, when called with \c InvokedAs set to `i686-linux-android-g++`,
 /// the arguments '-target', 'i686-linux-android`, `--driver-mode=g++` will
 /// be inserted after the first argument in \c CommandLine.
 ///
 /// This function will not add new `-target` or `--driver-mode` flags if they

cfe/trunk/lib/Tooling/Tooling.cpp

InMemoryFileSystem->addFile(
         FilenameWithContent.first, 0,
         llvm::MemoryBuffer::getMemBuffer(FilenameWithContent.second));
   }
 
   return runToolOnCodeWithArgs(ToolAction, Code, OverlayFileSystem, Args,
                                FileName, ToolName);
 }
 
-std::string getAbsolutePath(StringRef File) {
+llvm::Expected<std::string> getAbsolutePath(vfs::FileSystem &FS,
+                                            StringRef File) {
   StringRef RelativePath(File);
   // FIXME: Should '.\\' be accepted on Win32?
   if (RelativePath.startswith("./")) {
     RelativePath = RelativePath.substr(strlen("./"));
   }
 
   SmallString<1024> AbsolutePath = RelativePath;
-  std::error_code EC = llvm::sys::fs::make_absolute(AbsolutePath);
-  assert(!EC);
-  (void)EC;
+  if (auto EC = FS.makeAbsolute(AbsolutePath))
+    return llvm::errorCodeToError(EC);
   llvm::sys::path::native(AbsolutePath);
   return AbsolutePath.str();
 }
 
+std::string getAbsolutePath(StringRef File) {
+  return llvm::cantFail(getAbsolutePath(*vfs::getRealFileSystem(), File));
+}
+
 void addTargetAndModeForProgramName(std::vector<std::string> &CommandLine,
                                     StringRef InvokedAs) {
   if (!CommandLine.empty() && !InvokedAs.empty()) {
     bool AlreadyHasTarget = false;
     bool AlreadyHasMode = false;
     // Skip CommandLine[0].
     for (auto Token = ++CommandLine.begin(); Token != CommandLine.end();
          ++Token) {
Args.push_back("-resource-dir=" +
                  CompilerInvocation::GetResourcesPath(Argv0, MainAddr));
 }
 
 int ClangTool::run(ToolAction *Action) {
   // Exists solely for the purpose of lookup of the resource path.
   // This just needs to be some symbol in the binary.
   static int StaticSymbol;
 
-  std::string InitialDirectory;
-  if (llvm::ErrorOr<std::string> CWD =
-          OverlayFileSystem->getCurrentWorkingDirectory()) {
-    InitialDirectory = std::move(*CWD);
-  } else {
-    llvm::report_fatal_error("Cannot detect current path: " +
-                             Twine(CWD.getError().message()));
-  }
-
   // First insert all absolute paths into the in-memory VFS. These are global
   // for all compile commands.
   if (SeenWorkingDirectories.insert("/").second)
     for (const auto &MappedFile : MappedFileContents)
       if (llvm::sys::path::is_absolute(MappedFile.first))
         InMemoryFileSystem->addFile(
             MappedFile.first, 0,
             llvm::MemoryBuffer::getMemBuffer(MappedFile.second));
 
   bool ProcessingFailed = false;
   bool FileSkipped = false;
+  // Compute all absolute paths before we run any actions, as those will change
+  // the working directory.
+  std::vector<std::string> AbsolutePaths;
+  AbsolutePaths.reserve(SourcePaths.size());
   for (const auto &SourcePath : SourcePaths) {
-    std::string File(getAbsolutePath(SourcePath));
+    auto AbsPath = getAbsolutePath(*OverlayFileSystem, SourcePath);
+    if (!AbsPath) {
+      llvm::errs() << "Skipping " << SourcePath
+                   << ". Error while getting an absolute path: "
+                   << llvm::toString(AbsPath.takeError()) << "\n";
+      continue;
+    }
+    AbsolutePaths.push_back(std::move(*AbsPath));
+  }
 
+  for (llvm::StringRef File : AbsolutePaths) {
     // Currently implementations of CompilationDatabase::getCompileCommands can
     // change the state of the file system (e.g.  prepare generated headers), so
     // this method needs to run right before we invoke the tool, as the next
     // file may require a different (incompatible) state of the file system.
     //
     // FIXME: Make the compilation database interface more explicit about the
     // requirements to the order of invocation of its members.
     std::vector<CompileCommand> CompileCommandsForFile =
for (CompileCommand &CompileCommand : CompileCommandsForFile) {
                                 PCHContainerOps);
       Invocation.setDiagnosticConsumer(DiagConsumer);
 
       if (!Invocation.run()) {
         // FIXME: Diagnostics should be used instead.
         llvm::errs() << "Error while processing " << File << ".\n";
         ProcessingFailed = true;
       }
-      // Return to the initial directory to correctly resolve next file by
-      // relative path.
-      if (OverlayFileSystem->setCurrentWorkingDirectory(InitialDirectory.c_str()))
-        llvm::report_fatal_error("Cannot chdir into \"" +
-                                 Twine(InitialDirectory) + "\n!");
     }
   }
   return ProcessingFailed ? 1 : (FileSkipped ? 2 : 0);
 }
 
 namespace {
 
 class ASTBuilderAction : public ToolAction {