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