Merge pull request #5113 from bnbarham/cherry-fm

[stable/20220421] Cherry-pick FileManager and FileEntry updates

Author: bnbarham

clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

@@ -24,6 +24,7 @@
 #include "clang/Tooling/ReplacementsYaml.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
@@ -140,7 +141,7 @@ std::error_code collectReplacementsFromDirectory(
 static llvm::DenseMap<const FileEntry *, std::vector<tooling::Replacement>>
 groupReplacements(const TUReplacements &TUs, const TUDiagnostics &TUDs,
                   const clang::SourceManager &SM) {
-  std::set<StringRef> Warned;
+  llvm::StringSet<> Warned;
   llvm::DenseMap<const FileEntry *, std::vector<tooling::Replacement>>
       GroupedReplacements;
 
@@ -156,11 +157,15 @@ groupReplacements(const TUReplacements &TUs, const TUDiagnostics &TUDs,
                         const tooling::TranslationUnitDiagnostics *SourceTU,
                         const llvm::Optional<std::string> BuildDir) {
     // Use the file manager to deduplicate paths. FileEntries are
-    // automatically canonicalized.
-    auto PrevWorkingDir = SM.getFileManager().getFileSystemOpts().WorkingDir;
+    // automatically canonicalized. Since relative paths can come from different
+    // build directories, make them absolute immediately.
+    SmallString<128> Path = R.getFilePath();
     if (BuildDir)
-      SM.getFileManager().getFileSystemOpts().WorkingDir = std::move(*BuildDir);
-    if (auto Entry = SM.getFileManager().getFile(R.getFilePath())) {
+      llvm::sys::fs::make_absolute(*BuildDir, Path);
+    else
+      SM.getFileManager().makeAbsolutePath(Path);
+
+    if (auto Entry = SM.getFileManager().getFile(Path)) {
       if (SourceTU) {
         auto &Replaces = DiagReplacements[*Entry];
         auto It = Replaces.find(R);
@@ -171,11 +176,10 @@ groupReplacements(const TUReplacements &TUs, const TUDiagnostics &TUDs,
           return;
       }
       GroupedReplacements[*Entry].push_back(R);
-    } else if (Warned.insert(R.getFilePath()).second) {
+    } else if (Warned.insert(Path).second) {
       errs() << "Described file '" << R.getFilePath()
              << "' doesn't exist. Ignoring...\n";
     }
-    SM.getFileManager().getFileSystemOpts().WorkingDir = PrevWorkingDir;
   };
 
   for (const auto &TU : TUs)

clang/include/clang/Basic/FileEntry.h

@@ -59,11 +59,21 @@ class FileEntry;
 /// accessed by the FileManager's client.
 class FileEntryRef {
 public:
-  StringRef getName() const { return ME->first(); }
+  /// The name of this FileEntry. If a VFS uses 'use-external-name', this is
+  /// the redirected name. See getRequestedName().
+  StringRef getName() const { return getBaseMapEntry().first(); }
+
+  /// The name of this FileEntry, as originally requested without applying any
+  /// remappings for VFS 'use-external-name'.
+  ///
+  /// FIXME: this should be the semantics of getName(). See comment in
+  /// FileManager::getFileRef().
+  StringRef getNameAsRequested() const { return ME->first(); }
+
   const FileEntry &getFileEntry() const {
-    return *ME->second->V.get<FileEntry *>();
+    return *getBaseMapEntry().second->V.get<FileEntry *>();
   }
-  DirectoryEntryRef getDir() const { return *ME->second->Dir; }
+  DirectoryEntryRef getDir() const { return *getBaseMapEntry().second->Dir; }
 
   inline off_t getSize() const;
   inline unsigned getUID() const;
@@ -150,13 +160,20 @@ class FileEntryRef {
   explicit FileEntryRef(const MapEntry &ME) : ME(&ME) {
     assert(ME.second && "Expected payload");
     assert(ME.second->V && "Expected non-null");
-    assert(ME.second->V.is<FileEntry *>() && "Expected FileEntry");
   }
 
   /// Expose the underlying MapEntry to simplify packing in a PointerIntPair or
   /// PointerUnion and allow construction in Optional.
   const clang::FileEntryRef::MapEntry &getMapEntry() const { return *ME; }
 
+  /// Retrieve the base MapEntry after redirects.
+  const MapEntry &getBaseMapEntry() const {
+    const MapEntry *ME = this->ME;
+    while (const void *Next = ME->second->V.dyn_cast<const void *>())
+      ME = static_cast<const MapEntry *>(Next);
+    return *ME;
+  }
+
 private:
   friend class FileMgr::MapEntryOptionalStorage<FileEntryRef>;
   struct optional_none_tag {};

clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h

@@ -233,6 +233,11 @@ class ModuleDepCollector final : public DependencyCollector {
   /// Checks whether the module is known as being prebuilt.
   bool isPrebuiltModule(const Module *M);
 
+  /// Adds \p Path to \c FileDeps, making it absolute if necessary.
+  void addFileDep(StringRef Path);
+  /// Adds \p Path to \c MD.FileDeps, making it absolute if necessary.
+  void addFileDep(ModuleDeps &MD, StringRef Path);
+
   /// Constructs a CompilerInvocation that can be used to build the given
   /// module, excluding paths to discovered modular dependencies that are yet to
   /// be built.

clang/lib/Basic/FileManager.cpp

@@ -213,13 +213,7 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
     if (!SeenFileInsertResult.first->second)
       return llvm::errorCodeToError(
           SeenFileInsertResult.first->second.getError());
-    // Construct and return and FileEntryRef, unless it's a redirect to another
-    // filename.
-    FileEntryRef::MapValue Value = *SeenFileInsertResult.first->second;
-    if (LLVM_LIKELY(Value.V.is<FileEntry *>()))
-      return FileEntryRef(*SeenFileInsertResult.first);
-    return FileEntryRef(*reinterpret_cast<const FileEntryRef::MapEntry *>(
-        Value.V.get<const void *>()));
+    return FileEntryRef(*SeenFileInsertResult.first);
   }
 
   // We've not seen this before. Fill it in.
@@ -275,8 +269,8 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
   if (!UFE)
     UFE = new (FilesAlloc.Allocate()) FileEntry();
 
-  if (Status.getName() == Filename) {
-    // The name matches. Set the FileEntry.
+  if (!Status.ExposesExternalVFSPath || Status.getName() == Filename) {
+    // Use the requested name. Set the FileEntry.
     NamedFileEnt->second = FileEntryRef::MapValue(*UFE, DirInfo);
   } else {
     // Name mismatch. We need a redirect. First grab the actual entry we want
@@ -293,25 +287,9 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
     // filesystems behave and confuses parts of clang expect to see the
     // name-as-accessed on the \a FileEntryRef.
     //
-    // Further, it isn't *just* external names, but will also give back absolute
-    // paths when a relative path was requested - the check is comparing the
-    // name from the status, which is passed an absolute path resolved from the
-    // current working directory. `clang-apply-replacements` appears to depend
-    // on this behaviour, though it's adjusting the working directory, which is
-    // definitely not supported. Once that's fixed this hack should be able to
-    // be narrowed to only when there's an externally mapped name given back.
-    //
     // A potential plan to remove this is as follows -
-    //   - Add API to determine if the name has been rewritten by the VFS.
-    //   - Fix `clang-apply-replacements` to pass down the absolute path rather
-    //     than changing the CWD. Narrow this hack down to just externally
-    //     mapped paths.
-    //   - Expose the requested filename. One possibility would be to allow
-    //     redirection-FileEntryRefs to be returned, rather than returning
-    //     the pointed-at-FileEntryRef, and customizing `getName()` to look
-    //     through the indirection.
     //   - Update callers such as `HeaderSearch::findUsableModuleForHeader()`
-    //     to explicitly use the requested filename rather than just using
+    //     to explicitly use the `getNameAsRequested()` rather than just using
     //     `getName()`.
     //   - Add a `FileManager::getExternalPath` API for explicitly getting the
     //     remapped external filename when there is one available. Adopt it in
@@ -342,9 +320,6 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
     // Cache the redirection in the previously-inserted entry, still available
     // in the tentative return value.
     NamedFileEnt->second = FileEntryRef::MapValue(Redirection);
-
-    // Fix the tentative return value.
-    NamedFileEnt = &Redirection;
   }
 
   FileEntryRef ReturnedRef(*NamedFileEnt);

clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

@@ -29,24 +29,26 @@ namespace {
 class DependencyConsumerForwarder : public DependencyFileGenerator {
 public:
   DependencyConsumerForwarder(std::unique_ptr<DependencyOutputOptions> Opts,
-                              DependencyConsumer &C,
+                              StringRef WorkingDirectory, DependencyConsumer &C,
                               bool EmitDependencyFile)
-      : DependencyFileGenerator(*Opts), Opts(std::move(Opts)), C(C),
-        EmitDependencyFile(EmitDependencyFile) {}
+      : DependencyFileGenerator(*Opts), WorkingDirectory(WorkingDirectory),
+        Opts(std::move(Opts)), C(C), EmitDependencyFile(EmitDependencyFile) {}
 
   void finishedMainFile(DiagnosticsEngine &Diags) override {
     C.handleDependencyOutputOpts(*Opts);
     llvm::SmallString<256> CanonPath;
     for (const auto &File : getDependencies()) {
       CanonPath = File;
       llvm::sys::path::remove_dots(CanonPath, /*remove_dot_dot=*/true);
+      llvm::sys::fs::make_absolute(WorkingDirectory, CanonPath);
       C.handleFileDependency(CanonPath);
     }
     if (EmitDependencyFile)
       DependencyFileGenerator::finishedMainFile(Diags);
   }
 
 private:
+  StringRef WorkingDirectory;
   std::unique_ptr<DependencyOutputOptions> Opts;
   DependencyConsumer &C;
   bool EmitDependencyFile = false;
@@ -310,8 +312,8 @@ class DependencyScanningAction : public tooling::ToolAction {
     case ScanningOutputFormat::Tree:
       ScanInstance.addDependencyCollector(
           std::make_shared<DependencyConsumerForwarder>(
-              std::move(Opts), static_cast<DependencyConsumer &>(Consumer),
-              EmitDependencyFile));
+              std::move(Opts), WorkingDirectory,
+              static_cast<DependencyConsumer &>(Consumer), EmitDependencyFile));
       break;
     case ScanningOutputFormat::IncludeTree: {
       ScanInstance.addDependencyCollector(

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

@@ -243,8 +243,7 @@ void ModuleDepCollectorPP::FileChanged(SourceLocation Loc,
   // We do not want #line markers to affect dependency generation!
   if (Optional<StringRef> Filename =
           SM.getNonBuiltinFilenameForID(SM.getFileID(SM.getExpansionLoc(Loc))))
-    MDC.FileDeps.push_back(
-        std::string(llvm::sys::path::remove_leading_dotslash(*Filename)));
+    MDC.addFileDep(llvm::sys::path::remove_leading_dotslash(*Filename));
 }
 
 void ModuleDepCollectorPP::InclusionDirective(
@@ -255,7 +254,7 @@ void ModuleDepCollectorPP::InclusionDirective(
   if (!File && !Imported) {
     // This is a non-modular include that HeaderSearch failed to find. Add it
     // here as `FileChanged` will never see it.
-    MDC.FileDeps.push_back(std::string(FileName));
+    MDC.addFileDep(FileName);
   }
   handleImport(Imported);
 }
@@ -285,8 +284,7 @@ void ModuleDepCollectorPP::EndOfMainFile() {
                                  ->getName());
 
   if (!MDC.ScanInstance.getPreprocessorOpts().ImplicitPCHInclude.empty())
-    MDC.FileDeps.push_back(
-        MDC.ScanInstance.getPreprocessorOpts().ImplicitPCHInclude);
+    MDC.addFileDep(MDC.ScanInstance.getPreprocessorOpts().ImplicitPCHInclude);
 
   for (const Module *M : DirectModularDeps) {
     // A top-level module might not be actually imported as a module when
@@ -349,10 +347,10 @@ ModuleID ModuleDepCollectorPP::handleTopLevelModule(const Module *M) {
         // handle it like normal. With explicitly built modules we don't need
         // to play VFS tricks, so replace it with the correct module map.
         if (IF.getFile()->getName().endswith("__inferred_module.map")) {
-          MD.FileDeps.insert(ModuleMap->getName());
+          MDC.addFileDep(MD, ModuleMap->getName());
           return;
         }
-        MD.FileDeps.insert(IF.getFile()->getName());
+        MDC.addFileDep(MD, IF.getFile()->getName());
       });
 
   // We usually don't need to list the module map files of our dependencies when
@@ -497,3 +495,24 @@ bool ModuleDepCollector::isPrebuiltModule(const Module *M) {
          PrebuiltModuleFileIt->second == M->getASTFile()->getName());
   return true;
 }
+
+static StringRef makeAbsolute(CompilerInstance &CI, StringRef Path,
+                              SmallVectorImpl<char> &Storage) {
+  if (llvm::sys::path::is_absolute(Path))
+    return Path;
+  Storage.assign(Path.begin(), Path.end());
+  CI.getFileManager().makeAbsolutePath(Storage);
+  return StringRef(Storage.data(), Storage.size());
+}
+
+void ModuleDepCollector::addFileDep(StringRef Path) {
+  llvm::SmallString<256> Storage;
+  Path = makeAbsolute(ScanInstance, Path, Storage);
+  FileDeps.push_back(std::string(Path));
+}
+
+void ModuleDepCollector::addFileDep(ModuleDeps &MD, StringRef Path) {
+  llvm::SmallString<256> Storage;
+  Path = makeAbsolute(ScanInstance, Path, Storage);
+  MD.FileDeps.insert(Path);
+}

clang/unittests/Basic/FileEntryTest.cpp

@@ -50,6 +50,14 @@ class FileEntryTestHelper {
                             const_cast<FileEntry &>(Base.getFileEntry()), DR)})
              .first);
   }
+  FileEntryRef addFileRedirect(StringRef Name, FileEntryRef Base) {
+    return FileEntryRef(
+        *Files
+             .insert({Name, FileEntryRef::MapValue(
+                                const_cast<FileEntryRef::MapEntry &>(
+                                    Base.getMapEntry()))})
+             .first);
+  }
 };
 
 namespace {
@@ -58,13 +66,23 @@ TEST(FileEntryTest, FileEntryRef) {
   FileEntryRef R1 = Refs.addFile("1");
   FileEntryRef R2 = Refs.addFile("2");
   FileEntryRef R1Also = Refs.addFileAlias("1-also", R1);
+  FileEntryRef R1Redirect = Refs.addFileRedirect("1-redirect", R1);
+  FileEntryRef R1Redirect2 = Refs.addFileRedirect("1-redirect2", R1Redirect);
 
   EXPECT_EQ("1", R1.getName());
   EXPECT_EQ("2", R2.getName());
   EXPECT_EQ("1-also", R1Also.getName());
+  EXPECT_EQ("1", R1Redirect.getName());
+  EXPECT_EQ("1", R1Redirect2.getName());
+
+  EXPECT_EQ("1", R1.getNameAsRequested());
+  EXPECT_EQ("1-redirect", R1Redirect.getNameAsRequested());
+  EXPECT_EQ("1-redirect2", R1Redirect2.getNameAsRequested());
 
   EXPECT_NE(&R1.getFileEntry(), &R2.getFileEntry());
   EXPECT_EQ(&R1.getFileEntry(), &R1Also.getFileEntry());
+  EXPECT_EQ(&R1.getFileEntry(), &R1Redirect.getFileEntry());
+  EXPECT_EQ(&R1Redirect.getFileEntry(), &R1Redirect2.getFileEntry());
 
   const FileEntry *CE1 = R1;
   EXPECT_EQ(CE1, &R1.getFileEntry());
@@ -93,13 +111,17 @@ TEST(FileEntryTest, equals) {
   FileEntryRef R1 = Refs.addFile("1");
   FileEntryRef R2 = Refs.addFile("2");
   FileEntryRef R1Also = Refs.addFileAlias("1-also", R1);
+  FileEntryRef R1Redirect = Refs.addFileRedirect("1-redirect", R1);
+  FileEntryRef R1Redirect2 = Refs.addFileRedirect("1-redirect2", R1Redirect);
 
   EXPECT_EQ(R1, &R1.getFileEntry());
   EXPECT_EQ(&R1.getFileEntry(), R1);
   EXPECT_EQ(R1, R1Also);
   EXPECT_NE(R1, &R2.getFileEntry());
   EXPECT_NE(&R2.getFileEntry(), R1);
   EXPECT_NE(R1, R2);
+  EXPECT_EQ(R1, R1Redirect);
+  EXPECT_EQ(R1, R1Redirect2);
 
   OptionalFileEntryRefDegradesToFileEntryPtr M1 = R1;
 
@@ -114,11 +136,16 @@ TEST(FileEntryTest, isSameRef) {
   FileEntryRef R1 = Refs.addFile("1");
   FileEntryRef R2 = Refs.addFile("2");
   FileEntryRef R1Also = Refs.addFileAlias("1-also", R1);
+  FileEntryRef R1Redirect = Refs.addFileRedirect("1-redirect", R1);
+  FileEntryRef R1Redirect2 = Refs.addFileRedirect("1-redirect2", R1Redirect);
 
   EXPECT_TRUE(R1.isSameRef(FileEntryRef(R1)));
   EXPECT_TRUE(R1.isSameRef(FileEntryRef(R1.getMapEntry())));
   EXPECT_FALSE(R1.isSameRef(R2));
   EXPECT_FALSE(R1.isSameRef(R1Also));
+  EXPECT_FALSE(R1.isSameRef(R1Redirect));
+  EXPECT_FALSE(R1.isSameRef(R1Redirect2));
+  EXPECT_FALSE(R1Redirect.isSameRef(R1Redirect2));
 }
 
 TEST(FileEntryTest, DenseMapInfo) {