[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
This revision was automatically updated to reflect the committed changes.
Closed by commit rC361226: [Preamble] Reuse preamble even if an unsaved file
does not exist (authored by nik, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D41005?vs=198809&id=200415#toc
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D41005/new/
https://reviews.llvm.org/D41005
Files:
include/clang/Frontend/ASTUnit.h
lib/Frontend/ASTUnit.cpp
lib/Frontend/PrecompiledPreamble.cpp
unittests/Frontend/PCHPreambleTest.cpp
Index: lib/Frontend/ASTUnit.cpp
===
--- lib/Frontend/ASTUnit.cpp
+++ lib/Frontend/ASTUnit.cpp
@@ -1304,22 +1304,22 @@
PreambleInvocationIn.getDiagnosticOpts());
getDiagnostics().setNumWarnings(NumWarningsInPreamble);
- PreambleRebuildCounter = 1;
+ PreambleRebuildCountdown = 1;
return MainFileBuffer;
} else {
Preamble.reset();
PreambleDiagnostics.clear();
TopLevelDeclsInPreamble.clear();
PreambleSrcLocCache.clear();
- PreambleRebuildCounter = 1;
+ PreambleRebuildCountdown = 1;
}
}
// If the preamble rebuild counter > 1, it's because we previously
// failed to build a preamble and we're not yet ready to try
// again. Decrement the counter and return a failure.
- if (PreambleRebuildCounter > 1) {
---PreambleRebuildCounter;
+ if (PreambleRebuildCountdown > 1) {
+--PreambleRebuildCountdown;
return nullptr;
}
@@ -1329,6 +1329,8 @@
if (!AllowRebuild)
return nullptr;
+ ++PreambleCounter;
+
SmallVector NewPreambleDiagsStandalone;
SmallVector NewPreambleDiags;
ASTUnitPreambleCallbacks Callbacks;
@@ -1356,18 +1358,18 @@
if (NewPreamble) {
Preamble = std::move(*NewPreamble);
- PreambleRebuildCounter = 1;
+ PreambleRebuildCountdown = 1;
} else {
switch (static_cast(NewPreamble.getError().value())) {
case BuildPreambleError::CouldntCreateTempFile:
// Try again next time.
-PreambleRebuildCounter = 1;
+PreambleRebuildCountdown = 1;
return nullptr;
case BuildPreambleError::CouldntCreateTargetInfo:
case BuildPreambleError::BeginSourceFileFailed:
case BuildPreambleError::CouldntEmitPCH:
// These erros are more likely to repeat, retry after some period.
-PreambleRebuildCounter = DefaultPreambleRebuildInterval;
+PreambleRebuildCountdown = DefaultPreambleRebuildInterval;
return nullptr;
}
llvm_unreachable("unexpected BuildPreambleError");
@@ -1507,7 +1509,7 @@
AST->OnlyLocalDecls = OnlyLocalDecls;
AST->CaptureDiagnostics = CaptureDiagnostics;
if (PrecompilePreambleAfterNParses > 0)
-AST->PreambleRebuildCounter = PrecompilePreambleAfterNParses;
+AST->PreambleRebuildCountdown = PrecompilePreambleAfterNParses;
AST->TUKind = Action ? Action->getTranslationUnitKind() : TU_Complete;
AST->ShouldCacheCodeCompletionResults = CacheCodeCompletionResults;
AST->IncludeBriefCommentsInCodeCompletion
@@ -1641,7 +1643,7 @@
std::unique_ptr OverrideMainBuffer;
if (PrecompilePreambleAfterNParses > 0) {
-PreambleRebuildCounter = PrecompilePreambleAfterNParses;
+PreambleRebuildCountdown = PrecompilePreambleAfterNParses;
OverrideMainBuffer =
getMainBufferWithPrecompiledPreamble(PCHContainerOps, *Invocation, VFS);
getDiagnostics().Reset();
@@ -1819,7 +1821,7 @@
// If we have a preamble file lying around, or if we might try to
// build a precompiled preamble, do so now.
std::unique_ptr OverrideMainBuffer;
- if (Preamble || PreambleRebuildCounter > 0)
+ if (Preamble || PreambleRebuildCountdown > 0)
OverrideMainBuffer =
getMainBufferWithPrecompiledPreamble(PCHContainerOps, *Invocation, VFS);
Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -454,20 +454,33 @@
Status.getSize(), llvm::sys::toTimeT(Status.getLastModificationTime()));
}
+ // OverridenFileBuffers tracks only the files not found in VFS.
+ llvm::StringMap OverridenFileBuffers;
for (const auto &RB : PreprocessorOpts.RemappedFileBuffers) {
-llvm::vfs::Status Status;
-if (!moveOnNoError(VFS->status(RB.first), Status))
- return false;
-
-OverriddenFiles[Status.getUniqueID()] =
+const PrecompiledPreamble::PreambleFileHash PreambleHash =
PreambleFileHash::createForMemoryBuffer(RB.second);
+llvm::vfs::Status Status;
+if (moveOnNoError(VFS->status(RB.first), Status))
+ OverriddenFiles[Status.getUniqueID()] = PreambleHash;
+else
+ OverridenFileBuffers[RB.first] = PreambleHash;
}
// Check whether anything has changed.
for (const auto &F : FilesInPreamble) {
+auto OverridenFileBuff
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. Thanks! LGTM Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D41005/new/ https://reviews.llvm.org/D41005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
nik updated this revision to Diff 198809.
nik added a comment.
Addressed inline comments.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D41005/new/
https://reviews.llvm.org/D41005
Files:
include/clang/Frontend/ASTUnit.h
lib/Frontend/ASTUnit.cpp
lib/Frontend/PrecompiledPreamble.cpp
unittests/Frontend/PCHPreambleTest.cpp
Index: unittests/Frontend/PCHPreambleTest.cpp
===
--- unittests/Frontend/PCHPreambleTest.cpp
+++ unittests/Frontend/PCHPreambleTest.cpp
@@ -52,7 +52,10 @@
FileSystemOptions FSOpts;
public:
- void SetUp() override {
+ void SetUp() override { ResetVFS(); }
+ void TearDown() override {}
+
+ void ResetVFS() {
VFS = new ReadCountingInMemoryFileSystem();
// We need the working directory to be set to something absolute,
// otherwise it ends up being inadvertently set to the current
@@ -63,9 +66,6 @@
VFS->setCurrentWorkingDirectory("//./");
}
- void TearDown() override {
- }
-
void AddFile(const std::string &Filename, const std::string &Contents) {
::time_t now;
::time(&now);
@@ -123,6 +123,72 @@
}
};
+TEST_F(PCHPreambleTest, ReparseReusesPreambleWithUnsavedFileNotExistingOnDisk) {
+ std::string Header1 = "//./header1.h";
+ std::string MainName = "//./main.cpp";
+ AddFile(MainName, R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp");
+ RemapFile(Header1, "#define ZERO 0\n");
+
+ // Parse with header file provided as unsaved file, which does not exist on
+ // disk.
+ std::unique_ptr AST(ParseAST(MainName));
+ ASSERT_TRUE(AST.get());
+ ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+ // Reparse and check that the preamble was reused.
+ ASSERT_TRUE(ReparseAST(AST));
+ ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+}
+
+TEST_F(PCHPreambleTest, ReparseReusesPreambleAfterUnsavedFileWasCreatedOnDisk) {
+ std::string Header1 = "//./header1.h";
+ std::string MainName = "//./main.cpp";
+ AddFile(MainName, R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp");
+ RemapFile(Header1, "#define ZERO 0\n");
+
+ // Parse with header file provided as unsaved file, which does not exist on
+ // disk.
+ std::unique_ptr AST(ParseAST(MainName));
+ ASSERT_TRUE(AST.get());
+ ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+ // Create the unsaved file also on disk and check that preamble was reused.
+ AddFile(Header1, "#define ZERO 0\n");
+ ASSERT_TRUE(ReparseAST(AST));
+ ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+}
+
+TEST_F(PCHPreambleTest,
+ ReparseReusesPreambleAfterUnsavedFileWasRemovedFromDisk) {
+ std::string Header1 = "//./foo/header1.h";
+ std::string MainName = "//./main.cpp";
+ std::string MainFileContent = R"cpp(
+#include "//./foo/header1.h"
+int main() { return ZERO; }
+)cpp";
+ AddFile(MainName, MainFileContent);
+ AddFile(Header1, "#define ZERO 0\n");
+ RemapFile(Header1, "#define ZERO 0\n");
+
+ // Parse with header file provided as unsaved file, which exists on disk.
+ std::unique_ptr AST(ParseAST(MainName));
+ ASSERT_TRUE(AST.get());
+ ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+ ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+
+ // Remove the unsaved file from disk and check that the preamble was reused.
+ ResetVFS();
+ AddFile(MainName, MainFileContent);
+ ASSERT_TRUE(ReparseAST(AST));
+ ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+}
+
TEST_F(PCHPreambleTest, ReparseWithOverriddenFileDoesNotInvalidatePreamble) {
std::string Header1 = "//./header1.h";
std::string Header2 = "//./header2.h";
Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -454,20 +454,33 @@
Status.getSize(), llvm::sys::toTimeT(Status.getLastModificationTime()));
}
+ // OverridenFileBuffers tracks only the files not found in VFS.
+ llvm::StringMap OverridenFileBuffers;
for (const auto &RB : PreprocessorOpts.RemappedFileBuffers) {
-llvm::vfs::Status Status;
-if (!moveOnNoError(VFS->status(RB.first), Status))
- return false;
-
-OverriddenFiles[Status.getUniqueID()] =
+const PrecompiledPreamble::PreambleFileHash PreambleHash =
PreambleFileHash::createForMemoryBuffer(RB.second);
+llvm::vfs::Status Status;
+if (moveOnNoError(VFS->status(RB.first), Status))
+ OverriddenFiles[Status.getUniqueID()] = PreambleHash;
+else
+ OverridenFileBuffers[RB.first] = PreambleHash;
}
// Check whether anything has changed.
for (const auto &F : FilesInPreamble) {
+auto OverridenFileBuffer = OverridenFileBuffers.find(F.first());
+if (OverridenFileBuffer != OverridenFileBuffers.end()) {
+ // The file's buffer was remapped and the file was not found in VFS.
+ // Check whether it matches up with the previous mapping.
+
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
ilya-biryukov added a comment.
Again, sorry for the delay. This looks good, just a few NITs from me before I
stamp it
Comment at: lib/Frontend/PrecompiledPreamble.cpp:457
+ llvm::StringMap OverridenFileBuffers;
for (const auto &RB : PreprocessorOpts.RemappedFileBuffers) {
Could you add a comment that this contains only the files there were not found
on disk (i.e. the vfs call failed and we couldn't get a `UniqueID`)
Comment at: lib/Frontend/PrecompiledPreamble.cpp:472
+if (OverridenFileBuffer != OverridenFileBuffers.end()) {
+ // The file's buffer was remapped; check whether it matches up
+ // with the previous mapping.
NIT: change to: The file's buffer was remapped **and the file was not found in
VFS**
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D41005/new/
https://reviews.llvm.org/D41005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
nik updated this revision to Diff 198656.
nik added a comment.
Herald added a subscriber: dexonsmith.
Minor diff update fixing indentation and removing not needed include.
Ping.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D41005/new/
https://reviews.llvm.org/D41005
Files:
include/clang/Frontend/ASTUnit.h
lib/Frontend/ASTUnit.cpp
lib/Frontend/PrecompiledPreamble.cpp
unittests/Frontend/PCHPreambleTest.cpp
Index: unittests/Frontend/PCHPreambleTest.cpp
===
--- unittests/Frontend/PCHPreambleTest.cpp
+++ unittests/Frontend/PCHPreambleTest.cpp
@@ -52,7 +52,10 @@
FileSystemOptions FSOpts;
public:
- void SetUp() override {
+ void SetUp() override { ResetVFS(); }
+ void TearDown() override {}
+
+ void ResetVFS() {
VFS = new ReadCountingInMemoryFileSystem();
// We need the working directory to be set to something absolute,
// otherwise it ends up being inadvertently set to the current
@@ -63,9 +66,6 @@
VFS->setCurrentWorkingDirectory("//./");
}
- void TearDown() override {
- }
-
void AddFile(const std::string &Filename, const std::string &Contents) {
::time_t now;
::time(&now);
@@ -123,6 +123,72 @@
}
};
+TEST_F(PCHPreambleTest, ReparseReusesPreambleWithUnsavedFileNotExistingOnDisk) {
+ std::string Header1 = "//./header1.h";
+ std::string MainName = "//./main.cpp";
+ AddFile(MainName, R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp");
+ RemapFile(Header1, "#define ZERO 0\n");
+
+ // Parse with header file provided as unsaved file, which does not exist on
+ // disk.
+ std::unique_ptr AST(ParseAST(MainName));
+ ASSERT_TRUE(AST.get());
+ ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+ // Reparse and check that the preamble was reused.
+ ASSERT_TRUE(ReparseAST(AST));
+ ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+}
+
+TEST_F(PCHPreambleTest, ReparseReusesPreambleAfterUnsavedFileWasCreatedOnDisk) {
+ std::string Header1 = "//./header1.h";
+ std::string MainName = "//./main.cpp";
+ AddFile(MainName, R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp");
+ RemapFile(Header1, "#define ZERO 0\n");
+
+ // Parse with header file provided as unsaved file, which does not exist on
+ // disk.
+ std::unique_ptr AST(ParseAST(MainName));
+ ASSERT_TRUE(AST.get());
+ ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+ // Create the unsaved file also on disk and check that preamble was reused.
+ AddFile(Header1, "#define ZERO 0\n");
+ ASSERT_TRUE(ReparseAST(AST));
+ ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+}
+
+TEST_F(PCHPreambleTest,
+ ReparseReusesPreambleAfterUnsavedFileWasRemovedFromDisk) {
+ std::string Header1 = "//./foo/header1.h";
+ std::string MainName = "//./main.cpp";
+ std::string MainFileContent = R"cpp(
+#include "//./foo/header1.h"
+int main() { return ZERO; }
+)cpp";
+ AddFile(MainName, MainFileContent);
+ AddFile(Header1, "#define ZERO 0\n");
+ RemapFile(Header1, "#define ZERO 0\n");
+
+ // Parse with header file provided as unsaved file, which exists on disk.
+ std::unique_ptr AST(ParseAST(MainName));
+ ASSERT_TRUE(AST.get());
+ ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+ ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+
+ // Remove the unsaved file from disk and check that the preamble was reused.
+ ResetVFS();
+ AddFile(MainName, MainFileContent);
+ ASSERT_TRUE(ReparseAST(AST));
+ ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+}
+
TEST_F(PCHPreambleTest, ReparseWithOverriddenFileDoesNotInvalidatePreamble) {
std::string Header1 = "//./header1.h";
std::string Header2 = "//./header2.h";
Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -454,20 +454,32 @@
Status.getSize(), llvm::sys::toTimeT(Status.getLastModificationTime()));
}
+ llvm::StringMap OverridenFileBuffers;
for (const auto &RB : PreprocessorOpts.RemappedFileBuffers) {
-llvm::vfs::Status Status;
-if (!moveOnNoError(VFS->status(RB.first), Status))
- return false;
-
-OverriddenFiles[Status.getUniqueID()] =
+const PrecompiledPreamble::PreambleFileHash PreambleHash =
PreambleFileHash::createForMemoryBuffer(RB.second);
+llvm::vfs::Status Status;
+if (moveOnNoError(VFS->status(RB.first), Status))
+ OverriddenFiles[Status.getUniqueID()] = PreambleHash;
+else
+ OverridenFileBuffers[RB.first] = PreambleHash;
}
// Check whether anything has changed.
for (const auto &F : FilesInPreamble) {
+auto OverridenFileBuffer = OverridenFileBuffers.find(F.first());
+if (OverridenFileBuffer != OverridenFileBuffers.end()) {
+ // The file's buffer was remapped; check whether it matches up
+ // with the previous mapping.
+ if (Overrid
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
nik added a comment. Ping. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D41005/new/ https://reviews.llvm.org/D41005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
nik added a comment. Ping. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D41005/new/ https://reviews.llvm.org/D41005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
nik updated this revision to Diff 186436.
nik added a comment.
> Meh, something changed in the meanwhile.
> ReparseReusesPreambleAfterUnsavedFileWasRemovedFromDisk fails now. Looking
> into it.
No, it's just me ;) I've referenced the header file wrong.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D41005/new/
https://reviews.llvm.org/D41005
Files:
include/clang/Frontend/ASTUnit.h
lib/Frontend/ASTUnit.cpp
lib/Frontend/PrecompiledPreamble.cpp
unittests/Frontend/PCHPreambleTest.cpp
Index: unittests/Frontend/PCHPreambleTest.cpp
===
--- unittests/Frontend/PCHPreambleTest.cpp
+++ unittests/Frontend/PCHPreambleTest.cpp
@@ -52,7 +52,10 @@
FileSystemOptions FSOpts;
public:
- void SetUp() override {
+ void SetUp() override { ResetVFS(); }
+ void TearDown() override {}
+
+ void ResetVFS() {
VFS = new ReadCountingInMemoryFileSystem();
// We need the working directory to be set to something absolute,
// otherwise it ends up being inadvertently set to the current
@@ -63,9 +66,6 @@
VFS->setCurrentWorkingDirectory("//./");
}
- void TearDown() override {
- }
-
void AddFile(const std::string &Filename, const std::string &Contents) {
::time_t now;
::time(&now);
@@ -123,6 +123,72 @@
}
};
+TEST_F(PCHPreambleTest, ReparseReusesPreambleWithUnsavedFileNotExistingOnDisk) {
+ std::string Header1 = "//./header1.h";
+ std::string MainName = "//./main.cpp";
+ AddFile(MainName, R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp");
+ RemapFile(Header1, "#define ZERO 0\n");
+
+ // Parse with header file provided as unsaved file, which does not exist on
+ // disk.
+ std::unique_ptr AST(ParseAST(MainName));
+ ASSERT_TRUE(AST.get());
+ ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+ // Reparse and check that the preamble was reused.
+ ASSERT_TRUE(ReparseAST(AST));
+ ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+}
+
+TEST_F(PCHPreambleTest, ReparseReusesPreambleAfterUnsavedFileWasCreatedOnDisk) {
+ std::string Header1 = "//./header1.h";
+ std::string MainName = "//./main.cpp";
+ AddFile(MainName, R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp");
+ RemapFile(Header1, "#define ZERO 0\n");
+
+ // Parse with header file provided as unsaved file, which does not exist on
+ // disk.
+ std::unique_ptr AST(ParseAST(MainName));
+ ASSERT_TRUE(AST.get());
+ ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+ // Create the unsaved file also on disk and check that preamble was reused.
+ AddFile(Header1, "#define ZERO 0\n");
+ ASSERT_TRUE(ReparseAST(AST));
+ ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+}
+
+TEST_F(PCHPreambleTest,
+ ReparseReusesPreambleAfterUnsavedFileWasRemovedFromDisk) {
+ std::string Header1 = "//./foo/header1.h";
+ std::string MainName = "//./main.cpp";
+ std::string MainFileContent = R"cpp(
+#include "//./foo/header1.h"
+int main() { return ZERO; }
+)cpp";
+ AddFile(MainName, MainFileContent);
+ AddFile(Header1, "#define ZERO 0\n");
+ RemapFile(Header1, "#define ZERO 0\n");
+
+ // Parse with header file provided as unsaved file, which exists on disk.
+ std::unique_ptr AST(ParseAST(MainName));
+ ASSERT_TRUE(AST.get());
+ ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+ ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+
+ // Remove the unsaved file from disk and check that the preamble was reused.
+ ResetVFS();
+ AddFile(MainName, MainFileContent);
+ ASSERT_TRUE(ReparseAST(AST));
+ ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+}
+
TEST_F(PCHPreambleTest, ReparseWithOverriddenFileDoesNotInvalidatePreamble) {
std::string Header1 = "//./header1.h";
std::string Header2 = "//./header2.h";
Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -28,6 +28,7 @@
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Mutex.h"
#include "llvm/Support/MutexGuard.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/Process.h"
#include "llvm/Support/VirtualFileSystem.h"
#include
@@ -451,20 +452,32 @@
Status.getSize(), llvm::sys::toTimeT(Status.getLastModificationTime()));
}
+ llvm::StringMap OverridenFileBuffers;
for (const auto &RB : PreprocessorOpts.RemappedFileBuffers) {
-llvm::vfs::Status Status;
-if (!moveOnNoError(VFS->status(RB.first), Status))
- return false;
-
-OverriddenFiles[Status.getUniqueID()] =
+const PrecompiledPreamble::PreambleFileHash PreambleHash =
PreambleFileHash::createForMemoryBuffer(RB.second);
+llvm::vfs::Status Status;
+if (moveOnNoError(VFS->status(RB.first), Status))
+ OverriddenFiles[Status.getUniqueID()] = PreambleHash;
+else
+ OverridenFileBuffers[RB.first] = PreambleHash;
}
// Check whe
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
nik added a comment. Meh, something changed in the meanwhile. ReparseReusesPreambleAfterUnsavedFileWasRemovedFromDisk fails now. Looking into it. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D41005/new/ https://reviews.llvm.org/D41005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
nik updated this revision to Diff 186429.
nik marked an inline comment as done.
nik added a comment.
Herald added a subscriber: jdoerfert.
Herald added a project: clang.
Addressed comment.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D41005/new/
https://reviews.llvm.org/D41005
Files:
include/clang/Frontend/ASTUnit.h
lib/Frontend/ASTUnit.cpp
lib/Frontend/PrecompiledPreamble.cpp
unittests/Frontend/PCHPreambleTest.cpp
Index: unittests/Frontend/PCHPreambleTest.cpp
===
--- unittests/Frontend/PCHPreambleTest.cpp
+++ unittests/Frontend/PCHPreambleTest.cpp
@@ -52,7 +52,10 @@
FileSystemOptions FSOpts;
public:
- void SetUp() override {
+ void SetUp() override { ResetVFS(); }
+ void TearDown() override {}
+
+ void ResetVFS() {
VFS = new ReadCountingInMemoryFileSystem();
// We need the working directory to be set to something absolute,
// otherwise it ends up being inadvertently set to the current
@@ -63,9 +66,6 @@
VFS->setCurrentWorkingDirectory("//./");
}
- void TearDown() override {
- }
-
void AddFile(const std::string &Filename, const std::string &Contents) {
::time_t now;
::time(&now);
@@ -123,6 +123,72 @@
}
};
+TEST_F(PCHPreambleTest, ReparseReusesPreambleWithUnsavedFileNotExistingOnDisk) {
+ std::string Header1 = "//./header1.h";
+ std::string MainName = "//./main.cpp";
+ AddFile(MainName, R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp");
+ RemapFile(Header1, "#define ZERO 0\n");
+
+ // Parse with header file provided as unsaved file, which does not exist on
+ // disk.
+ std::unique_ptr AST(ParseAST(MainName));
+ ASSERT_TRUE(AST.get());
+ ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+ // Reparse and check that the preamble was reused.
+ ASSERT_TRUE(ReparseAST(AST));
+ ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+}
+
+TEST_F(PCHPreambleTest, ReparseReusesPreambleAfterUnsavedFileWasCreatedOnDisk) {
+ std::string Header1 = "//./header1.h";
+ std::string MainName = "//./main.cpp";
+ AddFile(MainName, R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp");
+ RemapFile(Header1, "#define ZERO 0\n");
+
+ // Parse with header file provided as unsaved file, which does not exist on
+ // disk.
+ std::unique_ptr AST(ParseAST(MainName));
+ ASSERT_TRUE(AST.get());
+ ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+ // Create the unsaved file also on disk and check that preamble was reused.
+ AddFile(Header1, "#define ZERO 0\n");
+ ASSERT_TRUE(ReparseAST(AST));
+ ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+}
+
+TEST_F(PCHPreambleTest,
+ ReparseReusesPreambleAfterUnsavedFileWasRemovedFromDisk) {
+ std::string Header1 = "//./foo/header1.h";
+ std::string MainName = "//./main.cpp";
+ std::string MainFileContent = R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp";
+ AddFile(MainName, MainFileContent);
+ AddFile(Header1, "#define ZERO 0\n");
+ RemapFile(Header1, "#define ZERO 0\n");
+
+ // Parse with header file provided as unsaved file, which exists on disk.
+ std::unique_ptr AST(ParseAST(MainName));
+ ASSERT_TRUE(AST.get());
+ ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+ ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+
+ // Remove the unsaved file from disk and check that the preamble was reused.
+ ResetVFS();
+ AddFile(MainName, MainFileContent);
+ ASSERT_TRUE(ReparseAST(AST));
+ ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+}
+
TEST_F(PCHPreambleTest, ReparseWithOverriddenFileDoesNotInvalidatePreamble) {
std::string Header1 = "//./header1.h";
std::string Header2 = "//./header2.h";
Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -28,6 +28,7 @@
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Mutex.h"
#include "llvm/Support/MutexGuard.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/Process.h"
#include "llvm/Support/VirtualFileSystem.h"
#include
@@ -451,20 +452,32 @@
Status.getSize(), llvm::sys::toTimeT(Status.getLastModificationTime()));
}
+ llvm::StringMap OverridenFileBuffers;
for (const auto &RB : PreprocessorOpts.RemappedFileBuffers) {
-llvm::vfs::Status Status;
-if (!moveOnNoError(VFS->status(RB.first), Status))
- return false;
-
-OverriddenFiles[Status.getUniqueID()] =
+const PrecompiledPreamble::PreambleFileHash PreambleHash =
PreambleFileHash::createForMemoryBuffer(RB.second);
+llvm::vfs::Status Status;
+if (moveOnNoError(VFS->status(RB.first), Status))
+ OverriddenFiles[Status.getUniqueID()] = PreambleHash;
+else
+ OverridenFileBuffers[RB.first] = PreambleHash;
}
// Check whether anything has changed.
for (const auto &F : FilesInPreamble) {
+
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
ilya-biryukov added inline comments.
Comment at: lib/Frontend/PrecompiledPreamble.cpp:459
+if (moveOnNoError(VFS->status(RB.first), Status)) {
+ OverriddenFiles[Status.getUniqueID()] = PreambleHash;
+} else {
NIT: remove braces around single-statement branches, they are usually omitted
in the LLVM code
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D41005/new/
https://reviews.llvm.org/D41005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
nik marked 4 inline comments as done.
nik added inline comments.
Comment at: include/clang/Frontend/ASTUnit.h:581
+ unsigned getPreambleCounter() const { return PreambleCounter; }
+
ilya-biryukov wrote:
> NIT: `getPreambleCounterForTests()`? This is clearly an internal detail,
> would try giving it a name that discourages using it outside the testing code.
Done.
Side note: I hardly see something like this used in clang, but I agree that
it's good.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D41005/new/
https://reviews.llvm.org/D41005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
nik updated this revision to Diff 176962.
nik marked an inline comment as done.
nik added a comment.
Addressed comments.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D41005/new/
https://reviews.llvm.org/D41005
Files:
include/clang/Frontend/ASTUnit.h
lib/Frontend/ASTUnit.cpp
lib/Frontend/PrecompiledPreamble.cpp
unittests/Frontend/PCHPreambleTest.cpp
Index: unittests/Frontend/PCHPreambleTest.cpp
===
--- unittests/Frontend/PCHPreambleTest.cpp
+++ unittests/Frontend/PCHPreambleTest.cpp
@@ -53,7 +53,10 @@
FileSystemOptions FSOpts;
public:
- void SetUp() override {
+ void SetUp() override { ResetVFS(); }
+ void TearDown() override {}
+
+ void ResetVFS() {
VFS = new ReadCountingInMemoryFileSystem();
// We need the working directory to be set to something absolute,
// otherwise it ends up being inadvertently set to the current
@@ -64,9 +67,6 @@
VFS->setCurrentWorkingDirectory("//./");
}
- void TearDown() override {
- }
-
void AddFile(const std::string &Filename, const std::string &Contents) {
::time_t now;
::time(&now);
@@ -124,6 +124,72 @@
}
};
+TEST_F(PCHPreambleTest, ReparseReusesPreambleWithUnsavedFileNotExistingOnDisk) {
+ std::string Header1 = "//./header1.h";
+ std::string MainName = "//./main.cpp";
+ AddFile(MainName, R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp");
+ RemapFile(Header1, "#define ZERO 0\n");
+
+ // Parse with header file provided as unsaved file, which does not exist on
+ // disk.
+ std::unique_ptr AST(ParseAST(MainName));
+ ASSERT_TRUE(AST.get());
+ ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+ // Reparse and check that the preamble was reused.
+ ASSERT_TRUE(ReparseAST(AST));
+ ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+}
+
+TEST_F(PCHPreambleTest, ReparseReusesPreambleAfterUnsavedFileWasCreatedOnDisk) {
+ std::string Header1 = "//./header1.h";
+ std::string MainName = "//./main.cpp";
+ AddFile(MainName, R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp");
+ RemapFile(Header1, "#define ZERO 0\n");
+
+ // Parse with header file provided as unsaved file, which does not exist on
+ // disk.
+ std::unique_ptr AST(ParseAST(MainName));
+ ASSERT_TRUE(AST.get());
+ ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+ // Create the unsaved file also on disk and check that preamble was reused.
+ AddFile(Header1, "#define ZERO 0\n");
+ ASSERT_TRUE(ReparseAST(AST));
+ ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+}
+
+TEST_F(PCHPreambleTest,
+ ReparseReusesPreambleAfterUnsavedFileWasRemovedFromDisk) {
+ std::string Header1 = "//./foo/header1.h";
+ std::string MainName = "//./main.cpp";
+ std::string MainFileContent = R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp";
+ AddFile(MainName, MainFileContent);
+ AddFile(Header1, "#define ZERO 0\n");
+ RemapFile(Header1, "#define ZERO 0\n");
+
+ // Parse with header file provided as unsaved file, which exists on disk.
+ std::unique_ptr AST(ParseAST(MainName));
+ ASSERT_TRUE(AST.get());
+ ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+ ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+
+ // Remove the unsaved file from disk and check that the preamble was reused.
+ ResetVFS();
+ AddFile(MainName, MainFileContent);
+ ASSERT_TRUE(ReparseAST(AST));
+ ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+}
+
TEST_F(PCHPreambleTest, ReparseWithOverriddenFileDoesNotInvalidatePreamble) {
std::string Header1 = "//./header1.h";
std::string Header2 = "//./header2.h";
Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -28,6 +28,7 @@
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Mutex.h"
#include "llvm/Support/MutexGuard.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/Process.h"
#include "llvm/Support/VirtualFileSystem.h"
#include
@@ -449,20 +450,33 @@
Status.getSize(), llvm::sys::toTimeT(Status.getLastModificationTime()));
}
+ llvm::StringMap OverridenFileBuffers;
for (const auto &RB : PreprocessorOpts.RemappedFileBuffers) {
-llvm::vfs::Status Status;
-if (!moveOnNoError(VFS->status(RB.first), Status))
- return false;
-
-OverriddenFiles[Status.getUniqueID()] =
+const PrecompiledPreamble::PreambleFileHash PreambleHash =
PreambleFileHash::createForMemoryBuffer(RB.second);
+llvm::vfs::Status Status;
+if (moveOnNoError(VFS->status(RB.first), Status)) {
+ OverriddenFiles[Status.getUniqueID()] = PreambleHash;
+} else {
+ OverridenFileBuffers[RB.first] = PreambleHash;
+}
}
// Check whether anything has changed.
for (const auto &F : FilesInPreamble) {
+auto OverridenFileBuffer = OverridenFileBuffers.fin
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
ilya-biryukov added inline comments.
Comment at: include/clang/Frontend/ASTUnit.h:581
+ unsigned getPreambleCounter() const { return PreambleCounter; }
+
NIT: `getPreambleCounterForTests()`? This is clearly an internal detail, would
try giving it a name that discourages using it outside the testing code.
Comment at: lib/Frontend/ASTUnit.cpp:1369
// Try again next time.
-PreambleRebuildCounter = 1;
+++PreambleCounter;
+PreambleRebuildCountdown = 1;
Why not increase this counter unconditionally for **every** error? (And a
non-error case too, I guess)
Comment at: lib/Frontend/PrecompiledPreamble.cpp:471
+} else {
+ auto FilePath = PathNormalized(RB.first.begin(), RB.first.end());
+ OverridenFileBuffers[FilePath] = PreambleHash;
Could we avoid adding path normalization in this patch? Would it break anything?
This is clearly an important detail that preamble **might** get wrong, but I'd
rather add it in a separate patch to avoid putting multiple things in the same
patch.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D41005/new/
https://reviews.llvm.org/D41005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
nik added a comment. Ping. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D41005/new/ https://reviews.llvm.org/D41005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
nik added a comment. Ping. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D41005/new/ https://reviews.llvm.org/D41005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
nik updated this revision to Diff 174022.
nik added a comment.
Addressed comments.
Repository:
rC Clang
https://reviews.llvm.org/D41005
Files:
include/clang/Frontend/ASTUnit.h
lib/Frontend/ASTUnit.cpp
lib/Frontend/PrecompiledPreamble.cpp
unittests/Frontend/PCHPreambleTest.cpp
Index: unittests/Frontend/PCHPreambleTest.cpp
===
--- unittests/Frontend/PCHPreambleTest.cpp
+++ unittests/Frontend/PCHPreambleTest.cpp
@@ -53,7 +53,10 @@
FileSystemOptions FSOpts;
public:
- void SetUp() override {
+ void SetUp() override { ResetVFS(); }
+ void TearDown() override {}
+
+ void ResetVFS() {
VFS = new ReadCountingInMemoryFileSystem();
// We need the working directory to be set to something absolute,
// otherwise it ends up being inadvertently set to the current
@@ -64,9 +67,6 @@
VFS->setCurrentWorkingDirectory("//./");
}
- void TearDown() override {
- }
-
void AddFile(const std::string &Filename, const std::string &Contents) {
::time_t now;
::time(&now);
@@ -124,6 +124,72 @@
}
};
+TEST_F(PCHPreambleTest, ReparseReusesPreambleWithUnsavedFileNotExistingOnDisk) {
+ std::string Header1 = "//./header1.h";
+ std::string MainName = "//./main.cpp";
+ AddFile(MainName, R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp");
+ RemapFile(Header1, "#define ZERO 0\n");
+
+ // Parse with header file provided as unsaved file, which does not exist on
+ // disk.
+ std::unique_ptr AST(ParseAST(MainName));
+ ASSERT_TRUE(AST.get());
+ ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+ // Reparse and check that the preamble was reused.
+ ASSERT_TRUE(ReparseAST(AST));
+ ASSERT_EQ(AST->getPreambleCounter(), 1U);
+}
+
+TEST_F(PCHPreambleTest, ReparseReusesPreambleAfterUnsavedFileWasCreatedOnDisk) {
+ std::string Header1 = "//./header1.h";
+ std::string MainName = "//./main.cpp";
+ AddFile(MainName, R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp");
+ RemapFile(Header1, "#define ZERO 0\n");
+
+ // Parse with header file provided as unsaved file, which does not exist on
+ // disk.
+ std::unique_ptr AST(ParseAST(MainName));
+ ASSERT_TRUE(AST.get());
+ ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+ // Create the unsaved file also on disk and check that preamble was reused.
+ AddFile(Header1, "#define ZERO 0\n");
+ ASSERT_TRUE(ReparseAST(AST));
+ ASSERT_EQ(AST->getPreambleCounter(), 1U);
+}
+
+TEST_F(PCHPreambleTest,
+ ReparseReusesPreambleAfterUnsavedFileWasRemovedFromDisk) {
+ std::string Header1 = "//./foo/../header1.h";
+ std::string MainName = "//./main.cpp";
+ std::string MainFileContent = R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp";
+ AddFile(MainName, MainFileContent);
+ AddFile(Header1, "#define ZERO 0\n");
+ RemapFile(Header1, "#define ZERO 0\n");
+
+ // Parse with header file provided as unsaved file, which exists on disk.
+ std::unique_ptr AST(ParseAST(MainName));
+ ASSERT_TRUE(AST.get());
+ ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+ ASSERT_EQ(AST->getPreambleCounter(), 1U);
+
+ // Remove the unsaved file from disk and check that the preamble was reused.
+ ResetVFS();
+ AddFile(MainName, MainFileContent);
+ ASSERT_TRUE(ReparseAST(AST));
+ ASSERT_EQ(AST->getPreambleCounter(), 1U);
+}
+
TEST_F(PCHPreambleTest, ReparseWithOverriddenFileDoesNotInvalidatePreamble) {
std::string Header1 = "//./header1.h";
std::string Header2 = "//./header2.h";
Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -28,6 +28,7 @@
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Mutex.h"
#include "llvm/Support/MutexGuard.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/Process.h"
#include "llvm/Support/VirtualFileSystem.h"
#include
@@ -221,6 +222,14 @@
return true;
}
+template
+llvm::SmallString<256> PathNormalized(IteratorType Start, IteratorType End) {
+ llvm::SmallString<256> Path{Start, End};
+ llvm::sys::path::remove_dots(Path, /*remove_dot_dots=*/true);
+ Path = llvm::sys::path::convert_to_slash(Path);
+ return Path;
+}
+
} // namespace
PreambleBounds clang::ComputePreambleBounds(const LangOptions &LangOpts,
@@ -367,13 +376,15 @@
const FileEntry *File = Clang->getFileManager().getFile(Filename);
if (!File || File == SourceMgr.getFileEntryForID(SourceMgr.getMainFileID()))
continue;
+auto FilePath =
+PathNormalized(File->getName().begin(), File->getName().end());
if (time_t ModTime = File->getModificationTime()) {
- FilesInPreamble[File->getName()] =
+ FilesInPreamble[FilePath] =
PrecompiledPreamble::PreambleFileHash::createForFile(File->getSize(),
ModTime);
} else {
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
nik added a comment. In https://reviews.llvm.org/D41005#1295632, @ilya-biryukov wrote: > > Before this change, this was not a problem because OverriddenFiles were > > keyed on Status.getUniqueID(). Starting with this change, the key is the > > file path. > > I suggest keeping two maps for overridden files: one for existing files (key > is UniqueID), another one for the ones that don't exist (key is the file > path). Done. >> Is there a nice way to map different file paths of the same file to the same >> id without touching the real file system? Would it be sufficient to >> normalize the file paths? If yes, is there a utility function for this >> (can't find it right now). > > I don't think there is one, the unique ids are closely tied to the > filesystem. IIUC the unique ids are the same whenever two different paths are > symlinks (or hardlinks?) pointing to the same physical file and there's no > way to tell if they're the same without resolving the symlink. OK, so if the unsaved file is not being backed up by a real file on disk and symlinks are used, we can't do much about it. I've normalized the file paths to handle at least that case where they might differ. Repository: rC Clang https://reviews.llvm.org/D41005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
ilya-biryukov added a comment. > Before this change, this was not a problem because OverriddenFiles were keyed > on Status.getUniqueID(). Starting with this change, the key is the file path. I suggest keeping two maps for overridden files: one for existing files (key is UniqueID), another one for the ones that don't exist (key is the file path). > Is there a nice way to map different file paths of the same file to the same > id without touching the real file system? Would it be sufficient to normalize > the file paths? If yes, is there a utility function for this (can't find it > right now). I don't think there is one, the unique ids are closely tied to the filesystem. IIUC the unique ids are the same whenever two different paths are symlinks (or hardlinks?) pointing to the same physical file and there's no way to tell if they're the same without resolving the symlink. Repository: rC Clang https://reviews.llvm.org/D41005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
nik added a comment. Ping. Repository: rC Clang https://reviews.llvm.org/D41005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
nik added a comment.
Coming back to this one, I see a failing test:
PCHPreambleTest.ReparseWithOverriddenFileDoesNotInvalidatePreamble
Note that PCHPreambleTest.ReparseWithOverriddenFileDoesNotInvalidatePreamble
references the header paths in different ways ("//./header1.h" vs
"//./foo/../header1.h"). Before this change, this was not a problem because
OverriddenFiles were keyed on Status.getUniqueID(). Starting with this change,
the key is the file path.
Is there a nice way to map different file paths of the same file to the same id
without touching the real file system? Would it be sufficient to normalize the
file paths? If yes, is there a utility function for this (can't find it right
now).
Comment at: include/clang/Frontend/ASTUnit.h:193
/// some number of calls.
- unsigned PreambleRebuildCounter;
+ unsigned PreambleRebuildCountdownCounter;
+
ilya-biryukov wrote:
> NIT: Maybe shorten to `PreambleRebuildCountdown`?
> It's not a great name, but a bit shorter than now and seems to convey the
> same meaning.
Will do.
Comment at: unittests/Frontend/PCHPreambleTest.cpp:130
+ std::string MainName = "//./main.cpp";
+ AddFile(MainName, "#include \"//./header1.h\"\n"
+"int main() { return ZERO; }");
ilya-biryukov wrote:
> NIT: Maybe use raw string literals? Escpated string are hard to read.
> E.g.,
>
> ```R"cpp(
> #include "//./header1.h"
> int main() { return ZERO; }
> )cpp"
> ```
Will do.
Comment at: unittests/Frontend/PCHPreambleTest.cpp:133
+
+ std::unique_ptr AST(ParseAST(MainName));
+ ASSERT_TRUE(AST.get());
ilya-biryukov wrote:
> Are we testing the right thing here?
>
> We're testing that preamble does not get rebuild when some header that was
> previously unresolved when seen inside `#include` directive is now added to
> the filesystem. That is actually a bug, we should rebuild the preamble in
> that case.
>
> We should probably call `RemapFile` before calling `ParseAST` instead and
> make sure it's properly resolved.
> ```
> AddFile(MainName, ...);
> // We don't call AddFile for the header at this point, so that it does not
> exist on the filesystem.
> RemapFile(Header1, "#define ZERO 0\n");
>
> std::unique_ptr AST(ParseAST(MainName));
> // Also assert that there were no compiler errors? (I.e. file was resolved
> properly)
> //
>
> // Now the file is on the filesystem, call reparse and check that we reused
> the preamble.
> AddFile(Header1, "#define ZERO 0\n");
> ASSERT_TRUE(ReparseAST(AST));
> ASSERT_EQ(AST->getPreambleCounter(), 1U);
> ```
> Are we testing the right thing here?
Huch, indeed, the test is wrong.
I'll incorporate your suggested test (real file is added after providing
unsaved file) and add also another one: real file is removed after providing an
unsaved file.
> That is actually a bug, we should rebuild the preamble in that case.
Agree, the preamble should be rebuild in such a case, but that's something for
a separate change.
Repository:
rC Clang
https://reviews.llvm.org/D41005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
nik added a comment. Sorry for the delay, I think I'll come back to this one soon. Repository: rC Clang https://reviews.llvm.org/D41005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
yvvan added a comment. In https://reviews.llvm.org/D41005#1001854, @cameron314 wrote: > @yvvan: The clang frontend tests (`PCHPreambleTest` and friends) are disabled > on Windows in the makefile (I think because the VFS tests depend on > linux-like paths). So running the tests on Windows without failures is > encouraging but not the whole story. Nice to know. But the bad thing is that it is not obvious at all from the code. And another bad thing is that tests are platform dependent. Repository: rC Clang https://reviews.llvm.org/D41005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
cameron314 added a comment. @yvvan: The clang frontend tests (`PCHPreambleTest` and friends) are disabled on Windows in the makefile (I think because the VFS tests depend on linux-like paths). So running the tests on Windows without failures is encouraging but not the whole story. Repository: rC Clang https://reviews.llvm.org/D41005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
ilya-biryukov added inline comments.
Comment at: include/clang/Frontend/ASTUnit.h:193
/// some number of calls.
- unsigned PreambleRebuildCounter;
+ unsigned PreambleRebuildCountdownCounter;
+
NIT: Maybe shorten to `PreambleRebuildCountdown`?
It's not a great name, but a bit shorter than now and seems to convey the same
meaning.
Comment at: unittests/Frontend/PCHPreambleTest.cpp:130
+ std::string MainName = "//./main.cpp";
+ AddFile(MainName, "#include \"//./header1.h\"\n"
+"int main() { return ZERO; }");
NIT: Maybe use raw string literals? Escpated string are hard to read.
E.g.,
```R"cpp(
#include "//./header1.h"
int main() { return ZERO; }
)cpp"
```
Comment at: unittests/Frontend/PCHPreambleTest.cpp:133
+
+ std::unique_ptr AST(ParseAST(MainName));
+ ASSERT_TRUE(AST.get());
Are we testing the right thing here?
We're testing that preamble does not get rebuild when some header that was
previously unresolved when seen inside `#include` directive is now added to the
filesystem. That is actually a bug, we should rebuild the preamble in that case.
We should probably call `RemapFile` before calling `ParseAST` instead and make
sure it's properly resolved.
```
AddFile(MainName, ...);
// We don't call AddFile for the header at this point, so that it does not
exist on the filesystem.
RemapFile(Header1, "#define ZERO 0\n");
std::unique_ptr AST(ParseAST(MainName));
// Also assert that there were no compiler errors? (I.e. file was resolved
properly)
//
// Now the file is on the filesystem, call reparse and check that we reused
the preamble.
AddFile(Header1, "#define ZERO 0\n");
ASSERT_TRUE(ReparseAST(AST));
ASSERT_EQ(AST->getPreambleCounter(), 1U);
```
Repository:
rC Clang
https://reviews.llvm.org/D41005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
yvvan added a comment. No regression in tests on Windows with and without extra patch ([PATCH] Use file path instead of uniqueID) Repository: rC Clang https://reviews.llvm.org/D41005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
nik updated this revision to Diff 128416.
nik added a comment.
Rebased and renamed the counter variable only.
I do not feel comfortable changing the "std::map OverriddenFiles". I can do this in a follow-up change if you
want.
@Ivan: Coul you please run the tests with this change on Windows?! If it goes
well (no failures), then please also give it also a try with the additional
change (reply to "Will anything fail if we remove the map from UniqueID to
hashes of overriden files and the corresponding checks?").
Repository:
rC Clang
https://reviews.llvm.org/D41005
Files:
include/clang/Frontend/ASTUnit.h
lib/Frontend/ASTUnit.cpp
lib/Frontend/PrecompiledPreamble.cpp
unittests/Frontend/PCHPreambleTest.cpp
Index: unittests/Frontend/PCHPreambleTest.cpp
===
--- unittests/Frontend/PCHPreambleTest.cpp
+++ unittests/Frontend/PCHPreambleTest.cpp
@@ -124,6 +124,22 @@
}
};
+TEST_F(PCHPreambleTest, ReparseDoesNotInvalidatePreambleDueToNotExistingUnsavedFile) {
+ std::string Header1 = "//./header1.h";
+ std::string MainName = "//./main.cpp";
+ AddFile(MainName, "#include \"//./header1.h\"\n"
+"int main() { return ZERO; }");
+
+ std::unique_ptr AST(ParseAST(MainName));
+ ASSERT_TRUE(AST.get());
+ ASSERT_EQ(AST->getPreambleCounter(), 1U);
+
+ RemapFile(Header1, "#define ZERO 0\n");
+ ASSERT_TRUE(ReparseAST(AST));
+
+ ASSERT_EQ(AST->getPreambleCounter(), 1U);
+}
+
TEST_F(PCHPreambleTest, ReparseWithOverriddenFileDoesNotInvalidatePreamble) {
std::string Header1 = "//./header1.h";
std::string Header2 = "//./header2.h";
Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -446,21 +446,28 @@
Status.getSize(), llvm::sys::toTimeT(Status.getLastModificationTime()));
}
+ llvm::StringMap OverridenFileBuffers;
for (const auto &RB : PreprocessorOpts.RemappedFileBuffers) {
-vfs::Status Status;
-if (!moveOnNoError(VFS->status(RB.first), Status))
- return false;
-
-OverriddenFiles[Status.getUniqueID()] =
+OverridenFileBuffers[RB.first] =
PreambleFileHash::createForMemoryBuffer(RB.second);
}
// Check whether anything has changed.
for (const auto &F : FilesInPreamble) {
+auto OverridenFileBuffer = OverridenFileBuffers.find(F.first());
+if (OverridenFileBuffer != OverridenFileBuffers.end()) {
+ // The file's buffer was remapped; check whether it matches up
+ // with the previous mapping.
+ if (OverridenFileBuffer->second != F.second)
+return false;
+ continue;
+}
+
vfs::Status Status;
if (!moveOnNoError(VFS->status(F.first()), Status)) {
- // If we can't stat the file, assume that something horrible happened.
- return false;
+// If the file's buffer is not remapped and we can't stat it,
+// assume that something horrible happened.
+return false;
}
std::map::iterator Overridden =
@@ -473,9 +480,10 @@
continue;
}
-// The file was not remapped; check whether it has changed on disk.
+// Neither the file's buffer nor the file itself was remapped;
+// check whether it has changed on disk.
if (Status.getSize() != uint64_t(F.second.Size) ||
-llvm::sys::toTimeT(Status.getLastModificationTime()) !=
+ llvm::sys::toTimeT(Status.getLastModificationTime()) !=
F.second.ModTime)
return false;
}
Index: lib/Frontend/ASTUnit.cpp
===
--- lib/Frontend/ASTUnit.cpp
+++ lib/Frontend/ASTUnit.cpp
@@ -189,7 +189,8 @@
TUKind(TU_Complete), WantTiming(getenv("LIBCLANG_TIMING")),
OwnsRemappedFileBuffers(true),
NumStoredDiagnosticsFromDriver(0),
-PreambleRebuildCounter(0),
+PreambleRebuildCountdownCounter(0),
+PreambleCounter(0),
NumWarningsInPreamble(0),
ShouldCacheCodeCompletionResults(false),
IncludeBriefCommentsInCodeCompletion(false), UserFilesAreVolatile(false),
@@ -1253,21 +1254,21 @@
PreambleInvocationIn.getDiagnosticOpts());
getDiagnostics().setNumWarnings(NumWarningsInPreamble);
- PreambleRebuildCounter = 1;
+ PreambleRebuildCountdownCounter = 1;
return MainFileBuffer;
} else {
Preamble.reset();
PreambleDiagnostics.clear();
TopLevelDeclsInPreamble.clear();
- PreambleRebuildCounter = 1;
+ PreambleRebuildCountdownCounter = 1;
}
}
// If the preamble rebuild counter > 1, it's because we previously
// failed to build a preamble and we're not yet ready to try
// again. Decrement the counter and return a failure.
- if (PreambleRebuildCounter > 1) {
---PreambleRebuildCounter;
+ if (PreambleRebuildCountdownCounter > 1) {
+--PreambleRebuildCountdownCounter;
return
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
nik added inline comments.
Comment at: lib/Frontend/PrecompiledPreamble.cpp:461
std::map::iterator Overridden =
OverriddenFiles.find(Status.getUniqueID());
nik wrote:
> ilya-biryukov wrote:
> > Will anything fail if we remove the map from `UniqueID` to hashes of
> > overriden files and the corresponding checks?
> >
> > We should either document why having `UniqueID`-based checks is important
> > or remove the code doing those checks.
> > Will anything fail if we remove the map from UniqueID to hashes of
> > overriden files and the corresponding checks?
>
> Hmm, I would need to remove/replace it and run the tests.
>
> I see these reasons for UniqueID:
> * It's already there :)
> * Normalization of file paths is not necessary.
> * It potentially can deal with hard links, though I'm not sure whether this
> is real world use case at all.
>
> > We should either document why having UniqueID-based checks is important or
> > remove the code doing those checks.
>
> Agree.
>
> Will anything fail if we remove the map from UniqueID to hashes of overriden
> files and the corresponding checks?
OK, I've applied the following patch (on this patch set/revision here) applied
and run check-clang - no failures here.
```
From 8f755128d1e167193c8f6de1456aec01d14307f6 Mon Sep 17 00:00:00 2001
From: Nikolai Kosjar
Date: Tue, 2 Jan 2018 15:14:07 +0100
Subject: [PATCH] Use file path instead of uniqueID
---
lib/Frontend/PrecompiledPreamble.cpp | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/lib/Frontend/PrecompiledPreamble.cpp
b/lib/Frontend/PrecompiledPreamble.cpp
index a43205a1c3..09e2b20a48 100644
--- a/lib/Frontend/PrecompiledPreamble.cpp
+++ b/lib/Frontend/PrecompiledPreamble.cpp
@@ -433,7 +433,7 @@ bool PrecompiledPreamble::CanReuse(const CompilerInvocation
&Invocation,
// Check that none of the files used by the preamble have changed.
// First, make a record of those files that have been overridden via
// remapping or unsaved_files.
- std::map OverriddenFiles;
+ std::map OverriddenFiles;
for (const auto &R : PreprocessorOpts.RemappedFiles) {
vfs::Status Status;
if (!moveOnNoError(VFS->status(R.second), Status)) {
@@ -442,7 +442,7 @@ bool PrecompiledPreamble::CanReuse(const CompilerInvocation
&Invocation,
return false;
}
-OverriddenFiles[Status.getUniqueID()] = PreambleFileHash::createForFile(
+OverriddenFiles[R.first] = PreambleFileHash::createForFile(
Status.getSize(),
llvm::sys::toTimeT(Status.getLastModificationTime()));
}
@@ -470,8 +470,8 @@ bool PrecompiledPreamble::CanReuse(const CompilerInvocation
&Invocation,
return false;
}
-std::map::iterator Overridden =
-OverriddenFiles.find(Status.getUniqueID());
+std::map::iterator Overridden =
+OverriddenFiles.find(F.first());
if (Overridden != OverriddenFiles.end()) {
// This file was remapped; check whether the newly-mapped file
// matches up with the previous mapping.
--
2.15.1
```
Repository:
rC Clang
https://reviews.llvm.org/D41005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
nik added inline comments. Comment at: include/clang/Frontend/ASTUnit.h:196 + /// \brief Counter indicating how often the preamble was build in total. + unsigned PreambleCounter; + ilya-biryukov wrote: > nik wrote: > > Any better name for this one? Otherwise I would suggest renaming > > PreambleRebuildCounter to PreambleRebuildCountdownCounter to make them more > > distinct. > +1, names PreambleRebuildCounter and PreambleCounter are too similar. > OK, will address in the next patch set / diff. Comment at: lib/Frontend/PrecompiledPreamble.cpp:461 std::map::iterator Overridden = OverriddenFiles.find(Status.getUniqueID()); ilya-biryukov wrote: > Will anything fail if we remove the map from `UniqueID` to hashes of > overriden files and the corresponding checks? > > We should either document why having `UniqueID`-based checks is important or > remove the code doing those checks. > Will anything fail if we remove the map from UniqueID to hashes of overriden > files and the corresponding checks? Hmm, I would need to remove/replace it and run the tests. I see these reasons for UniqueID: * It's already there :) * Normalization of file paths is not necessary. * It potentially can deal with hard links, though I'm not sure whether this is real world use case at all. > We should either document why having UniqueID-based checks is important or > remove the code doing those checks. Agree. Repository: rC Clang https://reviews.llvm.org/D41005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
ilya-biryukov added inline comments.
Comment at: include/clang/Frontend/ASTUnit.h:196
+ /// \brief Counter indicating how often the preamble was build in total.
+ unsigned PreambleCounter;
+
nik wrote:
> Any better name for this one? Otherwise I would suggest renaming
> PreambleRebuildCounter to PreambleRebuildCountdownCounter to make them more
> distinct.
+1, names PreambleRebuildCounter and PreambleCounter are too similar.
Comment at: lib/Frontend/PrecompiledPreamble.cpp:437
+ vfs::OverlayFileSystem OFS(VFS);
+ IntrusiveRefCntPtr IMFS(
nik wrote:
> ilya-biryukov wrote:
> > Can we do the same thing without creating an additional `OverlayFileSystem`?
> >
> > If we add a separate map to check for those:
> > ```
> > llvm::StringMap OverriddenFilesWithoutFS.
> > // Populate the map with paths not existing in VFS.
> >
> > for (const auto &F : FilesInPreamble) {
> > vfs::Status Status;
> > if (!moveOnNoError(OFS.status(F.first()), Status)) {
> > // If we can't stat the file, check whether it was overriden
> > auto It = OverriddenFilesWithoutFS[F.first()];
> > if (It == OverriddenFilesWithoutFS.end())
> > return false;
> > //.
> > }
> > //..
> >
> > }
> > ```
> > Can we do the same thing without creating an additional OverlayFileSystem?
>
> Hmm, I thought I had a good reason for this one. I don't remember it and the
> test still passes, so I did it without it now.
>
> Is there any real advantage in first trying the stat, then checking
> OverriddenFilesWithoutFS? Since I don't see any, I've changed order because
> the stat can then be avoided; also, it removes some nesting.
I don't see any advantage.
I used to think it had something to do with overriden symlinks, but after
thinking more about it I'm not even sure what should the semantics of those be.
And I don't think the current code handles that (we have neither tests nor
comments suggesting that this use-case was considered in the first place).
This possibly handles case-insensitive filesystems, like NTFS on Windows. But
I'm not sure if that was the original intention of this code.
Comment at: lib/Frontend/PrecompiledPreamble.cpp:461
std::map::iterator Overridden =
OverriddenFiles.find(Status.getUniqueID());
Will anything fail if we remove the map from `UniqueID` to hashes of overriden
files and the corresponding checks?
We should either document why having `UniqueID`-based checks is important or
remove the code doing those checks.
Repository:
rC Clang
https://reviews.llvm.org/D41005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
nik added inline comments. Comment at: include/clang/Frontend/ASTUnit.h:196 + /// \brief Counter indicating how often the preamble was build in total. + unsigned PreambleCounter; + Any better name for this one? Otherwise I would suggest renaming PreambleRebuildCounter to PreambleRebuildCountdownCounter to make them more distinct. Repository: rC Clang https://reviews.llvm.org/D41005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
nik updated this revision to Diff 126353.
nik marked 2 inline comments as done.
nik added a comment.
Addressed Ilya's comments.
Repository:
rC Clang
https://reviews.llvm.org/D41005
Files:
include/clang/Frontend/ASTUnit.h
lib/Frontend/ASTUnit.cpp
lib/Frontend/PrecompiledPreamble.cpp
unittests/Frontend/PCHPreambleTest.cpp
Index: unittests/Frontend/PCHPreambleTest.cpp
===
--- unittests/Frontend/PCHPreambleTest.cpp
+++ unittests/Frontend/PCHPreambleTest.cpp
@@ -124,6 +124,22 @@
}
};
+TEST_F(PCHPreambleTest, ReparseDoesNotInvalidatePreambleDueToNotExistingUnsavedFile) {
+ std::string Header1 = "//./header1.h";
+ std::string MainName = "//./main.cpp";
+ AddFile(MainName, "#include \"//./header1.h\"\n"
+"int main() { return ZERO; }");
+
+ std::unique_ptr AST(ParseAST(MainName));
+ ASSERT_TRUE(AST.get());
+ ASSERT_EQ(AST->getPreambleCounter(), 1U);
+
+ RemapFile(Header1, "#define ZERO 0\n");
+ ASSERT_TRUE(ReparseAST(AST));
+
+ ASSERT_EQ(AST->getPreambleCounter(), 1U);
+}
+
TEST_F(PCHPreambleTest, ReparseWithOverriddenFileDoesNotInvalidatePreamble) {
std::string Header1 = "//./header1.h";
std::string Header2 = "//./header2.h";
Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -434,21 +434,28 @@
Status.getSize(), llvm::sys::toTimeT(Status.getLastModificationTime()));
}
+ llvm::StringMap OverridenFileBuffers;
for (const auto &RB : PreprocessorOpts.RemappedFileBuffers) {
-vfs::Status Status;
-if (!moveOnNoError(VFS->status(RB.first), Status))
- return false;
-
-OverriddenFiles[Status.getUniqueID()] =
+OverridenFileBuffers[RB.first] =
PreambleFileHash::createForMemoryBuffer(RB.second);
}
// Check whether anything has changed.
for (const auto &F : FilesInPreamble) {
+auto OverridenFileBuffer = OverridenFileBuffers.find(F.first());
+if (OverridenFileBuffer != OverridenFileBuffers.end()) {
+ // The file's buffer was remapped; check whether it matches up
+ // with the previous mapping.
+ if (OverridenFileBuffer->second != F.second)
+return false;
+ continue;
+}
+
vfs::Status Status;
if (!moveOnNoError(VFS->status(F.first()), Status)) {
- // If we can't stat the file, assume that something horrible happened.
- return false;
+// If the file's buffer is not remapped and we can't stat it,
+// assume that something horrible happened.
+return false;
}
std::map::iterator Overridden =
@@ -461,9 +468,10 @@
continue;
}
-// The file was not remapped; check whether it has changed on disk.
+// Neither the file's buffer nor the file itself was remapped;
+// check whether it has changed on disk.
if (Status.getSize() != uint64_t(F.second.Size) ||
-llvm::sys::toTimeT(Status.getLastModificationTime()) !=
+ llvm::sys::toTimeT(Status.getLastModificationTime()) !=
F.second.ModTime)
return false;
}
Index: lib/Frontend/ASTUnit.cpp
===
--- lib/Frontend/ASTUnit.cpp
+++ lib/Frontend/ASTUnit.cpp
@@ -190,6 +190,7 @@
OwnsRemappedFileBuffers(true),
NumStoredDiagnosticsFromDriver(0),
PreambleRebuildCounter(0),
+PreambleCounter(0),
NumWarningsInPreamble(0),
ShouldCacheCodeCompletionResults(false),
IncludeBriefCommentsInCodeCompletion(false), UserFilesAreVolatile(false),
@@ -1296,6 +1297,7 @@
PCHContainerOps, /*StoreInMemory=*/false, Callbacks);
if (NewPreamble) {
Preamble = std::move(*NewPreamble);
+ ++PreambleCounter;
PreambleRebuildCounter = 1;
} else {
switch (static_cast(NewPreamble.getError().value())) {
Index: include/clang/Frontend/ASTUnit.h
===
--- include/clang/Frontend/ASTUnit.h
+++ include/clang/Frontend/ASTUnit.h
@@ -192,6 +192,9 @@
/// some number of calls.
unsigned PreambleRebuildCounter;
+ /// \brief Counter indicating how often the preamble was build in total.
+ unsigned PreambleCounter;
+
/// \brief Cache pairs "filename - source location"
///
/// Cache contains only source locations from preamble so it is
@@ -547,7 +550,11 @@
return SourceRange(mapLocationToPreamble(R.getBegin()),
mapLocationToPreamble(R.getEnd()));
}
-
+
+ unsigned getPreambleCounter() const {
+return PreambleCounter;
+ }
+
// Retrieve the diagnostics associated with this AST
typedef StoredDiagnostic *stored_diag_iterator;
typedef const StoredDiagnostic *stored_diag_const_iterator;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailm
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
nik marked 2 inline comments as done.
nik added inline comments.
Comment at: include/clang/Frontend/PrecompiledPreamble.h:109
+ std::chrono::steady_clock::time_point getCreationTimePoint() const {
+return CreationTimePoint;
ilya-biryukov wrote:
> Having this time stamp in the interface of `PrecompiledPreamble` doesn't
> really makes sense.
>
> I propose we remove this method and test in a different way instead.
>
> For example, we could add a counter to `ASTUnit` that increases when preamble
> is built and check this counter.
> Or we could add a unit-test that uses `PrecompiledPreamble` directly.
> For example, we could add a counter to ASTUnit that increases when preamble
> is built and check this counter.
OK, I've followed this proposal because there is already test infrastructure in
PCHPreambleTest that I can easily reuse.
Comment at: lib/Frontend/PrecompiledPreamble.cpp:437
+ vfs::OverlayFileSystem OFS(VFS);
+ IntrusiveRefCntPtr IMFS(
ilya-biryukov wrote:
> Can we do the same thing without creating an additional `OverlayFileSystem`?
>
> If we add a separate map to check for those:
> ```
> llvm::StringMap OverriddenFilesWithoutFS.
> // Populate the map with paths not existing in VFS.
>
> for (const auto &F : FilesInPreamble) {
> vfs::Status Status;
> if (!moveOnNoError(OFS.status(F.first()), Status)) {
> // If we can't stat the file, check whether it was overriden
> auto It = OverriddenFilesWithoutFS[F.first()];
> if (It == OverriddenFilesWithoutFS.end())
> return false;
> //.
> }
> //..
>
> }
> ```
> Can we do the same thing without creating an additional OverlayFileSystem?
Hmm, I thought I had a good reason for this one. I don't remember it and the
test still passes, so I did it without it now.
Is there any real advantage in first trying the stat, then checking
OverriddenFilesWithoutFS? Since I don't see any, I've changed order because the
stat can then be avoided; also, it removes some nesting.
Comment at: lib/Frontend/PrecompiledPreamble.cpp:444
vfs::Status Status;
-if (!moveOnNoError(VFS->status(RB.first), Status))
- return false;
-
+assert(moveOnNoError(IMFS->status(RB.first), Status));
OverriddenFiles[Status.getUniqueID()] =
ilya-biryukov wrote:
> `assert` macro is a no-op when `NDEBUG` is defined.
> One must never put an operation with side-effects inside `assert`.
Huch, forgot to remove it on cleanup. Anyway, it's obsolete now.
Repository:
rC Clang
https://reviews.llvm.org/D41005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
ilya-biryukov added a comment. In https://reviews.llvm.org/D41005#949550, @cameron314 wrote: > It's been a while since I was in this code, but I seem to recall the file > needed to exist on disk in order to uniquely identify it (via inode). Does > this break the up-to-date check? When the file is missing from the disk, but was remapped, preamble can not be reused. (Because we're always looking at uniqueids). If the remapped file did not exist on disk originally when building the preamble, we should allow the preamble to be reused if it's still remapped, but not created on disk. Repository: rC Clang https://reviews.llvm.org/D41005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
cameron314 added a comment. It's been a while since I was in this code, but I seem to recall the file needed to exist on disk in order to uniquely identify it (via inode). Does this break the up-to-date check? Repository: rC Clang https://reviews.llvm.org/D41005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.
This looks useful. Could we avoid creating the `OverlayFileSystem`, though?
Comment at: include/clang/Frontend/PrecompiledPreamble.h:109
+ std::chrono::steady_clock::time_point getCreationTimePoint() const {
+return CreationTimePoint;
Having this time stamp in the interface of `PrecompiledPreamble` doesn't really
makes sense.
I propose we remove this method and test in a different way instead.
For example, we could add a counter to `ASTUnit` that increases when preamble
is built and check this counter.
Or we could add a unit-test that uses `PrecompiledPreamble` directly.
Comment at: lib/Frontend/PrecompiledPreamble.cpp:437
+ vfs::OverlayFileSystem OFS(VFS);
+ IntrusiveRefCntPtr IMFS(
Can we do the same thing without creating an additional `OverlayFileSystem`?
If we add a separate map to check for those:
```
llvm::StringMap OverriddenFilesWithoutFS.
// Populate the map with paths not existing in VFS.
for (const auto &F : FilesInPreamble) {
vfs::Status Status;
if (!moveOnNoError(OFS.status(F.first()), Status)) {
// If we can't stat the file, check whether it was overriden
auto It = OverriddenFilesWithoutFS[F.first()];
if (It == OverriddenFilesWithoutFS.end())
return false;
//.
}
//..
}
```
Comment at: lib/Frontend/PrecompiledPreamble.cpp:444
vfs::Status Status;
-if (!moveOnNoError(VFS->status(RB.first), Status))
- return false;
-
+assert(moveOnNoError(IMFS->status(RB.first), Status));
OverriddenFiles[Status.getUniqueID()] =
`assert` macro is a no-op when `NDEBUG` is defined.
One must never put an operation with side-effects inside `assert`.
Repository:
rC Clang
https://reviews.llvm.org/D41005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41005: Reuse preamble even if an unsaved file does not exist
nik created this revision.
When a preamble is created an unsaved file not existing on disk is
already part of PrecompiledPreamble::FilesInPreamble. However, when
checking whether the preamble can be re-used, a failed stat of such an
unsaved file invalidated the preamble, which led to pointless and time
consuming preamble regenerations on subsequent reparses.
Do not require anymore that unsaved files should exist on disk.
This avoids costly preamble invalidations depending on timing issues for
the cases where the file on disk might be removed just to be regenerated
a bit later.
It also allows an IDE to provide in-memory files that might not exist on
disk, e.g. because the build system hasn't generated those yet.
Repository:
rC Clang
https://reviews.llvm.org/D41005
Files:
include/clang/Frontend/ASTUnit.h
include/clang/Frontend/PrecompiledPreamble.h
lib/Frontend/ASTUnit.cpp
lib/Frontend/PrecompiledPreamble.cpp
unittests/Frontend/PCHPreambleTest.cpp
Index: unittests/Frontend/PCHPreambleTest.cpp
===
--- unittests/Frontend/PCHPreambleTest.cpp
+++ unittests/Frontend/PCHPreambleTest.cpp
@@ -124,6 +124,23 @@
}
};
+TEST_F(PCHPreambleTest, ReparseDoesNotInvalidatePreambleDueToNotExistingUnsavedFile) {
+ std::string Header1 = "//./header1.h";
+ std::string MainName = "//./main.cpp";
+ AddFile(MainName, "#include \"//./header1.h\"\n"
+"int main() { return ZERO; }");
+
+ std::unique_ptr AST(ParseAST(MainName));
+ ASSERT_TRUE(AST.get());
+ auto InitialPreambleCreationTimePoint = AST->getPreambleCreationTimePoint();
+ ASSERT_NE(std::chrono::steady_clock::time_point(), InitialPreambleCreationTimePoint);
+
+ RemapFile(Header1, "#define ZERO 0\n");
+ ASSERT_TRUE(ReparseAST(AST));
+
+ ASSERT_EQ(InitialPreambleCreationTimePoint, AST->getPreambleCreationTimePoint());
+}
+
TEST_F(PCHPreambleTest, ReparseWithOverriddenFileDoesNotInvalidatePreamble) {
std::string Header1 = "//./header1.h";
std::string Header2 = "//./header2.h";
Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -390,10 +390,10 @@
return PreambleBounds(PreambleBytes.size(), PreambleEndsAtStartOfLine);
}
-bool PrecompiledPreamble::CanReuse(const CompilerInvocation &Invocation,
- const llvm::MemoryBuffer *MainFileBuffer,
- PreambleBounds Bounds,
- vfs::FileSystem *VFS) const {
+bool PrecompiledPreamble::CanReuse(
+const CompilerInvocation &Invocation,
+const llvm::MemoryBuffer *MainFileBuffer, PreambleBounds Bounds,
+IntrusiveRefCntPtr VFS) const {
assert(
Bounds.Size <= MainFileBuffer->getBufferSize() &&
@@ -434,19 +434,22 @@
Status.getSize(), llvm::sys::toTimeT(Status.getLastModificationTime()));
}
+ vfs::OverlayFileSystem OFS(VFS);
+ IntrusiveRefCntPtr IMFS(
+ new vfs::InMemoryFileSystem());
+ OFS.pushOverlay(IMFS);
for (const auto &RB : PreprocessorOpts.RemappedFileBuffers) {
+IMFS->addFile(RB.first, 0, llvm::MemoryBuffer::getMemBuffer(""));
vfs::Status Status;
-if (!moveOnNoError(VFS->status(RB.first), Status))
- return false;
-
+assert(moveOnNoError(IMFS->status(RB.first), Status));
OverriddenFiles[Status.getUniqueID()] =
PreambleFileHash::createForMemoryBuffer(RB.second);
}
// Check whether anything has changed.
for (const auto &F : FilesInPreamble) {
vfs::Status Status;
-if (!moveOnNoError(VFS->status(F.first()), Status)) {
+if (!moveOnNoError(OFS.status(F.first()), Status)) {
// If we can't stat the file, assume that something horrible happened.
return false;
}
@@ -495,7 +498,8 @@
llvm::StringMap FilesInPreamble)
: Storage(std::move(Storage)), FilesInPreamble(std::move(FilesInPreamble)),
PreambleBytes(std::move(PreambleBytes)),
- PreambleEndsAtStartOfLine(PreambleEndsAtStartOfLine) {
+ PreambleEndsAtStartOfLine(PreambleEndsAtStartOfLine),
+ CreationTimePoint(std::chrono::steady_clock::now()){
assert(this->Storage.getKind() != PCHStorage::Kind::Empty);
}
Index: lib/Frontend/ASTUnit.cpp
===
--- lib/Frontend/ASTUnit.cpp
+++ lib/Frontend/ASTUnit.cpp
@@ -1244,7 +1244,7 @@
if (Preamble) {
if (Preamble->CanReuse(PreambleInvocationIn, MainFileBuffer.get(), Bounds,
- VFS.get())) {
+ VFS)) {
// Okay! We can re-use the precompiled preamble.
// Set the state of the diagnostic object to mimic its state
Index: include/clang/Frontend/PrecompiledPreamble.h
===
--- include/clang/Frontend/PrecompiledPreamble.h
+++ include/clang/Frontend
