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

[simark]

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.

[simark]

Update commit message

[hokein]

I'm not familiar with this part of code, but the change looks fine to me. I think @bkramer is the right person to review it.

Please make sure the style align with LLVM code style.

lib/Basic/VirtualFileSystem.cpp

513–515

I think we should have a comment saying the InMemoryFile has the same behavior as the real file system.

[ilya-biryukov]

Mimicing RealFS seems like the right thing to do here, so I would vouch for checking this change in.
I'm a little worried that there are tests/clients relying on the old behavior, have you run all the tests?

Also, could you please run git-clang-format to fix the formatting issues?

lib/Basic/VirtualFileSystem.cpp

524

NIT: The formatting is broken here.

527

Maybe add a RequestedName parameter to the InMemoryNode instead to make sure it's not misused?
It looks like all the clients calling it have to change the name and some are not doing it now, e.g. the directory iterator will use statuses with full paths.

528

Maybe add a comment that this matches the real file system behavior?

727

NIT: The indent is incorrect here.

729

NIT: we don't need str() here

[ilya-biryukov]

Sorry, have missed the @hokein 's comments, so one of mine seems like a duplicate.

[simark]

In D48903#1151847, @hokein wrote:

I'm not familiar with this part of code, but the change looks fine to me. I think @bkramer is the right person to review it.

Please make sure the style align with LLVM code style.

Woops indeed I forgot to run git clang-format.

[simark]

In D48903#1151866, @ilya-biryukov wrote:

Mimicing RealFS seems like the right thing to do here, so I would vouch for checking this change in.
I'm a little worried that there are tests/clients relying on the old behavior, have you run all the tests?

I didn't have time yesterday, I'm doing this now. Is ninja/make clang-test sufficient?

[simark]

• Fixed formatting (ran git-clang-format)
• Fixed expectation in TEST_F(InMemoryFileSystemTest, WorkingDirectory)
• Added test TEST_F(InMemoryFileSystemTest, StatusName)

[simark]

I ran ninja clang-test:

Testing Time: 1720.20s
  Expected Passes    : 12472
  Expected Failures  : 18
  Unsupported Tests  : 263

[ilya-biryukov]

I usually run ninja check-clang check-clang-tools for clang changes. Have never used clang-test, not sure what it does.
I ran it with this change, found a few failures from clang-move:

Failing Tests (5):
    Extra Tools Unit Tests :: clang-move/./ClangMoveTests/ClangMove.AddDependentNewHeader
    Extra Tools Unit Tests :: clang-move/./ClangMoveTests/ClangMove.AddDependentOldHeader
    Extra Tools Unit Tests :: clang-move/./ClangMoveTests/ClangMove.DontMoveAll
    Extra Tools Unit Tests :: clang-move/./ClangMoveTests/ClangMove.MoveHeaderAndCC
    Extra Tools Unit Tests :: clang-move/./ClangMoveTests/ClangMove.MoveHeaderOnly

[simark]

In D48903#1152485, @ilya-biryukov wrote:

I usually run ninja check-clang check-clang-tools for clang changes. Have never used clang-test, not sure what it does.

I think clang-test is an alias for check-clang.

I ran it with this change, found a few failures from clang-move:

Failing Tests (5):
    Extra Tools Unit Tests :: clang-move/./ClangMoveTests/ClangMove.AddDependentNewHeader
    Extra Tools Unit Tests :: clang-move/./ClangMoveTests/ClangMove.AddDependentOldHeader
    Extra Tools Unit Tests :: clang-move/./ClangMoveTests/ClangMove.DontMoveAll
    Extra Tools Unit Tests :: clang-move/./ClangMoveTests/ClangMove.MoveHeaderAndCC
    Extra Tools Unit Tests :: clang-move/./ClangMoveTests/ClangMove.MoveHeaderOnly

Doh, I'll run check-clang-tools too.

[simark]

I opened D48951 to fix the failures in clang-move.

[hokein]

Seems to me you have a few comments unaddressed (and make sure you marked them done when updating the patch).

[simark]

In D48903#1153142, @hokein wrote:

Seems to me you have a few comments unaddressed (and make sure you marked them done when updating the patch).

Ah damn I missed them, I'm not too used to how Phabricator displays things. I'll do that.

[simark]

lib/Basic/VirtualFileSystem.cpp

524

Hmm this is what git-clang-format does (unless this comment refers to a previous version where I had not run clang-format).

527

Ok, I tried to do something like that.

528

I added a comment to InMemoryNode::getSatus, since that's where the copyWithNewName is done now.

729

Otherwise I'm getting this:

/home/emaisin/src/llvm/tools/clang/lib/Basic/VirtualFileSystem.cpp:1673:9: error: no matching function for call to 'copyWithNewName'
    S = Status::copyWithNewName(S, Path);
        ^~~~~~~~~~~~~~~~~~~~~~~
/home/emaisin/src/llvm/tools/clang/lib/Basic/VirtualFileSystem.cpp:76:16: note: candidate function not viable: no known conversion from 'const llvm::Twine' to 'llvm::StringRef' for 2nd argument
Status Status::copyWithNewName(const Status &In, StringRef NewName) {
               ^
/home/emaisin/src/llvm/tools/clang/lib/Basic/VirtualFileSystem.cpp:82:16: note: candidate function not viable: no known conversion from 'clang::vfs::Status' to 'const llvm::sys::fs::file_status' for 1st argument
Status Status::copyWithNewName(const file_status &In, StringRef NewName) {
               ^

[simark]

• Add RequestedName to InMemoryNode::getStatus.
• Also fix the directory_iterator code path.

[hokein]

The code looks good. I'll let Ben take a final look.

lib/Basic/VirtualFileSystem.cpp

485

Can we use llvm::StringRef here?

unittests/Basic/VirtualFileSystemTest.cpp

950

This is not used.

[simark]

• Use StringRef in InMemoryNode::getStatus
• Remove unused variable in TEST_F(InMemoryFileSystemTest, StatusName)

[simark]

I found something fishy. There is this code in FileManager.cpp:

if (UFE.File)
  if (auto RealPathName = UFE.File->getName())
    UFE.RealPathName = *RealPathName;

The real path is obtained from UFE.File->getName(). In the RealFile implementation, it returns the RealName field, which is fine. For other implementations, it uses File::getName, which is:

/// Get the name of the file
virtual llvm::ErrorOr<std::string> getName() {
  if (auto Status = status())
    return Status->getName().str();
  else
    return Status.getError();
}

With the InMemoryFileSystem, this now returns a non-real path. The result is that we fill RealPathName with that non-real path. I see two options here:

1. Either the FileManager is wrong to assume that File::getName returns a real path, and should call FS->getRealPath to do the job.
2. If the contract is that the File::getName interface should return a real path, then we should fix the File::getName implementation to do that somehow.

I would opt for 1. This way, we could compute the RealPathName field even if we don't open the file (unlike currently).

[ilya-biryukov]

In D48903#1154846, @simark wrote:

With the InMemoryFileSystem, this now returns a non-real path. The result is that we fill RealPathName with that non-real path. I see two options here:

1. Either the FileManager is wrong to assume that File::getName returns a real path, and should call FS->getRealPath to do the job.
2. If the contract is that the File::getName interface should return a real path, then we should fix the File::getName implementation to do that somehow.

I would opt for 1. This way, we could compute the RealPathName field even if we don't open the file (unlike currently).

I'd also say FileManager should use FileSystem::getRealPath. The code that does it differently was there before FileSystem::getRealPath was added.
And RealFile should probably not do any magic in getName, we could add a separate method for (getRealName?) if that's absolutely needed.

Refactorings in that area would probably break stuff and won't be trivial and I don't think this change should be blocked by those. So I'd be happy if this landed right away with a FIXME in FileManager mentioning that InMemoryFileSystem might give surprising results there.
@ioeric added FileSystem::getRealPath, he may more ideas on how we should proceed.

lib/Basic/VirtualFileSystem.cpp

524

This LG now, it was unindented in the original version.

729

Sorry, I thought Path is StringRef, but it's actually Twine, so we do need the str() call.

[ilya-biryukov]

lib/Basic/VirtualFileSystem.cpp

488

Given that this method is inconsistent with getStatus() and seems to be only used in toString methods, maybe we could make it protected? Otherwise it's really easy to write code that gets the wrong path.

[simark]

lib/Basic/VirtualFileSystem.cpp

488

I now use it in InMemoryDirIterator::setCurrentEntry. I will write a comment to the getName method to clarify this.

[simark]

In D48903#1155403, @ilya-biryukov wrote:

In D48903#1154846, @simark wrote:

With the InMemoryFileSystem, this now returns a non-real path. The result is that we fill RealPathName with that non-real path. I see two options here:

1. Either the FileManager is wrong to assume that File::getName returns a real path, and should call FS->getRealPath to do the job.
2. If the contract is that the File::getName interface should return a real path, then we should fix the File::getName implementation to do that somehow.

I would opt for 1. This way, we could compute the RealPathName field even if we don't open the file (unlike currently).

I'd also say FileManager should use FileSystem::getRealPath. The code that does it differently was there before FileSystem::getRealPath was added.
And RealFile should probably not do any magic in getName, we could add a separate method for (getRealName?) if that's absolutely needed.

I made FileManager::getFile use FileSystem::getRealPath and see no regression in clang and clang-tools-extra.

[simark]

• Use FileSystem::getRealPath in FileManager::getFile

[simark]

• Change InMemoryNode::getName to InMemoryNode::getFileName, to reduce the risk

of mis-using it. Make the Stat field protected, make the subclasses' toString
access it directly.

[ilya-biryukov]

lib/Basic/FileManager.cpp

320 ↗

(On Diff #154662)

NIT: replace replace equality with negative test, i.e. if (!FS->getRealPath(…))

I'm not a big fan of bash-like error codes, but that seems to be the idiomatic way to use them.

lib/Basic/VirtualFileSystem.cpp

478

The inheritors should not be able to modify this field.
Can we get away with a private field and a protected getter that returns a const reference instead?

488

getFileName as a public method and its usage in setCurrentEntry LG , thanks!

[simark]

• Make InMemoryNode::Stat private again, add protected accessor.
• Change condition formatting.

[ilya-biryukov]

LGTM if that does not introduce any regressions in clang and clang-tools.

lib/Basic/VirtualFileSystem.cpp

757

NIT: maybe increase the size to 256? This could save an extra allocation in some cases, and hopefully won't be expensive, stack allocs are cheap in most cases.

[simark]

In D48903#1157330, @ilya-biryukov wrote:

LGTM if that does not introduce any regressions in clang and clang-tools.

Thanks. I have seen no failures in check-clang and check-clang-tools, so I will push it.

[simark]

Bump SmallString size from 32 to 256

[ilya-biryukov]

In D48903#1157600, @simark wrote:

Thanks. I have seen no failures in check-clang and check-clang-tools, so I will push it.

LG! We can always revert the change is anything breaks...

[ioeric]

In D48903#1155403, @ilya-biryukov wrote:

In D48903#1154846, @simark wrote:

With the InMemoryFileSystem, this now returns a non-real path. The result is that we fill RealPathName with that non-real path. I see two options here:

1. Either the FileManager is wrong to assume that File::getName returns a real path, and should call FS->getRealPath to do the job.
2. If the contract is that the File::getName interface should return a real path, then we should fix the File::getName implementation to do that somehow.

I would opt for 1. This way, we could compute the RealPathName field even if we don't open the file (unlike currently).

I'd also say FileManager should use FileSystem::getRealPath. The code that does it differently was there before FileSystem::getRealPath was added.
And RealFile should probably not do any magic in getName, we could add a separate method for (getRealName?) if that's absolutely needed.

Refactorings in that area would probably break stuff and won't be trivial and I don't think this change should be blocked by those. So I'd be happy if this landed right away with a FIXME in FileManager mentioning that InMemoryFileSystem might give surprising results there.
@ioeric added FileSystem::getRealPath, he may more ideas on how we should proceed.

Sorry for being late in the discussion. I'm not sure if getRealPath is the right thing to do in FileManager as it also supports virtual files added via FileManager::getVirtualFile, and they don't necessary have a real path. This would also break users of ClangTool::mapVirtualFile when the mapped files are relative. As a matter of fact, I'm already seeing related breakages in our downstream branch :(

Would you mind reverting this patch for now so that we can come up with a solution to address those use cases?

Sorry again about missing the discussion earlier!

[ioeric]

In D48903#1159023, @ioeric wrote:

In D48903#1155403, @ilya-biryukov wrote:

In D48903#1154846, @simark wrote:

With the InMemoryFileSystem, this now returns a non-real path. The result is that we fill RealPathName with that non-real path. I see two options here:

1. Either the FileManager is wrong to assume that File::getName returns a real path, and should call FS->getRealPath to do the job.
2. If the contract is that the File::getName interface should return a real path, then we should fix the File::getName implementation to do that somehow.

I would opt for 1. This way, we could compute the RealPathName field even if we don't open the file (unlike currently).

I'd also say FileManager should use FileSystem::getRealPath. The code that does it differently was there before FileSystem::getRealPath was added.
And RealFile should probably not do any magic in getName, we could add a separate method for (getRealName?) if that's absolutely needed.

Refactorings in that area would probably break stuff and won't be trivial and I don't think this change should be blocked by those. So I'd be happy if this landed right away with a FIXME in FileManager mentioning that InMemoryFileSystem might give surprising results there.
@ioeric added FileSystem::getRealPath, he may more ideas on how we should proceed.

Sorry for being late in the discussion. I'm not sure if getRealPath is the right thing to do in FileManager as it also supports virtual files added via FileManager::getVirtualFile, and they don't necessary have a real path. This would also break users of ClangTool::mapVirtualFile when the mapped files are relative. As a matter of fact, I'm already seeing related breakages in our downstream branch :(

Would you mind reverting this patch for now so that we can come up with a solution to address those use cases?

Sorry again about missing the discussion earlier!

Sorry, having taken a closer look, it seems that only ClangTool::mapVirtualFile would be affected. I'll try to see if there is an easy fix.

[simark]

In D48903#1159023, @ioeric wrote:

Would you mind reverting this patch for now so that we can come up with a solution to address those use cases?

Sorry again about missing the discussion earlier!

Of course, feel free to revert if needed (I'm not sure how to do that). Are you able to come up with a test case that covers the use case you mention?

[ioeric]

In D48903#1159046, @simark wrote:

In D48903#1159023, @ioeric wrote:

Would you mind reverting this patch for now so that we can come up with a solution to address those use cases?

Sorry again about missing the discussion earlier!

Of course, feel free to revert if needed (I'm not sure how to do that). Are you able to come up with a test case that covers the use case you mention?

Thanks! I'll try to come up with a smaller test case asap.

The virtual file support in FileManager is totally confusing and full of traps (when used with VFS). I really feel we should replace it with InMemoryFileSystem at some point :(

[ioeric]

After looking at a few more use cases of the in-memory file system, I think a problem we need to address is the consistency of file paths you get from clang when using in-mem vfs. The clang-move tests that you have mitigated in D48951 could be an example.

For example, suppose you have header search directories -I/path/to/include and -I. in your compile command. When preprocessor searches for an #include "x.h", it will try to stat "/path/to/include/x.h" and "./x.h" and take the first one that exists. This can introduce indeterminism for the path (./x.h or /abs/x.h) you later get for the header file, e.g. when you try to look up file name by FileID through SourceManager. The path you get for a FileEntry or FileID would depend on how clang looks up a file and how a file is first opened into SourceManager/FileManager. It seems that the current behavior of clangd + in-memory file system would give you paths that are relative to the working directory for both cases. I'm not sure if that's the best behavior, but I think the consistency has its value. For example, in unit tests where in-memory file systems are heavily used, it's important to have a way to compare the reported file path (e.g. file path corresponding to a source location) with the expected paths. We could choose to always return real path, which could be potentially expensive, or we could require users to always compare real paths (it's unclear how well this would work though e.g. ClangTool doesn't expose its vfs), but they need to be enforced by the API. Otherwise, I worry it would cause more confusions for folks who use clang with in-memory file system in the future.

[simark]

In D48903#1159303, @ioeric wrote:

For example, suppose you have header search directories -I/path/to/include and -I. in your compile command. When preprocessor searches for an #include "x.h", it will try to stat "/path/to/include/x.h" and "./x.h" and take the first one that exists. This can introduce indeterminism for the path (./x.h or /abs/x.h) you later get for the header file, e.g. when you try to look up file name by FileID through SourceManager. The path you get for a FileEntry or FileID would depend on how clang looks up a file and how a file is first opened into SourceManager/FileManager. It seems that the current behavior of clangd + in-memory file system would give you paths that are relative to the working directory for both cases. I'm not sure if that's the best behavior, but I think the consistency has its value. For example, in unit tests where in-memory file systems are heavily used, it's important to have a way to compare the reported file path (e.g. file path corresponding to a source location) with the expected paths. We could choose to always return real path, which could be potentially expensive, or we could require users to always compare real paths (it's unclear how well this would work though e.g. ClangTool doesn't expose its vfs), but they need to be enforced by the API. Otherwise, I worry it would cause more confusions for folks who use clang with in-memory file system in the future.

It's hard to tell without seeing an actual failing use case. I understand the argument, but I think the necessity to work as closely as RealFileSystem as possible is important. Otherwise, it becomes impossible to reproduce bugs happening in the real world in unittests.

[ioeric]

In D48903#1159985, @simark wrote:

In D48903#1159303, @ioeric wrote:

For example, suppose you have header search directories -I/path/to/include and -I. in your compile command. When preprocessor searches for an #include "x.h", it will try to stat "/path/to/include/x.h" and "./x.h" and take the first one that exists. This can introduce indeterminism for the path (./x.h or /abs/x.h) you later get for the header file, e.g. when you try to look up file name by FileID through SourceManager. The path you get for a FileEntry or FileID would depend on how clang looks up a file and how a file is first opened into SourceManager/FileManager. It seems that the current behavior of clangd + in-memory file system would give you paths that are relative to the working directory for both cases. I'm not sure if that's the best behavior, but I think the consistency has its value. For example, in unit tests where in-memory file systems are heavily used, it's important to have a way to compare the reported file path (e.g. file path corresponding to a source location) with the expected paths. We could choose to always return real path, which could be potentially expensive, or we could require users to always compare real paths (it's unclear how well this would work though e.g. ClangTool doesn't expose its vfs), but they need to be enforced by the API. Otherwise, I worry it would cause more confusions for folks who use clang with in-memory file system in the future.

It's hard to tell without seeing an actual failing use case.

I think the clang-move test you mitigated in D48951 is an example. When setting up compiler instance, it uses -I. in the compile command (https://github.com/llvm-mirror/clang-tools-extra/blob/master/unittests/clang-move/ClangMoveTests.cpp#L236), which results in the header paths that start with ./. If you change replace . with the absolute path of the working directory e.g. -I/usr/include, the paths you get will start with /usr/include/. This is caused by the way preprocessor looks up include headers. We could require users (e.g. the clang-move test writer) to clean up dots, but this has to be enforced somehow by the framework.

I understand the argument, but I think the necessity to work as closely as RealFileSystem as possible is important. Otherwise, it becomes impossible to reproduce bugs happening in the real world in unittests.

FWIW, I don't think we could reproduce all real world bugs with unit tests ;) But I agree with you that we should try to converge the behavior if possible, and I support the motivation of this change ;) This change would be perfectly fine as is if in-mem fs is always used directly by users, but the problem is that it's also used inside clang and clang tooling, where users don't control over how files are opened. My point is we should do this in a way that doesn't introduce inconsistency/confusion for users of clang and tooling framework.

[simark]

I uploaded a new version of this patch here:
https://reviews.llvm.org/D49804

[ioeric]

It would make it easier for your reviewers to look at the new changes if
you just reopen this patch and update the diff :)

[simark]

In D48903#1175317, @ioeric wrote:

It would make it easier for your reviewers to look at the new changes if
you just reopen this patch and update the diff :)

I tried but didn't know how. But I just saw the "Add Action" drop down with "Reopen" in it.

[simark]

Fix tests on Windows

Fix InMemoryFileSystem tests on Windows. The paths returned by the
InMemoryFileSystem directory iterators in the tests mix posix and windows
directory separators. THis is because we do queries with posix-style
separators ("/a/b") but filnames are appended using native-style separators
(backslash on Windows). So we end up with "/a/b\c".

I fixed the test by re-using the ReplaceBackslashes function defined in another
test. I'm not sure this is the best fix, but the only alternative I see would
be to completely rewrite tests to use posix-style paths on non-Windows and
Windows-style paths on Windows. That would lead to quite a bit of
duplication...

[ilya-biryukov]

Many thanks! Great cleanup. Just a few nits and we're good to go

lib/Basic/VirtualFileSystem.cpp

476

NIT: maybe keep the order of members the same to keep the patch more focused? Unless there's a good reason to swap them.

518–521

NIT: remove this blank line to follow the code style of the file more closely?

759

NIT: Path.str() can be replaced with Path (SmallString is convertible to StringRef)

unittests/Basic/VirtualFileSystemTest.cpp

155

Maybe replace lambda with a funciton?

155

Could the name mention the expected result? E.g. getUnixPath() or something similar

156

Maybe use llvm::sys::path::native(S, style::posix)?

785

Maybe add a comment about windows and the path we get there?

[simark]

lib/Basic/VirtualFileSystem.cpp

476

Oops, that was not intended.

[simark]

I think this addresses all of Ilya's comments.

[ilya-biryukov]

LGTM

[simark]

@eric_niebler, question for you:

This patch causes clang to emit a -Wnonportable-include-path warning where it did not before. It affects the following test on Windows:
https://github.com/llvm-mirror/clang/blob/master/test/PCH/case-insensitive-include.c

The warning is currently not emitted for the last clang invocation (the one that includes PCH), because the real path value is not available, and therefore this condition is false:
https://github.com/llvm-mirror/clang/blob/fe1098c84823b8eac46b0bfffc5f5788b6c26d1a/lib/Lex/PPDirectives.cpp#L2015

With this patch, the real path value is available, so we emit the warning. Is it on purpose that this warning is not emitted in this case? If not should I simply add -Wno-nonportable-include-path to the last clang invocation, as was done earlier with the first invocation?

It seems to me like the warning is valid, even though we use precompiled headers.

[eric_niebler]

It seems to me like the warning is valid, even though we use precompiled headers.

Agreed.

[simark]

Add -Wno-nonportable-include-path to test/PCH/case-insensitive-include.c

[ilya-biryukov]

This revision got 'reopened' and is now in the list of accepted revision. Should we close it again?

[simark]

In D48903#1187596, @ilya-biryukov wrote:

This revision got 'reopened' and is now in the list of accepted revision. Should we close it again?

It got reverted a second time because it was breaking a test on Windows. The new bit is the change to test/PCH/case-insensitive-include.c, so it would need review again. If somebody else could run the tests on Windows, it would make me a bit more confident too.

[malaperle]

In D48903#1187605, @simark wrote:

If somebody else could run the tests on Windows, it would make me a bit more confident too.

Which tests/targets exactly? If you know

[malaperle]

In D48903#1188437, @malaperle wrote:

In D48903#1187605, @simark wrote:

If somebody else could run the tests on Windows, it would make me a bit more confident too.

Which tests/targets exactly? If you know

NVM, I saw the "check-clang and check-clang-tools" above.

[malaperle]

Both check-clang and check-clang-tools pass successfully for me on Windows with the patch.

[simark]

In D48903#1188566, @malaperle wrote:

Both check-clang and check-clang-tools pass successfully for me on Windows with the patch.

Awesome, thanks!

[ilya-biryukov]

In D48903#1187605, @simark wrote:

In D48903#1187596, @ilya-biryukov wrote:

This revision got 'reopened' and is now in the list of accepted revision. Should we close it again?

It got reverted a second time because it was breaking a test on Windows. The new bit is the change to test/PCH/case-insensitive-include.c, so it would need review again. If somebody else could run the tests on Windows, it would make me a bit more confident too.

I see, thanks for the explanation.

LGTM for the update revision, given we have confirmation the tests pass on Windows.

[simark]

In D48903#1189056, @ilya-biryukov wrote:

I see, thanks for the explanation.

LGTM for the update revision, given we have confirmation the tests pass on Windows.

Thanks, I'll push it, let's hope this time is the right time!

lib/Basic/VirtualFileSystem.cpp

 
 enum InMemoryNodeKind { IME_File, IME_Directory };
 
 /// 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;
 
 public:
   InMemoryNode(Status Stat, InMemoryNodeKind Kind)
       : Stat(std::move(Stat)), Kind(Kind) {}
   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(std::string RequestedName) const {
+    return Status::copyWithNewName(Stat, RequestedName);
+  }
+  StringRef getName() const { return Stat.getName(); }
   InMemoryNodeKind getKind() const { return Kind; }
   virtual std::string toString(unsigned Indent) const = 0;
 };
 
 namespace {
 
 class InMemoryFile : public InMemoryNode {
   std::unique_ptr<llvm::MemoryBuffer> Buffer;
 
 public:
   InMemoryFile(Status Stat, std::unique_ptr<llvm::MemoryBuffer> Buffer)
       : InMemoryNode(std::move(Stat), IME_File), Buffer(std::move(Buffer)) {}
 
   llvm::MemoryBuffer *getBuffer() { return Buffer.get(); }
 
   std::string toString(unsigned Indent) const override {
-    return (std::string(Indent, ' ') + getStatus().getName() + "\n").str();
+    return (std::string(Indent, ' ') + getName() + "\n").str();
   }
 
   static bool classof(const InMemoryNode *N) {
     return N->getKind() == IME_File;
   }
 };
 
-/// 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,
             bool IsVolatile) override {
     llvm::MemoryBuffer *Buf = Node.getBuffer();
     return llvm::MemoryBuffer::getMemBuffer(
         Buf->getBuffer(), Buf->getBufferIdentifier(), RequiresNullTerminator);
   }
public:
   }
 
   using const_iterator = decltype(Entries)::const_iterator;
 
   const_iterator begin() const { return Entries.begin(); }
   const_iterator end() const { return Entries.end(); }
 
   std::string toString(unsigned Indent) const override {
-    std::string Result =
-        (std::string(Indent, ' ') + getStatus().getName() + "\n").str();
+    std::string Result = (std::string(Indent, ' ') + getName() + "\n").str();
     for (const auto &Entry : Entries)
       Result += Entry.second->toString(Indent + 2);
     return Result;
   }
 
   static bool classof(const InMemoryNode *N) {
     return N->getKind() == IME_Directory;
   }
while (true) {
     if (I == E)
       return Dir;
   }
 }
 
 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();
 }
 
 llvm::ErrorOr<std::unique_ptr<File>>
 InMemoryFileSystem::openFileForRead(const Twine &Path) {
   auto Node = lookupInMemoryNode(*this, Root.get(), Path);
   if (!Node)
     return Node.getError();
 
   // 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);
 }
 
 namespace {
 
 /// Adaptor from InMemoryDir::iterator to directory_iterator.
 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<32> Path(RequestedDirName);
+      llvm::sys::path::append(Path,
+                              llvm::sys::path::filename(I->second->getName()));
+      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 {};
   }
 };
 
 } // namespace
 
 directory_iterator InMemoryFileSystem::dir_begin(const Twine &Dir,
                                                  std::error_code &EC) {
   auto Node = lookupInMemoryNode(*this, Root.get(), Dir);
   if (!Node) {
     EC = Node.getError();
     return directory_iterator(std::make_shared<InMemoryDirIterator>());
   }
 
   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>());
 }
 
 std::error_code InMemoryFileSystem::setCurrentWorkingDirectory(const Twine &P) {
   SmallString<128> Path;
   P.toVector(Path);

unittests/Basic/VirtualFileSystemTest.cpp

public:
   void addSymlink(StringRef Path) {
     vfs::Status S(Path, UniqueID(FSID, FileID++),
                   std::chrono::system_clock::now(), 0, 0, 0,
                   sys::fs::file_type::symlink_file, sys::fs::all_all);
     addEntry(Path, S);
   }
 };
 } // end anonymous namespace
 
 TEST(VirtualFileSystemTest, StatusQueries) {
   IntrusiveRefCntPtr<DummyFileSystem> D(new DummyFileSystem());
   ErrorOr<vfs::Status> Status((std::error_code()));
 
   D->addRegularFile("/foo");
   Status = D->status("/foo");
   ASSERT_FALSE(Status.getError());
   EXPECT_TRUE(Status->isStatusKnown());
   EXPECT_FALSE(Status->isDirectory());
TEST_F(InMemoryFileSystemTest, DirectoryIteration) {
   ASSERT_FALSE(EC);
   ASSERT_EQ("/b", I->getName());
   I.increment(EC);
   ASSERT_FALSE(EC);
   ASSERT_EQ(vfs::directory_iterator(), I);
 
   I = FS.dir_begin("/b", EC);
   ASSERT_FALSE(EC);
   ASSERT_EQ("/b/c", I->getName());
   I.increment(EC);
   ASSERT_FALSE(EC);
   ASSERT_EQ(vfs::directory_iterator(), I);
 }
 
 TEST_F(InMemoryFileSystemTest, WorkingDirectory) {
   FS.setCurrentWorkingDirectory("/b");
   FS.addFile("c", 0, MemoryBuffer::getMemBuffer(""));
 
   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");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
 
   auto ReplaceBackslashes = [](std::string S) {
     std::replace(S.begin(), S.end(), '\\', '/');
     return S;
TEST_F(InMemoryFileSystemTest, AddDirectoryThenAddChild) {
   auto Stat = FS.status("/a");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
   ASSERT_TRUE(Stat->isDirectory());
   Stat = FS.status("/a/b");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
   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);
+  clang::vfs::directory_iterator End;
+
+  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 {
 public:
   int NumDiagnostics;
 
   void SetUp() override { NumDiagnostics = 0; }
 

unittests/Driver/ToolChainTest.cpp

TEST(ToolChainTest, VFSGCCInstallationRelativeDir) {
   {
     llvm::raw_string_ostream OS(S);
     C->getDefaultToolChain().printVerboseInfo(OS);
   }
 #if _WIN32
   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"
             "Selected multilib: .;@m32\n",
             S);
 }
 
 TEST(ToolChainTest, DefaultDriverMode) {