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
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 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
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 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 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
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
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 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
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
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
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. 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
