r336807 - [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

Galina Kistanova via cfe-commits cfe-commits at lists.llvm.org
Wed Jul 11 11:23:08 PDT 2018


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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180711/430165bf/attachment-0001.html>


More information about the cfe-commits mailing list