fyi, I've reverted this commit with r336831 due to some other issues. On Wed, Jul 11, 2018 at 8:23 PM Galina Kistanova via cfe-commits < cfe-commits@lists.llvm.org> wrote:
> Hello Simon, > > This commit added broken tests to one of our builders: > > http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/18386/steps/test/logs/stdio > . . . > Failing Tests (4): > Clang :: PCH/case-insensitive-include.c > Clang-Unit :: > Basic/./BasicTests.exe/InMemoryFileSystemTest.DirectoryIteration > Clang-Unit :: Basic/./BasicTests.exe/InMemoryFileSystemTest.StatusName > LLVM :: MC/ELF/debug-prefix-map.s > > Please have a look? > The builder was already red and did not send notifications. > > Thanks > > > Galina > > On Wed, Jul 11, 2018 at 7:08 AM, Simon Marchi via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> Author: simark >> Date: Wed Jul 11 07:08:17 2018 >> New Revision: 336807 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=336807&view=rev >> Log: >> [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the >> requested name >> >> Summary: >> InMemoryFileSystem::status behaves differently than >> RealFileSystem::status. The Name contained in the Status returned by >> RealFileSystem::status will be the path as requested by the caller, >> whereas InMemoryFileSystem::status returns the normalized path. >> >> For example, when requested the status for "../src/first.h", >> RealFileSystem returns a Status with "../src/first.h" as the Name. >> InMemoryFileSystem returns "/absolute/path/to/src/first.h". >> >> The reason for this change is that I want to make a unit test in the >> clangd testsuite (where we use an InMemoryFileSystem) to reproduce a >> bug I get with the clangd program (where a RealFileSystem is used). >> This difference in behavior "hides" the bug in the unit test version. >> >> In general, I guess it's good if InMemoryFileSystem works as much as >> possible like RealFileSystem. >> >> Doing so made the FileEntry::RealPathName value (assigned in >> FileManager::getFile) wrong when using the InMemoryFileSystem. That's >> because it assumes that vfs::File::getName will always return the real >> path. I changed to to use FileSystem::getRealPath instead. >> >> Subscribers: ilya-biryukov, ioeric, cfe-commits >> >> Differential Revision: https://reviews.llvm.org/D48903 >> >> Modified: >> cfe/trunk/lib/Basic/FileManager.cpp >> cfe/trunk/lib/Basic/VirtualFileSystem.cpp >> cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp >> cfe/trunk/unittests/Driver/ToolChainTest.cpp >> >> Modified: cfe/trunk/lib/Basic/FileManager.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=336807&r1=336806&r2=336807&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/Basic/FileManager.cpp (original) >> +++ cfe/trunk/lib/Basic/FileManager.cpp Wed Jul 11 07:08:17 2018 >> @@ -315,9 +315,11 @@ const FileEntry *FileManager::getFile(St >> UFE.InPCH = Data.InPCH; >> UFE.File = std::move(F); >> UFE.IsValid = true; >> - if (UFE.File) >> - if (auto RealPathName = UFE.File->getName()) >> - UFE.RealPathName = *RealPathName; >> + >> + SmallString<128> RealPathName; >> + if (!FS->getRealPath(InterndFileName, RealPathName)) >> + UFE.RealPathName = RealPathName.str(); >> + >> return &UFE; >> } >> >> >> Modified: cfe/trunk/lib/Basic/VirtualFileSystem.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/VirtualFileSystem.cpp?rev=336807&r1=336806&r2=336807&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/Basic/VirtualFileSystem.cpp (original) >> +++ cfe/trunk/lib/Basic/VirtualFileSystem.cpp Wed Jul 11 07:08:17 2018 >> @@ -471,15 +471,33 @@ enum InMemoryNodeKind { IME_File, IME_Di >> /// The in memory file system is a tree of Nodes. Every node can either >> be a >> /// file or a directory. >> class InMemoryNode { >> - Status Stat; >> InMemoryNodeKind Kind; >> + Status Stat; >> + >> +protected: >> + /// Return Stat. This should only be used for internal/debugging >> use. When >> + /// clients wants the Status of this node, they should use >> + /// \p getStatus(StringRef). >> + const Status& getStatus() const { >> + return Stat; >> + } >> >> public: >> InMemoryNode(Status Stat, InMemoryNodeKind Kind) >> - : Stat(std::move(Stat)), Kind(Kind) {} >> + : Kind(Kind), Stat(std::move(Stat)) {} >> virtual ~InMemoryNode() = default; >> >> - const Status &getStatus() const { return Stat; } >> + /// Return the \p Status for this node. \p RequestedName should be the >> name >> + /// through which the caller referred to this node. It will override >> + /// \p Status::Name in the return value, to mimic the behavior of \p >> RealFile. >> + Status getStatus(StringRef RequestedName) const { >> + return Status::copyWithNewName(Stat, RequestedName); >> + } >> + >> + /// Get the filename of this node (the name without the directory >> part). >> + StringRef getFileName() const { >> + return llvm::sys::path::filename(Stat.getName()); >> + } >> InMemoryNodeKind getKind() const { return Kind; } >> virtual std::string toString(unsigned Indent) const = 0; >> }; >> @@ -504,14 +522,22 @@ public: >> } >> }; >> >> -/// Adapt a InMemoryFile for VFS' File interface. >> +/// Adapt a InMemoryFile for VFS' File interface. The goal is to make >> +/// \p InMemoryFileAdaptor mimic as much as possible the behavior of >> +/// \p RealFile. >> class InMemoryFileAdaptor : public File { >> InMemoryFile &Node; >> >> + /// The name to use when returning a Status for this file. >> + std::string RequestedName; >> + >> public: >> - explicit InMemoryFileAdaptor(InMemoryFile &Node) : Node(Node) {} >> + explicit InMemoryFileAdaptor(InMemoryFile &Node, std::string >> RequestedName) >> + : Node(Node), RequestedName(std::move(RequestedName)) {} >> >> - llvm::ErrorOr<Status> status() override { return Node.getStatus(); } >> + llvm::ErrorOr<Status> status() override { >> + return Node.getStatus(RequestedName); >> + } >> >> llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> >> getBuffer(const Twine &Name, int64_t FileSize, bool >> RequiresNullTerminator, >> @@ -711,7 +737,7 @@ lookupInMemoryNode(const InMemoryFileSys >> llvm::ErrorOr<Status> InMemoryFileSystem::status(const Twine &Path) { >> auto Node = lookupInMemoryNode(*this, Root.get(), Path); >> if (Node) >> - return (*Node)->getStatus(); >> + return (*Node)->getStatus(Path.str()); >> return Node.getError(); >> } >> >> @@ -724,7 +750,8 @@ InMemoryFileSystem::openFileForRead(cons >> // When we have a file provide a heap-allocated wrapper for the memory >> buffer >> // to match the ownership semantics for File. >> if (auto *F = dyn_cast<detail::InMemoryFile>(*Node)) >> - return std::unique_ptr<File>(new detail::InMemoryFileAdaptor(*F)); >> + return std::unique_ptr<File>( >> + new detail::InMemoryFileAdaptor(*F, Path.str())); >> >> // FIXME: errc::not_a_file? >> return make_error_code(llvm::errc::invalid_argument); >> @@ -736,21 +763,33 @@ namespace { >> class InMemoryDirIterator : public clang::vfs::detail::DirIterImpl { >> detail::InMemoryDirectory::const_iterator I; >> detail::InMemoryDirectory::const_iterator E; >> + std::string RequestedDirName; >> + >> + void setCurrentEntry() { >> + if (I != E) { >> + SmallString<256> Path(RequestedDirName); >> + llvm::sys::path::append(Path, I->second->getFileName()); >> + CurrentEntry = I->second->getStatus(Path.str()); >> + } else { >> + // When we're at the end, make CurrentEntry invalid and >> DirIterImpl will >> + // do the rest. >> + CurrentEntry = Status(); >> + } >> + } >> >> public: >> InMemoryDirIterator() = default; >> >> - explicit InMemoryDirIterator(detail::InMemoryDirectory &Dir) >> - : I(Dir.begin()), E(Dir.end()) { >> - if (I != E) >> - CurrentEntry = I->second->getStatus(); >> + explicit InMemoryDirIterator(detail::InMemoryDirectory &Dir, >> + std::string RequestedDirName) >> + : I(Dir.begin()), E(Dir.end()), >> + RequestedDirName(std::move(RequestedDirName)) { >> + setCurrentEntry(); >> } >> >> std::error_code increment() override { >> ++I; >> - // When we're at the end, make CurrentEntry invalid and DirIterImpl >> will do >> - // the rest. >> - CurrentEntry = I != E ? I->second->getStatus() : Status(); >> + setCurrentEntry(); >> return {}; >> } >> }; >> @@ -766,7 +805,8 @@ directory_iterator InMemoryFileSystem::d >> } >> >> if (auto *DirNode = dyn_cast<detail::InMemoryDirectory>(*Node)) >> - return >> directory_iterator(std::make_shared<InMemoryDirIterator>(*DirNode)); >> + return directory_iterator( >> + std::make_shared<InMemoryDirIterator>(*DirNode, Dir.str())); >> >> EC = make_error_code(llvm::errc::not_a_directory); >> return directory_iterator(std::make_shared<InMemoryDirIterator>()); >> >> Modified: cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp?rev=336807&r1=336806&r2=336807&view=diff >> >> ============================================================================== >> --- cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp (original) >> +++ cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp Wed Jul 11 >> 07:08:17 2018 >> @@ -794,7 +794,7 @@ TEST_F(InMemoryFileSystemTest, WorkingDi >> >> auto Stat = FS.status("/b/c"); >> ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << >> FS.toString(); >> - ASSERT_EQ("c", Stat->getName()); >> + ASSERT_EQ("/b/c", Stat->getName()); >> ASSERT_EQ("/b", *FS.getCurrentWorkingDirectory()); >> >> Stat = FS.status("c"); >> @@ -919,6 +919,37 @@ TEST_F(InMemoryFileSystemTest, AddDirect >> ASSERT_TRUE(Stat->isRegularFile()); >> } >> >> +// Test that the name returned by status() is in the same form as the >> path that >> +// was requested (to match the behavior of RealFileSystem). >> +TEST_F(InMemoryFileSystemTest, StatusName) { >> + NormalizedFS.addFile("/a/b/c", 0, MemoryBuffer::getMemBuffer("abc"), >> + /*User=*/None, >> + /*Group=*/None, sys::fs::file_type::regular_file); >> + NormalizedFS.setCurrentWorkingDirectory("/a/b"); >> + >> + // Access using InMemoryFileSystem::status. >> + auto Stat = NormalizedFS.status("../b/c"); >> + ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" >> + << NormalizedFS.toString(); >> + ASSERT_TRUE(Stat->isRegularFile()); >> + ASSERT_EQ("../b/c", Stat->getName()); >> + >> + // Access using InMemoryFileAdaptor::status. >> + auto File = NormalizedFS.openFileForRead("../b/c"); >> + ASSERT_FALSE(File.getError()) << File.getError() << "\n" >> + << NormalizedFS.toString(); >> + Stat = (*File)->status(); >> + ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" >> + << NormalizedFS.toString(); >> + ASSERT_TRUE(Stat->isRegularFile()); >> + ASSERT_EQ("../b/c", Stat->getName()); >> + >> + // Access using a directory iterator. >> + std::error_code EC; >> + clang::vfs::directory_iterator It = NormalizedFS.dir_begin("../b", EC); >> + ASSERT_EQ("../b/c", It->getName()); >> +} >> + >> // NOTE: in the tests below, we use '//root/' as our root directory, >> since it is >> // a legal *absolute* path on Windows as well as *nix. >> class VFSFromYAMLTest : public ::testing::Test { >> >> Modified: cfe/trunk/unittests/Driver/ToolChainTest.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Driver/ToolChainTest.cpp?rev=336807&r1=336806&r2=336807&view=diff >> >> ============================================================================== >> --- cfe/trunk/unittests/Driver/ToolChainTest.cpp (original) >> +++ cfe/trunk/unittests/Driver/ToolChainTest.cpp Wed Jul 11 07:08:17 2018 >> @@ -113,7 +113,7 @@ TEST(ToolChainTest, VFSGCCInstallationRe >> std::replace(S.begin(), S.end(), '\\', '/'); >> #endif >> EXPECT_EQ("Found candidate GCC installation: " >> - "/home/test/lib/gcc/arm-linux-gnueabi/4.6.1\n" >> + "/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n" >> "Selected GCC installation: " >> "/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n" >> "Candidate multilib: .;@m32\n" >> >> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
