diff --git a/clang/include/clang/Basic/FileManager.h b/clang/include/clang/Basic/FileManager.h --- a/clang/include/clang/Basic/FileManager.h +++ b/clang/include/clang/Basic/FileManager.h @@ -77,12 +77,10 @@ /// accessed by the FileManager's client. class FileEntryRef { public: - FileEntryRef() = delete; - FileEntryRef(StringRef Name, const FileEntry &Entry) - : Name(Name), Entry(&Entry) {} - - const StringRef getName() const { return Name; } - const FileEntry &getFileEntry() const { return *Entry; } + const StringRef getName() const { return Entry->first(); } + const FileEntry &getFileEntry() const { + return *Entry->second->V.get(); + } inline bool isValid() const; inline off_t getSize() const; @@ -91,15 +89,43 @@ inline time_t getModificationTime() const; friend bool operator==(const FileEntryRef &LHS, const FileEntryRef &RHS) { - return LHS.Entry == RHS.Entry && LHS.Name == RHS.Name; + return LHS.Entry == RHS.Entry; } friend bool operator!=(const FileEntryRef &LHS, const FileEntryRef &RHS) { return !(LHS == RHS); } + struct MapValue; + + /// Type used in the StringMap. + using MapEntry = llvm::StringMapEntry>; + + /// Type stored in the StringMap. + struct MapValue { + /// The pointer at another MapEntry is used when the FileManager should + /// silently forward from one name to another, which occurs in Redirecting + /// VFSs that use external names. In that case, the \c FileEntryRef + /// returned by the \c FileManager will have the external name, and not the + /// name that was used to lookup the file. + llvm::PointerUnion V; + + MapValue() = delete; + MapValue(FileEntry &FE) : V(&FE) {} + MapValue(MapEntry &ME) : V(&ME) {} + }; + private: - StringRef Name; - const FileEntry *Entry; + friend class FileManager; + + FileEntryRef() = delete; + explicit FileEntryRef(const MapEntry &Entry) + : Entry(&Entry) { + assert(Entry.second && "Expected payload"); + assert(Entry.second->V && "Expected non-null"); + assert(Entry.second->V.is() && "Expected FileEntry"); + } + + const MapEntry *Entry; }; /// Cached information about one file (either on disk @@ -110,7 +136,6 @@ class FileEntry { friend class FileManager; - StringRef Name; // Name of the file. std::string RealPathName; // Real path to the file; could be empty. off_t Size; // File size in bytes. time_t ModTime; // Modification time of file. @@ -123,6 +148,14 @@ /// The open file, if it is owned by the \p FileEntry. mutable std::unique_ptr File; + // First access name for this FileEntry. + // + // This is Optional only to allow delayed construction (FileEntryRef has no + // default constructor). It should always have a value in practice. + // + // TODO: remote this once everyone that needs a name uses FileEntryRef. + Optional LastRef; + public: FileEntry() : UniqueID(0, 0), IsNamedPipe(false), IsValid(false) @@ -131,7 +164,9 @@ FileEntry(const FileEntry &) = delete; FileEntry &operator=(const FileEntry &) = delete; - StringRef getName() const { return Name; } + StringRef getName() const { return LastRef->getName(); } + FileEntryRef getLastRef() const { return *LastRef; } + StringRef tryGetRealPathName() const { return RealPathName; } bool isValid() const { return IsValid; } off_t getSize() const { return Size; } @@ -212,26 +247,21 @@ llvm::StringMap, llvm::BumpPtrAllocator> SeenDirEntries; - /// A reference to the file entry that is associated with a particular - /// filename, or a reference to another filename that should be looked up - /// instead of the accessed filename. - /// - /// The reference to another filename is specifically useful for Redirecting - /// VFSs that use external names. In that case, the \c FileEntryRef returned - /// by the \c FileManager will have the external name, and not the name that - /// was used to lookup the file. - using SeenFileEntryOrRedirect = - llvm::PointerUnion; - /// A cache that maps paths to file entries (either real or /// virtual) we have looked up, or an error that occurred when we looked up /// the file. /// /// \see SeenDirEntries - llvm::StringMap, - llvm::BumpPtrAllocator> + llvm::StringMap, llvm::BumpPtrAllocator> SeenFileEntries; + /// A mirror of SeenFileEntries to give fake answers for getBypassFile(). + /// + /// Don't bother hooking up a BumpPtrAllocator. This should be rarely used, + /// and only on error paths. + std::unique_ptr>> + SeenBypassFileEntries; + /// The canonical names of files and directories . llvm::DenseMap CanonicalNames; diff --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp --- a/clang/lib/Basic/FileManager.cpp +++ b/clang/lib/Basic/FileManager.cpp @@ -212,11 +212,10 @@ SeenFileInsertResult.first->second.getError()); // Construct and return and FileEntryRef, unless it's a redirect to another // filename. - SeenFileEntryOrRedirect Value = *SeenFileInsertResult.first->second; - FileEntry *FE; - if (LLVM_LIKELY(FE = Value.dyn_cast())) - return FileEntryRef(SeenFileInsertResult.first->first(), *FE); - return getFileRef(*Value.get(), openFile, CacheFailure); + FileEntryRef::MapValue Value = *SeenFileInsertResult.first->second; + if (LLVM_LIKELY(Value.V.is())) + return FileEntryRef(*SeenFileInsertResult.first); + return FileEntryRef(*Value.V.get()); } // We've not seen this before. Fill it in. @@ -268,26 +267,29 @@ // This occurs when one dir is symlinked to another, for example. FileEntry &UFE = UniqueRealFiles[Status.getUniqueID()]; - NamedFileEnt->second = &UFE; - - // If the name returned by getStatValue is different than Filename, re-intern - // the name. - if (Status.getName() != Filename) { - auto &NewNamedFileEnt = - *SeenFileEntries.insert({Status.getName(), &UFE}).first; - assert((*NewNamedFileEnt.second).get() == &UFE && + if (Status.getName() == Filename) { + // The name matches. Set the FileEntry. + NamedFileEnt->second = FileEntryRef::MapValue(UFE); + } else { + // Name mismatch. We need a redirect. First grab the actual entry we want + // to return. + auto &Redirection = + *SeenFileEntries.insert({Status.getName(), FileEntryRef::MapValue(UFE)}) + .first; + assert(Redirection.second->V.is() && + "filename redirected to a non-canonical filename?"); + assert(Redirection.second->V.get() == &UFE && "filename from getStatValue() refers to wrong file"); - InterndFileName = NewNamedFileEnt.first().data(); - // In addition to re-interning the name, construct a redirecting seen file - // entry, that will point to the name the filesystem actually wants to use. - StringRef *Redirect = new (CanonicalNameStorage) StringRef(InterndFileName); - auto SeenFileInsertResultIt = SeenFileEntries.find(Filename); - assert(SeenFileInsertResultIt != SeenFileEntries.end() && - "unexpected SeenFileEntries cache miss"); - SeenFileInsertResultIt->second = Redirect; - NamedFileEnt = &*SeenFileInsertResultIt; + + // 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); if (UFE.isValid()) { // Already have an entry with this inode, return it. // FIXME: this hack ensures that if we look up a file by a virtual path in @@ -299,20 +301,20 @@ if (DirInfo != UFE.Dir && Status.IsVFSMapped) UFE.Dir = DirInfo; - // Always update the name to use the last name by which a file was accessed. - // FIXME: Neither this nor always using the first name is correct; we want - // to switch towards a design where we return a FileName object that + // Always update LastRef to the last name by which a file was accessed. + // FIXME: Neither this nor always using the first reference is correct; we + // want to switch towards a design where we return a FileName object that // encapsulates both the name by which the file was accessed and the // corresponding FileEntry. - // FIXME: The Name should be removed from FileEntry once all clients - // adopt FileEntryRef. - UFE.Name = InterndFileName; + // FIXME: LastRef should be removed from FileEntry once all clients adopt + // FileEntryRef. + UFE.LastRef = ReturnedRef; - return FileEntryRef(InterndFileName, UFE); + return ReturnedRef; } // Otherwise, we don't have this file yet, add it. - UFE.Name = InterndFileName; + UFE.LastRef = ReturnedRef; UFE.Size = Status.getSize(); UFE.ModTime = llvm::sys::toTimeT(Status.getLastModificationTime()); UFE.Dir = DirInfo; @@ -329,7 +331,7 @@ // We should still fill the path even if we aren't opening the file. fillRealPathName(&UFE, InterndFileName); } - return FileEntryRef(InterndFileName, UFE); + return ReturnedRef; } const FileEntry * @@ -341,12 +343,12 @@ auto &NamedFileEnt = *SeenFileEntries.insert( {Filename, std::errc::no_such_file_or_directory}).first; if (NamedFileEnt.second) { - SeenFileEntryOrRedirect Value = *NamedFileEnt.second; + FileEntryRef::MapValue Value = *NamedFileEnt.second; FileEntry *FE; - if (LLVM_LIKELY(FE = Value.dyn_cast())) + if (LLVM_LIKELY(FE = Value.V.dyn_cast())) return FE; - return getVirtualFile(*Value.get(), Size, - ModificationTime); + return &FileEntryRef(*Value.V.get()) + .getFileEntry(); } // We've not seen this before, or the file is cached as non-existent. @@ -372,7 +374,7 @@ Status.getUser(), Status.getGroup(), Size, Status.getType(), Status.getPermissions()); - NamedFileEnt.second = UFE; + NamedFileEnt.second = FileEntryRef::MapValue(*UFE); // If we had already opened this file, close it now so we don't // leak the descriptor. We're not going to use the file @@ -381,6 +383,9 @@ UFE->closeFile(); // If we already have an entry with this inode, return it. + // + // FIXME: Surely this should add a reference by the new name, and return + // it instead... if (UFE->isValid()) return UFE; @@ -390,10 +395,10 @@ } else { VirtualFileEntries.push_back(std::make_unique()); UFE = VirtualFileEntries.back().get(); - NamedFileEnt.second = UFE; + NamedFileEnt.second = FileEntryRef::MapValue(*UFE); } - UFE->Name = InterndFileName; + UFE->LastRef = FileEntryRef(NamedFileEnt); UFE->Size = Size; UFE->ModTime = ModificationTime; UFE->Dir = *DirInfo; @@ -409,17 +414,30 @@ if (getStatValue(VF.getName(), Status, /*isFile=*/true, /*F=*/nullptr)) return None; - // Fill it in from the stat. + if (!SeenBypassFileEntries) + SeenBypassFileEntries = std::make_unique< + llvm::StringMap>>(); + + // If we've already bypassed just use the existing one. + auto Insertion = SeenBypassFileEntries->insert( + {VF.getName(), std::errc::no_such_file_or_directory}); + if (!Insertion.second) + return FileEntryRef(*Insertion.first); + + // Fill in the new entry from the stat. BypassFileEntries.push_back(std::make_unique()); const FileEntry &VFE = VF.getFileEntry(); FileEntry &BFE = *BypassFileEntries.back(); - BFE.Name = VFE.getName(); + Insertion.first->second = FileEntryRef::MapValue(BFE); + BFE.LastRef = FileEntryRef(*Insertion.first); BFE.Size = Status.getSize(); BFE.Dir = VFE.Dir; BFE.ModTime = llvm::sys::toTimeT(Status.getLastModificationTime()); BFE.UID = NextFileUID++; BFE.IsValid = true; - return FileEntryRef(VF.getName(), BFE); + + // Save the entry in the bypass table and return. + return FileEntryRef(*Insertion.first); } bool FileManager::FixupRelativePath(SmallVectorImpl &path) const { @@ -534,13 +552,13 @@ UIDToFiles.resize(NextFileUID); // Map file entries - for (llvm::StringMap, + for (llvm::StringMap, llvm::BumpPtrAllocator>::const_iterator FE = SeenFileEntries.begin(), FEEnd = SeenFileEntries.end(); FE != FEEnd; ++FE) - if (llvm::ErrorOr Entry = FE->getValue()) { - if (const auto *FE = (*Entry).dyn_cast()) + if (llvm::ErrorOr Entry = FE->getValue()) { + if (const auto *FE = Entry->V.dyn_cast()) UIDToFiles[FE->getUID()] = FE; } diff --git a/clang/lib/Basic/SourceManager.cpp b/clang/lib/Basic/SourceManager.cpp --- a/clang/lib/Basic/SourceManager.cpp +++ b/clang/lib/Basic/SourceManager.cpp @@ -702,7 +702,7 @@ SourceManager::bypassFileContentsOverride(const FileEntry &File) { assert(isFileOverridden(&File)); llvm::Optional BypassFile = - FileMgr.getBypassFile(FileEntryRef(File.getName(), File)); + FileMgr.getBypassFile(File.getLastRef()); // If the file can't be found in the FS, give up. if (!BypassFile) diff --git a/clang/unittests/Basic/FileManagerTest.cpp b/clang/unittests/Basic/FileManagerTest.cpp --- a/clang/unittests/Basic/FileManagerTest.cpp +++ b/clang/unittests/Basic/FileManagerTest.cpp @@ -285,9 +285,18 @@ StatCache->InjectFile("dir/f1-alias.cpp", 41, "dir/f1.cpp"); StatCache->InjectFile("dir/f2.cpp", 42); StatCache->InjectFile("dir/f2-alias.cpp", 42, "dir/f2.cpp"); + + // This unintuitive rename-the-file-on-stat behaviour supports how the + // RedirectingFileSystem VFS layer responds to stats. However, even if you + // have two layers, you should only get a single filename back. As such the + // following stat cache behaviour is not supported (the correct stat entry + // for a double-redirection would be "dir/f1.cpp") and the getFileRef below + // should assert. + StatCache->InjectFile("dir/f1-alias-alias.cpp", 41, "dir/f1-alias.cpp"); + manager.setStatCache(std::move(StatCache)); - // With F2, test accessing the non-redirected name first. + // With F1, test accessing the non-redirected name first. auto F1 = manager.getFileRef("dir/f1.cpp"); auto F1Alias = manager.getFileRef("dir/f1-alias.cpp"); auto F1Alias2 = manager.getFileRef("dir/f1-alias.cpp"); @@ -301,6 +310,11 @@ EXPECT_EQ(&F1->getFileEntry(), &F1Alias->getFileEntry()); EXPECT_EQ(&F1->getFileEntry(), &F1Alias2->getFileEntry()); +#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST + EXPECT_DEATH((void)manager.getFileRef("dir/f1-alias-alias.cpp"), + "filename redirected to a non-canonical filename?"); +#endif + // With F2, test accessing the redirected name first. auto F2Alias = manager.getFileRef("dir/f2-alias.cpp"); auto F2 = manager.getFileRef("dir/f2.cpp"); @@ -487,29 +501,34 @@ // Set up a virtual file with a different size than FakeStatCache uses. const FileEntry *File = Manager.getVirtualFile("/tmp/test", /*Size=*/10, 0); ASSERT_TRUE(File); - FileEntryRef Ref("/tmp/test", *File); - EXPECT_TRUE(Ref.isValid()); - EXPECT_EQ(Ref.getSize(), 10); + const FileEntry &FE = *File; + EXPECT_TRUE(FE.isValid()); + EXPECT_EQ(FE.getSize(), 10); // Calling a second time should not affect the UID or size. - unsigned VirtualUID = Ref.getUID(); - EXPECT_EQ(*expectedToOptional(Manager.getFileRef("/tmp/test")), Ref); - EXPECT_EQ(Ref.getUID(), VirtualUID); - EXPECT_EQ(Ref.getSize(), 10); + unsigned VirtualUID = FE.getUID(); + EXPECT_EQ( + &FE, + &expectedToOptional(Manager.getFileRef("/tmp/test"))->getFileEntry()); + EXPECT_EQ(FE.getUID(), VirtualUID); + EXPECT_EQ(FE.getSize(), 10); // Bypass the file. - llvm::Optional BypassRef = Manager.getBypassFile(Ref); + llvm::Optional BypassRef = + Manager.getBypassFile(File->getLastRef()); ASSERT_TRUE(BypassRef); EXPECT_TRUE(BypassRef->isValid()); - EXPECT_EQ(BypassRef->getName(), Ref.getName()); + EXPECT_EQ("/tmp/test", BypassRef->getName()); // Check that it's different in the right ways. EXPECT_NE(&BypassRef->getFileEntry(), File); EXPECT_NE(BypassRef->getUID(), VirtualUID); - EXPECT_NE(BypassRef->getSize(), Ref.getSize()); + EXPECT_NE(BypassRef->getSize(), FE.getSize()); // The virtual file should still be returned when searching. - EXPECT_EQ(*expectedToOptional(Manager.getFileRef("/tmp/test")), Ref); + EXPECT_EQ( + &FE, + &expectedToOptional(Manager.getFileRef("/tmp/test"))->getFileEntry()); } } // anonymous namespace