r336807 - [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
Eric Liu via cfe-commits
cfe-commits at lists.llvm.org
Wed Jul 11 11:51:39 PDT 2018
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 at 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 at 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 at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180711/a01c7f53/attachment-0001.html>
More information about the cfe-commits
mailing list