Author: Keith Smiley Date: 2021-11-13T12:14:34-08:00 New Revision: 86e2af8043c7728710a711b623f27425801a36c3
URL: https://github.com/llvm/llvm-project/commit/86e2af8043c7728710a711b623f27425801a36c3 DIFF: https://github.com/llvm/llvm-project/commit/86e2af8043c7728710a711b623f27425801a36c3.diff LOG: reland: [VFS] Use original path when falling back to external FS This reverts commit f0cf544d6f6fe6cbca4c07772998272d6bb433d8. 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 Added: clang/test/VFS/relative-path-errors.c Modified: llvm/include/llvm/Support/VirtualFileSystem.h llvm/lib/Support/VirtualFileSystem.cpp llvm/unittests/Support/VirtualFileSystemTest.cpp Removed: ################################################################################ diff --git a/clang/test/VFS/relative-path-errors.c b/clang/test/VFS/relative-path-errors.c new file mode 100644 index 0000000000000..8d3773a5eb592 --- /dev/null +++ b/clang/test/VFS/relative-path-errors.c @@ -0,0 +1,11 @@ +// RUN: mkdir -p %t +// RUN: cd %t +// RUN: cp %s main.c +// RUN: not %clang_cc1 main.c 2>&1 | FileCheck %s +// RUN: echo '{"roots": [],"version": 0}' > %t.yaml +// RUN: not %clang_cc1 -ivfsoverlay %t.yaml main.c 2>&1 | FileCheck %s +// RUN: echo '{"version": 0,"roots":[{"type":"directory","name":"%/t","contents":[{"type":"file","name":"vfsname", "external-contents":"main.c"}]}]}' > %t.yaml +// RUN: not %clang_cc1 -ivfsoverlay %t.yaml vfsname 2>&1 | FileCheck %s + +// CHECK: {{^}}main.c:[[# @LINE + 1]]:1: error: +foobarbaz diff --git a/llvm/include/llvm/Support/VirtualFileSystem.h b/llvm/include/llvm/Support/VirtualFileSystem.h index 38f426ff04432..10d2389ee079d 100644 --- a/llvm/include/llvm/Support/VirtualFileSystem.h +++ b/llvm/include/llvm/Support/VirtualFileSystem.h @@ -121,6 +121,14 @@ class File { /// Closes the file. virtual std::error_code close() = 0; + + // Get the same file with a diff erent path. + static ErrorOr<std::unique_ptr<File>> + getWithPath(ErrorOr<std::unique_ptr<File>> Result, const Twine &P); + +protected: + // Set the file's underlying path. + virtual void setPath(const Twine &Path) {} }; /// A member of a directory, yielded by a directory_iterator. @@ -757,6 +765,12 @@ class RedirectingFileSystem : public vfs::FileSystem { /// with the given error code on a path associated with the provided Entry. bool shouldFallBackToExternalFS(std::error_code EC, Entry *E = nullptr) const; + /// Get the File status, or error, from the underlying external file system. + /// This returns the status with the originally requested name, while looking + /// up the entry using the canonical path. + ErrorOr<Status> getExternalStatus(const Twine &CanonicalPath, + const Twine &OriginalPath) const; + // In a RedirectingFileSystem, keys can be specified in Posix or Windows // style (or even a mixture of both), so this comparison helper allows // slashes (representing a root) to match backslashes (and vice versa). Note @@ -814,7 +828,8 @@ class RedirectingFileSystem : public vfs::FileSystem { Entry *From) const; /// Get the status for a path with the provided \c LookupResult. - ErrorOr<Status> status(const Twine &Path, const LookupResult &Result); + ErrorOr<Status> status(const Twine &CanonicalPath, const Twine &OriginalPath, + const LookupResult &Result); public: /// Looks up \p Path in \c Roots and returns a LookupResult giving the diff --git a/llvm/lib/Support/VirtualFileSystem.cpp b/llvm/lib/Support/VirtualFileSystem.cpp index a4abfe19bcbdc..9bf0384b5f1b0 100644 --- a/llvm/lib/Support/VirtualFileSystem.cpp +++ b/llvm/lib/Support/VirtualFileSystem.cpp @@ -194,6 +194,7 @@ class RealFile : public File { bool RequiresNullTerminator, bool IsVolatile) override; std::error_code close() override; + void setPath(const Twine &Path) override; }; } // namespace @@ -229,6 +230,12 @@ std::error_code RealFile::close() { return EC; } +void RealFile::setPath(const Twine &Path) { + RealName = Path.str(); + if (auto Status = status()) + S = Status.get().copyWithNewName(Status.get(), Path); +} + namespace { /// A file system according to your operating system. @@ -639,6 +646,8 @@ class InMemoryFileAdaptor : public File { } std::error_code close() override { return {}; } + + void setPath(const Twine &Path) override { RequestedName = Path.str(); } }; } // namespace @@ -1244,7 +1253,7 @@ directory_iterator RedirectingFileSystem::dir_begin(const Twine &Dir, } // Use status to make sure the path exists and refers to a directory. - ErrorOr<Status> S = status(Path, *Result); + ErrorOr<Status> S = status(Path, Dir, *Result); if (!S) { if (shouldFallBackToExternalFS(S.getError(), Result->E)) return ExternalFS->dir_begin(Dir, EC); @@ -1971,47 +1980,68 @@ RedirectingFileSystem::lookupPathImpl( return make_error_code(llvm::errc::no_such_file_or_directory); } -static Status getRedirectedFileStatus(const Twine &Path, bool UseExternalNames, +static Status getRedirectedFileStatus(const Twine &OriginalPath, + bool UseExternalNames, Status ExternalStatus) { Status S = ExternalStatus; if (!UseExternalNames) - S = Status::copyWithNewName(S, Path); + S = Status::copyWithNewName(S, OriginalPath); S.IsVFSMapped = true; return S; } ErrorOr<Status> RedirectingFileSystem::status( - const Twine &Path, const RedirectingFileSystem::LookupResult &Result) { + const Twine &CanonicalPath, const Twine &OriginalPath, + const RedirectingFileSystem::LookupResult &Result) { if (Optional<StringRef> ExtRedirect = Result.getExternalRedirect()) { - ErrorOr<Status> S = ExternalFS->status(*ExtRedirect); + SmallString<256> CanonicalRemappedPath((*ExtRedirect).str()); + if (std::error_code EC = makeCanonical(CanonicalRemappedPath)) + return EC; + + ErrorOr<Status> S = ExternalFS->status(CanonicalRemappedPath); if (!S) return S; + S = Status::copyWithNewName(*S, *ExtRedirect); auto *RE = cast<RedirectingFileSystem::RemapEntry>(Result.E); - return getRedirectedFileStatus(Path, RE->useExternalName(UseExternalNames), - *S); + return getRedirectedFileStatus(OriginalPath, + RE->useExternalName(UseExternalNames), *S); } auto *DE = cast<RedirectingFileSystem::DirectoryEntry>(Result.E); - return Status::copyWithNewName(DE->getStatus(), Path); + return Status::copyWithNewName(DE->getStatus(), CanonicalPath); } -ErrorOr<Status> RedirectingFileSystem::status(const Twine &Path_) { - SmallString<256> Path; - Path_.toVector(Path); +ErrorOr<Status> +RedirectingFileSystem::getExternalStatus(const Twine &CanonicalPath, + const Twine &OriginalPath) const { + if (auto Result = ExternalFS->status(CanonicalPath)) { + return Result.get().copyWithNewName(Result.get(), OriginalPath); + } else { + return Result.getError(); + } +} - if (std::error_code EC = makeCanonical(Path)) +ErrorOr<Status> RedirectingFileSystem::status(const Twine &OriginalPath) { + SmallString<256> CanonicalPath; + OriginalPath.toVector(CanonicalPath); + + if (std::error_code EC = makeCanonical(CanonicalPath)) return EC; - ErrorOr<RedirectingFileSystem::LookupResult> Result = lookupPath(Path); + ErrorOr<RedirectingFileSystem::LookupResult> Result = + lookupPath(CanonicalPath); if (!Result) { - if (shouldFallBackToExternalFS(Result.getError())) - return ExternalFS->status(Path); + if (shouldFallBackToExternalFS(Result.getError())) { + return getExternalStatus(CanonicalPath, OriginalPath); + } return Result.getError(); } - ErrorOr<Status> S = status(Path, *Result); - if (!S && shouldFallBackToExternalFS(S.getError(), Result->E)) - S = ExternalFS->status(Path); + ErrorOr<Status> S = status(CanonicalPath, OriginalPath, *Result); + if (!S && shouldFallBackToExternalFS(S.getError(), Result->E)) { + return getExternalStatus(CanonicalPath, OriginalPath); + } + return S; } @@ -2036,22 +2066,39 @@ class FileWithFixedStatus : public File { } std::error_code close() override { return InnerFile->close(); } + + void setPath(const Twine &Path) override { S = S.copyWithNewName(S, Path); } }; } // namespace ErrorOr<std::unique_ptr<File>> -RedirectingFileSystem::openFileForRead(const Twine &Path_) { - SmallString<256> Path; - Path_.toVector(Path); +File::getWithPath(ErrorOr<std::unique_ptr<File>> Result, const Twine &P) { + if (!Result) + return Result; - if (std::error_code EC = makeCanonical(Path)) + ErrorOr<std::unique_ptr<File>> F = std::move(*Result); + auto Name = F->get()->getName(); + if (Name && Name.get() != P.str()) + F->get()->setPath(P); + return F; +} + +ErrorOr<std::unique_ptr<File>> +RedirectingFileSystem::openFileForRead(const Twine &OriginalPath) { + SmallString<256> CanonicalPath; + OriginalPath.toVector(CanonicalPath); + + if (std::error_code EC = makeCanonical(CanonicalPath)) return EC; - ErrorOr<RedirectingFileSystem::LookupResult> Result = lookupPath(Path); + ErrorOr<RedirectingFileSystem::LookupResult> Result = + lookupPath(CanonicalPath); if (!Result) { if (shouldFallBackToExternalFS(Result.getError())) - return ExternalFS->openFileForRead(Path); + return File::getWithPath(ExternalFS->openFileForRead(CanonicalPath), + OriginalPath); + return Result.getError(); } @@ -2059,12 +2106,18 @@ RedirectingFileSystem::openFileForRead(const Twine &Path_) { return make_error_code(llvm::errc::invalid_argument); StringRef ExtRedirect = *Result->getExternalRedirect(); + SmallString<256> CanonicalRemappedPath(ExtRedirect.str()); + if (std::error_code EC = makeCanonical(CanonicalRemappedPath)) + return EC; + auto *RE = cast<RedirectingFileSystem::RemapEntry>(Result->E); - auto ExternalFile = ExternalFS->openFileForRead(ExtRedirect); + auto ExternalFile = File::getWithPath( + ExternalFS->openFileForRead(CanonicalRemappedPath), ExtRedirect); if (!ExternalFile) { if (shouldFallBackToExternalFS(ExternalFile.getError(), Result->E)) - return ExternalFS->openFileForRead(Path); + return File::getWithPath(ExternalFS->openFileForRead(CanonicalPath), + OriginalPath); return ExternalFile; } @@ -2074,7 +2127,7 @@ RedirectingFileSystem::openFileForRead(const Twine &Path_) { // FIXME: Update the status with the name and VFSMapped. Status S = getRedirectedFileStatus( - Path, RE->useExternalName(UseExternalNames), *ExternalStatus); + OriginalPath, RE->useExternalName(UseExternalNames), *ExternalStatus); return std::unique_ptr<File>( std::make_unique<FileWithFixedStatus>(std::move(*ExternalFile), S)); } diff --git a/llvm/unittests/Support/VirtualFileSystemTest.cpp b/llvm/unittests/Support/VirtualFileSystemTest.cpp index ca35b5e9f8baa..be32971908ea3 100644 --- a/llvm/unittests/Support/VirtualFileSystemTest.cpp +++ b/llvm/unittests/Support/VirtualFileSystemTest.cpp @@ -1627,6 +1627,114 @@ TEST_F(VFSFromYAMLTest, RemappedDirectoryOverlayNoFallthrough) { EXPECT_EQ(0, NumDiagnostics); } +TEST_F(VFSFromYAMLTest, ReturnsRequestedPathVFSMiss) { + IntrusiveRefCntPtr<vfs::InMemoryFileSystem> BaseFS( + new vfs::InMemoryFileSystem); + BaseFS->addFile("//root/foo/a", 0, + MemoryBuffer::getMemBuffer("contents of a")); + ASSERT_FALSE(BaseFS->setCurrentWorkingDirectory("//root/foo")); + auto RemappedFS = vfs::RedirectingFileSystem::create( + {}, /*UseExternalNames=*/false, *BaseFS); + + auto OpenedF = RemappedFS->openFileForRead("a"); + ASSERT_FALSE(OpenedF.getError()); + llvm::ErrorOr<std::string> Name = (*OpenedF)->getName(); + ASSERT_FALSE(Name.getError()); + EXPECT_EQ("a", Name.get()); + + auto OpenedS = (*OpenedF)->status(); + ASSERT_FALSE(OpenedS.getError()); + EXPECT_EQ("a", OpenedS->getName()); + EXPECT_FALSE(OpenedS->IsVFSMapped); + + auto DirectS = RemappedFS->status("a"); + ASSERT_FALSE(DirectS.getError()); + EXPECT_EQ("a", DirectS->getName()); + EXPECT_FALSE(DirectS->IsVFSMapped); + + EXPECT_EQ(0, NumDiagnostics); +} + +TEST_F(VFSFromYAMLTest, ReturnsExternalPathVFSHit) { + IntrusiveRefCntPtr<vfs::InMemoryFileSystem> BaseFS( + new vfs::InMemoryFileSystem); + BaseFS->addFile("//root/foo/realname", 0, + MemoryBuffer::getMemBuffer("contents of a")); + auto FS = + getFromYAMLString("{ 'use-external-names': true,\n" + " 'roots': [\n" + "{\n" + " 'type': 'directory',\n" + " 'name': '//root/foo',\n" + " 'contents': [ {\n" + " 'type': 'file',\n" + " 'name': 'vfsname',\n" + " 'external-contents': 'realname'\n" + " }\n" + " ]\n" + "}]}", + BaseFS); + ASSERT_FALSE(FS->setCurrentWorkingDirectory("//root/foo")); + + auto OpenedF = FS->openFileForRead("vfsname"); + ASSERT_FALSE(OpenedF.getError()); + llvm::ErrorOr<std::string> Name = (*OpenedF)->getName(); + ASSERT_FALSE(Name.getError()); + EXPECT_EQ("realname", Name.get()); + + auto OpenedS = (*OpenedF)->status(); + ASSERT_FALSE(OpenedS.getError()); + EXPECT_EQ("realname", OpenedS->getName()); + EXPECT_TRUE(OpenedS->IsVFSMapped); + + auto DirectS = FS->status("vfsname"); + ASSERT_FALSE(DirectS.getError()); + EXPECT_EQ("realname", DirectS->getName()); + EXPECT_TRUE(DirectS->IsVFSMapped); + + EXPECT_EQ(0, NumDiagnostics); +} + +TEST_F(VFSFromYAMLTest, ReturnsInternalPathVFSHit) { + IntrusiveRefCntPtr<vfs::InMemoryFileSystem> BaseFS( + new vfs::InMemoryFileSystem); + BaseFS->addFile("//root/foo/realname", 0, + MemoryBuffer::getMemBuffer("contents of a")); + auto FS = + getFromYAMLString("{ 'use-external-names': false,\n" + " 'roots': [\n" + "{\n" + " 'type': 'directory',\n" + " 'name': '//root/foo',\n" + " 'contents': [ {\n" + " 'type': 'file',\n" + " 'name': 'vfsname',\n" + " 'external-contents': 'realname'\n" + " }\n" + " ]\n" + "}]}", + BaseFS); + ASSERT_FALSE(FS->setCurrentWorkingDirectory("//root/foo")); + + auto OpenedF = FS->openFileForRead("vfsname"); + ASSERT_FALSE(OpenedF.getError()); + llvm::ErrorOr<std::string> Name = (*OpenedF)->getName(); + ASSERT_FALSE(Name.getError()); + EXPECT_EQ("vfsname", Name.get()); + + auto OpenedS = (*OpenedF)->status(); + ASSERT_FALSE(OpenedS.getError()); + EXPECT_EQ("vfsname", OpenedS->getName()); + EXPECT_TRUE(OpenedS->IsVFSMapped); + + auto DirectS = FS->status("vfsname"); + ASSERT_FALSE(DirectS.getError()); + EXPECT_EQ("vfsname", DirectS->getName()); + EXPECT_TRUE(DirectS->IsVFSMapped); + + EXPECT_EQ(0, NumDiagnostics); +} + TEST_F(VFSFromYAMLTest, CaseInsensitive) { IntrusiveRefCntPtr<DummyFileSystem> Lower(new DummyFileSystem()); Lower->addRegularFile("//root/foo/bar/a"); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
