[clang][cas] Switch to chained representation of IncludeTree::FileList

Instead of flattening file lists when importing PCH/modules, just create
a chained reference to the list and flatten it only in
FileList::forEachFile. This reduces the storage cost and performance
overhead of merging file lists.

(cherry picked from commit ee7ddda)
(cherry picked from commit 2fbfa49)

Author: benlangmuir

clang/include/clang/CAS/IncludeTree.h

@@ -264,25 +264,15 @@ class IncludeTree::File : public IncludeTreeBase<File> {
   }
 };
 
-/// A flat list of \c File entries. This is used along with a simple
-/// implementation of a \p vfs::FileSystem produced via
-/// \p createIncludeTreeFileSystem().
+/// A list of \c File entries. Multiple \c FileList can be combined without
+/// copying their contents. This is used along with a simple implementation of a
+/// \p vfs::FileSystem produced via \p createIncludeTreeFileSystem().
 class IncludeTree::FileList : public IncludeTreeBase<FileList> {
 public:
   static constexpr StringRef getNodeKind() { return "List"; }
 
   using FileSizeTy = uint32_t;
 
-  size_t getNumFiles() const { return getNumReferences(); }
-
-  ObjectRef getFileRef(size_t I) const {
-    assert(I < getNumFiles());
-    return getReference(I);
-  }
-
-  Expected<File> getFile(size_t I) { return getFile(getFileRef(I)); }
-  FileSizeTy getFileSize(size_t I) const;
-
   /// \returns each \c File entry along with its file size.
   llvm::Error
   forEachFile(llvm::function_ref<llvm::Error(File, FileSizeTy)> Callback);
@@ -294,7 +284,8 @@ class IncludeTree::FileList : public IncludeTreeBase<FileList> {
     ObjectRef FileRef;
     FileSizeTy Size;
   };
-  static Expected<FileList> create(ObjectStore &DB, ArrayRef<FileEntry> Files);
+  static Expected<FileList> create(ObjectStore &DB, ArrayRef<FileEntry> Files,
+                                   ArrayRef<ObjectRef> FileLists);
 
   static Expected<FileList> get(ObjectStore &CAS, ObjectRef Ref);
 
@@ -308,6 +299,9 @@ class IncludeTree::FileList : public IncludeTreeBase<FileList> {
     assert(isValid(*this));
   }
 
+  size_t getNumFilesCurrentList() const;
+  FileSizeTy getFileSize(size_t I) const;
+
   Expected<File> getFile(ObjectRef Ref) {
     auto Node = getCAS().getProxy(Ref);
     if (!Node)
@@ -744,26 +738,4 @@ createIncludeTreeFileSystem(IncludeTreeRoot &Root);
 } // namespace cas
 } // namespace clang
 
-namespace llvm {
-template <> struct DenseMapInfo<clang::cas::IncludeTree::FileList::FileEntry> {
-  using FileEntry = clang::cas::IncludeTree::FileList::FileEntry;
-
-  static FileEntry getEmptyKey() {
-    return {cas::ObjectRef::getDenseMapEmptyKey(), 0};
-  }
-
-  static FileEntry getTombstoneKey() {
-    return {cas::ObjectRef::getDenseMapTombstoneKey(), 0};
-  }
-
-  static unsigned getHashValue(FileEntry F) {
-    return F.FileRef.getDenseMapHash();
-  }
-
-  static bool isEqual(FileEntry LHS, FileEntry RHS) {
-    return LHS.FileRef == RHS.FileRef && LHS.Size == RHS.Size;
-  }
-};
-} // namespace llvm
-
 #endif

clang/lib/CAS/IncludeTree.cpp

@@ -7,8 +7,11 @@
 //===----------------------------------------------------------------------===//
 
 #include "clang/CAS/IncludeTree.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallBitVector.h"
 #include "llvm/Support/EndianStream.h"
+#include "llvm/Support/Error.h"
+#include <utility>
 
 using namespace clang;
 using namespace clang::cas;
@@ -220,10 +223,15 @@ IncludeTree::ModuleImport::create(ObjectStore &DB, StringRef ModuleName,
   return IncludeTreeBase::create(DB, {}, Buffer);
 }
 
+size_t IncludeTree::FileList::getNumFilesCurrentList() const {
+  return llvm::support::endian::read<uint32_t, llvm::support::little>(
+      getData().data());
+}
+
 IncludeTree::FileList::FileSizeTy
 IncludeTree::FileList::getFileSize(size_t I) const {
-  assert(I < getNumFiles());
-  StringRef Data = getData();
+  assert(I < getNumFilesCurrentList());
+  StringRef Data = getData().drop_front(sizeof(uint32_t));
   assert(Data.size() >= (I + 1) * sizeof(FileSizeTy));
   return llvm::support::endian::read<FileSizeTy, llvm::support::little>(
       Data.data() + I * sizeof(FileSizeTy));
@@ -232,29 +240,44 @@ IncludeTree::FileList::getFileSize(size_t I) const {
 llvm::Error IncludeTree::FileList::forEachFile(
     llvm::function_ref<llvm::Error(File, FileSizeTy)> Callback) {
   size_t I = 0;
+  size_t FileCount = getNumFilesCurrentList();
   return forEachReference([&](ObjectRef Ref) -> llvm::Error {
-    auto Include = getFile(Ref);
-    if (!Include)
-      return Include.takeError();
-    return Callback(std::move(*Include), getFileSize(I++));
+    if (I < FileCount) {
+      auto Include = getFile(Ref);
+      if (!Include)
+        return Include.takeError();
+      return Callback(std::move(*Include), getFileSize(I++));
+    }
+    // Otherwise, it's a chained FileList.
+    ++I;
+    auto Proxy = getCAS().getProxy(Ref);
+    if (!Proxy)
+      return Proxy.takeError();
+    FileList FL(std::move(*Proxy));
+    return FL.forEachFile(Callback);
   });
 }
 
 Expected<IncludeTree::FileList>
-IncludeTree::FileList::create(ObjectStore &DB, ArrayRef<FileEntry> Files) {
+IncludeTree::FileList::create(ObjectStore &DB, ArrayRef<FileEntry> Files,
+                              ArrayRef<ObjectRef> FileLists) {
   SmallVector<ObjectRef, 16> Refs;
-  Refs.reserve(Files.size());
+  Refs.reserve(Files.size() + FileLists.size());
   SmallString<256> Buffer;
-  Buffer.reserve(Files.size() * sizeof(FileSizeTy));
+  Buffer.reserve(sizeof(uint32_t) + Files.size() * sizeof(FileSizeTy));
 
   llvm::raw_svector_ostream BufOS(Buffer);
   llvm::support::endian::Writer Writer(BufOS, llvm::support::little);
+  Writer.write(static_cast<uint32_t>(Files.size()));
 
   for (const FileEntry &Entry : Files) {
     assert(File::isValid(DB, Entry.FileRef));
     Refs.push_back(Entry.FileRef);
     Writer.write(Entry.Size);
   }
+
+  Refs.append(FileLists.begin(), FileLists.end());
+
   return IncludeTreeBase::create(DB, Refs, Buffer);
 }
 
@@ -273,9 +296,13 @@ bool IncludeTree::FileList::isValid(const ObjectProxy &Node) {
   if (!IncludeTreeBase::isValid(Node))
     return false;
   IncludeTreeBase Base(Node);
-  unsigned NumFiles = Base.getNumReferences();
-  return NumFiles != 0 &&
-         Base.getData().size() == NumFiles * sizeof(FileSizeTy);
+  StringRef Data = Base.getData();
+  if (Data.size() < sizeof(uint32_t))
+    return false;
+  unsigned NumFiles =
+      llvm::support::endian::read<uint32_t, llvm::support::little>(Data.data());
+  return NumFiles != 0 && NumFiles <= Base.getNumReferences() &&
+         Data.size() == sizeof(uint32_t) + NumFiles * sizeof(FileSizeTy);
 }
 
 static constexpr char ModuleFlagFramework = 1 << 0;
@@ -845,20 +872,47 @@ class IncludeTreeFileSystem : public llvm::vfs::FileSystem {
 };
 } // namespace
 
+static llvm::Error diagnoseFileChange(IncludeTree::File F, ObjectRef Content) {
+  auto FilenameBlob = F.getFilename();
+  if (!FilenameBlob)
+    return FilenameBlob.takeError();
+  cas::ObjectStore &DB = F.getCAS();
+  std::string Filename(FilenameBlob->getData());
+  std::string OldID = DB.getID(Content).toString();
+  std::string NewID = DB.getID(F.getContentsRef()).toString();
+  return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                 "file '%s' changed during build; include-tree "
+                                 "contents changed from %s to %s",
+                                 Filename.c_str(), OldID.c_str(),
+                                 NewID.c_str());
+}
+
 Expected<IntrusiveRefCntPtr<llvm::vfs::FileSystem>>
 cas::createIncludeTreeFileSystem(IncludeTreeRoot &Root) {
   auto FileList = Root.getFileList();
   if (!FileList)
     return FileList.takeError();
 
+  // Map from FilenameRef to ContentsRef.
+  llvm::DenseMap<ObjectRef, ObjectRef> SeenContents;
+
   IntrusiveRefCntPtr<IncludeTreeFileSystem> IncludeTreeFS =
       new IncludeTreeFileSystem(Root.getCAS());
   llvm::Error E = FileList->forEachFile(
       [&](IncludeTree::File File,
           IncludeTree::FileList::FileSizeTy Size) -> llvm::Error {
+        auto InsertPair = SeenContents.insert(
+            std::make_pair(File.getFilenameRef(), File.getContentsRef()));
+        if (!InsertPair.second) {
+          if (InsertPair.first->second != File.getContentsRef())
+            return diagnoseFileChange(File, InsertPair.first->second);
+          return llvm::Error::success();
+        }
+
         auto FilenameBlob = File.getFilename();
         if (!FilenameBlob)
           return FilenameBlob.takeError();
+
         SmallString<128> Filename(FilenameBlob->getData());
         // Strip './' in the filename to match the behaviour of ASTWriter; we
         // also strip './' in IncludeTreeFileSystem::getPath.

clang/lib/Tooling/DependencyScanning/IncludeTreeActionController.cpp

@@ -129,7 +129,8 @@ class IncludeTreeBuilder {
   // are recorded in the PCH, ordered by \p FileEntry::UID index.
   SmallVector<StringRef> PreIncludedFileNames;
   llvm::BitVector SeenIncludeFiles;
-  llvm::SetVector<cas::IncludeTree::FileList::FileEntry> IncludedFiles;
+  SmallVector<cas::IncludeTree::FileList::FileEntry> IncludedFiles;
+  SmallVector<cas::ObjectRef> IncludedFileLists;
   Optional<cas::ObjectRef> PredefinesBufferRef;
   Optional<cas::ObjectRef> ModuleIncludesBufferRef;
   Optional<cas::ObjectRef> ModuleMapRef;
@@ -454,9 +455,23 @@ void IncludeTreeBuilder::handleHasIncludeCheck(Preprocessor &PP, bool Result) {
   IncludeStack.back().HasIncludeChecks.push_back(Result);
 }
 
+// FIXME: duplicates code in PPDirectives
+static bool isForModuleBuilding(const Module *M, StringRef CurrentModule,
+                                StringRef ModuleName) {
+  StringRef TopLevelName = M->getTopLevelModuleName();
+
+  // When building framework Foo, we wanna make sure that Foo *and* Foo_Private
+  // are textually included and no modules are built for both.
+  if (M->getTopLevelModule()->IsFramework && CurrentModule == ModuleName &&
+      !CurrentModule.endswith("_Private") && TopLevelName.endswith("_Private"))
+    TopLevelName = TopLevelName.drop_back(8);
+
+  return TopLevelName == CurrentModule;
+}
+
 void IncludeTreeBuilder::moduleImport(Preprocessor &PP, const Module *M,
                                       SourceLocation EndLoc) {
-  bool VisibilityOnly = M->isForBuilding(PP.getLangOpts());
+  bool VisibilityOnly = isForModuleBuilding(M, PP.getLangOpts().CurrentModule, PP.getLangOpts().ModuleName);
   auto Import = check(cas::IncludeTree::ModuleImport::create(
       DB, M->getFullModuleName(), VisibilityOnly));
   if (!Import)
@@ -655,7 +670,7 @@ IncludeTreeBuilder::finishIncludeTree(CompilerInstance &ScanInstance,
   }
 
   auto FileList =
-      cas::IncludeTree::FileList::create(DB, IncludedFiles.getArrayRef());
+      cas::IncludeTree::FileList::create(DB, IncludedFiles, IncludedFileLists);
   if (!FileList)
     return FileList.takeError();
 
@@ -681,16 +696,8 @@ Error IncludeTreeBuilder::addModuleInputs(ASTReader &Reader) {
     Optional<cas::IncludeTreeRoot> Root;
     if (Error E = cas::IncludeTreeRoot::get(DB, *Ref).moveInto(Root))
       return E;
-    Optional<cas::IncludeTree::FileList> Files;
-    if (Error E = Root->getFileList().moveInto(Files))
-      return E;
 
-    Error E = Files->forEachFile([&](auto IF, auto Size) -> Error {
-      IncludedFiles.insert({IF.getRef(), Size});
-      return Error::success();
-    });
-    if (E)
-      return E;
+    IncludedFileLists.push_back(Root->getFileListRef());
   }
 
   return Error::success();
@@ -771,7 +778,7 @@ IncludeTreeBuilder::addToFileList(FileManager &FM, const FileEntry *FE) {
     auto FileNode = createIncludeFile(Filename, **CASContents);
     if (!FileNode)
       return FileNode.takeError();
-    IncludedFiles.insert(
+    IncludedFiles.push_back(
         {FileNode->getRef(),
          static_cast<cas::IncludeTree::FileList::FileSizeTy>(FE->getSize())});
     return FileNode->getRef();

clang/unittests/CAS/IncludeTreeTest.cpp

@@ -4,6 +4,7 @@
 #include "llvm/CAS/CASProvidingFileSystem.h"
 #include "llvm/CAS/CachingOnDiskFileSystem.h"
 #include "llvm/CAS/ObjectStore.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 
@@ -135,29 +136,74 @@ TEST(IncludeTree, IncludeTreeScan) {
 
   Optional<IncludeTree::FileList> FileList;
   ASSERT_THAT_ERROR(Root->getFileList().moveInto(FileList), llvm::Succeeded());
-  ASSERT_EQ(FileList->getNumFiles(), size_t(4));
-  {
-    Optional<IncludeTree::File> File;
-    ASSERT_THAT_ERROR(FileList->getFile(0).moveInto(File), llvm::Succeeded());
-    EXPECT_EQ(File->getRef(), MainFile->getRef());
-    EXPECT_EQ(FileList->getFileSize(0), MainContents.size());
-  }
-  {
-    Optional<IncludeTree::File> File;
-    ASSERT_THAT_ERROR(FileList->getFile(1).moveInto(File), llvm::Succeeded());
-    EXPECT_EQ(File->getRef(), A1File->getRef());
-    EXPECT_EQ(FileList->getFileSize(1), A1Contents.size());
-  }
-  {
-    Optional<IncludeTree::File> File;
-    ASSERT_THAT_ERROR(FileList->getFile(2).moveInto(File), llvm::Succeeded());
-    EXPECT_EQ(File->getRef(), B1File->getRef());
-    EXPECT_EQ(FileList->getFileSize(2), IncludeTree::FileList::FileSizeTy(0));
-  }
-  {
+
+  SmallVector<std::pair<IncludeTree::File, IncludeTree::FileList::FileSizeTy>>
+      Files;
+  ASSERT_THAT_ERROR(FileList->forEachFile([&](auto F, auto S) -> llvm::Error {
+    Files.push_back({F, S});
+    return llvm::Error::success();
+  }),
+                    llvm::Succeeded());
+
+  ASSERT_EQ(Files.size(), size_t(4));
+  EXPECT_EQ(Files[0].first.getRef(), MainFile->getRef());
+  EXPECT_EQ(Files[0].second, MainContents.size());
+  EXPECT_EQ(Files[1].first.getRef(), A1File->getRef());
+  EXPECT_EQ(Files[1].second, A1Contents.size());
+  EXPECT_EQ(Files[2].first.getRef(), B1File->getRef());
+  EXPECT_EQ(Files[2].second, IncludeTree::FileList::FileSizeTy(0));
+  EXPECT_EQ(Files[3].first.getRef(), SysFile->getRef());
+  EXPECT_EQ(Files[3].second, IncludeTree::FileList::FileSizeTy(0));
+}
+
+TEST(IncludeTree, IncludeTreeFileList) {
+  std::shared_ptr<ObjectStore> DB = llvm::cas::createInMemoryCAS();
+  SmallVector<IncludeTree::File> Files;
+  for (unsigned I = 0; I < 10; ++I) {
     Optional<IncludeTree::File> File;
-    ASSERT_THAT_ERROR(FileList->getFile(3).moveInto(File), llvm::Succeeded());
-    EXPECT_EQ(File->getRef(), SysFile->getRef());
-    EXPECT_EQ(FileList->getFileSize(3), IncludeTree::FileList::FileSizeTy(0));
+    std::string Path = "/file" + std::to_string(I);
+    static constexpr StringRef Bytes = "123456789";
+    Optional<ObjectRef> Content;
+    ASSERT_THAT_ERROR(
+        DB->storeFromString({}, Bytes.substr(0, I)).moveInto(Content),
+        llvm::Succeeded());
+    ASSERT_THAT_ERROR(
+        IncludeTree::File::create(*DB, Path, *Content).moveInto(File),
+        llvm::Succeeded());
+    Files.push_back(std::move(*File));
   }
+
+  auto MakeFileList = [&](unsigned Begin, unsigned End,
+                          ArrayRef<ObjectRef> Lists) {
+    SmallVector<IncludeTree::FileList::FileEntry> Entries;
+    for (; Begin != End; ++Begin)
+      Entries.push_back({Files[Begin].getRef(), Begin});
+    return IncludeTree::FileList::create(*DB, Entries, Lists);
+  };
+
+  Optional<IncludeTree::FileList> L89, L7, L29, L;
+  ASSERT_THAT_ERROR(MakeFileList(8, 10, {}).moveInto(L89), llvm::Succeeded());
+  EXPECT_EQ(L89->getNumReferences(), 2u);
+  ASSERT_THAT_ERROR(MakeFileList(7, 8, {}).moveInto(L7), llvm::Succeeded());
+  EXPECT_EQ(L7->getNumReferences(), 1u);
+  ASSERT_THAT_ERROR(
+      MakeFileList(2, 7, {L7->getRef(), L89->getRef()}).moveInto(L29),
+      llvm::Succeeded());
+  EXPECT_EQ(L29->getNumReferences(), 7u); // 2,3,4,5,6, {7}, {8, 9}
+  ASSERT_THAT_ERROR(MakeFileList(0, 2, {L29->getRef()}).moveInto(L),
+                    llvm::Succeeded());
+  EXPECT_EQ(L->getNumReferences(), 3u); // 0, 1, {2, ...}
+
+  size_t I = 0;
+  ASSERT_THAT_ERROR(
+      L->forEachFile([&](IncludeTree::File F, auto Size) -> llvm::Error {
+        EXPECT_EQ(F.getFilenameRef(), Files[I].getFilenameRef())
+            << "filename mismatch at " << I;
+        EXPECT_EQ(F.getContentsRef(), Files[I].getContentsRef())
+            << "contents mismatch at " << I;
+        EXPECT_EQ(Size, I) << "size mismatch at " << I;
+        I += 1;
+        return llvm::Error::success();
+      }),
+      llvm::Succeeded());
 }