Skip to content

Commit 55a9501

Browse files
keithkeith
committed
reland: [VFS] Use original path when falling back to external FS
This reverts commit f0cf544. Just a small change to fix: ``` /home/buildbot/as-builder-4/llvm-clang-x86_64-expensive-checks-ubuntu/llvm-project/llvm/lib/Support/VirtualFileSystem.cpp: In static member function ‘static llvm::ErrorOr<std::unique_ptr<llvm::vfs::File> > llvm::vfs::File::getWithPath(llvm::ErrorOr<std::unique_ptr<llvm::vfs::File> >, const llvm::Twine&)’: /home/buildbot/as-builder-4/llvm-clang-x86_64-expensive-checks-ubuntu/llvm-project/llvm/lib/Support/VirtualFileSystem.cpp:2084:10: error: could not convert ‘F’ from ‘std::unique_ptr<llvm::vfs::File>’ to ‘llvm::ErrorOr<std::unique_ptr<llvm::vfs::File> >’ return F; ^ ``` Differential Revision: https://reviews.llvm.org/D113832 (cherry picked from commit 86e2af8)
1 parent 93bcb83 commit 55a9501

File tree

4 files changed

+215
-28
lines changed

4 files changed

+215
-28
lines changed

‎clang/test/VFS/relative-path-errors.c

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
// RUN: mkdir -p %t
2+
// RUN: cd %t
3+
// RUN: cp %s main.c
4+
// RUN: not %clang_cc1 main.c 2>&1 | FileCheck %s
5+
// RUN: echo '{"roots": [],"version": 0}' > %t.yaml
6+
// RUN: not %clang_cc1 -ivfsoverlay %t.yaml main.c 2>&1 | FileCheck %s
7+
// RUN: echo '{"version": 0,"roots":[{"type":"directory","name":"%/t","contents":[{"type":"file","name":"vfsname", "external-contents":"main.c"}]}]}' > %t.yaml
8+
// RUN: not %clang_cc1 -ivfsoverlay %t.yaml vfsname 2>&1 | FileCheck %s
9+
10+
// CHECK: {{^}}main.c:[[# @LINE + 1]]:1: error:
11+
foobarbaz

‎llvm/include/llvm/Support/VirtualFileSystem.h

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,14 @@ class File {
121121

122122
/// Closes the file.
123123
virtual std::error_code close() = 0;
124+
125+
// Get the same file with a different path.
126+
static ErrorOr<std::unique_ptr<File>>
127+
getWithPath(ErrorOr<std::unique_ptr<File>> Result, const Twine &P);
128+
129+
protected:
130+
// Set the file's underlying path.
131+
virtual void setPath(const Twine &Path) {}
124132
};
125133

126134
/// A member of a directory, yielded by a directory_iterator.
@@ -746,6 +754,12 @@ class RedirectingFileSystem : public vfs::FileSystem {
746754
/// with the given error code on a path associated with the provided Entry.
747755
bool shouldFallBackToExternalFS(std::error_code EC, Entry *E = nullptr) const;
748756

757+
/// Get the File status, or error, from the underlying external file system.
758+
/// This returns the status with the originally requested name, while looking
759+
/// up the entry using the canonical path.
760+
ErrorOr<Status> getExternalStatus(const Twine &CanonicalPath,
761+
const Twine &OriginalPath) const;
762+
749763
// In a RedirectingFileSystem, keys can be specified in Posix or Windows
750764
// style (or even a mixture of both), so this comparison helper allows
751765
// slashes (representing a root) to match backslashes (and vice versa). Note
@@ -808,7 +822,8 @@ class RedirectingFileSystem : public vfs::FileSystem {
808822
Entry *From) const;
809823

810824
/// Get the status for a path with the provided \c LookupResult.
811-
ErrorOr<Status> status(const Twine &Path, const LookupResult &Result);
825+
ErrorOr<Status> status(const Twine &CanonicalPath, const Twine &OriginalPath,
826+
const LookupResult &Result);
812827

813828
public:
814829
/// Looks up \p Path in \c Roots and returns a LookupResult giving the

‎llvm/lib/Support/VirtualFileSystem.cpp

Lines changed: 80 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,7 @@ class RealFile : public File {
193193
bool RequiresNullTerminator,
194194
bool IsVolatile) override;
195195
std::error_code close() override;
196+
void setPath(const Twine &Path) override;
196197
};
197198

198199
} // namespace
@@ -228,6 +229,12 @@ std::error_code RealFile::close() {
228229
return EC;
229230
}
230231

232+
void RealFile::setPath(const Twine &Path) {
233+
RealName = Path.str();
234+
if (auto Status = status())
235+
S = Status.get().copyWithNewName(Status.get(), Path);
236+
}
237+
231238
namespace {
232239

233240
/// A file system according to your operating system.
@@ -638,6 +645,8 @@ class InMemoryFileAdaptor : public File {
638645
}
639646

640647
std::error_code close() override { return {}; }
648+
649+
void setPath(const Twine &Path) override { RequestedName = Path.str(); }
641650
};
642651
} // namespace
643652

@@ -1207,7 +1216,7 @@ directory_iterator RedirectingFileSystem::dir_begin(const Twine &Dir,
12071216
}
12081217

12091218
// Use status to make sure the path exists and refers to a directory.
1210-
ErrorOr<Status> S = status(Path, *Result);
1219+
ErrorOr<Status> S = status(Path, Dir, *Result);
12111220
if (!S) {
12121221
if (shouldFallBackToExternalFS(S.getError(), Result->E))
12131222
return ExternalFS->dir_begin(Dir, EC);
@@ -1933,47 +1942,68 @@ RedirectingFileSystem::lookupPathImpl(
19331942
return make_error_code(llvm::errc::no_such_file_or_directory);
19341943
}
19351944

1936-
static Status getRedirectedFileStatus(const Twine &Path, bool UseExternalNames,
1945+
static Status getRedirectedFileStatus(const Twine &OriginalPath,
1946+
bool UseExternalNames,
19371947
Status ExternalStatus) {
19381948
Status S = ExternalStatus;
19391949
if (!UseExternalNames)
1940-
S = Status::copyWithNewName(S, Path);
1950+
S = Status::copyWithNewName(S, OriginalPath);
19411951
S.IsVFSMapped = true;
19421952
return S;
19431953
}
19441954

19451955
ErrorOr<Status> RedirectingFileSystem::status(
1946-
const Twine &Path, const RedirectingFileSystem::LookupResult &Result) {
1956+
const Twine &CanonicalPath, const Twine &OriginalPath,
1957+
const RedirectingFileSystem::LookupResult &Result) {
19471958
if (Optional<StringRef> ExtRedirect = Result.getExternalRedirect()) {
1948-
ErrorOr<Status> S = ExternalFS->status(*ExtRedirect);
1959+
SmallString<256> CanonicalRemappedPath((*ExtRedirect).str());
1960+
if (std::error_code EC = makeCanonical(CanonicalRemappedPath))
1961+
return EC;
1962+
1963+
ErrorOr<Status> S = ExternalFS->status(CanonicalRemappedPath);
19491964
if (!S)
19501965
return S;
1966+
S = Status::copyWithNewName(*S, *ExtRedirect);
19511967
auto *RE = cast<RedirectingFileSystem::RemapEntry>(Result.E);
1952-
return getRedirectedFileStatus(Path, RE->useExternalName(UseExternalNames),
1953-
*S);
1968+
return getRedirectedFileStatus(OriginalPath,
1969+
RE->useExternalName(UseExternalNames), *S);
19541970
}
19551971

19561972
auto *DE = cast<RedirectingFileSystem::DirectoryEntry>(Result.E);
1957-
return Status::copyWithNewName(DE->getStatus(), Path);
1973+
return Status::copyWithNewName(DE->getStatus(), CanonicalPath);
19581974
}
19591975

1960-
ErrorOr<Status> RedirectingFileSystem::status(const Twine &Path_) {
1961-
SmallString<256> Path;
1962-
Path_.toVector(Path);
1976+
ErrorOr<Status>
1977+
RedirectingFileSystem::getExternalStatus(const Twine &CanonicalPath,
1978+
const Twine &OriginalPath) const {
1979+
if (auto Result = ExternalFS->status(CanonicalPath)) {
1980+
return Result.get().copyWithNewName(Result.get(), OriginalPath);
1981+
} else {
1982+
return Result.getError();
1983+
}
1984+
}
19631985

1964-
if (std::error_code EC = makeCanonical(Path))
1986+
ErrorOr<Status> RedirectingFileSystem::status(const Twine &OriginalPath) {
1987+
SmallString<256> CanonicalPath;
1988+
OriginalPath.toVector(CanonicalPath);
1989+
1990+
if (std::error_code EC = makeCanonical(CanonicalPath))
19651991
return EC;
19661992

1967-
ErrorOr<RedirectingFileSystem::LookupResult> Result = lookupPath(Path);
1993+
ErrorOr<RedirectingFileSystem::LookupResult> Result =
1994+
lookupPath(CanonicalPath);
19681995
if (!Result) {
1969-
if (shouldFallBackToExternalFS(Result.getError()))
1970-
return ExternalFS->status(Path);
1996+
if (shouldFallBackToExternalFS(Result.getError())) {
1997+
return getExternalStatus(CanonicalPath, OriginalPath);
1998+
}
19711999
return Result.getError();
19722000
}
19732001

1974-
ErrorOr<Status> S = status(Path, *Result);
1975-
if (!S && shouldFallBackToExternalFS(S.getError(), Result->E))
1976-
S = ExternalFS->status(Path);
2002+
ErrorOr<Status> S = status(CanonicalPath, OriginalPath, *Result);
2003+
if (!S && shouldFallBackToExternalFS(S.getError(), Result->E)) {
2004+
return getExternalStatus(CanonicalPath, OriginalPath);
2005+
}
2006+
19772007
return S;
19782008
}
19792009

@@ -1998,35 +2028,58 @@ class FileWithFixedStatus : public File {
19982028
}
19992029

20002030
std::error_code close() override { return InnerFile->close(); }
2031+
2032+
void setPath(const Twine &Path) override { S = S.copyWithNewName(S, Path); }
20012033
};
20022034

20032035
} // namespace
20042036

20052037
ErrorOr<std::unique_ptr<File>>
2006-
RedirectingFileSystem::openFileForRead(const Twine &Path_) {
2007-
SmallString<256> Path;
2008-
Path_.toVector(Path);
2038+
File::getWithPath(ErrorOr<std::unique_ptr<File>> Result, const Twine &P) {
2039+
if (!Result)
2040+
return Result;
20092041

2010-
if (std::error_code EC = makeCanonical(Path))
2042+
ErrorOr<std::unique_ptr<File>> F = std::move(*Result);
2043+
auto Name = F->get()->getName();
2044+
if (Name && Name.get() != P.str())
2045+
F->get()->setPath(P);
2046+
return F;
2047+
}
2048+
2049+
ErrorOr<std::unique_ptr<File>>
2050+
RedirectingFileSystem::openFileForRead(const Twine &OriginalPath) {
2051+
SmallString<256> CanonicalPath;
2052+
OriginalPath.toVector(CanonicalPath);
2053+
2054+
if (std::error_code EC = makeCanonical(CanonicalPath))
20112055
return EC;
20122056

2013-
ErrorOr<RedirectingFileSystem::LookupResult> Result = lookupPath(Path);
2057+
ErrorOr<RedirectingFileSystem::LookupResult> Result =
2058+
lookupPath(CanonicalPath);
20142059
if (!Result) {
20152060
if (shouldFallBackToExternalFS(Result.getError()))
2016-
return ExternalFS->openFileForRead(Path);
2061+
return File::getWithPath(ExternalFS->openFileForRead(CanonicalPath),
2062+
OriginalPath);
2063+
20172064
return Result.getError();
20182065
}
20192066

20202067
if (!Result->getExternalRedirect()) // FIXME: errc::not_a_file?
20212068
return make_error_code(llvm::errc::invalid_argument);
20222069

20232070
StringRef ExtRedirect = *Result->getExternalRedirect();
2071+
SmallString<256> CanonicalRemappedPath(ExtRedirect.str());
2072+
if (std::error_code EC = makeCanonical(CanonicalRemappedPath))
2073+
return EC;
2074+
20242075
auto *RE = cast<RedirectingFileSystem::RemapEntry>(Result->E);
20252076

2026-
auto ExternalFile = ExternalFS->openFileForRead(ExtRedirect);
2077+
auto ExternalFile = File::getWithPath(
2078+
ExternalFS->openFileForRead(CanonicalRemappedPath), ExtRedirect);
20272079
if (!ExternalFile) {
20282080
if (shouldFallBackToExternalFS(ExternalFile.getError(), Result->E))
2029-
return ExternalFS->openFileForRead(Path);
2081+
return File::getWithPath(ExternalFS->openFileForRead(CanonicalPath),
2082+
OriginalPath);
20302083
return ExternalFile;
20312084
}
20322085

@@ -2036,7 +2089,7 @@ RedirectingFileSystem::openFileForRead(const Twine &Path_) {
20362089

20372090
// FIXME: Update the status with the name and VFSMapped.
20382091
Status S = getRedirectedFileStatus(
2039-
Path, RE->useExternalName(UseExternalNames), *ExternalStatus);
2092+
OriginalPath, RE->useExternalName(UseExternalNames), *ExternalStatus);
20402093
return std::unique_ptr<File>(
20412094
std::make_unique<FileWithFixedStatus>(std::move(*ExternalFile), S));
20422095
}

‎llvm/unittests/Support/VirtualFileSystemTest.cpp

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1607,6 +1607,114 @@ TEST_F(VFSFromYAMLTest, RemappedDirectoryOverlayNoFallthrough) {
16071607
EXPECT_EQ(0, NumDiagnostics);
16081608
}
16091609

1610+
TEST_F(VFSFromYAMLTest, ReturnsRequestedPathVFSMiss) {
1611+
IntrusiveRefCntPtr<vfs::InMemoryFileSystem> BaseFS(
1612+
new vfs::InMemoryFileSystem);
1613+
BaseFS->addFile("//root/foo/a", 0,
1614+
MemoryBuffer::getMemBuffer("contents of a"));
1615+
ASSERT_FALSE(BaseFS->setCurrentWorkingDirectory("//root/foo"));
1616+
auto RemappedFS = vfs::RedirectingFileSystem::create(
1617+
{}, /*UseExternalNames=*/false, *BaseFS);
1618+
1619+
auto OpenedF = RemappedFS->openFileForRead("a");
1620+
ASSERT_FALSE(OpenedF.getError());
1621+
llvm::ErrorOr<std::string> Name = (*OpenedF)->getName();
1622+
ASSERT_FALSE(Name.getError());
1623+
EXPECT_EQ("a", Name.get());
1624+
1625+
auto OpenedS = (*OpenedF)->status();
1626+
ASSERT_FALSE(OpenedS.getError());
1627+
EXPECT_EQ("a", OpenedS->getName());
1628+
EXPECT_FALSE(OpenedS->IsVFSMapped);
1629+
1630+
auto DirectS = RemappedFS->status("a");
1631+
ASSERT_FALSE(DirectS.getError());
1632+
EXPECT_EQ("a", DirectS->getName());
1633+
EXPECT_FALSE(DirectS->IsVFSMapped);
1634+
1635+
EXPECT_EQ(0, NumDiagnostics);
1636+
}
1637+
1638+
TEST_F(VFSFromYAMLTest, ReturnsExternalPathVFSHit) {
1639+
IntrusiveRefCntPtr<vfs::InMemoryFileSystem> BaseFS(
1640+
new vfs::InMemoryFileSystem);
1641+
BaseFS->addFile("//root/foo/realname", 0,
1642+
MemoryBuffer::getMemBuffer("contents of a"));
1643+
auto FS =
1644+
getFromYAMLString("{ 'use-external-names': true,\n"
1645+
" 'roots': [\n"
1646+
"{\n"
1647+
" 'type': 'directory',\n"
1648+
" 'name': '//root/foo',\n"
1649+
" 'contents': [ {\n"
1650+
" 'type': 'file',\n"
1651+
" 'name': 'vfsname',\n"
1652+
" 'external-contents': 'realname'\n"
1653+
" }\n"
1654+
" ]\n"
1655+
"}]}",
1656+
BaseFS);
1657+
ASSERT_FALSE(FS->setCurrentWorkingDirectory("//root/foo"));
1658+
1659+
auto OpenedF = FS->openFileForRead("vfsname");
1660+
ASSERT_FALSE(OpenedF.getError());
1661+
llvm::ErrorOr<std::string> Name = (*OpenedF)->getName();
1662+
ASSERT_FALSE(Name.getError());
1663+
EXPECT_EQ("realname", Name.get());
1664+
1665+
auto OpenedS = (*OpenedF)->status();
1666+
ASSERT_FALSE(OpenedS.getError());
1667+
EXPECT_EQ("realname", OpenedS->getName());
1668+
EXPECT_TRUE(OpenedS->IsVFSMapped);
1669+
1670+
auto DirectS = FS->status("vfsname");
1671+
ASSERT_FALSE(DirectS.getError());
1672+
EXPECT_EQ("realname", DirectS->getName());
1673+
EXPECT_TRUE(DirectS->IsVFSMapped);
1674+
1675+
EXPECT_EQ(0, NumDiagnostics);
1676+
}
1677+
1678+
TEST_F(VFSFromYAMLTest, ReturnsInternalPathVFSHit) {
1679+
IntrusiveRefCntPtr<vfs::InMemoryFileSystem> BaseFS(
1680+
new vfs::InMemoryFileSystem);
1681+
BaseFS->addFile("//root/foo/realname", 0,
1682+
MemoryBuffer::getMemBuffer("contents of a"));
1683+
auto FS =
1684+
getFromYAMLString("{ 'use-external-names': false,\n"
1685+
" 'roots': [\n"
1686+
"{\n"
1687+
" 'type': 'directory',\n"
1688+
" 'name': '//root/foo',\n"
1689+
" 'contents': [ {\n"
1690+
" 'type': 'file',\n"
1691+
" 'name': 'vfsname',\n"
1692+
" 'external-contents': 'realname'\n"
1693+
" }\n"
1694+
" ]\n"
1695+
"}]}",
1696+
BaseFS);
1697+
ASSERT_FALSE(FS->setCurrentWorkingDirectory("//root/foo"));
1698+
1699+
auto OpenedF = FS->openFileForRead("vfsname");
1700+
ASSERT_FALSE(OpenedF.getError());
1701+
llvm::ErrorOr<std::string> Name = (*OpenedF)->getName();
1702+
ASSERT_FALSE(Name.getError());
1703+
EXPECT_EQ("vfsname", Name.get());
1704+
1705+
auto OpenedS = (*OpenedF)->status();
1706+
ASSERT_FALSE(OpenedS.getError());
1707+
EXPECT_EQ("vfsname", OpenedS->getName());
1708+
EXPECT_TRUE(OpenedS->IsVFSMapped);
1709+
1710+
auto DirectS = FS->status("vfsname");
1711+
ASSERT_FALSE(DirectS.getError());
1712+
EXPECT_EQ("vfsname", DirectS->getName());
1713+
EXPECT_TRUE(DirectS->IsVFSMapped);
1714+
1715+
EXPECT_EQ(0, NumDiagnostics);
1716+
}
1717+
16101718
TEST_F(VFSFromYAMLTest, CaseInsensitive) {
16111719
IntrusiveRefCntPtr<DummyFileSystem> Lower(new DummyFileSystem());
16121720
Lower->addRegularFile("//root/foo/bar/a");

0 commit comments

Comments
 (0)