[PATCH] D52620: Added Support for StatOnly Files in VFS.
ilya-biryukov added a comment. > I would call it a requirement instead of an assumption. The replay must be > exactly the same (even the file stats and reads). If Clang reads the file in > replay which was only stat()ed during compilation, it indicates > non-determinism or something wrong (in clang or FS). > We currently deal with such files by adding empty buffers for them based on > this assumption/requirement only. Does adding a flag to clang to skip the module inputs checks seems feasible/reasonable? +1 to Sam's comments, the mode for FS that has the file sizes, but not the file contents seems too specific for the use-case at hand, does not fit well into the design. Repository: rC Clang https://reviews.llvm.org/D52620 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52620: Added Support for StatOnly Files in VFS.
usaxena95 added inline comments.
Comment at: lib/Basic/VirtualFileSystem.cpp:2097
void YAMLVFSWriter::write(llvm::raw_ostream ) {
- llvm::sort(Mappings, [](const YAMLVFSEntry , const YAMLVFSEntry ) {
+ llvm::sort(Mappings.begin(), Mappings.end(),
+ [](const YAMLVFSEntry , const YAMLVFSEntry ) {
kbobyrev wrote:
> This used `llvm::sort(Container &, Compare Comp)` before, I would think
> that the range-based API is the preferred one. There's also a cleanup patch
> migrating STL-like calls to idiomatic LLVM's STL Extension API: D52576.
I think these are some very recent changes and does not belong to my patch.
Will sync these. Thanks for pointing out.
Repository:
rC Clang
https://reviews.llvm.org/D52620
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52620: Added Support for StatOnly Files in VFS.
usaxena95 added a comment. > it sounds like this is for record-replay. Yes this is for record-replay purpose. > What are these files in practice? During loading a modules the files that were required to make the module are stat()ed and their sizes are used to verify that the module is still valid. There are occurrences of more stat() only files (apart from the use in modules) which I am not aware of. > In the replayed compilation, we assume the accessed files are the same, is it > safe to assume we never read files we previously stat()ed? I would call it a requirement instead of an assumption. The replay must be exactly the same (even the file stats and reads). If Clang reads the file in replay which was only stat()ed during compilation, it indicates non-determinism or something wrong (in clang or FS). We currently deal with such files by adding empty buffers for them based on this assumption/requirement only. > it seems like this could also be achieved with an overlayFS adding a simple > specialized FS that only provides the stat-only files. If this is a > relatively niche feature, cramming it into InMemoryFileSystem may not be the > best option. Any thoughts on the tradeoff here? We can reuse the InMemoryFS to make another FS which just serves stat-only files. A tradeoff I can think of is: we might need to store a random buffer of required size to make final status of the file correct. Repository: rC Clang https://reviews.llvm.org/D52620 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52620: Added Support for StatOnly Files in VFS.
kbobyrev added inline comments.
Comment at: lib/Basic/VirtualFileSystem.cpp:2097
void YAMLVFSWriter::write(llvm::raw_ostream ) {
- llvm::sort(Mappings, [](const YAMLVFSEntry , const YAMLVFSEntry ) {
+ llvm::sort(Mappings.begin(), Mappings.end(),
+ [](const YAMLVFSEntry , const YAMLVFSEntry ) {
This used `llvm::sort(Container &, Compare Comp)` before, I would think that
the range-based API is the preferred one. There's also a cleanup patch
migrating STL-like calls to idiomatic LLVM's STL Extension API: D52576.
Repository:
rC Clang
https://reviews.llvm.org/D52620
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52620: Added Support for StatOnly Files in VFS.
sammccall added a comment. Could you give some more detail about what this is for? - it sounds like this is for record-replay. In the replayed compilation, we assume the accessed files are the same, is it safe to assume we never read files we previously stat()ed? What are these files in practice? - it seems like this could also be achieved with an overlayFS adding a simple specialized FS that only provides the stat-only files. If this is a relatively niche feature, cramming it into InMemoryFileSystem may not be the best option. Any thoughts on the tradeoff here? Comment at: include/clang/Basic/VirtualFileSystem.h:414 + // Add a file with the given size but with no contents. The buffer of this + // file must never be requested. + /// \return true if the file successfully added, false if the file already Please add some motivation here, e.g. "such files are useful when replaying a recorded compilation, if the original compilation never read the file contents" Comment at: lib/Basic/VirtualFileSystem.cpp:495 std::unique_ptr Buffer; + bool StatOnlyFile; can't you just represent this by a null buffer? Repository: rC Clang https://reviews.llvm.org/D52620 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52620: Added Support for StatOnly Files in VFS.
usaxena95 created this revision.
Herald added subscribers: cfe-commits, mgrang.
Some files are only Statted by Clang and not read. Clang mostly uses
them for checking the existence of some files and in rare cases uses the
value of the Status to proceed further (for example while loading
module files, it checks the sizes of some files).
This change adds support to represent files that are supposed to be only
statted by Clang.
Repository:
rC Clang
https://reviews.llvm.org/D52620
Files:
include/clang/Basic/VirtualFileSystem.h
lib/Basic/VirtualFileSystem.cpp
unittests/Basic/VirtualFileSystemTest.cpp
Index: unittests/Basic/VirtualFileSystemTest.cpp
===
--- unittests/Basic/VirtualFileSystemTest.cpp
+++ unittests/Basic/VirtualFileSystemTest.cpp
@@ -553,8 +553,8 @@
for (DirIter E; !EC && I != E; I.increment(EC))
InputToCheck.push_back(I->path());
- llvm::sort(InputToCheck);
- llvm::sort(Expected);
+ llvm::sort(InputToCheck.begin(), InputToCheck.end());
+ llvm::sort(Expected.begin(), Expected.end());
EXPECT_EQ(InputToCheck.size(), Expected.size());
unsigned LastElt = std::min(InputToCheck.size(), Expected.size());
@@ -1063,15 +1063,50 @@
TEST_F(InMemoryFileSystemTest, RecursiveIterationWithHardLink) {
std::error_code EC;
FS.addFile("/a/b", 0, MemoryBuffer::getMemBuffer("content string"));
+ FS.addStatOnlyFile("/a/d", 0, 10);
EXPECT_TRUE(FS.addHardLink("/c/d", "/a/b"));
auto I = vfs::recursive_directory_iterator(FS, "/", EC);
ASSERT_FALSE(EC);
std::vector Nodes;
for (auto E = vfs::recursive_directory_iterator(); !EC && I != E;
I.increment(EC)) {
Nodes.push_back(getPosixPath(I->path()));
}
- EXPECT_THAT(Nodes, testing::UnorderedElementsAre("/a", "/a/b", "/c", "/c/d"));
+ EXPECT_THAT(Nodes, testing::UnorderedElementsAre("/a", "/a/d", "/a/b",
+ "/c", "/c/d"));
+}
+
+TEST_F(InMemoryFileSystemTest, AddStatOnlyFileWithCorrectSize) {
+ FS.addStatOnlyFile("/a/b", 0, 10);
+ auto Stat = FS.status("/a/b");
+ EXPECT_EQ(Stat.get().getSize(), 10);
+}
+
+TEST_F(InMemoryFileSystemTest, AddMultipleFilesUnderStatOnlyFile) {
+ FS.addStatOnlyFile("/a/b", 0, 10);
+ EXPECT_FALSE(FS.addStatOnlyFile("/a/b", 0, 11));
+ EXPECT_TRUE(FS.addStatOnlyFile("/a/b", 0, 10));
+ EXPECT_FALSE(FS.addFile("/a/b", 0, MemoryBuffer::getMemBuffer("content")));
+}
+
+TEST_F(InMemoryFileSystemTest, DeathWhenBufferRequestedForStatOnlyFile) {
+ FS.addStatOnlyFile("/a/b", 0, 10);
+ EXPECT_DEATH(FS.getBufferForFile("/a/b"),
+ "Cannot get buffer for StatOnlyFile");
+}
+
+TEST_F(InMemoryFileSystemTest, AddHardLinksToStatOnlyFile) {
+ FS.addStatOnlyFile("/target", 0, 10);
+ FS.addHardLink("/link1", "/target");
+ FS.addHardLink("/link2", "/link1");
+ EXPECT_THAT("/link1", IsHardLinkTo(, "/target"));
+ EXPECT_THAT("/link2", IsHardLinkTo(, "/target"));
+}
+
+TEST_F(InMemoryFileSystemTest, AddFileUnderHardLinkToStatOnlyFile) {
+ FS.addStatOnlyFile("/target", 0, 10);
+ FS.addHardLink("/link", "/target");
+ EXPECT_FALSE(FS.addFile("/link", 0, MemoryBuffer::getMemBuffer("content")));
}
// NOTE: in the tests below, we use '//root/' as our root directory, since it is
Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -492,20 +492,26 @@
class InMemoryFile : public InMemoryNode {
Status Stat;
std::unique_ptr Buffer;
+ bool StatOnlyFile;
public:
- InMemoryFile(Status Stat, std::unique_ptr Buffer)
+ InMemoryFile(Status Stat, std::unique_ptr Buffer,
+ bool StatOnlyFile)
: InMemoryNode(Stat.getName(), IME_File), Stat(std::move(Stat)),
-Buffer(std::move(Buffer)) {}
+Buffer(std::move(Buffer)), StatOnlyFile(StatOnlyFile) {}
/// 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);
}
- llvm::MemoryBuffer *getBuffer() const { return Buffer.get(); }
+ llvm::MemoryBuffer *getBuffer() const {
+assert(!StatOnlyFile && "Cannot get buffer for StatOnlyFile");
+return Buffer.get();
+ }
+ bool IsStatOnlyFile() const { return StatOnlyFile; }
std::string toString(unsigned Indent) const override {
return (std::string(Indent, ' ') + Stat.getName() + "\n").str();
}
@@ -640,7 +646,8 @@
Optional Group,
Optional Type,
Optional Perms,
- const detail::InMemoryFile *HardLinkTarget) {
+ const detail::InMemoryFile *HardLinkTarget,
+
