[PATCH] D20338: [PCH] Fixed overridden files always invalidating preamble even when unchanged
cameron314 abandoned this revision. cameron314 added a comment. This patch is obsolete. While waiting over a year for a review, somebody else came across the same fix (for a different manifestation of the same bug) in https://reviews.llvm.org/D27810 and managed to get it through. I think my test case and supporting changes are still useful, however, so I've submitted that separately in https://reviews.llvm.org/D37474. https://reviews.llvm.org/D20338 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20338: [PCH] Fixed overridden files always invalidating preamble even when unchanged
cameron314 added a comment. Anyone have time to check this out this week? It's a one-line fix, includes a test, and is for a fairly important bug :-) https://reviews.llvm.org/D20338 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20338: [PCH] Fixed overridden files always invalidating preamble even when unchanged
cameron314 added a comment. Anyone have a few minutes to look at this? http://reviews.llvm.org/D20338 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20338: [PCH] Fixed overridden files always invalidating preamble even when unchanged
cameron314 added a comment. Ping? :-) http://reviews.llvm.org/D20338 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20338: [PCH] Fixed overridden files always invalidating preamble even when unchanged
cameron314 updated this revision to Diff 60250.
cameron314 added a comment.
Here's the final fix (it's the line in FileManager.cpp, plus a test).
http://reviews.llvm.org/D20338
Files:
include/clang/Frontend/ASTUnit.h
lib/Basic/FileManager.cpp
lib/Frontend/ASTUnit.cpp
unittests/Frontend/CMakeLists.txt
unittests/Frontend/PchPreambleTest.cpp
Index: unittests/Frontend/PchPreambleTest.cpp
===
--- /dev/null
+++ unittests/Frontend/PchPreambleTest.cpp
@@ -0,0 +1,155 @@
+//-- unittests/Frontend/PchPreambleTest.cpp - FrontendAction tests ---//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Frontend/FrontendOptions.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/FileManager.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "gtest/gtest.h"
+#include
+
+using namespace llvm;
+using namespace clang;
+
+namespace {
+
+class ReadCountingInMemoryFileSystem : public vfs::InMemoryFileSystem
+{
+ std::map ReadCounts;
+
+public:
+ ErrorOr> openFileForRead(const Twine &Path) override
+ {
+SmallVector PathVec;
+Path.toVector(PathVec);
+llvm::sys::path::remove_dots(PathVec, true);
+++ReadCounts[std::string(PathVec.begin(), PathVec.end())];
+return InMemoryFileSystem::openFileForRead(Path);
+ }
+
+ unsigned GetReadCount(const Twine &Path) const
+ {
+auto it = ReadCounts.find(Path.str());
+return it == ReadCounts.end() ? 0 : it->second;
+ }
+};
+
+class PchPreambleTest : public ::testing::Test {
+ IntrusiveRefCntPtr VFS;
+ StringMap RemappedFiles;
+ std::shared_ptr PCHContainerOpts;
+ FileSystemOptions FSOpts;
+
+public:
+ void SetUp() override {
+VFS = new ReadCountingInMemoryFileSystem();
+// We need the working directory to be set to something absolute,
+// otherwise it ends up being inadvertently set to the current
+// working directory in the real file system due to a series of
+// unfortunate conditions interacting badly.
+// What's more, this path *must* be absolute on all (real)
+// filesystems, so just '/' won't work (e.g. on Win32).
+VFS->setCurrentWorkingDirectory("//./");
+ }
+
+ void TearDown() override {
+ }
+
+ void AddFile(const std::string &Filename, const std::string &Contents) {
+::time_t now;
+::time(&now);
+VFS->addFile(Filename, now, MemoryBuffer::getMemBufferCopy(Contents, Filename));
+ }
+
+ void RemapFile(const std::string &Filename, const std::string &Contents) {
+RemappedFiles[Filename] = Contents;
+ }
+
+ std::unique_ptr ParseAST(const std::string &EntryFile) {
+PCHContainerOpts = std::make_shared();
+CompilerInvocation *CI = new CompilerInvocation;
+CI->getFrontendOpts().Inputs.push_back(
+ FrontendInputFile(EntryFile, FrontendOptions::getInputKindForExtension(
+llvm::sys::path::extension(EntryFile).substr(1;
+
+CI->getTargetOpts().Triple = "i386-unknown-linux-gnu";
+
+CI->getPreprocessorOpts().RemappedFileBuffers = GetRemappedFiles();
+
+PreprocessorOptions &PPOpts = CI->getPreprocessorOpts();
+PPOpts.RemappedFilesKeepOriginalName = true;
+
+IntrusiveRefCntPtr
+ Diags(CompilerInstance::createDiagnostics(new DiagnosticOptions));
+
+FileManager *FileMgr = new FileManager(FSOpts, VFS);
+
+std::unique_ptr AST = ASTUnit::LoadFromCompilerInvocation(
+ CI, PCHContainerOpts, Diags, FileMgr, false, false,
+ /*PrecompilePreambleAfterNParses=*/1);
+return AST;
+ }
+
+ bool ReparseAST(const std::unique_ptr &AST) {
+FileManager *FileMgr = new FileManager(FSOpts, VFS);
+bool reparseFailed = AST->Reparse(PCHContainerOpts, GetRemappedFiles(), FileMgr);
+return reparseFailed;
+ }
+
+ unsigned GetFileReadCount(const std::string &Filename) const {
+return VFS->GetReadCount(Filename);
+ }
+
+private:
+ std::vector>
+ GetRemappedFiles() const {
+std::vector> Remapped;
+for (const auto &RemappedFile : RemappedFiles) {
+ std::unique_ptr buf = MemoryBuffer::getMemBufferCopy(
+RemappedFile.second, RemappedFile.first());
+ Remapped.emplace_back(RemappedFile.first(), buf.release());
+}
+return Remapped;
+ }
+};
+
+TEST_F(PchPreambleTest, ReparseWithOverriddenFileDoesNotInvalidatePreamble) {
+ std::string Header1 = "//./header1.h";
+ std::string Header2 = "//./header2.h";
+ std::string MainName = "//./main.cpp";
+ AddFile(Header1, "");
+ AddFile(Header2, "#pragma once");
+ AddFile(MainName,
+"#include \"//./foo/../header1.h\"\n"
+"#include \"
Re: [PATCH] D20338: [PCH] Fixed overridden files always invalidating preamble even when unchanged
cameron314 added inline comments. Comment at: lib/Basic/FileManager.cpp:389 @@ -383,2 +388,3 @@ UFE->File.reset(); + UFE->IsVirtual = true; return UFE; rsmith wrote: > Yes. The `IsValid` flag is just supposed to mean that this file has actually > been added to the `UniqueRealFiles` map rather than having been > default-constructed by `operator[]`. Excellent then, I'll get rid of `IsVirtual` and use `IsValid` in place. This will condense things down to a one-line change plus a large diff for the test ^^ http://reviews.llvm.org/D20338 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20338: [PCH] Fixed overridden files always invalidating preamble even when unchanged
rsmith added inline comments. Comment at: lib/Basic/FileManager.cpp:389 @@ -383,2 +388,3 @@ UFE->File.reset(); + UFE->IsVirtual = true; return UFE; Yes. The `IsValid` flag is just supposed to mean that this file has actually been added to the `UniqueRealFiles` map rather than having been default-constructed by `operator[]`. http://reviews.llvm.org/D20338 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20338: [PCH] Fixed overridden files always invalidating preamble even when unchanged
cameron314 added a comment. This is a fairly important bug for anyone hosting clang as a library (e.g. IDEs). Can someone have a look at this patch when they have a free moment? http://reviews.llvm.org/D20338 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20338: [PCH] Fixed overridden files always invalidating preamble even when unchanged
cameron314 updated the summary for this revision.
cameron314 removed rL LLVM as the repository for this revision.
cameron314 updated this revision to Diff 59577.
cameron314 added a comment.
It took some modifications to the ASTUnit to support a virtual file system with
a PCH parse/reparse (preliminary VFS support had already been added in
http://reviews.llvm.org/rL249410 but it did not support initial parses using
PCHs, nor reparses), but I was finally able to write a test that checks that
the reparse actually uses the PCH with my fix, and rejects the PCH (rereading
everything and failing the test) without it.
http://reviews.llvm.org/D20338
Files:
include/clang/Basic/FileManager.h
include/clang/Frontend/ASTUnit.h
lib/Basic/FileManager.cpp
lib/Frontend/ASTUnit.cpp
unittests/Frontend/CMakeLists.txt
unittests/Frontend/PchPreambleTest.cpp
Index: unittests/Frontend/PchPreambleTest.cpp
===
--- /dev/null
+++ unittests/Frontend/PchPreambleTest.cpp
@@ -0,0 +1,155 @@
+//-- unittests/Frontend/PchPreambleTest.cpp - FrontendAction tests ---//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Frontend/FrontendOptions.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/FileManager.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "gtest/gtest.h"
+#include
+
+using namespace llvm;
+using namespace clang;
+
+namespace {
+
+class ReadCountingInMemoryFileSystem : public vfs::InMemoryFileSystem
+{
+ std::map ReadCounts;
+
+public:
+ ErrorOr> openFileForRead(const Twine &Path) override
+ {
+SmallVector PathVec;
+Path.toVector(PathVec);
+llvm::sys::path::remove_dots(PathVec, true);
+++ReadCounts[std::string(PathVec.begin(), PathVec.end())];
+return InMemoryFileSystem::openFileForRead(Path);
+ }
+
+ unsigned GetReadCount(const Twine &Path) const
+ {
+auto it = ReadCounts.find(Path.str());
+return it == ReadCounts.end() ? 0 : it->second;
+ }
+};
+
+class PchPreambleTest : public ::testing::Test {
+ IntrusiveRefCntPtr VFS;
+ StringMap RemappedFiles;
+ std::shared_ptr PCHContainerOpts;
+ FileSystemOptions FSOpts;
+
+public:
+ void SetUp() override {
+VFS = new ReadCountingInMemoryFileSystem();
+// We need the working directory to be set to something absolute,
+// otherwise it ends up being inadvertently set to the current
+// working directory in the real file system due to a series of
+// unfortunate conditions interacting badly.
+// What's more, this path *must* be absolute on all (real)
+// filesystems, so just '/' won't work (e.g. on Win32).
+VFS->setCurrentWorkingDirectory("//./");
+ }
+
+ void TearDown() override {
+ }
+
+ void AddFile(const std::string &Filename, const std::string &Contents) {
+::time_t now;
+::time(&now);
+VFS->addFile(Filename, now, MemoryBuffer::getMemBufferCopy(Contents, Filename));
+ }
+
+ void RemapFile(const std::string &Filename, const std::string &Contents) {
+RemappedFiles[Filename] = Contents;
+ }
+
+ std::unique_ptr ParseAST(const std::string &EntryFile) {
+PCHContainerOpts = std::make_shared();
+CompilerInvocation *CI = new CompilerInvocation;
+CI->getFrontendOpts().Inputs.push_back(
+ FrontendInputFile(EntryFile, FrontendOptions::getInputKindForExtension(
+llvm::sys::path::extension(EntryFile).substr(1;
+
+CI->getTargetOpts().Triple = "i386-unknown-linux-gnu";
+
+CI->getPreprocessorOpts().RemappedFileBuffers = GetRemappedFiles();
+
+PreprocessorOptions &PPOpts = CI->getPreprocessorOpts();
+PPOpts.RemappedFilesKeepOriginalName = true;
+
+IntrusiveRefCntPtr
+ Diags(CompilerInstance::createDiagnostics(new DiagnosticOptions));
+
+FileManager *FileMgr = new FileManager(FSOpts, VFS);
+
+std::unique_ptr AST = ASTUnit::LoadFromCompilerInvocation(
+ CI, PCHContainerOpts, Diags, FileMgr, false, false,
+ /*PrecompilePreambleAfterNParses=*/1);
+return AST;
+ }
+
+ bool ReparseAST(const std::unique_ptr &AST) {
+FileManager *FileMgr = new FileManager(FSOpts, VFS);
+bool reparseFailed = AST->Reparse(PCHContainerOpts, GetRemappedFiles(), FileMgr);
+return reparseFailed;
+ }
+
+ unsigned GetFileReadCount(const std::string &Filename) const {
+return VFS->GetReadCount(Filename);
+ }
+
+private:
+ std::vector>
+ GetRemappedFiles() const {
+std::vector> Remapped;
+for (const auto &RemappedFile : RemappedFiles) {
+ std::unique_ptr buf = MemoryBuffer::getMemBufferC
Re: [PATCH] D20338: [PCH] Fixed overridden files always invalidating preamble even when unchanged
cameron314 added a comment. Thanks @bruno, I'll have a look at using a VFS for the test. Comment at: lib/Basic/FileManager.cpp:389 @@ -383,2 +388,3 @@ UFE->File.reset(); + UFE->IsVirtual = true; return UFE; rsmith wrote: > Rather than adding this `IsVirtual` flag, could you just set `UFE->IsValid` > to `true` here? It looks like a simple oversight that this code fails to set > the `IsValid` flag properly. I could, but I didn't want to accidentally break something else that depends on virtual files not being valid. That's the type of change that can easily introduce a subtle bug not caught by the tests. Semantically, is a virtual file always valid? Repository: rL LLVM http://reviews.llvm.org/D20338 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20338: [PCH] Fixed overridden files always invalidating preamble even when unchanged
rsmith added inline comments. Comment at: lib/Basic/FileManager.cpp:389 @@ -383,2 +388,3 @@ UFE->File.reset(); + UFE->IsVirtual = true; return UFE; Rather than adding this `IsVirtual` flag, could you just set `UFE->IsValid` to `true` here? It looks like a simple oversight that this code fails to set the `IsValid` flag properly. Repository: rL LLVM http://reviews.llvm.org/D20338 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20338: [PCH] Fixed overridden files always invalidating preamble even when unchanged
bruno added a comment. You can probably find a way to test this by taking a look at unittests/Basic/VirtualFileSystemTest.cpp Repository: rL LLVM http://reviews.llvm.org/D20338 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20338: [PCH] Fixed overridden files always invalidating preamble even when unchanged
cameron314 added a comment. I'm not sure how to test this (originally I found this bug by stepping through with a debugger) -- is there a way to determine if an ASTUnit used a PCH for the preamble or not? I'd call the `getMainBufferWithPrecompiledPreamble` method manually but it's private. Repository: rL LLVM http://reviews.llvm.org/D20338 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20338: [PCH] Fixed overridden files always invalidating preamble even when unchanged
bruno added a subscriber: bruno. bruno added a comment. Hi Cameron, Can you add a testcase? Repository: rL LLVM http://reviews.llvm.org/D20338 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D20338: [PCH] Fixed overridden files always invalidating preamble even when unchanged
cameron314 created this revision.
cameron314 added a reviewer: rsmith.
cameron314 added a subscriber: cfe-commits.
cameron314 set the repository for this revision to rL LLVM.
Remapped files would always cause the preamble's PCH to be invalidated (even if
they hadn't changed) because the file manager was replacing the remapped
virtual entry (no modified time) with a real file entry (which has a modified
time) when an include directive was processed. This happens when the include
directive results in a slightly different path for the same file (e.g.
different slash direction, which happens very often on Windows).
Note: This was initially part of D20137 but I had incorrectly applied my own
patch, so the `IsVirtual = true` line was in the wrong spot and served no
purpose (and was subsequently removed from the patch). Now it actually does
something!
Repository:
rL LLVM
http://reviews.llvm.org/D20338
Files:
include/clang/Basic/FileManager.h
lib/Basic/FileManager.cpp
Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -301,6 +301,11 @@
return &UFE;
}
+ if (UFE.isVirtual()) {
+UFE.Name = InterndFileName;
+return &UFE;
+ }
+
// Otherwise, we don't have this file yet, add it.
UFE.Name= InterndFileName;
UFE.Size = Data.Size;
@@ -381,6 +386,7 @@
UFE->Dir = DirInfo;
UFE->UID = NextFileUID++;
UFE->File.reset();
+ UFE->IsVirtual = true;
return UFE;
}
Index: include/clang/Basic/FileManager.h
===
--- include/clang/Basic/FileManager.h
+++ include/clang/Basic/FileManager.h
@@ -60,6 +60,7 @@
bool IsNamedPipe;
bool InPCH;
bool IsValid; // Is this \c FileEntry initialized and valid?
+ bool IsVirtual; // Is this \c FileEntry a virtual file?
/// \brief The open file, if it is owned by the \p FileEntry.
mutable std::unique_ptr File;
@@ -69,20 +70,23 @@
public:
FileEntry()
- : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false)
+ : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false),
+IsVirtual(false)
{}
// FIXME: this is here to allow putting FileEntry in std::map. Once we have
// emplace, we shouldn't need a copy constructor anymore.
/// Intentionally does not copy fields that are not set in an uninitialized
/// \c FileEntry.
FileEntry(const FileEntry &FE) : UniqueID(FE.UniqueID),
- IsNamedPipe(FE.IsNamedPipe), InPCH(FE.InPCH), IsValid(FE.IsValid) {
+ IsNamedPipe(FE.IsNamedPipe), InPCH(FE.InPCH), IsValid(FE.IsValid),
+ IsVirtual(FE.IsVirtual) {
assert(!isValid() && "Cannot copy an initialized FileEntry");
}
const char *getName() const { return Name; }
bool isValid() const { return IsValid; }
+ bool isVirtual() const { return IsVirtual; }
off_t getSize() const { return Size; }
unsigned getUID() const { return UID; }
const llvm::sys::fs::UniqueID &getUniqueID() const { return UniqueID; }
Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -301,6 +301,11 @@
return &UFE;
}
+ if (UFE.isVirtual()) {
+UFE.Name = InterndFileName;
+return &UFE;
+ }
+
// Otherwise, we don't have this file yet, add it.
UFE.Name= InterndFileName;
UFE.Size = Data.Size;
@@ -381,6 +386,7 @@
UFE->Dir = DirInfo;
UFE->UID = NextFileUID++;
UFE->File.reset();
+ UFE->IsVirtual = true;
return UFE;
}
Index: include/clang/Basic/FileManager.h
===
--- include/clang/Basic/FileManager.h
+++ include/clang/Basic/FileManager.h
@@ -60,6 +60,7 @@
bool IsNamedPipe;
bool InPCH;
bool IsValid; // Is this \c FileEntry initialized and valid?
+ bool IsVirtual; // Is this \c FileEntry a virtual file?
/// \brief The open file, if it is owned by the \p FileEntry.
mutable std::unique_ptr File;
@@ -69,20 +70,23 @@
public:
FileEntry()
- : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false)
+ : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false),
+IsVirtual(false)
{}
// FIXME: this is here to allow putting FileEntry in std::map. Once we have
// emplace, we shouldn't need a copy constructor anymore.
/// Intentionally does not copy fields that are not set in an uninitialized
/// \c FileEntry.
FileEntry(const FileEntry &FE) : UniqueID(FE.UniqueID),
- IsNamedPipe(FE.IsNamedPipe), InPCH(FE.InPCH), IsValid(FE.IsValid) {
+ IsNamedPipe(FE.IsNamedPipe), InPCH(FE.InPCH), IsValid(FE.IsValid),
+ IsVirtual(FE.IsVirtual) {
assert(!isValid() && "Cannot copy an initialized FileEntry");
}
const char *getName() const { return Name; }
b
